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 fd7c04658..cc15b4c84 100644 --- a/docs/browser-review-seam-audit.md +++ b/docs/browser-review-seam-audit.md @@ -116,35 +116,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` / @@ -155,6 +211,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 @@ -258,6 +320,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 @@ -265,11 +336,30 @@ 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. + Residual (found in review): `lowerAppCommandToReviewIntent` has no production caller yet — + the terminal's navigation handlers read the catalog's declared scope/direction but build + their intents inline, and `toggleAgentNotes` reads no catalog data at all, so the lowering + and the terminal closures can diverge with only `commandCatalog.test.ts` noticing half the + drift. Closes when the lowering gains its second consumer (the Phase 5 palette / wire + command path); until then any change to a declared review effect must update both sites, + and a review-effect parity check is the missing test. - **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. @@ -305,6 +395,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/scripts/source-boundaries.test.ts b/scripts/source-boundaries.test.ts index 429041a72..6e56bb342 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 ]; // Function-level deletions the file tombstones cannot see: each entry bans one named @@ -143,6 +143,10 @@ const EXTRACTED_DUPLICATE_SYMBOLS: ReadonlyArray<{ { file: "src/core/liveComments.ts", symbol: "hunkLineRange", finding: "A3" }, { file: "src/core/liveComments.ts", symbol: "firstCommentTargetForHunk", finding: "A10" }, { file: "src/core/review/state.ts", symbol: "reviewLineAnchor", finding: "A3" }, + { file: "src/ui/lib/files.ts", symbol: "filterReviewFiles", finding: "B5" }, + { file: "src/ui/lib/reviewState.ts", symbol: "findNextAnnotatedFile", finding: "B2" }, + { file: "src/ui/lib/reviewState.ts", symbol: "resolveSelectedFile", finding: "B4" }, + { file: "src/ui/lib/agentAnnotations.ts", symbol: "alwaysShowReviewNote", finding: "B9" }, ]; describe("source architecture boundaries", () => { 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..c344d688d --- /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. + * + * Builds the intent one semantic command lowers to, so a keyboard chord, a browser + * palette entry, and an agent command naming 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/core/review/address.test.ts b/src/core/review/address.test.ts new file mode 100644 index 000000000..62f3a7e35 --- /dev/null +++ b/src/core/review/address.test.ts @@ -0,0 +1,99 @@ +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: canonical serialization must never produce text the strict parser rejects. + test("refuses values the address grammar cannot represent", () => { + const invalid: ReviewAddress[] = [ + { kind: "file", fileKey: "" }, + { kind: "hunk", fileKey: "abc", hunkIndex: -1 }, + { kind: "hunk", fileKey: "abc", hunkIndex: 1.5 }, + { kind: "hunk", fileKey: "abc", hunkIndex: Number.POSITIVE_INFINITY }, + { kind: "line", fileKey: "abc", side: "new", line: 0 }, + { kind: "line", fileKey: "abc", side: "new", line: -1 }, + { kind: "line", fileKey: "abc", side: "new", line: 1.5 }, + { kind: "line", fileKey: "abc", side: "left" as never, line: 1 }, + { kind: "note", fileKey: "abc", noteId: "" }, + ]; + + for (const address of invalid) { + expect(() => formatReviewAddress(address)).toThrow(); + } + expect(() => formatReviewAddress({ kind: "file", fileKey: "\ud800" })).toThrow( + "must be valid Unicode", + ); + }); + + // 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..a0a65fd1f --- /dev/null +++ b/src/core/review/address.ts @@ -0,0 +1,135 @@ +/** + * 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 }; + +/** Encode one required identifier, rejecting values the address grammar cannot represent. */ +function encodeIdentifier(value: string, label: "file key" | "note id") { + if (value.length === 0) { + throw new TypeError(`Review address ${label} must not be empty.`); + } + + try { + return encodeURIComponent(value); + } catch { + // A lone surrogate cannot be percent-encoded into a URL-safe identifier. + throw new TypeError(`Review address ${label} must be valid Unicode.`); + } +} + +/** Format one integer segment the parser will accept. */ +function formatIndex(value: number, label: "hunk index" | "line", minimum: number) { + if (!Number.isSafeInteger(value) || value < minimum) { + throw new RangeError(`Review address ${label} must be an integer at least ${minimum}.`); + } + return String(value); +} + +/** Validate the line side that remains a runtime value at this public boundary. */ +function formatSide(side: ReviewSide) { + if (side !== "old" && side !== "new") { + throw new TypeError("Review address line side must be old or new."); + } + return side; +} + +/** Serialize one valid address into its canonical string form. */ +export function formatReviewAddress(address: ReviewAddress): string { + const file = `file/${encodeIdentifier(address.fileKey, "file key")}`; + switch (address.kind) { + case "file": + return file; + case "hunk": + return `${file}/hunk/${formatIndex(address.hunkIndex, "hunk index", 0)}`; + case "line": + return `${file}/line/${formatSide(address.side)}/${formatIndex(address.line, "line", 1)}`; + case "note": + return `${file}/note/${encodeIdentifier(address.noteId, "note id")}`; + } +} + +/** 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/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..7169e0133 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; @@ -100,7 +143,7 @@ export class ReviewIntentPlanningError extends Error { } /** - * The one empty-note-body rule. + * Decides what counts as a blank note body. * * Surfaces differ in what they do about a blank body — the terminal quietly retires the * draft, an agent command reports a rejection — but not in what counts as blank. @@ -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..046aedadb 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; + +/** + * Matches one file against the shared review filter. + * + * 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..791267329 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"; } +/** + * Decides whether one note stays visible under the agent-notes toggle. + * + * 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..ddb98fa90 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,10 +1822,7 @@ export function App({ alignCurrentLine, applyFilePresentationToAllMatching, focusFilter, - moveToAnnotatedFile, - moveToAnnotatedHunk, - moveToFile, - moveToHunk: review.moveToHunk, + moveSelection: review.moveSelection, openAgentSkill, openThemeSelector, requestQuit, @@ -2047,7 +2038,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 +2198,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.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 9a48d5312..87babe983 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/src/ui/hooks/useReviewController.test.tsx @@ -223,7 +223,7 @@ async function renderReviewController( } describe("useReviewController", () => { - test("reselects the first visible file when filtering hides the current selection", async () => { + test("preserves a filtered-out selection until the reviewer clears the filter", async () => { const { controllerRef, setup } = await renderReviewController([ createDiffFile("alpha", "alpha.ts", "export const alpha = 1;\n", "export const alpha = 2;\n"), createDiffFile( @@ -246,9 +246,21 @@ describe("useReviewController", () => { expect(expectValue(controllerRef.current).visibleFiles.map((file) => file.path)).toEqual([ "beta.ts", ]); - expect(expectValue(controllerRef.current).selectedFileId).toBe("beta"); - expect(expectValue(controllerRef.current).selectedFile?.path).toBe("beta.ts"); + expect(expectValue(controllerRef.current).selectedFileId).toBe("alpha"); + expect(expectValue(controllerRef.current).selectedFile?.path).toBe("alpha.ts"); expect(expectValue(controllerRef.current).selectedHunkIndex).toBe(0); + + await act(async () => { + expectValue(controllerRef.current).clearFilter(); + }); + await flush(setup); + + expect(expectValue(controllerRef.current).visibleFiles.map((file) => file.path)).toEqual([ + "alpha.ts", + "beta.ts", + ]); + expect(expectValue(controllerRef.current).selectedFileId).toBe("alpha"); + expect(expectValue(controllerRef.current).selectedFile?.path).toBe("alpha.ts"); } finally { await act(async () => { setup.renderer.destroy(); @@ -338,7 +350,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 +360,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 +369,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 +378,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 +387,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 +396,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 +423,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 +471,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 +1678,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 +1702,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); @@ -1732,7 +1744,7 @@ describe("useReviewController", () => { } }); - test("re-seeds the current line into the file a filter leaves visible", async () => { + test("clears the line cursor while its selected file is filtered out", async () => { const { controllerRef, setup } = await renderReviewController([ createDiffFile("alpha", "alpha.ts", "export const alpha = 1;\n", "export const alpha = 2;\n"), createDiffFile( @@ -1752,7 +1764,14 @@ describe("useReviewController", () => { }); await flush(setup); - expect(expectValue(expectValue(controllerRef.current).lineCursor).fileId).toBe("beta"); + expect(expectValue(controllerRef.current).lineCursor).toBeNull(); + + await act(async () => { + expectValue(controllerRef.current).clearFilter(); + }); + await flush(setup); + + expect(expectValue(expectValue(controllerRef.current).lineCursor).fileId).toBe("alpha"); } finally { await act(async () => { setup.renderer.destroy(); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index 6d394b301..cb2386391 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -34,10 +34,12 @@ 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, + selectNormalizedSelection, } from "../../core/review/selectors"; import type { ReviewDraftNote, ReviewRevealRequest } from "../../core/review/state"; import { createReviewStore, type ReviewStore } from "../../core/review/store"; @@ -59,7 +61,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 +83,13 @@ import { type UserReviewNote, } from "../lib/reviewProjection"; import { + buildReviewAnnotationIndex, buildReviewStreamState, buildSelectedHunkSummary, - findNextAnnotatedFile, + planTerminalSelectionReconciliation, 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,11 +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; - moveToAnnotatedFile: (delta: number) => void; - moveToAnnotatedHunk: (delta: number) => void; - moveToFile: (delta: number) => void; - moveToHunk: (delta: number) => void; + /** Step the selection through one navigable scope; the scope owns wrap and reveal. */ + moveSelection: (scope: ReviewSelectionScope, delta: number) => void; scrollToNote: boolean; selectedFile: DiffFile | undefined; selectedFileId: string; @@ -214,7 +209,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: ( @@ -293,7 +289,6 @@ export function useReviewController({ const state = useReviewStoreSnapshot(store); const filter = state.filter; - const selectedHunkIndex = state.selection.hunkIndex; const scrollToNote = state.reveal.scrollToNote; const [lineCursor, setLineCursor] = useState(null); // A held key drains as one stdin chunk, so every press in the burst would otherwise read the @@ -361,7 +356,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 +365,23 @@ export function useReviewController({ }), [deferredFilter, files, liveCommentsByFileId, userNotesByFileId], ); - const selectedFileId = state.selection.fileKey - ? (fileByKey.get(state.selection.fileKey)?.id ?? "") - : ""; - const selectedFile = useMemo( - () => resolveSelectedFile(allFiles, visibleFiles, selectedFileId), - [allFiles, selectedFileId, visibleFiles], + // 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 selectedHunkIndex = normalizedSelection.hunkIndex; + const selectedFileId = normalizedSelection.fileKey + ? (fileByKey.get(normalizedSelection.fileKey)?.id ?? "") + : ""; + const selectedFile = normalizedSelection.fileKey + ? fileByKey.get(normalizedSelection.fileKey) + : undefined; const selectedHunk = selectedFile?.metadata.hunks[selectedHunkIndex]; /** Run one semantic intent against the review store. */ @@ -404,48 +409,50 @@ export function useReviewController({ [keyByFileId, runIntent], ); - /** Select one file and optionally one specific hunk within it. */ - const selectFile = useCallback( - (fileId: string, nextHunkIndex = 0, options?: ReviewSelectionOptions) => { - selectHunk(fileId, nextHunkIndex, options); - }, - [selectHunk], - ); - /** - * Keep the selection on a file the review stream still shows, with its hunk in range. + * Adopt a selection the viewport arrived at on its own. * - * Reconciliation carries no reveal request: filters and reloads move the selection on - * the reviewer's behalf, and must not also move the viewport under them. + * 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 reconcileSelection = useCallback(() => { - const selection = store.getSnapshot().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]; - - if (!isVisible && fallback) { - const fallbackKey = keyByFileId.get(fallback.id); - if (fallbackKey) { - store.dispatch({ type: "selection/select", fileKey: fallbackKey, hunkIndex: 0 }); + const anchorSelection = useCallback( + (fileId: string, hunkIndex: number) => { + const fileKey = keyByFileId.get(fileId); + if (!fileKey) { + return; } - return; - } - if (selection.fileKey) { - store.dispatch({ - type: "selection/select", - fileKey: selection.fileKey, - hunkIndex: selection.hunkIndex, - }); + runIntent({ type: "selection/anchor", fileKey, hunkIndex }); + }, + [keyByFileId, runIntent], + ); + + /** Jump to one file through the shared file-jump rule. */ + const selectFile = useCallback( + (fileId: string, options?: ReviewSelectionOptions) => { + const fileKey = keyByFileId.get(fileId); + if (!fileKey) { + return; + } + + runIntent({ type: "selection/select-file", fileKey, reveal: revealRequestFor(options) }); + }, + [keyByFileId, runIntent], + ); + + /** Reconcile only a stale document selection; filtering preserves the reviewer's place. */ + const reconcileSelection = useCallback(() => { + const action = planTerminalSelectionReconciliation(store.getSnapshot()); + if (action) { + store.dispatch(action); } - }, [fileByKey, keyByFileId, store, visibleFiles]); + }, [store]); useEffect(() => { reconcileSelection(); // The store's own state is what needs reconciling, so re-run when it moves. - }, [reconcileSelection, state.document, state.selection]); + }, [reconcileSelection, state.document, state.filter, state.selection]); /** * Keep the current line on a row the review stream still renders. @@ -463,9 +470,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 +531,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 +549,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. */ @@ -779,18 +720,36 @@ export function useReviewController({ } }, [selectedFile, selectedHunkIndex, toggleGap]); - /** Resolve one session-daemon navigation request against the current review state and select it. */ + /** + * 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, @@ -798,7 +757,7 @@ export function useReviewController({ selectedHunk: buildSelectedHunkSummary(target.file, target.hunkIndex), }; }, - [allFiles, selectHunk, selectedFile?.id, selectedHunkIndex, visibleFiles], + [allFiles, fileByKey, moveSelection, selectHunk], ); /** @@ -1018,11 +977,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); }, [ @@ -1188,14 +1147,12 @@ export function useReviewController({ addLiveComment, addLiveCommentBatch, anchorLineCursor, + anchorSelection, clearFilter, cancelDraftNote, clearLiveComments, moveLineCursor, - moveToAnnotatedFile, - moveToAnnotatedHunk, - moveToFile, - moveToHunk, + moveSelection, navigateToLocation, removeLiveComment, removeUserNote, 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/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/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/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. * 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..6448e60bb 100644 --- a/src/ui/lib/reviewState.ts +++ b/src/ui/lib/reviewState.ts @@ -1,23 +1,26 @@ /** * 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 type { ReviewAction } from "../../core/review/actions"; import { reviewHunkRanges } from "../../core/review/geometry"; +import type { ReviewAnnotationIndex } from "../../core/review/navigation"; +import { reviewFileMatchesFilter, selectNormalizedSelection } from "../../core/review/selectors"; +import type { ReviewState } from "../../core/review/state"; 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 +31,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 +45,80 @@ 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] - ); +/** + * Plan the terminal follow-up when a document replacement invalidates its selection. + * + * A filter only changes the visible stream, so a selected file it hides stays selected. + * The shared selector instead supplies a replacement only when the selected file vanished + * or its hunk index became stale. This returns no reveal request: reconciliation must not + * move the reviewer's viewport. + */ +export function planTerminalSelectionReconciliation( + state: ReviewState, +): Extract | undefined { + const normalized = selectNormalizedSelection(state); + const fileKey = normalized.fileKey; + if ( + fileKey === null || + (fileKey === state.selection.fileKey && normalized.hunkIndex === state.selection.hunkIndex) + ) { + return undefined; + } + + return { type: "selection/select", fileKey, hunkIndex: normalized.hunkIndex }; +} + +/** + * 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 +134,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 +170,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 }; } diff --git a/test/review-conformance/conformance.test.ts b/test/review-conformance/conformance.test.ts index e52c76885..f83ceac94 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,32 @@ describe("review conformance corpus", () => { "core review model", "terminal render planning", ]); + expect(REVIEW_NAVIGATION_CONSUMERS.map((consumer) => consumer.name)).toEqual([ + "core intent planner", + "terminal review controller", + ]); }); 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..0eab1857d 100644 --- a/test/review-conformance/consumers.ts +++ b/test/review-conformance/consumers.ts @@ -7,10 +7,24 @@ * own has joined (`docs/browser-review-rebuild.md` § "Per-phase seam verification"). */ import { coreModelConsumer } from "./consumers/coreModel"; +import { intentPlannerNavigationConsumer } from "./consumers/intentPlanner"; +import { terminalReviewControllerNavigationConsumer } from "./consumers/terminalReviewController"; 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, + terminalReviewControllerNavigationConsumer, +]; diff --git a/test/review-conformance/consumers/intentPlanner.ts b/test/review-conformance/consumers/intentPlanner.ts new file mode 100644 index 000000000..63ce925e9 --- /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. */ +export const VANISHED_FILE_KEY = "vanished:no-such-file"; + +/** Resolve a fixture's positional selection into the semantic one core reads. */ +export 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. */ +export 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. */ +export 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/consumers/terminalReviewController.ts b/test/review-conformance/consumers/terminalReviewController.ts new file mode 100644 index 000000000..924d73290 --- /dev/null +++ b/test/review-conformance/consumers/terminalReviewController.ts @@ -0,0 +1,103 @@ +/** + * The terminal review controller as a navigation conformance consumer. + * + * The controller submits relative moves through `applyReviewIntent` and reconciles its + * rendered selection after document/filter updates. This adapter drives those same two + * planning paths without mounting OpenTUI, so the shared corpus catches a terminal + * lifecycle regression as well as a core-planner regression. + */ +import type { ReviewAction } from "../../../src/core/review/actions"; +import { projectReviewDocument } from "../../../src/core/review/document"; +import { applyReviewIntent } from "../../../src/core/review/intents"; +import { reduceReviewState } from "../../../src/core/review/reducer"; +import { selectRevealTarget } from "../../../src/core/review/selectors"; +import { createInitialReviewState, type ReviewState } from "../../../src/core/review/state"; +import type { ReviewStore } from "../../../src/core/review/store"; +import { planTerminalSelectionReconciliation } from "../../../src/ui/lib/reviewState"; +import { toAnnotationIndex, toConformanceSelection, toSemanticSelection } from "./intentPlanner"; +import type { ReviewNavigationConsumer, ReviewNavigationFixture } from "../types"; + +/** Apply the terminal's render-lifecycle reconciliation without requesting a reveal. */ +function reconcileTerminalSelection(state: ReviewState) { + const action = planTerminalSelectionReconciliation(state); + return action ? reduceReviewState(state, action) : state; +} + +/** Build the minimal synchronous store the controller's `runIntent` helper receives. */ +function createTestReviewStore(initial: ReviewState): ReviewStore { + let snapshot = initial; + return { + getSnapshot: () => snapshot, + subscribe: () => () => {}, + dispatch(action: ReviewAction) { + snapshot = reduceReviewState(snapshot, action); + return snapshot; + }, + }; +} + +/** Recover the semantic reveal request from the counters the store published. */ +function revealFromTransition(before: ReviewState, after: ReviewState) { + return { + anchor: + after.reveal.fileTopToken !== before.reveal.fileTopToken + ? ("file-top" as const) + : after.reveal.hunkToken !== before.reveal.hunkToken + ? ("hunk" as const) + : ("none" as const), + scrollToNote: after.reveal.scrollToNote, + }; +} + +export const terminalReviewControllerNavigationConsumer: ReviewNavigationConsumer = { + name: "terminal review controller", + 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: ReviewNavigationFixture["selections"][number]) => + reconcileTerminalSelection({ + ...baseState, + selection: toSemanticSelection(input, document), + }); + + return { + moves: fixture.moves.map((move) => { + // This is the same semantic-intent call `useReviewController` makes for its + // keyboard and session navigation handlers. + const store = createTestReviewStore(stateAt(move.from)); + const before = store.getSnapshot(); + const outcome = applyReviewIntent( + store, + { type: "selection/move", scope: move.scope, delta: move.delta }, + { annotations }, + ); + if (!outcome) { + return { to: null }; + } + const after = store.getSnapshot(); + return { + to: toConformanceSelection(after.selection.fileKey, after.selection.hunkIndex, document), + reveal: revealFromTransition(before, after), + }; + }), + normalizedSelections: fixture.selections.map((input) => { + const selection = stateAt(input).selection; + return toConformanceSelection(selection.fileKey, selection.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; +}