From c8dd414896e29272358db4fb77dcfd3b03a2a6a6 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 16:33:32 +1000 Subject: [PATCH 01/25] chore(porch): 1060 init pir --- .../status.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 codev/projects/1060-vscode-next-file-previous-file/status.yaml diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml new file mode 100644 index 000000000..622bd3b2d --- /dev/null +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -0,0 +1,18 @@ +id: '1060' +title: vscode-next-file-previous-file +protocol: pir +phase: plan +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: pending + dev-approval: + status: pending + pr: + status: pending +iteration: 1 +build_complete: false +history: [] +started_at: '2026-06-17T06:33:31.988Z' +updated_at: '2026-06-17T06:33:31.989Z' From 9ef2ee2e63532e89d8d9b11a094951e45958a39a Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 16:41:43 +1000 Subject: [PATCH 02/25] [PIR #1060] Plan draft --- .../1060-vscode-next-file-previous-file.md | 255 ++++++++++++++++++ codev/state/pir-1060_thread.md | 17 ++ 2 files changed, 272 insertions(+) create mode 100644 codev/plans/1060-vscode-next-file-previous-file.md create mode 100644 codev/state/pir-1060_thread.md diff --git a/codev/plans/1060-vscode-next-file-previous-file.md b/codev/plans/1060-vscode-next-file-previous-file.md new file mode 100644 index 000000000..733d8a21b --- /dev/null +++ b/codev/plans/1060-vscode-next-file-previous-file.md @@ -0,0 +1,255 @@ +# PIR Plan: Cross-file navigation in a Codev View Diff session + +## Understanding + +`codev.viewDiff` (`packages/vscode/src/commands/view-diff.ts`) opens a builder +worktree's full delta as a single multi-file diff editor with a file-list pane +on the left. VSCode handles **within-file** hunk navigation natively (F7 / +Shift+F7 → `editor.action.diffReview.next/prev`), but there is no **cross-file** +"jump to next/previous file" gesture — the reviewer must click in the file list. +GitHub's PR review UI has `j` / `k` for exactly this; #1060 asks for the +equivalent on the Codev View Diff surface. + +Two new commands, scoped to the active Codev diff session: + +- `codev.diffNextFile` — reveal the next file in the file-list order. +- `codev.diffPreviousFile` — reveal the previous file. + +Both palette-discoverable, keyboard-bindable, and operating relative to the file +the diff editor is currently showing — without requiring the file-list pane to +be focused or visible. + +### What the codebase + VSCode actually expose (verified, not assumed) + +I disassembled the VSCode 1.105 workbench bundle and read the relevant +extension source to pin down the mechanism before committing to an approach: + +1. **Today's open path can't be re-addressed.** `viewDiff` opens the editor via + the public `vscode.changes` command, which delegates to `_workbench.changes` + and creates the multi-diff input with a **non-deterministic** source URI + (`multi-diff-editor:${new Date().getMilliseconds()+Math.random()}`). There is + no way to compute that URI afterward, so I cannot target that editor to + programmatically scroll/reveal a file. + +2. **No built-in file-granular navigation exists.** The bundle registers + `multiDiffEditor.goToNextChange` / `goToPreviousChange`, but they are + **hunk**-granular (they call `pane.goToNextChange()` and walk change-by-change, + crossing file boundaries only incidentally). `multiDiffEditor.goToFile` opens + the *currently focused* file as a standalone editor — not a walk. Neither is a + "next file" primitive. + +3. **The reveal primitive lives on an internal command.** `_workbench.openMultiDiffEditor` + accepts: + ```ts + { + multiDiffSourceUri?: UriComponents, + resources?: { originalUri: UriComponents, modifiedUri: UriComponents }[], + title?: string, + reveal?: { modifiedUri: UriComponents, range?: IRange }, + } + ``` + When `reveal.modifiedUri` matches one of `resources`' `modifiedUri`s, the + editor scrolls to that file (`viewState.revealData`). Reveal resolution + **requires** `resources` to be passed on the call (it searches that array). + +4. **Editor identity is the source URI.** The multi-diff input is keyed by its + `multiDiffSource` URI. Re-invoking `_workbench.openMultiDiffEditor` with the + **same** source URI reuses the existing editor and just applies the reveal — + in place, no duplicate tab, and focus stays on the diff editor (strictly + better than clicking the file list, which can shift focus). + +The consequence: to reveal files programmatically I must own a **deterministic** +source URI. That means migrating `viewDiff`'s open call from `vscode.changes` to +`_workbench.openMultiDiffEditor` with an explicit per-builder source URI, and +driving navigation through the same command. + +## Proposed Change + +### 1. Make the diff editor addressable (open path) + +In `view-diff.ts`, replace the `vscode.changes` call with `_workbench.openMultiDiffEditor`, +passing a deterministic source URI per builder: + +```ts +const sourceUri = vscode.Uri.from({ scheme: 'codev-multidiff', path: `/${builder.id}` }); +await vscode.commands.executeCommand('_workbench.openMultiDiffEditor', { + multiDiffSourceUri: sourceUri, + title: `Reviewing #${builder.issueId ?? builder.id} (${defaultBranch} ↔ HEAD)`, + resources: plans.map(plan => { + const { left, right } = diffUrisForChange(plan, { wt, ref: baseRef }); + return { originalUri: left, modifiedUri: right }; + }), +}); +``` + +- The resources are built from the **same** `planResources` / `diffUrisForChange` + seam used today — left = base blob (or empty/binary placeholder), right = + on-disk worktree file (or placeholder). Only the *shape* changes (triples → + `{originalUri, modifiedUri}`) and the source URI becomes explicit. +- A dedicated scheme `codev-multidiff` (distinct from the `codev-diff` content + provider scheme) is used purely as an identity key. Because `resources` is + passed inline, no `IMultiDiffSourceResolver` is needed for the scheme. +- The CodeLens "Forward to Builder" session registration (`setDiffInjectSession` + with the right-side `file:` fsPaths + hunks) is **unchanged** — it keys off the + modified-side fsPaths, which the migration preserves. + +### 2. Track the navigable session + +Add a small module-level nav-session store in `view-diff.ts` (or a sibling +`diff-nav.ts`), populated by `viewDiff` right after it opens the editor: + +```ts +interface DiffNavFile { originalUri: vscode.Uri; modifiedUri: vscode.Uri; fsPath: string; } +interface DiffNavSession { builderId: string; sourceUri: vscode.Uri; title: string; files: DiffNavFile[]; index: number; } +``` + +- Keyed by `builderId` in a `Map`, so **multiple** builders' diff sessions + coexist with independent file lists + pointers (acceptance: multi-builder + isolation). Re-running View Diff for the same builder replaces that builder's + session. +- `files` order == `plans` order == `git diff --name-status` order == the + file-list pane order (acceptance: navigation order = visual order). +- `index` starts at 0. + +### 3. The two navigation commands + +`codev.diffNextFile` / `codev.diffPreviousFile` both call a shared `navigateDiff(direction)`: + +1. **Resolve the active session.** Find the session whose `files` contains + `vscode.window.activeTextEditor?.document.uri.fsPath`. If none matches (e.g. + the user hasn't clicked into a sub-editor yet), fall back to the most-recently + opened session. If there are no sessions at all → status-bar message + ("No Codev diff session active") and return. +2. **Reconcile the pointer.** If the active editor's fsPath is in the session, + set `index` to that file's index (keeps navigation correct after the user + clicks a file in the list). +3. **Compute the target** via a pure helper: `target = index + direction`. If out + of `[0, files.length)` → status-bar message ("Last file in diff session" / + "First file in diff session"), **no wrap**, return (acceptance: edge behavior). +4. **Reveal** by re-invoking `_workbench.openMultiDiffEditor` with the session's + `sourceUri` + `files` (as resources) + `reveal: { modifiedUri: files[target].modifiedUri }`. + Same source URI ⇒ in-place reveal. +5. Update `session.index = target`. + +Because reveal works on the open editor regardless of file-list pane visibility, +the commands work with the pane collapsed/hidden (acceptance). + +### 4. Contributions + +- `package.json` → `contributes.commands`: two entries, + `Codev: Go to Next File in Diff` / `Codev: Go to Previous File in Diff`. +- **No default keybindings** (plan-gate decision #1, see below) — palette-only. +- No `when`-clause hiding from the palette (acceptance: palette-discoverable); + they no-op with a status message when no session is active. + +### 5. Pure helpers (the unit-test surface) + +Exported, no vscode/git dependency: + +- `diffFileOrder(plans: ResourcePlan[]): string[]` — modified fsPaths in list + order (asserts ordering == file-list order). +- `computeNavTarget(index: number, count: number, direction: 1 | -1): { index: number; atEdge: boolean }` + — clamp + edge detection (asserts next-at-end / prev-at-start no-op). +- `indexOfFsPath(files: { fsPath: string }[], fsPath: string | undefined): number` + — pointer reconciliation, and the basis for the multi-builder-isolation test + (two independent session objects, each resolves its own index). + +## Files to Change + +- `packages/vscode/src/commands/view-diff.ts` + - Swap `vscode.changes` → `_workbench.openMultiDiffEditor` with explicit + `codev-multidiff:/` source URI (~line 378-382). + - Add the nav-session store + `navigateDiff(direction)` + the three pure + helpers. (Could live in a new `packages/vscode/src/commands/diff-nav.ts` if + `view-diff.ts` gets crowded — decided at implementation time; leaning to keep + it in `view-diff.ts` since it shares the resource-building seam.) +- `packages/vscode/src/extension.ts` + - Register `codev.diffNextFile` / `codev.diffPreviousFile` via `reg(...)` + (CLI-independent; they operate on editor state, not Tower) near the existing + `codev.viewDiff` registration (~line 838). +- `packages/vscode/package.json` + - Two `contributes.commands` entries. No `keybindings` entries. +- `packages/vscode/src/__tests__/diff-nav.test.ts` (new) + - Unit tests for the three pure helpers (ordering, edge no-op, isolation). +- `packages/vscode/src/__tests__/contributes-commands.test.ts` + - Extend to assert the two new commands are declared (mirrors existing pattern). +- `packages/vscode/CHANGELOG.md` + `docs/releases/UNRELEASED.md` + - Per-PR changelog accumulation (this is a vscode-relevant change). + +No `codev-skeleton/` mirror: the VSCode extension is a published package, not a +skeleton-mirrored framework file, so the dual-tree rule does not apply here. + +## Plan-Gate Decisions + +The issue locks five design calls at plan-approval. My recommendations: + +1. **Default keybindings** → **None (palette-only) + documentation.** Smallest + blast radius; no risk of clobbering a reviewer's existing bindings. Heavy + users bind `j`/`k` (or Alt+J/K) themselves via `keybindings.json`. *(issue's + stated lean)* +2. **Edge behavior** → **status-bar message + no wrap.** Wrapping invites + accidental loops; silent no-op feels broken. *(issue's stated lean)* +3. **Scope resolution** → **per-diff-session (Codev View Diff only) for v1.** + Generic any-diff-editor mode is a follow-up. *(issue's stated lean)* +4. **File-list pane collapsed/hidden** → **commands still work.** Reveal targets + the editor, not the pane; pane visibility is a display preference. *(issue's + stated lean)* +5. **Restore last-viewed file across re-opens** → **out of scope; always open at + first file.** *(issue's stated lean)* + +I concur with all five leans; calling them out explicitly so the gate can +confirm rather than infer. + +## Risks & Alternatives Considered + +- **Risk: `_workbench.openMultiDiffEditor` is an internal (underscore) command.** + It's undocumented and could change across VSCode versions. Mitigations: (a) + it's the same family the public `vscode.changes` already delegates into + (`_workbench.changes`), and is widely used by VSCode's own SCM/timeline UI, so + it's de-facto stable; (b) the migration is the *only* way to get programmatic + file reveal — the public surface has no equivalent; (c) wrap the call so a + throw degrades gracefully (navigation no-ops with a status message rather than + breaking View Diff). I'll pin the engine expectation in a code comment. +- **Risk: migrating the working `viewDiff` open path (regression surface).** + `vscode.changes` takes `[resource, original, modified]` triples and uses the + first (a `file:` URI) for the file-list label; `_workbench.openMultiDiffEditor` + takes `{originalUri, modifiedUri}` and derives the label from the modified + side. For **deleted** files the modified side is a `codev-diff:` empty + placeholder, so the list entry's *icon* may render generically instead of by + file type (the path/label is still correct). This is cosmetic; I'll verify it + at the dev-approval gate and, if it regresses noticeably, evaluate a + file-typed placeholder URI. All other statuses (A/M/R/C) keep a `file:` + modified URI → unchanged. +- **Alternative: keep `vscode.changes`, build file-nav on `goToNextChange`.** + Rejected — it's hunk-granular; emulating file-granular by calling it in a loop + until the active file changes overshoots and is fragile. +- **Alternative: per-file `vscode.diff` instead of the multi-diff editor.** + Rejected — changes the established View Diff UX (single multi-file editor with + a file list) the issue explicitly builds on. +- **Alternative: store the source URI from the existing `vscode.changes` open.** + Impossible — that URI is randomly synthesized and not exposed. + +## Test Plan + +### Unit (`diff-nav.test.ts`, vitest) +- `diffFileOrder` returns modified fsPaths in plans order (ordering == + file-list order). +- `computeNavTarget`: mid-list advances/retreats by one; at last index + forward + → `atEdge: true`, index unchanged; at index 0 + backward → `atEdge: true`. +- `indexOfFsPath`: resolves a file's index; returns -1 for an unknown path; two + independent session objects each resolve against their own `files` (multi- + builder isolation). +- `contributes-commands.test.ts`: both new command ids are declared with titles. + +### Manual (dev-approval gate — the load-bearing verification) +- Spawn/identify two builders with non-trivial multi-file diffs. +- `codev.viewDiff` on builder A; run `codev.diffNextFile` repeatedly → editor + walks A's files in list order; at the last file → status-bar "Last file…", + no wrap. `codev.diffPreviousFile` walks back; at first → "First file…". +- Confirm focus stays in the diff editor (not stolen by the file list) and that + it works with the file-list pane collapsed. +- Confirm within-file F7 / Shift+F7 hunk navigation still works unchanged. +- Open builder B's View Diff; navigate → B's own file list/pointer, independent + of A (isolation). Switch back to A's tab, navigate → resumes A's pointer. +- Sanity-check the existing View Diff still renders correctly post-migration + (added/modified/renamed/deleted/binary files, correct labels/icons). diff --git a/codev/state/pir-1060_thread.md b/codev/state/pir-1060_thread.md new file mode 100644 index 000000000..f60d5483f --- /dev/null +++ b/codev/state/pir-1060_thread.md @@ -0,0 +1,17 @@ +# PIR #1060 — vscode cross-file diff navigation + +## Plan phase + +**Goal**: add `codev.diffNextFile` / `codev.diffPreviousFile` to walk files in a Codev View Diff session. + +### Key research findings (from the VSCode 1.105 bundle + codebase) + +- `codev.viewDiff` (`packages/vscode/src/commands/view-diff.ts`) opens the multi-file diff via the **public** `vscode.changes` command → `_workbench.changes`. That path creates the multi-diff editor with a **non-deterministic** source URI (`multi-diff-editor:${ms+Math.random()}`), so I can't address that editor later to reveal a file. +- VSCode has **no** built-in file-granular navigation. `multiDiffEditor.goToNextChange/goToPreviousChange` are **hunk**-granular (call `pane.goToNextChange()`); `multiDiffEditor.goToFile` opens the focused file as a standalone editor. None walk file-to-file. +- The internal command `_workbench.openMultiDiffEditor({ multiDiffSourceUri, resources:[{originalUri,modifiedUri}], title, reveal:{modifiedUri,range} })` **does** support revealing a specific file (`viewState.revealData`). Reveal only resolves when `resources` is passed (it searches the resources array for the matching `modifiedUri`). +- Editor identity is keyed by `multiDiffSource` URI → re-invoking with the **same** source URI reveals **in place** (no duplicate tab, focus stays on the diff editor). + +### Design decision +Migrate `viewDiff`'s open call from `vscode.changes` to `_workbench.openMultiDiffEditor` with an explicit, deterministic per-builder source URI (`codev-multidiff:/`). Navigation commands re-invoke the same command with the same source URI + resources + `reveal.modifiedUri`. A module-level nav-session store (keyed by builder id) holds the ordered resources + current index; active session resolved from `activeTextEditor`'s fsPath (falls back to most-recent). Pure helpers (ordering, edge-clamp, index-by-fsPath) carry the unit tests. + +Plan written to `codev/plans/1060-vscode-next-file-previous-file.md`. Awaiting `plan-approval`. From 07cfcd33990d8850e2edec75761127b180e57b34 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 16:41:51 +1000 Subject: [PATCH 03/25] chore(porch): 1060 plan-approval gate-requested --- codev/projects/1060-vscode-next-file-previous-file/status.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index 622bd3b2d..90804c5c3 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -7,6 +7,7 @@ current_plan_phase: null gates: plan-approval: status: pending + requested_at: '2026-06-17T06:41:51.549Z' dev-approval: status: pending pr: @@ -15,4 +16,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T06:33:31.989Z' +updated_at: '2026-06-17T06:41:51.550Z' From 359cea65809bb4a1129e5b1216d7ad9d94e29b61 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 16:52:58 +1000 Subject: [PATCH 04/25] [PIR #1060] Plan revised: reuse sidebar changed-file list, drop multi-diff internals --- .../1060-vscode-next-file-previous-file.md | 414 ++++++++---------- codev/state/pir-1060_thread.md | 18 + 2 files changed, 204 insertions(+), 228 deletions(-) diff --git a/codev/plans/1060-vscode-next-file-previous-file.md b/codev/plans/1060-vscode-next-file-previous-file.md index 733d8a21b..fe5440906 100644 --- a/codev/plans/1060-vscode-next-file-previous-file.md +++ b/codev/plans/1060-vscode-next-file-previous-file.md @@ -2,254 +2,212 @@ ## Understanding -`codev.viewDiff` (`packages/vscode/src/commands/view-diff.ts`) opens a builder -worktree's full delta as a single multi-file diff editor with a file-list pane -on the left. VSCode handles **within-file** hunk navigation natively (F7 / -Shift+F7 → `editor.action.diffReview.next/prev`), but there is no **cross-file** -"jump to next/previous file" gesture — the reviewer must click in the file list. -GitHub's PR review UI has `j` / `k` for exactly this; #1060 asks for the -equivalent on the Codev View Diff surface. - -Two new commands, scoped to the active Codev diff session: - -- `codev.diffNextFile` — reveal the next file in the file-list order. -- `codev.diffPreviousFile` — reveal the previous file. - -Both palette-discoverable, keyboard-bindable, and operating relative to the file -the diff editor is currently showing — without requiring the file-list pane to -be focused or visible. - -### What the codebase + VSCode actually expose (verified, not assumed) - -I disassembled the VSCode 1.105 workbench bundle and read the relevant -extension source to pin down the mechanism before committing to an approach: - -1. **Today's open path can't be re-addressed.** `viewDiff` opens the editor via - the public `vscode.changes` command, which delegates to `_workbench.changes` - and creates the multi-diff input with a **non-deterministic** source URI - (`multi-diff-editor:${new Date().getMilliseconds()+Math.random()}`). There is - no way to compute that URI afterward, so I cannot target that editor to - programmatically scroll/reveal a file. - -2. **No built-in file-granular navigation exists.** The bundle registers - `multiDiffEditor.goToNextChange` / `goToPreviousChange`, but they are - **hunk**-granular (they call `pane.goToNextChange()` and walk change-by-change, - crossing file boundaries only incidentally). `multiDiffEditor.goToFile` opens - the *currently focused* file as a standalone editor — not a walk. Neither is a - "next file" primitive. - -3. **The reveal primitive lives on an internal command.** `_workbench.openMultiDiffEditor` - accepts: - ```ts - { - multiDiffSourceUri?: UriComponents, - resources?: { originalUri: UriComponents, modifiedUri: UriComponents }[], - title?: string, - reveal?: { modifiedUri: UriComponents, range?: IRange }, - } - ``` - When `reveal.modifiedUri` matches one of `resources`' `modifiedUri`s, the - editor scrolls to that file (`viewState.revealData`). Reveal resolution - **requires** `resources` to be passed on the call (it searches that array). - -4. **Editor identity is the source URI.** The multi-diff input is keyed by its - `multiDiffSource` URI. Re-invoking `_workbench.openMultiDiffEditor` with the - **same** source URI reuses the existing editor and just applies the reveal — - in place, no duplicate tab, and focus stays on the diff editor (strictly - better than clicking the file list, which can shift focus). - -The consequence: to reveal files programmatically I must own a **deterministic** -source URI. That means migrating `viewDiff`'s open call from `vscode.changes` to -`_workbench.openMultiDiffEditor` with an explicit per-builder source URI, and -driving navigation through the same command. +`codev.viewDiff` opens a builder worktree's full delta with a file-list pane; +clicking a file row (`codev.openBuilderFileDiff`) opens that file's per-file +diff. VSCode handles **within-file** hunk navigation (F7 / Shift+F7), but there +is no **cross-file** "next/previous file" gesture — the reviewer must click in +the file list. #1060 asks for the keyboard equivalent (GitHub PR review's +`j`/`k`). + +Two new commands: + +- `codev.diffNextFile` — open the next file's diff. +- `codev.diffPreviousFile` — open the previous file's diff. + +### Approach (per architect direction): reuse the builder's changed-file list + +The architect pointed out the clean path: **the Builders sidebar already builds +an ordered list of a builder's changed files** (`BuilderDiffCache.getDiff` → +`getBuilderChanges` → `planResources`, in `builder-diff-cache.ts` / +`view-diff.ts`). Navigation just walks that list top-to-bottom, opening each +file's per-file diff via the **existing** `codev.openBuilderFileDiff` open logic. + +This needs **no** VSCode multi-diff editor internals and **no** changes to how +`viewDiff` opens — it's built entirely on already-shipped, public mechanisms: + +1. **The ordered list** comes from `BuilderDiffCache.getDiff(builderId, worktreePath)`, + which returns `{ baseRef, files: BuilderFileChange[] }`. `files` is in + `git diff --name-status` order — exactly the flat-list sidebar order and the + View Diff editor's file-list order. The cache has a 15s TTL, so navigation + keypresses don't spawn a `git` process each. +2. **The current position** is resolved from the active editor: the diff-inject + registry (`getDiffInjectEntry(fsPath)`, populated by both `viewDiff` and + `openBuilderFileDiff`) maps the active editor's right-side worktree fsPath → + `{ builderId, relPath }`. So "where am I" = the file the diff editor is + currently showing; no stored pointer to drift out of sync. +3. **Opening the next file** reuses the per-file open path (`diffUrisForChange` + → `vscode.diff` → `registerFileInjectSession`), i.e. the same thing clicking + a sidebar file row does today. + +The opened per-file diff becomes the active editor, so the *next* keypress +resolves the new current file and steps again — a clean walk. + +> I initially scoped this around revealing files inside the `vscode.changes` +> multi-file editor, which would have required migrating `viewDiff` to the +> internal `_workbench.openMultiDiffEditor` command (the only thing that exposes +> a programmatic file reveal). The architect's file-list approach is strictly +> better: no internal/undocumented command, no regression surface on the working +> View Diff open path. That earlier approach is dropped. ## Proposed Change -### 1. Make the diff editor addressable (open path) +### 1. Extract a reusable per-file open helper -In `view-diff.ts`, replace the `vscode.changes` call with `_workbench.openMultiDiffEditor`, -passing a deterministic source URI per builder: +Today the `codev.openBuilderFileDiff` handler (extension.ts ~877) inlines: +`diffUrisForChange` → `vscode.diff` → `registerFileInjectSession` → +`ensureDiffEditorCodeLens`. Extract that into a single exported function so the +command **and** the navigation commands share one code path: ```ts -const sourceUri = vscode.Uri.from({ scheme: 'codev-multidiff', path: `/${builder.id}` }); -await vscode.commands.executeCommand('_workbench.openMultiDiffEditor', { - multiDiffSourceUri: sourceUri, - title: `Reviewing #${builder.issueId ?? builder.id} (${defaultBranch} ↔ HEAD)`, - resources: plans.map(plan => { - const { left, right } = diffUrisForChange(plan, { wt, ref: baseRef }); - return { originalUri: left, modifiedUri: right }; - }), -}); +// view-diff.ts (or a sibling) +export async function openBuilderFileDiff(args: { + worktreePath: string; baseRef: string; builderId: string; plan: ResourcePlan; +}): Promise { /* the existing handler body */ } ``` -- The resources are built from the **same** `planResources` / `diffUrisForChange` - seam used today — left = base blob (or empty/binary placeholder), right = - on-disk worktree file (or placeholder). Only the *shape* changes (triples → - `{originalUri, modifiedUri}`) and the source URI becomes explicit. -- A dedicated scheme `codev-multidiff` (distinct from the `codev-diff` content - provider scheme) is used purely as an identity key. Because `resources` is - passed inline, no `IMultiDiffSourceResolver` is needed for the scheme. -- The CodeLens "Forward to Builder" session registration (`setDiffInjectSession` - with the right-side `file:` fsPaths + hunks) is **unchanged** — it keys off the - modified-side fsPaths, which the migration preserves. - -### 2. Track the navigable session - -Add a small module-level nav-session store in `view-diff.ts` (or a sibling -`diff-nav.ts`), populated by `viewDiff` right after it opens the editor: - -```ts -interface DiffNavFile { originalUri: vscode.Uri; modifiedUri: vscode.Uri; fsPath: string; } -interface DiffNavSession { builderId: string; sourceUri: vscode.Uri; title: string; files: DiffNavFile[]; index: number; } -``` - -- Keyed by `builderId` in a `Map`, so **multiple** builders' diff sessions - coexist with independent file lists + pointers (acceptance: multi-builder - isolation). Re-running View Diff for the same builder replaces that builder's - session. -- `files` order == `plans` order == `git diff --name-status` order == the - file-list pane order (acceptance: navigation order = visual order). -- `index` starts at 0. - -### 3. The two navigation commands - -`codev.diffNextFile` / `codev.diffPreviousFile` both call a shared `navigateDiff(direction)`: - -1. **Resolve the active session.** Find the session whose `files` contains - `vscode.window.activeTextEditor?.document.uri.fsPath`. If none matches (e.g. - the user hasn't clicked into a sub-editor yet), fall back to the most-recently - opened session. If there are no sessions at all → status-bar message - ("No Codev diff session active") and return. -2. **Reconcile the pointer.** If the active editor's fsPath is in the session, - set `index` to that file's index (keeps navigation correct after the user - clicks a file in the list). -3. **Compute the target** via a pure helper: `target = index + direction`. If out - of `[0, files.length)` → status-bar message ("Last file in diff session" / - "First file in diff session"), **no wrap**, return (acceptance: edge behavior). -4. **Reveal** by re-invoking `_workbench.openMultiDiffEditor` with the session's - `sourceUri` + `files` (as resources) + `reveal: { modifiedUri: files[target].modifiedUri }`. - Same source URI ⇒ in-place reveal. -5. Update `session.index = target`. - -Because reveal works on the open editor regardless of file-list pane visibility, -the commands work with the pane collapsed/hidden (acceptance). - -### 4. Contributions - -- `package.json` → `contributes.commands`: two entries, - `Codev: Go to Next File in Diff` / `Codev: Go to Previous File in Diff`. -- **No default keybindings** (plan-gate decision #1, see below) — palette-only. -- No `when`-clause hiding from the palette (acceptance: palette-discoverable); - they no-op with a status message when no session is active. - -### 5. Pure helpers (the unit-test surface) - -Exported, no vscode/git dependency: - -- `diffFileOrder(plans: ResourcePlan[]): string[]` — modified fsPaths in list - order (asserts ordering == file-list order). -- `computeNavTarget(index: number, count: number, direction: 1 | -1): { index: number; atEdge: boolean }` - — clamp + edge detection (asserts next-at-end / prev-at-start no-op). -- `indexOfFsPath(files: { fsPath: string }[], fsPath: string | undefined): number` - — pointer reconciliation, and the basis for the multi-builder-isolation test - (two independent session objects, each resolves its own index). +The existing handler becomes a thin caller (it already receives a +`BuilderFileTreeItem` and narrows via `instanceof`). No behavior change. + +### 2. The navigation module (`commands/diff-nav.ts`, new) + +`navigateDiff(direction: 1 | -1, deps)` where deps give access to the overview +(builderId → worktreePath) and the shared `BuilderDiffCache`: + +1. **Resolve current file + builder.** Read + `vscode.window.activeTextEditor?.document.uri.fsPath`; look it up via + `getDiffInjectEntry` → `{ builderId, relPath }`. If unresolved, fall back to a + module-level "last navigated position" (`{ builderId, relPath }`, updated on + every successful navigation/open). If still unresolved → status-bar message + ("Open a Codev file diff first") and return. +2. **Load the builder's ordered list.** Look up `worktreePath` from the overview + by `builderId` (as `viewDiff` does); `cache.getDiff(builderId, worktreePath)` + → `{ baseRef, files }`. +3. **Find current index** = `indexOfRelPath(files, relPath)`. (-1 → treat as + "not in this list"; status message + return.) +4. **Compute target** via pure `computeNavTarget(index, files.length, direction)`. + If `atEdge` → status-bar message ("Last file in diff" / "First file in diff"), + **no wrap**, return. +5. **Open** the target via the extracted `openBuilderFileDiff` helper + (`files[target].plan`, `worktreePath`, `baseRef`, `builderId`). Update the + "last navigated position". + +Two commands, both thin wrappers: `codev.diffNextFile` → `navigateDiff(1)`, +`codev.diffPreviousFile` → `navigateDiff(-1)`. + +### 3. Wiring (`extension.ts`) + +- The shared `builderDiffCache` already exists (extension.ts:359). Pass it + + `connectionManager` into the two new `reg(...)` registrations (CLI-independent; + they act on editor state, not Tower) near `codev.viewDiff` (~838). + +### 4. Contributions (`package.json`) + +- `contributes.commands`: `Codev: Go to Next File in Diff` / + `Codev: Go to Previous File in Diff`. +- **No default keybindings** (decision #1) — palette-only. +- No palette `when`-hiding (acceptance: palette-discoverable); they no-op with a + status message when there's no active diff. + +### 5. Pure helpers (unit-test surface) in `diff-nav.ts` + +- `orderedRelPaths(files: BuilderFileChange[]): string[]` — the navigation order + (asserts it equals `files` / git order, matching the sidebar flat list). +- `computeNavTarget(index, count, direction): { index: number; atEdge: boolean }` + — clamp + edge detection (next-at-end / prev-at-start no-op). +- `indexOfRelPath(files, relPath): number` — current-file resolution; two + independent file lists drive the multi-builder-isolation test. ## Files to Change -- `packages/vscode/src/commands/view-diff.ts` - - Swap `vscode.changes` → `_workbench.openMultiDiffEditor` with explicit - `codev-multidiff:/` source URI (~line 378-382). - - Add the nav-session store + `navigateDiff(direction)` + the three pure - helpers. (Could live in a new `packages/vscode/src/commands/diff-nav.ts` if - `view-diff.ts` gets crowded — decided at implementation time; leaning to keep - it in `view-diff.ts` since it shares the resource-building seam.) -- `packages/vscode/src/extension.ts` - - Register `codev.diffNextFile` / `codev.diffPreviousFile` via `reg(...)` - (CLI-independent; they operate on editor state, not Tower) near the existing - `codev.viewDiff` registration (~line 838). -- `packages/vscode/package.json` - - Two `contributes.commands` entries. No `keybindings` entries. -- `packages/vscode/src/__tests__/diff-nav.test.ts` (new) - - Unit tests for the three pure helpers (ordering, edge no-op, isolation). -- `packages/vscode/src/__tests__/contributes-commands.test.ts` - - Extend to assert the two new commands are declared (mirrors existing pattern). -- `packages/vscode/CHANGELOG.md` + `docs/releases/UNRELEASED.md` - - Per-PR changelog accumulation (this is a vscode-relevant change). +- `packages/vscode/src/commands/view-diff.ts` — extract `openBuilderFileDiff(args)` + helper from the extension.ts handler body (move the `vscode.diff` + + `registerFileInjectSession` + `ensureDiffEditorCodeLens` sequence). Re-export + what's needed. +- `packages/vscode/src/commands/diff-nav.ts` (new) — `navigateDiff` + the three + pure helpers. +- `packages/vscode/src/extension.ts` — register `codev.diffNextFile` / + `codev.diffPreviousFile`; refactor the `codev.openBuilderFileDiff` handler to + call the extracted helper. +- `packages/vscode/package.json` — two `contributes.commands` entries. No + keybindings. +- `packages/vscode/src/__tests__/diff-nav.test.ts` (new) — unit tests for the + three pure helpers (ordering, edge no-op, multi-builder isolation). +- `packages/vscode/src/__tests__/contributes-commands.test.ts` — assert the two + new commands are declared (mirrors existing pattern). +- `packages/vscode/CHANGELOG.md` + `docs/releases/UNRELEASED.md` — per-PR + changelog accumulation (vscode-relevant change). No `codev-skeleton/` mirror: the VSCode extension is a published package, not a -skeleton-mirrored framework file, so the dual-tree rule does not apply here. +skeleton-mirrored framework file. ## Plan-Gate Decisions -The issue locks five design calls at plan-approval. My recommendations: - -1. **Default keybindings** → **None (palette-only) + documentation.** Smallest - blast radius; no risk of clobbering a reviewer's existing bindings. Heavy - users bind `j`/`k` (or Alt+J/K) themselves via `keybindings.json`. *(issue's - stated lean)* -2. **Edge behavior** → **status-bar message + no wrap.** Wrapping invites - accidental loops; silent no-op feels broken. *(issue's stated lean)* -3. **Scope resolution** → **per-diff-session (Codev View Diff only) for v1.** - Generic any-diff-editor mode is a follow-up. *(issue's stated lean)* -4. **File-list pane collapsed/hidden** → **commands still work.** Reveal targets - the editor, not the pane; pane visibility is a display preference. *(issue's - stated lean)* -5. **Restore last-viewed file across re-opens** → **out of scope; always open at - first file.** *(issue's stated lean)* - -I concur with all five leans; calling them out explicitly so the gate can -confirm rather than infer. +1. **Default keybindings** → **None (palette-only) + docs.** Smallest blast + radius; no clobbering existing bindings. Heavy users bind `j`/`k` themselves. + *(issue's lean)* +2. **Edge behavior** → **status-bar message + no wrap.** *(issue's lean)* +3. **Scope** → **per-builder Codev diff list (v1).** Generic any-diff-editor is a + follow-up. *(issue's lean)* +4. **File-list pane collapsed/hidden** → **works regardless.** Navigation reads + the cache + active editor, not any pane. *(issue's lean)* +5. **Restore last-viewed file across re-opens** → **out of scope.** Current file + is always the active editor. *(issue's lean)* +6. **(New) Navigation order** → **canonical `getBuilderChanges` order (git + `--name-status`).** This matches the View Diff editor's file list and the + sidebar's **flat (list) mode**. Note: the sidebar's **file-tree mode** renders + folders-first / alphabetical (`buildFilePathTree`), so its *visual* order + differs from navigation order in that mode. Recommend git order for v1 + (deterministic, mode-independent, matches the issue's "file-list pane" + acceptance). Flag for the gate: if you want navigation to mirror the + file-tree-mode visual order exactly, say so and I'll flatten `buildFilePathTree` + DFS instead. +7. **(New) What surface navigation opens** → **per-file `vscode.diff`** (the + `openBuilderFileDiff` surface), one file at a time — the GitHub-PR-review + model. Invoking next/prev while the multi-file View Diff editor is focused + drills into the next file as a per-file diff. I'll confirm this feels right at + the dev-approval gate. ## Risks & Alternatives Considered -- **Risk: `_workbench.openMultiDiffEditor` is an internal (underscore) command.** - It's undocumented and could change across VSCode versions. Mitigations: (a) - it's the same family the public `vscode.changes` already delegates into - (`_workbench.changes`), and is widely used by VSCode's own SCM/timeline UI, so - it's de-facto stable; (b) the migration is the *only* way to get programmatic - file reveal — the public surface has no equivalent; (c) wrap the call so a - throw degrades gracefully (navigation no-ops with a status message rather than - breaking View Diff). I'll pin the engine expectation in a code comment. -- **Risk: migrating the working `viewDiff` open path (regression surface).** - `vscode.changes` takes `[resource, original, modified]` triples and uses the - first (a `file:` URI) for the file-list label; `_workbench.openMultiDiffEditor` - takes `{originalUri, modifiedUri}` and derives the label from the modified - side. For **deleted** files the modified side is a `codev-diff:` empty - placeholder, so the list entry's *icon* may render generically instead of by - file type (the path/label is still correct). This is cosmetic; I'll verify it - at the dev-approval gate and, if it regresses noticeably, evaluate a - file-typed placeholder URI. All other statuses (A/M/R/C) keep a `file:` - modified URI → unchanged. -- **Alternative: keep `vscode.changes`, build file-nav on `goToNextChange`.** - Rejected — it's hunk-granular; emulating file-granular by calling it in a loop - until the active file changes overshoots and is fragile. -- **Alternative: per-file `vscode.diff` instead of the multi-diff editor.** - Rejected — changes the established View Diff UX (single multi-file editor with - a file list) the issue explicitly builds on. -- **Alternative: store the source URI from the existing `vscode.changes` open.** - Impossible — that URI is randomly synthesized and not exposed. +- **Risk: resolving "current file" from the active editor.** If the user focuses + a non-diff editor and hits the key, there's no current file. Mitigation: a + module-level "last navigated position" fallback; failing that, a clear + status-bar message rather than a silent no-op. +- **Risk: stale cache.** `BuilderDiffCache` has a 15s TTL; a file added/removed + in the last 15s might not be in the list yet. Acceptable — it's the same list + the sidebar shows, and the reviewer is reviewing a (mostly settled) branch. A + refresh (collapse/expand the builder, or the 60s poll) reconciles it. +- **Risk: opening per-file diffs accumulates tabs.** `vscode.diff` reuses the + active diff tab when opening a new diff in place (preview-tab behavior), so a + walk doesn't pile up N tabs. I'll verify at dev-approval. +- **Alternative: scroll/reveal within the multi-file `vscode.changes` editor.** + Rejected per architect direction — requires the internal + `_workbench.openMultiDiffEditor` command and migrating the working `viewDiff` + open path. The file-list approach avoids both. +- **Alternative: store a full nav session per builder at `viewDiff` time.** + Unnecessary — the cache already holds the list and the active editor already + encodes the current position; deriving both on demand avoids a second source + of truth that could drift. ## Test Plan ### Unit (`diff-nav.test.ts`, vitest) -- `diffFileOrder` returns modified fsPaths in plans order (ordering == - file-list order). -- `computeNavTarget`: mid-list advances/retreats by one; at last index + forward - → `atEdge: true`, index unchanged; at index 0 + backward → `atEdge: true`. -- `indexOfFsPath`: resolves a file's index; returns -1 for an unknown path; two - independent session objects each resolve against their own `files` (multi- - builder isolation). -- `contributes-commands.test.ts`: both new command ids are declared with titles. - -### Manual (dev-approval gate — the load-bearing verification) -- Spawn/identify two builders with non-trivial multi-file diffs. -- `codev.viewDiff` on builder A; run `codev.diffNextFile` repeatedly → editor - walks A's files in list order; at the last file → status-bar "Last file…", - no wrap. `codev.diffPreviousFile` walks back; at first → "First file…". -- Confirm focus stays in the diff editor (not stolen by the file list) and that - it works with the file-list pane collapsed. -- Confirm within-file F7 / Shift+F7 hunk navigation still works unchanged. -- Open builder B's View Diff; navigate → B's own file list/pointer, independent - of A (isolation). Switch back to A's tab, navigate → resumes A's pointer. -- Sanity-check the existing View Diff still renders correctly post-migration - (added/modified/renamed/deleted/binary files, correct labels/icons). +- `orderedRelPaths` returns `files` rel-paths in git order. +- `computeNavTarget`: mid-list advances/retreats by one; at last + forward → + `atEdge:true`, index unchanged; at 0 + backward → `atEdge:true`. +- `indexOfRelPath`: resolves an index; -1 for unknown; two independent lists each + resolve against their own files (multi-builder isolation). +- `contributes-commands.test.ts`: both new command ids declared with titles. + +### Manual (dev-approval gate — load-bearing) +- Two builders with non-trivial multi-file diffs. +- Open a file diff for builder A (sidebar click or View Diff). `codev.diffNextFile` + repeatedly → walks A's files in list order; at the last file → status-bar + "Last file…", no wrap. `codev.diffPreviousFile` walks back; at first → + "First file…". +- Confirm within-file F7 / Shift+F7 still works on each opened diff. +- Confirm it works with the Builders sidebar collapsed/hidden. +- Open a file for builder B; navigate → B's own list, independent of A + (isolation). Switch back to an A file; navigate → resumes within A's list. +- Confirm no tab pile-up across a full walk; confirm existing `codev.viewDiff` + and `codev.openBuilderFileDiff` are unchanged. diff --git a/codev/state/pir-1060_thread.md b/codev/state/pir-1060_thread.md index f60d5483f..6394b7cdd 100644 --- a/codev/state/pir-1060_thread.md +++ b/codev/state/pir-1060_thread.md @@ -15,3 +15,21 @@ Migrate `viewDiff`'s open call from `vscode.changes` to `_workbench.openMultiDiffEditor` with an explicit, deterministic per-builder source URI (`codev-multidiff:/`). Navigation commands re-invoke the same command with the same source URI + resources + `reveal.modifiedUri`. A module-level nav-session store (keyed by builder id) holds the ordered resources + current index; active session resolved from `activeTextEditor`'s fsPath (falls back to most-recent). Pure helpers (ordering, edge-clamp, index-by-fsPath) carry the unit tests. Plan written to `codev/plans/1060-vscode-next-file-previous-file.md`. Awaiting `plan-approval`. + +### Revision 1 (architect feedback) +Architect redirected: **don't rely on VSCode multi-diff internals at all.** Reuse +the Builders sidebar's existing changed-file list mechanism. Rewrote the plan +around: +- `BuilderDiffCache.getDiff(builderId, worktreePath)` → ordered `files` (same + source the sidebar uses; 15s TTL so no per-keypress git spawn). +- Current position resolved from the active editor via `getDiffInjectEntry(fsPath)` + → `{builderId, relPath}` (already populated by viewDiff + openBuilderFileDiff). +- Open the next/prev file via the **existing** per-file `vscode.diff` path + (extract `openBuilderFileDiff` helper, shared by the sidebar command + nav). +- No `_workbench.openMultiDiffEditor`, no viewDiff migration, no regression + surface on the working View Diff open path. + +New plan-gate items surfaced: navigation order (recommend canonical git order = +flat sidebar + View Diff list; file-tree mode visual order differs), and that +navigation opens per-file diffs (GitHub PR-review model). Recommitted; still +awaiting `plan-approval`. From 6f80195c68c1a76b605ff479e5d1dfe1d8bf3c6e Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 17:15:24 +1000 Subject: [PATCH 05/25] [PIR #1060] Plan: specify preview-tab reuse for navigation (decision #8) --- .../1060-vscode-next-file-previous-file.md | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/codev/plans/1060-vscode-next-file-previous-file.md b/codev/plans/1060-vscode-next-file-previous-file.md index fe5440906..594a66c93 100644 --- a/codev/plans/1060-vscode-next-file-previous-file.md +++ b/codev/plans/1060-vscode-next-file-previous-file.md @@ -88,8 +88,9 @@ The existing handler becomes a thin caller (it already receives a If `atEdge` → status-bar message ("Last file in diff" / "First file in diff"), **no wrap**, return. 5. **Open** the target via the extracted `openBuilderFileDiff` helper - (`files[target].plan`, `worktreePath`, `baseRef`, `builderId`). Update the - "last navigated position". + (`files[target].plan`, `worktreePath`, `baseRef`, `builderId`), passing + `{ preview: true }` (see decision #8 — reuses a single tab instead of piling + up). Update the "last navigated position". Two commands, both thin wrappers: `codev.diffNextFile` → `navigateDiff(1)`, `codev.diffPreviousFile` → `navigateDiff(-1)`. @@ -166,6 +167,18 @@ skeleton-mirrored framework file. model. Invoking next/prev while the multi-file View Diff editor is focused drills into the next file as a per-file diff. I'll confirm this feels right at the dev-approval gate. +8. **(New) Tab behavior on a walk** → **open with `preview: true`; do not + force-close.** VSCode preview tabs are "replaced and reused until set to stay," + so walking files reuses **one** diff tab rather than accumulating N tabs — the + same mechanic as single-clicking Explorer files or stepping through search + results. Caveat (per the API doc): the preview flag is *ignored* if the user + has disabled preview editors (`workbench.editor.enablePreview: false`), in + which case tabs accumulate — consistent with that user's global choice that + every open is permanent. Recommend respecting the setting (no force-close). + Flag for the gate: if you want guaranteed single-tab behavior even with + preview disabled, I'll add a "close the previously-navigated diff tab before + opening the next" step (more complexity; must avoid closing a pinned/dirty + tab). ## Risks & Alternatives Considered @@ -177,9 +190,10 @@ skeleton-mirrored framework file. in the last 15s might not be in the list yet. Acceptable — it's the same list the sidebar shows, and the reviewer is reviewing a (mostly settled) branch. A refresh (collapse/expand the builder, or the 60s poll) reconciles it. -- **Risk: opening per-file diffs accumulates tabs.** `vscode.diff` reuses the - active diff tab when opening a new diff in place (preview-tab behavior), so a - walk doesn't pile up N tabs. I'll verify at dev-approval. +- **Risk: opening per-file diffs accumulates tabs.** Handled by opening with + `preview: true` (decision #8) — preview tabs are replaced/reused, so a walk + leaves one tab. Only piles up if the user disabled preview editors globally. + Verified at dev-approval. - **Alternative: scroll/reveal within the multi-file `vscode.changes` editor.** Rejected per architect direction — requires the internal `_workbench.openMultiDiffEditor` command and migrating the working `viewDiff` From 452e8dc2c461c450f49acc569539a1934fb83d36 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 17:22:01 +1000 Subject: [PATCH 06/25] chore(porch): 1060 plan-approval gate-approved --- .../projects/1060-vscode-next-file-previous-file/status.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index 90804c5c3..4c836d365 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -6,8 +6,9 @@ plan_phases: [] current_plan_phase: null gates: plan-approval: - status: pending + status: approved requested_at: '2026-06-17T06:41:51.549Z' + approved_at: '2026-06-17T07:22:01.551Z' dev-approval: status: pending pr: @@ -16,4 +17,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T06:41:51.550Z' +updated_at: '2026-06-17T07:22:01.551Z' From fa1623dbc28fee8081d541568fa7493aae2e31a4 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 17:22:53 +1000 Subject: [PATCH 07/25] chore(porch): 1060 implement phase-transition --- .../projects/1060-vscode-next-file-previous-file/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index 4c836d365..b680c12dc 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -1,7 +1,7 @@ id: '1060' title: vscode-next-file-previous-file protocol: pir -phase: plan +phase: implement plan_phases: [] current_plan_phase: null gates: @@ -17,4 +17,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T07:22:01.551Z' +updated_at: '2026-06-17T07:22:52.966Z' From 5bda593a848a578d3464f321b21a52328520e6f1 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 17:31:14 +1000 Subject: [PATCH 08/25] [PIR #1060] Extract openBuilderFileDiff helper, refactor sidebar handler --- packages/vscode/src/commands/view-diff.ts | 39 +++++++++++++++++++++++ packages/vscode/src/extension.ts | 25 +++++++-------- 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/packages/vscode/src/commands/view-diff.ts b/packages/vscode/src/commands/view-diff.ts index d173aabe8..00fd995c0 100644 --- a/packages/vscode/src/commands/view-diff.ts +++ b/packages/vscode/src/commands/view-diff.ts @@ -30,6 +30,7 @@ import * as path from 'node:path'; import type { ConnectionManager } from '../connection-manager.js'; import { parseHunkRanges, parseUnifiedDiff } from '../diff-inject-ref.js'; import { setDiffInjectSession, upsertDiffInjectEntry, type DiffInjectSessionEntry } from '../diff-inject-codelens.js'; +import { ensureDiffEditorCodeLens } from '../ensure-diff-codelens.js'; const execFileAsync = promisify(execFile); @@ -403,6 +404,44 @@ export async function viewDiff( } } +/** + * Open one builder changed-file as a per-file `vscode.diff` and register its + * inject-codelens session. This is the shared seam behind both the Builders + * tree's `codev.openBuilderFileDiff` command (a single sidebar-row click) and + * the cross-file navigation commands (`codev.diffNextFile` / `diffPreviousFile`, + * #1060), so a navigated file opens exactly like a clicked one. + * + * The diff opens FIRST so it appears instantly; the lens registration + git + * hunk computation happen after (the entry registers synchronously so the + * symbol/file lenses render right away, and the hunk lenses refresh once git + * resolves — #789). `showOptions` is forwarded to `vscode.diff`: navigation + * passes `{ preview: true }` so a walk reuses one preview tab instead of piling + * up a tab per file (#1060). The sidebar click passes nothing, preserving its + * existing open behavior exactly. + */ +export async function openBuilderFileDiff( + context: vscode.ExtensionContext, + args: { worktreePath: string; baseRef: string; builderId: string; plan: ResourcePlan }, + showOptions?: vscode.TextDocumentShowOptions, +): Promise { + const { left, right } = diffUrisForChange(args.plan, { wt: args.worktreePath, ref: args.baseRef }); + const title = `${args.plan.resourcePath} (#${args.builderId})`; + if (showOptions) { + await vscode.commands.executeCommand('vscode.diff', left, right, title, showOptions); + } else { + await vscode.commands.executeCommand('vscode.diff', left, right, title); + } + await registerFileInjectSession({ + worktreePath: args.worktreePath, + baseRef: args.baseRef, + builderId: args.builderId, + plan: args.plan, + }); + // Offer to enable diffEditor.codeLens (off by default — VS Code hides CodeLens + // in diff editors). After the open, so it never delays it. + await ensureDiffEditorCodeLens(context); +} + /** * Register the inject-codelens entry for a single changed file — the per-file * `vscode.diff` opened from the Builders tree (`openBuilderFileDiff`). Mirrors diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index 47dabb34a..9e772f82f 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -7,9 +7,9 @@ import { sendMessage } from './commands/send.js'; import { approveGate } from './commands/approve.js'; import { cleanupBuilder } from './commands/cleanup.js'; import { openWorktreeWindow } from './commands/open-worktree-window.js'; -import { viewDiff, activateDiffView, diffUrisForChange, registerFileInjectSession } from './commands/view-diff.js'; +import { viewDiff, activateDiffView, openBuilderFileDiff } from './commands/view-diff.js'; +import { navigateDiff } from './commands/diff-nav.js'; import { activateDiffInjectCodeLens, getDiffInjectEntry } from './diff-inject-codelens.js'; -import { ensureDiffEditorCodeLens } from './ensure-diff-codelens.js'; import { buildBuilderRangeRef } from './diff-inject-ref.js'; import { runWorktreeDev } from './commands/run-worktree-dev.js'; import { stopWorktreeDev } from './commands/stop-worktree-dev.js'; @@ -876,24 +876,21 @@ export async function activate(context: vscode.ExtensionContext) { }), reg('codev.openBuilderFileDiff', async (arg: unknown) => { if (!(arg instanceof BuilderFileTreeItem)) { return; } - // Open the diff FIRST so it appears instantly. Lens registration and - // the git hunk computation happen after — the entry registers - // synchronously (symbol/file lenses render right away) and the hunk - // lenses refresh in once git resolves (#789). Doing this before the - // open used to block the diff on a git subprocess. - const { left, right } = diffUrisForChange(arg.plan, { wt: arg.worktreePath, ref: arg.baseRef }); - const title = `${arg.plan.resourcePath} (#${arg.builderId})`; - await vscode.commands.executeCommand('vscode.diff', left, right, title); - await registerFileInjectSession({ + await openBuilderFileDiff(context, { worktreePath: arg.worktreePath, baseRef: arg.baseRef, builderId: arg.builderId, plan: arg.plan, }); - // Offer to enable diffEditor.codeLens (off by default — VS Code hides - // CodeLens in diff editors). After the open, so it never delays it. - await ensureDiffEditorCodeLens(context); }), + // Cross-file navigation in a builder diff review (#1060): walk the + // builder's changed-file list top-to-bottom (next) / bottom-to-top (prev), + // opening each file's per-file diff. CLI-independent — operates on editor + // state + the shared diff cache, not Tower. + reg('codev.diffNextFile', () => + navigateDiff(1, { context, overviewCache, diffCache: builderDiffCache })), + reg('codev.diffPreviousFile', () => + navigateDiff(-1, { context, overviewCache, diffCache: builderDiffCache })), regCli('codev.runWorktreeDev', (arg: vscode.TreeItem | string | undefined) => runWorktreeDev(connectionManager!, terminalManager!, extractBuilderId(arg))), regCli('codev.stopWorktreeDev', () => From 50f7fb25948032676098f98f7130e6042d055b2b Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 17:31:14 +1000 Subject: [PATCH 09/25] [PIR #1060] Add diffNextFile/diffPreviousFile cross-file navigation commands --- packages/vscode/package.json | 8 ++ packages/vscode/src/commands/diff-nav.ts | 133 +++++++++++++++++++++++ 2 files changed, 141 insertions(+) create mode 100644 packages/vscode/src/commands/diff-nav.ts diff --git a/packages/vscode/package.json b/packages/vscode/package.json index 779fb7104..61cba58a6 100644 --- a/packages/vscode/package.json +++ b/packages/vscode/package.json @@ -202,6 +202,14 @@ "command": "codev.openBuilderFileDiff", "title": "Codev: Open Builder File Diff" }, + { + "command": "codev.diffNextFile", + "title": "Codev: Go to Next File in Diff" + }, + { + "command": "codev.diffPreviousFile", + "title": "Codev: Go to Previous File in Diff" + }, { "command": "codev.openWorktreeWindow", "title": "Codev: Open Worktree in New Window" diff --git a/packages/vscode/src/commands/diff-nav.ts b/packages/vscode/src/commands/diff-nav.ts new file mode 100644 index 000000000..77f643fbd --- /dev/null +++ b/packages/vscode/src/commands/diff-nav.ts @@ -0,0 +1,133 @@ +/** + * Cross-file navigation in a Codev diff review (#1060). + * + * `codev.diffNextFile` / `codev.diffPreviousFile` walk a builder's changed-file + * list top-to-bottom, opening each file's per-file diff — the keyboard + * equivalent of clicking the next file row in the Builders sidebar (GitHub PR + * review's `j` / `k`). + * + * The mechanic reuses already-shipped pieces rather than VS Code multi-diff + * internals: + * - The ordered list comes from `BuilderDiffCache.getDiff` — the same source + * (and 15s TTL cache) that backs the sidebar's changed-file rows, in + * `git diff --name-status` order. + * - The current position is read from the active editor via the diff-inject + * registry (`getDiffInjectEntry`), which both View Diff and the per-file + * diff already populate with `{ builderId, relPath }`. So "where am I" is + * just the file the diff editor is showing — no stored pointer to drift. + * - The target file opens through the shared `openBuilderFileDiff` helper, + * with `{ preview: true }` so a walk reuses one tab. + * + * A module-level "last navigated position" is the only retained state — a + * fallback for when the active editor isn't a tracked diff file (e.g. the user + * focused a terminal between keypresses). + */ + +import * as vscode from 'vscode'; +import type { OverviewCache } from '../views/overview-data.js'; +import type { BuilderDiffCache, BuilderFileChange } from '../views/builder-diff-cache.js'; +import { getDiffInjectEntry } from '../diff-inject-codelens.js'; +import { openBuilderFileDiff } from './view-diff.js'; + +// ── Pure helpers (no vscode/git dependency — unit-tested directly) ────────── + +/** Navigation order: changed-file rel-paths in list (git `--name-status`) order. */ +export function orderedRelPaths(files: BuilderFileChange[]): string[] { + return files.map(f => f.plan.resourcePath); +} + +/** + * Step `index` by `direction` within `[0, count)`. At the first/last file a step + * past the edge is a no-op (`atEdge: true`, index unchanged) — no wrap-around. + */ +export function computeNavTarget( + index: number, + count: number, + direction: 1 | -1, +): { index: number; atEdge: boolean } { + const target = index + direction; + if (target < 0 || target >= count) { + return { index, atEdge: true }; + } + return { index: target, atEdge: false }; +} + +/** Index of `relPath` in the file list, or -1 if absent / undefined. */ +export function indexOfRelPath(files: BuilderFileChange[], relPath: string | undefined): number { + if (relPath === undefined) { return -1; } + return files.findIndex(f => f.plan.resourcePath === relPath); +} + +// ── Command implementation ────────────────────────────────────────────────── + +interface NavDeps { + context: vscode.ExtensionContext; + overviewCache: OverviewCache; + diffCache: BuilderDiffCache; +} + +/** Last file opened via navigation — fallback when the active editor isn't a + * tracked diff file. Module-level by design: navigation is a singleton gesture. */ +let lastPosition: { builderId: string; relPath: string } | undefined; + +function flash(message: string): void { + vscode.window.setStatusBarMessage(`Codev: ${message}`, 4000); +} + +export async function navigateDiff(direction: 1 | -1, deps: NavDeps): Promise { + // 1. Resolve the current builder + file from the active editor, falling back + // to the last navigated position. + const activeFsPath = vscode.window.activeTextEditor?.document.uri.fsPath; + const entry = activeFsPath ? getDiffInjectEntry(activeFsPath) : undefined; + const current = entry + ? { builderId: entry.builderId, relPath: entry.relPath } + : lastPosition; + if (!current) { + flash('open a builder file diff first'); + return; + } + + // 2. Resolve the worktree path for that builder (synchronous overview read). + const worktreePath = deps.overviewCache + .getData() + ?.builders.find(b => b.id === current.builderId)?.worktreePath; + if (!worktreePath) { + flash(`no worktree on record for ${current.builderId}`); + return; + } + + // 3. Load the builder's ordered changed-file list (cached). + const result = await deps.diffCache.getDiff(current.builderId, worktreePath); + if (result.error || result.files.length === 0) { + flash('no changed files to navigate'); + return; + } + + // 4. Find where we are; bail if the current file isn't in this list. + const idx = indexOfRelPath(result.files, current.relPath); + if (idx < 0) { + flash('current file is not in this diff'); + return; + } + + // 5. Step, honoring the edges (no wrap). + const { index: targetIdx, atEdge } = computeNavTarget(idx, result.files.length, direction); + if (atEdge) { + flash(direction === 1 ? 'last file in diff' : 'first file in diff'); + return; + } + + // 6. Open the target file's diff as a reused preview tab. + const target = result.files[targetIdx]!; + await openBuilderFileDiff( + deps.context, + { worktreePath, baseRef: result.baseRef, builderId: current.builderId, plan: target.plan }, + { preview: true }, + ); + lastPosition = { builderId: current.builderId, relPath: target.plan.resourcePath }; +} + +/** Reset retained navigation state — for tests. */ +export function resetDiffNavState(): void { + lastPosition = undefined; +} From f9fe8b7638b9d20735dde82044bec03c5a64cc17 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 17:31:14 +1000 Subject: [PATCH 10/25] [PIR #1060] Tests for nav helpers + command declarations --- .../__tests__/contributes-commands.test.ts | 13 +++ .../vscode/src/__tests__/diff-nav.test.ts | 103 ++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 packages/vscode/src/__tests__/diff-nav.test.ts diff --git a/packages/vscode/src/__tests__/contributes-commands.test.ts b/packages/vscode/src/__tests__/contributes-commands.test.ts index 7435ffcb3..5222928a9 100644 --- a/packages/vscode/src/__tests__/contributes-commands.test.ts +++ b/packages/vscode/src/__tests__/contributes-commands.test.ts @@ -45,6 +45,19 @@ describe('package.json contributes.commands', () => { expect(offenders).toEqual([]); }); + it('declares the cross-file diff navigation commands (#1060) with palette titles', () => { + // Palette-discoverable: declared with a title and NOT hidden via a + // commandPalette `when:false` entry (decision #1 — no default keybindings, + // palette is the discovery surface). + const palette: Array<{ command: string; when?: string }> = + PKG.contributes.menus?.commandPalette ?? []; + for (const command of ['codev.diffNextFile', 'codev.diffPreviousFile']) { + expect(titleByCommand.get(command), `${command} missing title`).toBeTruthy(); + const hidden = palette.find((m) => m.command === command && m.when === 'false'); + expect(hidden, `${command} must stay palette-discoverable`).toBeUndefined(); + } + }); + it('does not label a command "(internal)" if it is exposed in view/item/context', () => { const offenders = viewContextCommands .filter((cmd) => /\(internal\)/i.test(titleByCommand.get(cmd) ?? '')) diff --git a/packages/vscode/src/__tests__/diff-nav.test.ts b/packages/vscode/src/__tests__/diff-nav.test.ts new file mode 100644 index 000000000..56f5533f1 --- /dev/null +++ b/packages/vscode/src/__tests__/diff-nav.test.ts @@ -0,0 +1,103 @@ +/** + * #1060 — cross-file diff navigation pure helpers. + * + * These cover the logic that the acceptance criteria pin down without needing a + * live VS Code: navigation order matches the file list, the edges no-op (no + * wrap), and two builders' lists resolve independently (multi-builder + * isolation). The command glue (`navigateDiff`) is exercised manually at the + * dev-approval gate. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { BuilderFileChange } from '../views/builder-diff-cache.js'; + +// `diff-nav.ts` imports `vscode` and pulls in `diff-inject-codelens`, which +// instantiates an `EventEmitter` at module load. The pure helpers under test +// touch none of it — this minimal mock just lets the import chain resolve. +vi.mock('vscode', () => ({ + EventEmitter: class { + event = (): { dispose(): void } => ({ dispose() {} }); + fire(): void {} + dispose(): void {} + }, +})); + +const { orderedRelPaths, computeNavTarget, indexOfRelPath } = await import('../commands/diff-nav.js'); + +/** Minimal `BuilderFileChange` — the helpers only read `plan.resourcePath`. */ +function mk(relPath: string): BuilderFileChange { + return { + change: { status: 'M', oldPath: null, path: relPath }, + plan: { resourcePath: relPath, left: { kind: 'base', path: relPath }, right: { kind: 'file', path: relPath } }, + }; +} + +describe('orderedRelPaths', () => { + it('returns rel-paths in the file-list (git --name-status) order, unchanged', () => { + const files = [mk('src/z.ts'), mk('src/a.ts'), mk('README.md')]; + // Deliberately non-alphabetical: navigation order is the list order, NOT a sort. + expect(orderedRelPaths(files)).toEqual(['src/z.ts', 'src/a.ts', 'README.md']); + }); + + it('is empty for an empty list', () => { + expect(orderedRelPaths([])).toEqual([]); + }); +}); + +describe('computeNavTarget', () => { + const count = 3; + + it('advances by one mid-list', () => { + expect(computeNavTarget(0, count, 1)).toEqual({ index: 1, atEdge: false }); + expect(computeNavTarget(1, count, 1)).toEqual({ index: 2, atEdge: false }); + }); + + it('retreats by one mid-list', () => { + expect(computeNavTarget(2, count, -1)).toEqual({ index: 1, atEdge: false }); + expect(computeNavTarget(1, count, -1)).toEqual({ index: 0, atEdge: false }); + }); + + it('no-ops at the last file going forward (no wrap)', () => { + expect(computeNavTarget(2, count, 1)).toEqual({ index: 2, atEdge: true }); + }); + + it('no-ops at the first file going backward (no wrap)', () => { + expect(computeNavTarget(0, count, -1)).toEqual({ index: 0, atEdge: true }); + }); + + it('treats a single-file list as both edges', () => { + expect(computeNavTarget(0, 1, 1)).toEqual({ index: 0, atEdge: true }); + expect(computeNavTarget(0, 1, -1)).toEqual({ index: 0, atEdge: true }); + }); +}); + +describe('indexOfRelPath', () => { + const files = [mk('a.ts'), mk('b.ts'), mk('c.ts')]; + + it('finds the index of a present file', () => { + expect(indexOfRelPath(files, 'b.ts')).toBe(1); + }); + + it('returns -1 for an absent file', () => { + expect(indexOfRelPath(files, 'zzz.ts')).toBe(-1); + }); + + it('returns -1 for an undefined rel-path', () => { + expect(indexOfRelPath(files, undefined)).toBe(-1); + }); + + it('resolves two builders independently (multi-builder isolation)', () => { + const builderA = [mk('a/one.ts'), mk('a/two.ts')]; + const builderB = [mk('b/alpha.ts'), mk('b/beta.ts'), mk('b/gamma.ts')]; + + // A's file isn't in B's list and vice-versa; each list has its own indices. + expect(indexOfRelPath(builderA, 'a/two.ts')).toBe(1); + expect(indexOfRelPath(builderB, 'a/two.ts')).toBe(-1); + expect(indexOfRelPath(builderB, 'b/gamma.ts')).toBe(2); + expect(indexOfRelPath(builderA, 'b/gamma.ts')).toBe(-1); + + // Stepping in one list is unaffected by the other's length. + expect(computeNavTarget(indexOfRelPath(builderA, 'a/two.ts'), builderA.length, 1)).toEqual({ index: 1, atEdge: true }); + expect(computeNavTarget(indexOfRelPath(builderB, 'b/beta.ts'), builderB.length, 1)).toEqual({ index: 2, atEdge: false }); + }); +}); From a8babdcbcdb8fb3186bdfe458b57c1956da34378 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 17:31:14 +1000 Subject: [PATCH 11/25] [PIR #1060] Update thread log (implement phase) --- codev/state/pir-1060_thread.md | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/codev/state/pir-1060_thread.md b/codev/state/pir-1060_thread.md index 6394b7cdd..9a2814901 100644 --- a/codev/state/pir-1060_thread.md +++ b/codev/state/pir-1060_thread.md @@ -33,3 +33,43 @@ New plan-gate items surfaced: navigation order (recommend canonical git order = flat sidebar + View Diff list; file-tree mode visual order differs), and that navigation opens per-file diffs (GitHub PR-review model). Recommitted; still awaiting `plan-approval`. + +Decision #8 added (tab behavior): navigation opens with `preview: true` so a walk +reuses one tab; no force-close (respects user's enablePreview setting). + +## Implement phase + +plan-approval APPROVED. Building per approved plan: +1. Extract `openBuilderFileDiff(args)` helper from the extension.ts command body + into view-diff.ts (shared by sidebar command + nav); pass `preview: true`. +2. New `commands/diff-nav.ts`: `navigateDiff(dir, deps)` + pure helpers + (`orderedRelPaths`, `computeNavTarget`, `indexOfRelPath`). +3. Register `codev.diffNextFile` / `codev.diffPreviousFile` in extension.ts. +4. package.json contributes.commands (no keybindings). +5. Tests: diff-nav.test.ts + extend contributes-commands.test.ts. +6. CHANGELOG + UNRELEASED. + +### Implementation complete +- `view-diff.ts`: added `openBuilderFileDiff(context, args, showOptions?)` — the + shared per-file open seam (vscode.diff + registerFileInjectSession + + ensureDiffEditorCodeLens). Sidebar handler now calls it with no options + (byte-identical behavior); nav calls it with `{preview:true}`. +- `commands/diff-nav.ts` (new): `navigateDiff(dir, deps)` + pure helpers. Current + position from `getDiffInjectEntry(activeEditor.fsPath)` with a module-level + last-position fallback; list from `BuilderDiffCache.getDiff`; worktree from + `overviewCache`. Status-bar message + no wrap at edges. +- `extension.ts`: registered `codev.diffNextFile` / `codev.diffPreviousFile` + (reg = CLI-independent). +- `package.json`: two palette commands, no keybindings. +- Tests: `diff-nav.test.ts` (11 cases — ordering, edge no-op, isolation), + extended `contributes-commands.test.ts`. + +Verified: `pnpm compile` (check-types + lint + esbuild) ✓, `pnpm test:unit` ✓ +(438 tests, 11 new). Had to build workspace deps first (types/core/ +artifact-canvas had no dist in the fresh worktree). + +**Changelog deviation from plan**: the plan listed CHANGELOG.md + UNRELEASED.md, +but the established workflow keeps those on the divergent `docs/vscode-changelog` +branch (`worktrees/changelog/`), updated by the architect post-cleanup — "neither +branch touches the other's files" by design. So I did NOT touch them on the +builder branch; flagged for the architect instead. From 57219588e083cb39d0e7a322a26ba1be0d53ef56 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 17:31:51 +1000 Subject: [PATCH 12/25] chore(porch): 1060 dev-approval gate-requested --- codev/projects/1060-vscode-next-file-previous-file/status.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index b680c12dc..7a860c217 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -11,10 +11,11 @@ gates: approved_at: '2026-06-17T07:22:01.551Z' dev-approval: status: pending + requested_at: '2026-06-17T07:31:51.489Z' pr: status: pending iteration: 1 build_complete: false history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T07:22:52.966Z' +updated_at: '2026-06-17T07:31:51.490Z' From 6649e752add59e0b0e36d8b7421e4b191663f456 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 18:19:02 +1000 Subject: [PATCH 13/25] [PIR #1060] Add Ctrl+Alt+] / Ctrl+Alt+[ keybindings (builder-diff scoped) --- packages/vscode/package.json | 10 ++++++++++ .../__tests__/contributes-commands.test.ts | 20 ++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/packages/vscode/package.json b/packages/vscode/package.json index 61cba58a6..cb76aeb86 100644 --- a/packages/vscode/package.json +++ b/packages/vscode/package.json @@ -708,6 +708,16 @@ "mac": "cmd+k b", "when": "codev.activeEditorIsBuilderFile && editorHasSelection" }, + { + "command": "codev.diffNextFile", + "key": "ctrl+alt+]", + "when": "codev.activeEditorIsBuilderFile" + }, + { + "command": "codev.diffPreviousFile", + "key": "ctrl+alt+[", + "when": "codev.activeEditorIsBuilderFile" + }, { "command": "workbench.view.extension.codev", "key": "ctrl+alt+c", diff --git a/packages/vscode/src/__tests__/contributes-commands.test.ts b/packages/vscode/src/__tests__/contributes-commands.test.ts index 5222928a9..7418ca609 100644 --- a/packages/vscode/src/__tests__/contributes-commands.test.ts +++ b/packages/vscode/src/__tests__/contributes-commands.test.ts @@ -45,16 +45,30 @@ describe('package.json contributes.commands', () => { expect(offenders).toEqual([]); }); - it('declares the cross-file diff navigation commands (#1060) with palette titles', () => { + it('declares the cross-file diff navigation commands (#1060), palette-discoverable + bound', () => { // Palette-discoverable: declared with a title and NOT hidden via a - // commandPalette `when:false` entry (decision #1 — no default keybindings, - // palette is the discovery surface). + // commandPalette `when:false` entry. Also bound to Ctrl+Alt+] / Ctrl+Alt+[, + // gated by `codev.activeEditorIsBuilderFile` so the keys only fire on a + // builder diff (and don't shadow those chords elsewhere). Avoids function + // keys — within-file hunk nav keeps F7 / Shift+F7. const palette: Array<{ command: string; when?: string }> = PKG.contributes.menus?.commandPalette ?? []; + const keybindings: Array<{ command: string; key?: string; when?: string }> = + PKG.contributes.keybindings ?? []; + const expectedKey: Record = { + 'codev.diffNextFile': 'ctrl+alt+]', + 'codev.diffPreviousFile': 'ctrl+alt+[', + }; for (const command of ['codev.diffNextFile', 'codev.diffPreviousFile']) { expect(titleByCommand.get(command), `${command} missing title`).toBeTruthy(); const hidden = palette.find((m) => m.command === command && m.when === 'false'); expect(hidden, `${command} must stay palette-discoverable`).toBeUndefined(); + + const binding = keybindings.find((k) => k.command === command); + expect(binding, `${command} missing keybinding`).toBeDefined(); + expect(binding!.key).toBe(expectedKey[command]); + expect(binding!.key, `${command} must not use a function key`).not.toMatch(/F\d/i); + expect(binding!.when).toBe('codev.activeEditorIsBuilderFile'); } }); From 7ddfd1bd8972058927ddbd25fa0b881f34c9f5c8 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 18:19:02 +1000 Subject: [PATCH 14/25] [PIR #1060] Update thread log (keybindings at dev-approval) --- codev/state/pir-1060_thread.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/codev/state/pir-1060_thread.md b/codev/state/pir-1060_thread.md index 9a2814901..676d86ad9 100644 --- a/codev/state/pir-1060_thread.md +++ b/codev/state/pir-1060_thread.md @@ -73,3 +73,12 @@ but the established workflow keeps those on the divergent `docs/vscode-changelog branch (`worktrees/changelog/`), updated by the architect post-cleanup — "neither branch touches the other's files" by design. So I did NOT touch them on the builder branch; flagged for the architect instead. + +### At dev-approval gate: added default keybindings (reverses decision #1) +Architect asked for shortcut keys, avoiding function keys. Added defaults: +`Ctrl+Alt+]` (next) / `Ctrl+Alt+[` (prev), `when: codev.activeEditorIsBuilderFile`. +Used the Codev-specific context key (not generic `isInDiffEditor`) — matches the +Cmd/Ctrl+K B convention and prevents the keys firing in an unrelated diff (which +would fall back to a stale last-position and jump a different builder's file). +Within-file hunk nav stays VSCode-native F7 / Shift+F7. Extended +contributes-commands.test.ts to lock the bindings. Tests ✓ (438). From f7ece56982ff01e721f1d567e61fe70d25a0557f Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 20:53:25 +1000 Subject: [PATCH 15/25] [PIR #1060] Thread: filed sidebar-sync follow-up #1066 --- codev/state/pir-1060_thread.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/codev/state/pir-1060_thread.md b/codev/state/pir-1060_thread.md index 676d86ad9..eccaf3575 100644 --- a/codev/state/pir-1060_thread.md +++ b/codev/state/pir-1060_thread.md @@ -82,3 +82,11 @@ Cmd/Ctrl+K B convention and prevents the keys firing in an unrelated diff (which would fall back to a stale last-position and jump a different builder's file). Within-file hunk nav stays VSCode-native F7 / Shift+F7. Extended contributes-commands.test.ts to lock the bindings. Tests ✓ (438). + +### Follow-up filed: sidebar selection sync +Architect reviewed nav at dev-approval; nav "looks good" but doesn't sync the +Builders sidebar selection to the active diff file. Assessed reveal feasibility +(createTreeView gives reveal, but getParent is builder-row-only, file rows lack +stable ids, accordion id-versioning + tree-mode folder hierarchy needed). Agreed +to keep #1060 clean and file a follow-up. Filed #1066 (area/vscode). #1060 still +at dev-approval. From 274e6554a2a0644022ae22e4d7eb39f5240af819 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 20:57:58 +1000 Subject: [PATCH 16/25] chore(porch): 1060 dev-approval gate-approved --- .../projects/1060-vscode-next-file-previous-file/status.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index 7a860c217..3564f387e 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -10,12 +10,13 @@ gates: requested_at: '2026-06-17T06:41:51.549Z' approved_at: '2026-06-17T07:22:01.551Z' dev-approval: - status: pending + status: approved requested_at: '2026-06-17T07:31:51.489Z' + approved_at: '2026-06-17T10:57:58.745Z' pr: status: pending iteration: 1 build_complete: false history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T07:31:51.490Z' +updated_at: '2026-06-17T10:57:58.747Z' From db050b31393e04d8ac6912f3c437c8a8da795ea9 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 20:59:28 +1000 Subject: [PATCH 17/25] chore(porch): 1060 review phase-transition --- .../projects/1060-vscode-next-file-previous-file/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index 3564f387e..2702bafb9 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -1,7 +1,7 @@ id: '1060' title: vscode-next-file-previous-file protocol: pir -phase: implement +phase: review plan_phases: [] current_plan_phase: null gates: @@ -19,4 +19,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T10:57:58.747Z' +updated_at: '2026-06-17T10:59:28.052Z' From c3f949f400e37a9e8af6bc8f686a9f97509df64c Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 21:01:07 +1000 Subject: [PATCH 18/25] [PIR #1060] Review + retrospective --- codev/resources/lessons-learned.md | 2 + .../1060-vscode-next-file-previous-file.md | 64 +++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 codev/reviews/1060-vscode-next-file-previous-file.md diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index 90762a4af..d4ea76401 100644 --- a/codev/resources/lessons-learned.md +++ b/codev/resources/lessons-learned.md @@ -323,6 +323,8 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated - [From #789] CodeLens placement in diff editors is doubly constrained, and it dictated a whole feature's design. (1) VSCode's **multi-file diff editor** (`vscode.changes` — what `codev.viewDiff` opens) renders **no** CodeLenses at all; (2) the **single-file** diff editor hides them unless `diffEditor.codeLens` is enabled (off by default — microsoft/vscode#97640). So a "lens on the diff" feature can only live in the per-file `vscode.diff` / normal editor tabs, and must prompt to enable the setting (at **Global** scope — it's a personal editor preference; writing Workspace would edit the repo's shared `.vscode/settings.json` and force it on collaborators). **Context menus are NOT suppressed in the multi-file editor**, so a right-click/keybinding action is the only affordance that works in the scan view. Validate the rendering surface with a running-worktree spike *before* building on it — unit tests assert lens *shape*, never whether the host editor displays them (sibling to [From 799]). - [From #789] For annotating a diff, drive granularity off the **code (document symbols), not git hunks**. A brand-new file is a single whole-file hunk, so hunk-driven lenses give it exactly one lens regardless of size; symbol-driven lenses (via `vscode.executeDocumentSymbolProvider`) give every function/class/interface its own affordance, so new and modified files are equally addressable. Also: emit one lens per **contiguous changed run** (split each hunk on context lines; deletions don't break a run), not per git hunk — git coalesces adjacent edits into one hunk, which would collapse several distinct changes onto one lens. - [From #789] Don't add a "performance" cache before confirming the hot path. A per-version document-symbol cache was added to fix a ~100% CPU spike, but the spike was an unrelated newly-installed extension (the **Extension Test Runner**) walking the `.builders/` worktree farm (`rg --no-ignore --follow` over pnpm symlinks) — the extension-host CPU profile was ~99% idle. `provideCodeLenses` is not a hot path (VSCode caches lenses and only re-queries on invalidation), so the cache bought nothing and risked pinning an empty result when the language server was still warming up on first open. Profile first; cache only a proven-hot, proven-pure path. +- [From #1060] To programmatically navigate/reveal a file inside VSCode's multi-file diff editor, the public `vscode.changes` command is a dead end: it synthesizes a **random** source URI (`multi-diff-editor:${ms+Math.random()}`), so you can't re-address that editor afterward, and there's no public file-granular nav (`multiDiffEditor.goToNextChange` is *hunk*-granular). The only programmatic reveal is the internal `_workbench.openMultiDiffEditor({ multiDiffSourceUri, resources, reveal:{modifiedUri} })` with a deterministic source URI. But the cheaper, more robust design (chosen here) sidesteps multi-diff internals entirely: reuse the **already-built changed-file list** (the `BuilderDiffCache` that backs the sidebar) and open each file as a per-file `vscode.diff`, resolving "current file" from `activeTextEditor` via the diff-inject registry. Before reaching for an internal `_workbench.*` command, check whether an existing list + the per-file diff surface already gives you what you need. +- [From #1060] VSCode preview tabs (`TextDocumentShowOptions.preview: true`) are "replaced and reused until set to stay," so a file-walk reuses one tab instead of piling up N — but the flag is **ignored when the user disabled preview editors** (`workbench.editor.enablePreview: false`). Pass `preview: true` for sequential-open UX and respect the user's global setting rather than force-closing tabs yourself. Scope a navigation keybinding to the **feature-specific** context key (`codev.activeEditorIsBuilderFile`), not the generic `isInDiffEditor`: the broad key would fire the command in unrelated diffs where it falls back to stale state and acts on the wrong target. ## Documentation diff --git a/codev/reviews/1060-vscode-next-file-previous-file.md b/codev/reviews/1060-vscode-next-file-previous-file.md new file mode 100644 index 000000000..50d3f5a78 --- /dev/null +++ b/codev/reviews/1060-vscode-next-file-previous-file.md @@ -0,0 +1,64 @@ +# PIR Review: Cross-file navigation in a Codev View Diff session + +Fixes #1060 + +## Summary + +Adds two palette + keyboard commands, `codev.diffNextFile` / `codev.diffPreviousFile` (Ctrl+Alt+] / Ctrl+Alt+[), that walk a builder's changed-file list and open each file's per-file diff — the keyboard equivalent of clicking the next file row in the Builders sidebar (GitHub PR review's `j`/`k`). The implementation deliberately reuses already-shipped machinery (the `BuilderDiffCache` that backs the sidebar for the ordered list, the diff-inject registry for the current position, and the per-file `vscode.diff` open path) rather than driving VSCode's multi-file diff editor internals, so there is no change to how `codev.viewDiff` opens. + +## Files Changed + +- `packages/vscode/src/commands/diff-nav.ts` (+133 / -0) — new: `navigateDiff` + pure helpers (`orderedRelPaths`, `computeNavTarget`, `indexOfRelPath`) +- `packages/vscode/src/commands/view-diff.ts` (+39 / -0) — extracted `openBuilderFileDiff(context, args, showOptions?)`, the shared per-file open seam +- `packages/vscode/src/extension.ts` (+11 / -14) — registered the two commands; refactored `codev.openBuilderFileDiff` to call the extracted helper +- `packages/vscode/package.json` (+18 / -0) — two `contributes.commands` + two `contributes.keybindings` entries +- `packages/vscode/src/__tests__/diff-nav.test.ts` (+103 / -0) — new: unit tests for the pure helpers +- `packages/vscode/src/__tests__/contributes-commands.test.ts` (+27 / -0) — assert the commands are declared, palette-discoverable, and bound (no function keys) +- `codev/resources/lessons-learned.md` — two cold-tier lessons (see below) + +## Commits + +- `5bda593a` [PIR #1060] Extract openBuilderFileDiff helper, refactor sidebar handler +- `50f7fb25` [PIR #1060] Add diffNextFile/diffPreviousFile cross-file navigation commands +- `f9fe8b76` [PIR #1060] Tests for nav helpers + command declarations +- `6649e752` [PIR #1060] Add Ctrl+Alt+] / Ctrl+Alt+[ keybindings (builder-diff scoped) +- (plus thread-log commits) + +## Test Results + +- `pnpm compile` (check-types + lint + esbuild): ✓ pass +- `pnpm test:unit`: ✓ pass (438 tests, 11 new in `diff-nav.test.ts` + 1 extended in `contributes-commands.test.ts`) +- Manual verification (human, at the `dev-approval` gate): walked files via the new commands in a running worktree — confirmed working ("File Navigation looks good"). The sidebar-selection-sync gap noticed during this review was spun out to a separate issue (#1066) rather than folded in. + +## Architecture Updates + +No arch changes needed. The feature introduces no new module boundaries: it reuses `BuilderDiffCache` (changed-file list), the diff-inject registry (`getDiffInjectEntry`), and the existing per-file diff path. The one structural touch — extracting `openBuilderFileDiff` from the `extension.ts` command handler into `view-diff.ts` — is a same-seam refactor (the sidebar command and the nav commands now share it), not a boundary change, so it doesn't rise to an `arch.md` / `arch-critical.md` entry. + +## Lessons Learned Updates + +Two cold-tier lessons added to `codev/resources/lessons-learned.md` (UI/UX section) — both VSCode-diff-narrow, so COLD not HOT: + +1. Programmatic reveal inside VSCode's multi-file diff editor requires the internal `_workbench.openMultiDiffEditor` (the public `vscode.changes` synthesizes a random source URI and has no file-granular nav) — but the cheaper, more robust path is to reuse an existing changed-file list + per-file diffs. Check for an existing list before reaching for an internal `_workbench.*` command. +2. `preview: true` reuses one tab for a sequential-open walk (but is ignored when the user disabled preview editors — respect that, don't force-close); and scope a nav keybinding to the feature-specific context key (`codev.activeEditorIsBuilderFile`), not the broad `isInDiffEditor`, so it can't fire in an unrelated diff and act on stale state. + +## Things to Look At During PR Review + +- **`navigateDiff` current-position resolution** (`diff-nav.ts`): it reads the active editor's fsPath via the diff-inject registry, with a module-level `lastPosition` fallback for when the active editor isn't a tracked diff file. Worth a look that the fallback + the no-op status messages (no session / file-not-in-list / at-edge) cover the cases sensibly. +- **`openBuilderFileDiff` behavior preservation**: the sidebar command (`codev.openBuilderFileDiff`) now calls the helper with no `showOptions`, so it runs `vscode.diff(left, right, title)` with no 4th arg — byte-identical to the pre-refactor call. Only navigation passes `{ preview: true }`. Confirm the refactor didn't change the sidebar-click path. +- **Keybinding `when` clause**: I used `codev.activeEditorIsBuilderFile` rather than the generic `isInDiffEditor` shown in the plan-gate option, to avoid the chord firing in unrelated diffs (where it would fall back to a stale position). Flagging the deliberate deviation. +- **Navigation order**: canonical `git --name-status` order (matches the View Diff file list and the sidebar's flat-list mode). In the sidebar's *file-tree* mode the visual order differs (folders-first/alphabetical) — documented as a plan decision, not a bug. + +## How to Test Locally + +For reviewers pulling the branch: + +- **Run dev server**: VSCode sidebar → right-click builder `pir-1060` → **Run Dev Server**, or `afx dev pir-1060` +- **What to verify**: + - Open a builder file diff (sidebar click or View Diff), run *Codev: Go to Next File in Diff* (or Ctrl+Alt+]) repeatedly → steps through files in list order, reusing one tab; at the last file a status-bar message ("last file in diff"), no wrap. Previous (Ctrl+Alt+[) walks back; at the first, "first file in diff". + - Within-file hunk nav (F7 / Shift+F7) still works on each opened file. + - Open a second builder's file; navigation stays within whichever builder's file is currently shown (isolation). + - Works with the Builders sidebar collapsed/hidden. + +## Changelog + +Per the established workflow, `packages/vscode/CHANGELOG.md` and `docs/releases/UNRELEASED.md` are **not** touched on this branch — they live on the divergent `docs/vscode-changelog` branch (`worktrees/changelog/`) and are added by the architect post-cleanup. Flagged in the architect notification. From 748c99ef447dee026771b0ca1179632477e9d42b Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 21:01:25 +1000 Subject: [PATCH 19/25] chore(porch): 1060 record PR #1067 --- .../1060-vscode-next-file-previous-file/status.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index 2702bafb9..f4fb95901 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -19,4 +19,9 @@ iteration: 1 build_complete: false history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T10:59:28.052Z' +updated_at: '2026-06-17T11:01:25.099Z' +pr_history: + - phase: review + pr_number: 1067 + branch: builder/pir-1060 + created_at: '2026-06-17T11:01:25.098Z' From 33d2071602392c6155e96d6723763895ccb5ba24 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 21:01:35 +1000 Subject: [PATCH 20/25] chore(porch): 1060 review build-complete --- .../projects/1060-vscode-next-file-previous-file/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index f4fb95901..56ecfe70d 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -16,10 +16,10 @@ gates: pr: status: pending iteration: 1 -build_complete: false +build_complete: true history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T11:01:25.099Z' +updated_at: '2026-06-17T11:01:35.053Z' pr_history: - phase: review pr_number: 1067 From c5d657fde393898c0a6c79c1449bdd60fcba208f Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 21:08:00 +1000 Subject: [PATCH 21/25] [PIR #1060] Seed nav anchor on every diff open (fixes deleted/binary-file nav start) --- .../vscode/src/__tests__/diff-nav.test.ts | 48 ++++++++++++++++++- packages/vscode/src/commands/diff-nav.ts | 32 +++++++++++-- packages/vscode/src/extension.ts | 6 ++- 3 files changed, 80 insertions(+), 6 deletions(-) diff --git a/packages/vscode/src/__tests__/diff-nav.test.ts b/packages/vscode/src/__tests__/diff-nav.test.ts index 56f5533f1..4429de4c4 100644 --- a/packages/vscode/src/__tests__/diff-nav.test.ts +++ b/packages/vscode/src/__tests__/diff-nav.test.ts @@ -8,7 +8,7 @@ * dev-approval gate. */ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import type { BuilderFileChange } from '../views/builder-diff-cache.js'; // `diff-nav.ts` imports `vscode` and pulls in `diff-inject-codelens`, which @@ -22,7 +22,14 @@ vi.mock('vscode', () => ({ }, })); -const { orderedRelPaths, computeNavTarget, indexOfRelPath } = await import('../commands/diff-nav.js'); +const { + orderedRelPaths, + computeNavTarget, + indexOfRelPath, + recordDiffNavPosition, + peekDiffNavPosition, + resetDiffNavState, +} = await import('../commands/diff-nav.js'); /** Minimal `BuilderFileChange` — the helpers only read `plan.resourcePath`. */ function mk(relPath: string): BuilderFileChange { @@ -86,6 +93,21 @@ describe('indexOfRelPath', () => { expect(indexOfRelPath(files, undefined)).toBe(-1); }); + it('resolves a deleted file — deletions are in the list and navigable once anchored', () => { + // Regression for the Codex review finding: a deleted file (status 'D') has no + // `file:` doc, so it can't be resolved through the diff-inject registry — but + // it IS in the changed-file list, so once the nav anchor points at it (seeded + // on open), indexOfRelPath finds it and stepping works. + const withDeleted: BuilderFileChange[] = [ + mk('keep.ts'), + { change: { status: 'D', oldPath: null, path: 'gone.ts' }, + plan: { resourcePath: 'gone.ts', left: { kind: 'base', path: 'gone.ts' }, right: { kind: 'empty' } } }, + mk('next.ts'), + ]; + expect(indexOfRelPath(withDeleted, 'gone.ts')).toBe(1); + expect(computeNavTarget(1, withDeleted.length, 1)).toEqual({ index: 2, atEdge: false }); + }); + it('resolves two builders independently (multi-builder isolation)', () => { const builderA = [mk('a/one.ts'), mk('a/two.ts')]; const builderB = [mk('b/alpha.ts'), mk('b/beta.ts'), mk('b/gamma.ts')]; @@ -101,3 +123,25 @@ describe('indexOfRelPath', () => { expect(computeNavTarget(indexOfRelPath(builderB, 'b/beta.ts'), builderB.length, 1)).toEqual({ index: 2, atEdge: false }); }); }); + +describe('nav position anchor (recordDiffNavPosition / peek / reset)', () => { + beforeEach(() => resetDiffNavState()); + + it('starts empty', () => { + expect(peekDiffNavPosition()).toBeUndefined(); + }); + + it('records and overwrites the anchor (seeded on every open, incl. deleted/binary)', () => { + recordDiffNavPosition('b1', 'src/gone.ts'); // e.g. a deleted file opened from the sidebar + expect(peekDiffNavPosition()).toEqual({ builderId: 'b1', relPath: 'src/gone.ts' }); + + recordDiffNavPosition('b2', 'pkg/other.ts'); // a later open replaces it (latest wins) + expect(peekDiffNavPosition()).toEqual({ builderId: 'b2', relPath: 'pkg/other.ts' }); + }); + + it('reset clears the anchor', () => { + recordDiffNavPosition('b1', 'a.ts'); + resetDiffNavState(); + expect(peekDiffNavPosition()).toBeUndefined(); + }); +}); diff --git a/packages/vscode/src/commands/diff-nav.ts b/packages/vscode/src/commands/diff-nav.ts index 77f643fbd..f6511708c 100644 --- a/packages/vscode/src/commands/diff-nav.ts +++ b/packages/vscode/src/commands/diff-nav.ts @@ -66,8 +66,19 @@ interface NavDeps { diffCache: BuilderDiffCache; } -/** Last file opened via navigation — fallback when the active editor isn't a - * tracked diff file. Module-level by design: navigation is a singleton gesture. */ +/** + * The last builder-diff file opened — by navigation OR by a direct open (sidebar + * click / View Diff, via `recordDiffNavPosition`). Used to resolve "where am I" + * when the active editor can't be matched through the diff-inject registry. + * + * That fallback is load-bearing for DELETED / BINARY files: their right side is + * a `codev-diff:` placeholder, not a `file:` document, so `registerFileInjectSession` + * never records them and `getDiffInjectEntry` can't resolve them. Seeding this on + * every open means navigation can still *start* from a deleted/binary file + * (otherwise next/prev would bail with "open a builder file diff first"). + * + * Module-level by design: navigation is a singleton gesture. + */ let lastPosition: { builderId: string; relPath: string } | undefined; function flash(message: string): void { @@ -124,7 +135,22 @@ export async function navigateDiff(direction: 1 | -1, deps: NavDeps): Promise Date: Wed, 17 Jun 2026 21:08:00 +1000 Subject: [PATCH 22/25] [PIR #1060] Record keybinding plan amendment + consultation dispositions --- codev/plans/1060-vscode-next-file-previous-file.md | 11 ++++++++--- codev/reviews/1060-vscode-next-file-previous-file.md | 8 ++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/codev/plans/1060-vscode-next-file-previous-file.md b/codev/plans/1060-vscode-next-file-previous-file.md index 594a66c93..459624e3e 100644 --- a/codev/plans/1060-vscode-next-file-previous-file.md +++ b/codev/plans/1060-vscode-next-file-previous-file.md @@ -143,9 +143,14 @@ skeleton-mirrored framework file. ## Plan-Gate Decisions -1. **Default keybindings** → **None (palette-only) + docs.** Smallest blast - radius; no clobbering existing bindings. Heavy users bind `j`/`k` themselves. - *(issue's lean)* +1. **Default keybindings** → ~~None (palette-only) + docs.~~ **AMENDED at the + dev-approval gate (architect direction):** ship defaults **Ctrl+Alt+]** (next) + / **Ctrl+Alt+[** (prev), avoiding function keys (F7 / Shift+F7 stay for + within-file hunk nav). Scoped to `when: codev.activeEditorIsBuilderFile` (not + the generic `isInDiffEditor`) so the chords only fire on a builder diff and + don't act on stale state in unrelated diffs. The original palette-only lean + was reversed by the human during dev-approval; recording it here so the plan + matches the shipped code. 2. **Edge behavior** → **status-bar message + no wrap.** *(issue's lean)* 3. **Scope** → **per-builder Codev diff list (v1).** Generic any-diff-editor is a follow-up. *(issue's lean)* diff --git a/codev/reviews/1060-vscode-next-file-previous-file.md b/codev/reviews/1060-vscode-next-file-previous-file.md index 50d3f5a78..bb5f5b8ba 100644 --- a/codev/reviews/1060-vscode-next-file-previous-file.md +++ b/codev/reviews/1060-vscode-next-file-previous-file.md @@ -41,6 +41,14 @@ Two cold-tier lessons added to `codev/resources/lessons-learned.md` (UI/UX secti 1. Programmatic reveal inside VSCode's multi-file diff editor requires the internal `_workbench.openMultiDiffEditor` (the public `vscode.changes` synthesizes a random source URI and has no file-granular nav) — but the cheaper, more robust path is to reuse an existing changed-file list + per-file diffs. Check for an existing list before reaching for an internal `_workbench.*` command. 2. `preview: true` reuses one tab for a sequential-open walk (but is ignored when the user disabled preview editors — respect that, don't force-close); and scope a nav keybinding to the feature-specific context key (`codev.activeEditorIsBuilderFile`), not the broad `isInDiffEditor`, so it can't fire in an unrelated diff and act on stale state. +## 3-Way Consultation (single advisory pass) + +- **Claude — APPROVE** (HIGH, no issues). Confirmed the refactor is behavior-preserving and called the keybindings a "good deviation." +- **Codex — REQUEST_CHANGES** (HIGH), two findings, both addressed: + 1. *Keybindings deviate from plan decision #1 (palette-only).* **Disposition: authorized, plan amended.** The keybindings were added at the human's explicit direction during the dev-approval gate (reversing the original lean, avoiding function keys); dev-approval was granted afterward. Plan decision #1 is now updated to record the amendment so the artifact matches the code. No code change. + 2. *Navigation can't be **initiated** from a deleted/binary file diff opened directly.* **Disposition: real defect, fixed + regression test.** Deleted/binary files have no `file:` right side, so `registerFileInjectSession` skips them and `getDiffInjectEntry` can't resolve them; with `lastPosition` unset, next/prev bailed. Fix: seed the nav anchor on *every* open via `recordDiffNavPosition` (called from the `codev.openBuilderFileDiff` handler), not just after a navigation step — so a subsequent next/prev resolves through the fallback. Regression coverage: `diff-nav.test.ts` now tests the record/peek/reset anchor and that a deleted file resolves in the list. Commit ``. +- **Gemini — no usable verdict.** The `agy`/Antigravity run misfired: it went off investigating a "--sandbox" prompt and never reviewed the diff (output in `codev/projects/1060-*/1060-review-iter1-gemini.txt`). Not re-run (PIR consultation is single-pass advisory); flagged for the human. + ## Things to Look At During PR Review - **`navigateDiff` current-position resolution** (`diff-nav.ts`): it reads the active editor's fsPath via the diff-inject registry, with a module-level `lastPosition` fallback for when the active editor isn't a tracked diff file. Worth a look that the fallback + the no-op status messages (no session / file-not-in-list / at-edge) cover the cases sensibly. From 5f973fbffd9fbfa9b098bc82f44e83b7778b537a Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 21:08:49 +1000 Subject: [PATCH 23/25] [PIR #1060] Consultation rebuttals (iteration 1) --- .../1060-review-iter1-rebuttals.md | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 codev/projects/1060-vscode-next-file-previous-file/1060-review-iter1-rebuttals.md diff --git a/codev/projects/1060-vscode-next-file-previous-file/1060-review-iter1-rebuttals.md b/codev/projects/1060-vscode-next-file-previous-file/1060-review-iter1-rebuttals.md new file mode 100644 index 000000000..5dc54c74c --- /dev/null +++ b/codev/projects/1060-vscode-next-file-previous-file/1060-review-iter1-rebuttals.md @@ -0,0 +1,39 @@ +# PIR #1060 — Consultation rebuttals (iteration 1, single advisory pass) + +Verdicts: **Claude APPROVE**, **Codex REQUEST_CHANGES**, **Gemini REQUEST_CHANGES** (recorded by porch; in practice the Gemini run misfired — see below). + +PIR consultation is a single advisory pass (`max_iterations: 1`) — there is no automated re-review. Dispositions below; the human is the remaining reviewer at the `pr` gate. + +## Codex — REQUEST_CHANGES (HIGH) + +### Finding 1: Keybindings deviate from the approved plan (decision #1 said palette-only) +**Disposition: authorized deviation, not a defect. Plan amended; no code change.** + +The `Ctrl+Alt+]` / `Ctrl+Alt+[` keybindings were added at the **human architect's explicit direction during the dev-approval gate** — they asked for shortcut keys and to avoid function keys. The dev-approval gate was then approved *with* the keybindings in place, so the running code the human signed off on already included them. Codex (correctly) flagged the plan↔code divergence without the conversation context that authorized it; Claude reviewed the same divergence and judged it sound ("good deviation"). + +To make the artifact self-consistent (Codex's suggested remedy was "plan amendment or revert"), I amended **plan decision #1** to record the reversal and the rationale (Ctrl+Alt+] / [, function-keys avoided, scoped to `codev.activeEditorIsBuilderFile`). No code change warranted. + +### Finding 2: Navigation cannot be *initiated* from a deleted/binary file diff +**Disposition: real defect. Fixed + regression test.** + +Confirmed: deleted/binary files have a `codev-diff:` placeholder right side, not a `file:` document, so `registerFileInjectSession` skips them (`view-diff.ts`) and `getDiffInjectEntry` can't resolve them. With `lastPosition` unset (no prior navigation this session), opening such a file directly from the sidebar and pressing next/prev bailed with "open a builder file diff first." Deletions *are* in the changed-file list, so they should be navigable. + +**Fix:** seed the nav anchor on **every** open, not just after a navigation step. Added `recordDiffNavPosition(builderId, relPath)` to `diff-nav.ts` and call it from the `codev.openBuilderFileDiff` command handler (`extension.ts`) after the diff opens. So a deleted/binary file opened from the sidebar now seeds `lastPosition`, and the subsequent next/prev resolves through the fallback. `navigateDiff` uses the same setter internally (no behavior change there). No circular import: `extension.ts` already imports from `diff-nav.ts`; `view-diff.ts` is untouched by this wiring. + +**Regression test** (`diff-nav.test.ts`): the record/peek/reset anchor state machine, plus a case asserting a deleted file (status `D`) resolves in the list and steps forward. Build + 442 unit tests green (4 new). + +(Residual edge, noted for transparency: focusing a deleted file *inside the multi-file View Diff editor* without a prior open/nav still can't seed the anchor — viewDiff registers only `file`-kind right sides. The sidebar-open path Codex called out is fixed; this narrower case is documented, low-frequency, and a candidate for follow-up if it matters.) + +## Gemini — REQUEST_CHANGES (recorded), but the run misfired +**Disposition: no actionable feedback — the consultation did not review the diff.** + +The Gemini (`agy`) output (`1060-review-iter1-gemini.txt`) shows the session went off investigating a "--sandbox" prompt and never inspected the PR or produced a structured verdict. There is no finding to address. Porch recorded the non-APPROVE as REQUEST_CHANGES, but there is nothing to act on. Flagged for the human at the `pr` gate; not re-run (single-pass advisory). + +## Claude — APPROVE (HIGH) +No issues. Verified behavior-preservation of the refactor and test coverage of the pure-logic surface. + +## Net +- One real defect fixed (deleted/binary nav-start) with a regression test. +- One divergence reconciled by amending the plan (human-authorized at dev-approval). +- One misfired model run with nothing to act on. +- The fix is pushed to PR #1067; the human verifies at the `pr` gate (PIR does not re-review). From b96574c0c6adefe455ac7214a4acda209fec1956 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 21:08:51 +1000 Subject: [PATCH 24/25] chore(porch): 1060 pr gate-requested --- .../projects/1060-vscode-next-file-previous-file/status.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/codev/projects/1060-vscode-next-file-previous-file/status.yaml b/codev/projects/1060-vscode-next-file-previous-file/status.yaml index 56ecfe70d..7b3d6190b 100644 --- a/codev/projects/1060-vscode-next-file-previous-file/status.yaml +++ b/codev/projects/1060-vscode-next-file-previous-file/status.yaml @@ -15,13 +15,15 @@ gates: approved_at: '2026-06-17T10:57:58.745Z' pr: status: pending + requested_at: '2026-06-17T11:08:51.510Z' iteration: 1 build_complete: true history: [] started_at: '2026-06-17T06:33:31.988Z' -updated_at: '2026-06-17T11:01:35.053Z' +updated_at: '2026-06-17T11:08:51.510Z' pr_history: - phase: review pr_number: 1067 branch: builder/pir-1060 created_at: '2026-06-17T11:01:25.098Z' +pr_ready_for_human: true From 4cc61cdbcb7c9396806adfa7b953a802fc5df643 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Wed, 17 Jun 2026 21:09:21 +1000 Subject: [PATCH 25/25] [PIR #1060] Thread: review phase + consultation dispositions --- codev/state/pir-1060_thread.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/codev/state/pir-1060_thread.md b/codev/state/pir-1060_thread.md index eccaf3575..7f5a5d91b 100644 --- a/codev/state/pir-1060_thread.md +++ b/codev/state/pir-1060_thread.md @@ -90,3 +90,18 @@ Builders sidebar selection to the active diff file. Assessed reveal feasibility stable ids, accordion id-versioning + tree-mode folder hierarchy needed). Agreed to keep #1060 clean and file a follow-up. Filed #1066 (area/vscode). #1060 still at dev-approval. + +## Review phase +Wrote review (codev/reviews/1060-*.md) + 2 cold lessons (lessons-learned.md UI/UX). +PR #1067 opened (Fixes #1060), recorded with porch. 3-way consult (single pass): +- claude APPROVE (no issues; called keybindings a "good deviation") +- codex REQUEST_CHANGES (HIGH): (1) keybindings vs plan decision #1 — authorized + by architect at dev-approval, amended the plan; (2) can't START nav from a + deleted/binary file diff — REAL defect, FIXED (32ff63f1): seed nav anchor on + every open via recordDiffNavPosition (called from openBuilderFileDiff handler) + + regression test. 442 tests green. +- gemini REQUEST_CHANGES recorded but the agy run MISFIRED (off on a --sandbox + tangent, never reviewed) — nothing actionable. +Rebuttals in codev/projects/1060-*/1060-review-iter1-rebuttals.md. PR body updated. +Notified architect (led with REQUEST_CHANGES + dispositions). Now waiting at the +`pr` gate — merge only on porch gate-approved wake-up, never on typed prose.