From 425ae3be386e10ff6cfa2b79305f51374f1dae0c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 20:12:14 +0000 Subject: [PATCH 1/3] refactor(review): plan navigation through one shared walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hunk navigation, file navigation, and annotated navigation were implemented twice — once for the keyboard over the terminal's diff-file model, once for the session daemon's comment navigation — so the two could disagree about where a repeated step lands, and a browser client would have made three (docs/browser-review-seam-audit.md, B1-B9/B11). Move the walk into core/review/navigation.ts behind a `selection/move` intent, with the two rules the copies disagreed about stated by name: the wrap policy is per scope (annotated-file cycles, everything else clamps), and a move carries the reveal its scope earns. File jumps get `selection/select-file`, and a viewport that reports where it settled publishes through `selection/anchor`, which moves the selection without asking any viewport to scroll. The selectors those plans read are shared too: one filter matcher, one selection normalization and fallback rule, one reveal target that only picks a side with rows, one notes-by-hunk grouping over recorded ownership, and one note-visibility predicate. Behavior is unchanged, including the quirks that behavior includes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4 --- docs/browser-review-seam-audit.md | 62 +++++ scripts/source-boundaries.test.ts | 2 +- src/core/review/geometry.test.ts | 36 +++ src/core/review/geometry.ts | 26 ++ src/core/review/intents.test.ts | 146 +++++++++++ src/core/review/intents.ts | 117 ++++++++- src/core/review/navigation.test.ts | 196 +++++++++++++++ src/core/review/navigation.ts | 358 +++++++++++++++++++++++++++ src/core/review/selectors.test.ts | 155 +++++++++++- src/core/review/selectors.ts | 180 +++++++++++++- src/core/review/state.test.ts | 19 +- src/core/review/state.ts | 28 +++ src/ui/App.tsx | 22 +- src/ui/components/panes/DiffPane.tsx | 12 +- src/ui/hooks/useReviewController.ts | 251 ++++++++++--------- src/ui/lib/agentAnnotations.ts | 5 - src/ui/lib/files.ts | 20 -- src/ui/lib/hunks.test.ts | 224 ----------------- src/ui/lib/hunks.ts | 98 -------- src/ui/lib/reviewState.test.ts | 87 +++---- src/ui/lib/reviewState.ts | 153 +++++------- 21 files changed, 1566 insertions(+), 631 deletions(-) create mode 100644 src/core/review/navigation.test.ts create mode 100644 src/core/review/navigation.ts delete mode 100644 src/ui/lib/hunks.test.ts delete mode 100644 src/ui/lib/hunks.ts diff --git a/docs/browser-review-seam-audit.md b/docs/browser-review-seam-audit.md index 2ed99438d..21bd19289 100644 --- a/docs/browser-review-seam-audit.md +++ b/docs/browser-review-seam-audit.md @@ -111,35 +111,91 @@ duplication); hunk header text (browser delegates to Pierre separators); platfor (terminal `DiffFile` model vs `intersectingHunkIndices`). Fix: `selection/move` intent (`scope: hunk|file|annotated-hunk|annotated-file`, delta, wrap policy) planned over shared selectors; delete `ui/lib/hunks.ts` and the runtime walk. + _Repaid (Phase 1 PR 3, core and terminal sites)_: `planReviewSelectionMove` in + `core/review/navigation.ts`, reached through the `selection/move` intent. `ui/lib/hunks.ts` is + deleted and tombstoned, and the session runtime's separate walk is gone — comment navigation + (`--next-comment` / `--prev-comment`) now plans the same intent the keyboard does, so the + multi-step carry rule it lacked applies there too. Fixture `annotated-hunk-multi-step-carry` + in `test/review-conformance/navigationFixtures.ts`; the planner is registered as a navigation + consumer. Residual: which hunks count as annotated is still derived from the terminal's merged + diff-file model (`buildReviewAnnotationIndex`) and handed to the planner as a caller-owned + fact, because the semantic document does not carry notes yet — one derivation now, consumed by + both the keyboard and the session. - **B2. Wrap vs clamp policy split.** `moveToFile` clamps; `moveToAnnotatedFile` wraps (`ui/lib/reviewState.ts`). Encode per-scope wrap policy in the `selection/move` intent. + _Repaid (Phase 1 PR 3)_: `REVIEW_SELECTION_WRAP_POLICY` names the policy per scope — + hunk/file/annotated-hunk clamp, annotated-file wraps — and `findNextAnnotatedFile` is deleted. + The asymmetry is today's terminal behavior kept deliberately, including two quirks now stated + rather than implied: a clamping hunk move at an edge re-selects and re-reveals the same hunk + while a file move at an edge publishes nothing at all, and annotated-file navigation from a + file with no notes enters the ring at its start (so the first forward step lands on the ring's + _second_ entry). Fixture `scope-wrap-and-clamp`. - **B3. File-jump semantics — hard-coded identically in both clients.** "Hunk 0 + file-top reveal" in terminal `App.tsx`/`useReviewController.ts` and web `App.tsx`; the terminal-only forward-cross-file alignment rule has no web counterpart. Fix: `selection/select-file` intent owning the rule. + _Repaid (Phase 1 PR 3, core and terminal sites)_: `selection/select-file` owns "first hunk" + (`REVIEW_FILE_JUMP_HUNK_INDEX`) with `REVIEW_FILE_JUMP_REVEAL` as its default reveal, and the + forward-cross-file alignment rule moved into the hunk-move planner, where the crossing is + known. The terminal's `selectFile` lowers to the intent and no longer takes a hunk index. + Fixture `scope-wrap-and-clamp` pins both reveals. The browser's copy lands in Phase 5. - **B4. Selection fallback after reload/filter — 2 divergent answers.** Terminal `resolveSelectedFile` returns undefined (renders "no file"); web `validSelection` and `treeSource.reset` silently fall back to `files[0]`. Core permits `fileKey: null`. Fix: `selectNormalizedSelection`/`selectFallbackFileKey` selectors; delete both client fallbacks. + _Repaid (Phase 1 PR 3, core and terminal sites)_: both selectors live in + `core/review/selectors.ts`, and `resolveSelectedFile` is deleted. Recorded difference from this + finding's description: the terminal in this repo did _not_ render "no file" for a selection the + filter hides — it kept rendering the selected file, and fell back only when the file was gone + from the document. That behavior is authoritative and is what the selector now states, so + `fileKey: null` is reached exactly when nothing is visible at all. Fixtures + `selection-outliving-its-file` and `selection-with-nothing-visible`. The browser's + `validSelection`/`treeSource.reset` fallbacks close in Phase 5. - **B5. Filter matching — 3 matchers.** Core `reviewFileMatchesFilter` (path, previousPath, agentSummary), terminal `filterReviewFiles` (normalized paths), web tree search (canonicalPath only) — browser sidebar and stream can disagree on the same query. Also live-per-keystroke (terminal) vs apply-on-Enter (web, which clobbers in-flight typing on snapshot). Fix: one matcher; one committed-vs-live decision. + _Repaid (Phase 1 PR 3, core and terminal sites)_: `reviewFileMatchesFilter` in + `core/review/selectors.ts` is the only matcher, and `filterReviewFiles` is deleted. The + terminal's behavior won on both points of difference: paths are normalized before matching + (core's matcher did not), and the three fields are joined before the substring test, so a query + may span the boundary between them. Residual: the committed-vs-live decision is still open — + the terminal matches live per keystroke, and planning reads the immediate filter while + rendering reads a one-render-deferred copy of it. The browser's tree search closes in Phase 5. - **B6. Reveal-target derivation — web re-derives, wrongly.** Web `App.tsx` recomputes the hunk target line (`newRange ? "new" : "old"`), duplicating core `canonicalLineForHunk` which prefers by side counts and requires a backed line — pure-deletion hunks scroll the wrong side in the browser. Fix: `selectRevealTarget(state)` selector; clients only resolve DOM/rows. + _Repaid (Phase 1 PR 3, core site)_: `reviewCanonicalHunkLine` in `core/review/geometry.ts` + (preferred side first, backed sides only), behind `selectRevealTarget`. Fixture + `pure-deletion-reveal-target` pins the case the prototype browser got wrong, and pins that a + hunk's position is its first row while a note about the whole hunk hangs from its first change. + The terminal's reveal is row geometry it measures itself and stays renderer-local; the browser + consumes the selector in Phase 5. - **B7. "Jump to note" target — terminal geometry decides, web ignores.** The active-note choice lives in `DiffPane.tsx` row scanning; web never reads `reveal.scrollToNote`. Fix: `selectActiveRevealNoteId(state)` in core. + _Repaid (Phase 1 PR 3, core site)_: `selectActiveRevealNoteId` names the policy — an active + draft in the selected hunk first, else the note anchored earliest in it, arrival order breaking + ties — which is what `DiffPane`'s row scan resolves geometrically today. The terminal site + stays open: it looks the answer up by measured row bounds, and swapping that for the selector + is a rendering change rather than a semantic one. Browser adoption is Phase 5. - **B8. Notes-by-hunk grouping — web re-filters by range containment.** `ReviewStream.tsx` drops annotations whose anchor came from core's fallback path or expanded context even after `pierreDocument` accepted them — notes silently disappear in the browser. Fix: group by `ownerHunkIndex` via a shared `selectNotesByHunk`. + _Repaid (Phase 1 PR 3, core site)_: `selectNotesByHunk` groups by `reviewNoteOwnerHunkIndex`, + reading the ownership `resolveReviewNoteAnchor` decided instead of re-testing containment. The + terminal site stays open deliberately: `DiffPane` groups by range overlap and therefore renders + a note under _every_ hunk it overlaps, so converting it changes what a reviewer sees and + belongs in a behavior-changing PR, not this one. - **B9. Note-visibility policy — two core predicates for one rule.** `selectors.ts` `reviewNoteVisibleByPolicy` (web path) vs `notes.ts` `alwaysShowReviewNote` (terminal path). Collapse to one predicate over `{source}`. + _Repaid (Phase 1 PR 3)_: one `reviewNoteVisibleByPolicy` over `{source}` in + `core/review/state.ts`, beside the other stored-note policies; `alwaysShowReviewNote` is + deleted, and `DiffPane` calls the predicate over the normalized source. - **B10. Selected-line semantics — browser structurally weaker.** Terminal maps rendered rows to semantic side/line with `expandedLineProof` and separates "anchor" from "reveal"; web sends raw lines with `reveal` forced on every click, and the wire `selection/set-line` / @@ -150,6 +206,12 @@ duplication); hunk header text (browser delegates to Pierre separators); platfor nearest-hunk-to-center into shared state; web keeps IntersectionObserver results local. With both clients attached, terminal scrolling rewrites shared selection under the browser. Fix: one core policy (e.g. `selection/anchor` intent that never bumps reveal tokens). + _Repaid (Phase 1 PR 3, core and terminal sites)_: `REVIEW_VIEWPORT_ANCHOR_REVEAL` in + `core/review/state.ts`, and the `selection/anchor` intent that carries it. The terminal's + viewport-centered hunk selection and its line-cursor anchoring both publish through it, and the + local `preserveViewport` selection option is deleted. The policy is stated, but the + multi-client question it exists for — whether an anchor should be shared at all — is G2, + decided before Phase 5 PR 2. - **B12. Wire vocabulary hand-restates `ReviewIntent` — 3 places.** `HunkReviewActionV1` union, capability list in `registration.ts`, validation list in `wire.ts`, remapped field-by-field in `reviewSessionRuntime.ts`; forgotten fields/actions are silently diff --git a/scripts/source-boundaries.test.ts b/scripts/source-boundaries.test.ts index 08bcca7b9..099038832 100644 --- a/scripts/source-boundaries.test.ts +++ b/scripts/source-boundaries.test.ts @@ -123,7 +123,7 @@ function privateProviderApiImports() { // with the finding id, and this gate keeps them deleted — a reappearing path means the // duplication came back. Entries are repo-relative with forward slashes. const EXTRACTED_DUPLICATE_TOMBSTONES: readonly string[] = [ - // e.g. "src/ui/lib/hunks.ts", // B1: replaced by core/review selection/move planning + "src/ui/lib/hunks.ts", // B1: replaced by core/review selection/move planning ]; describe("source architecture boundaries", () => { diff --git a/src/core/review/geometry.test.ts b/src/core/review/geometry.test.ts index 5c0e1e549..262897ef2 100644 --- a/src/core/review/geometry.test.ts +++ b/src/core/review/geometry.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { normalizedReviewSourceLines, rebaseReviewHunk, + reviewCanonicalHunkLine, reviewDefaultHunkLineTarget, reviewHunkIndexForLine, reviewHunkRange, @@ -114,6 +115,41 @@ describe("reviewDefaultHunkLineTarget", () => { }); }); +describe("reviewCanonicalHunkLine", () => { + test("takes the preferred side when it has rows", () => { + expect(reviewCanonicalHunkLine(span(3, 7))).toEqual({ side: "new", line: 3 }); + expect(reviewCanonicalHunkLine(span(3, 7), "old")).toEqual({ side: "old", line: 3 }); + }); + + // Intent: the case the browser prototype got wrong — every hunk reports a new-side + // range, so choosing a side by "has a range" scrolls a deletion to a line that is not there. + test("falls back to the backed side of a pure deletion", () => { + const deletion = { + additionStart: 5, + additionCount: 0, + deletionStart: 6, + deletionCount: 1, + }; + + expect(reviewCanonicalHunkLine(deletion)).toEqual({ side: "old", line: 6 }); + }); + + test("falls back to the backed side of a pure insertion asked for the old one", () => { + const insertion = { + additionStart: 7, + additionCount: 1, + deletionStart: 6, + deletionCount: 0, + }; + + expect(reviewCanonicalHunkLine(insertion, "old")).toEqual({ side: "new", line: 7 }); + }); + + test("reports nothing for a hunk with rows on neither side", () => { + expect(reviewCanonicalHunkLine(span(1, 0))).toBeUndefined(); + }); +}); + describe("rebaseReviewHunk", () => { const hunk = { deletionLineIndex: 4, diff --git a/src/core/review/geometry.ts b/src/core/review/geometry.ts index d9f010246..5b49da746 100644 --- a/src/core/review/geometry.ts +++ b/src/core/review/geometry.ts @@ -107,6 +107,32 @@ export function reviewDefaultHunkLineTarget( : { side: "new", line: reviewHunkRange(hunk, "new")[0] }; } +/** + * The line one hunk is canonically addressed by, on a side that really has rows. + * + * Every hunk reports a range on both sides — a pure insertion still has an old-side + * position — so choosing a side by "does it have a range" always answers "new" and + * scrolls a pure-deletion hunk to a line the file does not contain + * (`docs/browser-review-seam-audit.md`, B6). The choice is made by row counts instead: + * the preferred side when it is backed, otherwise the other, and undefined when a hunk + * has rows on neither. + * + * This is the hunk's *position*, which is what a reveal scrolls to. Where a note about + * the whole hunk hangs is a different question, answered by `reviewDefaultHunkLineTarget`. + */ +export function reviewCanonicalHunkLine( + hunk: ReviewHunkSpan, + preferredSide: ReviewSide = "new", +): ReviewLineAddressV1 | undefined { + for (const side of [preferredSide, preferredSide === "new" ? "old" : "new"] as const) { + const count = side === "new" ? hunk.additionCount : hunk.deletionCount; + if (count > 0) { + return { side, line: side === "new" ? hunk.additionStart : hunk.deletionStart }; + } + } + return undefined; +} + /** Zero-based array origins one hunk's content is re-based onto. */ export interface ReviewHunkOrigins { deletionLineIndex: number; diff --git a/src/core/review/intents.test.ts b/src/core/review/intents.test.ts index a55084a86..7a5fb2d0f 100644 --- a/src/core/review/intents.test.ts +++ b/src/core/review/intents.test.ts @@ -70,6 +70,152 @@ describe("selection intent", () => { }); }); +describe("selection movement intent", () => { + const annotations = { + annotatedHunkIndicesByFileKey: new Map([["beta", new Set([1])]]), + annotatedFileKeys: new Set(["beta"]), + }; + + test("plans a move over the visible stream and reports where it landed", () => { + const state = { ...createTestReviewState(), selection: { fileKey: "alpha", hunkIndex: 1 } }; + + expect(planReviewIntent(state, { type: "selection/move", scope: "hunk", delta: 1 })).toEqual({ + actions: [ + { + type: "selection/select", + fileKey: "beta", + hunkIndex: 0, + reveal: { anchor: "file-top", scrollToNote: false }, + }, + ], + outcome: { type: "selection/changed", fileKey: "beta", hunkIndex: 0 }, + }); + }); + + test("publishes nothing when the scope refuses the move", () => { + const state = { ...createTestReviewState(), selection: { fileKey: "beta", hunkIndex: 1 } }; + + expect(planReviewIntent(state, { type: "selection/move", scope: "file", delta: 1 })).toEqual({ + actions: [], + }); + }); + + test("navigates only what the filter leaves visible", () => { + const state = { + ...createTestReviewState(), + filter: "alpha", + selection: { fileKey: "alpha", hunkIndex: 1 }, + }; + + // Beta is filtered out, so the stream ends at alpha's last hunk. + expect(planReviewIntent(state, { type: "selection/move", scope: "hunk", delta: 1 })).toEqual({ + actions: [ + { + type: "selection/select", + fileKey: "alpha", + hunkIndex: 1, + reveal: { anchor: "hunk", scrollToNote: false }, + }, + ], + outcome: { type: "selection/changed", fileKey: "alpha", hunkIndex: 1 }, + }); + }); + + test("requires the annotation index for annotated navigation", () => { + const state = createTestReviewState(); + + expect(() => + planReviewIntent(state, { type: "selection/move", scope: "annotated-hunk", delta: 1 }), + ).toThrow(ReviewIntentPlanningError); + expect( + planReviewIntent( + state, + { type: "selection/move", scope: "annotated-hunk", delta: 1 }, + { annotations }, + ).actions, + ).toEqual([ + { + type: "selection/select", + fileKey: "beta", + hunkIndex: 1, + reveal: { anchor: "hunk", scrollToNote: true }, + }, + ]); + }); +}); + +describe("file jump intent", () => { + test("lands on the file's first hunk and reveals its header by default", () => { + expect( + planReviewIntent(createTestReviewState(), { + type: "selection/select-file", + fileKey: "beta", + }), + ).toEqual({ + actions: [ + { + type: "selection/select", + fileKey: "beta", + hunkIndex: 0, + reveal: { anchor: "file-top", scrollToNote: false }, + }, + ], + outcome: { type: "selection/changed", fileKey: "beta", hunkIndex: 0 }, + }); + }); + + test("accepts a caller's own reveal request", () => { + expect( + planReviewIntent(createTestReviewState(), { + type: "selection/select-file", + fileKey: "beta", + reveal: { anchor: "hunk", scrollToNote: false }, + }).actions[0], + ).toEqual({ + type: "selection/select", + fileKey: "beta", + hunkIndex: 0, + reveal: { anchor: "hunk", scrollToNote: false }, + }); + }); + + test("rejects a file the review does not contain", () => { + expect(() => + planReviewIntent(createTestReviewState(), { + type: "selection/select-file", + fileKey: "missing", + }), + ).toThrow(ReviewIntentPlanningError); + }); +}); + +describe("viewport anchor intent", () => { + // Intent: a viewport reporting where it settled must not scroll anybody, including itself. + test("moves the selection without bumping a reveal counter", () => { + const state = createTestReviewState(); + const plan = planReviewIntent(state, { + type: "selection/anchor", + fileKey: "beta", + hunkIndex: 1, + }); + + expect(plan).toEqual({ + actions: [ + { + type: "selection/select", + fileKey: "beta", + hunkIndex: 1, + reveal: { anchor: "none", scrollToNote: false }, + }, + ], + }); + + const next = plan.actions.reduce(reduceReviewState, state); + expect(next.selection).toEqual({ fileKey: "beta", hunkIndex: 1 }); + expect(next.reveal).toEqual({ fileTopToken: 0, hunkToken: 0, scrollToNote: false }); + }); +}); + describe("user note creation", () => { test("anchors the note to the draft's line and hunk", () => { const plan = planReviewIntent( diff --git a/src/core/review/intents.ts b/src/core/review/intents.ts index f6abb2f6b..c61c069b8 100644 --- a/src/core/review/intents.ts +++ b/src/core/review/intents.ts @@ -11,8 +11,26 @@ */ import type { ReviewAction } from "./actions"; import { reviewLineAnchor } from "./anchors"; -import { isReviewNoteWithinClearScope, selectReviewFileByKey } from "./selectors"; -import { type ReviewRevealRequest, type ReviewState, type ReviewStoredNote } from "./state"; +import { + EMPTY_REVIEW_ANNOTATION_INDEX, + planReviewSelectionMove, + REVIEW_FILE_JUMP_HUNK_INDEX, + REVIEW_FILE_JUMP_REVEAL, + type ReviewAnnotationIndex, + type ReviewSelectionScope, +} from "./navigation"; +import { + isReviewNoteWithinClearScope, + selectNormalizedSelection, + selectReviewFileByKey, + selectReviewNavigationFiles, +} from "./selectors"; +import { + REVIEW_VIEWPORT_ANCHOR_REVEAL, + type ReviewRevealRequest, + type ReviewState, + type ReviewStoredNote, +} from "./state"; import type { ReviewStore } from "./store"; import type { ReviewFileV1 } from "./types"; @@ -21,10 +39,24 @@ export interface ReviewIntentFacts { noteId?: string; /** Caller-owned ISO timestamp for note creation. */ timestamp?: string; + /** + * Which files and hunks currently carry notes, for annotated navigation. + * + * A caller-owned fact like the two above: notes reach a review from sources the + * semantic document does not carry, and only the consumer that merged them knows the + * full set. + */ + annotations?: ReviewAnnotationIndex; } export type ReviewIntent = | { type: "selection/select"; fileKey: string; hunkIndex: number; reveal: ReviewRevealRequest } + /** Step the selection through one navigable scope; the scope decides wrap and reveal. */ + | { type: "selection/move"; scope: ReviewSelectionScope; delta: number } + /** Jump to one file, landing on its first hunk. */ + | { type: "selection/select-file"; fileKey: string; reveal?: ReviewRevealRequest } + /** Adopt the position a renderer's viewport settled on, without moving any viewport. */ + | { type: "selection/anchor"; fileKey: string; hunkIndex: number } | { type: "filter/set"; filter: string } | { type: "notes/set-visibility"; visible: boolean } /** Persist the active draft; a blank body retires the draft instead. */ @@ -33,6 +65,12 @@ export type ReviewIntent = | { type: "notes/remove-live"; noteId: string } | { type: "notes/clear"; fileKey?: string; includeUser?: boolean }; +export interface ReviewSelectionChangedOutcome { + type: "selection/changed"; + fileKey: string; + hunkIndex: number; +} + export interface ReviewNoteCreatedOutcome { type: "notes/created"; note: ReviewStoredNote; @@ -53,6 +91,7 @@ export interface ReviewNotesClearedOutcome { } export type ReviewIntentOutcome = + | ReviewSelectionChangedOutcome | ReviewNoteCreatedOutcome | ReviewNoteRemovedOutcome | ReviewNotesClearedOutcome; @@ -66,6 +105,10 @@ export type ReviewIntentOutcome = */ export interface ReviewIntentOutcomeByType { "selection/select": undefined; + /** Absent when the scope refused the move and left the selection alone. */ + "selection/move": ReviewSelectionChangedOutcome | undefined; + "selection/select-file": ReviewSelectionChangedOutcome; + "selection/anchor": undefined; "filter/set": undefined; "notes/set-visibility": undefined; "notes/create-user": ReviewNoteCreatedOutcome | undefined; @@ -139,6 +182,51 @@ function requireHunk(file: ReviewFileV1, hunkIndex: number) { } } +/** Lower one resolved selection target into the action that commits it. */ +function planSelection( + fileKey: string, + hunkIndex: number, + reveal: ReviewRevealRequest, +): ReviewIntentPlan { + return { + actions: [{ type: "selection/select", fileKey, hunkIndex, reveal }], + outcome: { type: "selection/changed", fileKey, hunkIndex }, + }; +} + +/** + * Plan one relative selection move over the currently visible stream. + * + * Navigation walks what the reviewer can see: a filtered-out file is not a step away, and + * the selection it starts from is the normalized one, so a move from a vanished file + * begins where the review actually is. + */ +function planSelectionMove( + state: ReviewState, + intent: Extract, + facts: ReviewIntentFacts, +): ReviewIntentPlan { + const annotated = intent.scope === "annotated-hunk" || intent.scope === "annotated-file"; + if (annotated && !facts.annotations) { + throw new ReviewIntentPlanningError( + "missing-fact", + `Review intent requires annotations for ${intent.scope} navigation.`, + ); + } + + const target = planReviewSelectionMove( + { + files: selectReviewNavigationFiles(state), + annotations: facts.annotations ?? EMPTY_REVIEW_ANNOTATION_INDEX, + }, + selectNormalizedSelection(state), + { scope: intent.scope, delta: intent.delta }, + ); + // A refused move publishes nothing at all: no selection change, and no reveal token + // bump that would scroll a viewport for a key press that went nowhere. + return target ? planSelection(target.fileKey, target.hunkIndex, target.reveal) : { actions: [] }; +} + /** Plan persistence of the active draft as one user note. */ function planUserNoteCreation(state: ReviewState, facts: ReviewIntentFacts): ReviewIntentPlan { const draft = state.draftNote; @@ -242,6 +330,31 @@ export function planReviewIntent( ], }; } + case "selection/move": + return planSelectionMove(state, intent, facts); + case "selection/select-file": { + // The file-jump rule, owned here rather than restated per surface: selecting a file + // means its first hunk, and the reveal defaults to the file's own header. + const file = requireFile(state, intent.fileKey); + return planSelection( + file.key, + REVIEW_FILE_JUMP_HUNK_INDEX, + intent.reveal ?? REVIEW_FILE_JUMP_REVEAL, + ); + } + case "selection/anchor": { + const file = requireFile(state, intent.fileKey); + return { + actions: [ + { + type: "selection/select", + fileKey: file.key, + hunkIndex: intent.hunkIndex, + reveal: REVIEW_VIEWPORT_ANCHOR_REVEAL, + }, + ], + }; + } case "filter/set": return { actions: [{ type: "filter/set", filter: intent.filter }] }; case "notes/set-visibility": diff --git a/src/core/review/navigation.test.ts b/src/core/review/navigation.test.ts new file mode 100644 index 000000000..3955bb90e --- /dev/null +++ b/src/core/review/navigation.test.ts @@ -0,0 +1,196 @@ +import { describe, expect, test } from "bun:test"; +import { + EMPTY_REVIEW_ANNOTATION_INDEX, + planReviewSelectionMove, + REVIEW_SELECTION_WRAP_POLICY, + reviewAnnotatedCursors, + reviewStreamCursors, + type ReviewAnnotationIndex, + type ReviewNavigationFile, + type ReviewNavigationModel, + type ReviewSelectionScope, +} from "./navigation"; +import type { ReviewSemanticSelection } from "./state"; + +/** Three files of two hunks each: alpha, beta, gamma. */ +const FILES: ReviewNavigationFile[] = [ + { fileKey: "alpha", hunkCount: 2 }, + { fileKey: "beta", hunkCount: 2 }, + { fileKey: "gamma", hunkCount: 2 }, +]; + +/** Build an annotation index from hunk membership, plus the files those hunks live in. */ +function annotationIndex( + membership: Record, + extraFileKeys: string[] = [], +): ReviewAnnotationIndex { + return { + annotatedHunkIndicesByFileKey: new Map( + Object.entries(membership).map(([fileKey, hunks]) => [fileKey, new Set(hunks)]), + ), + annotatedFileKeys: new Set([...Object.keys(membership), ...extraFileKeys]), + }; +} + +function model(annotations = EMPTY_REVIEW_ANNOTATION_INDEX): ReviewNavigationModel { + return { files: FILES, annotations }; +} + +function at(fileKey: string | null, hunkIndex: number): ReviewSemanticSelection { + return { fileKey, hunkIndex }; +} + +/** Move and report the landing position plus what it asked the viewport to reveal. */ +function move( + navigationModel: ReviewNavigationModel, + selection: ReviewSemanticSelection, + scope: ReviewSelectionScope, + delta: number, +) { + const target = planReviewSelectionMove(navigationModel, selection, { scope, delta }); + return target + ? { at: `${target.fileKey}:${target.hunkIndex}`, reveal: target.reveal } + : { at: null }; +} + +describe("review selection movement", () => { + // Intent: the wrap policy is a named per-scope decision, not arithmetic that happens to differ. + test("declares one wrap policy per scope", () => { + expect(REVIEW_SELECTION_WRAP_POLICY).toEqual({ + hunk: "clamp", + file: "clamp", + "annotated-hunk": "clamp", + "annotated-file": "wrap", + }); + }); + + test("flattens the stream and its annotated subset in review order", () => { + expect(reviewStreamCursors(FILES)).toEqual([ + { fileKey: "alpha", hunkIndex: 0 }, + { fileKey: "alpha", hunkIndex: 1 }, + { fileKey: "beta", hunkIndex: 0 }, + { fileKey: "beta", hunkIndex: 1 }, + { fileKey: "gamma", hunkIndex: 0 }, + { fileKey: "gamma", hunkIndex: 1 }, + ]); + expect(reviewAnnotatedCursors(FILES, annotationIndex({ alpha: [1], gamma: [0] }))).toEqual([ + { fileKey: "alpha", hunkIndex: 1 }, + { fileKey: "gamma", hunkIndex: 0 }, + ]); + }); + + test("steps hunks across file boundaries and clamps at both ends", () => { + expect(move(model(), at("alpha", 1), "hunk", 1).at).toBe("beta:0"); + expect(move(model(), at("beta", 0), "hunk", -1).at).toBe("alpha:1"); + expect(move(model(), at("alpha", 0), "hunk", 3).at).toBe("beta:1"); + // Clamping, not wrapping: the ends of the stream are where hunk navigation stops. + expect(move(model(), at("gamma", 1), "hunk", 1).at).toBe("gamma:1"); + expect(move(model(), at("alpha", 0), "hunk", -1).at).toBe("alpha:0"); + }); + + test("reveals the file header only when a hunk move crosses forward into another file", () => { + expect(move(model(), at("alpha", 1), "hunk", 1).reveal).toEqual({ + anchor: "file-top", + scrollToNote: false, + }); + expect(move(model(), at("beta", 0), "hunk", -1).reveal).toEqual({ + anchor: "hunk", + scrollToNote: false, + }); + expect(move(model(), at("alpha", 0), "hunk", 1).reveal).toEqual({ + anchor: "hunk", + scrollToNote: false, + }); + }); + + test("steps files onto their first hunk and refuses a move that would go nowhere", () => { + expect(move(model(), at("alpha", 1), "file", 1)).toEqual({ + at: "beta:0", + reveal: { anchor: "file-top", scrollToNote: false }, + }); + expect(move(model(), at("alpha", 0), "file", 2).at).toBe("gamma:0"); + // At an end, file navigation does nothing at all rather than re-revealing the current file. + expect(move(model(), at("gamma", 1), "file", 1).at).toBeNull(); + expect(move(model(), at("alpha", 0), "file", -1).at).toBeNull(); + // A selection outside the visible stream has nowhere to step from. + expect(move(model(), at("hidden", 0), "file", 1).at).toBeNull(); + }); + + test("carries the remaining steps after reaching the nearest annotated hunk", () => { + const annotated = model(annotationIndex({ alpha: [0], beta: [1], gamma: [0, 1] })); + + // From an unannotated position, one step reaches the nearest annotated hunk ahead. + expect(move(annotated, at("alpha", 1), "annotated-hunk", 1).at).toBe("beta:1"); + // Two steps reach it and then take one more, rather than spending both on the approach. + expect(move(annotated, at("alpha", 1), "annotated-hunk", 2).at).toBe("gamma:0"); + expect(move(annotated, at("alpha", 1), "annotated-hunk", 3).at).toBe("gamma:1"); + // The same rule backwards. + expect(move(annotated, at("gamma", 0), "annotated-hunk", -1).at).toBe("beta:1"); + expect(move(annotated, at("gamma", 0), "annotated-hunk", -2).at).toBe("alpha:0"); + // Annotated navigation clamps like plain hunk navigation. + expect(move(annotated, at("alpha", 1), "annotated-hunk", 9).at).toBe("gamma:1"); + expect(move(annotated, at("beta", 0), "annotated-hunk", -9).at).toBe("alpha:0"); + }); + + test("asks for the note when annotated-hunk navigation lands", () => { + const annotated = model(annotationIndex({ beta: [0] })); + + expect(move(annotated, at("alpha", 0), "annotated-hunk", 1).reveal).toEqual({ + anchor: "hunk", + scrollToNote: true, + }); + }); + + test("refuses annotated navigation when the review has no notes", () => { + expect(move(model(), at("alpha", 0), "annotated-hunk", 1).at).toBeNull(); + expect(move(model(), at("alpha", 0), "annotated-file", 1).at).toBeNull(); + }); + + test("cycles annotated files, wrapping past both ends", () => { + const annotated = model(annotationIndex({ alpha: [0], gamma: [0] })); + + expect(move(annotated, at("alpha", 0), "annotated-file", 1).at).toBe("gamma:0"); + expect(move(annotated, at("gamma", 0), "annotated-file", 1).at).toBe("alpha:0"); + expect(move(annotated, at("alpha", 0), "annotated-file", -1).at).toBe("gamma:0"); + // Landing on a file shows its content rather than aligning its header. + expect(move(annotated, at("alpha", 0), "annotated-file", 1).reveal).toEqual({ + anchor: "hunk", + scrollToNote: false, + }); + }); + + test("treats a file with no notes as the start of the annotated ring", () => { + const annotated = model(annotationIndex({ alpha: [0], gamma: [0] })); + + // From unannotated beta, "next" is the ring's second entry, exactly as the terminal + // has always behaved: an absent position normalizes to index 0 before stepping. + expect(move(annotated, at("beta", 0), "annotated-file", 1).at).toBe("gamma:0"); + expect(move(annotated, at("beta", 0), "annotated-file", -1).at).toBe("gamma:0"); + }); + + test("visits a file whose review context lives outside any hunk", () => { + // A file-level summary with no note inside a hunk: annotated-file navigation stops + // there, annotated-hunk navigation does not. + const annotated = model(annotationIndex({ alpha: [0] }, ["beta"])); + + expect(move(annotated, at("alpha", 0), "annotated-file", 1).at).toBe("beta:0"); + expect(move(annotated, at("alpha", 0), "annotated-hunk", 1).at).toBe("alpha:0"); + }); + + test("starts from an edge when the current position is not on the stream", () => { + const annotated = model(annotationIndex({ beta: [0], gamma: [1] })); + + expect(move(model(), at(null, 0), "hunk", 1).at).toBe("alpha:0"); + expect(move(model(), at(null, 0), "hunk", -1).at).toBe("gamma:1"); + expect(move(annotated, at("hidden", 4), "annotated-hunk", 1).at).toBe("beta:0"); + expect(move(annotated, at("hidden", 4), "annotated-hunk", -1).at).toBe("gamma:1"); + }); + + test("refuses every move over an empty stream", () => { + const empty: ReviewNavigationModel = { files: [], annotations: EMPTY_REVIEW_ANNOTATION_INDEX }; + + for (const scope of Object.keys(REVIEW_SELECTION_WRAP_POLICY) as ReviewSelectionScope[]) { + expect(move(empty, at(null, 0), scope, 1).at).toBeNull(); + } + }); +}); diff --git a/src/core/review/navigation.ts b/src/core/review/navigation.ts new file mode 100644 index 000000000..cacc05793 --- /dev/null +++ b/src/core/review/navigation.ts @@ -0,0 +1,358 @@ +/** + * Relative navigation over the review stream: where one move lands, and what it reveals. + * + * A reviewer stepping to the next hunk, the previous file, or the next annotated hunk is + * asking the same question every surface has to answer — the terminal's keyboard, the + * agent session's `--next-comment`, and later a browser client. The prototype answered it + * three times over (`docs/browser-review-seam-audit.md`, B1), so the walk lives here once + * and every surface plans through it. + * + * Two rules are stated rather than implied, because the previous copies disagreed about + * both: + * + * - **Wrap policy is per scope** (B2). Plain hunk and file navigation clamps at the ends + * of the stream; annotated-file navigation cycles. That asymmetry is the terminal's + * long-standing behavior and is named here instead of falling out of whichever + * arithmetic each copy happened to use. + * - **A move carries its own reveal request** (B3). "Next hunk" crossing forward into + * another file puts that file's header on screen; crossing backward reveals the hunk + * itself; annotated-hunk navigation asks for the note. Callers do not re-decide this. + * + * The model this plans over is deliberately structural — file keys and hunk counts, plus + * an annotation index the consumer supplies. Which hunks count as annotated depends on + * note sources the semantic document does not carry (an imported sidecar, a renderer's + * merged live comments), so it arrives as a caller-owned fact rather than being guessed. + */ +import type { ReviewRevealRequest, ReviewSemanticSelection } from "./state"; + +export type ReviewSelectionScope = "hunk" | "file" | "annotated-hunk" | "annotated-file"; + +/** What a move does when it runs off the end of the stream it walks. */ +export type ReviewSelectionWrapPolicy = "clamp" | "wrap"; + +/** + * The wrap policy each scope navigates under. + * + * Clamping is the default because hunk and file navigation double as "am I at the end + * yet?"; annotated-file navigation cycles because a review with two annotated files is a + * ring the reviewer tours rather than a list they walk off. + */ +export const REVIEW_SELECTION_WRAP_POLICY: Readonly< + Record +> = Object.freeze({ + hunk: "clamp", + file: "clamp", + "annotated-hunk": "clamp", + "annotated-file": "wrap", +}); + +/** One navigable file, reduced to what a walk over the stream needs. */ +export interface ReviewNavigationFile { + fileKey: string; + hunkCount: number; +} + +/** + * Which files and hunks currently carry notes. + * + * A caller-owned fact: notes reach a review from several places (a sidecar loaded with the + * changeset, live agent comments, the reviewer's own notes), and only the consumer that + * merged them knows the full set. File-level and hunk-level membership are separate + * entries on purpose — a file can carry review context without any note landing inside a + * hunk, and annotated-file navigation has always visited it. + */ +export interface ReviewAnnotationIndex { + annotatedHunkIndicesByFileKey: ReadonlyMap>; + annotatedFileKeys: ReadonlySet; +} + +/** An empty annotation index, for a review that has no notes at all. */ +export const EMPTY_REVIEW_ANNOTATION_INDEX: ReviewAnnotationIndex = Object.freeze({ + annotatedHunkIndicesByFileKey: new Map>(), + annotatedFileKeys: new Set(), +}); + +/** One addressable position in the flattened review stream. */ +export interface ReviewHunkCursor { + fileKey: string; + hunkIndex: number; +} + +export interface ReviewSelectionMove { + scope: ReviewSelectionScope; + /** Signed step count; magnitude repeats the move, sign chooses the direction. */ + delta: number; +} + +/** Where one planned move lands, and what it asks the viewport to show. */ +export interface ReviewSelectionMoveTarget { + fileKey: string; + hunkIndex: number; + reveal: ReviewRevealRequest; +} + +export interface ReviewNavigationModel { + /** Navigable files in review order — the visible stream, not the whole document. */ + files: readonly ReviewNavigationFile[]; + annotations: ReviewAnnotationIndex; +} + +/** + * The reveal a file jump asks for: put the file's header on screen. + * + * Selecting a file is a jump to somewhere else in the review, so the reviewer needs the + * landmark that tells them where they arrived rather than a hunk floating mid-viewport. + */ +export const REVIEW_FILE_JUMP_REVEAL: ReviewRevealRequest = Object.freeze({ + anchor: "file-top", + scrollToNote: false, +}); + +/** Selecting a file selects its first hunk; nothing else would be a defined position. */ +export const REVIEW_FILE_JUMP_HUNK_INDEX = 0; + +/** Clamp one index into an inclusive range. */ +function clamp(value: number, min: number, max: number) { + return Math.min(Math.max(value, min), max); +} + +/** Compare two stream positions. */ +function cursorMatches(cursor: ReviewHunkCursor, selection: ReviewSemanticSelection) { + return cursor.fileKey === selection.fileKey && cursor.hunkIndex === selection.hunkIndex; +} + +/** Flatten every hunk of every navigable file into one top-to-bottom cursor list. */ +export function reviewStreamCursors(files: readonly ReviewNavigationFile[]): ReviewHunkCursor[] { + return files.flatMap((file) => + Array.from({ length: file.hunkCount }, (_unused, hunkIndex) => ({ + fileKey: file.fileKey, + hunkIndex, + })), + ); +} + +/** Flatten only the hunks carrying notes, in the same top-to-bottom order. */ +export function reviewAnnotatedCursors( + files: readonly ReviewNavigationFile[], + annotations: ReviewAnnotationIndex, +): ReviewHunkCursor[] { + return files.flatMap((file) => { + const annotated = annotations.annotatedHunkIndicesByFileKey.get(file.fileKey); + if (!annotated || annotated.size === 0) { + return []; + } + return Array.from({ length: file.hunkCount }, (_unused, hunkIndex) => ({ + fileKey: file.fileKey, + hunkIndex, + })).filter((cursor) => annotated.has(cursor.hunkIndex)); + }); +} + +/** The files annotated-file navigation tours, in review order. */ +function annotatedFiles( + files: readonly ReviewNavigationFile[], + annotations: ReviewAnnotationIndex, +) { + return files.filter((file) => annotations.annotatedFileKeys.has(file.fileKey)); +} + +/** + * Resolve a move whose starting position is not itself in the cursor subset. + * + * Annotated navigation walks a subset of the stream, so the reviewer is usually standing + * between two of its entries. The rule is "reach the nearest entry in the direction of + * travel, then spend what is left of the step count" — a repeat count must not be + * swallowed by the approach, and must not overshoot past the end either, since annotated + * navigation clamps like the plain kind. + */ +function nearestCursorIndex( + cursors: readonly ReviewHunkCursor[], + streamCursors: readonly ReviewHunkCursor[], + selection: ReviewSemanticSelection, + delta: number, +) { + const edgeIndex = delta >= 0 ? 0 : cursors.length - 1; + if (selection.fileKey === null) { + return edgeIndex; + } + + const currentStreamIndex = streamCursors.findIndex((cursor) => cursorMatches(cursor, selection)); + if (currentStreamIndex < 0) { + return edgeIndex; + } + + const streamIndexByCursor = new Map( + streamCursors.map( + (cursor, index) => [`${cursor.fileKey}\0${cursor.hunkIndex}`, index] as const, + ), + ); + const indexedCursors = cursors + .map((cursor, index) => ({ + index, + streamIndex: streamIndexByCursor.get(`${cursor.fileKey}\0${cursor.hunkIndex}`) ?? -1, + })) + .filter(({ streamIndex }) => streamIndex >= 0); + if (indexedCursors.length === 0) { + return edgeIndex; + } + + const remainingSteps = Math.max(0, Math.abs(delta) - 1); + if (delta >= 0) { + const nextCursor = indexedCursors.find(({ streamIndex }) => streamIndex > currentStreamIndex); + const nearestIndex = nextCursor?.index ?? indexedCursors[indexedCursors.length - 1]!.index; + return Math.min(nearestIndex + remainingSteps, cursors.length - 1); + } + + for (let index = indexedCursors.length - 1; index >= 0; index -= 1) { + const indexedCursor = indexedCursors[index]!; + if (indexedCursor.streamIndex < currentStreamIndex) { + return Math.max(0, indexedCursor.index - remainingSteps); + } + } + + return 0; +} + +/** + * Step through one cursor list under the clamping policy. + * + * A position inside the list moves by exactly `delta` and stops at the ends; a position + * outside it lands through the nearest-then-carry rule above. + */ +function stepCursors( + cursors: readonly ReviewHunkCursor[], + streamCursors: readonly ReviewHunkCursor[], + selection: ReviewSemanticSelection, + delta: number, +): ReviewHunkCursor | null { + if (cursors.length === 0) { + return null; + } + + const currentIndex = cursors.findIndex((cursor) => cursorMatches(cursor, selection)); + const nextIndex = + currentIndex >= 0 + ? clamp(currentIndex + delta, 0, cursors.length - 1) + : nearestCursorIndex(cursors, streamCursors, selection, delta); + return cursors[nextIndex] ?? null; +} + +/** Plan a move through every hunk of the visible stream. */ +function planHunkMove( + model: ReviewNavigationModel, + selection: ReviewSemanticSelection, + delta: number, +): ReviewSelectionMoveTarget | null { + const cursors = reviewStreamCursors(model.files); + const target = stepCursors(cursors, cursors, selection, delta); + if (!target) { + return null; + } + + // Forward jumps into another file land on its header, so the reviewer sees which file + // they entered. Backward jumps reveal the hunk itself: the target usually sits near the + // bottom of the previous file, and a header alignment would leave it off screen. + const crossesFileForward = target.fileKey !== selection.fileKey && delta > 0; + return { + ...target, + reveal: { anchor: crossesFileForward ? "file-top" : "hunk", scrollToNote: false }, + }; +} + +/** Plan a move through every visible file, selecting its first hunk. */ +function planFileMove( + model: ReviewNavigationModel, + selection: ReviewSemanticSelection, + delta: number, +): ReviewSelectionMoveTarget | null { + const currentIndex = model.files.findIndex((file) => file.fileKey === selection.fileKey); + // A selection outside the visible stream has no place to step from, and moving anyway + // would teleport the reviewer somewhere they never navigated to. + if (currentIndex < 0) { + return null; + } + + const nextIndex = clamp(currentIndex + delta, 0, model.files.length - 1); + const nextFile = model.files[nextIndex]; + // Standing at either end is a no-op rather than a re-selection: file navigation must + // not scroll the viewport back to the current file's header on a key that did nothing. + if (nextIndex === currentIndex || !nextFile) { + return null; + } + + return { + fileKey: nextFile.fileKey, + hunkIndex: REVIEW_FILE_JUMP_HUNK_INDEX, + reveal: REVIEW_FILE_JUMP_REVEAL, + }; +} + +/** Plan a move through only the hunks carrying notes. */ +function planAnnotatedHunkMove( + model: ReviewNavigationModel, + selection: ReviewSemanticSelection, + delta: number, +): ReviewSelectionMoveTarget | null { + const target = stepCursors( + reviewAnnotatedCursors(model.files, model.annotations), + reviewStreamCursors(model.files), + selection, + delta, + ); + // The note is what the reviewer asked to see; the hunk around it comes along with it. + return target ? { ...target, reveal: { anchor: "hunk", scrollToNote: true } } : null; +} + +/** Plan a cycle through only the files carrying notes. */ +function planAnnotatedFileMove( + model: ReviewNavigationModel, + selection: ReviewSemanticSelection, + delta: number, +): ReviewSelectionMoveTarget | null { + const files = annotatedFiles(model.files, model.annotations); + if (files.length === 0) { + return null; + } + + // A reviewer standing on a file with no notes is treated as standing at the start of + // the ring, so the first forward step lands on its second entry rather than its first. + const currentIndex = files.findIndex((file) => file.fileKey === selection.fileKey); + const normalizedIndex = currentIndex >= 0 ? currentIndex : 0; + const nextIndex = (((normalizedIndex + delta) % files.length) + files.length) % files.length; + const nextFile = files[nextIndex]; + if (!nextFile) { + return null; + } + + // Deliberately not the file-jump reveal: touring annotated files shows the note-bearing + // content, and starting each file at its header would push that content down the page. + return { + fileKey: nextFile.fileKey, + hunkIndex: REVIEW_FILE_JUMP_HUNK_INDEX, + reveal: { anchor: "hunk", scrollToNote: false }, + }; +} + +/** + * Plan one relative selection move, or report that the review has nowhere to go. + * + * `null` means the move is refused — an empty stream, a scope with no members, or an edge + * a clamping scope declines to re-select — and a refused move must leave both the + * selection and the viewport exactly as they were. + */ +export function planReviewSelectionMove( + model: ReviewNavigationModel, + selection: ReviewSemanticSelection, + move: ReviewSelectionMove, +): ReviewSelectionMoveTarget | null { + switch (move.scope) { + case "hunk": + return planHunkMove(model, selection, move.delta); + case "file": + return planFileMove(model, selection, move.delta); + case "annotated-hunk": + return planAnnotatedHunkMove(model, selection, move.delta); + case "annotated-file": + return planAnnotatedFileMove(model, selection, move.delta); + } +} diff --git a/src/core/review/selectors.test.ts b/src/core/review/selectors.test.ts index e0c1584d8..4b05f7cbd 100644 --- a/src/core/review/selectors.test.ts +++ b/src/core/review/selectors.test.ts @@ -1,11 +1,22 @@ import { describe, expect, test } from "bun:test"; -import { createTestReviewState } from "../../../test/helpers/review-store-helpers"; +import { + createTestReviewState, + createTestStoredNote, +} from "../../../test/helpers/review-store-helpers"; import { reduceReviewState } from "./reducer"; import { isReviewGapExpanded, reviewFileKeysWithRetiredContent, + reviewFileMatchesFilter, + selectActiveRevealNoteId, selectExpandedGapIdsByFileKey, + selectFallbackFileKey, + selectNormalizedSelection, + selectNotesByHunk, selectReviewFileByKey, + selectReviewNavigationFiles, + selectRevealTarget, + selectVisibleReviewFiles, } from "./selectors"; describe("file selectors", () => { @@ -61,3 +72,145 @@ describe("expansion selectors", () => { }); }); }); + +describe("filter selectors", () => { + // Intent: one matcher answers for every surface, over every field a reviewer expects. + test("match a query against path, previous path, and agent summary", () => { + const file = { + path: "src/review/stream.ts", + previousPath: "src/legacy/stream.ts", + agentSummary: "Rewrites the note placement policy", + }; + + expect(reviewFileMatchesFilter(file, "")).toBe(true); + expect(reviewFileMatchesFilter(file, " ")).toBe(true); + expect(reviewFileMatchesFilter(file, "REVIEW/stream")).toBe(true); + expect(reviewFileMatchesFilter(file, "legacy")).toBe(true); + expect(reviewFileMatchesFilter(file, "placement policy")).toBe(true); + expect(reviewFileMatchesFilter(file, "unrelated")).toBe(false); + }); + + // Intent: a parser's stray line ending must not decide whether a file matches. + test("normalize paths before matching", () => { + expect(reviewFileMatchesFilter({ path: "src/alpha.ts\r\n" }, "alpha.ts")).toBe(true); + }); + + test("select visible files and the navigable stream in review order", () => { + const state = { ...createTestReviewState(["alpha", "beta"]), filter: "beta" }; + + expect(selectVisibleReviewFiles(state).map((file) => file.key)).toEqual(["beta"]); + expect(selectReviewNavigationFiles(state)).toEqual([{ fileKey: "beta", hunkCount: 2 }]); + }); +}); + +describe("selection selectors", () => { + // Intent: filtering changes what is browsable, not where the reviewer was looking. + test("keep a selection the filter hides", () => { + const state = { + ...createTestReviewState(["alpha", "beta"]), + filter: "beta", + selection: { fileKey: "alpha", hunkIndex: 1 }, + }; + + expect(selectNormalizedSelection(state)).toEqual({ fileKey: "alpha", hunkIndex: 1 }); + }); + + // Intent: a selection whose file vanished falls back, and reports nothing when it cannot. + test("fall back only when the document no longer has the selected file", () => { + const state = { + ...createTestReviewState(["alpha", "beta"]), + selection: { fileKey: "vanished", hunkIndex: 3 }, + }; + + expect(selectFallbackFileKey(state)).toBe("alpha"); + expect(selectNormalizedSelection(state)).toEqual({ fileKey: "alpha", hunkIndex: 0 }); + + const filteredOut = { ...state, filter: "nothing-matches" }; + expect(selectFallbackFileKey(filteredOut)).toBeNull(); + expect(selectNormalizedSelection(filteredOut)).toEqual({ fileKey: null, hunkIndex: 0 }); + }); + + test("clamp a stale hunk index onto the file it addresses", () => { + const state = { + ...createTestReviewState([{ key: "alpha", hunkCount: 2 }]), + selection: { fileKey: "alpha", hunkIndex: 9 }, + }; + + expect(selectNormalizedSelection(state)).toEqual({ fileKey: "alpha", hunkIndex: 1 }); + }); +}); + +describe("reveal selectors", () => { + test("resolve the selected hunk's canonical line", () => { + const state = createTestReviewState(["alpha"]); + + expect(selectRevealTarget(state)).toEqual({ side: "new", line: 1 }); + expect(selectRevealTarget({ ...state, selection: { fileKey: "alpha", hunkIndex: 1 } })).toEqual( + { side: "new", line: 11 }, + ); + expect( + selectRevealTarget({ ...state, selection: { fileKey: null, hunkIndex: 0 } }), + ).toBeUndefined(); + }); +}); + +describe("note selectors", () => { + test("group notes by the hunk that owns them, not by range containment", () => { + const state = { + ...createTestReviewState(["alpha", "beta"]), + liveNotes: [ + createTestStoredNote({ id: "live-1", fileKey: "alpha", hunkIndex: 1, line: 11 }), + createTestStoredNote({ id: "live-2", fileKey: "beta", hunkIndex: 0, line: 1 }), + createTestStoredNote({ + id: "orphaned", + fileKey: "alpha", + hunkIndex: 0, + resolution: "orphaned" as const, + }), + ], + userNotes: [createTestStoredNote({ id: "user-1", fileKey: "alpha", hunkIndex: 1, line: 12 })], + }; + + const byHunk = selectNotesByHunk(state, "alpha"); + expect([...byHunk.keys()]).toEqual([1]); + expect(byHunk.get(1)?.map((note) => note.id)).toEqual(["live-1", "user-1"]); + }); + + // Intent: the reviewer's own draft is what a "jump to the note" reveal is about. + test("prefer an active draft in the selected hunk over stored notes", () => { + const state = { + ...createTestReviewState(["alpha"]), + selection: { fileKey: "alpha", hunkIndex: 0 }, + liveNotes: [createTestStoredNote({ id: "live-1", fileKey: "alpha", line: 2 })], + draftNote: { + id: "draft:1", + fileKey: "alpha", + hunkIndex: 0, + side: "new" as const, + line: 3, + body: "", + }, + }; + + expect(selectActiveRevealNoteId(state)).toBe("draft:1"); + // A draft being written in another hunk does not steal this hunk's reveal. + expect( + selectActiveRevealNoteId({ ...state, draftNote: { ...state.draftNote, hunkIndex: 1 } }), + ).toBe("live-1"); + }); + + test("otherwise take the earliest anchored note in the selected hunk", () => { + const state = { + ...createTestReviewState(["alpha"]), + selection: { fileKey: "alpha", hunkIndex: 0 }, + liveNotes: [ + createTestStoredNote({ id: "live-late", fileKey: "alpha", line: 3 }), + createTestStoredNote({ id: "live-early", fileKey: "alpha", line: 1 }), + ], + userNotes: [createTestStoredNote({ id: "user-late", fileKey: "alpha", line: 2 })], + }; + + expect(selectActiveRevealNoteId(state)).toBe("live-early"); + expect(selectActiveRevealNoteId({ ...state, liveNotes: [], userNotes: [] })).toBeUndefined(); + }); +}); diff --git a/src/core/review/selectors.ts b/src/core/review/selectors.ts index 2f21632f7..301a736ff 100644 --- a/src/core/review/selectors.ts +++ b/src/core/review/selectors.ts @@ -6,8 +6,18 @@ * differently. Selectors stay pure functions of state, and the ones that encode a rule * rather than a lookup say so by name. */ -import type { ReviewState, ReviewStoredNote } from "./state"; -import type { ReviewDocumentV1, ReviewFileV1 } from "./types"; +import { normalizeDiffPath } from "../diffPaths"; +import { reviewCanonicalHunkLine } from "./geometry"; +import type { ReviewNavigationFile } from "./navigation"; +import { + isRenderableStoredReviewNote, + reviewNoteAnchorLine, + reviewNoteOwnerHunkIndex, + type ReviewSemanticSelection, + type ReviewState, + type ReviewStoredNote, +} from "./state"; +import type { ReviewDocumentV1, ReviewFileV1, ReviewLineAddressV1, ReviewNoteV1 } from "./types"; /** Select one semantic file by key. */ export function selectReviewFileByKey( @@ -49,6 +59,172 @@ export function reviewFileKeysWithRetiredContent( ); } +/** The file facts the shared filter reads. */ +export type ReviewFilterFile = Pick; + +/** + * The one review filter matcher. + * + * A query matches a file's current path, the path it came from, or the agent's summary of + * it, case-insensitively. The fields are joined before matching, so a query may span the + * boundary between them — that is the terminal's long-standing behavior, kept exactly + * (`docs/browser-review-seam-audit.md`, B5), rather than three surfaces each searching a + * different subset of the same file. + * + * Paths are normalized first: a parser leaving a stray carriage return on a path must not + * decide whether that file matches. + */ +export function reviewFileMatchesFilter(file: ReviewFilterFile, filter: string) { + const query = filter.trim().toLowerCase(); + if (!query) { + return true; + } + + return [normalizeDiffPath(file.path), normalizeDiffPath(file.previousPath), file.agentSummary] + .filter(Boolean) + .join(" ") + .toLowerCase() + .includes(query); +} + +/** Select the files the current filter leaves visible, in review order. */ +export function selectVisibleReviewFiles( + state: Pick, +): ReviewFileV1[] { + return state.document.files.filter((file) => reviewFileMatchesFilter(file, state.filter)); +} + +/** Reduce the visible stream to what relative navigation walks over. */ +export function selectReviewNavigationFiles( + state: Pick, +): ReviewNavigationFile[] { + return selectVisibleReviewFiles(state).map((file) => ({ + fileKey: file.key, + hunkCount: file.hunks.length, + })); +} + +/** + * The file a selection falls back to when its own file is gone. + * + * The first visible file, or nothing at all. "Nothing" is a real answer: a review whose + * filter matches no file has no selection to offer, and inventing one would put the + * reviewer somewhere they never asked to be. + */ +export function selectFallbackFileKey( + state: Pick, +): string | null { + return selectVisibleReviewFiles(state)[0]?.key ?? null; +} + +/** + * The selection every consumer should read, normalized against the current document. + * + * Two rules, both of which the prototype's clients answered differently (B4): + * + * - A selected file the filter currently hides is still the selection. Filtering changes + * what the reviewer is browsing, not what they were last looking at, and quietly + * re-pointing the selection at another file would lose their place. + * - A selected file the document no longer has falls back to the first visible file, and + * to nothing when there is none — never to a hidden file. + * + * The hunk index is clamped rather than rejected, so a stale index from a file that was + * re-parsed smaller still lands on a real hunk. + */ +export function selectNormalizedSelection( + state: Pick, +): ReviewSemanticSelection { + const file = selectReviewFileByKey(state, state.selection.fileKey); + if (!file) { + return { fileKey: selectFallbackFileKey(state), hunkIndex: 0 }; + } + + return { + fileKey: file.key, + hunkIndex: Math.min(Math.max(state.selection.hunkIndex, 0), Math.max(0, file.hunks.length - 1)), + }; +} + +/** + * The line a reveal should bring into view for the current selection. + * + * Resolved from the selected hunk's backed sides rather than from whichever side happens + * to report a range: every hunk has a position on both sides, so testing for a range + * scrolls a pure-deletion hunk to a new-side line that does not exist (B6). Undefined + * means the selection has no hunk to reveal, and the caller should reveal the file + * instead. + */ +export function selectRevealTarget( + state: Pick, +): ReviewLineAddressV1 | undefined { + const hunk = selectReviewFileByKey(state, state.selection.fileKey)?.hunks[ + state.selection.hunkIndex + ]; + return hunk ? reviewCanonicalHunkLine(hunk) : undefined; +} + +/** Every mutable note currently safe to render, live notes before the reviewer's own. */ +function renderableNotes(state: Pick): ReviewNoteV1[] { + return [...state.liveNotes, ...state.userNotes] + .filter(isRenderableStoredReviewNote) + .map((entry) => entry.note); +} + +/** + * Group one file's notes by the hunk that renders them. + * + * Grouping reads the ownership the anchor resolver already decided; it never re-tests + * range containment. A note core placed through its fallback path — one anchored to an + * expanded context line, or to a range the current patch collapsed — has an owner but no + * intersecting hunk, and a consumer that re-filters by containment drops it (B8). + */ +export function selectNotesByHunk( + state: Pick, + fileKey: string, +): ReadonlyMap { + const byHunk = new Map(); + for (const note of renderableNotes(state)) { + if (note.fileKey !== fileKey) { + continue; + } + const hunkIndex = reviewNoteOwnerHunkIndex(note); + const notes = byHunk.get(hunkIndex); + if (notes) { + notes.push(note); + } else { + byHunk.set(hunkIndex, [note]); + } + } + return byHunk; +} + +/** + * Which note a "jump to the note" reveal targets. + * + * Named policy: an active draft in the selected hunk wins, because the reviewer is + * writing it right now; otherwise the note whose anchor sits earliest in the hunk, with + * arrival order breaking ties. Undefined means the selected hunk has nothing to reveal + * and the caller should fall back to revealing the hunk itself. + */ +export function selectActiveRevealNoteId( + state: Pick, +): string | undefined { + const { fileKey, hunkIndex } = state.selection; + if (fileKey === null) { + return undefined; + } + + const draft = state.draftNote; + if (draft && draft.fileKey === fileKey && draft.hunkIndex === hunkIndex) { + return draft.id; + } + + return renderableNotes(state) + .filter((note) => note.fileKey === fileKey && reviewNoteOwnerHunkIndex(note) === hunkIndex) + .map((note, arrival) => ({ note, arrival, line: reviewNoteAnchorLine(note).line })) + .sort((left, right) => left.line - right.line || left.arrival - right.arrival)[0]?.note.id; +} + /** Return whether one collapsed gap is currently expanded. */ export function isReviewGapExpanded( state: Pick, diff --git a/src/core/review/state.test.ts b/src/core/review/state.test.ts index cd3730f8a..c09f7247f 100644 --- a/src/core/review/state.test.ts +++ b/src/core/review/state.test.ts @@ -1,6 +1,10 @@ import { describe, expect, test } from "bun:test"; import { createTestReviewDocument } from "../../../test/helpers/review-store-helpers"; -import { createInitialReviewState, isRenderableStoredReviewNote } from "./state"; +import { + createInitialReviewState, + isRenderableStoredReviewNote, + reviewNoteVisibleByPolicy, +} from "./state"; import type { ReviewStoredNote } from "./state"; /** Build one minimal review document with the given file keys. */ @@ -44,6 +48,19 @@ describe("createInitialReviewState", () => { }); }); +describe("reviewNoteVisibleByPolicy", () => { + test("keeps the reviewer's own notes when the agent layer is hidden", () => { + expect(reviewNoteVisibleByPolicy({ source: "user" }, false)).toBe(true); + expect(reviewNoteVisibleByPolicy({ source: "agent" }, false)).toBe(false); + expect(reviewNoteVisibleByPolicy({ source: "ai" }, false)).toBe(false); + }); + + test("shows every note when the layer is on", () => { + expect(reviewNoteVisibleByPolicy({ source: "agent" }, true)).toBe(true); + expect(reviewNoteVisibleByPolicy({ source: "ai" }, true)).toBe(true); + }); +}); + describe("isRenderableStoredReviewNote", () => { test("keeps stale notes visible at their last known anchor", () => { expect(isRenderableStoredReviewNote(testStoredNote("active"))).toBe(true); diff --git a/src/core/review/state.ts b/src/core/review/state.ts index d52ee3a6e..c5afd4a67 100644 --- a/src/core/review/state.ts +++ b/src/core/review/state.ts @@ -29,6 +29,21 @@ export function isRenderableStoredReviewNote(entry: ReviewStoredNote) { return entry.resolution !== "orphaned"; } +/** + * The one note-layer visibility rule. + * + * Hiding the note layer hides what the review was given, not what the reviewer wrote: + * their own notes are their working state and stay on screen. Stated once over the + * normalized source, so no surface can answer it from a raw producer label + * (`docs/browser-review-seam-audit.md`, B9). + */ +export function reviewNoteVisibleByPolicy( + note: Pick, + showAgentNotes: boolean, +) { + return showAgentNotes || note.source === "user"; +} + /** * Which hunk renders one note. * @@ -80,6 +95,19 @@ export interface ReviewRevealIntent { scrollToNote: boolean; } +/** + * The viewport-anchor policy: adopt what the viewport already settled on. + * + * A renderer that scrolls and then publishes the hunk it came to rest on is reporting + * where the reviewer is, not asking to be moved. Anchoring therefore requests no anchor + * and clears any note preference, so the counters stay put and no other attached surface + * is scrolled by this one's scrolling (`docs/browser-review-seam-audit.md`, B11). + */ +export const REVIEW_VIEWPORT_ANCHOR_REVEAL: ReviewRevealRequest = Object.freeze({ + anchor: "none", + scrollToNote: false, +}); + /** Advance the reveal counters one selection request asks for. */ export function applyReviewRevealRequest( current: ReviewRevealIntent, diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 324f302cf..4c6b25ed7 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -520,15 +520,9 @@ export function App({ () => extensionSelectionInputsRef.current.selectedFileId, [], ); - const moveToAnnotatedFile = review.moveToAnnotatedFile; - const moveToAnnotatedHunk = review.moveToAnnotatedHunk; - const moveToFile = review.moveToFile; - const jumpToFile = useCallback( - (fileId: string, nextHunkIndex = 0, options?: { alignFileHeaderTop?: boolean }) => { - review.selectFile(fileId, nextHunkIndex, { - alignFileHeaderTop: options?.alignFileHeaderTop, - }); + (fileId: string, options?: { alignFileHeaderTop?: boolean }) => { + review.selectFile(fileId, { alignFileHeaderTop: options?.alignFileHeaderTop }); }, [review.selectFile], ); @@ -1715,7 +1709,7 @@ export function App({ extensionCommandNavigationRef.current = { onSelectFile: (fileId) => { focusFiles(); - jumpToFile(fileId, 0, { alignFileHeaderTop: true }); + jumpToFile(fileId, { alignFileHeaderTop: true }); }, onSelectHunk: (fileId, hunkIndex) => { focusFiles(); @@ -1828,9 +1822,9 @@ export function App({ alignCurrentLine, applyFilePresentationToAllMatching, focusFilter, - moveToAnnotatedFile, - moveToAnnotatedHunk, - moveToFile, + moveToAnnotatedFile: review.moveToAnnotatedFile, + moveToAnnotatedHunk: review.moveToAnnotatedHunk, + moveToFile: review.moveToFile, moveToHunk: review.moveToHunk, openAgentSkill, openThemeSelector, @@ -2047,7 +2041,7 @@ export function App({ notify={(message, type) => extensions?.context.notify(message, type)} onSelectFile={(fileId) => { focusFiles(); - jumpToFile(fileId, 0, { alignFileHeaderTop: true }); + jumpToFile(fileId, { alignFileHeaderTop: true }); }} onSelectHunk={(fileId, hunkIndex) => { focusFiles(); @@ -2207,7 +2201,7 @@ export function App({ onSelectFile={jumpToFile} onToggleGap={review.toggleGap} onViewportCenteredHunkChange={(fileId, hunkIndex) => - review.selectHunk(fileId, hunkIndex, { preserveViewport: true }) + review.anchorSelection(fileId, hunkIndex) } onLineCursorsChange={setLineCursors} currentLinePaintRequested={currentLinePaintRequested} diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 8f756a2ef..3be4cc1a7 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -21,15 +21,12 @@ import type { LayoutMode, UserNoteLineTarget, } from "../../../core/types"; +import { reviewNoteVisibleByPolicy } from "../../../core/review/state"; import type { FileSourceStatus } from "../../diff/expandCollapsedRows"; import type { ActiveAddNoteAffordance } from "../../diff/PierreDiffView"; import type { CursorHighlight } from "../../diff/renderRows"; import type { DraftReviewNote } from "../../lib/reviewProjection"; -import { - alwaysShowReviewNote, - reviewNoteSource, - type VisibleAgentNote, -} from "../../lib/agentAnnotations"; +import { reviewNoteSource, type VisibleAgentNote } from "../../lib/agentAnnotations"; import { computeRapidScrollOverscanRows, RAPID_SCROLL_OVERSCAN_IDLE_MS, @@ -471,7 +468,10 @@ export function DiffPane({ files.forEach((file) => { const annotations = (file.agent?.annotations ?? []).filter( - (annotation) => showAgentNotes || alwaysShowReviewNote(annotation), + // One shared visibility rule over the normalized note source, so the terminal and + // any other surface hide the same notes when the layer is off. + (annotation) => + reviewNoteVisibleByPolicy({ source: reviewNoteSource(annotation) }, showAgentNotes), ); const notes: VisibleAgentNote[] = annotations.map((annotation, index) => { const source = reviewNoteSource(annotation); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 8819f838f..9ba7a8b54 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -34,10 +34,13 @@ import { import { projectReviewDocument } from "../../core/review/document"; import { reviewExpansionSide, reviewTrailingGap } from "../../core/review/expansion"; import { reviewDefaultHunkLineTarget } from "../../core/review/geometry"; +import type { ReviewSelectionScope } from "../../core/review/navigation"; import { isReviewGapExpanded, reviewFileKeysWithRetiredContent, selectExpandedGapIdsByFileKey, + selectFallbackFileKey, + selectNormalizedSelection, } from "../../core/review/selectors"; import type { ReviewDraftNote, ReviewRevealRequest } from "../../core/review/state"; import { createReviewStore, type ReviewStore } from "../../core/review/store"; @@ -59,7 +62,6 @@ import type { import type { FileSourceStatus } from "../diff/expandCollapsedRows"; import { selectGapForKeyboardToggle } from "../diff/expandCollapsedRows"; -import { findNextHunkCursor } from "../lib/hunks"; import { EMPTY_LINE_CURSORS, findNextLineCursor, @@ -82,18 +84,12 @@ import { type UserReviewNote, } from "../lib/reviewProjection"; import { + buildReviewAnnotationIndex, buildReviewStreamState, buildSelectedHunkSummary, - findNextAnnotatedFile, resolveReviewNavigationTarget, - resolveSelectedFile, } from "../lib/reviewState"; -/** Clamp one numeric index into an inclusive range. */ -function clamp(value: number, min: number, max: number) { - return Math.min(Math.max(value, min), max); -} - /** Merge file-id keyed annotation maps without losing their concrete item types. */ function mergeAnnotationMaps( first: Record, @@ -146,19 +142,18 @@ interface SourceLoadRequest { export interface ReviewSelectionOptions { alignFileHeaderTop?: boolean; - preserveViewport?: boolean; scrollToNote?: boolean; } /** * Translate the terminal's selection options into the shared reveal request. * - * File-header alignment outranks viewport preservation: a caller passing both is - * crossing into another file, where the header is what belongs on screen. + * Selections that deliberately leave the viewport alone do not come through here at all: + * that is the viewport-anchor policy, expressed as its own intent. */ function revealRequestFor(options?: ReviewSelectionOptions): ReviewRevealRequest { return { - anchor: options?.alignFileHeaderTop ? "file-top" : options?.preserveViewport ? "none" : "hunk", + anchor: options?.alignFileHeaderTop ? "file-top" : "hunk", scrollToNote: Boolean(options?.scrollToNote), }; } @@ -178,7 +173,11 @@ export interface ReviewController { lineCursor: LineCursor | null; lineCursorRevealRequestId: number; anchorLineCursor: (cursor: LineCursor) => void; + /** Adopt the hunk a viewport settled on, without asking any viewport to move. */ + anchorSelection: (fileId: string, hunkIndex: number) => void; moveLineCursor: (delta: number) => void; + /** Step the selection through one navigable scope; the scope owns wrap and reveal. */ + moveSelection: (scope: ReviewSelectionScope, delta: number) => void; moveToAnnotatedFile: (delta: number) => void; moveToAnnotatedHunk: (delta: number) => void; moveToFile: (delta: number) => void; @@ -214,7 +213,8 @@ export interface ReviewController { cancelDraftNote: () => void; removeUserNote: (noteId: string) => void; saveDraftNote: () => UserReviewNote | null; - selectFile: (fileId: string, nextHunkIndex?: number, options?: ReviewSelectionOptions) => void; + /** Jump to one file; the shared file-jump rule decides which hunk it lands on. */ + selectFile: (fileId: string, options?: ReviewSelectionOptions) => void; selectHunk: (fileId: string, hunkIndex: number, options?: ReviewSelectionOptions) => void; setShowAgentNotes: (visible: boolean) => void; startUserNote: ( @@ -361,7 +361,7 @@ export function useReviewController({ }, [fileByKey, state.sourceStatusByFileKey]); const deferredFilter = useDeferredValue(filter); - const { allFiles, visibleFiles, hunkCursors, annotatedHunkCursors } = useMemo( + const { allFiles, visibleFiles } = useMemo( () => buildReviewStreamState({ files, @@ -370,13 +370,22 @@ export function useReviewController({ }), [deferredFilter, files, liveCommentsByFileId, userNotesByFileId], ); + // Which files and hunks carry notes, for the shared annotated-navigation planner. Built + // from the merged stream, so a live comment that just arrived is navigable immediately. + const annotations = useMemo( + () => buildReviewAnnotationIndex(allFiles, keyByFileId), + [allFiles, keyByFileId], + ); + // The shared normalization rule, not a terminal copy: a selected file the filter hides + // is still the selection, and only a file the document lost falls back to the first + // visible one. + const normalizedSelection = selectNormalizedSelection(state); const selectedFileId = state.selection.fileKey ? (fileByKey.get(state.selection.fileKey)?.id ?? "") : ""; - const selectedFile = useMemo( - () => resolveSelectedFile(allFiles, visibleFiles, selectedFileId), - [allFiles, selectedFileId, visibleFiles], - ); + const selectedFile = normalizedSelection.fileKey + ? fileByKey.get(normalizedSelection.fileKey) + : undefined; const selectedHunk = selectedFile?.metadata.hunks[selectedHunkIndex]; /** Run one semantic intent against the review store. */ @@ -404,12 +413,36 @@ export function useReviewController({ [keyByFileId, runIntent], ); - /** Select one file and optionally one specific hunk within it. */ + /** + * Adopt a selection the viewport arrived at on its own. + * + * Scrolling reports where the reviewer is; it never asks to be scrolled back, and with + * several surfaces attached to one review it must not move anybody else's viewport + * either. That is the shared anchor policy rather than a local "preserve viewport" flag. + */ + const anchorSelection = useCallback( + (fileId: string, hunkIndex: number) => { + const fileKey = keyByFileId.get(fileId); + if (!fileKey) { + return; + } + + runIntent({ type: "selection/anchor", fileKey, hunkIndex }); + }, + [keyByFileId, runIntent], + ); + + /** Jump to one file through the shared file-jump rule. */ const selectFile = useCallback( - (fileId: string, nextHunkIndex = 0, options?: ReviewSelectionOptions) => { - selectHunk(fileId, nextHunkIndex, options); + (fileId: string, options?: ReviewSelectionOptions) => { + const fileKey = keyByFileId.get(fileId); + if (!fileKey) { + return; + } + + runIntent({ type: "selection/select-file", fileKey, reveal: revealRequestFor(options) }); }, - [selectHunk], + [keyByFileId, runIntent], ); /** @@ -419,17 +452,15 @@ export function useReviewController({ * the reviewer's behalf, and must not also move the viewport under them. */ const reconcileSelection = useCallback(() => { - const selection = store.getSnapshot().selection; + const snapshot = store.getSnapshot(); + const selection = snapshot.selection; const selected = selection.fileKey ? fileByKey.get(selection.fileKey) : undefined; const isVisible = selected !== undefined && visibleFiles.some((file) => file.id === selected.id); - const fallback = visibleFiles[0]; + const fallbackKey = selectFallbackFileKey(snapshot); - if (!isVisible && fallback) { - const fallbackKey = keyByFileId.get(fallback.id); - if (fallbackKey) { - store.dispatch({ type: "selection/select", fileKey: fallbackKey, hunkIndex: 0 }); - } + if (!isVisible && fallbackKey) { + store.dispatch({ type: "selection/select", fileKey: fallbackKey, hunkIndex: 0 }); return; } @@ -440,7 +471,7 @@ export function useReviewController({ hunkIndex: selection.hunkIndex, }); } - }, [fileByKey, keyByFileId, store, visibleFiles]); + }, [fileByKey, store, visibleFiles]); useEffect(() => { reconcileSelection(); @@ -463,9 +494,10 @@ export function useReviewController({ (cursor: LineCursor) => { applyLineCursor(cursor); setLineCursorRevealRequestId((current) => current + 1); - selectHunk(cursor.fileId, cursor.hunkIndex, { preserveViewport: true }); + // The line cursor carries its own reveal request; the selection only follows it. + anchorSelection(cursor.fileId, cursor.hunkIndex); }, - [applyLineCursor, selectHunk], + [anchorSelection, applyLineCursor], ); const reconcileLineCursor = useCallback(() => { @@ -523,9 +555,9 @@ export function useReviewController({ const anchorLineCursor = useCallback( (cursor: LineCursor) => { applyLineCursor(cursor); - selectHunk(cursor.fileId, cursor.hunkIndex, { preserveViewport: true }); + anchorSelection(cursor.fileId, cursor.hunkIndex); }, - [applyLineCursor, selectHunk], + [anchorSelection, applyLineCursor], ); /** Move the current line one row through the visible review stream. */ @@ -541,84 +573,17 @@ export function useReviewController({ [lineCursors, revealLineCursor], ); - /** Move through the full visible review stream one hunk at a time. */ - const moveToHunk = useCallback( - (delta: number) => { - const nextCursor = findNextHunkCursor( - hunkCursors, - selectedFile?.id, - selectedHunkIndex, - delta, - ); - if (!nextCursor) { - return; - } - - const crossingFileBoundary = nextCursor.fileId !== selectedFile?.id; - selectHunk(nextCursor.fileId, nextCursor.hunkIndex, { - // Align the file header to top only for forward cross-file jumps so the new file - // starts at its header. Backward jumps should reveal the target hunk directly, - // since the target is often near the bottom of the previous file and the file-top - // align would require an extra navigation press to reach it. - alignFileHeaderTop: crossingFileBoundary && delta > 0, - }); - }, - [hunkCursors, selectHunk, selectedFile?.id, selectedHunkIndex], - ); - - /** Move through only hunks that currently have agent notes or live comments. */ - const moveToAnnotatedHunk = useCallback( - (delta: number) => { - const nextCursor = findNextHunkCursor( - annotatedHunkCursors, - selectedFile?.id, - selectedHunkIndex, - delta, - hunkCursors, - ); - if (!nextCursor) { - return; - } - - selectHunk(nextCursor.fileId, nextCursor.hunkIndex, { scrollToNote: true }); - }, - [annotatedHunkCursors, hunkCursors, selectHunk, selectedFile?.id, selectedHunkIndex], - ); - - /** Cycle through only the currently visible files that carry annotations. */ - const moveToAnnotatedFile = useCallback( - (delta: number) => { - const nextFile = findNextAnnotatedFile(visibleFiles, selectedFile?.id, delta); - if (!nextFile) { - return; - } - - selectFile(nextFile.id); - }, - [selectFile, selectedFile?.id, visibleFiles], - ); - - /** Move through all currently visible files without wrapping past either end. */ - const moveToFile = useCallback( - (delta: number) => { - const currentIndex = visibleFiles.findIndex((file) => file.id === selectedFile?.id); - if (currentIndex < 0) { - return; - } - - const nextIndex = clamp(currentIndex + delta, 0, visibleFiles.length - 1); - if (nextIndex === currentIndex) { - return; - } - - const nextFile = visibleFiles[nextIndex]; - if (!nextFile) { - return; - } - - selectFile(nextFile.id, 0, { alignFileHeaderTop: true }); - }, - [selectFile, selectedFile?.id, visibleFiles], + /** + * Step the selection through one navigable scope. + * + * The walk itself — which hunk or file is next, whether the scope wraps, and what the + * landing asks the viewport to reveal — lives in the shared planner, so the keyboard, + * the session's comment navigation, and later a browser client all move identically. + */ + const moveSelection = useCallback( + (scope: ReviewSelectionScope, delta: number) => + runIntent({ type: "selection/move", scope, delta }, { annotations }), + [annotations, runIntent], ); /** Set the shared file filter. */ @@ -753,18 +718,48 @@ export function useReviewController({ } }, [selectedFile, selectedHunkIndex, toggleGap]); - /** Resolve one session-daemon navigation request against the current review state and select it. */ + /** Named scopes the keyboard binds today, each one step of the shared walk. */ + const moveToHunk = useCallback((delta: number) => moveSelection("hunk", delta), [moveSelection]); + const moveToFile = useCallback((delta: number) => moveSelection("file", delta), [moveSelection]); + const moveToAnnotatedHunk = useCallback( + (delta: number) => moveSelection("annotated-hunk", delta), + [moveSelection], + ); + const moveToAnnotatedFile = useCallback( + (delta: number) => moveSelection("annotated-file", delta), + [moveSelection], + ); + + /** + * Resolve one session-daemon navigation request against the current review and select it. + * + * Relative comment navigation is the same walk the keyboard performs and goes through + * the shared planner; only absolute addressing (a path plus a hunk or a line) is + * resolved against the terminal's diff-file model here. + */ const navigateToLocation = useCallback( (input: NavigateToHunkToolInput): NavigatedSelectionResult => { - const target = resolveReviewNavigationTarget({ - allFiles, - currentFileId: selectedFile?.id, - currentHunkIndex: selectedHunkIndex, - input, - visibleFiles, - }); + if (input.commentDirection) { + const moved = moveSelection("annotated-hunk", input.commentDirection === "next" ? 1 : -1); + if (!moved) { + throw new Error("No annotated hunks found in the current review."); + } + + const file = fileByKey.get(moved.fileKey); + if (!file) { + throw new Error("Resolved annotated hunk references an unknown file."); + } - selectHunk(target.file.id, target.hunkIndex, { scrollToNote: target.scrollToNote }); + return { + fileId: file.id, + filePath: file.path, + hunkIndex: moved.hunkIndex, + selectedHunk: buildSelectedHunkSummary(file, moved.hunkIndex), + }; + } + + const target = resolveReviewNavigationTarget({ allFiles, input }); + selectHunk(target.file.id, target.hunkIndex); return { fileId: target.file.id, filePath: target.file.path, @@ -772,7 +767,7 @@ export function useReviewController({ selectedHunk: buildSelectedHunkSummary(target.file, target.hunkIndex), }; }, - [allFiles, selectHunk, selectedFile?.id, selectedHunkIndex, visibleFiles], + [allFiles, fileByKey, moveSelection, selectHunk], ); /** @@ -992,11 +987,11 @@ export function useReviewController({ body: "", }; store.dispatch({ type: "draft/start", draft }); - selectHunk( - file.id, - hunkIndex, - options?.preserveViewport ? { preserveViewport: true } : { scrollToNote: true }, - ); + if (options?.preserveViewport) { + anchorSelection(file.id, hunkIndex); + } else { + selectHunk(file.id, hunkIndex, { scrollToNote: true }); + } return storedDraftToDraftNote(draft, file); }, [ @@ -1162,10 +1157,12 @@ export function useReviewController({ addLiveComment, addLiveCommentBatch, anchorLineCursor, + anchorSelection, clearFilter, cancelDraftNote, clearLiveComments, moveLineCursor, + moveSelection, moveToAnnotatedFile, moveToAnnotatedHunk, moveToFile, diff --git a/src/ui/lib/agentAnnotations.ts b/src/ui/lib/agentAnnotations.ts index 51f38a310..662659cf3 100644 --- a/src/ui/lib/agentAnnotations.ts +++ b/src/ui/lib/agentAnnotations.ts @@ -38,11 +38,6 @@ export function reviewNoteSource(annotation: AgentAnnotation): ReviewNoteSource return "ai"; } -/** Return whether a note should remain visible when the AI note layer is hidden. */ -export function alwaysShowReviewNote(annotation: AgentAnnotation) { - return reviewNoteSource(annotation) === "user"; -} - /** Check whether two inclusive line ranges overlap. */ function overlap(rangeA: [number, number], rangeB: [number, number]) { return rangeA[0] <= rangeB[1] && rangeB[0] <= rangeA[1]; diff --git a/src/ui/lib/files.ts b/src/ui/lib/files.ts index 2799cf0cd..38cc77b79 100644 --- a/src/ui/lib/files.ts +++ b/src/ui/lib/files.ts @@ -121,26 +121,6 @@ export function mergeFileAnnotationsByFileId( }); } -/** Apply the app's file filter query to the visible review stream. */ -export function filterReviewFiles(files: DiffFile[], query: string): DiffFile[] { - const trimmedQuery = query.trim().toLowerCase(); - if (!trimmedQuery) { - return files; - } - - return files.filter((file) => { - const haystack = [ - normalizeDiffPath(file.path), - normalizeDiffPath(file.previousPath), - file.agent?.summary, - ] - .filter(Boolean) - .join(" ") - .toLowerCase(); - return haystack.includes(trimmedQuery); - }); -} - /** Build the grouped sidebar entries while preserving the review stream order. */ export function buildSidebarEntries(files: readonly SidebarFileSource[]): SidebarEntry[] { const entries: SidebarEntry[] = []; diff --git a/src/ui/lib/hunks.test.ts b/src/ui/lib/hunks.test.ts deleted file mode 100644 index e3dcaa646..000000000 --- a/src/ui/lib/hunks.test.ts +++ /dev/null @@ -1,224 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { parseDiffFromFile } from "@pierre/diffs"; -import type { DiffFile } from "../../core/types"; -import { buildAnnotatedHunkCursors, findNextHunkCursor, type HunkCursor } from "./hunks"; - -/** Build a minimal DiffFile with real Pierre-parsed hunks and optional annotations. */ -function createTestFile( - id: string, - path: string, - before: string, - after: string, - annotations: DiffFile["agent"], -): DiffFile { - const metadata = parseDiffFromFile( - { name: path, contents: before, cacheKey: `${id}:before` }, - { name: path, contents: after, cacheKey: `${id}:after` }, - { context: 3 }, - true, - ); - - return { - id, - path, - patch: "", - language: "typescript", - stats: { additions: 0, deletions: 0 }, - metadata, - agent: annotations, - }; -} - -describe("hunk navigation", () => { - const cursors: HunkCursor[] = [ - { fileId: "alpha", hunkIndex: 0 }, - { fileId: "alpha", hunkIndex: 1 }, - { fileId: "beta", hunkIndex: 0 }, - ]; - - test("moves forward across hunk and file boundaries", () => { - expect(findNextHunkCursor(cursors, "alpha", 0, 1)).toEqual({ fileId: "alpha", hunkIndex: 1 }); - expect(findNextHunkCursor(cursors, "alpha", 1, 1)).toEqual({ fileId: "beta", hunkIndex: 0 }); - }); - - test("moves backward across file boundaries", () => { - expect(findNextHunkCursor(cursors, "beta", 0, -1)).toEqual({ fileId: "alpha", hunkIndex: 1 }); - expect(findNextHunkCursor(cursors, "alpha", 1, -1)).toEqual({ fileId: "alpha", hunkIndex: 0 }); - }); - - test("applies multi-step movement atomically and clamps at the target", () => { - expect(findNextHunkCursor(cursors, "alpha", 0, 2)).toEqual({ - fileId: "beta", - hunkIndex: 0, - }); - expect(findNextHunkCursor(cursors, "beta", 0, -2)).toEqual({ - fileId: "alpha", - hunkIndex: 0, - }); - }); - - test("clamps at the ends of the review stream", () => { - expect(findNextHunkCursor(cursors, "alpha", 0, -1)).toEqual({ fileId: "alpha", hunkIndex: 0 }); - expect(findNextHunkCursor(cursors, "beta", 0, 1)).toEqual({ fileId: "beta", hunkIndex: 0 }); - }); - - test("starts at the nearest stream edge when no current hunk is selected", () => { - expect(findNextHunkCursor(cursors, undefined, 0, 1)).toEqual({ fileId: "alpha", hunkIndex: 0 }); - expect(findNextHunkCursor(cursors, undefined, 0, -1)).toEqual({ fileId: "beta", hunkIndex: 0 }); - }); -}); - -describe("annotated hunk navigation", () => { - // Two-hunk file: lines 1-10 change in hunk 0, lines 20-30 change in hunk 1. - const beforeA = - "line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n" + - "gap11\ngap12\ngap13\ngap14\ngap15\ngap16\ngap17\ngap18\ngap19\n" + - "line20\nline21\nline22\nline23\nline24\nline25\nline26\nline27\nline28\nline29\nline30\n"; - const afterA = - "CHANGED1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\n" + - "gap11\ngap12\ngap13\ngap14\ngap15\ngap16\ngap17\ngap18\ngap19\n" + - "CHANGED20\nline21\nline22\nline23\nline24\nline25\nline26\nline27\nline28\nline29\nline30\n"; - - // Single-hunk file: one change at line 1. - const beforeB = "old\n"; - const afterB = "new\n"; - - test("only includes hunks that have overlapping annotations", () => { - // Hunk 0 new range is [1,1], hunk 1 new range is [17,17]. - // Annotate only hunk 1 in file alpha, and hunk 0 in file beta. - const fileA = createTestFile("alpha", "alpha.ts", beforeA, afterA, { - path: "alpha.ts", - annotations: [{ newRange: [17, 17], summary: "Note on hunk 1" }], - }); - const fileB = createTestFile("beta", "beta.ts", beforeB, afterB, { - path: "beta.ts", - annotations: [{ newRange: [1, 1], summary: "Note on beta" }], - }); - - expect(fileA.metadata.hunks.length).toBe(2); - const annotatedCursors = buildAnnotatedHunkCursors([fileA, fileB]); - - // Alpha hunk 0 (line 1) has no annotation, so it should be skipped. - expect(annotatedCursors).toEqual([ - { fileId: "alpha", hunkIndex: 1 }, - { fileId: "beta", hunkIndex: 0 }, - ]); - }); - - test("returns an empty list when no files have annotations", () => { - const fileA = createTestFile("alpha", "alpha.ts", beforeA, afterA, null); - const fileB = createTestFile("beta", "beta.ts", beforeB, afterB, null); - - expect(buildAnnotatedHunkCursors([fileA, fileB])).toEqual([]); - }); - - test("skips files with agent context but no matching annotations", () => { - // Annotation range doesn't overlap any hunk (line 10 is in the gap between hunks). - const fileA = createTestFile("alpha", "alpha.ts", beforeA, afterA, { - path: "alpha.ts", - annotations: [{ newRange: [10, 10], summary: "Note in gap, no hunk overlap" }], - }); - - expect(buildAnnotatedHunkCursors([fileA])).toEqual([]); - }); - - test("navigates forward and backward through annotated cursors only", () => { - // Annotate only hunk 1 (new range [17,17]) in alpha, and hunk 0 in beta. - const fileA = createTestFile("alpha", "alpha.ts", beforeA, afterA, { - path: "alpha.ts", - annotations: [{ newRange: [17, 17], summary: "Note on hunk 1 only" }], - }); - const fileB = createTestFile("beta", "beta.ts", beforeB, afterB, { - path: "beta.ts", - annotations: [{ newRange: [1, 1], summary: "Note on beta" }], - }); - - const annotatedCursors = buildAnnotatedHunkCursors([fileA, fileB]); - - // Forward from alpha hunk 1 → beta hunk 0 - expect(findNextHunkCursor(annotatedCursors, "alpha", 1, 1)).toEqual({ - fileId: "beta", - hunkIndex: 0, - }); - - // Backward from beta hunk 0 → alpha hunk 1 - expect(findNextHunkCursor(annotatedCursors, "beta", 0, -1)).toEqual({ - fileId: "alpha", - hunkIndex: 1, - }); - - // Clamps at ends - expect(findNextHunkCursor(annotatedCursors, "alpha", 1, -1)).toEqual({ - fileId: "alpha", - hunkIndex: 1, - }); - expect(findNextHunkCursor(annotatedCursors, "beta", 0, 1)).toEqual({ - fileId: "beta", - hunkIndex: 0, - }); - }); - - test("jumps from an unannotated hunk to the nearest annotated one", () => { - // Only hunk 1 (new range [17,17]) is annotated; hunk 0 is not. - const fileA = createTestFile("alpha", "alpha.ts", beforeA, afterA, { - path: "alpha.ts", - annotations: [{ newRange: [17, 17], summary: "Note on hunk 1 only" }], - }); - - const annotatedCursors = buildAnnotatedHunkCursors([fileA]); - - // Current position is alpha hunk 0, which is not in the annotated list. - // Forward should land on the first annotated cursor. - expect(findNextHunkCursor(annotatedCursors, "alpha", 0, 1)).toEqual({ - fileId: "alpha", - hunkIndex: 1, - }); - - // Backward from an unknown position should land on the last annotated cursor. - expect(findNextHunkCursor(annotatedCursors, "alpha", 0, -1)).toEqual({ - fileId: "alpha", - hunkIndex: 1, - }); - }); - - test("uses full stream position when annotated navigation starts on an unannotated hunk", () => { - const streamCursors: HunkCursor[] = [ - { fileId: "alpha", hunkIndex: 0 }, - { fileId: "alpha", hunkIndex: 1 }, - { fileId: "beta", hunkIndex: 0 }, - { fileId: "gamma", hunkIndex: 0 }, - { fileId: "gamma", hunkIndex: 1 }, - { fileId: "omega", hunkIndex: 0 }, - ]; - const annotatedCursors: HunkCursor[] = [ - { fileId: "alpha", hunkIndex: 1 }, - { fileId: "gamma", hunkIndex: 0 }, - { fileId: "gamma", hunkIndex: 1 }, - ]; - - expect(findNextHunkCursor(annotatedCursors, "beta", 0, 1, streamCursors)).toEqual({ - fileId: "gamma", - hunkIndex: 0, - }); - expect(findNextHunkCursor(annotatedCursors, "beta", 0, -1, streamCursors)).toEqual({ - fileId: "alpha", - hunkIndex: 1, - }); - expect(findNextHunkCursor(annotatedCursors, "alpha", 0, 2, streamCursors)).toEqual({ - fileId: "gamma", - hunkIndex: 0, - }); - expect(findNextHunkCursor(annotatedCursors, "alpha", 0, 1, streamCursors)).toEqual({ - fileId: "alpha", - hunkIndex: 1, - }); - expect(findNextHunkCursor(annotatedCursors, "gamma", 1, 1, streamCursors)).toEqual({ - fileId: "gamma", - hunkIndex: 1, - }); - expect(findNextHunkCursor(annotatedCursors, "omega", 0, 1, streamCursors)).toEqual({ - fileId: "gamma", - hunkIndex: 1, - }); - }); -}); diff --git a/src/ui/lib/hunks.ts b/src/ui/lib/hunks.ts deleted file mode 100644 index 7bf30ca41..000000000 --- a/src/ui/lib/hunks.ts +++ /dev/null @@ -1,98 +0,0 @@ -import type { DiffFile } from "../../core/types"; -import { getAnnotatedHunkIndices } from "./agentAnnotations"; - -export interface HunkCursor { - fileId: string; - hunkIndex: number; -} - -/** Flatten the visible files into one review-stream hunk cursor list. */ -export function buildHunkCursors(files: DiffFile[]): HunkCursor[] { - return files.flatMap((file) => - file.metadata.hunks.map((_, hunkIndex) => ({ fileId: file.id, hunkIndex })), - ); -} - -/** Flatten only the annotated hunks into a cursor list for comment navigation. */ -export function buildAnnotatedHunkCursors(files: DiffFile[]): HunkCursor[] { - return files.flatMap((file) => { - const annotated = getAnnotatedHunkIndices(file); - return file.metadata.hunks - .map((_, hunkIndex) => ({ fileId: file.id, hunkIndex })) - .filter((cursor) => annotated.has(cursor.hunkIndex)); - }); -} - -/** Move forward or backward through the review-stream hunk cursor list. */ -export function findNextHunkCursor( - cursors: HunkCursor[], - currentFileId: string | undefined, - currentHunkIndex: number, - delta: number, - streamCursors: HunkCursor[] = cursors, -): HunkCursor | null { - if (cursors.length === 0) { - return null; - } - - const currentIndex = cursors.findIndex( - (cursor) => cursor.fileId === currentFileId && cursor.hunkIndex === currentHunkIndex, - ); - const nextIndex = - currentIndex >= 0 - ? Math.min(Math.max(currentIndex + delta, 0), cursors.length - 1) - : findNearestCursorIndex(cursors, streamCursors, currentFileId, currentHunkIndex, delta); - - return cursors[nextIndex] ?? null; -} - -/** Resolve relative movement when the current hunk is not in the target cursor subset. */ -function findNearestCursorIndex( - cursors: HunkCursor[], - streamCursors: HunkCursor[], - currentFileId: string | undefined, - currentHunkIndex: number, - delta: number, -) { - if (!currentFileId) { - return delta >= 0 ? 0 : cursors.length - 1; - } - - const currentStreamIndex = streamCursors.findIndex( - (cursor) => cursor.fileId === currentFileId && cursor.hunkIndex === currentHunkIndex, - ); - if (currentStreamIndex < 0) { - return delta >= 0 ? 0 : cursors.length - 1; - } - - const streamIndexByCursor = new Map( - streamCursors.map((cursor, index) => [`${cursor.fileId}\0${cursor.hunkIndex}`, index] as const), - ); - const cursorStreamIndex = (cursor: HunkCursor) => - streamIndexByCursor.get(`${cursor.fileId}\0${cursor.hunkIndex}`) ?? -1; - const indexedCursors = cursors - .map((cursor, index) => ({ index, streamIndex: cursorStreamIndex(cursor) })) - .filter(({ streamIndex }) => streamIndex >= 0); - - if (indexedCursors.length === 0) { - return delta >= 0 ? 0 : cursors.length - 1; - } - - // Comment navigation is non-cyclic like normal hunk navigation, so positions outside - // the annotated span clamp to the nearest annotated edge instead of wrapping. - const remainingSteps = Math.max(0, Math.abs(delta) - 1); - if (delta >= 0) { - const nextCursor = indexedCursors.find(({ streamIndex }) => streamIndex > currentStreamIndex); - const nearestIndex = nextCursor?.index ?? indexedCursors[indexedCursors.length - 1]!.index; - return Math.min(nearestIndex + remainingSteps, cursors.length - 1); - } - - for (let index = indexedCursors.length - 1; index >= 0; index -= 1) { - const indexedCursor = indexedCursors[index]!; - if (indexedCursor.streamIndex < currentStreamIndex) { - return Math.max(0, indexedCursor.index - remainingSteps); - } - } - - return 0; -} diff --git a/src/ui/lib/reviewState.test.ts b/src/ui/lib/reviewState.test.ts index 0b3c47100..dcd2b10ff 100644 --- a/src/ui/lib/reviewState.test.ts +++ b/src/ui/lib/reviewState.test.ts @@ -1,8 +1,9 @@ import { describe, expect, test } from "bun:test"; import { createTestAgentFileContext, createTestDiffFile } from "../../../test/helpers/diff-helpers"; import { + buildReviewAnnotationIndex, + buildReviewStreamState, buildSelectedHunkSummary, - findNextAnnotatedFile, resolveReviewNavigationTarget, } from "./reviewState"; @@ -26,32 +27,51 @@ describe("review state helpers", () => { expect(buildSelectedHunkSummary(file, 99)).toEqual({ index: 99 }); }); - // Intent: annotated-file navigation wraps predictably and handles no-note streams. - test("findNextAnnotatedFile wraps through annotated files and handles empty streams", () => { - const alpha = createAnnotatedFile("alpha", "alpha.ts"); - const beta = createTestDiffFile({ id: "beta", path: "beta.ts", agent: null }); - const gamma = createAnnotatedFile("gamma", "gamma.ts"); + // Intent: the visible stream answers the same query the shared filter matcher does. + test("buildReviewStreamState filters on path, previous path, and agent summary", () => { + const alpha = createTestDiffFile({ id: "alpha", path: "src/alpha.ts" }); + const beta = createTestDiffFile({ + id: "beta", + path: "src/beta.ts", + previousPath: "src/legacy-name.ts", + }); + const gamma = createAnnotatedFile("gamma", "src/gamma.ts"); + const files = [alpha, beta, gamma]; - expect(findNextAnnotatedFile([alpha, beta, gamma], "alpha", 1)).toBe(gamma); - expect(findNextAnnotatedFile([alpha, beta, gamma], "gamma", 1)).toBe(alpha); - expect(findNextAnnotatedFile([alpha, beta, gamma], undefined, -1)).toBe(gamma); - expect(findNextAnnotatedFile([beta], "beta", 1)).toBeNull(); - }); + const visibleFor = (filterQuery: string) => + buildReviewStreamState({ files, liveCommentsByFileId: {}, filterQuery }).visibleFiles.map( + (file) => file.id, + ); - // Intent: comment navigation targets the next noted hunk and scrolls to the note. - test("resolveReviewNavigationTarget follows annotated comment navigation", () => { - const alpha = createAnnotatedFile("alpha", "alpha.ts"); - const gamma = createAnnotatedFile("gamma", "gamma.ts"); + expect(visibleFor("")).toEqual(["alpha", "beta", "gamma"]); + expect(visibleFor("ALPHA")).toEqual(["alpha"]); + expect(visibleFor("legacy-name")).toEqual(["beta"]); + // The agent's file summary is part of the haystack, not just the path. + expect(visibleFor("gamma.ts note")).toEqual(["gamma"]); + expect(visibleFor("nothing-matches")).toEqual([]); + }); - const target = resolveReviewNavigationTarget({ - allFiles: [alpha, gamma], - visibleFiles: [alpha, gamma], - currentFileId: "alpha", - currentHunkIndex: 0, - input: { commentDirection: "next" }, + // Intent: annotated navigation plans against a file-key index the terminal derives once. + test("buildReviewAnnotationIndex separates annotated files from annotated hunks", () => { + const annotated = createAnnotatedFile("alpha", "alpha.ts"); + const summaryOnly = createTestDiffFile({ + id: "beta", + path: "beta.ts", + agent: createTestAgentFileContext("beta.ts", { annotations: [] }), }); + const plain = createTestDiffFile({ id: "gamma", path: "gamma.ts", agent: null }); + const keyByFileId = new Map([ + ["alpha", "key:alpha"], + ["beta", "key:beta"], + ["gamma", "key:gamma"], + ]); + + const index = buildReviewAnnotationIndex([annotated, summaryOnly, plain], keyByFileId); - expect(target).toEqual({ file: gamma, hunkIndex: 0, scrollToNote: true }); + // A file carrying review context but no note inside a hunk is still an annotated file. + expect([...index.annotatedFileKeys]).toEqual(["key:alpha", "key:beta"]); + expect([...index.annotatedHunkIndicesByFileKey.keys()]).toEqual(["key:alpha"]); + expect([...(index.annotatedHunkIndicesByFileKey.get("key:alpha") ?? [])]).toEqual([0]); }); // Intent: absolute navigation supports both hunk index and side+line addressing. @@ -61,40 +81,23 @@ describe("review state helpers", () => { expect( resolveReviewNavigationTarget({ allFiles: [file], - visibleFiles: [file], - currentFileId: "alpha", - currentHunkIndex: 0, input: { filePath: "src/alpha.ts", hunkIndex: 0 }, }), - ).toEqual({ file, hunkIndex: 0, scrollToNote: false }); + ).toEqual({ file, hunkIndex: 0 }); expect( resolveReviewNavigationTarget({ allFiles: [file], - visibleFiles: [file], - currentFileId: "alpha", - currentHunkIndex: 0, input: { filePath: "src/alpha.ts", side: "new", line: 1 }, }), - ).toEqual({ file, hunkIndex: 0, scrollToNote: false }); + ).toEqual({ file, hunkIndex: 0 }); }); // Intent: invalid agent navigation requests fail before mutating review state. test("resolveReviewNavigationTarget rejects missing and invalid targets", () => { const file = createTestDiffFile({ id: "alpha", path: "src/alpha.ts" }); - const baseInput = { - allFiles: [file], - visibleFiles: [file], - currentFileId: "alpha", - currentHunkIndex: 0, - }; + const baseInput = { allFiles: [file] }; - expect(() => - resolveReviewNavigationTarget({ - ...baseInput, - input: { commentDirection: "next" }, - }), - ).toThrow("No annotated hunks"); expect(() => resolveReviewNavigationTarget({ ...baseInput, input: {} })).toThrow( "navigate requires --file", ); diff --git a/src/ui/lib/reviewState.ts b/src/ui/lib/reviewState.ts index d49621760..a286693ef 100644 --- a/src/ui/lib/reviewState.ts +++ b/src/ui/lib/reviewState.ts @@ -1,23 +1,24 @@ /** * Pure review-stream derivation helpers used by `useReviewController`. * - * This module turns raw diff files plus live comments into the current visible - * review state, sidebar entries, hunk cursors, and session-daemon navigation targets. It - * stays side-effect free so selection and navigation rules can be shared and - * tested without React state in the loop. + * This module turns raw diff files plus live comments into the current visible review + * state, the annotation index relative navigation plans against, and absolute + * session-daemon navigation targets. It stays side-effect free so selection and + * navigation rules can be tested without React state in the loop. + * + * Relative navigation itself lives in `core/review/navigation.ts`: what "next hunk" means + * is shared with every other review surface, and only the terminal-model facts it needs — + * which files and hunks carry notes — are derived here. */ import { findDiffFileByPath, findHunkIndexForLine } from "../../core/liveComments"; import { reviewHunkRanges } from "../../core/review/geometry"; +import type { ReviewAnnotationIndex } from "../../core/review/navigation"; +import { reviewFileMatchesFilter } from "../../core/review/selectors"; import { noDiffFileMatchesMessage } from "../../session/agent/errors"; import type { AgentAnnotation, DiffFile } from "../../core/types"; import type { NavigateToHunkToolInput, SelectedHunkSummary } from "../../session/types"; -import { filterReviewFiles, mergeFileAnnotationsByFileId } from "./files"; -import { - buildAnnotatedHunkCursors, - buildHunkCursors, - findNextHunkCursor, - type HunkCursor, -} from "./hunks"; +import { getAnnotatedHunkIndices } from "./agentAnnotations"; +import { mergeFileAnnotationsByFileId } from "./files"; export interface BuildReviewStreamStateOptions { files: DiffFile[]; @@ -28,14 +29,11 @@ export interface BuildReviewStreamStateOptions { export interface ReviewStreamState { allFiles: DiffFile[]; visibleFiles: DiffFile[]; - hunkCursors: HunkCursor[]; - annotatedHunkCursors: HunkCursor[]; } export interface ReviewNavigationTarget { file: DiffFile; hunkIndex: number; - scrollToNote: boolean; } /** Build selection-independent review stream state from files and filter text. */ @@ -45,27 +43,57 @@ export function buildReviewStreamState({ filterQuery, }: BuildReviewStreamStateOptions): ReviewStreamState { const allFiles = mergeFileAnnotationsByFileId(files, liveCommentsByFileId); - const visibleFiles = filterReviewFiles(allFiles, filterQuery); return { allFiles, - visibleFiles, - hunkCursors: buildHunkCursors(visibleFiles), - annotatedHunkCursors: buildAnnotatedHunkCursors(visibleFiles), + // The shared matcher, not a terminal copy of it: sidebar, review stream, and every + // other surface must agree on what one query matches. + visibleFiles: allFiles.filter((file) => + reviewFileMatchesFilter( + { + path: file.path, + ...(file.previousPath !== undefined ? { previousPath: file.previousPath } : {}), + ...(file.agent?.summary !== undefined ? { agentSummary: file.agent.summary } : {}), + }, + filterQuery, + ), + ), }; } -/** Resolve the selected file using the visible stream first, then the hidden current selection. */ -export function resolveSelectedFile( - allFiles: DiffFile[], - visibleFiles: DiffFile[], - selectedFileId: string, -) { - return ( - visibleFiles.find((file) => file.id === selectedFileId) ?? - allFiles.find((file) => file.id === selectedFileId) ?? - visibleFiles[0] - ); +/** + * Index which files and hunks currently carry notes, keyed by semantic file key. + * + * The terminal is where the review's note sources meet: a sidecar loaded with the + * changeset, live agent comments, and the reviewer's own notes are all merged onto the + * diff-file model before this runs. Annotated navigation plans against the result, so the + * set is derived once here and handed to the shared planner as a fact. + * + * File membership is deliberately broader than hunk membership: a file carrying review + * context but no note inside any hunk is still a stop on the annotated-file tour. + */ +export function buildReviewAnnotationIndex( + files: readonly DiffFile[], + keyByFileId: ReadonlyMap, +): ReviewAnnotationIndex { + const annotatedHunkIndicesByFileKey = new Map>(); + const annotatedFileKeys = new Set(); + + for (const file of files) { + const fileKey = keyByFileId.get(file.id); + if (!fileKey) { + continue; + } + if (file.agent) { + annotatedFileKeys.add(fileKey); + } + const annotatedHunks = getAnnotatedHunkIndices(file); + if (annotatedHunks.size > 0) { + annotatedHunkIndicesByFileKey.set(fileKey, annotatedHunks); + } + } + + return { annotatedHunkIndicesByFileKey, annotatedFileKeys }; } /** Format the currently selected hunk for daemon snapshots and session command replies. */ @@ -81,67 +109,20 @@ export function buildSelectedHunkSummary(file: DiffFile, hunkIndex: number): Sel }; } -/** Find the next or previous annotated file in the current visible review stream. */ -export function findNextAnnotatedFile( - visibleFiles: DiffFile[], - currentFileId: string | undefined, - delta: number, -) { - const annotatedFiles = visibleFiles.filter((file) => file.agent); - if (annotatedFiles.length === 0) { - return null; - } - - const currentIndex = annotatedFiles.findIndex((file) => file.id === currentFileId); - const normalizedIndex = currentIndex >= 0 ? currentIndex : 0; - const nextIndex = - (((normalizedIndex + delta) % annotatedFiles.length) + annotatedFiles.length) % - annotatedFiles.length; - return annotatedFiles[nextIndex] ?? null; -} - -/** Resolve one session-daemon navigation request against the review stream's current state. */ +/** + * Resolve one absolute session-daemon navigation request against the review stream. + * + * Absolute only — a path plus either a hunk index or a side and line. Relative requests + * (`--next-comment` / `--prev-comment`) are the same walk the keyboard performs and are + * planned through the shared `selection/move` intent instead of being resolved here. + */ export function resolveReviewNavigationTarget({ allFiles, - currentFileId, - currentHunkIndex, input, - visibleFiles, }: { allFiles: DiffFile[]; - visibleFiles: DiffFile[]; - currentFileId: string | undefined; - currentHunkIndex: number; input: NavigateToHunkToolInput; }): ReviewNavigationTarget { - if (input.commentDirection) { - const delta = input.commentDirection === "next" ? 1 : -1; - const hunkCursors = buildHunkCursors(visibleFiles); - const annotatedCursors = buildAnnotatedHunkCursors(visibleFiles); - const nextCursor = findNextHunkCursor( - annotatedCursors, - currentFileId, - currentHunkIndex, - delta, - hunkCursors, - ); - - if (!nextCursor) { - throw new Error("No annotated hunks found in the current review."); - } - - const targetFile = visibleFiles.find((file) => file.id === nextCursor.fileId); - if (!targetFile) { - throw new Error("Resolved annotated hunk references an unknown file."); - } - - return { - file: targetFile, - hunkIndex: nextCursor.hunkIndex, - scrollToNote: true, - }; - } - if (!input.filePath) { throw new Error("navigate requires --file when not using --next-comment or --prev-comment."); } @@ -164,9 +145,5 @@ export function resolveReviewNavigationTarget({ throw new Error(`No diff hunk in ${input.filePath} matches the requested target.`); } - return { - file, - hunkIndex, - scrollToNote: false, - }; + return { file, hunkIndex }; } From b9482302c35ba91694a522502d74a0904f45e275 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 20:13:00 +0000 Subject: [PATCH 2/3] refactor(ui): split command identity out of the terminal command table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The command table fused three separable things: identity (id, title, chords), binding (terminal KeyEvent matching), and effect (closures over live App state). A browser command palette or help screen would have had to restate the list, and the two clients would then drift on what a command is called, what it is bound to, and what it does (docs/browser-review-seam-audit.md, F1-F3). Move identity into src/core/commandCatalog.ts as data, with a declared resolution locus per command — semantic, client-local, or host-only — so a remote client can tell which commands it may invoke at all. Semantic commands declare their review effect as data too, and one lowering turns a command plus a repeat count into a ReviewIntent, which is what keeps the keyboard, an agent command, and a future palette meaning the same thing by "next hunk". The terminal keeps its matchers and handlers; its handler map is keyed by the catalog's id union, so a catalogued command with no handler fails to typecheck. Menus, help, and keybinding resolution are unchanged in behavior and now derive from the catalog, with a parity test asserting no surface names a command the catalog does not declare. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4 --- AGENTS.md | 1 + docs/browser-review-seam-audit.md | 21 + src/core/commandCatalog.test.ts | 102 ++++ src/core/commandCatalog.ts | 540 ++++++++++++++++++++++ src/ui/App.tsx | 5 +- src/ui/hooks/useReviewController.test.tsx | 20 +- src/ui/hooks/useReviewController.ts | 20 - src/ui/lib/appCommands.test.ts | 70 ++- src/ui/lib/appCommands.ts | 491 +++++--------------- src/ui/lib/appMenus.test.ts | 9 +- src/ui/lib/helpContent.ts | 10 + 11 files changed, 873 insertions(+), 416 deletions(-) create mode 100644 src/core/commandCatalog.test.ts create mode 100644 src/core/commandCatalog.ts diff --git a/AGENTS.md b/AGENTS.md index bac0ffdf4..a354347f2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,7 @@ CLI input - Keep split and stack views terminal-native and driven from the same normalized diff model. - Preserve mouse + keyboard parity for primary actions. - Keep the chrome restrained: top menu bar, minimal borders, no redundant metadata headers. +- Command identity is shared data: `src/core/commandCatalog.ts` owns every built-in command's id, title, category, default chords, and resolution locus (`semantic` / `client-local` / `host-only`). Key matching and handlers stay with each client, menus and help render from the catalog through the client's table, and semantic commands lower to `ReviewIntent`s. Add a command by adding a catalog entry, not by growing one client's table. - Shared review primitives are a hard seam: the semantic review model (`src/core/review/`) and its wire protocol (`src/session/reviewProtocol.ts`) are what every review consumer — terminal UI, session runtime, browser client — builds on. Both stay renderer-free and platform-neutral; `scripts/source-boundaries.test.ts` gates their imports, and its debt lists may only shrink. The staged plan for building on this seam is `docs/browser-review-rebuild.md`. ## component guidance diff --git a/docs/browser-review-seam-audit.md b/docs/browser-review-seam-audit.md index 21bd19289..3b6f8bfc0 100644 --- a/docs/browser-review-seam-audit.md +++ b/docs/browser-review-seam-audit.md @@ -315,6 +315,15 @@ here so the extraction happens before the duplication exists. Design detail in Fix: extract a renderer-neutral catalog (id, title, category, default chords, resolution locus — semantic / client-local / host-only); terminal keeps matchers and handlers, browser adds its own, both render menus/help/palette from the catalog. + _Repaid (Phase 1 PR 3)_: `src/core/commandCatalog.ts` carries id, title, category, default + chords, resolution locus, extension visibility, and menu-closing behavior for all 44 built-ins. + `ui/lib/appCommands.ts` builds its dispatch table from it — the handler map is keyed by + `AppCommandId`, so a catalogued command with no terminal handler fails to typecheck — and + menus and help keep reading identity through that table. Parity is asserted in + `appCommands.test.ts` ("command catalog parity"): the table is exactly the catalog in catalog + order, and no menu item or help row names a command the catalog does not declare. Placement + note: the catalog sits outside `core/review`, which stays review semantics only, so Phase 5 + must add it to the web boundary gate's allowed import targets. - **F2. Semantic command effects are closures instead of intent dispatches.** The ~15 review-semantic commands (hunk/file/annotated navigation, start note, toggle gap, toggle agent notes, filter) run as App closures; a browser implementation would re-derive each @@ -322,11 +331,23 @@ here so the extraction happens before the duplication exists. Design detail in `ReviewIntent`s (the Phase 1 store refactor is the same work); the browser fires them through the existing apply-action path, and the agent runtime's `hunk session` surface becomes a third consumer of the same lowering. + _Repaid (Phase 1 PR 3, core and terminal sites)_: semantic entries declare their effect as data + (`AppCommandReviewEffect`), and `lowerAppCommandToReviewIntent` is the one constructor turning + a command plus a repeat count into a `ReviewIntent`. The terminal's navigation handlers read + the scope and direction from that same declaration rather than restating them. Two semantic + commands have no intent to lower to yet — starting a note needs caller-owned draft identity, + and gap expansion is the Phase 2 `expansion/toggle` intent — and are listed by name in + `SEMANTIC_COMMANDS_WITHOUT_REVIEW_EFFECT`, so the gap is a decision rather than an oversight. - **F3. Keymap resolution is terminal-owned.** Chords are shared config strings (`keymap.ts`, `[keybindings]`), but resolution against defaults and conflict handling lives with the terminal table; a browser keymap would duplicate it and drift on user rebinds. Fix: resolve user keybindings against the catalog once; each client maps resolved chords to its own event type and masks platform-reserved chords (browser `Cmd+W` etc.). + _Repaid (Phase 1 PR 3, host site)_: `builtinCommandKeyDefaults` reads the catalog, so + `[keybindings]` resolution, conflict detection against extension commands, and key labels all + fold user config over catalogued defaults exactly once. Mapping resolved chords onto a client's + own event type stays per client, which is the part that cannot be shared; the browser's half + lands in Phase 5. - **F4. Host-only and extension commands — do not expose, by design.** Quit, source refresh, edit-in-`$EDITOR`, agent-skill helpers, and all extension commands execute host-side (with dialog access); browser invocation is remote code execution into the terminal session. diff --git a/src/core/commandCatalog.test.ts b/src/core/commandCatalog.test.ts new file mode 100644 index 000000000..d1557566f --- /dev/null +++ b/src/core/commandCatalog.test.ts @@ -0,0 +1,102 @@ +import { describe, expect, test } from "bun:test"; +import { + APP_COMMAND_CATALOG, + appCommandCatalogEntry, + lowerAppCommandToReviewIntent, + SEMANTIC_COMMANDS_WITHOUT_REVIEW_EFFECT, + type AppCommandCatalogEntry, +} from "./commandCatalog"; + +/** Look one entry up, failing loudly rather than silently skipping an assertion. */ +function entry(id: string): AppCommandCatalogEntry { + const found = appCommandCatalogEntry(id); + if (!found) { + throw new Error(`The catalog has no command ${id}.`); + } + return found; +} + +describe("app command catalog", () => { + test("gives every command one id under its own category", () => { + const ids = APP_COMMAND_CATALOG.map((command) => command.id); + + expect(new Set(ids).size).toBe(ids.length); + for (const command of APP_COMMAND_CATALOG) { + expect(command.id.startsWith(`hunk.${command.category}.`)).toBe(true); + expect(command.title.length).toBeGreaterThan(0); + } + }); + + // Intent: the resolution locus is what tells a remote client whether it may invoke a + // command at all, so only semantic commands may carry a review effect. + test("declares a review effect for semantic commands and nothing else", () => { + const missingEffect = APP_COMMAND_CATALOG.filter( + (command) => command.locus === "semantic" && command.review === undefined, + ).map((command) => command.id); + const strayEffect = APP_COMMAND_CATALOG.filter( + (command) => command.locus !== "semantic" && command.review !== undefined, + ).map((command) => command.id); + + expect(missingEffect).toEqual([...SEMANTIC_COMMANDS_WITHOUT_REVIEW_EFFECT]); + expect(strayEffect).toEqual([]); + }); + + test("keeps host-only commands to the ones that must run where the review is hosted", () => { + expect( + APP_COMMAND_CATALOG.filter((command) => command.locus === "host-only").map( + (command) => command.id, + ), + ).toEqual([ + "hunk.app.quit", + "hunk.app.openAgentSkill", + "hunk.app.refresh", + "hunk.review.editSelectedFile", + ]); + }); + + test("lowers navigation commands to the move their scope and direction declare", () => { + const state = { showAgentNotes: false }; + + expect( + lowerAppCommandToReviewIntent(entry("hunk.review.nextHunk"), { count: 1, state }), + ).toEqual({ type: "selection/move", scope: "hunk", delta: 1 }); + expect( + lowerAppCommandToReviewIntent(entry("hunk.review.previousHunk"), { count: 3, state }), + ).toEqual({ type: "selection/move", scope: "hunk", delta: -3 }); + expect( + lowerAppCommandToReviewIntent(entry("hunk.review.previousAnnotatedFile"), { + count: 2, + state, + }), + ).toEqual({ type: "selection/move", scope: "annotated-file", delta: -2 }); + }); + + test("lowers the note-layer toggle against current review state", () => { + expect( + lowerAppCommandToReviewIntent(entry("hunk.view.toggleAgentNotes"), { + count: 1, + state: { showAgentNotes: false }, + }), + ).toEqual({ type: "notes/set-visibility", visible: true }); + expect( + lowerAppCommandToReviewIntent(entry("hunk.view.toggleAgentNotes"), { + count: 1, + state: { showAgentNotes: true }, + }), + ).toEqual({ type: "notes/set-visibility", visible: false }); + }); + + test("lowers nothing for commands that resolve outside the review model", () => { + const state = { showAgentNotes: false }; + + expect( + lowerAppCommandToReviewIntent(entry("hunk.view.toggleSidebar"), { count: 1, state }), + ).toBeUndefined(); + expect( + lowerAppCommandToReviewIntent(entry("hunk.app.quit"), { count: 1, state }), + ).toBeUndefined(); + expect( + lowerAppCommandToReviewIntent(entry("hunk.review.toggleHunkGap"), { count: 1, state }), + ).toBeUndefined(); + }); +}); diff --git a/src/core/commandCatalog.ts b/src/core/commandCatalog.ts new file mode 100644 index 000000000..1d8303c83 --- /dev/null +++ b/src/core/commandCatalog.ts @@ -0,0 +1,540 @@ +/** + * Hunk's command vocabulary, as data every client can render. + * + * A command fuses three separable things: identity (id, title, chords), binding (matching + * one client's key events), and effect (doing the thing). Only identity is shared, so only + * identity lives here — a terminal `KeyEvent` matcher and a closure over live app state + * are not portable, and a browser palette that restated this list would immediately drift + * from the terminal's menus and help (`docs/browser-review-seam-audit.md`, F1–F3). + * + * Each entry also declares where it resolves: + * + * - `semantic` — it changes the review itself, so it lowers to a `ReviewIntent` and every + * attached surface sees the result. Its effect is declared as data rather than as a + * function, which is what lets the same declaration drive the terminal's handler, an + * agent command, and later a wire action. + * - `client-local` — deliberately per-client view state (scrolling, layout, theme, help). + * Each client implements its own handler; sharing identity is what keeps help screens + * and palettes agreeing about what the command is called and what it is bound to. + * - `host-only` — it runs where the review is hosted (quitting, reloading the source, + * opening `$EDITOR`). Not invocable from a remote client without an explicit allowlist, + * which is a scope boundary rather than a missing feature (audit F4). + * + * This module is deliberately renderer-neutral and dependency-light: no OpenTUI, no React, + * no Node builtins, chords as plain strings. It is not part of `src/core/review` because + * it describes UI vocabulary rather than review semantics, and that module stays purely + * about what a review *is*. + */ +import type { ReviewIntent } from "./review/intents"; +import type { ReviewSelectionScope } from "./review/navigation"; +import type { ReviewState } from "./review/state"; + +/** Where one command's effect resolves, and therefore who may invoke it. */ +export type AppCommandLocus = "semantic" | "client-local" | "host-only"; + +/** The id group a command lives under, and the menu-level grouping users read. */ +export type AppCommandCategory = "app" | "review" | "view"; + +/** + * What a semantic command does to the review, declared rather than closed over. + * + * Data, not a callback: the terminal reads the same declaration to wire its handler that + * the lowering below reads to build an intent, so "which way does `]` move" has one + * answer instead of one per surface. + */ +export type AppCommandReviewEffect = + | { kind: "selection/move"; scope: ReviewSelectionScope; direction: 1 | -1 } + | { kind: "notes/toggle-visibility" }; + +export interface AppCommandCatalogEntry { + /** Stable identifier, `hunk..` for every built-in. */ + id: string; + title: string; + category: AppCommandCategory; + /** Chords the command ships with, before the user's `[keybindings]` are folded in. */ + defaultKeys: readonly string[]; + locus: AppCommandLocus; + /** The review effect a semantic command lowers to, where one is already modelled. */ + review?: AppCommandReviewEffect; + /** True when extension command controls may invoke this command by id. */ + publicToExtensions: boolean; + /** Close an open dropdown menu after running. */ + closesMenu?: boolean; +} + +/** + * Every built-in command. + * + * Order is the tiebreaker when several commands could match one key, so entries keep the + * relative order the terminal's original key cascade had (uppercase forms before + * lowercase ones). A few ship with no chords at all: declaring them keeps them bindable + * from `[keybindings]` and dispatchable by id whether or not a menu presents them. + */ +const BUILTIN_COMMANDS = [ + { + id: "hunk.review.jumpToBottom", + title: "Jump to end", + category: "review", + defaultKeys: ["G", "end"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.jumpToTop", + title: "Jump to start", + category: "review", + defaultKeys: ["g", "home"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.app.quit", + title: "Quit", + category: "app", + defaultKeys: ["q"], + locus: "host-only", + publicToExtensions: true, + }, + { + id: "hunk.app.toggleHelp", + title: "Toggle help", + category: "app", + defaultKeys: ["?"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.app.openAgentSkill", + title: "Show agent skill", + category: "app", + defaultKeys: [], + locus: "host-only", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.app.toggleFocusArea", + title: "Switch focus between files and filter", + category: "app", + defaultKeys: ["tab"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.focusFilter", + title: "Focus the file filter", + category: "review", + defaultKeys: ["/"], + // Moving keyboard focus is this client's business; the filter value it edits is + // shared review state, changed through `filter/set` rather than by this command. + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.startNote", + title: "Add a review note", + category: "review", + defaultKeys: ["c"], + locus: "semantic", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.pageDown", + title: "Scroll down one page", + category: "review", + defaultKeys: ["pagedown", "space", "f"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.pageUp", + title: "Scroll up one page", + category: "review", + defaultKeys: ["pageup", "b", "shift+space"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.halfPageDown", + title: "Scroll down half a page", + category: "review", + defaultKeys: ["d"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.halfPageUp", + title: "Scroll up half a page", + category: "review", + defaultKeys: ["u"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.stepDown", + title: "Scroll down one row", + category: "review", + defaultKeys: ["down", "j"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.stepUp", + title: "Scroll up one row", + category: "review", + defaultKeys: ["up", "k"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.scrollCodeLeft", + title: "Scroll code left", + category: "review", + // Both chords run the same command; the shifted one scrolls further, so the handler + // reads the event rather than splitting this into two commands. + defaultKeys: ["left", "shift+left"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.scrollCodeRight", + title: "Scroll code right", + category: "review", + defaultKeys: ["right", "shift+right"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.alignCurrentLineTop", + title: "Align current line to top", + category: "review", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.alignCurrentLineCenter", + title: "Align current line to center", + category: "review", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.alignCurrentLineBottom", + title: "Align current line to bottom", + category: "review", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.view.cursorLineRow", + title: "Highlight the current row", + category: "view", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.cursorLineNumber", + title: "Mark the current line number", + category: "view", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.cursorLineOff", + title: "Hide the current-line marker", + category: "view", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.layoutSplit", + title: "Split layout", + category: "view", + defaultKeys: ["1"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.layoutStack", + title: "Stack layout", + category: "view", + defaultKeys: ["2"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.layoutAuto", + title: "Auto layout", + category: "view", + defaultKeys: ["0"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.applyFilePresentationToAllMatching", + title: "Apply the current file presentation to all matching files", + category: "view", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.toggleSidebar", + title: "Toggle sidebar", + category: "view", + defaultKeys: ["s"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.app.refresh", + title: "Refresh the review", + category: "app", + defaultKeys: ["r"], + locus: "host-only", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.openThemeSelector", + title: "Choose theme", + category: "view", + defaultKeys: ["t"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.toggleAgentNotes", + title: "Toggle agent notes", + category: "view", + defaultKeys: ["a"], + locus: "semantic", + review: { kind: "notes/toggle-visibility" }, + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.toggleLineNumbers", + title: "Toggle line numbers", + category: "view", + defaultKeys: ["l"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.toggleLineWrap", + title: "Toggle line wrapping", + category: "view", + defaultKeys: ["w"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.toggleMenuBar", + title: "Toggle menu bar", + category: "view", + defaultKeys: ["M"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.toggleHunkHeaders", + title: "Toggle hunk headers", + category: "view", + defaultKeys: ["m"], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.view.toggleCopyDecorations", + title: "Toggle copy decorations", + category: "view", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.toggleHunkGap", + title: "Expand or collapse context for the selected hunk", + category: "review", + defaultKeys: ["z"], + locus: "semantic", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.editSelectedFile", + title: "Open the selected file in your editor", + category: "review", + defaultKeys: ["e"], + locus: "host-only", + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.previousHunk", + title: "Previous hunk", + category: "review", + defaultKeys: ["["], + locus: "semantic", + review: { kind: "selection/move", scope: "hunk", direction: -1 }, + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.nextHunk", + title: "Next hunk", + category: "review", + defaultKeys: ["]"], + locus: "semantic", + review: { kind: "selection/move", scope: "hunk", direction: 1 }, + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.previousFile", + title: "Previous file", + category: "review", + defaultKeys: [","], + locus: "semantic", + review: { kind: "selection/move", scope: "file", direction: -1 }, + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.nextFile", + title: "Next file", + category: "review", + defaultKeys: ["."], + locus: "semantic", + review: { kind: "selection/move", scope: "file", direction: 1 }, + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.previousAnnotatedHunk", + title: "Previous annotated hunk", + category: "review", + defaultKeys: ["{"], + locus: "semantic", + review: { kind: "selection/move", scope: "annotated-hunk", direction: -1 }, + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.nextAnnotatedHunk", + title: "Next annotated hunk", + category: "review", + defaultKeys: ["}"], + locus: "semantic", + review: { kind: "selection/move", scope: "annotated-hunk", direction: 1 }, + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.previousAnnotatedFile", + title: "Previous annotated file", + category: "review", + defaultKeys: [], + locus: "semantic", + review: { kind: "selection/move", scope: "annotated-file", direction: -1 }, + publicToExtensions: true, + closesMenu: true, + }, + { + id: "hunk.review.nextAnnotatedFile", + title: "Next annotated file", + category: "review", + defaultKeys: [], + locus: "semantic", + review: { kind: "selection/move", scope: "annotated-file", direction: 1 }, + publicToExtensions: true, + closesMenu: true, + }, +] as const satisfies readonly AppCommandCatalogEntry[]; + +/** + * Every built-in command id, as a type — a client cannot name one that does not exist. + * + * Derived from the literal declaration above, which is why that one stays `as const` while + * the exported catalog is widened to the entry shape consumers read. + */ +export type AppCommandId = (typeof BUILTIN_COMMANDS)[number]["id"]; + +export const APP_COMMAND_CATALOG: readonly AppCommandCatalogEntry[] = BUILTIN_COMMANDS; + +/** + * Semantic commands whose review effect is not modelled as an intent yet. + * + * Named rather than implied: both change the review for every attached surface and belong + * at the producer, but neither has an intent to lower to today — starting a note needs a + * caller-owned draft identity and a resolved line target, and gap expansion is the + * `expansion/toggle` intent staged for the producer-runtime phase. Until those land, the + * terminal keeps resolving them locally, and this list is what keeps that a decision + * instead of an oversight. + */ +export const SEMANTIC_COMMANDS_WITHOUT_REVIEW_EFFECT: readonly AppCommandId[] = [ + "hunk.review.startNote", + "hunk.review.toggleHunkGap", +]; + +/** Look one command up by id. */ +export function appCommandCatalogEntry(id: string): AppCommandCatalogEntry | undefined { + return APP_COMMAND_CATALOG.find((entry) => entry.id === id); +} + +/** The facts a command needs to become one concrete review intent. */ +export interface AppCommandLoweringContext { + /** Host-normalized positive repeat count. */ + count: number; + /** Current review state, for commands whose effect depends on what it already says. */ + state: Pick; +} + +/** + * Lower one command into the review intent it asks for. + * + * The single constructor every surface goes through: a keyboard chord, a browser palette + * entry, and an agent command that name the same id produce the same intent. Undefined + * means the command has no declared review effect — it is client-local, host-only, or one + * of the semantic commands still listed above. + */ +export function lowerAppCommandToReviewIntent( + entry: AppCommandCatalogEntry, + { count, state }: AppCommandLoweringContext, +): ReviewIntent | undefined { + switch (entry.review?.kind) { + case "selection/move": + return { + type: "selection/move", + scope: entry.review.scope, + delta: entry.review.direction * count, + }; + case "notes/toggle-visibility": + return { type: "notes/set-visibility", visible: !state.showAgentNotes }; + case undefined: + return undefined; + } +} diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 4c6b25ed7..ddb98fa90 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -1822,10 +1822,7 @@ export function App({ alignCurrentLine, applyFilePresentationToAllMatching, focusFilter, - moveToAnnotatedFile: review.moveToAnnotatedFile, - moveToAnnotatedHunk: review.moveToAnnotatedHunk, - moveToFile: review.moveToFile, - moveToHunk: review.moveToHunk, + moveSelection: review.moveSelection, openAgentSkill, openThemeSelector, requestQuit, diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 9a48d5312..63e8d0491 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -338,7 +338,7 @@ describe("useReviewController", () => { expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(1); await act(async () => { - expectValue(controllerRef.current).moveToFile(1); + expectValue(controllerRef.current).moveSelection("file", 1); }); await flush(setup); @@ -348,7 +348,7 @@ describe("useReviewController", () => { expect(controller.selectedFileTopAlignRequestId).toBe(1); await act(async () => { - expectValue(controllerRef.current).moveToFile(1); + expectValue(controllerRef.current).moveSelection("file", 1); }); await flush(setup); @@ -357,7 +357,7 @@ describe("useReviewController", () => { expect(controller.selectedFileTopAlignRequestId).toBe(2); await act(async () => { - expectValue(controllerRef.current).moveToFile(1); + expectValue(controllerRef.current).moveSelection("file", 1); }); await flush(setup); @@ -366,7 +366,7 @@ describe("useReviewController", () => { expect(controller.selectedFileTopAlignRequestId).toBe(2); await act(async () => { - expectValue(controllerRef.current).moveToFile(-1); + expectValue(controllerRef.current).moveSelection("file", -1); }); await flush(setup); @@ -375,7 +375,7 @@ describe("useReviewController", () => { expect(controller.selectedFileTopAlignRequestId).toBe(3); await act(async () => { - expectValue(controllerRef.current).moveToFile(-1); + expectValue(controllerRef.current).moveSelection("file", -1); }); await flush(setup); @@ -384,7 +384,7 @@ describe("useReviewController", () => { expect(controller.selectedFileTopAlignRequestId).toBe(4); await act(async () => { - expectValue(controllerRef.current).moveToFile(-1); + expectValue(controllerRef.current).moveSelection("file", -1); }); await flush(setup); @@ -411,7 +411,7 @@ describe("useReviewController", () => { const initialAlignRequest = expectValue(controllerRef.current).selectedFileTopAlignRequestId; await act(async () => { - expectValue(controllerRef.current).moveToFile(3); + expectValue(controllerRef.current).moveSelection("file", 3); }); await flush(setup); @@ -459,7 +459,7 @@ describe("useReviewController", () => { ).toEqual(["Check beta rename"]); await act(async () => { - expectValue(controllerRef.current).moveToAnnotatedHunk(1); + expectValue(controllerRef.current).moveSelection("annotated-hunk", 1); }); await flush(setup); @@ -1666,7 +1666,7 @@ describe("useReviewController", () => { const initialRequestId = expectValue(controllerRef.current).selectedHunkRevealRequestId; await act(async () => { - expectValue(controllerRef.current).moveToHunk(2); + expectValue(controllerRef.current).moveSelection("hunk", 2); }); await flush(setup); @@ -1690,7 +1690,7 @@ describe("useReviewController", () => { expect(expectValue(expectValue(controllerRef.current).lineCursor).hunkIndex).toBe(0); await act(async () => { - expectValue(controllerRef.current).moveToHunk(1); + expectValue(controllerRef.current).moveSelection("hunk", 1); }); await flush(setup); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 9ba7a8b54..9a224a7ad 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -178,10 +178,6 @@ export interface ReviewController { moveLineCursor: (delta: number) => void; /** Step the selection through one navigable scope; the scope owns wrap and reveal. */ moveSelection: (scope: ReviewSelectionScope, delta: number) => void; - moveToAnnotatedFile: (delta: number) => void; - moveToAnnotatedHunk: (delta: number) => void; - moveToFile: (delta: number) => void; - moveToHunk: (delta: number) => void; scrollToNote: boolean; selectedFile: DiffFile | undefined; selectedFileId: string; @@ -718,18 +714,6 @@ export function useReviewController({ } }, [selectedFile, selectedHunkIndex, toggleGap]); - /** Named scopes the keyboard binds today, each one step of the shared walk. */ - const moveToHunk = useCallback((delta: number) => moveSelection("hunk", delta), [moveSelection]); - const moveToFile = useCallback((delta: number) => moveSelection("file", delta), [moveSelection]); - const moveToAnnotatedHunk = useCallback( - (delta: number) => moveSelection("annotated-hunk", delta), - [moveSelection], - ); - const moveToAnnotatedFile = useCallback( - (delta: number) => moveSelection("annotated-file", delta), - [moveSelection], - ); - /** * Resolve one session-daemon navigation request against the current review and select it. * @@ -1163,10 +1147,6 @@ export function useReviewController({ clearLiveComments, moveLineCursor, moveSelection, - moveToAnnotatedFile, - moveToAnnotatedHunk, - moveToFile, - moveToHunk, navigateToLocation, removeLiveComment, removeUserNote, diff --git a/src/ui/lib/appCommands.test.ts b/src/ui/lib/appCommands.test.ts index 430450ae2..219ca162b 100644 --- a/src/ui/lib/appCommands.test.ts +++ b/src/ui/lib/appCommands.test.ts @@ -10,6 +10,9 @@ import { type BuildAppCommandsOptions, type ResolvedCommandKeys, } from "./appCommands"; +import { APP_COMMAND_CATALOG } from "../../core/commandCatalog"; +import { buildAppMenus } from "./appMenus"; +import { buildHelpSections, HELP_COMMAND_IDS } from "./helpContent"; import { resolveCommandKeys } from "./keymap"; /** Build a key event with the fields command matching reads. */ @@ -44,10 +47,7 @@ function createTestCommands(resolvedKeys?: ResolvedCommandKeys) { alignCurrentLine: record("alignCurrentLine"), applyFilePresentationToAllMatching: record("applyFilePresentationToAllMatching"), focusFilter: record("focusFilter"), - moveToAnnotatedFile: record("moveToAnnotatedFile"), - moveToAnnotatedHunk: record("moveToAnnotatedHunk"), - moveToFile: record("moveToFile"), - moveToHunk: record("moveToHunk"), + moveSelection: record("moveSelection"), openAgentSkill: record("openAgentSkill"), openThemeSelector: record("openThemeSelector"), requestQuit: record("requestQuit"), @@ -270,7 +270,7 @@ describe("commands that ship unbound", () => { expect(dispatchAppCommand(commands, keyEvent({ name: "n", ctrl: true }))?.id).toBe( "hunk.review.nextAnnotatedFile", ); - expect(ran).toEqual(["moveToAnnotatedFile:1"]); + expect(ran).toEqual(["moveSelection:annotated-file,1"]); expect( commands.find((command) => command.id === "hunk.review.nextAnnotatedFile")?.keyLabels, ).toEqual(["Ctrl+N"]); @@ -309,7 +309,7 @@ describe("executeAppCommand", () => { expect(executeAppCommand(commands, "hunk.review.nextHunk", { count: 3 })).toBe(true); expect(executeAppCommand(commands, "hunk.review.stepUp", { count: 4 })).toBe(true); expect(executeAppCommand(commands, "hunk.review.pageDown", { count: 2 })).toBe(true); - expect(ran).toEqual(["moveToHunk:3", "stepDiffLine:-4", "scrollDiff:2,viewport"]); + expect(ran).toEqual(["moveSelection:hunk,3", "stepDiffLine:-4", "scrollDiff:2,viewport"]); }); test("runs one-shot commands once regardless of count", () => { @@ -330,3 +330,61 @@ describe("executeAppCommand", () => { expect(ran).toEqual([]); }); }); + +// The command-parity hook (audit F1–F3): every surface that presents a command — the +// terminal's dispatch table, its dropdown menus, its help dialog — must name one the +// shared catalog declares, and the table must present every catalogued command. A command +// added to one client without a catalog entry fails here instead of forking the vocabulary +// between the terminal and the browser palette that renders from the same data. +describe("command catalog parity", () => { + test("the built-in table is exactly the catalog, in catalog order", () => { + const { commands } = createTestCommands(); + + expect(commands.map((command) => command.id)).toEqual( + APP_COMMAND_CATALOG.map((entry) => entry.id), + ); + }); + + test("each built-in command carries the catalog's identity", () => { + const { commands } = createTestCommands(); + + for (const entry of APP_COMMAND_CATALOG) { + const command = commands.find((candidate) => candidate.id === entry.id); + expect(command?.title).toBe(entry.title); + expect(command?.defaultKeys).toEqual(entry.defaultKeys); + expect(command?.keys).toEqual(entry.defaultKeys); + expect(command?.publicToExtensions).toBe(entry.publicToExtensions); + expect(Boolean(command?.closesMenu)).toBe(Boolean(entry.closesMenu)); + } + }); + + test("menus and help only name catalogued commands", () => { + const { commands } = createTestCommands(); + const catalogued = new Set(APP_COMMAND_CATALOG.map((entry) => entry.id)); + const menus = buildAppMenus({ + commands, + copyDecorations: false, + cursorLine: "row", + layoutMode: "auto", + renderSidebar: true, + showAgentNotes: false, + showHelp: false, + showHunkHeaders: true, + showLineNumbers: true, + showMenuBar: true, + wrapLines: false, + }); + const menuCommandIds = Object.values(menus) + .flat() + .flatMap((entry) => (entry.kind === "item" && entry.commandId ? [entry.commandId] : [])); + + expect(menuCommandIds.length).toBeGreaterThan(0); + expect(menuCommandIds.filter((id) => !catalogued.has(id))).toEqual([]); + expect( + buildHelpSections(commands) + .flatMap((section) => section.rows) + .filter((row) => row.keys.length === 0), + ).toEqual([]); + expect(HELP_COMMAND_IDS.filter((id) => !catalogued.has(id))).toEqual([]); + }); +}); diff --git a/src/ui/lib/appCommands.ts b/src/ui/lib/appCommands.ts index 6db4b513a..d0441c48f 100644 --- a/src/ui/lib/appCommands.ts +++ b/src/ui/lib/appCommands.ts @@ -1,4 +1,10 @@ import type { KeyEvent } from "@opentui/core"; +import { + APP_COMMAND_CATALOG, + type AppCommandCatalogEntry, + type AppCommandId, +} from "../../core/commandCatalog"; +import type { ReviewSelectionScope } from "../../core/review/navigation"; import type { CursorLine, LayoutMode } from "../../core/types"; import type { ExtensionCommandExecutionOptions } from "../../extension-api/types"; import { @@ -69,15 +75,12 @@ export interface AppCommand { closesMenu?: boolean; } -/** One built-in command as declared: chords in, matcher and labels derived. */ -interface BuiltinCommandSpec { - id: string; - title: string; - /** Chords the command ships with; the user's config may replace them. */ - defaultKeys: readonly string[]; +/** What the terminal does for one catalogued command. */ +interface BuiltinCommandHandler { + /** Report whether the command may run right now; skipped when false. */ isEnabled?: () => boolean; - run: (key: KeyEvent, count: number) => void; - closesMenu?: boolean; + /** Run once with a host-normalized positive movement count and its catalog entry. */ + run: (key: KeyEvent, count: number, entry: AppCommandCatalogEntry) => void; } /** The callbacks the built-in command set drives; App supplies its own handlers. */ @@ -88,10 +91,8 @@ export interface BuildAppCommandsOptions { alignCurrentLine: (alignment: "top" | "center" | "bottom") => void; applyFilePresentationToAllMatching: () => void; focusFilter: () => void; - moveToAnnotatedFile: (delta: number) => void; - moveToAnnotatedHunk: (delta: number) => void; - moveToFile: (delta: number) => void; - moveToHunk: (delta: number) => void; + /** Step the review selection through one scope, as the catalog entry declares it. */ + moveSelection: (scope: ReviewSelectionScope, delta: number) => void; openAgentSkill: () => void; openThemeSelector: () => void; requestQuit: () => void; @@ -118,383 +119,122 @@ export interface BuildAppCommandsOptions { } /** - * Declare Hunk's built-in commands as ids, titles, and default chords. + * Bind Hunk's built-in commands to the terminal's effects. * - * Every id is `hunk..`: `hunk.` is the reserved vendor namespace - * no extension id may take, and the group below it is the menu-level grouping - * users read in `[keybindings]`. + * Identity — id, title, chords, category, resolution locus — lives in the shared command + * catalog, so this table says only what each command *does here*. The map is keyed by + * `AppCommandId`, which makes the compiler the parity check: a command added to the + * catalog without a terminal handler, or a handler for a command nobody declared, fails + * to typecheck rather than silently going missing from menus and help. * - * Order is the tiebreaker when several commands could match one key, exactly - * as the old cascade of if-statements was, so entries keep the old cascade's - * relative order where it mattered (uppercase before lowercase forms). - * - * A few commands ship with no chords at all. Declaring them here keeps semantic - * actions bindable from `[keybindings]` and dispatchable by id whether or not a - * menu currently presents them. + * Semantic navigation commands do not restate their scope or direction: they read the + * effect their catalog entry declares, which is the same declaration a browser palette + * and the agent surface lower through. */ -const PUBLIC_EXTENSION_COMMAND_IDS = new Set([ - "hunk.review.jumpToBottom", - "hunk.review.jumpToTop", - "hunk.app.quit", - "hunk.app.toggleHelp", - "hunk.app.openAgentSkill", - "hunk.app.toggleFocusArea", - "hunk.review.focusFilter", - "hunk.review.startNote", - "hunk.review.pageDown", - "hunk.review.pageUp", - "hunk.review.halfPageDown", - "hunk.review.halfPageUp", - "hunk.review.stepDown", - "hunk.review.stepUp", - "hunk.review.scrollCodeLeft", - "hunk.review.scrollCodeRight", - "hunk.review.alignCurrentLineTop", - "hunk.review.alignCurrentLineCenter", - "hunk.review.alignCurrentLineBottom", - "hunk.view.cursorLineRow", - "hunk.view.cursorLineNumber", - "hunk.view.cursorLineOff", - "hunk.view.layoutSplit", - "hunk.view.layoutStack", - "hunk.view.layoutAuto", - "hunk.view.applyFilePresentationToAllMatching", - "hunk.view.toggleSidebar", - "hunk.app.refresh", - "hunk.view.openThemeSelector", - "hunk.view.toggleAgentNotes", - "hunk.view.toggleLineNumbers", - "hunk.view.toggleLineWrap", - "hunk.view.toggleMenuBar", - "hunk.view.toggleHunkHeaders", - "hunk.view.toggleCopyDecorations", - "hunk.review.toggleHunkGap", - "hunk.review.editSelectedFile", - "hunk.review.previousHunk", - "hunk.review.nextHunk", - "hunk.review.previousFile", - "hunk.review.nextFile", - "hunk.review.previousAnnotatedHunk", - "hunk.review.nextAnnotatedHunk", - "hunk.review.previousAnnotatedFile", - "hunk.review.nextAnnotatedFile", -]); +function runSelectionMove( + options: BuildAppCommandsOptions, + entry: AppCommandCatalogEntry, + count: number, +) { + const effect = entry.review; + if (effect?.kind !== "selection/move") { + return; + } -function builtinCommandSpecs(options: BuildAppCommandsOptions): BuiltinCommandSpec[] { - return [ - { - id: "hunk.review.jumpToBottom", - title: "Jump to end", - defaultKeys: ["G", "end"], - run: () => options.scrollDiff(1, "content"), - }, - { - id: "hunk.review.jumpToTop", - title: "Jump to start", - defaultKeys: ["g", "home"], - run: () => options.scrollDiff(-1, "content"), - }, - { - id: "hunk.app.quit", - title: "Quit", - defaultKeys: ["q"], - run: () => options.requestQuit(), - }, - { - id: "hunk.app.toggleHelp", - title: "Toggle help", - defaultKeys: ["?"], - run: () => options.toggleHelp(), - closesMenu: true, - }, - { - id: "hunk.app.openAgentSkill", - title: "Show agent skill", - defaultKeys: [], - run: () => options.openAgentSkill(), - closesMenu: true, - }, - { - id: "hunk.app.toggleFocusArea", - title: "Switch focus between files and filter", - defaultKeys: ["tab"], - run: () => options.toggleFocusArea(), - }, - { - id: "hunk.review.focusFilter", - title: "Focus the file filter", - defaultKeys: ["/"], - run: () => options.focusFilter(), - }, - { - id: "hunk.review.startNote", - title: "Add a review note", - defaultKeys: ["c"], - run: () => options.startUserNote(), - closesMenu: true, - }, - { - id: "hunk.review.pageDown", - title: "Scroll down one page", - defaultKeys: ["pagedown", "space", "f"], - run: (_key, count) => options.scrollDiff(count, "viewport"), - }, - { - id: "hunk.review.pageUp", - title: "Scroll up one page", - defaultKeys: ["pageup", "b", "shift+space"], - run: (_key, count) => options.scrollDiff(-count, "viewport"), - }, - { - id: "hunk.review.halfPageDown", - title: "Scroll down half a page", - defaultKeys: ["d"], - run: (_key, count) => options.scrollDiff(count, "half"), - }, - { - id: "hunk.review.halfPageUp", - title: "Scroll up half a page", - defaultKeys: ["u"], - run: (_key, count) => options.scrollDiff(-count, "half"), - }, - { - id: "hunk.review.stepDown", - title: "Scroll down one row", - defaultKeys: ["down", "j"], - run: (_key, count) => options.stepDiffLine(count), - }, - { - id: "hunk.review.stepUp", - title: "Scroll up one row", - defaultKeys: ["up", "k"], - run: (_key, count) => options.stepDiffLine(-count), - }, - { - id: "hunk.review.scrollCodeLeft", - title: "Scroll code left", - // Both chords run the same command; the shifted one scrolls further, so - // the handler reads the event rather than splitting into two commands. - defaultKeys: ["left", "shift+left"], + options.moveSelection(effect.scope, effect.direction * count); +} + +function builtinCommandHandlers( + options: BuildAppCommandsOptions, +): Record { + return { + "hunk.review.jumpToBottom": { run: () => options.scrollDiff(1, "content") }, + "hunk.review.jumpToTop": { run: () => options.scrollDiff(-1, "content") }, + "hunk.app.quit": { run: () => options.requestQuit() }, + "hunk.app.toggleHelp": { run: () => options.toggleHelp() }, + "hunk.app.openAgentSkill": { run: () => options.openAgentSkill() }, + "hunk.app.toggleFocusArea": { run: () => options.toggleFocusArea() }, + "hunk.review.focusFilter": { run: () => options.focusFilter() }, + "hunk.review.startNote": { run: () => options.startUserNote() }, + "hunk.review.pageDown": { run: (_key, count) => options.scrollDiff(count, "viewport") }, + "hunk.review.pageUp": { run: (_key, count) => options.scrollDiff(-count, "viewport") }, + "hunk.review.halfPageDown": { run: (_key, count) => options.scrollDiff(count, "half") }, + "hunk.review.halfPageUp": { run: (_key, count) => options.scrollDiff(-count, "half") }, + "hunk.review.stepDown": { run: (_key, count) => options.stepDiffLine(count) }, + "hunk.review.stepUp": { run: (_key, count) => options.stepDiffLine(-count) }, + "hunk.review.scrollCodeLeft": { run: (key, count) => options.scrollCodeHorizontally( (key.shift ? -FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : -1) * count, ), }, - { - id: "hunk.review.scrollCodeRight", - title: "Scroll code right", - defaultKeys: ["right", "shift+right"], + "hunk.review.scrollCodeRight": { run: (key, count) => options.scrollCodeHorizontally( (key.shift ? FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : 1) * count, ), }, - { - id: "hunk.review.alignCurrentLineTop", - title: "Align current line to top", - defaultKeys: [], + "hunk.review.alignCurrentLineTop": { isEnabled: () => options.canAlignCurrentLine, run: () => options.alignCurrentLine("top"), }, - { - id: "hunk.review.alignCurrentLineCenter", - title: "Align current line to center", - defaultKeys: [], + "hunk.review.alignCurrentLineCenter": { isEnabled: () => options.canAlignCurrentLine, run: () => options.alignCurrentLine("center"), }, - { - id: "hunk.review.alignCurrentLineBottom", - title: "Align current line to bottom", - defaultKeys: [], + "hunk.review.alignCurrentLineBottom": { isEnabled: () => options.canAlignCurrentLine, run: () => options.alignCurrentLine("bottom"), }, - { - id: "hunk.view.cursorLineRow", - title: "Highlight the current row", - defaultKeys: [], - run: () => options.selectCursorLine("row"), - closesMenu: true, - }, - { - id: "hunk.view.cursorLineNumber", - title: "Mark the current line number", - defaultKeys: [], - run: () => options.selectCursorLine("number"), - closesMenu: true, - }, - { - id: "hunk.view.cursorLineOff", - title: "Hide the current-line marker", - defaultKeys: [], - run: () => options.selectCursorLine("off"), - closesMenu: true, - }, - { - id: "hunk.view.layoutSplit", - title: "Split layout", - defaultKeys: ["1"], - run: () => options.selectLayoutMode("split"), - closesMenu: true, - }, - { - id: "hunk.view.layoutStack", - title: "Stack layout", - defaultKeys: ["2"], - run: () => options.selectLayoutMode("stack"), - closesMenu: true, - }, - { - id: "hunk.view.layoutAuto", - title: "Auto layout", - defaultKeys: ["0"], - run: () => options.selectLayoutMode("auto"), - closesMenu: true, - }, - { - id: "hunk.view.applyFilePresentationToAllMatching", - title: "Apply the current file presentation to all matching files", - defaultKeys: [], + "hunk.view.cursorLineRow": { run: () => options.selectCursorLine("row") }, + "hunk.view.cursorLineNumber": { run: () => options.selectCursorLine("number") }, + "hunk.view.cursorLineOff": { run: () => options.selectCursorLine("off") }, + "hunk.view.layoutSplit": { run: () => options.selectLayoutMode("split") }, + "hunk.view.layoutStack": { run: () => options.selectLayoutMode("stack") }, + "hunk.view.layoutAuto": { run: () => options.selectLayoutMode("auto") }, + "hunk.view.applyFilePresentationToAllMatching": { isEnabled: () => options.canApplyFilePresentationToAllMatching, run: () => options.applyFilePresentationToAllMatching(), - closesMenu: true, - }, - { - id: "hunk.view.toggleSidebar", - title: "Toggle sidebar", - defaultKeys: ["s"], - run: () => options.toggleSidebar(), - closesMenu: true, }, - { - id: "hunk.app.refresh", - title: "Refresh the review", - defaultKeys: ["r"], + "hunk.view.toggleSidebar": { run: () => options.toggleSidebar() }, + "hunk.app.refresh": { isEnabled: () => options.canRefreshCurrentInput, run: () => options.triggerRefreshCurrentInput(), - closesMenu: true, - }, - { - id: "hunk.view.openThemeSelector", - title: "Choose theme", - defaultKeys: ["t"], - run: () => options.openThemeSelector(), - closesMenu: true, - }, - { - id: "hunk.view.toggleAgentNotes", - title: "Toggle agent notes", - defaultKeys: ["a"], - run: () => options.toggleAgentNotes(), - closesMenu: true, - }, - { - id: "hunk.view.toggleLineNumbers", - title: "Toggle line numbers", - defaultKeys: ["l"], - run: () => options.toggleLineNumbers(), - closesMenu: true, }, - { - id: "hunk.view.toggleLineWrap", - title: "Toggle line wrapping", - defaultKeys: ["w"], - run: () => options.toggleLineWrap(), - closesMenu: true, + "hunk.view.openThemeSelector": { run: () => options.openThemeSelector() }, + "hunk.view.toggleAgentNotes": { run: () => options.toggleAgentNotes() }, + "hunk.view.toggleLineNumbers": { run: () => options.toggleLineNumbers() }, + "hunk.view.toggleLineWrap": { run: () => options.toggleLineWrap() }, + "hunk.view.toggleMenuBar": { run: () => options.toggleMenuBar() }, + "hunk.view.toggleHunkHeaders": { run: () => options.toggleHunkHeaders() }, + "hunk.view.toggleCopyDecorations": { run: () => options.toggleCopyDecorations() }, + "hunk.review.toggleHunkGap": { run: () => options.toggleGapForSelectedHunk() }, + "hunk.review.editSelectedFile": { run: () => options.triggerEditSelectedFile() }, + "hunk.review.previousHunk": { + run: (_key, count, entry) => runSelectionMove(options, entry, count), }, - { - id: "hunk.view.toggleMenuBar", - title: "Toggle menu bar", - defaultKeys: ["M"], - run: () => options.toggleMenuBar(), - closesMenu: true, + "hunk.review.nextHunk": { + run: (_key, count, entry) => runSelectionMove(options, entry, count), }, - { - id: "hunk.view.toggleHunkHeaders", - title: "Toggle hunk headers", - defaultKeys: ["m"], - run: () => options.toggleHunkHeaders(), - closesMenu: true, + "hunk.review.previousFile": { + run: (_key, count, entry) => runSelectionMove(options, entry, count), }, - { - id: "hunk.view.toggleCopyDecorations", - title: "Toggle copy decorations", - defaultKeys: [], - run: () => options.toggleCopyDecorations(), - closesMenu: true, + "hunk.review.nextFile": { + run: (_key, count, entry) => runSelectionMove(options, entry, count), }, - { - id: "hunk.review.toggleHunkGap", - title: "Expand or collapse context for the selected hunk", - defaultKeys: ["z"], - run: () => options.toggleGapForSelectedHunk(), - closesMenu: true, + "hunk.review.previousAnnotatedHunk": { + run: (_key, count, entry) => runSelectionMove(options, entry, count), }, - { - id: "hunk.review.editSelectedFile", - title: "Open the selected file in your editor", - defaultKeys: ["e"], - run: () => options.triggerEditSelectedFile(), - closesMenu: true, + "hunk.review.nextAnnotatedHunk": { + run: (_key, count, entry) => runSelectionMove(options, entry, count), }, - { - id: "hunk.review.previousHunk", - title: "Previous hunk", - defaultKeys: ["["], - run: (_key, count) => options.moveToHunk(-count), - closesMenu: true, + "hunk.review.previousAnnotatedFile": { + run: (_key, count, entry) => runSelectionMove(options, entry, count), }, - { - id: "hunk.review.nextHunk", - title: "Next hunk", - defaultKeys: ["]"], - run: (_key, count) => options.moveToHunk(count), - closesMenu: true, + "hunk.review.nextAnnotatedFile": { + run: (_key, count, entry) => runSelectionMove(options, entry, count), }, - { - id: "hunk.review.previousFile", - title: "Previous file", - defaultKeys: [","], - run: (_key, count) => options.moveToFile(-count), - closesMenu: true, - }, - { - id: "hunk.review.nextFile", - title: "Next file", - defaultKeys: ["."], - run: (_key, count) => options.moveToFile(count), - closesMenu: true, - }, - { - id: "hunk.review.previousAnnotatedHunk", - title: "Previous annotated hunk", - defaultKeys: ["{"], - run: (_key, count) => options.moveToAnnotatedHunk(-count), - closesMenu: true, - }, - { - id: "hunk.review.nextAnnotatedHunk", - title: "Next annotated hunk", - defaultKeys: ["}"], - run: (_key, count) => options.moveToAnnotatedHunk(count), - closesMenu: true, - }, - { - id: "hunk.review.previousAnnotatedFile", - title: "Previous annotated file", - defaultKeys: [], - run: (_key, count) => options.moveToAnnotatedFile(-count), - closesMenu: true, - }, - { - id: "hunk.review.nextAnnotatedFile", - title: "Next annotated file", - defaultKeys: [], - run: (_key, count) => options.moveToAnnotatedFile(count), - closesMenu: true, - }, - ]; + }; } /** @@ -504,26 +244,40 @@ function builtinCommandSpecs(options: BuildAppCommandsOptions): BuiltinCommandSp * answers to its new key *and* advertises it; a command the user unbound * resolves to no chords and simply never matches. */ -function toAppCommand(spec: BuiltinCommandSpec, resolvedKeys?: ResolvedCommandKeys): AppCommand { - const keys = resolvedKeys?.get(spec.id) ?? spec.defaultKeys; +function toAppCommand( + entry: AppCommandCatalogEntry, + handler: BuiltinCommandHandler, + resolvedKeys?: ResolvedCommandKeys, +): AppCommand { + const keys = resolvedKeys?.get(entry.id) ?? entry.defaultKeys; return { - id: spec.id, - title: spec.title, - defaultKeys: spec.defaultKeys, + id: entry.id, + title: entry.title, + defaultKeys: entry.defaultKeys, keys, keyLabels: keys.map(formatKeyChord), - isEnabled: spec.isEnabled, - publicToExtensions: PUBLIC_EXTENSION_COMMAND_IDS.has(spec.id), + isEnabled: handler.isEnabled, + publicToExtensions: entry.publicToExtensions, match: matchesAnyKeyChord(keys), - run: spec.run, - closesMenu: spec.closesMenu, + run: (key, count) => handler.run(key, count, entry), + closesMenu: entry.closesMenu, }; } -/** Build Hunk's built-in command table over live callbacks. */ +/** + * Build Hunk's built-in command table: catalogued identity over live callbacks. + * + * Order follows the catalog, which is what makes first-match-wins dispatch and the + * conflict probes below agree with what a browser palette would show. + */ export function buildAppCommands(options: BuildAppCommandsOptions): AppCommand[] { - return builtinCommandSpecs(options).map((spec) => toAppCommand(spec, options.resolvedKeys)); + const handlers = builtinCommandHandlers(options); + // Sound because the handler map is keyed by `AppCommandId` and therefore covers every + // catalogued id; a missing one is a type error where the map is written, not here. + return APP_COMMAND_CATALOG.map((entry) => + toAppCommand(entry, handlers[entry.id as AppCommandId], options.resolvedKeys), + ); } const NOOP_COMMAND_OPTIONS: BuildAppCommandsOptions = (() => { @@ -535,10 +289,7 @@ const NOOP_COMMAND_OPTIONS: BuildAppCommandsOptions = (() => { alignCurrentLine: noop, applyFilePresentationToAllMatching: noop, focusFilter: noop, - moveToAnnotatedFile: noop, - moveToAnnotatedHunk: noop, - moveToFile: noop, - moveToHunk: noop, + moveSelection: noop, openAgentSkill: noop, openThemeSelector: noop, requestQuit: noop, @@ -600,9 +351,9 @@ export function builtinCommandMatchProbes( * the defaults the app dispatches can never drift apart. */ export function builtinCommandKeyDefaults(): readonly CommandKeyDefaults[] { - return builtinCommandSpecs(NOOP_COMMAND_OPTIONS).map((spec) => ({ - id: spec.id, - defaultKeys: spec.defaultKeys, + return APP_COMMAND_CATALOG.map((entry) => ({ + id: entry.id, + defaultKeys: entry.defaultKeys, })); } diff --git a/src/ui/lib/appMenus.test.ts b/src/ui/lib/appMenus.test.ts index 77e402439..4822d6eaf 100644 --- a/src/ui/lib/appMenus.test.ts +++ b/src/ui/lib/appMenus.test.ts @@ -43,10 +43,7 @@ function createTestCommands(overrides: Partial = {}) { alignCurrentLine: record("alignCurrentLine"), applyFilePresentationToAllMatching: record("applyFilePresentationToAllMatching"), focusFilter: noop, - moveToAnnotatedFile: record("moveToAnnotatedFile"), - moveToAnnotatedHunk: noop, - moveToFile: noop, - moveToHunk: noop, + moveSelection: record("moveSelection"), openAgentSkill: record("openAgentSkill"), openThemeSelector: noop, requestQuit: record("requestQuit"), @@ -194,8 +191,8 @@ describe("buildAppMenus", () => { "toggleSidebar", "toggleCopyDecorations", "openAgentSkill", - "moveToAnnotatedFile:1", - "moveToAnnotatedFile:-1", + "moveSelection:annotated-file,1", + "moveSelection:annotated-file,-1", ]); }); diff --git a/src/ui/lib/helpContent.ts b/src/ui/lib/helpContent.ts index 45e1c69ac..3dfa6cc19 100644 --- a/src/ui/lib/helpContent.ts +++ b/src/ui/lib/helpContent.ts @@ -115,6 +115,16 @@ const HELP_SECTIONS: readonly HelpSectionSpec[] = [ }, ]; +/** + * Every command id the help dialog documents. + * + * Exported so the command-parity check can assert help names only catalogued commands: a + * row pointing at an id nobody registered would silently vanish instead of failing. + */ +export const HELP_COMMAND_IDS: readonly string[] = HELP_SECTIONS.flatMap((section) => + section.entries.flatMap((entry) => ("commandIds" in entry ? [...entry.commandIds] : [])), +); + /** * Render one entry's key column, or nothing when it documents no live key. * From 9ebd8ed6a6efe4dfb334a8d8e651bbff1225987c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 20:13:23 +0000 Subject: [PATCH 3/3] feat(review): add the semantic address grammar and navigation fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three consumers will need to name a place in a review across a boundary: browser deep links and history, a terminal "copy link", and agent surfaces that already address targets by file and hunk (docs/browser-review-seam-audit.md, G3). Add one serialize/parse pair over semantic keys only — never an index into rendered rows, which mean something different in the next client — with strict parsing, since an address arriving from a link is untrusted input. Land the navigation half of the conformance corpus alongside it, with the intent planner registered as its first consumer: the multi-step carry the session's walk lacked, the wrap-versus-clamp difference between scopes, a selection that outlives its file, and the pure-deletion reveal target the prototype browser resolved to a line that does not exist. Expectations are written by hand from the semantics, so they catch the bug rather than following it, and the same fixtures drive the wire in Phase 3 and the browser in Phase 5. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018L6h5GBz6RAxRXbgUS4mx4 --- .changeset/review-navigation-intents.md | 2 + docs/browser-review-seam-audit.md | 5 + src/core/review/address.test.ts | 77 ++++++ src/core/review/address.ts | 105 ++++++++ test/review-conformance/conformance.test.ts | 23 +- test/review-conformance/consumers.ts | 14 +- .../consumers/intentPlanner.ts | 120 +++++++++ test/review-conformance/navigationFixtures.ts | 230 ++++++++++++++++++ test/review-conformance/types.ts | 75 ++++++ 9 files changed, 647 insertions(+), 4 deletions(-) create mode 100644 .changeset/review-navigation-intents.md create mode 100644 src/core/review/address.test.ts create mode 100644 src/core/review/address.ts create mode 100644 test/review-conformance/consumers/intentPlanner.ts create mode 100644 test/review-conformance/navigationFixtures.ts diff --git a/.changeset/review-navigation-intents.md b/.changeset/review-navigation-intents.md new file mode 100644 index 000000000..a845151cc --- /dev/null +++ b/.changeset/review-navigation-intents.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/docs/browser-review-seam-audit.md b/docs/browser-review-seam-audit.md index 3b6f8bfc0..d8cd9ebc9 100644 --- a/docs/browser-review-seam-audit.md +++ b/docs/browser-review-seam-audit.md @@ -383,6 +383,11 @@ implementation does. grammar over semantic keys (`fileKey`/`hunkIndex`/side/line/noteId — never array indices or rendered rows) in `core/review`, used everywhere an address crosses a boundary. Core primitive in Phase 1; browser adoption Phase 5; opener fragments Phase 6. + _Repaid (Phase 1 PR 3, core primitive)_: `core/review/address.ts` serializes and parses the + four address kinds over percent-encoded semantic identifiers, with round-trip coverage for keys + carrying separators, percent signs, and non-ASCII characters, and strict rejection of anything + outside the grammar. No consumers yet, by design — browser deep links are Phase 5 and opener + fragments Phase 6, which is when this finding closes. - **G4. User-facing error catalog.** The repo already solves this once for agents: `src/session/agent/errors.ts` single-sources every message the generated skill quotes, with contract tests. The browser has no equivalent — action rejections (`invalid-action`, diff --git a/src/core/review/address.test.ts b/src/core/review/address.test.ts new file mode 100644 index 000000000..2091643d5 --- /dev/null +++ b/src/core/review/address.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, test } from "bun:test"; +import { formatReviewAddress, parseReviewAddress, type ReviewAddress } from "./address"; + +/** Identifiers that have broken naive string formats: separators, percents, unicode. */ +const AWKWARD_IDENTIFIERS = [ + "sha256:abc123", + "user:1717171717-4", + "src/nested/path.ts#2", + "key with spaces", + "100% coverage", + "ünïcøde-ключ", + "a/b/c/d", + "?query=1&other=2", +]; + +describe("review address grammar", () => { + // Intent: the canonical form is stable, so a link a client wrote stays readable. + test("serializes each address kind to its documented form", () => { + expect(formatReviewAddress({ kind: "file", fileKey: "abc" })).toBe("file/abc"); + expect(formatReviewAddress({ kind: "hunk", fileKey: "abc", hunkIndex: 2 })).toBe( + "file/abc/hunk/2", + ); + expect(formatReviewAddress({ kind: "line", fileKey: "abc", side: "old", line: 41 })).toBe( + "file/abc/line/old/41", + ); + expect(formatReviewAddress({ kind: "note", fileKey: "abc", noteId: "user:1" })).toBe( + "file/abc/note/user%3A1", + ); + }); + + // Intent: identifiers are opaque, so no key or note id can break out of the grammar. + test("round-trips every address kind through awkward identifiers", () => { + for (const identifier of AWKWARD_IDENTIFIERS) { + const addresses: ReviewAddress[] = [ + { kind: "file", fileKey: identifier }, + { kind: "hunk", fileKey: identifier, hunkIndex: 0 }, + { kind: "hunk", fileKey: identifier, hunkIndex: 17 }, + { kind: "line", fileKey: identifier, side: "old", line: 1 }, + { kind: "line", fileKey: identifier, side: "new", line: 9001 }, + { kind: "note", fileKey: identifier, noteId: identifier }, + ]; + + for (const address of addresses) { + expect(parseReviewAddress(formatReviewAddress(address))).toEqual(address); + } + } + }); + + // Intent: an address is untrusted input; a half-understood one must not navigate. + test("rejects anything that is not exactly the grammar", () => { + const rejected = [ + "", + "abc", + "file", + "file/", + "files/abc", + "file/abc/", + "file/abc/hunk", + "file/abc/hunk/", + "file/abc/hunk/-1", + "file/abc/hunk/1.5", + "file/abc/hunk/1/2", + "file/abc/line/new", + "file/abc/line/left/3", + "file/abc/line/new/0", + "file/abc/line/new/x", + "file/abc/note", + "file/abc/note/", + "file/abc/row/4", + "file/%zz/hunk/0", + ]; + + for (const text of rejected) { + expect(parseReviewAddress(text)).toBeUndefined(); + } + }); +}); diff --git a/src/core/review/address.ts b/src/core/review/address.ts new file mode 100644 index 000000000..995cfbb01 --- /dev/null +++ b/src/core/review/address.ts @@ -0,0 +1,105 @@ +/** + * Semantic addresses: one grammar for pointing at a place in a review. + * + * Three consumers need to name a location across a boundary — a browser deep link and its + * history entries, a terminal "copy link" command, and agent surfaces that already address + * targets by file and hunk (`docs/browser-review-seam-audit.md`, G3). Without one grammar + * each would invent its own string format and they would stop understanding each other. + * + * Addresses are built from semantic keys only: a file key, a hunk index, a side and line, + * a note id. Never an index into rendered rows — those depend on layout, expansion state, + * and window width, so an address built from them means something different in the next + * client, or in the same client one keypress later. + * + * The serialized form is a slash-separated path with percent-encoded identifier segments, + * which makes it safe inside a URL fragment without further escaping. + */ +import type { ReviewSide } from "./types"; + +export type ReviewAddress = + | { kind: "file"; fileKey: string } + | { kind: "hunk"; fileKey: string; hunkIndex: number } + | { kind: "line"; fileKey: string; side: ReviewSide; line: number } + | { kind: "note"; fileKey: string; noteId: string }; + +/** Serialize one address into its canonical string form. */ +export function formatReviewAddress(address: ReviewAddress): string { + const file = `file/${encodeURIComponent(address.fileKey)}`; + switch (address.kind) { + case "file": + return file; + case "hunk": + return `${file}/hunk/${address.hunkIndex}`; + case "line": + return `${file}/line/${address.side}/${address.line}`; + case "note": + return `${file}/note/${encodeURIComponent(address.noteId)}`; + } +} + +/** Decode one identifier segment, rejecting an empty or malformed one. */ +function decodeSegment(segment: string | undefined) { + if (!segment) { + return undefined; + } + try { + const decoded = decodeURIComponent(segment); + return decoded.length > 0 ? decoded : undefined; + } catch { + // A stray percent sign is a malformed address, not a key containing one. + return undefined; + } +} + +/** Parse one non-negative integer segment, rejecting anything else. */ +function parseIndex(segment: string | undefined, minimum: number) { + if (segment === undefined || !/^\d+$/.test(segment)) { + return undefined; + } + const value = Number(segment); + return Number.isSafeInteger(value) && value >= minimum ? value : undefined; +} + +/** + * Parse one address, or report that the text is not one. + * + * Deliberately strict: an address that arrived from a link, a fragment, or an agent + * command is untrusted input, and a half-understood one would silently navigate somewhere + * other than where it points. Anything that is not exactly this grammar is rejected. + */ +export function parseReviewAddress(text: string): ReviewAddress | undefined { + const segments = text.split("/"); + if (segments[0] !== "file") { + return undefined; + } + + const fileKey = decodeSegment(segments[1]); + if (fileKey === undefined) { + return undefined; + } + + if (segments.length === 2) { + return { kind: "file", fileKey }; + } + + switch (segments[2]) { + case "hunk": { + const hunkIndex = segments.length === 4 ? parseIndex(segments[3], 0) : undefined; + return hunkIndex === undefined ? undefined : { kind: "hunk", fileKey, hunkIndex }; + } + case "line": { + const side = segments[3]; + // Lines are 1-based everywhere in the model, so line 0 is not an address. + const line = segments.length === 5 ? parseIndex(segments[4], 1) : undefined; + return line === undefined || (side !== "old" && side !== "new") + ? undefined + : { kind: "line", fileKey, side, line }; + } + case "note": { + const noteId = segments.length === 4 ? decodeSegment(segments[3]) : undefined; + return noteId === undefined ? undefined : { kind: "note", fileKey, noteId }; + } + default: + return undefined; + } +} diff --git a/test/review-conformance/conformance.test.ts b/test/review-conformance/conformance.test.ts index e52c76885..dac4b05bf 100644 --- a/test/review-conformance/conformance.test.ts +++ b/test/review-conformance/conformance.test.ts @@ -2,12 +2,13 @@ import { describe, expect, test } from "bun:test"; import { isBlankReviewNoteBody, planReviewIntent } from "../../src/core/review/intents"; import { createInitialReviewState } from "../../src/core/review/state"; import { createTestReviewDocument } from "../helpers/review-store-helpers"; -import { REVIEW_CONFORMANCE_CONSUMERS } from "./consumers"; +import { REVIEW_CONFORMANCE_CONSUMERS, REVIEW_NAVIGATION_CONSUMERS } from "./consumers"; import { REVIEW_CONFORMANCE_FIXTURES } from "./fixtures"; +import { REVIEW_NAVIGATION_FIXTURES } from "./navigationFixtures"; import { REVIEW_NOTE_BODY_FIXTURES } from "./noteBodies"; /** Findings whose adversarial fixture must exist for the finding to count as repaid. */ -const REQUIRED_FINDINGS = ["A1", "A2", "A3", "A4", "A8", "A10"]; +const REQUIRED_FINDINGS = ["A1", "A2", "A3", "A4", "A8", "A10", "B1", "B2", "B3", "B4", "B6"]; describe("review conformance corpus", () => { test("registers every consumer that has landed so far", () => { @@ -15,15 +16,31 @@ describe("review conformance corpus", () => { "core review model", "terminal render planning", ]); + expect(REVIEW_NAVIGATION_CONSUMERS.map((consumer) => consumer.name)).toEqual([ + "core intent planner", + ]); }); test("carries an adversarial fixture for every finding it claims to repay", () => { - const covered = new Set(REVIEW_CONFORMANCE_FIXTURES.flatMap((fixture) => fixture.findings)); + const covered = new Set([ + ...REVIEW_CONFORMANCE_FIXTURES.flatMap((fixture) => fixture.findings), + ...REVIEW_NAVIGATION_FIXTURES.flatMap((fixture) => fixture.findings), + ]); expect(REQUIRED_FINDINGS.filter((finding) => !covered.has(finding))).toEqual([]); }); }); +for (const consumer of REVIEW_NAVIGATION_CONSUMERS) { + describe(`review navigation conformance: ${consumer.name}`, () => { + for (const fixture of REVIEW_NAVIGATION_FIXTURES) { + test(`${fixture.id} (${fixture.findings.join(", ")})`, () => { + expect(consumer.project(fixture)).toEqual(fixture.expected); + }); + } + }); +} + for (const consumer of REVIEW_CONFORMANCE_CONSUMERS) { describe(`review conformance: ${consumer.name}`, () => { for (const fixture of REVIEW_CONFORMANCE_FIXTURES) { diff --git a/test/review-conformance/consumers.ts b/test/review-conformance/consumers.ts index 8703a8f22..f3143553c 100644 --- a/test/review-conformance/consumers.ts +++ b/test/review-conformance/consumers.ts @@ -7,10 +7,22 @@ * own has joined (`docs/browser-review-rebuild.md` § "Per-phase seam verification"). */ import { coreModelConsumer } from "./consumers/coreModel"; +import { intentPlannerNavigationConsumer } from "./consumers/intentPlanner"; import { terminalRenderPlanConsumer } from "./consumers/terminalRenderPlan"; -import type { ReviewConformanceConsumer } from "./types"; +import type { ReviewConformanceConsumer, ReviewNavigationConsumer } from "./types"; export const REVIEW_CONFORMANCE_CONSUMERS: readonly ReviewConformanceConsumer[] = [ coreModelConsumer, terminalRenderPlanConsumer, ]; + +/** + * Consumers of the shared navigation semantics. + * + * A separate registry because navigation answers different questions than geometry, under + * the same contract: the browser's projection and the wire join these fixtures in later + * phases, and every earlier consumer keeps running. + */ +export const REVIEW_NAVIGATION_CONSUMERS: readonly ReviewNavigationConsumer[] = [ + intentPlannerNavigationConsumer, +]; diff --git a/test/review-conformance/consumers/intentPlanner.ts b/test/review-conformance/consumers/intentPlanner.ts new file mode 100644 index 000000000..671b3517d --- /dev/null +++ b/test/review-conformance/consumers/intentPlanner.ts @@ -0,0 +1,120 @@ +/** + * The shared intent planner as a navigation conformance consumer. + * + * Everything is read back through `planReviewIntent` — the same entry point the terminal's + * keyboard, the session's comment navigation, and later the wire all go through — rather + * than by calling the walk directly, so a planner that stopped consulting the shared + * selectors would show up here. + */ +import { projectReviewDocument } from "../../../src/core/review/document"; +import { planReviewIntent } from "../../../src/core/review/intents"; +import type { ReviewAnnotationIndex } from "../../../src/core/review/navigation"; +import { selectNormalizedSelection, selectRevealTarget } from "../../../src/core/review/selectors"; +import { createInitialReviewState, type ReviewState } from "../../../src/core/review/state"; +import type { ReviewDocumentV1 } from "../../../src/core/review/types"; +import type { + ConformanceSelection, + ConformanceSelectionInput, + ReviewNavigationConsumer, + ReviewNavigationFixture, +} from "../types"; + +/** A key no projected document can produce, standing in for a file a reload dropped. */ +const VANISHED_FILE_KEY = "vanished:no-such-file"; + +/** Resolve a fixture's positional selection into the semantic one core reads. */ +function toSemanticSelection(input: ConformanceSelectionInput, document: ReviewDocumentV1) { + if (input.file === null) { + return { fileKey: null, hunkIndex: input.hunkIndex }; + } + if (input.file === "vanished") { + return { fileKey: VANISHED_FILE_KEY, hunkIndex: input.hunkIndex }; + } + return { + fileKey: document.files[input.file]?.key ?? VANISHED_FILE_KEY, + hunkIndex: input.hunkIndex, + }; +} + +/** Report a semantic position back as the file index the fixture states. */ +function toConformanceSelection( + fileKey: string | null, + hunkIndex: number, + document: ReviewDocumentV1, +): ConformanceSelection { + const index = document.files.findIndex((file) => file.key === fileKey); + return { file: index < 0 ? null : index, hunkIndex }; +} + +/** Build the annotation index the fixture declares, keyed by semantic file key. */ +function toAnnotationIndex( + fixture: ReviewNavigationFixture, + document: ReviewDocumentV1, +): ReviewAnnotationIndex { + const annotatedHunks = Object.entries(fixture.annotatedHunks ?? {}); + const annotatedHunkIndicesByFileKey = new Map( + annotatedHunks.flatMap(([fileIndex, hunkIndices]) => { + const file = document.files[Number(fileIndex)]; + return file ? [[file.key, new Set(hunkIndices)] as const] : []; + }), + ); + const annotatedFileIndices = + fixture.annotatedFiles ?? annotatedHunks.map(([fileIndex]) => Number(fileIndex)); + return { + annotatedHunkIndicesByFileKey, + annotatedFileKeys: new Set( + annotatedFileIndices.flatMap((fileIndex) => { + const file = document.files[fileIndex]; + return file ? [file.key] : []; + }), + ), + }; +} + +export const intentPlannerNavigationConsumer: ReviewNavigationConsumer = { + name: "core intent planner", + phase: "Phase 1 PR 3", + project(fixture: ReviewNavigationFixture) { + const document = projectReviewDocument(fixture.build()); + const annotations = toAnnotationIndex(fixture, document); + const baseState: ReviewState = { + ...createInitialReviewState(document), + filter: fixture.filter ?? "", + }; + const stateAt = (input: ConformanceSelectionInput) => ({ + ...baseState, + selection: toSemanticSelection(input, document), + }); + + return { + moves: fixture.moves.map((move) => { + const plan = planReviewIntent( + stateAt(move.from), + { type: "selection/move", scope: move.scope, delta: move.delta }, + { annotations }, + ); + const action = plan.actions[0]; + if (!action || action.type !== "selection/select" || !action.reveal) { + return { to: null }; + } + return { + to: toConformanceSelection(action.fileKey, action.hunkIndex, document), + reveal: action.reveal, + }; + }), + normalizedSelections: fixture.selections.map((input) => { + const normalized = selectNormalizedSelection(stateAt(input)); + return toConformanceSelection(normalized.fileKey, normalized.hunkIndex, document); + }), + revealTargets: document.files.map((file, fileIndex) => + file.hunks.map( + (_hunk, hunkIndex) => + selectRevealTarget({ + ...baseState, + selection: toSemanticSelection({ file: fileIndex, hunkIndex }, document), + }) ?? null, + ), + ), + }; + }, +}; diff --git a/test/review-conformance/navigationFixtures.ts b/test/review-conformance/navigationFixtures.ts new file mode 100644 index 000000000..b1b417aa6 --- /dev/null +++ b/test/review-conformance/navigationFixtures.ts @@ -0,0 +1,230 @@ +/** + * The navigation half of the golden corpus. + * + * These fixtures pin what the shared walk answers: where a repeated annotated step lands, + * which scopes wrap and which stop, what a selection means once its file is filtered away + * or gone, and which line a reveal scrolls to. Every expectation is written by hand from + * the semantics — the audit's B-findings are exactly the cases the old per-consumer copies + * disagreed about, so a captured expectation would preserve the disagreement. + */ +import { createTestDiffFile, lines } from "../helpers/diff-helpers"; +import type { DiffFile } from "../../src/core/types"; +import type { ReviewNavigationFixture } from "./types"; + +/** Twelve numbered lines, the base every navigation fixture edits. */ +const BASE_LINES = Array.from({ length: 12 }, (_unused, index) => `line ${index + 1}`); + +/** Rewrite the given 1-based lines, leaving the rest of the file alone. */ +function withEdits(edits: Record) { + return lines(...BASE_LINES.map((line, index) => edits[index + 1] ?? line)); +} + +/** + * A file whose two edits are far enough apart to parse as two hunks at zero context: + * `@@ -2,1 +2,1 @@` and `@@ -10,1 +10,1 @@`. + */ +function twoHunkFile(id: string): DiffFile { + return createTestDiffFile({ + id, + path: `${id}.ts`, + before: lines(...BASE_LINES), + after: withEdits({ 2: "line two", 10: "line ten" }), + context: 0, + }); +} + +/** The three-file, six-hunk stream most navigation fixtures walk. */ +function threeFileStream(): DiffFile[] { + return [twoHunkFile("alpha"), twoHunkFile("beta"), twoHunkFile("gamma")]; +} + +/** Both hunks of a `twoHunkFile`, as reveal targets: the changed line on each side. */ +const TWO_HUNK_REVEAL_TARGETS = [ + { side: "new", line: 2 }, + { side: "new", line: 10 }, +] as const; + +const THREE_FILE_REVEAL_TARGETS = [ + [...TWO_HUNK_REVEAL_TARGETS], + [...TWO_HUNK_REVEAL_TARGETS], + [...TWO_HUNK_REVEAL_TARGETS], +]; + +/** The reveal every annotated-hunk landing asks for. */ +const NOTE_REVEAL = { anchor: "hunk", scrollToNote: true } as const; +const HUNK_REVEAL = { anchor: "hunk", scrollToNote: false } as const; +const FILE_TOP_REVEAL = { anchor: "file-top", scrollToNote: false } as const; + +export const REVIEW_NAVIGATION_FIXTURES: readonly ReviewNavigationFixture[] = [ + { + id: "annotated-hunk-multi-step-carry", + findings: ["B1"], + description: + "Stepping from an unannotated hunk: the first step reaches the nearest annotated hunk, and the rest of the count is spent from there rather than swallowed by the approach.", + build: threeFileStream, + // Annotated cursors, in stream order: alpha:0, beta:1, gamma:0, gamma:1. + annotatedHunks: { 0: [0], 1: [1], 2: [0, 1] }, + moves: [ + { scope: "annotated-hunk", delta: 1, from: { file: 0, hunkIndex: 1 } }, + { scope: "annotated-hunk", delta: 2, from: { file: 0, hunkIndex: 1 } }, + { scope: "annotated-hunk", delta: 3, from: { file: 0, hunkIndex: 1 } }, + { scope: "annotated-hunk", delta: 9, from: { file: 0, hunkIndex: 1 } }, + { scope: "annotated-hunk", delta: -1, from: { file: 2, hunkIndex: 0 } }, + { scope: "annotated-hunk", delta: -2, from: { file: 2, hunkIndex: 0 } }, + { scope: "annotated-hunk", delta: -9, from: { file: 2, hunkIndex: 0 } }, + // From a position already in the subset, the count applies directly. + { scope: "annotated-hunk", delta: 2, from: { file: 0, hunkIndex: 0 } }, + ], + selections: [{ file: 1, hunkIndex: 1 }], + expected: { + moves: [ + { to: { file: 1, hunkIndex: 1 }, reveal: NOTE_REVEAL }, + { to: { file: 2, hunkIndex: 0 }, reveal: NOTE_REVEAL }, + { to: { file: 2, hunkIndex: 1 }, reveal: NOTE_REVEAL }, + { to: { file: 2, hunkIndex: 1 }, reveal: NOTE_REVEAL }, + { to: { file: 1, hunkIndex: 1 }, reveal: NOTE_REVEAL }, + { to: { file: 0, hunkIndex: 0 }, reveal: NOTE_REVEAL }, + { to: { file: 0, hunkIndex: 0 }, reveal: NOTE_REVEAL }, + { to: { file: 2, hunkIndex: 0 }, reveal: NOTE_REVEAL }, + ], + normalizedSelections: [{ file: 1, hunkIndex: 1 }], + revealTargets: THREE_FILE_REVEAL_TARGETS, + }, + }, + { + id: "scope-wrap-and-clamp", + findings: ["B2", "B3"], + description: + "The same edge, four scopes: hunk re-reveals, file declines to move at all, annotated-hunk clamps, annotated-file cycles.", + build: threeFileStream, + // Only the outer files carry notes, so the ring has two stops with a gap between them. + annotatedHunks: { 0: [0], 2: [0] }, + moves: [ + { scope: "hunk", delta: 1, from: { file: 2, hunkIndex: 1 } }, + { scope: "hunk", delta: -1, from: { file: 0, hunkIndex: 0 } }, + { scope: "hunk", delta: 1, from: { file: 0, hunkIndex: 1 } }, + { scope: "hunk", delta: -1, from: { file: 1, hunkIndex: 0 } }, + { scope: "file", delta: 1, from: { file: 2, hunkIndex: 1 } }, + { scope: "file", delta: -1, from: { file: 0, hunkIndex: 0 } }, + { scope: "file", delta: 1, from: { file: 0, hunkIndex: 1 } }, + { scope: "annotated-hunk", delta: 1, from: { file: 2, hunkIndex: 0 } }, + { scope: "annotated-file", delta: 1, from: { file: 2, hunkIndex: 0 } }, + { scope: "annotated-file", delta: -1, from: { file: 0, hunkIndex: 0 } }, + // From a file with no notes, the ring is entered at its start before stepping. + { scope: "annotated-file", delta: 1, from: { file: 1, hunkIndex: 0 } }, + ], + selections: [{ file: 2, hunkIndex: 1 }], + expected: { + moves: [ + // Clamping re-selects the same hunk and asks to be shown it again. + { to: { file: 2, hunkIndex: 1 }, reveal: HUNK_REVEAL }, + { to: { file: 0, hunkIndex: 0 }, reveal: HUNK_REVEAL }, + // Crossing forward into another file puts that file's header on screen. + { to: { file: 1, hunkIndex: 0 }, reveal: FILE_TOP_REVEAL }, + // Crossing backward reveals the hunk itself, near the previous file's end. + { to: { file: 0, hunkIndex: 1 }, reveal: HUNK_REVEAL }, + // File navigation at an end does nothing at all. + { to: null }, + { to: null }, + { to: { file: 1, hunkIndex: 0 }, reveal: FILE_TOP_REVEAL }, + { to: { file: 2, hunkIndex: 0 }, reveal: NOTE_REVEAL }, + { to: { file: 0, hunkIndex: 0 }, reveal: HUNK_REVEAL }, + { to: { file: 2, hunkIndex: 0 }, reveal: HUNK_REVEAL }, + { to: { file: 2, hunkIndex: 0 }, reveal: HUNK_REVEAL }, + ], + normalizedSelections: [{ file: 2, hunkIndex: 1 }], + revealTargets: THREE_FILE_REVEAL_TARGETS, + }, + }, + { + id: "selection-outliving-its-file", + findings: ["B4"], + description: + "A filter hiding the selected file leaves the selection alone; a selection whose file the document lost falls back to the first visible file, never to a hidden one.", + build: () => [twoHunkFile("alpha"), twoHunkFile("beta")], + filter: "beta", + moves: [ + // Navigation walks only what the filter shows, from wherever the selection resolves. + { scope: "hunk", delta: 1, from: { file: "vanished", hunkIndex: 0 } }, + { scope: "file", delta: 1, from: { file: 0, hunkIndex: 0 } }, + ], + selections: [ + { file: 0, hunkIndex: 1 }, + { file: "vanished", hunkIndex: 3 }, + { file: null, hunkIndex: 0 }, + { file: 1, hunkIndex: 9 }, + ], + expected: { + moves: [ + { to: { file: 1, hunkIndex: 1 }, reveal: HUNK_REVEAL }, + // Alpha is hidden, so it is not a step away from anything. + { to: null }, + ], + normalizedSelections: [ + // Hidden, but still where the reviewer was. + { file: 0, hunkIndex: 1 }, + { file: 1, hunkIndex: 0 }, + { file: 1, hunkIndex: 0 }, + // A stale index clamps onto the file it addresses. + { file: 1, hunkIndex: 1 }, + ], + revealTargets: [[...TWO_HUNK_REVEAL_TARGETS], [...TWO_HUNK_REVEAL_TARGETS]], + }, + }, + { + id: "selection-with-nothing-visible", + findings: ["B4"], + description: + "A filter matching no file leaves nothing to select: the review renders no file rather than quietly falling back to the first one.", + build: () => [twoHunkFile("alpha")], + filter: "matches-no-file", + moves: [{ scope: "hunk", delta: 1, from: { file: 0, hunkIndex: 0 } }], + selections: [ + { file: "vanished", hunkIndex: 0 }, + { file: null, hunkIndex: 0 }, + ], + expected: { + moves: [{ to: null }], + normalizedSelections: [ + { file: null, hunkIndex: 0 }, + { file: null, hunkIndex: 0 }, + ], + revealTargets: [[...TWO_HUNK_REVEAL_TARGETS]], + }, + }, + { + id: "pure-deletion-reveal-target", + findings: ["B6"], + description: + "@@ -6,1 +5,0 @@ — the new side has no rows, so the reveal target is the old-side line; a file whose hunk opens with context reveals its first row, not its first change.", + build: () => [ + createTestDiffFile({ + id: "deletion", + path: "deletion.ts", + before: lines(...BASE_LINES), + after: lines(...BASE_LINES.filter((line) => line !== "line 6")), + context: 0, + }), + createTestDiffFile({ + id: "context", + path: "context.ts", + before: lines(...BASE_LINES), + after: withEdits({ 6: "line six" }), + context: 3, + }), + ], + moves: [], + selections: [{ file: 0, hunkIndex: 0 }], + expected: { + moves: [], + normalizedSelections: [{ file: 0, hunkIndex: 0 }], + revealTargets: [ + // Not `{ side: "new", line: 5 }`: the new side has no rows here at all. + [{ side: "old", line: 6 }], + // The hunk spans lines 3-9; its position is its first row, while a note about the + // whole hunk would hang from the changed line 6. + [{ side: "new", line: 3 }], + ], + }, + }, +]; diff --git a/test/review-conformance/types.ts b/test/review-conformance/types.ts index 601c66e23..f0ad70320 100644 --- a/test/review-conformance/types.ts +++ b/test/review-conformance/types.ts @@ -16,6 +16,7 @@ * - The projection is renderer-neutral. Anything only one consumer can produce — rows, * widths, DOM — stays out, or the corpus stops being comparable. */ +import type { ReviewSelectionScope } from "../../src/core/review/navigation"; import type { DiffFile } from "../../src/core/types"; export interface ConformanceGap { @@ -93,3 +94,77 @@ export interface ReviewConformanceConsumer { phase: string; project: (fixture: ReviewConformanceFixture) => ReviewConformanceProjection; } + +/** + * One position in a fixture's review, addressed the way a fixture can state it. + * + * `"vanished"` names a file key the document does not have — the reload case, where a + * selection outlives the file it pointed at. + */ +export type ConformanceSelectionInput = + | { file: number; hunkIndex: number } + | { file: "vanished"; hunkIndex: number } + | { file: null; hunkIndex: number }; + +/** One resolved position: an index into the fixture's files, or nothing addressable. */ +export interface ConformanceSelection { + file: number | null; + hunkIndex: number; +} + +export interface ConformanceReveal { + anchor: "hunk" | "file-top" | "none"; + scrollToNote: boolean; +} + +export interface ConformanceMove { + scope: ReviewSelectionScope; + delta: number; + from: ConformanceSelectionInput; +} + +/** Where one move landed and what it asked the viewport for; `to: null` means refused. */ +export interface ConformanceMoveOutcome { + to: ConformanceSelection | null; + reveal?: ConformanceReveal; +} + +export interface ReviewNavigationProjection { + moves: ConformanceMoveOutcome[]; + /** What each declared starting point normalizes to under the fixture's filter. */ + normalizedSelections: ConformanceSelection[]; + /** The line a reveal targets, per hunk, per file. */ + revealTargets: Array>; +} + +export interface ReviewNavigationFixture { + id: string; + /** Audit finding ids this fixture guards, e.g. `B1`. */ + findings: string[]; + /** What makes this input adversarial, in one line. */ + description: string; + build: () => DiffFile[]; + /** The filter as the reviewer typed it, applied before anything is planned. */ + filter?: string; + /** Hunk indices carrying notes, by file index. */ + annotatedHunks?: Record; + /** File indices carrying review context; defaults to the files with annotated hunks. */ + annotatedFiles?: number[]; + moves: ConformanceMove[]; + selections: ConformanceSelectionInput[]; + /** Hand-written from the semantics — never captured from a primitive. */ + expected: ReviewNavigationProjection; +} + +/** + * One consumer of the shared navigation semantics. + * + * Registered separately from the geometry consumers because it answers different + * questions, against the same rule: expectations are hand-written, and a consumer joins by + * driving the code path it really uses. + */ +export interface ReviewNavigationConsumer { + name: string; + phase: string; + project: (fixture: ReviewNavigationFixture) => ReviewNavigationProjection; +}