Refactor branch divergence UX: quiet markers + undoable switches#95
Open
fgilio wants to merge 20 commits into
Open
Refactor branch divergence UX: quiet markers + undoable switches#95fgilio wants to merge 20 commits into
fgilio wants to merge 20 commits into
Conversation
The full-width stock-Flux warning callout clipped its primary button off the right window edge (unbounded branch names in `inline` callout actions) and was the only off-palette surface in the app. Replace it: - Diverged/Detached (recoverable): a quiet amber/info marker + popover on the branch control via x-divergence.marker. No full-width band, no content reflow — the canvas stays calm. The popover names the stakes (N comments on the target) and keeps branch names out of button labels. - MissingTarget (genuinely blocking): an on-brand gh-* bar (x-divergence.missing-bar) instead of a stock-danger callout, now dismissible rather than forcing an un-undoable switch. - Empty state is branch-aware: when the review is empty because the checkout drifted, it explains why instead of a generic "clean" message. Behavioral hardening: - "Switch review" is now undoable through the central undo stack (Cmd+Z). - Divergence suppresses per branch identity, not per sha, so committing on the diverged branch no longer re-nags on every commit. data-testids preserved; tests cover undo, per-branch suppression, the missing-target dismiss, the comment count, and per-state rendering. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
The monochrome-brutalist system had no enforcement, so stock Tailwind/Flux colors had accreted (the divergence callout was just the loudest case). - Alias Flux's --color-accent → gh-accent in app.css, so variant="primary" buttons, checkboxes and focus rings render the brand near-black/near-white instead of Flux's stock zinc, and track the theme. - Replace stray utilities with gh-* tokens: review-pair "R" (purple→link), trash/restore + delete hovers (green-400/red-400/red-500 → gh-*), git-error glyphs (red-400 → gh-red). - Replace stock-color Flux badges (worktree yellow, untracked zinc, image before/after red/green) with on-brand tokened labels. - Fix the undefined `text-gh-fg` token on the first-run hero → gh-text. - Drop color="green" from the comment Save button (now accent). Add two arch guards in BladeConventionsTest: no stock Tailwind palette utilities and no stock Flux color="…" props in blade. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
Four different empty-state visual languages collapse into one: - Add <x-empty-state>: rfa-wordmark watermark (or a Flux icon), display heading, body slot, optional actions; role/aria-live pass through for alert-grade states. One recipe, used everywhere. - Migrate review-page (git-error, no-commit-changes, diverged, clean-tree) and context-page (no context files) onto it. Headings now use an <h2> with explicit font-display, fixing the div-as-heading and weight drift. - Add <x-diff-skeleton>: a content-shaped placeholder (line-number + content bars in the diff rhythm) so lazy-loaded diffs don't pop in from a bare spinner. animate-pulse is collapsed by the global reduce-motion rule. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
… confirm() - Replace the native browser confirm() on submit-with-drafts (an unthemeable OS modal in a brutalist app) with an inline arm-to-confirm: the primary button becomes "Submit without N drafts?" on first click, submits on the second, disarms on click-outside. Timer-free, matches the app's arm-commit-button vocabulary. - Give Flux toasts a single defined home (bottom-right) via <flux:toast>, clear of the undo-toast and submit bar. - Override the Flux toast surface onto gh-surface/gh-border so it matches the hand-built undo-toast instead of stock white/zinc. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
- Strengthen the change-marker stripe (add-line/del-line) from ~0.25/0.30 to
0.60/0.65 alpha and nudge row fills to 0.10/0.12, so additions and
deletions segment pre-attentively while syntax highlighting still reads
through the light row fill. (The faint stripe was the diff viewer's biggest
gold-standard gap.)
- File-header action row: lift the copy/copy-content/discard cluster from a
near-invisible 30% rest opacity to 60% (the destructive discard especially
shouldn't hide), still revealing fully on hover.
- Fix the copy-content trigger: ellipsis-vertical ("more") glyph →
clipboard-document, with a tooltip, so the highest-frequency copy action
reads as copy, not overflow.
Follow-ups noted (need visual iteration): comment-thread open motion,
unifying the three expand-control styles, reconciling syntax-red vs del-red.
https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
Sidebar (changed-files list): - Distinguish status hues — modified (M, attention) vs renamed (R, link) no longer share amber — from a single source of truth (kills the dead stock-color tuple that re-derived color from the letter). - Selected-file row goes monochrome (bg-gh-text/10), matching project-list and branch-explorer instead of a one-off blue wash. - Add a focus-visible ring on rows for keyboard navigation. - Show a "No files match …" state so an active filter never leaves a void. - Opt-in <x-file-path :collapse> middle-ellipsizes deep directories (app/…/Jobs/) so the basename survives in the narrow list; full path stays in title. Default off everywhere else. Context tree: - Collapse single-child folder chains into one breadcrumb row (app › Console › Commands › Pla › Db → app/Console/Commands/Pla/Db), like VS Code, so indentation tracks real branching, not depth. - Drop the dead "Missing only — coming soon" disabled filter option. Tests cover the tree collapse (chain + branch) and the file-path collapse. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
Chrome: - Nav text uses the system tracking-brutal token instead of stock tracking-tight (mode-toggle tabs, branch pill name). - Status-strip review count drops its lone bordered chip for plain muted text, matching the flat +/- stats beside it. - Align header left-cluster gap between review and context pages (gap-2) and add min-w-0 so long project names truncate instead of shoving the toolbar. Motion (toward the 150-250ms house rule): - Find-in-page bar now eases in/out (fade + slight slide) instead of snapping — it was the literal instant-show anti-pattern. - Both right-click context menus go from 75ms opacity-only to a 150ms ease-out scale+fade enter with a faster ease-in leave. - Update-banner progress bar drops from duration-500 to 300 (under the 400ms ceiling). https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
Replace the ad-hoc expand bands with a single <x-diff.expand-control> shell that owns the ↕ icon, the muted "Show" verb, the dashed rule, and the loading swap. The slot carries only the blue (clickable) targets, so every control reads "↕ Show <target>" under one rule: blue means clickable. - "Show" is the single verb for both "full file" and "N hidden lines" (reads naturally where "Expand full file" does not) - Drop the @@ -a +b @@ plumbing from the section-context band; render just the muted context header - tiered-expand-gap renders bare blue targets (chips + "N hidden lines"); the verb now lives in the shell - full-file button keeps aria-label="Show full file" for screen readers https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
The inline and file comment forms mount via x-if, so they appeared instantly. Wrap each in a .comment-open container that, on mount, plays a 150ms height-collapse (grid-template-rows 0fr -> 1fr) plus an opacity fade, so the form unfolds and pushes the rows below down. Pure CSS, no new Alpine scope, so the $refs.commentInput autofocus and the formBody/handleEscape contract in diff-file.js stay intact. x-if stays (one mounted form, not one hidden form per line), preserving the large-diff perf model. Open-only by design: x-if removes the node on close. Reduced-motion is handled by the global rule in app.css. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
…ntract Quality pass over the expand-control work; no behavior change. - expand-control: drop the `verb` and `loadingLabel` props. Neither was ever overridden by a caller, so "Show" and "Loading…" are now inlined — removing two speculative knobs and the dead `$verb !== ''` guard. - Add <x-diff.expand-button>. Every expander button repeated the same wire:click / wire:target / @click="loading = true; markDiffActionStart()" block, naming its Livewire action three times over. The component names the action once; the four call sites (three in tiered-expand-gap, the master "full file" button in diff-file) become declarative, with per-button classes merged in. Removes the cross-file drift risk on the wire contract. Verified green: DiffFileTest (28), ExpandContextTest (2), Pint. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors diff expand/collapse controls into reusable components, introduces branch-based divergence suppression and handlers in the review page, adds shared UI components (empty-state, diff-skeleton, divergence marker/missing-bar), updates theme/CSS tokens and animations, and extends/adjusts tests and JS to support focus restoration for expanders. ChangesMain Feature Cohort
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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. 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 |
ScreenshotsUpdated for
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55f4c68e40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/Livewire/DiffFileTest.php (1)
358-362:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the hidden-line count assertion contiguous.
These checks got much looser: a standalone
'20'or'24'can match unrelated markup, so the test no longer proves the label text changed correctly. Assert the full phrase instead, e.g.20 hidden lines/24 hidden lines.Suggested tightening
- expect($html)->toContain('expandGap(1, 15)') - ->and($html)->toContain('expandGap(1)') - ->and($html)->toContain('20') - ->and($html)->toContain('hidden lines'); + expect($html)->toContain('expandGap(1, 15)') + ->and($html)->toContain('expandGap(1)') + ->and($html)->toContain('20 hidden lines');- expect($html)->toContain('expandGap(0, 15)') - ->and($html)->toContain('expandGap(0)') - ->and($html)->toContain('24') - ->and($html)->toContain('hidden lines'); + expect($html)->toContain('expandGap(0, 15)') + ->and($html)->toContain('expandGap(0)') + ->and($html)->toContain('24 hidden lines');Also applies to: 385-389
🤖 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/Unit/Livewire/DiffFileTest.php` around lines 358 - 362, The assertions in the test use loose contains checks like ->toContain('20') and ->toContain('24') which can match unrelated markup; update the expectations that check hidden-line counts to assert the full contiguous phrase (e.g. change ->toContain('20') to ->toContain('20 hidden lines') and similarly ->toContain('24 hidden lines') for the other occurrence) while keeping the existing calls to expandGap(1, 15) and expandGap(1) intact so the test still verifies the tiered-target behavior.
🧹 Nitpick comments (1)
resources/views/components/divergence/marker.blade.php (1)
47-52: ⚡ Quick winMatch the popover exit motion to the repo contract.
The new popover leaves with
ease-in duration-100, but this repo’s motion rules require departures to useease-out, and ordinary state changes should stay in the 150–250ms range.Suggested tweak
- x-transition:leave="transition ease-in duration-100" + x-transition:leave="transition ease-out duration-150"As per coding guidelines, "
resources/**/*.{blade.php,css,scss}: Use transition durations of 150–250ms for ordinary state changes" and "Useease-outeasing for departing elements."🤖 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/components/divergence/marker.blade.php` around lines 47 - 52, The popover's leave transition uses the wrong easing and duration; update the x-transition:leave attribute from "transition ease-in duration-100" to use "ease-out" and a 150–250ms duration (e.g., "transition ease-out duration-150" or "duration-200"), keeping the existing x-transition:leave-start and x-transition:leave-end values the same so departure matches the repo's motion contract; modify the x-transition:leave string in the marker component (the x-transition:leave attribute in resources/views/components/divergence/marker.blade.php).
🤖 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 `@resources/views/components/diff/expand-control.blade.php`:
- Around line 35-42: The current template hides the clicked trigger (the span
that renders {{ $slot }}) using x-show="!loading", causing the element to be
removed from the DOM and focus to be lost when loading=true; instead keep the
trigger mounted and only visually hide it while showing the spinner. Replace the
x-show="!loading" on the span that contains {{ $slot }} with a non-removing
approach (e.g., x-bind:class="{ 'invisible pointer-events-none': loading }" or
add a conditional aria-hidden attribute) so the element stays in the DOM and
retains focus; keep the existing flux:icon icon="arrow-path" x-show="loading"
x-cloak spinner as-is to show during loading. Ensure any x-cloak usage remains
on elements you truly want removed from view immediately, and verify keyboard
focus is preserved when loading toggles.
In `@resources/views/components/remote-link-menu.blade.php`:
- Around line 53-58: The leave transition uses "ease-in" which violates the
motion guideline; update the x-transition:leave attribute in the
remote-link-menu.blade.php component (the element with x-transition:enter/leave
attributes) to use "transition ease-out duration-100" instead of "transition
ease-in duration-100" so departing popovers use ease-out easing; ensure
x-transition:leave-start and x-transition:leave-end remain unchanged.
In `@resources/views/layouts/app.blade.php`:
- Around line 238-243: The leave transition for the find bar uses
"x-transition:leave='transition ease-in duration-100'" which violates the
repository convention; update the x-transition:leave attribute in the snippet
(the attributes x-transition:enter, x-transition:enter-start,
x-transition:enter-end, x-transition:leave, x-transition:leave-start,
x-transition:leave-end) so that x-transition:leave uses "ease-out" instead of
"ease-in" (keep the same "transition" and "duration-100" parts) to conform to
the departing-elements easing guideline.
In `@resources/views/livewire/`⚡diff-file.blade.php:
- Around line 545-548: The Livewire key for the trailing-gap expand wrapper
currently uses only count($hunks) which can stay the same while
trailingHiddenCount changes, causing Livewire to reuse the node and preserve
Alpine's loading state; update the wire:key on <x-diff.expand-control> (the
wrapper around <x-tiered-expand-gap>) to include the trailingHiddenCount (e.g.
expand-gap-trailing-{{ count($hunks) }}-{{ $trailingHiddenCount }}) so the key
changes when trailingHiddenCount changes and Livewire will remount the node.
In `@resources/views/livewire/update-banner.blade.php`:
- Line 243: Replace the Tailwind transition duration on the progress bar by
changing the class string "h-full bg-gh-link rounded-full transition-all
duration-300 ease-out" to use a 150–250ms duration (e.g., "h-full bg-gh-link
rounded-full transition-all duration-200 ease-out") so the element's transition
conforms to the project's ordinary-state timing guidelines.
In `@tests/Unit/Livewire/ReviewPageBranchDivergenceTest.php`:
- Around line 345-347: The test currently only asserts the dispatched
'undo-available' event has the right type and partial payload; update the
assertion on the result of $component->call('switchReviewToHead') to verify the
full undo event contract (type:string, payload:self-contained, message:string,
optional ttl:int). Replace the simple assertDispatched(...) with an
assertDispatched('undo-available', fn($event) => /* check $event->type ===
'switch-branch' && $event->payload === ['fromBranch'=>'main'] &&
is_string($event->message) && ( !isset($event->ttl) || is_int($event->ttl) ) */)
so the test explicitly asserts the message value/shape (and ttl type if present)
for the 'switchReviewToHead' flow.
- Around line 357-370: Extend the test around dismissMissingTarget to verify
branch scoping: after calling dismissMissingTarget (and asserting
divergenceState is Aligned and dismissedAtBranch is 'feature-x'), call
bindFakeCurrentHeadAction again to simulate a different branch with
targetExists: false (e.g., branch 'feature-y') and then call checkHeadDivergence
and assert divergenceState is DivergenceState::MissingTarget; this ensures
dismissMissingTarget only suppresses the banner for the dismissed branch. Use
the existing helpers and properties (bindFakeCurrentHeadAction,
dismissMissingTarget, checkHeadDivergence, divergenceState, dismissedAtBranch)
to implement the additional assertions.
---
Outside diff comments:
In `@tests/Unit/Livewire/DiffFileTest.php`:
- Around line 358-362: The assertions in the test use loose contains checks like
->toContain('20') and ->toContain('24') which can match unrelated markup; update
the expectations that check hidden-line counts to assert the full contiguous
phrase (e.g. change ->toContain('20') to ->toContain('20 hidden lines') and
similarly ->toContain('24 hidden lines') for the other occurrence) while keeping
the existing calls to expandGap(1, 15) and expandGap(1) intact so the test still
verifies the tiered-target behavior.
---
Nitpick comments:
In `@resources/views/components/divergence/marker.blade.php`:
- Around line 47-52: The popover's leave transition uses the wrong easing and
duration; update the x-transition:leave attribute from "transition ease-in
duration-100" to use "ease-out" and a 150–250ms duration (e.g., "transition
ease-out duration-150" or "duration-200"), keeping the existing
x-transition:leave-start and x-transition:leave-end values the same so departure
matches the repo's motion contract; modify the x-transition:leave string in the
marker component (the x-transition:leave attribute in
resources/views/components/divergence/marker.blade.php).
🪄 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: 0e0f6c77-4ca9-4ee0-8637-80c32872a402
📒 Files selected for processing (35)
config/theme.phpresources/css/app.cssresources/views/components/comment-display.blade.phpresources/views/components/comment-form.blade.phpresources/views/components/diff-skeleton.blade.phpresources/views/components/diff/expand-button.blade.phpresources/views/components/diff/expand-control.blade.phpresources/views/components/diff/file-header.blade.phpresources/views/components/diff/hunk.blade.phpresources/views/components/diff/line.blade.phpresources/views/components/divergence/marker.blade.phpresources/views/components/divergence/missing-bar.blade.phpresources/views/components/empty-state.blade.phpresources/views/components/feedback-submit-bar.blade.phpresources/views/components/file-path.blade.phpresources/views/components/mode-toggle.blade.phpresources/views/components/project-list.blade.phpresources/views/components/remote-link-menu.blade.phpresources/views/components/status-strip.blade.phpresources/views/components/tiered-expand-gap.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/context-tree.blade.phpresources/views/livewire/partials/context-tree-node.blade.phpresources/views/livewire/update-banner.blade.phpresources/views/livewire/⚡branch-explorer.blade.phpresources/views/livewire/⚡diff-file.blade.phpresources/views/pages/⚡context-page.blade.phpresources/views/pages/⚡review-page.blade.phpresources/views/pages/⚡select-repo-page.blade.phptests/Arch/BladeConventionsTest.phptests/Browser/ExpandContextTest.phptests/Feature/View/FilePathComponentTest.phptests/Unit/Livewire/ContextTreeTest.phptests/Unit/Livewire/DiffFileTest.phptests/Unit/Livewire/ReviewPageBranchDivergenceTest.php
Undoing "Switch review here" calls restoreReviewBranch -> autoFollowToHead to re-point at the old target. autoFollowToHead unconditionally markAligned()s, which is correct for its "follow HEAD" callers — but on the undo path HEAD is still on the branch we switched away from, so the review is diverged again. The head poller only transitions on a HEAD identity change, so it never re-fires, leaving the divergence marker hidden indefinitely. Recompute divergence right after restoring the old target so the marker re-surfaces. Also tightens the branch-divergence tests: assert the full undo-available contract (incl. message) and the post-undo Diverged state, and add a branch-boundary case proving missing-target dismissal is scoped per branch rather than a global mute.
- Key the trailing-gap expand-control by $trailingHiddenCount too, matching the
per-gap keys. A partial trailing expand can leave count($hunks) unchanged
while the remaining count shrinks; without it Livewire reuses the node and
the Alpine `loading` flag sticks on "Loading…".
- Tighten DiffFileTest hidden-line assertions to the contiguous phrase
("20 hidden lines" / "24 hidden lines") so a stray "20"/"24" can't pass them.
- Use ease-out (not ease-in) for the find-bar, remote-link-menu, and divergence
marker leave transitions, per the motion contract for departing elements.
Duration stays 100ms (leave is intentionally faster than the 150ms enter).
Five browser tests were failing on the redesigned surfaces (none related to
the expander cleanup; all confirmed by local repro):
- Selection clear / branch-switch: getByText('selected') is a substring match
that also caught toasts, tripping Playwright strict mode. Wait on the unique
getByLabel('Clear selection') affordance instead.
- Sidebar active-file highlight: the active row is now bg-gh-text/10, not
text-gh-link (which now only marks the renamed-R badge). Assert the new
class and waitFor it so the synchronous count() can't race the click.
- Draft submit: the native confirm() was replaced by an inline arm-to-confirm
(first click warns "Submit without N draft?", second submits; submit is
disabled without a real comment). Drive the two clicks and wait for the
draft to sync before arming.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Browser/DraftCommentTest.php (1)
337-358:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix vacuous “canceling confirm dialog” test for inline draft submit
window.confirmis unused here: drafts use the inline arm-to-confirm flow (feedback-submit-bar.blade.phptogglesarmedand dismisses via@click.outside), so the current single “Submit review” click never exercises a real cancel/dismiss path— it just arms, making the assertions pass for the wrong reason. Also, the test doesn’t wait for the draft to sync (assertSee('1 draft')like the other tests), which can make the first click submit immediately ifdraftCountisn’t updated yet.
- Remove the
window.confirmoverride and rework the test to: wait for1 draft, click “Submit review” to see “Submit without 1 draft?”, then cancel by clicking outside the submit button (triggering@click.outside) and assert “Review submitted” is still absent (and “Stay here” remains).🤖 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/Browser/DraftCommentTest.php` around lines 337 - 358, The test currently overrides window.confirm and never exercises the inline arm-to-confirm flow used by feedback-submit-bar.blade.php (which toggles an armed state and relies on `@click.outside` to dismiss), so remove the window.confirm override; instead wait for the draft to sync by asserting the "1 draft" indicator (like other tests), click the "Submit review" button to arm and surface the "Submit without 1 draft?" prompt, then simulate cancelling by clicking outside the submit UI (e.g., click the page body or another safe element to trigger `@click.outside`), and finally assert that "Review submitted" is not visible and "Stay here" still appears. Ensure you reference the test name (canceling confirm dialog does not submit review), the "Submit review" control, the "1 draft" indicator, and the 'Stay here' / 'Review submitted' assertions when making the changes.
🤖 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.
Outside diff comments:
In `@tests/Browser/DraftCommentTest.php`:
- Around line 337-358: The test currently overrides window.confirm and never
exercises the inline arm-to-confirm flow used by feedback-submit-bar.blade.php
(which toggles an armed state and relies on `@click.outside` to dismiss), so
remove the window.confirm override; instead wait for the draft to sync by
asserting the "1 draft" indicator (like other tests), click the "Submit review"
button to arm and surface the "Submit without 1 draft?" prompt, then simulate
cancelling by clicking outside the submit UI (e.g., click the page body or
another safe element to trigger `@click.outside`), and finally assert that "Review
submitted" is not visible and "Stay here" still appears. Ensure you reference
the test name (canceling confirm dialog does not submit review), the "Submit
review" control, the "1 draft" indicator, and the 'Stay here' / 'Review
submitted' assertions when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 385bead1-5bb6-443b-be9d-dae58cc293c3
📒 Files selected for processing (11)
resources/views/components/divergence/marker.blade.phpresources/views/components/remote-link-menu.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/⚡diff-file.blade.phpresources/views/pages/⚡review-page.blade.phptests/Browser/DraftCommentTest.phptests/Browser/FileInteractionTest.phptests/Browser/SelectionBranchSwitchTest.phptests/Browser/SelectionMultiSelectTest.phptests/Unit/Livewire/DiffFileTest.phptests/Unit/Livewire/ReviewPageBranchDivergenceTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- resources/views/components/remote-link-menu.blade.php
- tests/Unit/Livewire/DiffFileTest.php
- resources/views/livewire/⚡diff-file.blade.php
- resources/views/layouts/app.blade.php
- resources/views/pages/⚡review-page.blade.php
"clicking line with existing draft re-opens it" was ~2/3 flaky: the re-open mousedown handler reads $wire.fileComments synchronously, but the server-rendered draft badge can lead that client snapshot, so the click opened a blank form the inputValue poll could never recover. Poll the DiffFile's Livewire fileComments directly until the draft is present before re-clicking, replacing the unreliable networkidle proxy. Also rework "canceling confirm dialog" into "canceling the armed draft submit": the native confirm() was replaced by the inline arm-to-confirm, so the window.confirm override was dead and the single click only armed (passing for the wrong reason). Now it waits for the draft to sync, arms, cancels via @click.outside, and asserts the review does not submit.
Keyboard-activating a gap expander dropped focus to <body>: the
post-expand re-render replaces the activated node, and the wire:key
remount that resets the loading flag means a keep-mounted trick can't
preserve it either.
Gap expanders now tag themselves with data-expand-gap="{hunkIndex}" and
arm a keyboard-only refocus intent on click (gated on click.detail === 0,
so mouse focus never jumps). On the existing rfa:diff-action-completed
signal, focus returns to the expander now occupying the same gap,
anchored by the stable hunk index. Fully closing a gap degrades
gracefully (nothing left to focus there).
Also bring the update banner's download progress bar into the 150-250ms
motion band (duration-300 -> duration-200).
https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
Replace the hand-rolled 5s activeElement poll with Playwright's native :focus auto-wait, and use visit() like the sibling tests. The post-expand refocus tick can lag a heavily parallel CI runner past a fixed 5s budget; the :focus locator auto-retries so the assertion tracks the real focus restoration rather than a deadline. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
The reload-trigger evaluate (Livewire.first().$refresh()) was the one unprotected browser call in this test: session-recovery.js can begin the silent reload before the evaluate's round-trip resolves, destroying the execution context mid-call. In isolation the evaluate wins the race, but under heavy --parallel load it loses intermittently — surfacing as "Execution context was destroyed" failures on CI browser shards. Wrap it in the same Throwable tolerance the test's sessionStorage reads already use; the navigation it kicks off is exactly what the beforeunload assertion proves ran. Validated: 0/5 failures under the accumulating- contention scenario that previously failed 2/3. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ee9f1671
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| wire:loading.attr="disabled" | ||
| wire:target="{{ $action }}" | ||
| @if($gapKey !== null) data-expand-gap="{{ $gapKey }}" @endif | ||
| @click="loading = true; markDiffActionStart('{{ $action }}'); armExpandRefocus($event, @js($gapKey))" |
There was a problem hiding this comment.
When a gap expander is clicked, this sets the parent loading flag immediately, but it only gets cleared by a DOM replacement or the completion event. If the underlying diff changes between render and click so expandGap() hits one of its early returns (for example the full-context reload has no hunks), no completion event is dispatched and the keyed row does not change, leaving the expander stuck on “Loading…” until the file/page is refreshed. Either clear loading on Livewire request completion/failure or dispatch the completion event on the early-return paths.
Useful? React with 👍 / 👎.
The gap expander sets a per-control `loading` Alpine flag optimistically on click, but its only reset was the post-expand Livewire morph replacing the row. When expandGap() hits an early return — diff evicted from cache between render and click, or the full-context reload finds no diff because the file changed underneath the cached hunks — nothing morphs and no completion event fired, leaving the expander stuck on "Loading…" until a refresh. Dispatch rfa:diff-action-completed on both early-return paths and have the expand-control shell clear its loading flag on that event instead of relying on the morph. Reusing the existing completion signal also pairs the runtime-diagnostics start mark (previously orphaned on these paths) and lets keyboard focus return to the unchanged expander. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
The heaviest scenarios (review-page-100-files ~180ms, review-page-50-files ~108ms) carry ~5-7ms of run-to-run median variance even on a quiet box with no code change. CI compares two independent passes (base snapshot vs head checkout), so the delta swings past 10ms on a contended runner purely from measurement/contention noise — tripping --min-absolute-ms=10 as a false regression. Raising the floor to 15ms clears that noise band while still failing on a genuine regression (>15ms is >8% on the ~180ms scenario). The 5% percentage threshold and all memory floors are unchanged. https://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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". |
…t-confirm Three fixes surfaced by a max-effort review of this branch's diff: - expand-control: the loading spinner cleared on the global rfa:diff-action-completed window event with no fileId filter, so on the multi-file review page one file's expand completing reverted a sibling file's in-flight spinner. Guard on the event's fileId (String()-coerced both sides, since the dispatch casts to string while the Alpine fileId may be numeric), mirroring the sibling @window handlers in diff-file. - file-path: collapse on an absolute path dropped the first real directory ('/a/b/c/' collapsed to '/…/c/') because explode produced an empty leading segment. Preserve a leading slash so the root segment survives. Latent today (current callers pass relative paths); fixed defensively. - feedback-submit-bar: the arm-to-confirm guard that replaced native confirm() left `armed` true after the drafts it was armed against were removed via a skipRender'd action, so a later submit could skip the confirm. Drop the stale arm with x-effect when draftCount hits 0. Tests: add a tiered-chip expandGap($hunkIndex, $tier) early-return settle test (the most-clicked path, previously only the no-arg shape was covered), an absolute-path collapse test, and update the expand-control assertion to the fileId-scoped listener string.
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
2 participants
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.
Summary
Redesigns the branch divergence experience to be less intrusive while remaining actionable. Diverged/Detached states now surface as quiet header markers instead of full-width banners, keeping the canvas calm. Missing target remains a blocking bar. Branch suppression now keys on identity (not SHA) so committing on a diverged branch doesn't re-nag. Branch switches become undoable.
Key Changes
Divergence State Handling
dismissedAtBranchproperty to suppress banners by branch identity rather than SHA, so new commits on the same branch don't re-trigger the warningresolveDivergenceState()to check branch identity first for Diverged and MissingTarget statescommentCountto divergence context for the marker popoverUI Refactor
<x-divergence.marker>component—a small dot on the branch control in the header with a popover, replacing the full-width warning band<x-divergence.missing-bar>for the blocking MissingTarget state (still full-width, but on-brand styling)Branch Switch Undo
UNDO_TYPE_SWITCH_BRANCHconstant and undo handlerrestoreReviewBranch()switchReviewToHead()now captures the previous branch and dispatches an undo event (only for Diverged state with a real target)keepReviewing()now suppresses by branch for Diverged/MissingTarget, falls back to SHA for DetachedComponent Refactoring
persistedCommentQuery()to centralize the comment query logicpersistedCommentCount()method for the divergence contextautoFollowToHead()to clear bothdismissedAtHeadanddismissedAtBranchDiff Expand Controls
<x-diff.expand-control>wrapper component to unify chrome (verb, icon, loading state) across all expand affordances<x-diff.expand-button>to centralize the wire contract for expand buttons<x-tiered-expand-gap>to use the new control wrapper<x-diff.hunk>to use the new expand-control componentSupporting Components & Styling
<x-empty-state>for shared full-page zero states<x-diff-skeleton>placeholder for diff loading statesapp.cssto usegh-surfacetokengh-accentfor on-brand primary emphasiscomment-openanimation for inline comment form revealContext Tree
app/Console/Commands/Pla/Db/→app/…/Jobs/) so indentation tracks real branching, not depthFile Path Component
collapseprop for middle-ellipsis breadcrumbs in narrow lists (sidebar)Tests
ReviewPageBranchDivergenceTestto verify branch-based suppression persists across new commitsContextTreeTestcases for folder chain collapsingFilePathComponentTestcases for collapse modegh-*token usage and Flux color prop restrictionsStyling & Polish
flux:badgewith custom<span>for image diff labels to stay on-brandmin-w-0for flex overflow handlingarmedstatehttps://claude.ai/code/session_01MjD6k8JXwGZjxXw1fajJ97
Summary by CodeRabbit
New Features
UI/Style Improvements
Tests