Review: a global review destination, from queue to submitted PR

[jbachorik]

Rebuilds Review as a global destination that spans repositories, per the design handoff.

Reviewing a 75-file worktree: queue rail, intent rail, diff column with folded unchanged runs and ◆n pins, findings margin, and the verdict bar refusing approval while a blocking finding is open.

Narrowed past the collapse thresholds (§4.9)

Queue and intents fold to their icon strips; the findings margin and the verdict bar stay. Collapsing a rail never removes an action.

Review is no longer a per-session sub-tab. The old shape gated Review off for remote repositories — but reviewing a remote PR is the primary use case — and a per-session tab cannot show a queue that spans repos at all. OpenSessionTab.SubTab is now CLAUDE · TERMINAL · EXPLORER, and ⌘4 navigates.

Commit	Milestone
8d77f06	M1 — scope handles, queue assembly, sidebar destination, ⌘4 as navigation
9d6bbcc	M2 — virtualized diff column: hunk cards, gutter, syntax highlighting, folded runs, densities
29196c4	M3 — findings margin, threads, ◆n pins, verdict bar, submit
7831e94	M4 — the MCP review surface and the intent rail
6f32e22	M5 — the PR loop: checkout gate, read-patch-only, hand-off to Finish
71bdeb4	M6 — symbol lens and the MCP activity panel
50d1ac9–e5bfd4c	restart-stable scope handles, base-branch correctness, visual-verification fixes
a2a5739	merge of origin/main

Three things worth a close look

1. The handoff specifies a gh flag that does not exist. It gives the checkout as gh pr checkout <n> --worktree. There is no such flag (checked against gh 2.96), and gh pr checkout always operates on the working tree it runs in — so following it literally would move the reviewer's main checkout onto the PR branch mid-task. drydock instead does git worktree add --detach <dir> and then runs gh pr checkout <n> --branch pr-<n> inside that worktree. The gate prints that sequence rather than the handoff's, because the reader may run it themselves.

2. The MCP schema is deliberately narrowed in one place. §5 lists review_verdict / review_resolve / review_submit under "human-side writes". Exposing them as agent-callable tools would let an agent approve its own work and resolve its own findings, so they are the UI's actions against the store and agents observe them read-only through review_state. This is the one intentional departure from the schema's surface — flagged for a decision.

3. The agent step timeline is not built. It needs per-step line attribution and a survives-to-HEAD calculation; both are real work, and a plausible-looking approximation would be worse than nothing when the panel's entire value is answering "which of the agent's edits actually made it" correctly. promptHistory returns empty and t is unbound.

Bugs found by testing and by looking, rather than by reading

• Reply drafts were wiped whenever an agent touched the finding — M3's own done-when. Drafts now live in the margin keyed by (scopeId, id).
• ⋯ N unchanged could never appear. With git's default 3-line context no unchanged run is ever longer than the fold threshold, so the feature rendered correctly and never showed. Review asks for -U12.
• Persisted findings were orphaned on restart — scope handles were random per process; now HMAC-derived from scope identity under a persisted secret.
• A git switch in another terminal discarded every in-progress review. base was part of scope identity, so anything moving it re-derived every worktree's handle and its findings vanished from the UI while sitting untouched in the store. Identity is now a place, plus which PR it is; the base is the repository's default branch.
• The ◆n pins were never on screen. They rendered correctly, but the source column grew and pushed each pin to the right edge of the content — far outside the viewport on a wide diff.
• The verdict bar was dead on every freshly opened item. Intents are derived from the diff, which arrives asynchronously; the bar rendered once against an empty diff and never re-rendered, so Approve, Request change and Submit all did nothing. No existing test could reach it — every one supplied the diff synchronously.
• Diff rows were nearly twice their intended height. §4.8's "row padding" is the prototype's ipad, applied to the queue and intent cards, not to code lines.

Keying

Findings, verdicts, threads and drafts are keyed by (scopeId, id), never by id alone: finding ids are agent-chosen and repeat across scopes, so a reviewer re-run against two worktrees produces f_leak_1 in both. The regression test is the first test in AnnotationStoreTest. review_reply refuses an ambiguous id rather than guessing. Annotations written before scope handles existed are migrated rather than dropped.

Testing

924 tests, 0 failures, 0 skipped (from 828 before this branch).

This adds a headless JavaFX harness the repository did not have — TestFX + Monocle, so view tests show a real Stage, apply the real stylesheets and drive real key events with no window server, and run identically on the macos-14 CI runner. Mutation-checked to confirm it is not vacuous.

It also adds in-process visual verification: -Dapp.drydock.diag.screenshot=<path> renders the scene graph to a PNG on the FX thread, needing no screen-recording permission. The images above came from it. Three of the bugs listed were invisible to the test suite by construction, because they were about what reaches the screen.

Theme and font integration is enforced by ThemeTokenContractTest: both theme sheets define identical token names, app.css references no undefined token, and every font size is a px literal so the interface-size slider scales it.

Merge with main

a2a5739 merges origin/main. Five conflicts, plus one integration defect it surfaced: McpToolRouter signed every agent-written finding with the literal string "Claude" — precisely what main had just finished removing from the rest of the UI, and wrong for a Codex session. It now derives the name through main's own AgentLabels.

Review notes

docs/ui-redesign.md carries the new structure plus ~45 known deviations grouped by milestone, and flags the five open decisions in §10 of the handoff rather than inventing answers.

Not implemented, and flagged: MCP notifications (§7) need a server-initiated channel McpServer does not have — agents poll review_state; and the STACK queue group is modelled but never produced, since stacked PRs are open decision §10.4.

🤖 Generated with Claude Code

[jbachorik]

Fixed: a branch switch orphaned every in-progress review

Follow-up on the gh pr checkout discussion in the description. Tracing the consequence through the workflow turned up a live bug that had nothing to do with PR checkout — that was only one of its triggers.

base was part of scope identity, and the base was the main checkout's current branch. Findings and verdicts are keyed (scopeId, …), so anything that moved the base re-derived every worktree's handle. That worktree's review then vanished from the UI while sitting untouched in the store under the old handle — no error, nothing on screen to say so, just an empty margin.

git switch in another terminal was enough to do it.

What a reviewer would have experienced: part-way through feat/sidebar-resize with three findings and two approved intents, switch branches for something unrelated, come back — empty margin, renumbered intents. And the surviving line-key anchors resolve against a diff computed from a different base, so "this event filter leaks" ends up pinned to whatever now sits at that line, still looking authoritative.

Two fixes, for the two halves

• Scope identity is a place, plus which PR it is — kind, repo root, worktree, PR number. Base and head are properties of a review, not what makes it that review; a commit inside a worktree moves its head, and that is not a different review.
• The base is the repository's default branch (origin/HEAD, then conventional names, then the current branch as a last resort), so the diff stops moving too.

Found on the way

GitStatusService.runLines throws on any non-zero exit, and symbolic-ref --quiet refs/remotes/origin/HEAD exits 1 whenever origin/HEAD is absent — normal for a local-only repository. That turned a routine "no default branch" into a failed scan, which also flipped the scan-completeness flag and silently disabled scope revocation. Probes where "not there" is a legitimate answer now use a variant that tolerates it.

Verification

Four regression tests, mutation-checked: putting base/head back into the identity fails the two registry tests, restoring the current-branch base fails the two queue tests, and nothing else fails either way.

911 tests, 0 failures. Verified in the app — 5 queue items, no exceptions.

c8e7d3d

[jbachorik]

Checked: the base change did not break the other items' diffs — but it had introduced a different regression

Verified by running the real diff for every queue item, not by asserting on the base string. Against this repository:

item: Working tree      [base=main] 7 files
item: feat/review_ui    [base=main] 73 files
item: feat/ssh_ttl      [base=main] 8 files
item: fix/ui_bugs       [base=main] 15 files
item: fix/release_action[base=main] FAILED: Not a Git repository: …/olifer-release-action

Four of five resolve correctly. The fifth is unrelated and pre-existing: that worktree's .git file points into a different repository's admin directory (/Users/…/src/olifer/.git/worktrees/…) which no longer exists, so git -C there fails before any base is considered. It failed identically before this branch. Worth fixing separately — say the word — but it is not fallout from this change.

The regression the check did find

origin/HEAD names a remote default. git clone -b feat/x, and deleting a local main after a merge, both leave origin/HEAD → origin/main with no local branch of that name. Handing the bare name to the diff then fails every worktree item:

fatal: ambiguous argument 'main...HEAD': unknown revision or path not in the working tree

