feat(review): custom base branch for code review (#599)#602
Merged
backnotprop merged 25 commits intomainfrom Apr 23, 2026
Merged
feat(review): custom base branch for code review (#599)#602backnotprop merged 25 commits intomainfrom
backnotprop merged 25 commits intomainfrom
Conversation
The "Branch diff" and "PR Diff" modes were locked to whatever the repo
auto-detects as the default branch. Users on feature branches cut from
`develop`, `release/*`, or another feature branch had no way to re-base
the review. Adds a searchable branch picker that appears next to the
diff-type dropdown when the active mode uses a base.
- Shared: listBranches + resolveBaseBranch in review-core; GitContext
gains availableBranches. Both Bun (review.ts) and Pi (serverReview.ts)
consume the same helpers; Pi picks them up via vendor.sh.
- Server: /api/diff/switch and /api/file-content accept optional `base`,
fall back to detected default when unknown.
- UI: BaseBranchPicker (Radix Popover + search + grouped local/remote).
Mounts only for branch/merge-base modes, hidden in PR mode. Diff-type
labels become generic ("Branch diff", "PR Diff") when the picker is
wired up — branch name lives in the picker.
- Session-only state (no cookie) to avoid the random-port cookie-jar
quirk that would make local persistence look broken.
For provenance purposes, this commit was AI assisted.
…isible Addresses review feedback on #602 — three related issues where my dedup + detected-default logic could force diffs against stale or non-existent refs. - getDefaultBranch returns the full remote ref (origin/main) instead of stripping to bare main. Diffs now run against the upstream tip by default, which stays fresh; local main falls out of the picture when it's stale. Falls back to local main/master only when no remote is configured. - listBranches no longer dedupes origin/foo when local foo exists. Both refs stay selectable — they can point to different commits, and the user needs to be able to pick either explicitly. - resolveBaseBranch gains a reverse fallback: bare `main` resolves to `origin/main` when only the remote is tracked (fresh clones). - getGitContext treats `origin/X` and `X` as equivalent when deciding whether to show branch/merge-base options — otherwise users on local main would suddenly see Branch diff / PR Diff options after the detected-default change, which wasn't the intent. For provenance purposes, this commit was AI assisted.
…fault The Codex/Claude/Tour prompt builders were reading gitContext.defaultBranch, which is frozen at server startup. After a user switched to a different base via /api/diff/switch, the agent was told to run `git diff <stale-default>..HEAD` and analyzed a different diff than the one in the UI. Tracks the active base as server-side state (`currentBase`), initialized from the detected default and updated in /api/diff/switch alongside currentDiffType. buildCommand reads currentBase instead. Mirrored on Bun and Pi. For provenance purposes, this commit was AI assisted.
Replaces the three native <select> dropdowns with Radix primitives that match the base-branch picker visually and behaviorally. - DiffTypePicker (Radix DropdownMenu): per-option info icon with a plain-English tooltip explaining what each mode shows. References "picked below" to make the base picker's relationship explicit. - WorktreePicker (Radix Popover + search): matches BaseBranchPicker shape. Search input appears only when there are >3 worktrees. - Tooltip gains an opt-in `wide` prop so multi-line hints wrap cleanly. Existing nowrap callsites unaffected. - TooltipProvider mounted at the review-editor App root. - ReviewDiffPanel keys on `reviewBase` to remount DiffViewer on base change — fixes a transient "trailing context mismatch" warning from @pierre/diffs caused by a race between the new patch and the new file-content fetch. For provenance purposes, this commit was AI assisted.
…patch merge-base fell through to the default prompt case, which dumps the full patch into the agent's prompt. For big PRs that fills the context window before the agent starts reviewing. Adds an explicit case that mirrors the branch-mode treatment: hand the agent the base branch and tell it to run `git merge-base` + `git diff` itself. Same contract as every other local mode. Applied to both buildCodexReviewUserMessage and buildTourUserMessage. For provenance purposes, this commit was AI assisted.
The client was re-initializing selectedBase from gitContext.defaultBranch on every /api/diff load. After a user picked a custom base, a page refresh or reconnect would silently desync: the UI picker showed the detected default while the server kept serving the custom-base patch, and /api/file-content fetches used the wrong ref — causing Pierre "trailing context mismatch" warnings on hunk expansion. Fix: echo currentBase in both /api/diff (GET) and /api/diff/switch (POST) responses. Client hydrates from that, falling back to the detected default when not present. Also resyncs after a switch in case the server fell back to detected (unknown-branch case). Mirrored on Bun and Pi runtimes. For provenance purposes, this commit was AI assisted.
Boots the Pi review server and asserts: - Initial /api/diff echoes the detected default as the active base - /api/diff/switch with a custom base echoes the resolved value - A subsequent /api/diff reflects the switched base (proves the refresh/reconnect path that motivated the round-trip fix) - An unknown base falls back to the detected default and the response echoes it so the client can resync Covers both runtimes — shared review-core state tracking is the same. For provenance purposes, this commit was AI assisted.
Adds "merge-base" to the two worktree subtype allow-lists that were missed when PR Diff was introduced in #485: - packages/shared/review-core.ts: WORKTREE_SUB_TYPES - packages/review-editor/App.tsx: client-side activeDiffBase parser Without this, "worktree:/path:merge-base" would fall through to { path: "/path:merge-base", subType: "uncommitted" } — server ran git in a non-existent cwd, client hid the base picker and mis-selected the diff type. Pre-existing since #485 but surfaced more prominently with the new picker UI, so fixing it here. Also adds a parseWorktreeDiffType regression test that walks the full DiffType set — catches any future mode additions that forget the list. For provenance purposes, this commit was AI assisted.
1. Base-picker race with file-content fetch (App.tsx) Split selectedBase from committedBase. Picker reads selectedBase so the chip feels responsive. File-content queries read committedBase, which only updates after /api/diff/switch returns — closes the window where the viewer could pair an old patch with the new base's content. 2. Worktree switch loses the current diff mode (App.tsx) handleWorktreeSwitch hardcoded "uncommitted". Preserve activeDiffBase across the switch so reviewers stay in the mode they picked (Branch diff, PR Diff, etc). 3. origin/HEAD trusted without verifying the target ref exists (review-core) symbolic-ref can point at a ref that was never fetched (narrow / partial clones). Verify the target via `show-ref --verify --quiet` before returning it, otherwise fall through to local main/master. Regression test covers the unfetched-pointer case. For provenance purposes, this commit was AI assisted.
handleBaseSelect optimistically set selectedBase before the server round trip, but the catch path never undid that when the switch failed. The picker would keep showing the new base while the viewer rendered the old patch (committedBase) — misleading reviewers about what the diff actually represents. fetchDiffSwitch now returns a success boolean. handleBaseSelect remembers the previous base, updates optimistically, and reverts on failure. Other callers ignore the boolean — behavior unchanged for diff-type and worktree switches. For provenance purposes, this commit was AI assisted.
Matches Pi's existing typeof check. Without this, a client sending
{"base": 123} would hit resolveBaseBranch's string methods and throw a
TypeError (500 response) instead of cleanly falling through to the
detected default. Our UI only ever sends strings, so no user-visible
impact — purely defensive parity.
For provenance purposes, this commit was AI assisted.
…ix menus 1. initialBase passthrough (review.ts, serverReview.ts, plannotator-browser.ts) startReviewServer now accepts an `initialBase` option on both runtimes (parity). Pi's openCodeReview forwards its resolved defaultBranch override through it. Without this, a programmatic caller requesting `develop` would get the correct initial patch but the server's currentBase (and thus /api/diff, picker label, empty-state text, expandable file-content queries, and agent prompts) would silently desync to the detected default. Server state now matches the patch that was generated. Regression test in server.test.ts covers the override path end-to-end. 2. FileTree keyboard handler yields to floating overlays (FileTree.tsx) The new Radix DropdownMenu / Popover / Dialog primitives don't stop ArrowUp/Down from bubbling to the window-level file-tree nav. The old native <select> used to absorb them. Fix: skip handler when document.activeElement is inside a role="menu" / "dialog" / "listbox" ancestor. Keyboard users opening the diff-type menu no longer shift the active file underneath. For provenance purposes, this commit was AI assisted.
…vers 1. merge-base instructions are now shell-neutral The previous prompt used POSIX assignment (mb=\$(git merge-base ...)) which fails on Windows cmd / PowerShell. Rephrased as two independent commands — find the merge base, then diff against it — so the agent chains them however it likes. Mirrored in codex-review and tour-review. 2. FileTree hotkeys also yield to Radix Popovers The earlier guard only matched role="menu"/dialog/listbox, but both the base picker and worktree picker are Radix Popovers with no such role. Added [data-radix-popper-content-wrapper] — Radix's shared wrapper on every floating primitive — so Popover, DropdownMenu, Tooltip, HoverCard are all captured consistently. 3. WorktreePicker keyboard focus works for short worktree lists onOpenAutoFocus was preventing Radix's default focus and routing to the search input. But the search input only renders when there are >3 worktrees, so the common 1–3 case left focus on the trigger button and arrow keys bubbled out to the file tree. Now only override Radix when a search input actually exists. For provenance purposes, this commit was AI assisted.
Local-mode feedback previously sent just file paths and line numbers — receiving agent had to guess which diff those anchor to. Subtle but real: if a reviewer switched diff mode (Uncommitted → Branch diff) or picked a custom base and then sent line-specific feedback, the agent would map "Line 42 (old)" back to whichever diff it originally assumed, which could be a different one. Adds a `**Diff:** ...` line to the local-mode feedback header describing the committed mode + base + (optional) worktree. Reads from committedBase (what the server actually computed) and activeDiffBase (derived from the committed diffType) so the header always matches the diff the annotations are anchored to. PR mode unchanged — its existing header already carries the PR's baseBranch/headBranch context. Pre-existing gap (mode switching has always had this problem), but this PR introduced the base picker that makes it more visible, so surfacing the context here. For provenance purposes, this commit was AI assisted.
Added to both the SDK-side models registry (codex-sdk.ts) and the user-facing Agents settings tab (AgentsTab.tsx) so it's selectable in AI review flows. Default stays GPT-5.4; users opt in explicitly. For provenance purposes, this commit was AI assisted.
1. resolveBaseBranch now trusts unknown refs instead of silently swapping Previous behavior: if the caller's base wasn't a local branch or origin/* remote, we'd coerce it to the detected default. That lost tags, SHAs, and refs under non-tracked remotes — the initial diff could succeed while later file-content/switch calls silently contradicted the visible patch. Now we pass the ref through; git errors naturally if it's truly invalid. Updated Pi integration test. 2. Agent-job "Copy All" emits the same feedback as /api/feedback ReviewAgentJobDetailPanel was calling exportReviewFeedback without the diff context, so copying an agent run's findings dropped the "**Diff:** ..." header the main feedback path now includes. Threaded feedbackDiffContext through ReviewState so both paths produce identical markdown. 3. Info icon in the diff-type dropdown no longer selects on touch The wrapper stopped pointerdown+click but not pointerup. Radix DropdownMenu.Item consumes pointerup for selection, so taps on the icon would switch the diff mode instead of showing the hint. Added onPointerUp to the stopPropagation list. For provenance purposes, this commit was AI assisted.
Previous commit placed feedbackDiffContext after reviewStateValue, but
reviewStateValue now reads it — React evaluates the first useMemo
before the variable is initialized, throwing a Temporal Dead Zone
error ("Cannot access 'ui' before initialization" in minified builds)
and blanking the review UI.
Moved the feedbackDiffContext useMemo above reviewStateValue so it's
live by the time subsequent memos reference it.
For provenance purposes, this commit was AI assisted.
1. Drop resolveBaseBranch strip/prefix heuristics (P1) The bare↔`origin/` transforms silently retargeted caller-supplied refs to different commits (`upstream/main` would collapse to local `main`; tag `release` would collapse to `origin/release`). Removed both transforms. The resolver now just trusts the caller — git errors naturally on truly invalid refs. Signature simplified to drop the unused `available` param; all four callers updated. 2. Worktree switch rehydrates the sidebar (P2) /api/diff/switch (both runtimes) now recomputes gitContext for the effective cwd and returns it in the response. Client merges only the per-cwd fields — currentBranch, defaultBranch, diffOptions — keeping the original `worktrees` list and `availableBranches` (shared across worktrees of the same repo). Fixes the case where you switch to a worktree on a feature branch but the diff-type picker still hides Branch diff / PR Diff because it thinks you're on the main repo's branch. 3. Per-job diff-context snapshot for agent "Copy All" (P2) AgentJobInfo gains an optional diffContext. The server's buildCommand (both runtimes) snapshots mode + base + worktree at launch and attaches it to the job. The agent-result panel now reads `job.diffContext` for its Copy All export, falling back to the live feedbackDiffContext only for older jobs without a snapshot. Keeps the exported feedback honest even if the reviewer switches modes or bases after the job ran. For provenance purposes, this commit was AI assisted.
The worktree picker's top row represents "go back to plannotator's launch cwd" and is labeled with the repo/branch that was active at launch. The previous rehydrate merge overwrote gitContext.currentBranch with the post-switch worktree's branch, so the label followed the active worktree — clicking it still sent you back to launch, but the label claimed it would take you somewhere else. Fix: stop merging currentBranch on diff switches. Freeze it at initial-load value. defaultBranch and diffOptions keep updating — they describe the active diff and other UI (empty-state text, diff-type picker) needs them fresh. Verified currentBranch has a single consumer on the client (WorktreePicker mainLabel); freezing it has no other side effects. For provenance purposes, this commit was AI assisted.
1. Track currentGitContext as mutable server state
/api/diff GET previously returned the startup gitContext forever, so
a refresh after a worktree switch rebuilt the sidebar from stale
data. Track an updatable currentGitContext alongside currentPatch /
currentDiffType / currentBase. /api/diff/switch commits the fresh
context to it; /api/diff GET returns it. Same mutable-state pattern
as the other session fields. Mirrored on Bun and Pi. hasLocalAccess
stays anchored to the startup value (startup-only property).
2. Harden against refs that start with `-` via --end-of-options
User-supplied bases flowed directly into git argv: `git diff
${base}..HEAD`. A base like `--output=/tmp/foo` would be parsed as
the --output flag, redirecting diff output to an attacker-chosen
path (reachable via local CSRF + text/plain POSTs that bypass CORS
preflight). Added `--end-of-options` before the positional ref in
the branch diff, merge-base lookup, merge-base diff, and gitShow
helper. Git's native "stop parsing flags here" marker (git 2.24+).
Tests pin the meaningful parts of the error string rather than the
full argv so harmless flag reordering can't break them.
3. Always offer Branch diff / PR Diff when a default branch exists
The hide-on-default guard dated from the pre-base-picker era, when
"vs main" from main was always empty and the option was just noise.
With the base picker, users on main can meaningfully pick "vs
develop" or any other branch. The guard also trapped users after
my worktree-preserve and Pi initialBase paths landed them on the
default branch with Branch diff already active. Removed the guard;
options are always present when a default branch is detected.
4. Correct stale comment in App.tsx
Client comment claimed the resync covered server-side fallback, but
resolveBaseBranch trusts callers verbatim. Reworded to describe the
actual purpose (startup hydration + echo confirmation).
For provenance purposes, this commit was AI assisted.
…text After a self-review of the previous combined fix (075c96c, later reverted), the mutable currentGitContext change proved to have real regressions: "Main repo" from a worktree fell back to the last-visited worktree's cwd instead of the launch cwd, and /api/diff GET after a switch corrupted the worktree picker's state. The root cause: I conflated two uses of gitContext (stable fallback semantics vs dynamic display fields) into a single mutable variable. Once the hide-on-default guard is removed (this commit), diffOptions is the same across worktrees of the same repo and there's nothing worth rehydrating on refresh. The mutable gitContext "fix" was solving a problem that the guard removal makes moot, so it's dropped entirely. Shipped here: - --end-of-options hardens five git argv sites where user-controlled refs flow in (branch diff, merge-base lookup + diff, gitShow, file-content merge-base). Prevents a malicious client from passing "--output=/tmp/foo" as a base and having git treat it as a flag. Git 2.24+ feature; no test regressions. - Removed the !onDefaultBranch guard in getGitContext. Branch diff / PR Diff options now always appear when a default branch exists. The guard was correct when base was always the detected default (empty diff from default vs default is meaningless), but the base picker makes these options meaningful from any branch. It was also trapping users who landed on the default branch with branch mode already active via worktree switch or Pi initialBase. - Reworded three stale "server may have fallen back" comments — the resolver trusts the caller verbatim, it doesn't validate. - Test error-string assertion loosened to tolerate --end-of-options argv reorder. For provenance purposes, this commit was AI assisted.
Was used for dedup logic that has since been removed. The Set was still being populated but never read. For provenance purposes, this commit was AI assisted.
- WorktreePicker: removed inline `style={{ width: 'calc(100% - 0.5rem)' }}`
that duplicated what `w-full mx-1` already provides via Tailwind.
Matches BaseBranchPicker's pattern.
- CLAUDE.md: added `/api/diff/switch` POST (was missing entirely) and
documented the `base` field on `/api/diff` GET, `/api/diff/switch`
POST body, and `/api/file-content` GET query param.
For provenance purposes, this commit was AI assisted.
CLAUDE.md (symlinked to AGENTS.md) was edited in the previous commit but AGENTS.md wasn't staged. This captures the actual file change. For provenance purposes, this commit was AI assisted.
…ntent Switching between Branch diff and PR Diff for the same file and base didn't trigger a file-content refetch — the useEffect deps (filePath, oldPath, reviewBase) were all unchanged. But the two modes compute different "old" content (base tip vs merge-base ancestor), so Pierre could mismatch or show wrong expansion context. Fix: expose activeDiffBase through ReviewState and include it in the ReviewDiffPanel key. Mode changes now remount DiffViewer, which clears the cached content and fetches fresh. Same trade-off as the existing reviewBase-keyed remount (scroll position resets on mode switch). For provenance purposes, this commit was AI assisted.
4 tasks
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.
Summary
develop,release/*, or another feature branch can actually review the right delta instead of being locked to the auto-detected default.packages/shared/review-core.ts(listBranches,resolveBaseBranch,GitContext.availableBranches) so both the Bun runtime (packages/server/review.ts) and the Pi node runtime (apps/pi-extension/server/serverReview.ts) pick it up — Pi viavendor.sh.How it works
/api/diff/switchand/api/file-contentaccept an optionalbaseparam;resolveBaseBranchvalidates against the known branch list and falls back to the detected default for unknown values.BaseBranchPicker) with search, grouped Local/Remote, "detected" badge, and a "Reset to detected" action. Session-only state (no cookie) to avoid the random-port cookie-jar quirk that would make local persistence look broken.Closes
Closes #599
Test plan
main: picker defaults tomain, switching to any other branch refetches the diff.develop: pickdevelop, confirm the diff now shows only the feature's delta.plannotator review <PR-URL>) → picker does not appear.bash apps/pi-extension/vendor.sh, verify/api/diffreturnsavailableBranchesand acceptsbase.For provenance purposes, this PR was AI assisted.