Commit Graph

3 Commits

Author SHA1 Message Date
Michael Ramos 75e8b78cc7 fix(review): stop stubbing files whose worktree content the size probe cannot find (#1220)
The oversized preflight in buildBoundedTrackedDiff mapped every object the
cat-file batch could not size to infinity. `missing` is routine: for
tree-vs-worktree diffs git hashes the WORKING-TREE content of any path pulled
into rename/copy detection and prints that hash in --raw output without ever
writing the blob, and partial clones report it for unfetched blobs. Those
files were excluded by pathspec and replaced with a contents-free binary stub,
so a renamed-and-edited file rendered as a silently empty card and
/api/file-content refused it as binary.

Missing now means unknown, not oversized, and an unreadable new side is bounded
by the working-tree file's stat size instead. Every rendered diff stays bounded
git-side by core.bigFileThreshold (#1205), genuinely oversized files still stub
via real probe sizes plus the stat door, and a blob git truly cannot read now
fails loudly through assertGitSuccess instead of blanking a file.

Client side, a chunk with a binary marker and no hunks now renders an explicit
placeholder in both the all-files view and the single-file viewer, so an empty
card can never again pass for "no changes here".

AI-assisted.
2026-08-06 00:47:19 -07:00
Michael Ramos 795f381ebe feat(review): "All changes" git-status review view (#990)
* feat(review): "Since main" git-status review view + correctness fixes

Adds a composite `since-base` diff — merge-base(origin/main, HEAD) vs the
working tree, plus untracked — as the default code-review view, rendered as a
three-section "git status" panel (Committed / Changes / Untracked) with a
Sections|Tree toggle, a first-run setup chooser, and a "baseline behind GitHub"
fetch banner. Everything normalizes to one git patch, so the diff viewer,
annotations, and feedback path are unchanged.

New diff type wired through runGitDiff / fingerprint / file-content / context /
staging / agent-context in packages/shared + both server runtimes (Bun +
Pi mirror). Default flipped from `unstaged` to `since-base`; the old
DiffTypeSetupDialog is replaced by ReviewSetupDialog (view + default-diff
chooser, screenshots, Settings access).

Includes a reviewed batch of correctness fixes:
- P0: baseBehindRemote was permanently true (rev-parse missing --verify)
- fingerprint blind to quoted/unicode untracked paths (unquote)
- sections parser: record both sides of a rename; staged-delete wins over
  untracked (rm --cached collision)
- graceful degrade when merge-base can't resolve (trunk/no-remote repos)
- /api/fetch-base re-queries remote (narrow-refspec honesty)
- /api/diff/switch concurrency guard (diffSwitchEpoch) + draft rekey
- decouple remote-staleness probe from initialBase (Pi parity)
- keyboard file nav in the sections view; banner gated to base-relative modes

See adr/decisions/005-since-base-github-view-default-20260701-223706.md

* fix(review): since-base review-round hardening (11 fixes, both runtimes)

Addresses the multi-agent + PR-990 review findings:

- Sections rename parser split ` -> ` on the raw quoted porcelain token, so a
  filename containing ` -> ` tore into garbage and a dirty file could render as
  "Committed". Now quote-aware (splitPorcelainRename + unit tests).
- /api/diff/switch captured the epoch AFTER await req.json(), letting a
  slow-body older switch overwrite a newer confirmed one. Epoch now captured
  before any await; hideWhitespace committed only on win. Both runtimes.
- Unresolvable base (trunk / no origin/HEAD) no longer auto-defaults to a
  degraded since-base that hides committed work — getGitContext only offers
  since-base when the base ref resolves, so the default falls through to
  uncommitted. Fingerprint + file-content degrade to HEAD to match the diff.
- git rm --cached no longer yields two diff entries for one path (untracked
  files already in the tracked patch are dropped) — fixes wrong-file-open and
  j/k nav looping in the Sections panel; also fixes latent uncommitted/unstaged.
- Settings' "Default Diff View" list now preserves the sections<->since-base
  coupling (can't leave an invalid sections+classic pair).
- "Behind GitHub" banner only treats origin/* as fetchable; a bare local base
  ("main") is upgraded to its tracking ref at startup so Fetch can clear it.
- hashUntracked resolves untracked paths against the repo toplevel, not cwd, so
  a review launched from a subdirectory isn't blind to untracked edits.
- Agent review instruction now tells agents to enumerate/inspect untracked files.
- Terminology: one "Committed changes" label; the live "Since <base>" label is
  dynamic (matches the header); "Since main" kept only as product copy.
- Docs: CLAUDE.md endpoints/fields + since-base view section; ADR recap.

Verified: typecheck (all projects), bun test 1793 pass, and empirical repro of
the rm --cached dedup, trunk->uncommitted fallback, and rename parser.

* fix(review): restore "(PR view)" on the Committed-changes label (unify toward it, not away)

* fix(review): close since-base coverage gaps from PR-990 review round 2

- First-run setup no longer forces since-base on repos where it isn't available
  (base ref can't resolve). getGitContext omits since-base there; the first-run
  block now checks the same availability before resetting the default or showing
  the chooser, so committed work isn't silently hidden on trunk/no-origin repos.
- reviewBase now includes 'since-base', so changing the base while in the
  git-status view passes the selected base to /api/file-content — expandable
  context is fetched from the right merge-base (was falling back to default).
- Settings now mirror ReviewSetupDialog's coupling exactly: Sections ⇒ force
  since-base; Tree ⇒ leave the diff (Tree + since-base is valid and now
  saveable); classic diff ⇒ Tree; since-base diff ⇒ leave the view. Removes both
  coercions that previously discarded a supported preference.

* fix(review): PR-990 review round 3 — fingerprint/dedup/base-canonicalization

- Fingerprint now uses `git status --porcelain -uall`, so editing a file inside
  a brand-new untracked directory changes the fingerprint and the "Diff out of
  date" banner fires (was collapsed to `?? dir/` and hashed as unreadable).
- extractTrackedPatchPaths pairs `---`/`+++` and excludes only the file's KEYED
  path (new side, or old side for a pure deletion) — no longer drops a recreated
  rename-source file (`git mv a b && touch a`). Verified rm --cached still
  dedups to one entry and the recreated file still shows.
- resolveReviewBase canonicalizes a bare local default name ("main") to its
  tracking ref ("origin/main") on every call, so a client that loaded before the
  startup upgrade resolved can't revert the server to the stale local base on the
  next refresh/switch. Both runtimes (new Pi resolveReviewBase helper).
- Removed the dead activeDiffLabel prop chain (FileTree -> DiffTypePicker).
- ADR recap: documented the staleness-banner scope limitation (default base only).

Verified: typecheck (all projects), bun test 23/23 review-core+fingerprint pass,
empirical repro of the untracked-dir fingerprint, rm --cached dedup, and
rename-recreate cases.

* fix(review): PR-990 review round 4 — dedup content, staging gate, banner timing

- Dedup rewrite: instead of dropping the untracked side of a same-path collision
  (which hid content), drop the tracked DELETION block and keep the untracked
  working-tree content. `git rm --cached f` + edit now shows the new content, not
  a phantom deletion — still one entry per path (no dock/nav collision). New
  stripHeaderPath also strips git's trailing-tab metadata, so space-named files
  dedup correctly. Verified: rm --cached+modify shows content (1 entry),
  space-named (1 entry), rename+recreate still shows both.
- All-files (and the `a` shortcut on the focused file) now gate staging per-file:
  committed files in since-base mode are not stageable, matching SectionsPanel /
  FileTreeNode. Shared isPathStageable helper threaded via ReviewStateContext →
  AllFilesCodeView. Stops the confusing `git add` no-op that still flipped local
  staged/viewed state.
- /api/diff/switch now awaits recomputeBaseBehindRemote before building the
  response, so the "Baseline behind GitHub" banner reflects the new base
  immediately (no ~5s lag switching in, no stale banner switching away). Both
  runtimes.
- ReviewSetupDialog re-applies the recommended default only on first-run dismiss,
  not when reopened from the header menu (was snapping a mid-session diff back).
- SectionsPanel "N added" header now counts sidecar-staged files too, so it
  matches the staged dots on rows.

Verified: typecheck (all projects), bun test 1830 pass, empirical dedup repro,
both bundles build.

* refactor(review): reuse parsePatchPathToken in removeTrackedDeletions (self-review)

Drop the duplicated stripHeaderPath helper — the shared diff-paths
parsePatchPathToken already strips a/|b/ prefix + C-quoting + git trailing-tab,
and verifies the prefix instead of blindly slicing two chars. Documents the
binary-deletion edge (no --- line, so a binary rm --cached is not deduped).

* fix(review): PR-990 review round 5 — mixed-base + staging/sort/flicker nits

- Atomic base upgrade: the startup origin/* canonicalization swapped currentBase
  without rebuilding the patch, so /api/diff could advertise origin/main while
  the served hunks came from local main (mixed-base review on origin/HEAD-absent
  repos). Now rebuilds the diff for the new base and commits base+patch+ref+
  fingerprint together (only if no user switch happened); the fingerprint change
  makes the client's freshness poll pick it up. Both runtimes.
- isPathStageable: gate staging OFF when since-base is active but the sidecar
  hasn't loaded (was falling through to true, allowing a git-add no-op on a
  committed file).
- SectionsPanel: session-staged files now float to the top of Changes — the sort
  key was `false ?? stagedFiles.has(...)` which short-circuits; now ORs them.
- /api/diff/fresh: early returns now carry baseBehindRemote, so a snapshot change
  mid-probe no longer clears the "behind GitHub" banner for one poll. Both runtimes.
- Removed the now-dead activeLabelFallback prop from DiffTypePicker (FileTree
  stopped forwarding it in round 3).

Skipped (agreed): the fetch-base input-validation nit (no realistic attacker)
and the diff-switch TOCTOU (concurrent switches aren't UI-reachable).

Verified: typecheck (all projects), bun test 1830 pass, both bundles build.

* refactor(review): drop redundant staged-OR in SectionsRow (self-review)

item.staged (the grouping key) already ORs in stagedFiles as of the round-5
sort fix, so the row-level `|| stagedFiles.has(...)` is dead weight.

* fix(review): surface the startup base upgrade to already-loaded clients

Review round 6 fixes:

- The startup main -> origin/main upgrade re-baselined the freshness
  fingerprint, so a client that fetched /api/diff before the rebuild kept
  the old patch and every /api/diff/fresh probe reported fresh (the probe
  compares server state to itself, never to what the client renders). Now
  the fingerprint is only re-baselined when no client has loaded the
  pre-upgrade snapshot; otherwise the stale baseline trips the normal
  "Diff out of date - Refresh" banner. Bun + Pi.
- preserveFile refreshes (staleness Refresh, post-Fetch) now adopt the
  server's returned base — exactly the paths where the server may have
  canonicalized main -> origin/main; keeping the old name sent
  /api/file-content and Ask AI context against the wrong base.
- ReviewSetupDialog: clamp the fixed 800px height to the viewport so the
  dialog fits on small laptop screens.
- Docs: /api/diff/fresh response also carries baseBehindRemote/agentCwd.
- Documented the committed-deletion + untracked-recreation dedupe edge as
  accepted (code comment + ADR) — fixing it needs two same-path diff
  entries, which the path-keyed UI cannot represent.

* fix(review): stop the Git-status default from silently reverting to tree

Users with reviewPanelView=sections could keep opening in the tree view on
a stale diff type despite their cookie saying Git status. Three causes:

- The header Sections toggle persisted the view but not defaultDiffType,
  creating a conflicted pair (sections + non-since-base default) that every
  UI writer is supposed to prevent. It now couples the diff default like
  the setup dialog and Settings do.
- configStore's debounced POST /api/config could be lost when a session
  closed within 300ms of a change, leaving cookie and config.json split.
  Pending writes now flush on pagehide with a keepalive fetch.
- configStore.init() applies config.json over the cookie without the UI
  coupling, so a stale server value re-corrupted the pair on every load.
  The app now self-heals at load: if the view says sections but the diff
  default isn't since-base, it repairs the default (cookie + config.json)
  and switches the live session to since-base.

* fix(ui): keep the stage (+) button border visible on the active file row

The button's --border border has no contrast against the active row's 30%
primary tint, so it vanished on the selected row until hovered. Tint the
border with the row's primary color on active rows (both sections and tree
views); the button's own hover border still applies.

* ui(review): spell out the panel view toggle — 'Git status | Tree' text instead of icons

* fix(review): PR-990 review round 7 — export label, diff snapshot race, unicode paths

- exportFeedback describeDiff(): add the missing since-base case — every
  feedback export in the new default mode read "**Diff:** since-base".
- /api/diff GET: snapshot patch/base/ref/error BEFORE the sections-sidecar
  await and pass the pinned base into buildSectionsSidecar. The startup base
  upgrade landing mid-await could pair a rebuilt patch with sections grouped
  against the old base, with initialDiffServed still false so no refresh
  banner ever came. Bun + Pi.
- SectionsPanel: a session-staged untracked file now moves to the Changes
  section immediately (anticipating the server's next sidecar) instead of
  sitting in Untracked with a staged dot until refresh.
- unquoteGitPath: real C-style unquoting with octal (UTF-8 byte) escape
  decoding. JSON.parse rejects octal escapes, so non-ASCII names kept their
  literal \303\251 form — an untracked "café.txt" was silently ABSENT from
  the review (git diff --no-index could not access the quoted name) and the
  deletion dedupe Set lookup could never match. getUntrackedFileDiffs now
  unquotes ls-files output. Unit tests: octal decoding + end-to-end unicode
  untracked file in a real repo.
- review-core.test: worktree subtype round-trip now covers since-base and
  all (was 6 of 8).

Parked (deliberate): SectionsPanel/FileTree keyboard-nav dedup refactor.

* fix(shared): unquoteGitPath keeps literal unicode intact (self-review)

The byte-collector treated literal non-ASCII code units as single bytes,
which would mojibake headers synthesized by our own quoteGitPath
(JSON.stringify leaves unicode unescaped inside quotes — workspace-mode
prefixed headers round-trip through the same parser). Literal chars now
append as string code units (surrogate-safe); only octal escapes go
through the UTF-8 byte decoder.

* fix(review): PR-990 review round 8 — pre-staged toggles, explicit base, AI context race

- useGitAdd: session Set replaced with a tri-state override map folded over
  the sections sidecar; stagedFiles is now the EFFECTIVE staged set (sidecar
  + session stages - session unstages). Fixes pre-staged files: the first
  `a` press actually unstages (was a git-add no-op), the sidebar dot clears
  after a real unstage (was stuck via sidecar OR), and the All-files header
  agrees with the sidebar on load. Overrides reset whenever a fresh sidecar
  arrives (switch, preserveFile refresh, PR response) so stale session
  intent can't fight new porcelain truth. SectionsPanel drops its own
  sidecar OR; stagedCount = effective size.
- Explicit base picks are honored verbatim: the picker sends explicitBase,
  and the server permanently disables local-name -> origin/* canonicalization
  once set (the local/remote groups are distinct choices). The behind-GitHub
  banner also exempts an explicitly-picked local name — Fetch advances
  origin/*, so the banner would be un-clearable nagging. Bun + Pi.
- buildCurrentAiReviewContext(patch, base): GET /api/diff builds Ask AI
  context from the same served snapshot as the patch — the startup base
  upgrade could hand Ask AI a different changeset than the screen. Bun + Pi.
- recomputeBaseBehindRemote: capture remoteDefaultInfo once — a concurrent
  refresh nulling it mid-await threw. Bun + Pi.
- Revert stray "Status Update / Updated!" edit to adr/0001 (test debris
  swept into the round-2 commit).
- splitPorcelainRename comment corrected (porcelain v1 does NOT quote plain
  spaces; a name containing " -> " is ambiguous without -z) + ADR notes for
  the index-only-changes semantics and the rename edge.

* fix(review): FileTreeNode uses the effective staged set — round 9

Round 8 made stagedFiles the effective set (sidecar + session overrides)
and removed SectionsPanel's sidecar OR, but missed the same OR in
FileTreeNode's since-base row (sectionStaged). In the Tree fallback a
pre-staged file unstaged this session kept its dot and the next toggle
re-staged it. Grep-swept: this was the last surviving sidecar-staged OR.

* refactor(review): make the staged-display invariant unrepresentable

The round-8/9 bug class (sidecar staged flag ORed over the effective set)
existed because surfaces had a second staging source to reach for. Remove
it: stagedFiles is now a REQUIRED prop on SectionsPanel/FileTree/
FileTreeNode and the optional-prop fallback branches reading
sectionEntry.staged for display are deleted — a future surface cannot
reintroduce the OR because the pattern no longer exists to copy. The
sidecar type's staged field and AGENTS.md now document the invariant at
the point of temptation. Grep for display reads of .staged now hits only
useGitAdd (owner) and the sidecar builder (producer).

* refactor(review): self-review cleanups on the staged-invariant hardening

- orderFilesBySections: document why its snapshot .staged read is safe
  (only called at sidecar-fresh moments where snapshot = effective) and
  that it must not be reused mid-session — the one remaining display-side
  snapshot read the invariant sweep surfaced.
- FileTreeNode: drop the now-pure sectionStaged alias; use isStaged.

* fix(review): PR-990 review round 10 — header staging gate, fingerprint cap, single-writer coupling

- Single-file diff header now uses the per-path staging gate (canStagePath),
  closing the last ungated staging trigger: committed-only files in
  since-base offered a no-op Git Add that flipped local staged/viewed state.
  Full trigger inventory swept: App `a` shortcut, all-files `a` + header,
  SectionsPanel rows, FileTreeNode rows, single-file header — all six now
  per-path gated or group-gated; no context-menu staging exists.
- Fingerprint circuit-breaker: the freshness poll's `git status --porcelain
  -uall` degrades permanently (per cwd, per process) to collapsed -unormal
  once its output exceeds 2MB — a forgotten node_modules/ no longer burns
  CPU every 5s for the whole session. Costs untracked-dir edit sensitivity
  only on such repos; one possibly-spurious staleness banner at the switch.
- The sections ⟺ since-base coupling now has a single writer:
  setReviewPanelView/setReviewDefaultDiffType in @plannotator/ui/config.
  All five call sites (setup dialog x2, Settings x2, header toggle,
  first-run reset, self-heal) converted; grep for direct writes of either
  setting now hits only reviewView.ts.
- /api/diff/switch responses pass snapshot args to the AI context builder
  (both branches, Bun + Pi) — correct today, now robust against future
  awaits between the epoch check and the response.
- Docs: explicitBase in the /api/diff/switch body.

Parked per discussion: SectionsPanel/FileTree nav+search+footer dedup
(extract when the commit-list view adds a third panel), querySelector row
measurement, fetch-base stderr passthrough (standing decision), Pi
hasAgentLocalAccess (pre-existing follow-up list).

* fix(review): PR-990 review round 11 — subdirectory launches, escape decode, settings note

- Repo-root-relative patch paths now resolve against the git toplevel
  everywhere they meet a filesystem path or pathspec, via a shared
  resolveRepoToplevel helper: file-content working-tree reads (hunk
  expansion returned null from a subdirectory launch) and gitAddFile/
  gitResetFile (stage/unstage failed with pathspec errors). Both bugs
  pre-existed for uncommitted/unstaged; since-base made them the default
  experience. The two existing inline toplevel resolutions (untracked
  diffs, fingerprint) now use the same helper. Shared code — Pi inherits
  via vendoring. Real-repo subdirectory tests for both.
- unquoteGitPath decodes \uXXXX (JSON.stringify emits it for control
  chars without a short escape; our synthesized workspace headers
  round-trip through this decoder). Malformed \u stays literal. Tests.
- Pi explicitBase guard matches Bun byte-for-byte on empty-string base.
- Settings Git tab notes when the CURRENT repo can't serve the Git-status
  view (base ref unresolvable) instead of letting the preference look
  silently broken — it's a global preference, so the options stay.

Same-class items left parked (pre-existing, untouched by this PR):
open-in root resolution and code-nav file reads from subdirectory
launches — on the follow-up list with the Pi divergences.

* fix(review): PR-990 review round 12 — per-client freshness, keyboard-operable row controls

- Freshness is now judged PER CLIENT: every patch-carrying response
  (/api/diff, /api/diff/switch, pr-switch, pr-diff-scope) includes
  snapshotId (the server's draftKey), and the client echoes it on
  /api/diff/fresh probes. A mismatch reports stale for THAT client
  regardless of the VCS fingerprint. This fixes the round-12 finding —
  reloads/second tabs after the startup base upgrade got a permanently
  bogus staleness banner from the shared pre-upgrade baseline — and
  deletes the round-6 conditional re-baseline hack entirely (the
  fingerprint recaptures unconditionally again; the old client's banner
  now comes from its snapshot mismatch, not a deliberately stale
  baseline). Also gives unfingerprintable modes (P4, PR layer) snapshot-
  level staleness for free. Bun + Pi + client hook.
- StageControl and ViewedControl (which had the identical gap) are
  keyboard-operable: tabIndex + Enter/Space activation + focus outline.
  They're spans inside the row <button> (nested real buttons are invalid
  HTML), so they need their own focus stop; the a/v shortcuts remain the
  power path.
- Docs: snapshotId on /api/diff, ?snapshot= on /api/diff/fresh.

* fix(review): PR-990 review round 13 — re-key snapshots on scope switch, PR-tab refresh

- The PR scope switch now re-keys draftKey (= snapshotId + draft storage
  key) at BOTH commit points, unconditionally — matching every other
  snapshot commit site. The full-stack branch previously kept the layer
  patch's key: stale layer tabs never got the banner after a cross-tab
  scope switch (the exact case snapshotId exists for), and full-stack
  drafts collided with layer drafts (pre-existing). The layer branch's
  !layerPatchIncomplete conditional is gone — it only stayed consistent
  because full-stack never re-keyed. Invariant now: every currentPatch
  commit is followed by a re-key. Bun + Pi.
- Refresh works for any stale PR tab: re-selects the CURRENT scope
  instead of no-opping for layer (only full-stack could go stale in the
  fingerprint-only world; snapshot mismatch changed that). Accepted
  residual (documented in code): after a cross-tab PR switch, refresh
  updates the patch but not prMetadata — the scope endpoint doesn't
  carry it, and the full-rehydrate refactor isn't worth the two-tab edge.

* fix(review): stale incomplete-layer Refresh uses the non-blocking upgrade path (self-review)

Round 13 made Refresh re-select the current scope for stale PR tabs; for
an INCOMPLETE layer patch that POST triggers the server's local recompute,
which can park for minutes behind checkout warmup — and
handlePRDiffScopeSelect renders the full-screen switch overlay the whole
time. Route that case through handleLoadFullDiff (same POST, progress
notice instead of modal), which exists for exactly this slow path.

* fix(review): round 14 — composite snapshot id, fully-pinned /api/diff, "All changes" label

- snapshotId is now content hash + diff type (+ PR scope), built by a
  single currentSnapshotId() helper used at every response site and the
  freshness compare. A cross-tab MODE switch with a byte-identical patch
  (layer vs full-stack on a single-PR stack) now flags old tabs; the base
  is deliberately excluded so a same-commit main -> origin/main
  canonicalization stays banner-silent (round-12 noise-avoidance kept).
  draftKey stays a pure content hash — drafts survive content-identical
  round-trips. Bun + Pi.
- GET /api/diff pins ALL served fields (diffType, hideWhitespace,
  prDiffScope join patch/base/ref/error/snapshotId) and the sidecar + AI
  context builders take the pinned type instead of reading globals — a
  concurrent tab switch during the sidecar await can no longer produce a
  since-base patch labeled with another mode. Bun + Pi.
- First-user feedback: the since-base label is now plain English —
  "All changes since origin/main" (dynamic, follows the picked base);
  dialog/Settings short form "All changes"; feedback exports match.
  uncommitted reverts to "Uncommitted" where it had borrowed "All
  changes", so the two stay distinguishable side by side.

* docs(review): finish the Since-main -> All-changes terminology sweep in comments (self-review)

* fix(review): PR-990 review round 15 — base-revert race, freshness reset, unstage regroup

- resolveReviewBase gains a second, probe-independent rule: a non-explicit
  echo of the bare local name of the CURRENT origin/* base stays on the
  tracking ref. The existing rule keys off remoteDefaultInfo, which comes
  from a second network probe that can lag the startup upgrade by seconds;
  in that window a diff-type/whitespace switch echoing "main" committed
  the session back onto the stale local branch and set baseEverSwitched,
  permanently blocking the upgrade. Bun + Pi.
- useDiffFreshness resets stale/dismissed state on snapshotId too — since
  round 14 a new snapshot can reuse identical patch text with a different
  id, and the old banner state wrongly carried over until the next poll.
- SectionsPanel: unstaging a PRE-staged add moves it back to Untracked
  (mirror of the round-7 stage regroup). Detection deliberately uses the
  sidecar's snapshot flag to recognize "was pre-staged"; staged
  modifications stay in Changes, staged renames remain a refresh-heals
  edge.

* fix(review): PR-990 review round 16 — rename staged count, guarded fetch-base replay

- "N added" no longer double-counts staged renames: the sidecar truthfully
  marks BOTH porcelain sides staged, but an above-threshold rename renders
  as ONE file — the hidden old path inflated the count and left a phantom
  effective-staged entry that unstaging the visible row couldn't clear.
  The client now filters sidecar-staged paths to rendered files, which is
  correct in both patch shapes (below-threshold renames render delete+add
  as two rows and both sides pass the filter). Client-only; the sidecar
  stays faithful to porcelain.
- The fetch-base completion only replays the diff refresh if the user's
  diff type/base selection is unchanged since the click — a slow fetch no
  longer yanks the review back to the view captured at click time.

Declined with reasons (in review thread): first-run persist-before-confirm
(approved forced default, second flagging), explicit-base flag ordering
(early-set preserves user intent; commit-after-win would leave picker and
server disagreeing), base-picker revert target (near-unreachable compound
race, deferred).

* fix(review): PR-990 review round 17 — find origin/main on feature-only clones

getDefaultBranch's chain (origin/HEAD -> local main -> blind "master")
skipped the fetched remote-tracking ref entirely. On checkouts with no
origin/HEAD symref and no local main/master — CI checkouts, `clone
--branch feature`, extra worktrees — it guessed "master", the base
didn't resolve, getGitContext suppressed since-base, and the flagship
"All changes" view silently disabled itself for the whole session even
though origin/main was fetched and diffable. The startup upgrade can't
rescue it either (its guard reads the bogus "master" as a deliberate
non-default base).

Chain is now: origin/HEAD (verified) -> origin/main -> local main ->
origin/master -> "master" — remote-tracking refs preferred, matching the
function's stated prefer-upstream intent. Shared core (Pi inherits);
real-repo test reproducing the exact clone shape.
2026-07-04 09:53:49 -07:00
Oscar Silva f08764063d feat(review): support multi-repo workspace reviews (#543)
* feat(review): support multi-repo workspace reviews (#527)

* fix(workspace): address critical issues from deep review

- Fix race condition in label generation by pre-computing labels sequentially
- Fix rewritePatchLine to support quoted paths and rename/copy headers
- Add separator between aggregated patches to avoid invalid diffs
- Normalize input paths in resolveWorkspaceFilePath
- Add timeout to PR discovery (15s) to prevent server hangs
- Fix PATCH /api/workspace/repo to rollback state on failure via applyRepoMutation
- Validate body.source runtime (must be 'local' or 'pr')
- Snapshot active repo in agent jobs at launch to prevent race in onJobComplete
- Prevent double-prefixing of agent findings when paths are already prefixed
- Fix frontend findWorkspaceRepoForPath to use longest-prefix matching
- Fix shared types: diffType uses DiffType, platformUser is string | null

* fix: remove duplicate gitRuntime export in vcs.ts

* fix: resolve remaining merge conflict in local review mode, remove stale detectManagedVcs import

* Add local multi-repo workspace review support

* Fix workspace review edge cases

* Remove session query from browser launch

* Add switchable workspace review modes

* fix(review): cover opencode workspace bridge

* fix(review): clean workspace review plumbing

* fix(review): clarify workspace agent finding paths

* fix(review): preserve diff paths with spaces

---------

Co-authored-by: Michael Ramos <mdramos8@gmail.com>
2026-06-05 14:29:38 -07:00