The old behaviour couldn't hit this, because the main checkout's current branch always resolves. I reproduced it against a real clone before fixing it.

defaultBranch now returns the local branch when there is one and the remote-tracking ref (origin/main) otherwise; the conventional-name fallbacks are only offered when the ref exists.

Why the earlier tests missed it

They asserted on the base string (assertEquals("main", …)), which is exactly the wrong altitude — a base is only useful if git can resolve it. The new tests run the actual diff for every assembled item, and one builds the precise repository shape that breaks. Mutation-checked: removing the resolvability check fails the new test and nothing else.

919 tests, 0 failures.

d40716d

[jbachorik]

Visual verification, and two bugs only looking could find

macOS screen capture is unavailable in this environment (could not create image from display), so the FX layer had no visual evidence at all — every check so far proved data flowed, not that the UI was right. Added -Dapp.drydock.diag.screenshot=<path>, which renders the scene graph to a PNG from inside the process on the FX thread. No OS permission needed, and it cannot accidentally photograph another window. Uses java.desktop (already on the jlink module list) rather than javafx.swing (which is not).

Looking at the first image found two bugs in about ten seconds.

The verdict bar was dead on every freshly opened item

It showed no intent and 0/0 intents settled — so Approve, Request change and Submit did nothing at all.

Intents fall back to one-per-file, so they are derived from the diff — and the diff arrives on a background thread. The bar rendered once against an empty diff, correctly concluded there were no intents, and never re-rendered.

No existing test could have caught it: every one of them handed the view a diff synchronously. ReviewIntentFallbackTest now drives the real asynchronous path against a real repository, and fails without the fix.

Diff rows were almost twice their intended height

~36px where the design wants 20px. §4.8's "row padding 8 / 6 / 4px" is the prototype's ipad, and checking the prototype source shows it is applied to the queue and intent cards — code line height there is font-size × line-height. I had applied it to both.

Before → after, same window: rows went 33px → 20px (= 12.5 × 1.6, exactly the spec), and the bar went from no intent to intent 1 · Approve intent · Request change · Ask the agent to fix it · 0/6 intents settled · Submit (6 left).

Also

A latent flake this surfaced: aScopeWithNoChangesSaysSo waited for any Message row, and "Diffing…" is one — so it raced the placeholder instead of the result. It passed by timing luck until the machine was loaded by the full suite.

920 tests, 0 failures.

29f9422

[jbachorik]

Every rail expanded — and three more bugs only the picture showed

Photographed at 1800×1000 with representative findings seeded, so the intent rail and findings margin are both expanded and populated. Two new diagnostic flags make that possible: -Dapp.drydock.diag.windowSize= (the rails collapse at documented widths) and -Dapp.drydock.diag.seedFindings=true (the margin's pills, patch blocks and ASK chips cannot be seen without findings to render).

What it confirms — first time any of this has actually been seen

• Queue rail — MINE group header, ❯_ icon with its open-finding count beneath, mono title, dim subtitle, accent border on the selected item
• Intent rail — INTENTS 0/5 · i, numbered cards, by-file fallback rationale, change kind tag, risk heat bars
• Findings margin — FINDINGS 4 threads · 3 open, all/open filters, severity pills (blocking red / question amber / deviation purple), matching coloured ◆1 ◆2 ◆3, the proposed patch block, ASK chips, per-card reply boxes, Apply patch accented
• The open filter correctly hides the resolved nit — 4 threads, 3 open
• Verdict bar — intent 1 · Approve intent · Request change · Ask the agent to fix it · ⚠ a blocking finding is still open, with 0/5 intents settled and Submit (5 left)

The three bugs

The ◆n pins were never on screen. They rendered correctly the whole time. The source column had Hgrow, so it filled the row and pushed each pin to the right edge of the content — which on a diff whose widest line is hundreds of characters is far outside the viewport. They now sit immediately after the code. The design right-aligns them to the card edge, which a virtualized row as wide as the longest line in the whole diff cannot do; recorded as a deviation.

A filtered-out pin rendered as ◆0. The numbers are the margin's render order, so a finding it is not rendering has no number — 0 was invented. It keeps a bare dimmed diamond now: the line still carries a finding, and a made-up number is a lie.

Pins had no background, so they sat directly on the add/delete row tint and were hard to pick out. They now carry the code background and a border.

920 tests, 0 failures.

e5bfd4c