feat(desktop): let the Git changes panel pick its comparison base branch

[faga295]

Summary

The Changes panel previously compared only against the backend-resolved base branch, so stacked work could include inherited changes with no way to choose another comparison base. This PR adds a searchable base-branch picker beside the current branch (current → base).

• Explicit selections persist per Session and are sent on subsequent reads. With no explicit selection, reads continue to omit baseBranch and follow the repository default; displaying the resolved default does not pin it. If a saved branch becomes invalid, the panel clears the saved selection and retries once with the dynamic default.
• Branch options now carry { label, value }: readable names in the UI, fully qualified refs/heads/... or refs/remotes/... values for comparisons and storage. Unique legacy names migrate to canonical refs; ambiguous names and tag-only refs are rejected. Local and remote branches with identical labels remain distinct.
• Branch context is returned even when computing the diff fails (for example, unrelated histories), keeping the picker available so the user can recover by selecting another branch. A user-initiated switch spins the picker and dims/disables interaction with the old diff until the read finishes. Changing the comparison resets file pagination; refreshing the same comparison preserves it.
• Oversized unified diffs that exceed the child-process output buffer retain the available output and mark the review as truncated instead of failing the entire review.
• The picker has no selectable “auto” entry and is hidden when no branch options are available. It displays a branch label when resolved, or the localized field placeholder when unresolved. Its menu is capped at 180px wide with a 288px listbox height; long names ellipsize. Search empty-state and result-count feedback are localized in Simplified and Traditional Chinese.

The backend enumerates full refs instead of relying on %(refname:short), keeps origin/HEAD selectable, and shares the preferred-branch ordering with default resolution. The remote HEAD's resolved target takes precedence; the current branch can also be the base, with staged and unstaged changes still included.

Fixes #5119

Before:

After:

Verification

Checked against PR head abce32742c4c384acbb87842ade7746bf4f38296 on 2026-09-20:

• npm --workspace @maka/desktop run build:main — passed.
• node --test --test-force-exit apps/desktop/dist/main/__tests__/git-review-main.test.js apps/desktop/dist/main/__tests__/session-review-base-branch.test.js apps/desktop/dist/main/__tests__/session-review-panel-recovery.test.js — 19/19 passed. Covers full-ref and legacy-name resolution, tag/name collisions, default ordering, per-Session persistence, corrupt storage, recovery after invalid selections or failed diffs, switch feedback, and output-buffer truncation.
• Full desktop tests, all four typecheck configurations, lint/format, renderer build, runtime tests, and browser/Storybook checks were not rerun for this description update. See the PR checks for current CI results.

Earlier verification recorded in this PR (not revalidated against the current head): desktop typecheck, architecture checks, Biome and renderer build passed; desktop tests reported 2461/2462 with a settings-ipc-helpers Tavily failure. Storybook checks covered filtering, localized search feedback, long-name ellipsis, and updating/closing the picker after selection. The screenshots above are retained from that earlier verification.

Review focus

• The shared Git review contract changes options from strings to { label, value } and can include branch context in failure results. Legacy saved names are accepted only when they identify one enumerated branch.
• Scoped Astryx CSS caps the popover width and listbox height and allows option labels to shrink. The picker is included in the Astryx surface inventory, and Storybook fixtures use full-ref option values.
• This branch also stabilizes a runtime PTY test by waiting for the complete READY line and cursor position before taking the PID-observation baseline; it does not change runtime production behavior.

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: pi (DeepSeek V4.1 Flash) — implementation and earlier local verification. Codex — PR description refresh and the focused build/test verification listed above.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally (full current-head checks not rerun; focused checks above passed)

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[Astro-Han]

The picker holds up end to end — I traced the value from the Astryx trigger to the git command and found no correctness bug. One behavior I'd like a decision on, plus a few smaller things.

Behavior worth confirming

The resolved default is now persisted, so it stops following the repo. resolveAdoptedBaseBranch returns the backend's resolved branch when nothing is pinned (session-review-base-branch-model.ts:85-97) and the panel writes it to storage (session-review-panel.tsx:143-153). So a Session that never opens the picker pins a branch on its first read and keeps it for the life of that Session, across restarts. Before this PR every read re-resolved, so the panel tracked origin/HEAD. The blast radius is narrow — BASE_BRANCH_PRIORITY ranks refs/remotes/origin/HEAD first (git-review-main.ts:403-410) and that ref survives a rename of the default branch — but in a repo with no origin/HEAD the pin freezes on refs/heads/main and will not follow a later rename to trunk. If the goal is only "the trigger must name a real branch", the resolved snapshot.baseBranch could be displayed without being persisted (persist only an explicit pick) and the dynamic default would survive. Either way, the Summary should say the implicit default gets stored, since that is a behavior change beyond "the choice persists".

Findings

1. The invalid_base_branch recovery path has no test. The clear-the-pin-and-retry-once branch (session-review-panel.tsx:123-135) is the headline answer to a pinned branch disappearing, but session-review-panel-recovery.test.ts only drives the git_failed case, and the model test stops at resolveAdoptedBaseBranch. A case where review.read returns invalid_base_branch once and then succeeds would cover it: request sequence ['refs/heads/gh-pages', undefined], pin cleared in storage, no error banner.
2. invalid_base_branch drops branches even though it is in scope. branches is captured at git-review-main.ts:67, but the rejection at :71-73 returns without it, while the contract documents it as "available even when computing the selected branch diff fails" (packages/core/src/git-review.ts:66-67) — unborn_repository and git_failed both include it. Spreading it here too would make the contract uniform and let the panel keep the picker if the retry ever failed.
3. copy.invalidBaseBranch looks unreachable now. The panel always clears the pin and retries with no selection, and the backend only rejects a non-null baseBranch, so I could not construct a path where the banner at session-review-panel.tsx:233 renders in any of the three locales. Keeping it as a guard is fine; worth a line in the PR so nobody hunts for it.
4. nit — the picker offers the current branch as a comparison base. Options include it (the updated git-review-main.test.ts fixture has feature/review as both currentBranch and an option). Picking it diffs against merge-base(base, HEAD), i.e. HEAD, so the panel silently becomes "uncommitted changes" while the header still reads feature → feature. resolveBaseBranch deliberately skips the current branch (git-review-main.ts:431); marking that option disabled would close the gap.
5. nit — visibleFileCount is not reset when the base changes (session-review-panel.tsx:91,216), so a "show 20 more" position from the previous comparison carries into the new one and the button can appear or vanish. Pre-existing for Session switches, newly reachable through the picker.
6. nit — the buffer-overflow change is missing from the Summary and CHANGELOG. runDiffAllowTruncated (git-review-main.ts:220-245) changes what a huge branch diff produces, and the PR answers "Yes — described under Summary above" to the behavior question. Related: the sibling --name-status call at :175 is not wrapped, and a mid-file cut can leave a listed file with an empty or partial diff — acceptable degradation, but it belongs in the description.
7. nit — unrelated change: the PTY-output wait in packages/runtime/src/__tests__/shell-run-manager.test.ts is unrelated to this panel; better as its own PR.
8. nit — copy conventions: the sibling ICU plural uses {count, number} (packages/ui/src/astryx-copy.ts:150, commandPalette.resultCount), the new search.resultCount uses {count}. Also the comment above search ("Selector and MultiSelector render the same two search affordances") is stale — emptySearch/resultCount are Selector-only, MultiSelector has its own catalog keys.
9. nit — options={[...props.baseBranchOptions]} (session-review-base-branch-picker.tsx:51) allocates per render and defeats Selector's filteredItems memo; a useMemo on the panel side would keep it stable.
10. nit — process: only the first commit carries Generated-by: pi (…), while CONTRIBUTING.md asks for the trailer on each affected commit, kept through squash; and CONTRIBUTING.md asks for before/after images on UI changes — the Storybook measurements are useful evidence, but nothing is attached.
11. docs/superpowers/plans/2026-09-10-review-branch-refs.md is the only file under docs/superpowers/; tracked plans elsewhere live in docs/archive/ or docs/. Consider moving or dropping it.
12. Heads-up: merge state is DIRTY (conflicts with main); the test check is green.

Verified, no issue found

• Selector.onChange hands back the option value in the non-clearable variant (@astryxdesign/core@0.5.2, Selector.d.ts), so what gets persisted is the canonical ref, not a label; the first read omits baseBranch entirely (reviewBaseBranchRequestValue).
• Search filters on option.label (Selector.js:260), so short branch names are searchable, and @astryx.selector.emptySearchResults / .resultCount do exist in the Astryx catalog (locales/en.json:1270,1274), so the Chinese overrides land.
• The %(refname:short) bug is real: git reports refs/remotes/<remote>/HEAD as a bare <remote> (refs/remotes/author/HEAD → author on git 2.50 here), which the old .endsWith('/HEAD') filter let through. Enumerating %(refname) and stripping the namespace is the right fix.
• Ordering cannot drift: one BASE_BRANCH_PRIORITY list feeds both listBaseBranches (:450) and resolveBaseBranch (:414), and the unranked/label tiebreak reads correctly.
• Ambiguity is rejected rather than guessed: same-label local vs remote, tag vs branch, and tag-only names all fail closed (resolveRequestedBaseBranch, :440), and the orphan-history and tag-named-main cases behave.
• The CSS selectors are real (themeProps('popover-surface'), themeProps('item')); the layer renders inline inside the wrapper unless its parent has an unsafe writing context, and the popover is popover="auto" (top layer), so overflow-y: auto on .maka-session-review-panel will not clip it.
• Contract blast radius is small: baseBranchOptions is only consumed by the desktop app, the IPC already validated baseBranch (runtime-host-workspace-ipc-main.ts:66-79), and both new test files sit under src/main/__tests__ where test:dist (dist/main/**/*.test.js) picks them up.

[Astro-Han]

Could we have a before after comparison picture in the pr body? It would help clarify things easier. Thanks!

[faga295]

@Astro-Han done

[faga295]

@Astro-Han Thanks for the detailed review. Most items are addressed, including keeping implicit defaults dynamic. A few behavior, documentation, and process points remain open:

• Default persistence — fixed: Only explicit selections are persisted. The resolved default is displayed without pinning it, so subsequent reads continue to follow the repository default.
• 1 — fixed: Added recovery coverage for invalid_base_branch, verifying the request sequence, cleared storage, restored picker, and absence of an error banner.
• 2 — fixed: invalid_base_branch now returns branch context, preserving the picker during recovery.
• 3 — fixed: Removed the dedicated invalid-base-branch banner path.
• 4 — still open for discussion: The current branch remains selectable, with test coverage and documentation confirming that it shows staged and unstaged changes. Disabling it or clarifying the UI remains a UX decision.
• 5 — fixed: Pagination resets when the comparison or Session changes, while refreshes preserve expansion.
• 6 — partially addressed: The Summary documents oversized-diff truncation, but the CHANGELOG and the --name-status/partial-file limitations still need updating.
• 7 — fixed: The unrelated PTY test change is no longer included.
• 8–9 — fixed: Updated number formatting and the search comment; memoized the picker options.
• 10 — addressed: The screenshots have been updated in the PR description.
• 11 — fixed: The standalone plan file is no longer included.
• 12 — addressed: All CI checks have passed.

[Astro-Han]

Verified against head f6ffba1. My earlier behavior question stands resolved in shape — the picker is correct end to end: renderer strings reach merge-base/diff only after matching an enumerated for-each-ref entry (short-name shadowing of tags is now impossible — full refs fix a real latent bug), three-dot semantics via merge-base SHA pin both reads so a moving base can't skew them, invalid_base_branch → unpin → single bounded retry, picker stays alive on every failure that carries branch context, and it never blocks the panel (stale diff dims instead of skeletoning).

A simplification pass found the +1073 is mostly honest (the IPC plumbing already existed on main — this PR activates it), but ~135 lines defend a false premise:

P3 — the 'legacy display name' canonicalization chain is dead machinery (~100 lines incl. tests). git-review-main.ts:436-437 comments 'Old preferences contain display names', but no shipped version ever persisted a base-branch choice — the -v1 storage key is new in this PR and every producer sends option.value (a qualified ref). Once the label-matching arm dies, resolveAdoptedBaseBranch becomes a provable no-op (an ok snapshot implies exact option-value match, so adopted === current always). Cut: label matching in resolveRequestedBaseBranch, resolveAdoptedBaseBranch + its call site + testing.ts re-export, reviewBaseBranchRequestValue, and the migration/label tests. The invalid→unpin→retry path already covers a genuinely disappeared branch.

P3 — duplicated storage guards on a false premise. session-review-base-branch-model.ts:24-41 re-implements safeLocalStorageGet/Set; the comment claims an architecture check forbids the import, but siblings in the same feature already import browser-storage (model/workbar-layout.ts:23) — the ban is scoped to overlays. Delete lines 24-41, import the helpers.

P3 — session-switch reset effect is dead. panel.tsx:102-111 — SessionReviewPanel mounts with key={sessionId} (workbar-surface.tsx:521), so props.sessionId never changes in place; the useState initializer already reads storage.

P3 — stale picker options when the repo disappears. panel.tsx:140-146 — failures without branch context skip setBranches, leaving options of a gone repo rendered; setBranches(nextBranches ?? null).

Two non-blocking observations: session-keyed localStorage never prunes (dead sessions leave entries — trivial footprint, worth a hook if session-deletion has one); no story/e2e exercises the picker, so the new CSS block (review.css:28-68) has zero browser-tier verification — the label/value machinery is well tested but the popover layout isn't.

Decision for you, not a blocker: persistence scope is per-session — a new session on the same repo re-falls to the default. Per-repo persistence or no persistence (in-memory only, deletes the whole model file) are both defensible; the missing policy is whether a comparison base should outlive its session.

[faga295]

@Astro-Han Thanks for you patience review.All four P3 issues have been fixed in 1b46b24. Of the two non-blocking observations, Storybook coverage has been added; cleanup of preferences for deleted sessions remains deferred.

1. Legacy display-name canonicalization: removed.
   The backend now matches option.value === requestedBaseBranch directly. Removed resolveRequestedBaseBranch, resolveAdoptedBaseBranch, its call site and testing export. Replaced reviewBaseBranchRequestValue with selection ?? undefined. Migration tests were removed or updated to require qualified refs; tests for distinguishing same-label branches and rejecting unqualified names remain. Disappeared branches still follow the existing invalid_base_branch → clear saved selection → retry with the default recovery path.
2. Duplicated storage guards: removed.
   Deleted the local wrappers and the incorrect architecture-restriction comment. Persistence now goes through WorkbarServices.reviewBaseBranchPreference, implemented by a Desktop adapter that reuses safeLocalStorageGet/Set. The shared helpers live in the platform layer, with the original entry point re-exporting them.
3. Session-switch reset effect: removed.
   Since key={sessionId} remounts SessionReviewPanel when the session changes, the useState initializer reads the saved preference on each mount. The redundant reset effect is gone.
4. Stale picker options when the repository disappears: fixed.
   The panel now calls setBranches(nextBranches ?? null) unconditionally, clearing previous options when a failure provides no branch context. A regression test was added.
5. Storage cleanup for deleted sessions: deferred.
   Entries are currently removed when a selection is unpinned or an invalid branch is recovered, but cleanup is not wired into session deletion. These leftover entries have a small storage footprint and do not affect other sessions. A follow-up can remove them after successful session deletion.
6. Browser-level picker coverage: Storybook added; no separate E2E added.
   The new ChangesBaseBranchPicker interaction story exercises opening, searching, selecting, and reopening the picker in a 320px-wide panel. It also checks popover width, list height, long-name truncation, and retained selection. This directly covers the layout and interaction concerns raised here, so no additional full-application E2E was added.

[hqhq1025]

Reviewed this current head independently. The 27-file diff adds a per-session Git Review base-branch picker and persistence, uses fully qualified branch refs in main-process Git commands, retains branch options on comparison failure, and degrades oversized diff output to a marked truncated result. I inspected selection/reload generation fencing, invalid-branch recovery, ref validation, and the Git IPC/result paths; no substantiated P0–P3 issue emerged. Node 24 install/build and 22 focused Git Review/picker/recovery tests pass; current-head CI test is green. This branch currently conflicts with main in apps/desktop/stories/workhub.stories.tsx and docs/astryx-surface-file-inventory.md; resolve and revalidate the combined result before merge. I did not run a real Electron interaction, large-repository performance, or cross-platform Git fault injection.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[faga295]

@hqhq1025 Thanks for you review, conflicts are resolved now

[hqhq1025]

Reviewed the current head (e45f1c8c). The PR adds a per-session Git Review base-branch picker. After rebasing onto current main, it keeps explicit qualified refs in the Workbar preference service; an unpinned session follows the repository's current default, and an unavailable saved ref is cleared before one retry (apps/desktop/src/renderer/features/workbar/tools/review/session-review-panel.tsx:87-132, apps/desktop/src/renderer/platform/desktop/review-base-branch-preferences.ts:26-49). The Main process accepts only an enumerated qualified ref for merge-base (apps/desktop/src/main/git-review-main.ts:63-83). The previous conflicts are resolved. I found no substantiated P0–P3 on this head.

On Node 24, npm ci, build:test, 22 focused Git Review/preference/panel tests, Desktop typecheck, renderer architecture check, and git diff --check passed. A merge-tree against current main is clean. The current-head CI test was still running when I reviewed it, so its result is not yet verified. I did not run packaged Electron interactions, large-repository performance, or cross-platform Git fault injection. No schema or migration changes.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[Astro-Han]

Final multi-reviewer pass on e45f1c8c: no blocking issues (no P0–P2). Four minor (P3) notes below; none of them needs to hold the merge.

This round was reviewed independently by several automated reviewers built on different model families, each reading the full diff at this exact head without seeing the others' conclusions. They converged on the same result.

Correctness — checked and holding

• Ref validation / injection: the requested base is only accepted when it exactly equals a fully qualified ref enumerated by for-each-ref refs/heads refs/remotes (git-review-main.ts:67-76); short names, tags and refs/tags/* are rejected as invalid_base_branch, and git is invoked via execFile with argument arrays, so no unvalidated string reaches git or a shell.
• Stale pin recovery: an invalid_base_branch with a pin clears the pin (state, ref, storage) and re-reads once with no selection; the retry cannot be rejected again, so it cannot loop.
• Per-Session persistence and switching: the panel is keyed by sessionId in workbar-surface.tsx, revisionRef is re-checked after every await, and there is no await between the revision check and the pin-clearing write, so a superseded load cannot clear a newer pin.
• Storage failures: reads degrade to "unpinned", writes are best-effort, matching the port contract in ports.ts.

Is the diff size necessary?

Yes, broadly. Of +1113/−62 across 22 files, roughly 600 lines are tests, ~400 are the feature (qualified-ref enumeration and validation, the picker, panel selection/recovery, the persistence port and Desktop adapter, core types, copy), and ~65–120 are CSS, stories, README, CHANGELOG and the generated surface inventory. No reviewer found unrelated changes, and none found a materially smaller implementation that keeps both "qualified refs, no tag/short-name ambiguity" and "only explicit choices are pinned". Two optional trims:

• runDiffAllowTruncated / maxBufferStdout (~40 lines + one test) is a robustness fix that could be its own PR, though selecting a far-diverged base is exactly what makes the buffer overflow reachable, so keeping it here is defensible.
• platform/desktop/browser-storage.ts is byte-identical to the existing renderer/browser-storage.ts (copied because the architecture ratchet forbids the platform zone depending on that root file). The two small try/catch helpers could be inlined into review-base-branch-preferences.ts to drop the extra file.

P3 notes

1. Local and remote branches can render with the same label — see inline comment. (Found independently by three reviewers.)
2. PR description is out of date: it says unique legacy short names migrate to canonical refs, but there is no migration code — a short name fails validation and is cleared as a stale pin (harmless in practice, since nothing persisted a short name before this PR). It also says the picker menu is capped at 180px wide, while review.css and the story assert 280px, and it mentions a PTY test stabilisation that is not in this diff.
3. Base can now be the current branch: dropping the old candidate === currentBranch skip means a local-only repository on main resolves its default base to refs/heads/main and the header reads main → main. The diff content is correct (it degrades to working-tree changes, and there is a test for it) and the description says this is intentional; flagging only because it reads oddly in the UI.
4. Persistence lifecycle and race coverage: the per-Session map in review-base-branch-preferences.ts is never pruned when Sessions are deleted, and if storage quota is exhausted the write is silently dropped (choice works until restart). Separately, the switch/late-response protection above is structurally correct but no test drives "earlier read resolves after a later one"; a test with two pending reads resolved out of order would pin it.

Verification and limits

Live head re-checked before posting: e45f1c8c. The test check is green at this head, and the head merges cleanly with current main. Not run by this review: a packaged Electron build, real keyboard/long-name visual checks, or cross-platform git failure injection.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

[Astro-Han]

P3: a local branch literally named origin/release (refs/heads/origin/release) and the remote refs/remotes/origin/release both get the label origin/release here (line 449 and this line). The values stay distinct, so the comparison itself is right, but the picker renders only option.label (session-review-base-branch-picker.tsx:52-66), so the two entries — and the selected trigger — look identical. git-review-main.test.ts:160-175 already constructs this case on the main-process side. Suggest disambiguating only on collision (e.g. append "local"/"remote" or show the full ref) and adding a UI regression.

[Astro-Han]

Approved at @Astro-Han's explicit request after reviewing the findings above. The P3 notes are non-blocking follow-ups.