2 Commits

Author	SHA1	Message	Date
Michael Ramos	3ebb28a835	fix(review): avatar lookups can no longer delay /api/commits ... The gh/glab runner has no subprocess timeout, and the commits endpoint awaited avatar resolution before responding — a hanging network (proxy black-hole, dead DNS) held the already-computed commit list hostage to decoration. resolve() now races a 4s ceiling: on expiry the endpoint returns with the initials fallback, the in-flight fetch finishes in the background, and later calls pick its results out of the cache. Timed-out attempts memoize no misses. Shared module, so both runtimes get it.	2026-07-05 11:50:41 -07:00
Michael Ramos	e8df06db7c	feat(review): Commits panel — linear history rail with per-commit diffs (#994) ... * docs(adr): spec for the commit-list review view (linear history rail) * feat(review): commit:<sha> diff mode along the shared diff-type seams A new git-only diff family for reviewing one historical commit against its first parent (git-show style), threaded through every seam a diff type crosses: - runGitDiff: git diff <sha>^ <sha>; a root commit diffs against the empty tree. Label is 'Commit <shortsha> — <subject>'. The sha is validated as bare hex (parseCommitDiffType) before it reaches any argv position, on top of --end-of-options. - getGitDiffFingerprint: anchored to the sha alone (present/gone), NOT headSha — new commits landing mid-review don't change this diff and must not raise the staleness banner. A vanished commit (rebase + gc) flips present→gone and fires it honestly. - getFileContentsForDiff: old side <sha>^ (null on root), new side <sha>. - parseWorktreeDiffType learns worktree:<path>:commit:<sha> (the sub-type contains a colon, so it needs its own split) — commit review composes with worktree sessions like every other mode. - vcs-core: the git provider owns commit:*; staging stays gated off (canStageFiles allowlist unchanged — nothing in a historical commit is stageable). - Ask AI context: a commit-mode inspect string ('git show <sha>', with an explicit 'historical commit, not the working tree' note). Also adds listCommitHistory for the upcoming /api/commits endpoint: one --first-parent page from HEAD (before=<sha> continues at its first parent, +1 fetch for an honest hasMore), with isHead / isRepoUser / isPastBase flags. isPastBase is reachability from the base — computed as the complement of 'rev-list --first-parent HEAD ^base', which is a prefix of the walk, so the client's single divider is exhaustive. An unresolvable base degrades to no divider (matches since-base's posture on such repos). * feat(review): GET /api/commits — linear history endpoint (Bun + Pi) One page of the branch's --first-parent history (?limit=&before=), served by listCommitHistory against the active diff's cwd (worktree- aware via resolveVcsCwd) and the active base (so the divider tracks the same baseline the review compares against, including the startup origin/<default> upgrade). Gated to plain local git sessions — PR / workspace / jj / p4 return 400, mirroring the client's commitsCapable gate. Both runtimes; the route-parity test covers the pair. commit:<sha> switching needs no endpoint changes: /api/diff/switch already dispatches through runVcsDiff (the git provider owns the new family), the epoch guard applies as-is, the sections sidecar correctly stays since-base-only, staging 400s via the canStageFiles allowlist, and baseRelevantDiffType keeps the behind-GitHub banner suppressed for commit diffs. * feat(ui): Commits panel view — the linear history rail Third left-panel view: 'Git status | Commits | Tree'. The Commits view is a pure commit list (short sha, subject, relative age compacted to '2h', author only when it isn't the repo user, HEAD dot, a divider where the branch meets the resolved base, 'Show more' paging). It never becomes a file list: clicking a commit switches the diff to commit:<sha> and the existing needsInitialDiffPanel flow lands the center dock on the all-files surface; re-clicking the active commit just re-focuses that panel. Persistence: reviewPanelView gains 'commits' with NO diff coupling — the view persists, the diff opens on the user's normal default until a commit is clicked, and a sha is never persisted (it may not survive a rebase). The sections⟺since-base coupling is untouched, but the classic-diff→Tree snap in Settings/ReviewSetupDialog now only fires when the current view is 'sections', so picking a default diff no longer stomps a Commits preference. The load-time self-heal ignores 'commits' by construction (it keys on panelView === 'sections'). Gates: commitsCapable = plain local git session (no PR / workspace / jj / p4, matching the server's /api/commits gate); the segment is hidden elsewhere. Staging is inert in commit diffs on both ends (STAGEABLE_DIFF_TYPES and the server allowlist never match commit:*). Worktree sessions compose — the client parses worktree:<path>:commit:<sha> and handleDiffSwitch prefixes as usual; the rail refetches on worktree/base changes but deliberately not on commit clicks, so paging state survives selection. Accepted edges (v1): no j/k selection in the rail (each move would run a full diff switch); toggling Commits→Tree keeps a commit diff active, where the tree's diff-type dropdown marks nothing — the gitRef header still names the commit. * fix(review): self-review cleanups on the commit-list feature - listCommitHistory: a repo with no commits yet (unborn HEAD) returns an empty page instead of null — the panel showed a 500-backed 'Retry' error where 'No commits' is the truth. Every other review surface already degrades gracefully on a commit-less repo; now this one does. Only a genuinely unanswerable repo (not a repo at all) stays null. - Hoist the bare-hex sha rule into one BARE_HEX_SHA_RE shared by parseCommitDiffType and the before-cursor validation (was written twice). - The 'all' diff case now uses the getEmptyTreeSha helper the commit mode extracted, instead of keeping its own inline copy. - Drop CommitsPanel's isLoadingDiff prop — declared and passed but never used. * feat(ui): commit cards, labeled groups, avatars — review round 1 Four items from the live review round: - Toggle: Commits moves to the far right (Git status | Tree | Commits) and the segment text goes 10px → 12px (padding trimmed to keep three segments inside the 256px header). - The bare '── origin/main ──' divider confused more than it explained. Replaced with two labeled groups: 'On this branch' (commits not yet reachable from the base) and 'In <base>' (shared history), each with a tooltip spelling out what the boundary means. - Rows become overview cards: subject (2-line clamp) on top; a meta row with author avatar, author name (only when it isn't the repo user), a HEAD badge, short sha, and compact age. Active card gets the primary border/tint. - Author avatars, reusing the PR machinery rather than reinventing it: parseRemoteUrl/parseRemoteHost classify the origin forge; the gh/glab invocation shape (incl. the --hostname self-hosted convention) and GitLab's relative-avatar absolutization rule are mirrored from pr-github/pr-gitlab. What could NOT be reused directly: PR avatars are keyed by platform login, which local commits don't have — so GitHub resolves via one repos/{o}/{r}/commits?per_page=100 call per session building an author-EMAIL → avatar map (unpushed commits by the same author resolve through it; verified live), and GitLab uses its per-email /avatar endpoint (capped, deduped). Everything is memoized per session and fails closed to the initials fallback — a repo with no remote, an opaque self-hosted host, or an unauthenticated CLI never delays or errors the commits endpoint. The Avatar component moves out of PRCommentsTab into its own module, now shared by the PR timeline and the commit cards. New shared module commit-avatars.ts is vendored to Pi (vendor.sh) and the enrichment runs in both runtimes. CommitListEntry gains authorEmail (resolver key) and avatarUrl (server-enriched). * ui(review): flush commit rows, prominent base boundary — review round 2 - Cards are gone: rows stack flush (no borders, no margins), hover tint only. Subject is strictly one line, ellipsized (no clamp/wrap). - Author name always shown (was: only when it differed from the repo user), with the avatar, on the compact meta line under the subject. - The base boundary is a prominent labeled rule ('— In origin/main —', foreground-weight lines) instead of the faint border under a group header; the 'On this branch' header above stays quiet. * docs(adr): evaluate local stack-parent detection — scenario matrix, no build decision Empirical stress-test of the merge-base-reachability algorithm across 9 scenarios plus this repo itself. Findings: the stacked-ness test is graph truth (never lied); parent attribution has one content-harmless failure (same-merge-base sibling labels) and one dangerous, provably graph- undetectable one (branches pointing inside B's own history win and silently hide the user's commits). Determination: only viable as a suggest-and-confirm affordance with per-branch persisted choice — never silent auto-defaulting. Decision to build deliberately left open. * fix(ui): Commits view is session-only — never the opening view Live-review catch: the panel toggle inherited the sections/tree 'last choice becomes the default' cookie write, so clicking Commits in one session made the NEXT review open on the history rail instead of the diff. Ruling: a review always opens on the persisted sections/tree default; Commits is entered via the toggle each session. - reviewPanelView narrows back to 'sections' | 'tree'; a stale 'commits' cookie is treated as unset (self-heals to the default). - The live view is now a session overlay in App state: selecting Commits flips it without touching config; selecting Git status/Tree clears it and persists as before. Single choke point in selectPanelView, so every toggle site inherits the rule. - Settings loses the Commits segment (it configures the OPENING view, which Commits can no longer be). Spec updated with the revision. * ui(review): the panel toggle is session-only — it never writes the default Second live-review ruling on persistence: the header toggle shouldn't persist ANY choice, not just Commits. Looking at another view mid-review must not silently change what the next review opens on. - selectPanelView becomes a pure session override (local state layered over the persisted value); no configStore writes from the toggle path. - handleSwitchToSections drops its defaultDiffType write — that write only existed to keep the persisted view/diff pair consistent, and the toggle no longer persists a view. Settings and the setup dialog remain the only reviewPanelView/defaultDiffType writers and keep enforcing the sections ⟺ since-base coupling; the load-time self-heal still repairs conflicted pairs those writers may leave. - Spec + settings-registry comments updated to name the new rule. * feat(ui): commit description card + collapsed all-files for commit diffs Clicking a commit now answers 'what and why' before 'which lines': - New commitInfo sidecar (Bun + Pi, same mode-conditional shape as sections): when a commit:<sha> diff is active, /api/diff and /api/diff/switch carry the commit's full metadata — subject, multiline body, author (+ avatar via the session resolver), sha, age — from a single 'git show -s' (getCommitDiffInfo in review-core, body as the trailing format field so newlines survive). - CommitDescriptionHeader heads the all-files surface: subject, author row, and the full body rendered as markdown via the same MarkdownBody the PR viewer uses. Long bodies scroll inside the card (max-h) so the diff keeps the viewport. - Commit diffs open FOLDED: AllFilesCodeView gains defaultCollapsed — items seed collapsed at build time, the seed is part of fileSetKey (CodeView seeds once per instance, so a seed change must remount), and the collapse-all toggle state initializes to match. Every other diff mode keeps opening expanded; leaving the commit diff clears the card and the folded default together (the sidecar is absent). Verified live through the compiled binary: switching to a real commit returns subject/body/avatar, switching back to since-base clears it. * ui(review): commit description scrolls with the diff — no pinned card Round feedback: the fixed header with its own inner scrollbar felt embedded; the description should read as the top of the document and scroll away with the files. - AllFilesCodeView gains leadingContent: rendered via a portal INTO CodeView's scroll container (absolutely positioned at content top, so it participates in the scrollable overflow), with its measured height fed into layout.paddingTop so the virtualized items start below it. CodeView stays the single scroll authority — no nested scrollers, no wrapper flex row. - CommitDescriptionHeader shows the full body inline (no inner OverlayScrollArea). Only genuinely huge bodies (>24 lines / >1800 chars) get a CSS max-height clamp with a fade mask and a Show more/Show less toggle — CSS clamping keeps the markdown intact, and the ResizeObserver feeds the expanded height back into paddingTop. Keyed by sha so the toggle resets per commit. * ui(review): entering the Commits view auto-opens the HEAD commit Toggling to Commits used to leave the previous mode's all-files diff on screen — a rail full of commits next to content that belonged to none of them. Now entering the view selects the HEAD commit (top of the rail) so the center immediately shows its diff + description card. Once per entry, ref-guarded: an already-active commit diff survives a toggle round-trip, a user click supersedes it, and a failed switch doesn't retry-loop. When entry races the first log fetch, the effect fires as soon as the log lands. * perf/ui(review): fast commit navigation — instant veil + skip context recompute Rail clicks felt laggy and jumpy: the old diff sat on screen for the whole switch round-trip and then snapped, and the switch itself was doing a full getVcsContext recompute (branch + worktree + recent-commit enumeration — three git walks) that a commit click can never invalidate. - Same-cwd commit:<sha> switches skip the context recompute in both runtimes (the client keeps its existing context when the field is absent — long-standing contract). Measured through the compiled binary: commit switches now ~40-60ms server-side. - The center dock shows an immediate 'Loading commit…' veil whenever a commit switch is in flight (or the Commits view was just entered and HEAD auto-select hasn't landed) — the stale previous diff never shows and the click reads as instant. Errors surface through the normal empty-state, never trapped under the veil. * fix(ui): PR-994 review round — toggle/settings separation + commit-nav fixes (App) The headline fix: the load-time settings repair guarded on the live panelView, which now carries the session toggle override — so clicking 'Git status' once mid-review (saved prefs tree + a classic diff) hit the healer and silently persisted defaultDiffType='since-base'. The toggle must never be a settings writer, directly or through a repair path. Keyed to persistedPanelView now; audited the remaining configStore.set call sites — no toggle path can reach a write. Also in this file, same review round: - Commit-navigation veil gains a real predicate: it drops on a commit-log fetch error (rail shows Retry; center was stuck under the spinner forever) AND on a genuinely empty history (zero commits — the second stuck-veil trigger the automated review missed), while still covering the log-loading and pre-auto-select frames. - handleSelectCommit composes the worktree prefix once and calls fetchDiffSwitch directly (the equality check and the switch previously used two different composition paths). - commitsCapable/showCommitsPanel hoisted above the global keyboard handler; Cmd+F is no longer intercepted in the Commits view (no search input exists there — the browser find takes over instead of a silent no-op). * fix(review): PR-994 review round — commit-log staleness, loading flags, avatar cap - useCommitLog clears its cached list when the history context changes (worktree/base switched while the view was away): the HEAD auto-select acted on the stale rows and opened the previous context's commit. Same contextKey still keeps the cache (no empty flash on re-entry). - The hook's cleanup now resets both loading flags: a generation-skipped finally never cleared them, leaving 'Show more' stuck disabled as 'Loading…' after leaving mid-page. - commit-avatars memoizes misses only for emails an attempt actually covered — the GitLab per-call cap (10) was recording every email past it as permanently unresolvable for the session. Regression test added. - GitLab host detection tightened from a bare substring to a structured match (gitlab.com / gitlab.* / *.gitlab.*) — 'mygitlabproxy.example.com' no longer classifies. - Collapse-all mirror re-derives from live item state after per-file toggles: with commit diffs seeding all-collapsed, expanding one file left the dock button on 'Expand all', and clicking it re-collapsed the file the user just opened. - The commit description card element is memoized on commitInfo identity so the measuring ResizeObserver stops churning on every context render. * fix(ui): entering the Commits view ends any open search session (self-review) The Cmd+F guard from the review round only blocked NEW searches in the Commits view; a search opened in Git status/Tree survived the toggle as hidden-but-live state — the query kept matching against the commit diff, marks kept rendering in the all-files body, Enter/F3 kept stepping matches, and the input to see or edit the query didn't exist anywhere on screen. handlePanelViewSelect (the single choke point every toggle site routes through) now clears and closes search on entry to Commits. Also re-traced the rest of the round under self-review — veil terminal states (log error/empty/loaded, switch failure, background refresh with a commit active), the hook's cleanup-before-rerun flag ordering, the avatar attempted-set on broken platforms, and both collapse-mirror paths — no further findings. * fix(ui): PR-994 round 2 — live rail freshness, non-destructive errors - The rail now keeps itself fresh: while the Commits view is visible, a quiet 10s poll head-compares page 1 and adopts it only when history actually moved. The commit DIFF's sha-anchored fingerprint stays as designed (a historical commit never goes stale, so the banner correctly stays quiet) — but the rail no longer freezes while an agent commits in the background. Adoption bumps the fetch generation so an in-flight 'Show more' from the old history can't append stale rows, and transient poll failures disturb nothing. - A page-1 fetch resets isLoadingMore: a refresh superseding an in-flight paging request skipped that request's generation-guarded finally, leaving 'Show more' stuck on 'Loading…' — rare before, a live race once the poll exists. - Errors are non-destructive when a list is on screen: a failed page or background refresh renders as an inline row with Retry under the list instead of replacing the whole rail; the full-panel error state is reserved for an empty rail. * perf/refactor(review): parallel GitLab avatar lookups; drop dead isRepoUser - The ≤10 per-call GitLab avatar lookups run in parallel instead of sequentially — they sit on /api/commits' critical path, and serial subprocess spawns added seconds to the rail's first paint on multi-author GitLab histories (PR-994 round 2 nit). - CommitListEntry.isRepoUser removed: v1 showed the author only when it wasn't the repo user; the live-review pivot to always-shown names left the field computed (one git config subprocess per page), serialized, and tested but never read, with a doc comment describing behavior the panel no longer has. Spec updated to match the revised row design. * fix(ui): commit-poll adoption clears all superseded fetch state (self-review) Adopting a new history from the background poll bumps the generation, which strands any in-flight fetch's generation-guarded finally — the same stale-flag class just fixed for isLoadingMore, but for isLoading (slow first load overtaken by the poll) — and a lingering inline error from the replaced history would otherwise sit under the fresh list. Adoption now clears isLoading, isLoadingMore, and error together. * fix(ui)/docs: PR-994 round 3 — worktree switches drop commit diffs; API docs - handleWorktreeSwitch treats commit:<sha> as non-portable: every other diff mode recomputes meaningfully against the target worktree, but a commit diff is context-bound content (worktrees share one object database — the reviewer's claimed git error doesn't exist, verified empirically), so 'preserving' it just re-rendered the OLD context's commit byte-for-byte. It now falls back to the session default (same option-availability rule resolveInitialDiffType applies). Swept every other activeDiffBase/diffType carrier: the whitespace toggle, staleness refresh, and fetch-base deliberately recompute the SAME diff (valid for commits); handleBaseSelect and the base-picker row already exclude commit mode; job diff-context labeling and the guarded diff-type dropdown are display-only. This was the single leak. - AGENTS.md (CLAUDE.md symlink): /api/commits row added to the Review Server table; /api/diff and /api/diff/switch now document the commitInfo sidecar and the commit:<sha> diffType family; the since-main section describes the three-segment session-only toggle and the never-persisted Commits view. * docs: normalize arrow glyphs in the /api/commits table row (self-review) * fix/refactor(review): PR-994 round 4 — rebase-safe paging, locale-proof ages, shared parsing - Pagination is rebase-safe: a 'Show more' cursor from a history that was rewritten mid-session (rebase/force-push) still resolves in the object store but is no longer on the branch — paging from it walked the orphaned pre-rewrite chain for the ≤10s window before the freshness poll adopts the new history. listCommitHistory now ancestor-checks the cursor (merge-base --is-ancestor) and returns an empty terminal page for orphaned AND vanished cursors alike; the poll replaces the list moments later. Regression test rewrites history between pages. - Ages are locale-proof: git localizes %cr via gettext ('vor 2 Stunden'), which the English-only compactAge regex silently couldn't shorten. CommitListEntry/CommitDiffInfo now carry committedAt (epoch ms, %ct); the rail and description card format it with the existing formatRelativeTime — compactAge and the third formatter variant are gone. - The %x1f over-split repair (fixed head/tail fields, rejoined free-text middle) was triplicated across listRecentCommits, listCommitHistory, and getCommitDiffInfo; one splitCommitFormatFields helper (head/tail counts, tail=0 covers the multiline-body shape) owns the edge case. - Avatar resets its broken-image state when src changes — latent today (every caller keys by identity), but it's a shared component and must be safe for callers that update src in place. Left alone from this round: hashString/hashFingerprintPart duplication is pre-existing, and the suggested cross-package import would drag node:path into the browser bundle — the documented reason client mirrors exist. * fix(review): PR-994 round 5 — merge-commit prompts, boundary-aware poll, auto-select guard - Agent prompts for commit:<sha> diffs instruct the exact on-screen command: git diff <sha>^ <sha> (first parent), explicitly steering away from git show — whose combined-diff presentation for a MERGE commit renders a different (often empty) changeset than the one under review. Root-commit fallback noted. One site covers Ask AI and every launched reviewer (and Pi via vendoring); prompt test pins the command. - The rail's freshness poll now adopts on boundary/base movement, not just a new head: an agent running git fetch advances origin/<base> while HEAD stays put, which re-partitions isPastBase — previously the head-only compare skipped adoption and the 'In origin/main' divider stayed stale until the view was reopened. Boundary is compared over the page-1 overlap window (a divider paged deeper than the probe can see re-syncs on the next full reload — accepted micro-edge). The reviewer's proposed trigger (the Fetch banner) is unreachable in commit mode; the external-fetch trigger is the real one. - The HEAD auto-select never fires while any diff switch is in flight — hardening only: the claimed first-click overwrite isn't reachable (isLoadingDiff was never a dependency of that effect, and auto-select settles before a human can click), but the guard makes the invariant explicit. * docs(adr): spec the pre-merge commits-view structure refactor Three behavior-preserving moves scoped for PR #994 before merge, motivated by the audit finding that every recent review-round bug lived at the App.tsx <-> useCommitLog seam: (R1) unify the commits-session state machine (poll, auto-select, veil) in one hook; (R2) lift the commit-rail block out of the 1.6k-line review-core into commit-history.ts; (R3) share the isSameCwdCommitSwitch predicate both runtimes inlined. Ownership tables name what deliberately stays put; execution runs smallest-first with full gates per commit. * refactor(review): share isSameCwdCommitSwitch across runtimes (spec R3) The ~10-line parse-and-compare predicate both /api/diff/switch handlers inlined moves to review-core beside its inputs (parseWorktreeDiffType / parseCommitDiffType — the canonical home for diff-type string logic); both call sites collapse to one line. Unit tests cover plain, worktree, cross-worktree, and non-commit-target cases. Behavior identical. * refactor(shared): lift the commit-rail block into commit-history.ts (spec R2) review-core.ts (1.6k lines) was absorbing the ~230-line Commits-panel data layer it doesn't need to own: CommitListEntry/CommitHistoryPage/ listCommitHistory and CommitDiffInfo/getCommitDiffInfo move verbatim to a new commit-history.ts, matching the package's per-concept split (jj-core, pr-stack, commit-avatars). The commit:<sha> DIFF plumbing (parseCommitDiffType, the runGitDiff/fingerprint/file-contents cases) stays in review-core with the other diff types — it participates in the dispatch; the rail does not. Wiring: review-core exports its three parsing primitives (COMMIT_FIELD_ SEP, splitCommitFormatFields, BARE_HEX_SHA_RE — listRecentCommits still uses them, so they can't move); package exports + vendor.sh gain the module (vendored siblings import relatively, same as pr-provider → pr-github); types.ts re-exports split by source; both runtimes import from the new module. History/metadata test suites move to commit-history.test.ts with the package-style per-file git harness; commit-DIFF tests stay in review-core.test.ts. Behavior identical — verified live through the compiled binary (/api/commits + commit switch + commitInfo sidecar). * refactor(ui): useCommitsView owns the whole commits-session machine (spec R1) The list cache, freshness poll, HEAD auto-select, and center-dock veil were split between useCommitLog and App.tsx — and every sync bug found across three review rounds (stuck flags, stale auto-select, adoption races) lived at exactly that seam. The auto-select effect and the veil derivation move into the hook verbatim (renamed useCommitsView, git mv), so the machine's invariants are locally checkable in one file. App now supplies only what it owns — visibility, the active commit, switch state, and onOpenCommit (its handleSelectCommit: the SAME path user clicks take, so auto- and user-selection cannot diverge by construction) — and consumes panel props plus veilActive. Net ~45 lines of App.tsx's most delicate logic deleted. One equivalence note: the hook's single "enabled" (showCommitsPanel && origin) now gates auto-select/veil where App used bare showCommitsPanel; the two only differ when origin is unset, which implies no gitContext (demo mode) and therefore showCommitsPanel === false — identical in all reachable states. Deliberately still in App (each guards a flow App owns): capability gating, handleSelectCommit, the Cmd+F / worktree-fallback / staleness-refresh / search-clear guards, and render wiring. Behavior identical; spec: adr/specs/refactor-commits-view-structure-20260703.md.	2026-07-04 10:13:11 -07:00