Refactor review-page: first-party reviewed state and decomposition into actions, DTOs, traits, and a JS module#101
Merged
Conversation
…ed island Moves the reviewed-progress counter and meter out of the Alpine reviewedCount mirror into a 'reviewed-summary' Livewire island. A mark/un-mark now re-renders just the island via renderIsland() on the latency path (hide-reviewed mode still does a full render since the toggle changes the visible file list). The counter and unmark paths share one settleReviewedRender() helper. The @island lives in the page (Livewire) view and reaches the status strip through a $reviewedSummary slot, since the directive needs the Livewire view's scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The toggle's visibility moves off the Alpine reviewedCount mirror onto a same-named 'reviewed-summary' island, so renderIsland() flips it in step with the counter on a mark/un-mark. Removes the now-unused reviewedCount Alpine getter and adds browser coverage for the toggle appearing on the first mark. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A full re-render (hide-reviewed mode, filtering) emits only a skip marker for islands, so the reviewed-summary counter went stale when a file was marked while hidden. Declaring the island always:true re-renders it inline on full renders, while renderIsland() still scopes the skip-render latency path. Replaces a flaky browser probe (the lazy checkbox + hide re-render raced, so it never exercised hide mode) with a deterministic Livewire feature test asserting the counter reads 2/6 after a second mark in hide-reviewed mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e the Alpine mirror
The sidebar file list is now a file-list island (always:true). Its per-file reviewed button renders its state server-side instead of reading the page-root reviewedFiles Alpine object, and a mark/un-mark refreshes the island via renderIsland('file-list'). With the counter, Hide-reviewed toggle, and sidebar all server-owned, the page-root reviewedFiles mirror and its three sync handlers are removed. The file-reviewed-changed event is kept so DiffFile's own checkbox still flips in lockstep.
Adds browser coverage that a mark flips the sidebar button to its un-mark state through the island, alongside the existing active-highlight-survives-morph guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Retiring the Alpine mirror removed the only consumer of ReviewState::reviewedFileMap (the page-root seed). The id-keyed map is fully derivable from reviewedFileIds, so it is dropped from the DTO, the service builder, and toArray, mirroring the earlier visibleFileMap removal. The toArray key guard now also asserts reviewedFileMap stays gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e-root mirror file-reviewed-changed and reviewed-files-reverted are no longer received by a ReviewPage Alpine @window handler (those were removed with the mirror). Both now land only on DiffFile to flip its own checkbox flag; the sidebar and counter refresh through their islands. Updates the resources/CLAUDE.md event schema and the unmarkReviewed comment to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The sidebar reviewed button bakes its optimistic toggle direction into a server-rendered literal at island-render time. The un-mark branch already emits reviewed-files-reverted, but the mark branch dispatched nothing to the diff-file. So a rapid sidebar double-click on an already-reviewed file could leave DiffFile's `reviewed` flag stuck false while server, island, and the counter all showed reviewed. Emit file-reviewed-changed from the mark branch, symmetric with the un-mark revert, so DiffFile converges to the server state regardless of what the optimistic dispatch carried. No-op on the happy paths. Cover with a unit test (authoritative mark broadcast) and a browser test (slot-scoped counter island refreshes on a second normal-mode mark). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The class-header comment claimed five responsibility clusters but the component has six; Branch Divergence was missing. Update the map. Fold the duplicated "refresh file list, re-derive session state, inject orphans" sequence shared by the external-paths and global-gitignore handlers into reloadSessionAfterFileListChange(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ions Move two clusters of orchestration out of the review-page SFC into the app layer, leaving the component as the event/render coordinator. Divergence: resolveDivergenceState's decision tree becomes the pure ResolveDivergenceStateAction, returning a DivergenceDecision DTO (Noop / Aligned / AutoFollow / Show). The comment lookups arrive as closures so the common aligned path makes no DB query. Comment writes: addComment / updateComment / deleteComment delegate to ReviewCommentWorkflowAction, which returns a ReviewCommentMutation the page applies via applyCommentMutation(). It is an Action, not a service: the arch tests forbid Livewire from using Services and Services from touching Eloquent models, and the write path touches the Comment model. Dispatch and skipRender stay in the SFC so the 1+N hydration contract holds. Behavior is unchanged. Adds unit tests for both actions and corrects the pages/CLAUDE.md note that called the extraction a service. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Lift the ~250-line inline x-data (the page root and the external-change poller) into public/js/review-page.js as the reviewPage() and reviewChangePoller() factories, UMD-shaped like diff-file.js. The remote-link URL logic becomes the pure, unit-testable computeRemoteMenu(); the @browser keymap gate is passed in as a config flag; source/visible file entries stay dataset-backed getters. Adds Vitest coverage for the remote-link rules, the poller fingerprint handling, and the reset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…action
The page was a 2,307-line outlier, about 4x the next biggest component. It
stays one Livewire component (the 1+N hydration contract forbids nesting its
sections into children), so the extraction goes into the app layer:
- App\Concerns\ReviewPage\{ReviewsBranchDivergence, ManagesReviewTrash,
ExportsReview} hold the cohesive clusters with little render coupling. The
page keeps the lifecycle spine, the comment handlers, the undo() switchboard,
the reviewed-state render glue, and every computed.
- ReviewCommentWorkflowAction gains clearAll() and restore(), so the last
direct Comment writes leave the view file. The page applies the result via
applyCommentMutation(), matching the existing add/update/delete path.
- CountPersistedCommentsAction owns the divergence persisted-comment read,
keeping the cheap exists() gate alongside the count, because Livewire-layer
traits delegate model access to Actions.
- The DTO arch allowlist recognizes App\Concerns\ReviewPage as Livewire-layer
code: DTOs allowed, model access still Action-only.
Behavior-preserving. Pint, PHPStan, Pest, Vitest, and browser suites pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ials The Settings dropdown and Trash list were the two largest self-contained markup blocks. They move to resources/views/pages/partials/ behind @include, which shares the parent scope so wire: bindings and $wire keep resolving. The islands and toolbar stay inline because they carry the renderIsland targets the page re-renders selectively. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ScreenshotsUpdated for
|
|
Warning Review limit reached
More reviews will be available in 25 minutes and 43 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 Walkthrough
WalkthroughRefactors the review page into ChangesReview Page Refactor
Sequence Diagram(s)sequenceDiagram
participant Alpine as Alpine reviewPage / reviewChangePoller
participant Livewire as ReviewPage (Livewire)
participant WorkflowAction as ReviewCommentWorkflowAction
participant DivergenceAction as ResolveDivergenceStateAction
participant DB as Comment (Eloquent)
Alpine->>Livewire: createComment / deleteComment / clearAll / toggleReviewed
Livewire->>WorkflowAction: handle / delete / clearAll / restore
WorkflowAction->>DB: persist / delete / updateOrCreate
WorkflowAction-->>Livewire: ReviewCommentMutation
Livewire->>Livewire: applyCommentMutation → dispatch comment-updated, undo-available
Livewire->>DivergenceAction: checkHeadDivergence (if checksDivergence)
DivergenceAction-->>Livewire: DivergenceDecision
Livewire->>Livewire: settleReviewedRender → renderIsland OR skipRender
Livewire-->>Alpine: island re-renders reviewed-summary + file-list
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/Js/review-page.test.js (1)
124-202: ⚡ Quick winAdd lifecycle coverage for shortcut registration cleanup.
This suite covers polling/tooltip behavior well, but it doesn’t assert
createChangePollershortcut behavior acrossinit()/destroy()remount cycles. Add one test to verify no duplicate shortcut handlers after re-init.As per coding guidelines, scripts with non-trivial logic should include focused unit coverage for behavior likely to regress.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Js/review-page.test.js` around lines 124 - 202, Add a new test case to the createChangePoller test suite that verifies shortcut handler cleanup across init/destroy remount cycles. Create a poller instance, call its init() method to register shortcuts, call destroy() to clean up, then call init() again, and assert that shortcut handlers are not duplicated after the remount. This ensures that the destroy() method properly cleans up event listeners so that re-initializing the poller doesn't accumulate duplicate handlers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Concerns/ReviewPage/ReviewsBranchDivergence.php`:
- Around line 79-90: The dismissedAtBranch property is missing from the before
and after state snapshots used in change detection within the method that checks
for divergence changes. Add $this->dismissedAtBranch to both the $before and
$after arrays (alongside the existing $this->divergenceState,
$this->divergenceContext, $this->dismissedAtHead, and $this->projectBranch) so
that changes to the dismissed-at-branch state are properly detected and the
caller knows when to re-render the UI.
In `@public/js/review-page.js`:
- Around line 250-266: The init() method registers keymap shortcuts for ⌘R and
⌘⇧R, but the destroy() method does not unregister them, causing handlers to
accumulate on component remounts and trigger multiple refresh actions per
keypress. In the destroy() method, after the existing stopPoll cleanup, add
calls to unregister both keymap shortcuts using the same keymap.register API (or
an unregister method if available) to mirror the registrations done in init()
and prevent handler stacking across component lifecycle cycles.
In `@resources/views/pages/partials/review-trash-list.blade.php`:
- Around line 34-39: The permanentlyDeleteTrashed() method is a destructive
action that lacks integration with the central undo mechanism. Modify the
`@confirmed` event handler on the arm-commit-button component to dispatch the
permanent delete action through the central undo flow by using the
undo-available mechanism and ensuring a corresponding undo() case is added to
handle reversal of the deletion. This ensures that each permanent delete action
creates a single undo entry that can be reversed through the standard undo
interface, consistent with other destructive actions like hide and move.
- Around line 29-39: The restore and delete action controls are currently only
visible on hover, which prevents keyboard users from seeing these buttons when
they navigate via keyboard focus. Update the class attribute on both the restore
button (containing the arrow-uturn-left icon) and the x-arm-commit-button
component to include focus-based visibility classes in addition to the existing
hover classes. Add focus visibility utilities such as group-focus-within or
focus-visible variants to ensure these controls become visible when keyboard
users focus on them, providing parity with mouse users.
---
Nitpick comments:
In `@tests/Js/review-page.test.js`:
- Around line 124-202: Add a new test case to the createChangePoller test suite
that verifies shortcut handler cleanup across init/destroy remount cycles.
Create a poller instance, call its init() method to register shortcuts, call
destroy() to clean up, then call init() again, and assert that shortcut handlers
are not duplicated after the remount. This ensures that the destroy() method
properly cleans up event listeners so that re-initializing the poller doesn't
accumulate duplicate handlers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1612e3b-8fb3-41e9-b1f6-3b39afbfd404
📒 Files selected for processing (29)
app/Actions/CountPersistedCommentsAction.phpapp/Actions/ResolveDivergenceStateAction.phpapp/Actions/ReviewCommentWorkflowAction.phpapp/Concerns/ReviewPage/ExportsReview.phpapp/Concerns/ReviewPage/ManagesReviewTrash.phpapp/Concerns/ReviewPage/ReviewsBranchDivergence.phpapp/DTOs/DivergenceDecision.phpapp/DTOs/ReviewCommentMutation.phpapp/DTOs/ReviewState.phpapp/Enums/DivergenceDecisionKind.phpapp/Services/ReviewStateService.phppublic/js/review-page.jsresources/CLAUDE.mdresources/views/components/status-strip.blade.phpresources/views/pages/CLAUDE.mdresources/views/pages/partials/review-settings-dropdown.blade.phpresources/views/pages/partials/review-trash-list.blade.phpresources/views/pages/⚡review-page.blade.phptests/Arch/LayerDependenciesTest.phptests/Browser/FileInteractionTest.phptests/Browser/SessionPersistenceTest.phptests/Js/review-page.test.jstests/Unit/Actions/CountPersistedCommentsActionTest.phptests/Unit/Actions/DeriveReviewStateActionTest.phptests/Unit/Actions/ResolveDivergenceStateActionTest.phptests/Unit/Actions/ReviewCommentWorkflowActionTest.phptests/Unit/DTOs/ReviewStateTest.phptests/Unit/Livewire/ReviewPageReviewedUndoTest.phptests/Unit/Services/ReviewStateServiceTest.php
💤 Files with no reviewable changes (3)
- tests/Unit/Services/ReviewStateServiceTest.php
- app/Services/ReviewStateService.php
- app/DTOs/ReviewState.php
Comment on lines
+34
to
+39
| <x-arm-commit-button | ||
| icon="trash" | ||
| tooltip="Permanently delete" | ||
| @confirmed="$wire.permanentlyDeleteTrashed({{ $trashed['id'] }})" | ||
| class="opacity-0 group-hover:opacity-100 transition-opacity shrink-0" | ||
| /> |
There was a problem hiding this comment.
Add central undo coverage for permanent delete.
permanentlyDeleteTrashed(...) is destructive and currently appears irreversible from this flow. Please wire it into the central undo mechanism so one delete action maps to one undo entry.
Based on learnings, “Every destructive action (hide, delete, move) must offer undo via central mechanism.” As per coding guidelines, destructive actions should use the central undo flow (undo-available + ReviewPage::undo() case).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@resources/views/pages/partials/review-trash-list.blade.php` around lines 34 -
39, The permanentlyDeleteTrashed() method is a destructive action that lacks
integration with the central undo mechanism. Modify the `@confirmed` event handler
on the arm-commit-button component to dispatch the permanent delete action
through the central undo flow by using the undo-available mechanism and ensuring
a corresponding undo() case is added to handle reversal of the deletion. This
ensures that each permanent delete action creates a single undo entry that can
be reversed through the standard undo interface, consistent with other
destructive actions like hide and move.
Sources: Coding guidelines, Learnings
…ction refreshDivergenceState() compares before/after snapshots to decide whether a poll tick needs to render. dismissedAtBranch is divergence state (it suppresses the banner by branch identity) but was absent from both snapshots, so a change to it alone would go undetected. The dismiss paths also flip divergenceState, which masked the gap in practice, but the snapshot should be complete on its own terms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The change-poller registers the refresh shortcuts in init(), but the keymap store persists across Livewire navigation. Without a matching unregister in destroy(), a torn-down poller left the refresh combo bound to a dead component's softRefresh. Unregister both on teardown, and cover the register/unregister lifecycle in review-page.test.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The restore and permanent-delete controls were hover-only (opacity-0 with group-hover), so keyboard users could focus controls that stayed visually hidden. Add group-focus-within:opacity-100 so tabbing into a trash row reveals its controls, restoring keyboard parity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes:
Related:
Summary
Make the review page's reviewed-state server-owned (first-party), then decompose the page itself.
⚡review-page.blade.phpwas a ~2,900-line outlier holding state machines, comment writes, inline Alpine, and large markup blocks in one file. This PR relocates that logic into tested units while the page stays a single Livewire component.Why
The page renders N
diff-filechildren, so a parent re-render rehydrates them all (the 1+N hydration hazard). That rules out splitting its sections into nested components. The page can still shed weight by pushing pure decision/write logic into the app layer (Actions/DTOs), inline Alpine into a JS module, cohesive non-render clusters into traits, and large markup into partials, leaving a thin coordinator whose render contract reads in one place.Solution
Keep one Livewire component as the event/render coordinator. Move everything that does not need the render pipeline out of the view file, and keep
skipRender/renderIsland/dispatchglue on the coordinator.Changes
diff-fileconverges, retire the Alpine reviewed-file mirror and the deadreviewedFileMapprojection, and keep the counter island fresh on full renders.public/js/review-page.js(UMD, unit-tested with Vitest), including the pure remote-link menu logic.ResolveDivergenceStateAction+DivergenceDecisionDTO +DivergenceDecisionKindenum own the divergence decision tree; closures keep the aligned path query-free.ReviewCommentWorkflowActionhandles add/draft/update/delete/clearAll/restore and returns aReviewCommentMutationthe page applies throughapplyCommentMutation(). No directCommentwrites remain in the view file.App\Concerns\ReviewPage\{ReviewsBranchDivergence, ManagesReviewTrash, ExportsReview}hold the cohesive clusters;CountPersistedCommentsActionowns the divergence persisted-comment read so the Livewire-layer traits delegate model access to actions.resources/views/pages/partials/behind@include; islands and toolbar stay inline.App\Concerns\ReviewPageas Livewire-layer code (DTOs allowed, model access still action-only).⚡review-page.blade.phpis now 1,723 lines, with the extracted logic covered by new action/DTO/JS/browser tests.Testing
Behavior-preserving throughout: trait methods moved verbatim, comment clear/restore map onto the existing mutation tail. Reviewed by a 7-angle code review and a dedicated removed-behavior audit (no correctness findings).
Deployment
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements