diff --git a/.changeset/flicker-free-history-review.md b/.changeset/flicker-free-history-review.md new file mode 100644 index 000000000..abb8a3bc2 --- /dev/null +++ b/.changeset/flicker-free-history-review.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Keep the themed history loading screen visible until a selected commit review is ready to claim the terminal. diff --git a/src/app/startup.test.ts b/src/app/startup.test.ts index 647101aec..ed0edf260 100644 --- a/src/app/startup.test.ts +++ b/src/app/startup.test.ts @@ -749,6 +749,35 @@ describe("startup planning", () => { expect(opened).toBe(1); }); + test("inherits handoff theme mode without querying the parent-owned terminal", async () => { + const cliInput: CliInput = { + kind: "show", + ref: "opaque:id", + options: { theme: "auto" }, + }; + let detected = 0; + + const plan = await prepareStartupPlan(["bun", "hunk", "show", "opaque:id"], { + parseCliImpl: async () => cliInput as ParsedCliInput, + resolveRuntimeCliInputImpl: (input) => input, + resolveConfiguredCliInputImpl: (input) => createTestConfigResolution(input), + loadAppBootstrapImpl: async (input) => createBootstrap(input), + detectTerminalThemeModeFromBackgroundImpl: async () => { + detected += 1; + return "light"; + }, + stdinIsTTY: true, + stdoutIsTTY: true, + env: { + HUNK_TERMINAL_HANDOFF: "1", + HUNK_TERMINAL_HANDOFF_THEME_MODE: "dark", + }, + }); + + expect(plan).toMatchObject({ kind: "app", bootstrap: { initialThemeMode: "dark" } }); + expect(detected).toBe(0); + }); + test("opens the controlling terminal for piped patch startup", async () => { const cliInput: CliInput = { kind: "patch", diff --git a/src/app/startup.ts b/src/app/startup.ts index a88daea9d..48f8c0cab 100644 --- a/src/app/startup.ts +++ b/src/app/startup.ts @@ -5,6 +5,7 @@ import { resolveConfiguredCliInput } from "../core/run/config"; import { HunkUserError } from "../core/run/errors"; import type { loadAppBootstrap } from "../core/changeset/loaders"; import { looksLikePatchInput } from "../core/process/pager"; +import { terminalHandoffThemeMode } from "../core/process/terminalHandoff"; import { sanitizeTerminalText } from "../lib/terminalText"; import { detectTerminalThemeModeFromBackground } from "../core/theme/detection"; import { @@ -543,8 +544,10 @@ export async function prepareStartupPlan( controllingTerminal = openControllingTerminalImpl(); } - let initialThemeMode: AppBootstrap["initialThemeMode"]; - if (cliInput.options.theme === "auto" && stdoutIsTTY) { + // A handoff child inherits the parent's detected mode so bootstrap never queries a terminal + // whose input and renderer are still exclusively owned by the history process. + let initialThemeMode: AppBootstrap["initialThemeMode"] = terminalHandoffThemeMode(env); + if (!initialThemeMode && cliInput.options.theme === "auto" && stdoutIsTTY) { const themeInput = controllingTerminal?.stdin ?? (stdinIsTTY ? process.stdin : null); if (themeInput) { initialThemeMode = diff --git a/src/core/process/terminalHandoff.test.ts b/src/core/process/terminalHandoff.test.ts new file mode 100644 index 000000000..08753d939 --- /dev/null +++ b/src/core/process/terminalHandoff.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, test } from "bun:test"; +import { + hasTerminalHandoff, + parseTerminalHandoffMessage, + terminalHandoffEnv, + terminalHandoffMessage, + terminalHandoffThemeMode, +} from "./terminalHandoff"; + +describe("terminal handoff protocol", () => { + test("uses a private marker and inherits only a valid terminal mode", () => { + const env = terminalHandoffEnv({ PATH: "/bin" }, "dark"); + expect(hasTerminalHandoff(env)).toBe(true); + expect(terminalHandoffThemeMode(env)).toBe("dark"); + expect( + terminalHandoffThemeMode({ + HUNK_TERMINAL_HANDOFF: "1", + HUNK_TERMINAL_HANDOFF_THEME_MODE: "blue", + }), + ).toBeUndefined(); + expect(terminalHandoffThemeMode({ HUNK_TERMINAL_HANDOFF_THEME_MODE: "light" })).toBeUndefined(); + }); + + test("accepts only versioned bounded messages", () => { + expect(parseTerminalHandoffMessage(terminalHandoffMessage("ready"))).toEqual({ + protocol: "hunk-terminal-handoff-v1", + kind: "ready", + }); + expect(parseTerminalHandoffMessage({ protocol: "wrong", kind: "ready" })).toBeUndefined(); + expect( + parseTerminalHandoffMessage({ protocol: "hunk-terminal-handoff-v1", kind: "other" }), + ).toBeUndefined(); + expect( + parseTerminalHandoffMessage({ + protocol: "hunk-terminal-handoff-v1", + kind: "failed", + message: "x".repeat(3_000), + }), + ).toEqual({ + protocol: "hunk-terminal-handoff-v1", + kind: "failed", + message: "x".repeat(2_000), + }); + }); +}); diff --git a/src/core/process/terminalHandoff.ts b/src/core/process/terminalHandoff.ts new file mode 100644 index 000000000..0f8cc3956 --- /dev/null +++ b/src/core/process/terminalHandoff.ts @@ -0,0 +1,116 @@ +const HANDOFF_ENV = "HUNK_TERMINAL_HANDOFF"; +const HANDOFF_THEME_MODE_ENV = "HUNK_TERMINAL_HANDOFF_THEME_MODE"; +const PROTOCOL = "hunk-terminal-handoff-v1"; + +export type TerminalHandoffMessage = + | { protocol: typeof PROTOCOL; kind: "ready" } + | { protocol: typeof PROTOCOL; kind: "release" } + | { protocol: typeof PROTOCOL; kind: "failed"; message: string }; + +/** Return whether this process was launched for a coordinated terminal handoff. */ +export function hasTerminalHandoff(env: NodeJS.ProcessEnv = process.env) { + return env[HANDOFF_ENV] === "1"; +} + +/** Read the parent's already-detected terminal mode without querying the owned terminal again. */ +export function terminalHandoffThemeMode( + env: NodeJS.ProcessEnv = process.env, +): "dark" | "light" | undefined { + if (!hasTerminalHandoff(env)) return undefined; + const value = env[HANDOFF_THEME_MODE_ENV]; + return value === "dark" || value === "light" ? value : undefined; +} + +/** Add the private one-shot handoff marker and terminal mode to a child environment. */ +export function terminalHandoffEnv( + env: NodeJS.ProcessEnv, + themeMode: "dark" | "light" | undefined, +): NodeJS.ProcessEnv { + return { + ...env, + [HANDOFF_ENV]: "1", + ...(themeMode ? { [HANDOFF_THEME_MODE_ENV]: themeMode } : {}), + }; +} + +/** Narrow an IPC payload to one bounded handoff protocol message. */ +export function parseTerminalHandoffMessage(value: unknown): TerminalHandoffMessage | undefined { + if (!value || typeof value !== "object") return undefined; + const message = value as Record; + if (message.protocol !== PROTOCOL) return undefined; + if (message.kind === "ready" || message.kind === "release") { + return { protocol: PROTOCOL, kind: message.kind }; + } + if (message.kind === "failed" && typeof message.message === "string") { + return { protocol: PROTOCOL, kind: "failed", message: message.message.slice(0, 2_000) }; + } + return undefined; +} + +/** Build one authenticated-by-inheritance IPC message for the handoff peer. */ +export function terminalHandoffMessage(kind: "ready" | "release"): TerminalHandoffMessage { + return { protocol: PROTOCOL, kind }; +} + +/** Tell the parent startup succeeded, then wait boundedly for exclusive terminal ownership. */ +export async function awaitTerminalHandoffRelease({ + env = process.env, + timeoutMs = 10_000, +}: { + env?: NodeJS.ProcessEnv; + timeoutMs?: number; +} = {}) { + if (!hasTerminalHandoff(env)) return; + if (typeof process.send !== "function" || !process.connected) { + throw new Error("The terminal handoff channel is unavailable."); + } + + await new Promise((resolve, reject) => { + let settled = false; + const finish = (error?: Error) => { + if (settled) return; + settled = true; + clearTimeout(timeout); + process.off("message", onMessage); + process.off("disconnect", onDisconnect); + if (error) reject(error); + else resolve(); + }; + const onMessage = (value: unknown) => { + const message = parseTerminalHandoffMessage(value); + if (message?.kind === "release") finish(); + }; + const onDisconnect = () => finish(new Error("The terminal handoff parent disconnected.")); + const timeout = setTimeout( + () => finish(new Error("Timed out waiting for terminal ownership.")), + timeoutMs, + ); + timeout.unref?.(); + process.on("message", onMessage); + process.once("disconnect", onDisconnect); + process.send!(terminalHandoffMessage("ready"), (error) => { + if (error) finish(error); + }); + }); + process.disconnect?.(); + delete env[HANDOFF_ENV]; + delete env[HANDOFF_THEME_MODE_ENV]; +} + +/** Report a bounded pre-render startup failure to a waiting parent. */ +export async function reportTerminalHandoffFailure( + error: unknown, + env: NodeJS.ProcessEnv = process.env, +) { + if (!hasTerminalHandoff(env) || typeof process.send !== "function" || !process.connected) { + return false; + } + const message = error instanceof Error ? error.message : String(error); + await new Promise((resolve) => { + process.send!({ protocol: PROTOCOL, kind: "failed", message: message.slice(0, 2_000) }, () => + resolve(), + ); + }); + process.disconnect?.(); + return true; +} diff --git a/src/main.tsx b/src/main.tsx index 7137bc4ee..23732524f 100644 --- a/src/main.tsx +++ b/src/main.tsx @@ -7,6 +7,10 @@ import { prepareStartupPlan } from "./app/startup"; import { sanitizeTerminalText } from "./lib/terminalText"; import { serveSessionBrokerDaemon } from "./session/broker/brokerServer"; import { runSessionCommand } from "./session/agent/commands"; +import { + awaitTerminalHandoffRelease, + reportTerminalHandoffFailure, +} from "./core/process/terminalHandoff"; async function main() { const startupPlan = await prepareStartupPlan(); @@ -128,11 +132,18 @@ async function main() { } // OpenTUI stays behind the interactive plan so headless commands never materialize its embedded - // native library. The highlighting client starts the compiled worker only when an opted-in, - // eligible diff needs it, so normal sessions do not pay its startup cost. The interactive - // app owns that worker's disposal: this call returns once the app is mounted, not once it exits. + // native library. Load it before declaring a delegated review ready so terminal release is + // followed immediately by renderer creation rather than another module-loading gap. + const { runInteractiveApp } = await import("./ui/runInteractiveApp"); + + // A history parent keeps its loading frame mounted until review bootstrap and renderer code are + // ready. Wait for exclusive terminal ownership before mounting the child renderer. + await awaitTerminalHandoffRelease(); + + // The highlighting client starts the compiled worker only when an opted-in, eligible diff needs + // it, so normal sessions do not pay its startup cost. The interactive app owns that worker's + // disposal: this call returns once the app is mounted, not once it exits. try { - const { runInteractiveApp } = await import("./ui/runInteractiveApp"); await runInteractiveApp(startupPlan); } catch (error) { startupPlan.controllingTerminal?.close(); @@ -143,7 +154,7 @@ async function main() { } } -await main().catch((error) => { - process.stderr.write(formatCliError(error)); - process.exit(1); +await main().catch(async (error) => { + if (!(await reportTerminalHandoffFailure(error))) process.stderr.write(formatCliError(error)); + process.exitCode = 1; }); diff --git a/src/ui/log/LogApp.tsx b/src/ui/log/LogApp.tsx index e6635ab48..38090f7b5 100644 --- a/src/ui/log/LogApp.tsx +++ b/src/ui/log/LogApp.tsx @@ -28,7 +28,12 @@ import { projectResponsiveLogRow, resolveLogResponsiveLayout } from "./responsiv export type LogAppOutcome = | { kind: "quit"; exitCode?: number } - | { kind: "open-review"; action: ExtensionVcsHistoryReviewAction; themeId: string }; + | { + kind: "open-review"; + action: ExtensionVcsHistoryReviewAction; + themeId: string; + themeMode: "dark" | "light"; + }; /** Render the bounded history list inside Hunk's shared desktop chrome. */ export function LogApp({ @@ -39,7 +44,7 @@ export function LogApp({ }: { controller: LogController; runtime: HistoryRuntime; - onOutcome: (outcome: LogAppOutcome) => void; + onOutcome: (outcome: LogAppOutcome) => void | Promise; useColor: boolean; }) { const snapshot = useSyncExternalStore(controller.subscribe, controller.getSnapshot); @@ -48,6 +53,7 @@ export function LogApp({ const [showHelp, setShowHelp] = useState(false); const [parentSelectorIndex, setParentSelectorIndex] = useState(null); const [transientNotice, setTransientNotice] = useState(""); + const [openingCommit, setOpeningCommit] = useState<{ id: string; subject: string } | null>(null); const lastClick = useRef({ index: -1, at: 0 }); // Lock synchronously before awaiting provider planning so coalesced Enter+q input cannot // quit the log or leak the trailing command into the child review. @@ -86,14 +92,25 @@ export function LogApp({ const planned = controller.planSelectedReview(parentRevisionId); if (!planned) return; reviewPending.current = true; + const currentRow = controller.getSelectedRow(); + setOpeningCommit( + currentRow + ? { + id: sanitizeTerminalLine(currentRow.commit.displayId), + subject: sanitizeTerminalLine(currentRow.commit.subject), + } + : null, + ); try { - onOutcome({ + await onOutcome({ kind: "open-review", action: await planned, themeId: themeController.themeId, + themeMode: terminalThemeMode, }); } catch (error) { reviewPending.current = false; + setOpeningCommit(null); controller.setNotice(error instanceof Error ? error.message : String(error)); } }; @@ -372,91 +389,112 @@ export function LogApp({ else if (direction === "down") controller.move(3, viewportHeight); }} > - {visible.map((row, offset) => { - const index = snapshot.top + offset; - const selected = index === snapshot.selected; - const projected = projectResponsiveLogRow({ - row, - presentation: snapshot.presentation, - layout: responsiveLayout, - width: terminal.width, - }); - return ( - { - clearTransientNotice(); - const now = Date.now(); - const shouldOpen = - lastClick.current.index === index && now - lastClick.current.at < 400; - void controller.select(index, viewportHeight).then(() => { - if (shouldOpen) void openSelected(); - }); - lastClick.current = { index, at: now }; - }} - > - {projected.graphWidth ? ( - - {projected.graph} - {Array.from({ length: responsiveLayout.rowHeight - 1 }, (_, line) => ( - - {projected.continuation} - - ))} - - ) : null} - - {projected.title} - {responsiveLayout.showDescription ? ( - {projected.description} - ) : null} - {projected.metadata} - - {projected.columnGap ? : null} + {openingCommit ? ( + + Opening commit + + {fitText( + `${openingCommit.id} · ${openingCommit.subject}`, + Math.max(1, terminal.width - 4), + )} + + Preparing review… + + ) : ( + visible.map((row, offset) => { + const index = snapshot.top + offset; + const selected = index === snapshot.selected; + const projected = projectResponsiveLogRow({ + row, + presentation: snapshot.presentation, + layout: responsiveLayout, + width: terminal.width, + }); + return ( { - event.stopPropagation(); + onMouseUp={() => { clearTransientNotice(); - const copyIconStart = terminal.width - 1 - measureTextWidth(projected.copyIcon); + const now = Date.now(); + const shouldOpen = + lastClick.current.index === index && now - lastClick.current.at < 400; void controller.select(index, viewportHeight).then(() => { - if (event.x >= copyIconStart) copySelected(row); - else void openSelected(); + if (shouldOpen) void openSelected(); }); + lastClick.current = { index, at: now }; }} > - - {projected.displayId} - {projected.copyIcon} + {projected.graphWidth ? ( + + {projected.graph} + {Array.from({ length: responsiveLayout.rowHeight - 1 }, (_, line) => ( + + {projected.continuation} + + ))} + + ) : null} + + {projected.title} + {responsiveLayout.showDescription ? ( + {projected.description} + ) : null} + {projected.metadata} + + {projected.columnGap ? : null} + { + event.stopPropagation(); + clearTransientNotice(); + const copyIconStart = terminal.width - 1 - measureTextWidth(projected.copyIcon); + void controller.select(index, viewportHeight).then(() => { + if (event.x >= copyIconStart) copySelected(row); + else void openSelected(); + }); + }} + > + + {projected.displayId} + {projected.copyIcon} + + {projected.secondary ? {projected.secondary} : null} - {projected.secondary ? {projected.secondary} : null} - - ); - })} + ); + }) + )} void) => { + child.sent.push(message); + callback?.(null); + return true; + }) as ChildProcess["send"]; + child.kill = ((signal: NodeJS.Signals = "SIGTERM") => { + if (signal === "SIGTERM" && ignoreTerm) return true; + child.signalCode = signal; + child.connected = false; + queueMicrotask(() => child.emit("exit", null, signal)); + return true; + }) as ChildProcess["kill"]; + return child; +} + +describe("history review readiness", () => { + test("does not release terminal ownership until the child reports ready", async () => { + const child = createTestChild(); + const preparedPromise = prepareHistoryReview( + createTestRuntime(), + { kind: "revision-show", revisionId: "opaque:id" }, + { + current: { command: "hunk", args: [] }, + spawnImpl: (() => child) as unknown as typeof spawn, + env: {}, + }, + ); + + expect(child.sent).toEqual([]); + child.emit("message", { protocol: "hunk-terminal-handoff-v1", kind: "ready" }); + const prepared = await preparedPromise; + expect(child.sent).toEqual([]); + + const exit = prepared.run(); + expect(child.sent).toEqual([{ protocol: "hunk-terminal-handoff-v1", kind: "release" }]); + child.exitCode = 0; + child.emit("exit", 0, null); + expect(await exit).toBe(0); + }); + + test("observes a signalled exit that happens after readiness but before run", async () => { + const child = createTestChild(); + const preparedPromise = prepareHistoryReview( + createTestRuntime(), + { kind: "revision-show", revisionId: "racy" }, + { + current: { command: "hunk", args: [] }, + spawnImpl: (() => child) as unknown as typeof spawn, + env: {}, + }, + ); + + child.emit("message", { protocol: "hunk-terminal-handoff-v1", kind: "ready" }); + const prepared = await preparedPromise; + child.signalCode = "SIGTERM"; + child.connected = false; + child.emit("exit", null, "SIGTERM"); + expect(await prepared.run()).toBe(1); + }); + + test("surfaces bounded child bootstrap failures without releasing the terminal", async () => { + const child = createTestChild(); + const prepared = prepareHistoryReview( + createTestRuntime(), + { kind: "revision-show", revisionId: "missing" }, + { + current: { command: "hunk", args: [] }, + spawnImpl: (() => child) as unknown as typeof spawn, + env: {}, + }, + ); + child.emit("message", { + protocol: "hunk-terminal-handoff-v1", + kind: "failed", + message: "provider could not resolve revision", + }); + await expect(prepared).rejects.toThrow("provider could not resolve revision"); + expect(child.sent).toEqual([]); + expect(child.signalCode).toBe("SIGTERM"); + }); + + test("escalates when a timed-out child ignores graceful termination", async () => { + const child = createTestChild({ ignoreTerm: true }); + const prepared = prepareHistoryReview( + createTestRuntime(), + { kind: "revision-show", revisionId: "slow" }, + { + current: { command: "hunk", args: [] }, + spawnImpl: (() => child) as unknown as typeof spawn, + env: {}, + readyTimeoutMs: 5, + terminateGraceMs: 5, + }, + ); + await expect(prepared).rejects.toThrow("Timed out while preparing"); + expect(child.signalCode).toBe("SIGKILL"); + }); +}); diff --git a/src/ui/log/reviewLaunch.ts b/src/ui/log/reviewLaunch.ts index d0d60a39e..f982678d9 100644 --- a/src/ui/log/reviewLaunch.ts +++ b/src/ui/log/reviewLaunch.ts @@ -1,22 +1,67 @@ -import { spawn } from "node:child_process"; +import { spawn, type ChildProcess } from "node:child_process"; import { resolve } from "node:path"; import { resolveCurrentHunkCommand } from "../../core/process/relaunch"; +import { + parseTerminalHandoffMessage, + terminalHandoffEnv, + terminalHandoffMessage, +} from "../../core/process/terminalHandoff"; import type { ExtensionVcsHistoryReviewAction } from "../../extension-api/types"; import type { HistoryRuntime } from "../history/types"; +const DEFAULT_READY_TIMEOUT_MS = 30_000; +const MAX_BOOTSTRAP_ERROR_BYTES = 16_384; + +export interface PreparedHistoryReview { + /** Release exclusive terminal ownership and wait for the child review to exit. */ + run(): Promise; + /** Stop a child that never received terminal ownership. */ + abort(): Promise; +} + /** Convert a provider-owned review declaration into one option-safe child invocation. */ export function historyReviewArgs(action: ExtensionVcsHistoryReviewAction) { const payload = Buffer.from(JSON.stringify(action), "utf8").toString("base64url"); return [action.kind === "revision-range" ? "diff" : "show", "--history-review", payload]; } -/** Run one provider-planned child Hunk review after the log renderer yields the terminal. */ -export async function launchHistoryReview( +/** Collect bounded bootstrap diagnostics without allowing startup output to disturb the log UI. */ +function collectBootstrapError(child: ChildProcess) { + let output = ""; + child.stderr?.setEncoding("utf8"); + child.stderr?.on("data", (chunk: string) => { + if (output.length >= MAX_BOOTSTRAP_ERROR_BYTES) return; + output += chunk.slice(0, MAX_BOOTSTRAP_ERROR_BYTES - output.length); + }); + return () => output.trim(); +} + +/** Spawn and bootstrap one provider-planned review while the log still owns the terminal. */ +export async function prepareHistoryReview( runtime: HistoryRuntime, action: ExtensionVcsHistoryReviewAction, - themeId?: string, -) { - const current = resolveCurrentHunkCommand(); + { + themeId, + themeMode, + signal, + readyTimeoutMs = DEFAULT_READY_TIMEOUT_MS, + terminateGraceMs = 1_000, + spawnImpl = spawn, + current = resolveCurrentHunkCommand(), + env = process.env, + stderr = process.stderr, + }: { + themeId?: string; + themeMode?: "dark" | "light"; + signal?: AbortSignal; + readyTimeoutMs?: number; + terminateGraceMs?: number; + spawnImpl?: typeof spawn; + current?: ReturnType; + env?: NodeJS.ProcessEnv; + stderr?: NodeJS.WritableStream; + } = {}, +): Promise { const extensionArgs = runtime.input.extensionPaths.flatMap((path) => [ "--extension", resolve(path), @@ -29,13 +74,110 @@ export async function launchHistoryReview( ...(themeId ? ["--theme", themeId] : []), ...(runtime.input.extensionsEnabled ? extensionArgs : ["--no-extensions"]), ]; - const child = spawn(current.command, args, { + const child = spawnImpl(current.command, args, { cwd: runtime.repoRoot, - env: { ...process.env, HUNK_RETURN_TO_HISTORY: "1" }, - stdio: "inherit", + env: terminalHandoffEnv({ ...env, HUNK_RETURN_TO_HISTORY: "1" }, themeMode), + stdio: ["inherit", "inherit", "pipe", "ipc"], }); - return await new Promise((resolveExit, reject) => { - child.once("error", reject); - child.once("exit", (code, signal) => resolveExit(signal ? 1 : (code ?? 1))); + const bootstrapError = collectBootstrapError(child); + const exitResult = new Promise<{ code: number; error?: Error }>((resolveExit) => { + let settled = false; + const finish = (result: { code: number; error?: Error }) => { + if (settled) return; + settled = true; + resolveExit(result); + }; + child.once("error", (error) => finish({ code: 1, error })); + child.once("exit", (code, exitSignal) => finish({ code: exitSignal ? 1 : (code ?? 1) })); }); + const childRunning = () => child.exitCode === null && child.signalCode === null; + const terminateChild = async () => { + if (!childRunning()) return; + child.kill("SIGTERM"); + const stopped = await Promise.race([ + exitResult.then(() => true), + new Promise((resolveTimeout) => { + const timer = setTimeout(() => resolveTimeout(false), terminateGraceMs); + timer.unref?.(); + }), + ]); + if (!stopped && childRunning()) { + child.kill("SIGKILL"); + await exitResult; + } + }; + + try { + await new Promise((resolveReady, rejectReady) => { + let settled = false; + const finish = (error?: Error) => { + if (settled) return; + settled = true; + clearTimeout(timeout); + child.off("message", onMessage); + child.off("error", onError); + child.off("exit", onExit); + signal?.removeEventListener("abort", onAbort); + if (error) rejectReady(error); + else resolveReady(); + }; + const onMessage = (value: unknown) => { + const message = parseTerminalHandoffMessage(value); + if (message?.kind === "ready") finish(); + if (message?.kind === "failed") finish(new Error(message.message)); + }; + const onError = (error: Error) => finish(error); + const onExit = () => + finish(new Error(bootstrapError() || "The review exited before it was ready.")); + const onAbort = () => finish(new Error("Review launch was cancelled.")); + const timeout = setTimeout( + () => finish(new Error("Timed out while preparing the selected commit.")), + readyTimeoutMs, + ); + timeout.unref?.(); + child.on("message", onMessage); + child.once("error", onError); + child.once("exit", onExit); + signal?.addEventListener("abort", onAbort, { once: true }); + }); + } catch (error) { + await terminateChild(); + throw error; + } + + let released = false; + const waitForExit = async () => { + const result = await exitResult; + if (result.error) throw result.error; + return result.code; + }; + + return { + async run() { + if (released || !childRunning()) return await waitForExit(); + child.stderr?.pipe(stderr); + try { + await new Promise((resolveRelease, rejectRelease) => { + if (!child.connected) { + rejectRelease(new Error("The review disconnected before terminal release.")); + return; + } + child.send(terminalHandoffMessage("release"), (error) => { + if (error) rejectRelease(error); + else resolveRelease(); + }); + }); + released = true; + } catch (error) { + if (!childRunning()) return await waitForExit(); + await terminateChild(); + throw error; + } + return await waitForExit(); + }, + async abort() { + if (released || !childRunning()) return; + await terminateChild(); + }, + }; } diff --git a/src/ui/log/runInteractiveLog.tsx b/src/ui/log/runInteractiveLog.tsx index 7de12522d..518d7b532 100644 --- a/src/ui/log/runInteractiveLog.tsx +++ b/src/ui/log/runInteractiveLog.tsx @@ -14,7 +14,7 @@ import { } from "../../core/process/terminal"; import { LogApp, type LogAppOutcome } from "./LogApp"; import { LogController } from "./controller"; -import { launchHistoryReview } from "./reviewLaunch"; +import { prepareHistoryReview, type PreparedHistoryReview } from "./reviewLaunch"; import type { HistoryRuntime } from "../history/types"; import { interactiveLogUsesColor } from "./colorPolicy"; @@ -28,7 +28,11 @@ export function logSignalExitCode(signal: NodeJS.Signals) { return signal === "SIGINT" ? 130 : signal === "SIGHUP" ? 129 : 143; } -/** Mount one OpenTUI log surface and resolve only after it requests quit or review. */ +type MountedLogOutcome = + | Extract + | { kind: "open-review"; launch: PreparedHistoryReview }; + +/** Mount one OpenTUI log surface and keep it visible while the selected review bootstraps. */ async function mountLogSurface( controller: LogController, runtime: HistoryRuntime, @@ -52,15 +56,35 @@ async function mountLogSurface( throw error; } let settled = false; - let settle!: (outcome: LogAppOutcome) => void; - const outcome = new Promise((resolve) => { + let settle!: (outcome: MountedLogOutcome) => void; + const outcome = new Promise((resolve) => { settle = resolve; }); - const finish = (value: LogAppOutcome) => { + const launchAbort = new AbortController(); + let preparedLaunch: PreparedHistoryReview | undefined; + const finish = (value: MountedLogOutcome) => { if (settled) return; settled = true; settle(value); }; + const handleOutcome = async (value: LogAppOutcome) => { + if (value.kind === "quit") { + launchAbort.abort(); + finish(value); + return; + } + const launch = await prepareHistoryReview(runtime, value.action, { + themeId: value.themeId, + themeMode: value.themeMode, + signal: launchAbort.signal, + }); + if (settled) { + await launch.abort(); + return; + } + preparedLaunch = launch; + finish({ kind: "open-review", launch }); + }; const requestQuit = () => finish({ kind: "quit" }); const requestInterrupt = () => finish({ kind: "quit", exitCode: 130 }); const signalHandlers = new Map void>( @@ -86,11 +110,12 @@ async function mountLogSurface( controller={controller} runtime={runtime} useColor={interactiveLogUsesColor(runtime.input.color, process.env)} - onOutcome={finish} + onOutcome={handleOutcome} />, ); return await outcome; } finally { + if (!preparedLaunch) launchAbort.abort(); for (const [signal, handler] of signalHandlers) process.off(signal, handler); interrupt.dispose(); suspend.dispose(); @@ -124,7 +149,7 @@ export async function runInteractiveLog( return; } try { - const code = await launchHistoryReview(runtime, outcome.action, outcome.themeId); + const code = await outcome.launch.run(); controller.setNotice(code === 0 ? "" : "Could not open the selected commit."); } catch (error) { controller.setNotice(error instanceof Error ? error.message : String(error)); diff --git a/test/pty/log-integration.test.ts b/test/pty/log-integration.test.ts index be633c624..8e1528b97 100644 --- a/test/pty/log-integration.test.ts +++ b/test/pty/log-integration.test.ts @@ -108,6 +108,10 @@ describe("interactive hunk log", () => { session.writeRaw( `\x1b[<0;${commitColumn + 1};${firstRowIndex + 1}M\x1b[<0;${commitColumn + 1};${firstRowIndex + 1}m`, ); + const preparing = await session.waitForText(/Opening commit[\s\S]*Preparing review…/, { + timeout: 5_000, + }); + expect(preparing).not.toContain("historyValue = 'second'"); const review = await session.waitForText(/historyValue = 'second'/, { timeout: 15_000, });