Skip to content

Review tab: see what this branch will actually ship#520

Merged
srid merged 16 commits intomasterfrom
review-branch-diff
Apr 15, 2026
Merged

Review tab: see what this branch will actually ship#520
srid merged 16 commits intomasterfrom
review-branch-diff

Conversation

@srid
Copy link
Copy Markdown
Member

@srid srid commented Apr 14, 2026

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 vs merge-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/HEADorigin/mainmaster), 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.ts stays 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 merge origin/master into 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, resolveBase throws ORPCError("PRECONDITION_FAILED", { message }) with the actionable suggestion inline: Run: git fetch origin && git remote set-head origin --auto. One subtle gotcha worth recording: a plain throw new Error(...) gets sanitized by oRPC to "Internal server error" on the client, which would have hidden the suggestion entirely. ORPCError is the right vehicle whenever you want a server-side message to reach the user.

On the wire, git.status / git.diff gained a mode: "local" | "branch" discriminator, and GitStatusOutput became { 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.

Scope. Closes phase 2 of #514. Persisting mode across sessions, per-branch base override (for PRs targeting non-default branches), opportunistic PR-base detection via gh pr view/glab mr view, and binary/large-file handling (phase 5) all remain future work.

Try it locally

nix run github:juspay/kolu/review-branch-diff

srid added 2 commits April 14, 2026 19:08
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.
@srid
Copy link
Copy Markdown
Member Author

srid commented Apr 14, 2026

Hickey Analysis

Evaluated the plan before implementation. The structure is sound — parameterizing both git.status and git.diff with a mode discriminator (rather than splitting into four procedures) keeps the surface area small and makes the dispatch orthogonal. Threading the resolved base through the status response is composition, not complecting: files and base are independent computations but coherent at the domain level, and splitting them across two RPCs would fragment the concern.

Two Fix in this PR actions, both verified in the diff:

  1. Actionable error for missing origin/HEADresolveBase throws with Run: git fetch origin && git remote set-head origin --auto so the user knows exactly how to recover. (A followup fix promoted this to ORPCError so the message actually reaches the client — plain Error was being sanitized to "Internal server error" on the way through oRPC.)
  2. Invariant comment in getDiff — the branch-mode path skips the --no-index fallback because the file list is sourced from git diff --name-status <base>, which only surfaces files already in the diff. Documented inline at the rawDiff choice so a future reader doesn't "restore" the fallback in branch mode and bring back dead code.

@srid srid mentioned this pull request Apr 14, 2026
@srid srid marked this pull request as ready for review April 14, 2026 23:21
@srid
Copy link
Copy Markdown
Member Author

srid commented Apr 14, 2026

/do results

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

  • test dominated at 26% of wall-clock. The hit came from discovering oRPC sanitizes plain Error throws 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) during implement would have caught this before the full e2e cycle and shaved ~3m.
  • police spent ~2m re-running just check after 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.
  • ci was paced by e2e@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-clipboard cross-scenario clipboard bleed is the usual suspect on darwin; already tracked in Flaky tests log #320.

Workflow completed at 2026-04-14T23:21:29Z.

srid added 14 commits April 14, 2026 19:45
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.
@srid srid merged commit d95baba into master Apr 15, 2026
11 checks passed
@srid srid deleted the review-branch-diff branch April 15, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant