Review tab: see what this branch will actually ship#520
Merged
Conversation
Phase 2 of #514. The Review tab now toggles between "Local" (working tree vs HEAD — what the agent just touched that isn't committed yet) and "Branch" (working tree vs merge-base with origin/<defaultBranch> — what this branch will ship, the same answer GitHub's "Files changed" tab gives). Computed locally from git refs; no forge API, no GitHub dependency. Server: `git.status` and `git.diff` gain a `mode: "local" | "branch"` discriminator. Branch mode resolves `origin/<detectDefaultBranch>`, computes `merge-base HEAD <ref>`, and diffs against that SHA. Missing `origin/HEAD` surfaces as an actionable error ("Run: git fetch origin && git remote set-head origin --auto"), not silent degradation. Client: segmented toggle at the top of the tab, header label reflects the resolved base ("Changes vs origin/master"). Selection resets on mode change.
…client
Plain `throw new Error(...)` was sanitized by oRPC to a generic
"Internal server error" on the client, hiding the actionable
`git fetch origin && git remote set-head origin --auto` suggestion
resolveBase() tries to surface. Throw `ORPCError("PRECONDITION_FAILED",
{message})` instead — oRPC passes defined errors through with their
message intact.
Also fix the "Mode toggle defaults to Local" scenario: the test
terminal boots at `~` (not a git repo), so the toggle wasn't rendered
at all. Seed a throwaway git repo like the other Review-tab scenarios
already do.
Member
Author
Hickey AnalysisEvaluated the plan before implementation. The structure is sound — parameterizing both Two Fix in this PR actions, both verified in the diff:
|
Open
Member
Author
|
| Step | Status | Duration | Verification |
|---|---|---|---|
| sync | ✓ | 13s | fetched origin; forge=github; clean tree; HEAD at origin/master |
| research | ✓ | 1m50s | read git.ts, git-review.ts, common/index.ts, contract.ts, ReviewTab.tsx, feature+steps; 1 client caller |
| hickey | ✓ | 1m46s | structure sound; 2 Fix-in-PR actions captured |
| branch | ✓ | 26s | review-branch-diff off origin/master |
| implement | ✓ | 4m21s | server + client + schemas + 2 e2e scenarios + 3 step defs |
| check | ✓ | 26s | just check green across 7 typecheck projects |
| docs | ✓ | 35s | README/architecture don't reference Review tab; no updates |
| police | ✓ | 3m27s | 3 fixes: comment on translate-error catch, rawDiff reshape, Record over match |
| fmt | ✓ | 21s | just fmt reformatted 1 line |
| commit | ✓ | 29s | commit 2ddb033 pushed |
| test | ✓ | 7m02s | revealed oRPC error sanitization; fixed via ORPCError; 6/6 review-tab + 12/12 right-panel |
| create-pr | ✓ | 1m19s | draft PR #520 + hickey analysis posted |
| ci | ✓ | 4m39s | all 12 contexts green; one e2e@aarch64-darwin flake (osc52, known) retried clean |
| Total | 26m54s |
Optimization suggestions
testdominated at 26% of wall-clock. The hit came from discovering oRPC sanitizes plainErrorthrows into "Internal server error" on the client — a one-line dev check (just dev, click Branch in a repo without origin, read the rendered error) duringimplementwould have caught this before the full e2e cycle and shaved ~3m.policespent ~2m re-runningjust checkafter applying its fixes. Folding the post-police verification into the police step's own budget (rather than treating it as a fresh check invocation) would tighten this.ciwas paced bye2e@aarch64-darwin(74s). Linux contexts finished earlier but waited on darwin. No low-hanging fruit — darwin e2e is the critical path and scales with scenario count.- Flake retries cost ~1m20s. The
osc52-clipboardcross-scenario clipboard bleed is the usual suspect on darwin; already tracked in Flaky tests log #320.
Workflow completed at 2026-04-14T23:21:29Z.
Drop the segmented Local/Branch toggle and the redundant CAPS label that said the same thing twice. The header is now a single prose-style line that *is* the mode switch: Changes vs HEAD · uncommitted only Changes vs origin/master · this branch's diff Click the phrase to cycle modes. The ref name is mono-spaced and gets a dotted underline on hover (the visible affordance); the italic suffix disambiguates `HEAD` and `origin/master` for users who don't immediately parse them. Tooltip describes the click action for keyboard / hover discoverability. Reclaims the horizontal real estate the segmented toggle was eating in a 300px panel, and dissolves the "tabs-within-tabs" feel of having mode buttons inside an already-tabbed Review pane. E2e steps re-targeted to the new \`review-mode-label\` testid + \`data-mode\` attribute; one extra assertion added to the missing- origin scenario to verify the mode actually flipped before checking for the error.
The right-panel tab gets a more honest name: "Code Diff" instead of "Review". Two reasons: 1. The tab does one thing (show changed files + their unified diff); "Review" oversold a workflow that doesn't exist yet (no inline comments, no agent handoff — those are later phases). 2. "Review" is forge-coloured (suggests GitHub PR review). "Code Diff" is forge-agnostic and stays accurate if Bitbucket/GitLab support lands later, or if non-Git VCS support ever lands. Schema enum: "review" → "diff" (short, single token). Tab label: "Code Diff" (display only). Migration `1.9.0` maps existing `"review"` persisted values forward; the historical 1.8.0 migration keeps casting the stale enum through `string` so old codepaths still compile under the live `RightPanelTab` type. File renames: ReviewTab.tsx → DiffTab.tsx, review-tab.css → diff-tab.css, review_tab_steps.ts → diff_tab_steps.ts, review-tab.feature → diff-tab.feature. All `review-*` testids become `diff-*`; the diff content area is `diff-content` (avoiding the awkward `diff-diff`).
Brings in #523 (KOLU_STATE_SUFFIX crash fix). No conflicts; the state.ts changes are independent from this branch's RightPanelTab schema migration.
"Code Diff" → "Code" — the broader name leaves room for a future file-tree-browser mode under the same tab. The cycle-on-click label is replaced by icon sub-tabs (pencil for local, branch-fork for branch diff) that naturally extend to a third mode without rethinking the header layout. A muted mono context label (e.g. "vs origin/master") sits after the icons so the user still knows which base they're diffing against. New DiffLocalIcon and DiffBranchIcon added to Icons.tsx.
The tab is called "Code" (not "Code Diff" anymore) — the component and CSS file should match. A future file-tree-browser mode will live under the same tab alongside the two diff modes.
Covers renames (R100), copies (C075), type changes (T), unknown letters (→ '?'), empty input, blank-line tolerance, and sort order. The parser is hand-rolled from git's --name-status output and these edge cases aren't reachable via e2e. Exports parseNameStatus (was private) so the test can import it directly.
This was referenced Apr 15, 2026
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
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.
The Review tab now toggles between "Local" and "Branch". Local mode (phase 1) diffs working tree vs
HEAD— what the agent just touched that you haven't committed yet. Branch mode (new) diffs working tree vsmerge-base(HEAD, origin/<defaultBranch>)— the same answer GitHub's "Files changed" tab gives for a PR, computed locally from git refs. No forge API, no GitHub dependency; it's the same computation any forge would do on its own side.The base resolution reuses the existing
detectDefaultBranch()helper (origin/HEAD→origin/main→master), so Bitbucket / GitLab / self-hosted setups get the same answer without kolu ever learning what forge the repo lives on. The original issue framed phase 2 as "PR diff", and dropping that framing is deliberate: a PR diff is one forge's presentation of a branch diff, and coupling to a forge API here would have rippled out into every non-GitHub setup.meta/github.tsstays isolated and untouched.Two choices are worth flagging because the opposite instinct is defensible. Working tree, not
HEAD, on the left side — "what this branch will ship" is the question you want answered before opening a PR, and it makes local mode a strict subset of branch mode (so switching modes is always additive). Merge-base, not base tip (three-dot semantics,git diff origin/master...HEAD) — this is the piece that handles the "I mergeorigin/masterinto my branch several times per PR" workflow correctly: each merge advances the merge-base, so the diff only shows your branch's net contribution and never double-counts the commits you merged in. Two-dot diff would get this wrong.When
origin/<default>doesn't exist,resolveBasethrowsORPCError("PRECONDITION_FAILED", { message })with the actionable suggestion inline:Run: git fetch origin && git remote set-head origin --auto. One subtle gotcha worth recording: a plainthrow new Error(...)gets sanitized by oRPC to "Internal server error" on the client, which would have hidden the suggestion entirely.ORPCErroris the right vehicle whenever you want a server-side message to reach the user.On the wire,
git.status/git.diffgained amode: "local" | "branch"discriminator, andGitStatusOutputbecame{ files, base: null | { ref, sha } }so the client can label "Changes vs origin/master" without re-resolving. Two new e2e scenarios cover the toggle default and the missing-origin actionable error.Try it locally