From bfacdf4114fc1d6349cb53012f35773f4be24501 Mon Sep 17 00:00:00 2001 From: Aleksandr Kladov Date: Tue, 28 Jul 2026 18:19:39 +0300 Subject: [PATCH] feat: toggle review in a dedicated tab --- README.md | 12 ++- agent-guide.md | 13 ++- herdr-plugin.toml | 2 +- src/open-review.mjs | 115 +++++++++++++++++------ src/review/ui.mjs | 19 ++++ test-bun/ui.test.mjs | 30 ++++++ test/actions-integration.test.mjs | 151 +++++++++++++++++++++++++----- 7 files changed, 277 insertions(+), 65 deletions(-) diff --git a/README.md b/README.md index d83f65f..fe1f160 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,8 @@ installation below. - Shows saved comment text inline beneath its anchored diff range. - Re-anchors comments deterministically after refresh or marks them stale without guessing. -- Hides and restores the same live pane with `F6`. +- Opens the live review in its own tab and toggles between review and agent + tabs with `F6`. - Inserts saved comments into the associated agent with `F7`, preserving existing input and never pressing Enter. - Migrates legacy human Hunk notes once, preserving an untouched `.v1.bak`. @@ -139,9 +140,12 @@ herdr server reload-config ## Usage Focus a detected coding agent inside a Git repository and press `F6`. The -native review opens beside that exact agent. Pressing `F6` again from the agent -or review moves the live pane to a background tab; another press restores the -same process and state. +native review opens in a dedicated tab and Herdr switches to it immediately. +Press `F6` from that review tab to return to the exact source agent tab. Press +`F6` from the source agent again to switch back to the same live review +process and state. The pane is never moved into a split or restarted while +toggling. An already-running split review from an older release is moved once +into its dedicated tab without restarting the pane. The review pane uses this stable key contract: diff --git a/agent-guide.md b/agent-guide.md index c667c7b..12e9070 100644 --- a/agent-guide.md +++ b/agent-guide.md @@ -127,7 +127,7 @@ Fix only problems introduced by this setup. An attached client may need ## 5. Explain the workflow 1. Focus a detected coding agent in a Git repository. -2. Press `F6` to open the native review. +2. Press `F6` to open the native review in a dedicated tab and switch to it. 3. Choose `1` for current uncommitted work, `2` for everything since the branch diverged from its local main/master base, or `3` for the latest file-changing turn observed from this exact agent. Navigate with `j`/`k`, @@ -137,7 +137,8 @@ Fix only problems introduced by this setup. An attached client may need divider to resize it, and toggle long-row wrapping with `w`. Letter and bracket shortcuts also work from a Russian keyboard layout. 4. Select a line/range with `v`, press `c`, and save with `Ctrl+S`. -5. Press `F6` to hide/restore the same live pane. +5. Press `F6` from the review tab to return to the source agent tab; press it + from the source agent to switch back to the same live review tab. 6. Press `F7` to insert open validated saved comments into the exact source agent. 7. Review or edit the draft, then press Enter manually. @@ -158,9 +159,11 @@ idle-to-working transition and freezes when that turn finishes. Press `?` or `F1` inside the pane for the full keyboard/mouse reference. `s` chooses the old/new target only for unchanged context lines; additions are always new and deletions are always old. -`Ctrl+C` closes the review pane cleanly; `F6` can reopen it with saved -comments, resolution state, active scope, sidebar visibility, sidebar width, -and row-wrap preference intact. +`Ctrl+C` closes the review pane cleanly. Normal `F6` tab switching does not +move, close, or restart the pane, so saved comments, resolution state, active +scope, sidebar visibility, sidebar width, and row-wrap preference stay intact. +An existing split review from an older release is moved into a dedicated tab +once, without restarting it. ## 6. Report diff --git a/herdr-plugin.toml b/herdr-plugin.toml index 093d21b..60cd166 100644 --- a/herdr-plugin.toml +++ b/herdr-plugin.toml @@ -11,7 +11,7 @@ command = ["npm", "ci", "--omit=dev"] [[actions]] id = "open-review" title = "Review changes" -description = "Open a live native review beside the focused agent." +description = "Open a live native review in a dedicated tab and toggle back to the focused agent." contexts = ["pane"] command = ["node", "src/open-review.mjs"] diff --git a/src/open-review.mjs b/src/open-review.mjs index 821ab11..42e446d 100644 --- a/src/open-review.mjs +++ b/src/open-review.mjs @@ -28,17 +28,51 @@ function getPane(herdr, paneId) { return parseCommandJson(result.stdout, "herdr pane get")?.result?.pane; } -function runPaneMove(herdr, args) { - const moved = spawnSync(herdr, ["pane", "move", ...args], { +function focusTab(herdr, tabId) { + const focused = spawnSync(herdr, ["tab", "focus", tabId], { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"], }); + if (focused.status !== 0) { + throw new Error(describeCommandFailure("herdr tab focus", focused)); + } +} + +function renameTab(herdr, tabId) { + const renamed = spawnSync(herdr, ["tab", "rename", tabId, "Review"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + if (renamed.status !== 0) { + throw new Error(describeCommandFailure("herdr tab rename", renamed)); + } +} + +function moveReviewToDedicatedTab(herdr, reviewPaneId, workspaceId) { + const moved = spawnSync( + herdr, + [ + "pane", + "move", + reviewPaneId, + "--new-tab", + "--workspace", + workspaceId, + "--label", + "Review", + "--focus", + ], + { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }, + ); if (moved.status !== 0) { throw new Error(describeCommandFailure("herdr pane move", moved)); } } -function toggleExistingReview(herdr, review) { +function toggleExistingReview(herdr, review, focusedPaneId) { const reviewPane = getPane(herdr, review.reviewPaneId); const agentPane = getPane(herdr, review.agentPaneId); if (!reviewPane || !agentPane) { @@ -46,31 +80,34 @@ function toggleExistingReview(herdr, review) { } if (reviewPane.tab_id === agentPane.tab_id) { - runPaneMove(herdr, [ + const workspaceId = agentPane.workspace_id ?? reviewPane.workspace_id; + if (!workspaceId) { + throw new Error("Herdr did not provide the review workspace."); + } + moveReviewToDedicatedTab( + herdr, review.reviewPaneId, - "--new-tab", - "--workspace", - agentPane.workspace_id, - "--label", - "Review", - "--no-focus", - ]); + workspaceId, + ); + process.stdout.write( + `Moved the existing review for ${review.repo} into its dedicated tab.\n`, + ); + return true; + } + + renameTab(herdr, reviewPane.tab_id); + const focusedPane = focusedPaneId + ? getPane(herdr, focusedPaneId) + : undefined; + if (focusedPane?.tab_id === reviewPane.tab_id) { + focusTab(herdr, agentPane.tab_id); process.stdout.write( - `Hid the review for ${review.repo} without closing its session.\n`, + `Returned to ${review.agentKind ?? "agent"} (${review.agentPaneId}) for ${review.repo}.\n`, ); } else { - runPaneMove(herdr, [ - review.reviewPaneId, - "--tab", - agentPane.tab_id, - "--split", - "right", - "--target-pane", - review.agentPaneId, - "--focus", - ]); + focusTab(herdr, reviewPane.tab_id); process.stdout.write( - `Restored the review for ${review.repo} beside ${review.agentKind ?? "agent"} (${review.agentPaneId}).\n`, + `Switched to the review tab for ${review.repo}.\n`, ); } return true; @@ -85,7 +122,10 @@ function main() { const focusedReview = state.reviews.find( (review) => review.reviewPaneId === context.focused_pane_id, ); - if (focusedReview && toggleExistingReview(herdr, focusedReview)) { + if ( + focusedReview && + toggleExistingReview(herdr, focusedReview, context.focused_pane_id) + ) { return; } @@ -115,11 +155,19 @@ function main() { } if ( activeMatches.length === 1 && - toggleExistingReview(herdr, activeMatches[0]) + toggleExistingReview(herdr, activeMatches[0], context.focused_pane_id) ) { return; } + const agentPane = getPane(herdr, agentPaneId); + if (!agentPane) { + throw new Error("The focused agent pane is no longer active."); + } + const workspaceId = agentPane.workspace_id ?? context.workspace_id; + if (!workspaceId) { + throw new Error("Herdr did not provide the agent workspace."); + } const reviewKey = matchingReviews.length === 1 ? matchingReviews[0].reviewKey @@ -133,11 +181,9 @@ function main() { "--entrypoint", "review", "--placement", - "split", - "--target-pane", - agentPaneId, - "--direction", - "right", + "tab", + "--workspace", + workspaceId, "--env", `HERDR_HUNK_REVIEW_KEY=${reviewKey}`, "--env", @@ -173,8 +219,15 @@ function main() { }), ); + const reviewTabId = + pane.tab_id ?? getPane(herdr, pane.pane_id)?.tab_id; + if (!reviewTabId) { + throw new Error("Herdr opened the review but did not return its tab ID."); + } + renameTab(herdr, reviewTabId); + process.stdout.write( - `Opened a review for ${repo} beside ${context.focused_pane_agent} (${agentPaneId}).\n`, + `Opened a review tab for ${repo} and switched to it.\n`, ); } diff --git a/src/review/ui.mjs b/src/review/ui.mjs index 1be7cc4..dba2be2 100644 --- a/src/review/ui.mjs +++ b/src/review/ui.mjs @@ -169,6 +169,8 @@ export class ReviewUI { this.selectedDetachedNoteId = null; this.diffNavigation = []; this.pendingG = false; + this.renderQueued = false; + this.renderPromise = null; this.root = new BoxRenderable(this.ctx, { id: "review-root", @@ -527,6 +529,23 @@ export class ReviewUI { } async render() { + this.renderQueued = true; + if (this.renderPromise) return this.renderPromise; + this.renderPromise = (async () => { + while (this.renderQueued) { + this.renderQueued = false; + await this.renderPass(); + } + })(); + try { + await this.renderPromise; + } finally { + this.renderPromise = null; + } + if (this.renderQueued) return this.render(); + } + + async renderPass() { const version = ++this.renderVersion; const narrow = this.renderer.terminalWidth < 72; const sidebarVisible = diff --git a/test-bun/ui.test.mjs b/test-bun/ui.test.mjs index b3f0a8b..6d8f40c 100644 --- a/test-bun/ui.test.mjs +++ b/test-bun/ui.test.mjs @@ -222,6 +222,36 @@ test("sidebar toggles without losing selection and file rows are not text-select ); }); +test("concurrent resize renders are serialized and coalesced", async () => { + let measuring = false; + let active = 0; + let maxActive = 0; + let calls = 0; + const highlighter = { + highlight: async (value) => { + if (!measuring) return value; + calls += 1; + active += 1; + maxActive = Math.max(maxActive, active); + await Bun.sleep(5); + active -= 1; + return value; + }, + }; + const app = await setup(100, 24, highlighter); + measuring = true; + + await Promise.all([ + app.ui.render(), + app.ui.render(), + app.ui.render(), + ]); + await app.flush(); + + expect(maxActive).toBe(1); + expect(calls).toBe(app.controller.file.rows.length * 2); +}); + test("scope switching isolates comments and persists the active scope", async () => { const app = await setup(); diff --git a/test/actions-integration.test.mjs b/test/actions-integration.test.mjs index 1ee79c5..34be458 100644 --- a/test/actions-integration.test.mjs +++ b/test/actions-integration.test.mjs @@ -36,24 +36,31 @@ appendFileSync(root + "/commands.jsonl", JSON.stringify(args) + "\\n"); const paneStatePath = root + "/pane-state.json"; const paneState = existsSync(paneStatePath) ? JSON.parse(readFileSync(paneStatePath, "utf8")) - : { reviewTab: "tab-main" }; + : { reviewTab: "tab-review", focusedTab: "tab-main" }; const reply = (value) => process.stdout.write(JSON.stringify(value) + "\\n"); if (args[0] === "pane" && args[1] === "get") { const id = args[2]; if (id === "w1:p1") reply({ result: { pane: { pane_id: id, tab_id: "tab-main", workspace_id: "w1" } } }); else if (id === "w1:p2") reply({ result: { pane: { pane_id: id, tab_id: paneState.reviewTab, workspace_id: "w1" } } }); else process.exitCode = 1; -} else if (args[0] === "pane" && args[1] === "move") { - paneState.reviewTab = args.includes("--new-tab") - ? "tab-background" - : args[args.indexOf("--tab") + 1]; +} else if (args[0] === "tab" && args[1] === "focus") { + paneState.focusedTab = args[2]; writeFileSync(paneStatePath, JSON.stringify(paneState)); - reply({ result: { move_result: { pane: { pane_id: "w1:p2" } } } }); + reply({ result: { tab: { tab_id: args[2], focused: true } } }); +} else if (args[0] === "tab" && args[1] === "rename") { + paneState.reviewLabel = args.slice(3).join(" "); + writeFileSync(paneStatePath, JSON.stringify(paneState)); + reply({ result: { tab: { tab_id: args[2], label: paneState.reviewLabel } } }); +} else if (args[0] === "pane" && args[1] === "move" && args.includes("--new-tab")) { + paneState.reviewTab = "tab-review"; + if (args.includes("--focus")) paneState.focusedTab = "tab-review"; + writeFileSync(paneStatePath, JSON.stringify(paneState)); + reply({ result: { move_result: { pane: { pane_id: args[2], tab_id: "tab-review" } } } }); } else if (args[0] === "pane" && args[1] === "list") { reply({ result: { panes: [{ pane_id: "w1:p1" }, { pane_id: "w1:p2" }] } }); } else if (args[0] === "plugin" && args[1] === "pane" && args[2] === "open") { - writeFileSync(paneStatePath, JSON.stringify({ reviewTab: "tab-main" })); - reply({ result: { plugin_pane: { pane: { pane_id: "w1:p2", workspace_id: "w1" } } } }); + writeFileSync(paneStatePath, JSON.stringify({ reviewTab: "tab-review", focusedTab: "tab-review" })); + reply({ result: { plugin_pane: { pane: { pane_id: "w1:p2", tab_id: "tab-review", workspace_id: "w1" } } } }); } else if (args[0] === "agent" && args[1] === "focus") { reply({ result: { pane_id: args[2] } }); } else if (args[0] === "notification" && args[1] === "show") { @@ -86,32 +93,36 @@ function actionEnv({ repo, stateDir, fakeHerdr, fakeData, socketPath, context }) }; } -test("F6 opens, hides, and restores the same native pane without a close or Hunk command", () => { +test("F6 opens a dedicated review tab and toggles focus without moving or closing its pane", () => { const root = mkdtempSync(join(tmpdir(), "herdr-actions-f6-")); const repo = join(root, "repo"); const stateDir = join(root, "state"); mkdirSync(repo); spawnSync("git", ["init", "-q", repo]); const fakeHerdr = createFakeHerdr(root); - const env = actionEnv({ - repo, - stateDir, - fakeHerdr, - fakeData: root, - context: { - focused_pane_id: "w1:p1", - focused_pane_agent: "codex", - }, - }); - - for (let count = 0; count < 3; count += 1) { + const runAction = (context) => { const result = spawnSync(process.execPath, ["src/open-review.mjs"], { cwd: process.cwd(), - env, + env: actionEnv({ + repo, + stateDir, + fakeHerdr, + fakeData: root, + context, + }), encoding: "utf8", }); assert.equal(result.status, 0, result.stderr); - } + }; + runAction({ + focused_pane_id: "w1:p1", + focused_pane_agent: "codex", + }); + runAction({ focused_pane_id: "w1:p2" }); + runAction({ + focused_pane_id: "w1:p1", + focused_pane_agent: "codex", + }); const state = readState(stateDir); assert.equal(state.reviews.length, 1); @@ -125,11 +136,103 @@ test("F6 opens, hides, and restores the same native pane without a close or Hunk 1, ); assert.equal( - commands.filter((args) => args.slice(0, 2).join(" ") === "pane move").length, + commands.filter((args) => args.slice(0, 2).join(" ") === "tab focus").length, 2, ); + const open = commands.find( + (args) => args.slice(0, 3).join(" ") === "plugin pane open", + ); + assert.equal(open[open.indexOf("--placement") + 1], "tab"); + assert.equal(open[open.indexOf("--workspace") + 1], "w1"); + assert.equal(open.includes("--target-pane"), false); + assert.equal( + commands.some((args) => args.slice(0, 2).join(" ") === "pane move"), + false, + ); assert.equal(commands.some((args) => args.includes("close")), false); assert.equal(commands.some((args) => args[0] === "hunk"), false); + assert.deepEqual( + commands + .filter((args) => args.slice(0, 2).join(" ") === "tab focus") + .map((args) => args[2]), + ["tab-main", "tab-review"], + ); + assert.deepEqual( + commands + .filter((args) => args.slice(0, 2).join(" ") === "tab rename") + .map((args) => args.slice(2)), + [ + ["tab-review", "Review"], + ["tab-review", "Review"], + ["tab-review", "Review"], + ], + ); +}); + +test("F6 migrates an existing split review into a dedicated tab without restarting it", () => { + const root = mkdtempSync(join(tmpdir(), "herdr-actions-f6-migrate-")); + const repo = join(root, "repo"); + const stateDir = join(root, "state"); + mkdirSync(repo); + spawnSync("git", ["init", "-q", repo]); + const fakeHerdr = createFakeHerdr(root); + writeFileSync( + join(root, "pane-state.json"), + JSON.stringify({ reviewTab: "tab-main", focusedTab: "tab-main" }), + ); + writeState( + stateDir, + upsertReview( + { version: 1, reviews: [] }, + { + reviewKey: "legacy-split", + repo, + agentPaneId: "w1:p1", + agentKind: "codex", + reviewPaneId: "w1:p2", + workspaceId: "w1", + openedAt: new Date().toISOString(), + }, + ), + ); + + const result = spawnSync(process.execPath, ["src/open-review.mjs"], { + cwd: process.cwd(), + env: actionEnv({ + repo, + stateDir, + fakeHerdr, + fakeData: root, + context: { + focused_pane_id: "w1:p1", + focused_pane_agent: "codex", + }, + }), + encoding: "utf8", + }); + assert.equal(result.status, 0, result.stderr); + + const commands = readFileSync(join(root, "commands.jsonl"), "utf8") + .trim() + .split("\n") + .map(JSON.parse); + const move = commands.find( + (args) => args.slice(0, 2).join(" ") === "pane move", + ); + assert.ok(move); + assert.equal(move.includes("--new-tab"), true); + assert.equal(move.includes("--focus"), true); + assert.equal(move[move.indexOf("--label") + 1], "Review"); + assert.equal( + commands.some( + (args) => args.slice(0, 3).join(" ") === "plugin pane open", + ), + false, + ); + assert.equal( + JSON.parse(readFileSync(join(root, "pane-state.json"), "utf8")).reviewTab, + "tab-review", + ); }); test("F7 loads the exact native store and inserts one unsubmitted human-only draft", async () => {