diff --git a/.changeset/cancellable-shared-history-extensions.md b/.changeset/cancellable-shared-history-extensions.md new file mode 100644 index 000000000..7806a652b --- /dev/null +++ b/.changeset/cancellable-shared-history-extensions.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Keep commit reviews responsive while bundled VCS commands run, and share one correctly owned extension lifecycle across retained history and embedded reviews. diff --git a/.changeset/persistent-history-review.md b/.changeset/persistent-history-review.md new file mode 100644 index 000000000..8eff3a6d3 --- /dev/null +++ b/.changeset/persistent-history-review.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Keep `hunk log` and opened commit reviews in one terminal renderer so returning never exposes previous terminal output. diff --git a/docs/extensions.md b/docs/extensions.md index d4844dd62..60af74dc1 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -278,6 +278,22 @@ start watchers, processes, connections, and other long-lived resources from `startup`, and release them from `shutdown`. Extension-registry reloads create new instances and run that shutdown/startup pair around the replacement. +An interactive history workspace owns one extension instance for its complete +lifetime. Opening a commit review inside that workspace borrows the same +instance: the factory and `startup` do not run again, and returning to history +does not send `shutdown`. Review-specific `changeset_loaded` events still run +for each opened commit. The owning history workspace sends the one eventual +`shutdown` when it exits. This makes module-local clients and stores safe to +share deliberately between retained history and its commit reviews without a +review closing resources that history still uses. + +Keep mutable review-generation data keyed by the identities in event payloads +or replace it on `changeset_loaded`; module scope is workspace state, not a +fresh namespace per opened commit. An embedded review cannot replace its +borrowed registry; extension replacement remains an operation of the owning +workspace. A true owner-driven extension-registry reload creates a new instance +and retires the replaced instance at that explicit ownership boundary. + ### `hunk.apiVersion` The API generation this Hunk speaks (currently `19`). Branch on it if you want @@ -562,6 +578,12 @@ reports it as unsupported. Jujutsu supplies commit/change identities, bookmarks, and native merge-review semantics without routing through a colocated Git repository. Third-party adapters use exactly the same contract. +Every operation `load` receives `context.signal`. Use asynchronous subprocess +APIs, pass cancellation through, and terminate plus reap provider processes when +it aborts; a synchronous spawn blocks Hunk's renderer and prevents the abort +handler from running. Watch signatures remain synchronous because the watch +runtime calls them as short, noninteractive probes. + A `load` result is patch text plus how to label it. Everything else on it is optional, and each optional field buys one thing: diff --git a/src/app/extensionBootstrap.test.ts b/src/app/extensionBootstrap.test.ts index 692d87fa7..1a45aee54 100644 --- a/src/app/extensionBootstrap.test.ts +++ b/src/app/extensionBootstrap.test.ts @@ -1,6 +1,9 @@ import { describe, expect, test } from "bun:test"; import type { HunkConfigResolution } from "../core/run/config"; import type { CliInput } from "../core/run/commandInputs"; +import type { HunkExtensionAPI } from "../extension-api/types"; +import { emitExtensionEvent, retireExtensionLoadResult } from "../extensions/events"; +import { loadExtensions } from "../extensions/host"; import { createEmptyExtensionLoadResult } from "../extensions/types"; import { resolveConfiguredExtensions } from "./extensionBootstrap"; import { getBundledVcsCatalog } from "./vcsCatalog"; @@ -16,6 +19,63 @@ function createTestConfig(input: CliInput): HunkConfigResolution { } describe("resolveConfiguredExtensions", () => { + test("borrows one stateful extension instance across repeated review generations", async () => { + const input: CliInput = { kind: "show", ref: "opaque", options: { vcs: "git" } }; + const state = { factories: 0, startups: 0, changesets: 0, shutdowns: 0 }; + const borrowed = await loadExtensions({ + candidates: [{ id: "stateful", path: "/repo/stateful.ts", origin: "flag" }], + cwd: "/repo", + importExtensionModuleImpl: async () => ({ + default(hunk: HunkExtensionAPI) { + state.factories += 1; + hunk.on("startup", () => { + state.startups += 1; + }); + hunk.on("changeset_loaded", () => { + state.changesets += 1; + }); + hunk.on("shutdown", () => { + state.shutdowns += 1; + }); + }, + }), + }); + emitExtensionEvent(borrowed, "startup", { cwd: "/repo" }); + let loaderCalls = 0; + + for (let generation = 0; generation < 2; generation += 1) { + const resolved = await resolveConfiguredExtensions( + { + runtimeInput: input, + configured: createTestConfig(input), + cwd: "/repo", + baseVcsCatalog: getBundledVcsCatalog(), + borrowedLoad: borrowed, + }, + { + loadStartupExtensionsImpl: async () => { + loaderCalls += 1; + throw new Error("borrowed extensions must not reload"); + }, + }, + ); + expect(resolved.extensions).toBe(borrowed); + emitExtensionEvent(resolved.extensions, "changeset_loaded", { + changeset: { + id: `generation-${generation}`, + title: "Stateful review", + files: [], + sourceLabel: `generation-${generation}`, + }, + }); + } + + expect(state).toEqual({ factories: 1, startups: 1, changesets: 2, shutdowns: 0 }); + expect(loaderCalls).toBe(0); + await retireExtensionLoadResult(borrowed); + expect(state.shutdowns).toBe(1); + }); + test("retires provisional authority when the loader rejects before returning it", async () => { const input: CliInput = { kind: "vcs", staged: false, options: { vcs: "git" } }; const provisional = createEmptyExtensionLoadResult("/repo"); diff --git a/src/app/extensionBootstrap.ts b/src/app/extensionBootstrap.ts index 15adc2d84..d833f9de6 100644 --- a/src/app/extensionBootstrap.ts +++ b/src/app/extensionBootstrap.ts @@ -21,6 +21,8 @@ export interface ResolveConfiguredExtensionsOptions { notifications?: ExtensionNotificationHub; /** Registry already loaded by an extension CLI command before built-in delegation. */ previousLoad?: ExtensionLoadResult; + /** Session-owned registry borrowed by an embedded surface without lifecycle authority. */ + borrowedLoad?: ExtensionLoadResult; /** Publish provisional ownership before imports or asynchronous factories can suspend. */ onProvisionalLoad?: (result: ExtensionLoadResult) => void; /** Throw when the caller's lifetime ended so no later staged registry can be created. */ @@ -59,6 +61,24 @@ export async function resolveConfiguredExtensions( vcsCatalog: options.discoveryCatalog ?? options.baseVcsCatalog, }); + if (options.borrowedLoad) { + const adapters = resolveExtensionVcsAdapters( + options.borrowedLoad.registry, + options.baseVcsCatalog, + ).adapters; + const catalog = extendVcsCatalog(options.baseVcsCatalog, adapters); + const projectRoot = findProjectRootCandidateImpl(options.cwd, catalog); + if (projectRoot !== configured.projectRoot) { + configured = resolveConfiguredCliInputImpl(options.runtimeInput, { + cwd: options.cwd, + env: options.env, + vcsCatalog: catalog, + }); + } + options.assertActive?.(); + return { configured, extensions: options.borrowedLoad }; + } + let extensions: ExtensionLoadResult | undefined; let provisionalExtensions: ExtensionLoadResult | undefined; /** Retain loader ownership locally before forwarding it to a caller that may also suspend. */ diff --git a/src/app/historyBootstrap.ts b/src/app/historyBootstrap.ts index bd4372416..1e362c940 100644 --- a/src/app/historyBootstrap.ts +++ b/src/app/historyBootstrap.ts @@ -28,8 +28,11 @@ export interface HistoryBootstrap { source: VcsHistorySource; providerId: string; providerName: string; + startupCwd: string; repoRoot: string; extensions: ExtensionLoadResult; + /** History-owned extension authority borrowed by embedded reviews. */ + extensionSession: ExtensionLoadResult; notices: readonly string[]; customThemes: readonly NamedCustomThemeConfig[]; planReview( @@ -129,8 +132,10 @@ export async function loadHistoryBootstrap({ source, providerId: sanitizeTerminalLine(adapter.id), providerName: sanitizeTerminalLine(adapter.name), + startupCwd: cwd, repoRoot, extensions: resolved.extensions, + extensionSession: resolved.extensions, customThemes: sessionThemes.themes, notices: [ ...(mergeStartupNotices(resolved.configured.startupNotices, resolved.extensions) ?? []).map( diff --git a/src/app/session/registration.test.ts b/src/app/session/registration.test.ts index 1e119b635..6f3d41b3f 100644 --- a/src/app/session/registration.test.ts +++ b/src/app/session/registration.test.ts @@ -211,6 +211,12 @@ describe("session registration", () => { // Intent: the daemon can address every resource of the generation it mirrors, and can // map the renderer file ids the session surface uses onto the semantic keys resources // are addressed by. + test("createSessionRegistration accepts the owning surface's authoritative cwd", () => { + const bootstrap = createBootstrap(); + const registration = createSessionRegistration(bootstrap, publish(bootstrap), "embedded-cwd"); + expect(registration.cwd).toBe("embedded-cwd"); + }); + test("createSessionRegistration advertises the generation's resource catalog", () => { const bootstrap = createBootstrap(); const publication = publish(bootstrap); diff --git a/src/app/session/registration.ts b/src/app/session/registration.ts index bacf67637..748a5c14a 100644 --- a/src/app/session/registration.ts +++ b/src/app/session/registration.ts @@ -81,6 +81,7 @@ function buildReviewCatalog(publication: ReviewPublication): HunkReviewResourceC export function createSessionRegistration( bootstrap: AppBootstrap, publication: ReviewPublication, + cwd = process.cwd(), ): HunkSessionRegistration { const terminal = resolveSessionTerminalMetadata({ tty: ttyname() }); @@ -88,7 +89,7 @@ export function createSessionRegistration( registrationVersion: SESSION_BROKER_REGISTRATION_VERSION, sessionId: randomUUID(), pid: process.pid, - cwd: process.cwd(), + cwd, repoRoot: inferRepoRoot(bootstrap), launchedAt: new Date().toISOString(), terminal, diff --git a/src/app/sessionBootstrap.ts b/src/app/sessionBootstrap.ts index eb36b7ed7..25ad868b5 100644 --- a/src/app/sessionBootstrap.ts +++ b/src/app/sessionBootstrap.ts @@ -30,6 +30,8 @@ export interface SessionBootstrapOptions { loadAppBootstrapImpl?: typeof loadAppBootstrap; /** Base product adapters composed before user extensions are applied. */ baseVcsCatalog?: VcsCatalog; + /** Cancel initial provider-backed loading for an abandoned embedded surface. */ + signal?: AbortSignal; } export interface SessionBootstrapResult { @@ -57,7 +59,9 @@ export async function loadConfiguredSessionBootstrap({ loadAtCwd = false, loadAppBootstrapImpl = loadAppBootstrap, baseVcsCatalog = getBundledVcsCatalog(), + signal, }: SessionBootstrapOptions): Promise { + signal?.throwIfAborted(); const previousFileLanguages = fileLanguageRegistrationSnapshot(); try { @@ -84,8 +88,11 @@ export async function loadConfiguredSessionBootstrap({ ...(loadAtCwd ? { cwd } : {}), customThemes: sessionThemes.themes, vcsCatalog: applied.vcsCatalog, + signal, })) as AppBootstrap; + signal?.throwIfAborted(); bootstrap.changeset = await applyExtensionChangesetTransforms(extensions, bootstrap.changeset); + signal?.throwIfAborted(); bootstrap.initialThemeMode = initialThemeMode ?? bootstrap.initialThemeMode; bootstrap.extensions = extensions; bootstrap.viewPreferencesConfigPath = configured.viewPreferencesConfigPath; diff --git a/src/app/startup.test.ts b/src/app/startup.test.ts index ed0edf260..bc6edbcc1 100644 --- a/src/app/startup.test.ts +++ b/src/app/startup.test.ts @@ -749,7 +749,7 @@ describe("startup planning", () => { expect(opened).toBe(1); }); - test("inherits handoff theme mode without querying the parent-owned terminal", async () => { + test("inherits an embedded renderer theme mode without querying the shared terminal", async () => { const cliInput: CliInput = { kind: "show", ref: "opaque:id", @@ -768,10 +768,7 @@ describe("startup planning", () => { }, stdinIsTTY: true, stdoutIsTTY: true, - env: { - HUNK_TERMINAL_HANDOFF: "1", - HUNK_TERMINAL_HANDOFF_THEME_MODE: "dark", - }, + terminalThemeMode: "dark", }); expect(plan).toMatchObject({ kind: "app", bootstrap: { initialThemeMode: "dark" } }); diff --git a/src/app/startup.ts b/src/app/startup.ts index 48f8c0cab..0459ca847 100644 --- a/src/app/startup.ts +++ b/src/app/startup.ts @@ -5,7 +5,6 @@ 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 { @@ -136,6 +135,14 @@ export interface StartupDeps { runExtensionCliCommandImpl?: typeof import("../extensions/cliCommandRuntime").runExtensionCliCommand; env?: NodeJS.ProcessEnv; bunVersion?: string; + /** Override process cwd for an embedded review bootstrap. */ + cwd?: string; + /** Reuse the owning renderer's detected terminal mode. */ + terminalThemeMode?: "dark" | "light"; + /** Cancel provider-backed startup for an abandoned embedded surface. */ + signal?: AbortSignal; + /** Borrow the owning history session's already-loaded extension authority. */ + borrowedExtensionLoad?: import("../extensions/types").ExtensionLoadResult; } /** Carry the invocation's authoritative extension paths into a delegated review input. */ @@ -197,17 +204,20 @@ export async function prepareStartupPlan( const env = deps.env ?? process.env; const bunVersion = deps.bunVersion ?? Bun.version; const loadBaseVcsCatalog = createBundledVcsCatalogLoader(); - const startupCwd = process.cwd(); + const startupCwd = deps.cwd ?? process.cwd(); + deps.signal?.throwIfAborted(); let parsedCliInput = await parseCliImpl(argv); let controllingTerminal: ControllingTerminal | null = null; - let preloadedExtensions: import("../extensions/types").ExtensionLoadResult | undefined; + let preloadedExtensions: import("../extensions/types").ExtensionLoadResult | undefined = + deps.borrowedExtensionLoad; + const ownsPreloadedExtensions = !deps.borrowedExtensionLoad; let delegatedDiscoveryCatalog: VcsCatalog | undefined; let delegatedReview: ExtensionReviewDescriptor | undefined; /** Retire startup-owned extension state before returning a non-app plan. */ const retirePreloadedExtensions = async () => { - if (!preloadedExtensions) return; + if (!preloadedExtensions || !ownsPreloadedExtensions) return; await (await import("../extensions/events")).retireExtensionLoadResult(preloadedExtensions); preloadedExtensions = undefined; }; @@ -544,9 +554,9 @@ export async function prepareStartupPlan( controllingTerminal = openControllingTerminalImpl(); } - // 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); + // Embedded reviews inherit their owner's detected mode so bootstrap never queries a terminal + // whose input and renderer are already exclusively owned. + let initialThemeMode: AppBootstrap["initialThemeMode"] = deps.terminalThemeMode; if (!initialThemeMode && cliInput.options.theme === "auto" && stdoutIsTTY) { const themeInput = controllingTerminal?.stdin ?? (stdinIsTTY ? process.stdin : null); if (themeInput) { @@ -586,7 +596,9 @@ export async function prepareStartupPlan( env, baseVcsCatalog, discoveryCatalog: delegatedDiscoveryCatalog, - previousLoad: preloadedExtensions, + previousLoad: deps.borrowedExtensionLoad ? undefined : preloadedExtensions, + borrowedLoad: deps.borrowedExtensionLoad, + assertActive: () => deps.signal?.throwIfAborted(), }, { resolveConfiguredCliInputImpl, loadStartupExtensionsImpl }, ); @@ -594,6 +606,10 @@ export async function prepareStartupPlan( cliInput = configured.input; const extensionResult = resolvedExtensions.extensions; preloadedExtensions = extensionResult; + if (deps.signal?.aborted) { + await retirePreloadedExtensions(); + deps.signal.throwIfAborted(); + } let preparedSession: SessionBootstrapResult; try { @@ -604,6 +620,7 @@ export async function prepareStartupPlan( initialThemeMode, loadAppBootstrapImpl, baseVcsCatalog, + signal: deps.signal, }); } catch (error) { controllingTerminal?.close(); diff --git a/src/core/changeset/loaders.test.ts b/src/core/changeset/loaders.test.ts index 9950f19da..adf10ce2d 100644 --- a/src/core/changeset/loaders.test.ts +++ b/src/core/changeset/loaders.test.ts @@ -198,6 +198,14 @@ afterEach(() => { }); describe("loadAppBootstrap", () => { + test("refuses an already-cancelled embedded review load", async () => { + const abort = new AbortController(); + abort.abort(); + await expect( + loadAppBootstrap({ kind: "show", ref: "opaque", options: {} }, { signal: abort.signal }), + ).rejects.toThrow(); + }); + test("synthesizes untracked file diffs an adapter reported by path", async () => { const dir = createTempDir("hunk-adapter-untracked-"); writeFileSync(join(dir, "note.txt"), "hello\n"); @@ -1932,8 +1940,9 @@ describe("loadAppBootstrap source fetcher attachment", () => { mutableBun.spawn = originalSpawn; } - expect(syncCalls.some((call) => call.includes("rev-parse"))).toBe(true); - expect(syncCalls.some((call) => call.includes("diff"))).toBe(true); + expect(syncCalls).toEqual([]); + expect(asyncCalls.some((call) => call.includes("rev-parse"))).toBe(true); + expect(asyncCalls.some((call) => call.includes("diff"))).toBe(true); expect(asyncCalls).toContainEqual([gitExecutable, "show", ":value.txt"]); }); diff --git a/src/core/changeset/loaders.ts b/src/core/changeset/loaders.ts index aece09bc4..ff17a33d4 100644 --- a/src/core/changeset/loaders.ts +++ b/src/core/changeset/loaders.ts @@ -45,6 +45,8 @@ export interface LoadAppBootstrapOptions { customThemes?: readonly NamedCustomThemeConfig[]; /** Complete adapter catalog composed by the app for this session. */ vcsCatalog?: VcsCatalog; + /** Cancel provider-backed review loading before it mutates mounted state. */ + signal?: AbortSignal; } /** Return the final path segment for display-oriented labels. */ @@ -216,10 +218,11 @@ async function loadVcsChangeset( sidecar: SidecarContext | null, cwd: string, vcsCatalog: VcsCatalog, + signal?: AbortSignal, ) { const adapter = getConfiguredVcsAdapter(input.options.vcs, vcsCatalog); const operation = operationFromInput(input); - const result = await loadVcsReview(adapter, operation, { cwd }, vcsCatalog); + const result = await loadVcsReview(adapter, operation, { cwd, signal }, vcsCatalog); const parsedChangeset = changesetFromPatch( result.patchText, result.title, @@ -274,8 +277,9 @@ async function loadPatchChangeset( /** Resolve CLI input into the fully loaded app bootstrap state. */ export async function loadAppBootstrap( input: CliInput, - { cwd = process.cwd(), customThemes, vcsCatalog }: LoadAppBootstrapOptions = {}, + { cwd = process.cwd(), customThemes, vcsCatalog, signal }: LoadAppBootstrapOptions = {}, ): Promise { + signal?.throwIfAborted(); // Capture before loading content so watch mode can detect mutations that race initial loading. let initialWatchSignature: string | undefined; if (input.options.watch) { @@ -289,6 +293,7 @@ export async function loadAppBootstrap( } const sidecar = await loadSidecarContext(input.options.agentContext, { cwd }); + signal?.throwIfAborted(); let changeset: Changeset; let repoRoot: string | undefined; @@ -301,7 +306,7 @@ export async function loadAppBootstrap( if (!vcsCatalog) { throw new Error("VCS-backed reviews require a composed VCS catalog."); } - const result = await loadVcsChangeset(input, sidecar, cwd, vcsCatalog); + const result = await loadVcsChangeset(input, sidecar, cwd, vcsCatalog, signal); changeset = result.changeset; repoRoot = result.repoRoot; } @@ -317,6 +322,7 @@ export async function loadAppBootstrap( break; } + signal?.throwIfAborted(); changeset = { ...changeset, files: orderDiffFiles(changeset.files, sidecar), diff --git a/src/core/process/terminalHandoff.test.ts b/src/core/process/terminalHandoff.test.ts deleted file mode 100644 index 08753d939..000000000 --- a/src/core/process/terminalHandoff.test.ts +++ /dev/null @@ -1,45 +0,0 @@ -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 deleted file mode 100644 index 0f8cc3956..000000000 --- a/src/core/process/terminalHandoff.ts +++ /dev/null @@ -1,116 +0,0 @@ -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/extension-api/types.ts b/src/extension-api/types.ts index 9f4ccb4f5..f716e31a4 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -667,6 +667,8 @@ export interface ExtensionVcsDetection { /** Ambient information an operation may need to shell out. */ export interface ExtensionVcsLoadContext { cwd: string; + /** Abort provider setup or work when the owning host generation ends. */ + signal?: AbortSignal; } /** diff --git a/src/extensions/default/vcs/asyncProcess.test.ts b/src/extensions/default/vcs/asyncProcess.test.ts new file mode 100644 index 000000000..b95d2231d --- /dev/null +++ b/src/extensions/default/vcs/asyncProcess.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, test } from "bun:test"; +import { runAbortableCommand } from "./asyncProcess"; + +describe("abortable bundled VCS subprocesses", () => { + test("does not spawn after cancellation already won", async () => { + const abort = new AbortController(); + abort.abort(new Error("cancelled before spawn")); + await expect( + runAbortableCommand([process.execPath, "-e", "process.exit(99)"], { + cwd: process.cwd(), + signal: abort.signal, + }), + ).rejects.toThrow("cancelled before spawn"); + }); + + test("terminates, escalates, and reaps a command that ignores graceful cancellation", async () => { + const abort = new AbortController(); + const startedAt = Date.now(); + const pending = runAbortableCommand( + [ + process.execPath, + "-e", + 'process.on("SIGTERM",()=>{}); process.stdout.write("started\\n"); setTimeout(()=>process.stdout.write("late\\n"),5000)', + ], + { cwd: process.cwd(), signal: abort.signal, terminationGraceMs: 25 }, + ); + setTimeout(() => abort.abort(new Error("provider cancelled")), 20); + await expect(pending).rejects.toThrow("provider cancelled"); + expect(Date.now() - startedAt).toBeLessThan(2_000); + }); + + test("collects output and exit status on normal completion", async () => { + const result = await runAbortableCommand( + [process.execPath, "-e", 'process.stdout.write("ok"); process.stderr.write("note")'], + { cwd: process.cwd() }, + ); + expect(result).toEqual({ stdout: "ok", stderr: "note", exitCode: 0 }); + }); +}); diff --git a/src/extensions/default/vcs/asyncProcess.ts b/src/extensions/default/vcs/asyncProcess.ts new file mode 100644 index 000000000..6dce708f9 --- /dev/null +++ b/src/extensions/default/vcs/asyncProcess.ts @@ -0,0 +1,98 @@ +const DEFAULT_TERMINATION_GRACE_MS = 250; + +export interface AsyncCommandResult { + stdout: string; + stderr: string; + exitCode: number; +} + +/** Run one provider command without blocking renderer input and reap it after cancellation. */ +export async function runAbortableCommand( + command: string[], + { + cwd, + env, + signal, + terminationGraceMs = DEFAULT_TERMINATION_GRACE_MS, + }: { + cwd: string; + env?: Record; + signal?: AbortSignal; + terminationGraceMs?: number; + }, +): Promise { + signal?.throwIfAborted(); + const ownsProcessGroup = process.platform !== "win32"; + const proc = Bun.spawn(command, { + cwd, + env, + detached: ownsProcessGroup, + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + }); + + let killTimer: ReturnType | undefined; + let terminating = false; + const treeTerminationTasks: Promise[] = []; + const kill = (signal: "SIGTERM" | "SIGKILL") => { + if (ownsProcessGroup) { + try { + process.kill(-proc.pid, signal); + return; + } catch { + // Fall back when the child exited before its process group was signalled. + } + } + if (process.platform === "win32") { + // Bun cannot signal a Windows process group. taskkill owns the complete descendant + // tree so helpers that inherited our pipes cannot keep stream collection pending. + const task = Bun.spawn( + ["taskkill", "/pid", String(proc.pid), "/t", ...(signal === "SIGKILL" ? ["/f"] : [])], + { stdin: "ignore", stdout: "ignore", stderr: "ignore" }, + ); + treeTerminationTasks.push(task.exited.catch(() => undefined)); + return; + } + proc.kill(signal); + }; + const abort = () => { + if (terminating) return; + terminating = true; + try { + kill("SIGTERM"); + } catch { + // The process may already have exited between the abort and this handler. + } + killTimer = setTimeout(() => { + try { + kill("SIGKILL"); + } catch { + // Reaping below remains authoritative when the process already exited. + } + }, terminationGraceMs); + killTimer.unref?.(); + }; + signal?.addEventListener("abort", abort, { once: true }); + // Close the race between the pre-spawn check and listener registration. + if (signal?.aborted) abort(); + + try { + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + signal?.throwIfAborted(); + return { stdout, stderr, exitCode }; + } catch (error) { + if (signal?.aborted) signal.throwIfAborted(); + throw error; + } finally { + signal?.removeEventListener("abort", abort); + if (killTimer) clearTimeout(killTimer); + // `proc.exited` also reaps a child terminated during stream collection. Windows + // tree-kill helpers are awaited as well so cancellation leaves no owned processes. + await Promise.all([proc.exited.catch(() => undefined), ...treeTerminationTasks]); + } +} diff --git a/src/extensions/default/vcs/git/commands.ts b/src/extensions/default/vcs/git/commands.ts index 161554a8e..82d8ee873 100644 --- a/src/extensions/default/vcs/git/commands.ts +++ b/src/extensions/default/vcs/git/commands.ts @@ -9,6 +9,7 @@ import { import { LARGE_DIFF_FILE_MAX_BYTES, LARGE_DIFF_FILE_MAX_LINES } from "../../../../lib/largeFile"; import { normalizePathForOS } from "../../../../lib/osPath"; import { describeDiffRange, describeDiffTargets } from "../diffRange"; +import { runAbortableCommand } from "../asyncProcess"; /** * Every Git command Hunk runs, and the failures they translate into. @@ -31,6 +32,7 @@ export interface RunGitTextOptions { cwd?: string; gitExecutable?: string; preventOptionalLocks?: boolean; + signal?: AbortSignal; } interface RunGitCommandResult { @@ -513,6 +515,41 @@ export function runGitText(options: RunGitTextOptions) { return runGitCommand(options).stdout; } +/** Run one Git command asynchronously so embedded review preparation remains cancellable. */ +async function runGitCommandAsync({ + input, + args, + cwd = process.cwd(), + gitExecutable = "git", + preventOptionalLocks = false, + signal, + acceptedExitCodes = [0], +}: RunGitCommandOptions): Promise { + let result: Awaited>; + try { + result = await runAbortableCommand([gitExecutable, ...args], { + cwd, + signal, + env: preventOptionalLocks ? { ...process.env, GIT_OPTIONAL_LOCKS: "0" } : undefined, + }); + } catch (error) { + if (signal?.aborted) signal.throwIfAborted(); + throw translateGitSpawnFailure(input, error, gitExecutable); + } + if (!acceptedExitCodes.includes(result.exitCode)) { + throw translateGitExitFailure( + input, + result.stderr.trim() || `Command failed: ${gitExecutable} ${args.join(" ")}`, + ); + } + return result; +} + +/** Run one Git command asynchronously and return its decoded stdout. */ +export async function runGitTextAsync(options: RunGitTextOptions): Promise { + return (await runGitCommandAsync(options)).stdout; +} + const GIT_BOOLEAN_TRUE_VALUES = new Set(["true", "yes", "on", "1", "always"]); const GIT_BOOLEAN_FALSE_VALUES = new Set(["false", "no", "off", "0", "never"]); @@ -579,6 +616,27 @@ export function resolveGitColorMovedOptions( }; } +/** Resolve moved-line configuration without blocking an embedded renderer. */ +export async function resolveGitColorMovedOptionsAsync( + input: GitBackedInput, + options: Omit = {}, +): Promise { + const readConfig = async (key: string) => { + const result = await runGitCommandAsync({ + input, + args: ["config", "--get", key], + ...options, + acceptedExitCodes: [0, 1], + }); + return result.exitCode === 0 ? result.stdout.trim() || undefined : undefined; + }; + const gitMode = normalizeGitColorMovedMode(await readConfig("diff.colorMoved")); + if (gitMode === null) return null; + const mode = gitMode ?? (input.options.colorMoved ? "zebra" : undefined); + if (!mode) return null; + return { mode, whitespaceMode: await readConfig("diff.colorMovedWS") }; +} + /** * Return whether one `hunk diff` input still compares against the live working tree. * @@ -753,6 +811,63 @@ export function listGitUntrackedFiles( ); } +/** Return untracked files without blocking review preparation. */ +export async function listGitUntrackedFilesAsync( + input: ExtensionVcsDiffInput, + { + cwd = process.cwd(), + repoRoot, + gitExecutable = "git", + preventOptionalLocks = false, + signal, + }: Omit & { repoRoot?: string } = {}, +) { + if (input.staged || input.options.excludeUntracked === true) return []; + const range = requireGitDiffRangeArg(input); + if (range) { + const revs = ( + await runGitTextAsync({ + input, + args: ["rev-parse", "--revs-only", requireGitRevisionArg(input, range)], + cwd: repoRoot ?? cwd, + gitExecutable, + preventOptionalLocks, + signal, + }) + ) + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); + if ( + revs.filter((line) => !line.startsWith("^")).length !== 1 || + revs.some((line) => line.startsWith("^")) + ) { + return []; + } + } + const statusText = await runGitTextAsync({ + input, + args: buildGitStatusArgs(input), + cwd, + gitExecutable, + preventOptionalLocks, + signal, + }); + const untrackedFiles = parseUntrackedFilePaths(statusText); + if (untrackedFiles.length === 0) return []; + const normalizedRepoRoot = + repoRoot ?? + (await resolveGitRepoRootAsync(input, { + cwd, + gitExecutable, + preventOptionalLocks, + signal, + })); + return untrackedFiles.filter((filePath) => + isReviewableUntrackedPath(normalizedRepoRoot, filePath), + ); +} + export interface GitMetadata { repoRoot: string; gitDir: string; @@ -992,3 +1107,123 @@ export function resolveGitDiffEndpoints( // from the wrong revision. return null; } + +/** Resolve a Git repository root without blocking renderer input. */ +export async function resolveGitRepoRootAsync( + input: GitBackedInput, + options: Omit = {}, +) { + return normalizePathForOS( + (await runGitTextAsync({ input, args: ["rev-parse", "--show-toplevel"], ...options })).trim(), + ); +} + +/** Resolve one exact commit ref without blocking renderer input. */ +export async function resolveGitCommitRefAsync( + input: GitBackedInput, + ref: string, + options: Omit = {}, +) { + return ( + await runGitTextAsync({ + input, + args: ["rev-parse", "--verify", "--end-of-options", `${ref}^{commit}`], + ...options, + }) + ) + .split("\n")[0]! + .trim(); +} + +/** Resolve old/new Git endpoints asynchronously for review-load source capabilities. */ +export async function resolveGitDiffEndpointsAsync( + input: ExtensionVcsDiffInput, + { + cwd = process.cwd(), + gitExecutable = "git", + repoRoot, + signal, + }: Omit & { repoRoot?: string } = {}, +): Promise { + const range = requireGitDiffRangeArg(input); + const commandCwd = repoRoot ?? cwd; + const resolveRef = (ref: string) => + resolveGitCommitRefAsync(input, ref, { cwd: commandCwd, gitExecutable, signal }); + const resolveRevisions = async (value: string) => { + const revs = ( + await runGitTextAsync({ + input, + args: ["rev-parse", "--revs-only", requireGitRevisionArg(input, value)], + cwd: commandCwd, + gitExecutable, + signal, + }) + ) + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); + return { + positives: revs.filter((rev) => !rev.startsWith("^")), + negatives: revs.filter((rev) => rev.startsWith("^")).map((rev) => rev.slice(1)), + }; + }; + + if (input.staged) { + if (!range) { + const result = await runGitCommandAsync({ + input, + args: ["rev-parse", "--verify", "--end-of-options", "HEAD^{commit}"], + cwd: commandCwd, + gitExecutable, + signal, + acceptedExitCodes: [0, 1, 128], + }); + const headRef = result.exitCode === 0 ? result.stdout.split("\n")[0]!.trim() : null; + if (!headRef && !isUnknownRevisionMessage(result.stderr)) { + throw translateGitExitFailure( + input, + result.stderr.trim() || "Could not resolve Git ref HEAD.", + ); + } + return { + old: headRef ? { kind: "git-ref", ref: headRef } : { kind: "none" }, + new: { kind: "index" }, + }; + } + const { positives, negatives } = await resolveRevisions(range); + return positives.length === 1 && negatives.length === 0 + ? { old: { kind: "git-ref", ref: positives[0]! }, new: { kind: "index" } } + : null; + } + if (!range) return { old: { kind: "index" }, new: { kind: "worktree" } }; + const symmetric = parseSymmetricDiffRange(range); + if (symmetric) { + const mergeBase = ( + await runGitTextAsync({ + input, + args: ["merge-base", symmetric.left, symmetric.right], + cwd: commandCwd, + gitExecutable, + signal, + }) + ) + .split("\n")[0] + ?.trim(); + if (!mergeBase) return null; + return { + old: { kind: "git-ref", ref: mergeBase }, + new: { kind: "git-ref", ref: await resolveRef(symmetric.right) }, + }; + } + const { positives, negatives } = await resolveRevisions(range); + if (positives.length === 1 && negatives.length === 0) { + return { old: { kind: "git-ref", ref: positives[0]! }, new: { kind: "worktree" } }; + } + if (positives.length === 1 && negatives.length === 1) { + return { + old: { kind: "git-ref", ref: negatives[0]! }, + new: { kind: "git-ref", ref: positives[0]! }, + }; + } + return null; +} diff --git a/src/extensions/default/vcs/git/index.ts b/src/extensions/default/vcs/git/index.ts index 9f1cda3b4..a6489daa1 100644 --- a/src/extensions/default/vcs/git/index.ts +++ b/src/extensions/default/vcs/git/index.ts @@ -8,13 +8,17 @@ import { buildGitStashShowArgs, listGitIgnoredDirectoryRoots, listGitUntrackedFiles, + listGitUntrackedFilesAsync, parseGitNumstat, - resolveGitColorMovedOptions, - resolveGitCommitRef, + resolveGitColorMovedOptionsAsync, + resolveGitCommitRefAsync, resolveGitDiffEndpoints, + resolveGitDiffEndpointsAsync, resolveGitMetadata, resolveGitRepoRoot, + resolveGitRepoRootAsync, runGitText, + runGitTextAsync, shouldSkipLargeTrackedDiff, type GitBackedInput, type GitDiffEndpoints, @@ -87,13 +91,19 @@ interface GitSourceCapability { sourceCacheKey: string; } -/** Hash semantic index entries so filesystem-stat refreshes do not defeat cache reuse. */ -function gitIndexCacheKey(input: GitBackedInput, repoRoot: string, gitExecutable: string) { - const entries = runGitText({ +/** Hash index entries without blocking an embedded renderer. */ +async function gitIndexCacheKeyAsync( + input: GitBackedInput, + repoRoot: string, + gitExecutable: string, + signal?: AbortSignal, +) { + const entries = await runGitTextAsync({ input, args: ["ls-files", "--stage", "-z"], cwd: repoRoot, gitExecutable, + signal, }); return createHash("sha256").update(entries).digest("hex"); } @@ -109,22 +119,40 @@ function gitEndpointCacheKey(endpoint: GitDiffEndpoints["old"], indexCacheKey: s return endpoint.kind; } -/** - * Build a source reader that answers from exact Git old/new endpoints. - * - * The endpoint key omits worktree contents because the per-file patch fingerprint - * already describes their delta. Immutable refs and the index hash identify the - * base, so unchanged files can safely retain highlighting across watch reloads. - */ -function createGitSourceCapability( +/** Build a pinned revision source capability without blocking renderer input. */ +async function createGitRevisionSourceCapabilityAsync( + input: GitBackedInput, + ref: string, + repoRoot: string, + gitExecutable: string, + signal?: AbortSignal, +): Promise { + const newRef = await resolveGitCommitRefAsync(input, ref, { + cwd: repoRoot, + gitExecutable, + signal, + }); + return createGitSourceCapabilityAsync( + input, + repoRoot, + { old: { kind: "git-ref", ref: `${newRef}^` }, new: { kind: "git-ref", ref: newRef } }, + gitExecutable, + signal, + ); +} + +/** Build exact source capability data while asynchronously hashing a possible index. */ +async function createGitSourceCapabilityAsync( input: GitBackedInput, repoRoot: string, endpoints: GitDiffEndpoints, gitExecutable: string, -): GitSourceCapability { + signal?: AbortSignal, +): Promise { const needsIndex = endpoints.old.kind === "index" || endpoints.new.kind === "index"; - const indexCacheKey = needsIndex ? gitIndexCacheKey(input, repoRoot, gitExecutable) : "unused"; - + const indexCacheKey = needsIndex + ? await gitIndexCacheKeyAsync(input, repoRoot, gitExecutable, signal) + : "unused"; return { sourceCacheKey: [ "git-source-v1", @@ -132,19 +160,14 @@ function createGitSourceCapability( gitEndpointCacheKey(endpoints.new, indexCacheKey), ].join(":"), readFileSource: ({ path, previousPath, changeType, side }) => { - // An added file has no old side and a deleted one has no new side; asking - // Git for either would just be a failed lookup. if (side === "old") { return changeType === "new" ? Promise.resolve(null) : readGitFileSource( gitEndpointSourceSpec(endpoints.old, repoRoot, previousPath ?? path), - { - gitExecutable, - }, + { gitExecutable }, ); } - return changeType === "deleted" ? Promise.resolve(null) : readGitFileSource(gitEndpointSourceSpec(endpoints.new, repoRoot, path), { @@ -154,38 +177,22 @@ function createGitSourceCapability( }; } -/** Build a pinned source capability for a single-revision review. */ -function createGitRevisionSourceCapability( - input: GitBackedInput, - ref: string, - repoRoot: string, - gitExecutable: string, -): GitSourceCapability { - const newRef = resolveGitCommitRef(input, ref, { cwd: repoRoot, gitExecutable }); - return createGitSourceCapability( - input, - repoRoot, - { old: { kind: "git-ref", ref: `${newRef}^` }, new: { kind: "git-ref", ref: newRef } }, - gitExecutable, - ); -} - -/** - * Build a source capability for a working-tree review, when both sides are exact. - * - * Ranges that do not reduce to one old/new pair — octopus merges, multi-positive - * revision sets — deliberately get no reader at all rather than a guess, so - * expanded rows are never read from the wrong revision. - */ -function createGitDiffSourceCapability( +/** Build working-tree source capability without blocking renderer input. */ +async function createGitDiffSourceCapabilityAsync( input: ExtensionVcsDiffInput, repoRoot: string, cwd: string, gitExecutable: string, -): GitSourceCapability | undefined { - const endpoints = resolveGitDiffEndpoints(input, { cwd, repoRoot, gitExecutable }); + signal?: AbortSignal, +): Promise { + const endpoints = await resolveGitDiffEndpointsAsync(input, { + cwd, + repoRoot, + gitExecutable, + signal, + }); return endpoints - ? createGitSourceCapability(input, repoRoot, endpoints, gitExecutable) + ? createGitSourceCapabilityAsync(input, repoRoot, endpoints, gitExecutable, signal) : undefined; } @@ -300,8 +307,8 @@ export function createGitVcsAdapter({ }, operations: { "working-tree-diff": { - async load(input, { cwd }) { - const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); + async load(input, { cwd, signal }) { + const repoRoot = await resolveGitRepoRootAsync(input, { cwd, gitExecutable, signal }); const repoName = basename(repoRoot); const range = describeDiffRange(input); const title = input.staged @@ -311,22 +318,40 @@ export function createGitVcsAdapter({ : `${repoName} working tree`; // Ask for stats before the patch so files too large to render can be // excluded from the diff instead of generating output nobody reads. - const largeTrackedFiles = parseGitNumstat( - runGitText({ input, args: buildGitDiffNumstatArgs(input), cwd, gitExecutable }), - ).filter((file) => shouldSkipLargeTrackedDiff(file, repoRoot)); - const colorMoved = resolveGitColorMovedOptions(input, { cwd, gitExecutable }); - const sourceCapability = createGitDiffSourceCapability( + const numstat = await runGitTextAsync({ + input, + args: buildGitDiffNumstatArgs(input), + cwd, + gitExecutable, + signal, + }); + const colorMoved = await resolveGitColorMovedOptionsAsync(input, { + cwd, + gitExecutable, + signal, + }); + const sourceCapability = await createGitDiffSourceCapabilityAsync( input, repoRoot, cwd, gitExecutable, + signal, + ); + const untrackedPaths = await listGitUntrackedFilesAsync(input, { + cwd, + repoRoot, + gitExecutable, + signal, + }); + const largeTrackedFiles = parseGitNumstat(numstat).filter((file) => + shouldSkipLargeTrackedDiff(file, repoRoot), ); return { repoRoot, sourceLabel: repoRoot, title, - patchText: runGitText({ + patchText: await runGitTextAsync({ input, args: buildGitDiffArgs( input, @@ -335,6 +360,7 @@ export function createGitVcsAdapter({ ), cwd, gitExecutable, + signal, }), ...sourceCapability, extraFiles: largeTrackedFiles.map( @@ -350,7 +376,7 @@ export function createGitVcsAdapter({ // diff in-process. Rendering them through `git diff --no-index` // instead costs one subprocess per file, which made working-tree // review scale with the untracked file count. - untrackedPaths: listGitUntrackedFiles(input, { cwd, repoRoot, gitExecutable }), + untrackedPaths, }; }, watchPlan(input, { cwd }) { @@ -379,28 +405,30 @@ export function createGitVcsAdapter({ }, }, "revision-show": { - async load(input, { cwd }) { - const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); + async load(input, { cwd, signal }) { + const repoRoot = await resolveGitRepoRootAsync(input, { cwd, gitExecutable, signal }); const repoName = basename(repoRoot); - const sourceCapability = createGitRevisionSourceCapability( + const sourceCapability = await createGitRevisionSourceCapabilityAsync( input, input.ref ?? "HEAD", repoRoot, gitExecutable, + signal, ); return { repoRoot, sourceLabel: repoRoot, title: input.ref ? `${repoName} show ${input.ref}` : `${repoName} show HEAD`, - patchText: runGitText({ + patchText: await runGitTextAsync({ input, args: buildGitShowArgs( input, - resolveGitColorMovedOptions(input, { cwd, gitExecutable }), + await resolveGitColorMovedOptionsAsync(input, { cwd, gitExecutable, signal }), ), cwd, gitExecutable, + signal, }), ...sourceCapability, }; @@ -419,28 +447,30 @@ export function createGitVcsAdapter({ }, }, "stash-show": { - async load(input, { cwd }) { - const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); + async load(input, { cwd, signal }) { + const repoRoot = await resolveGitRepoRootAsync(input, { cwd, gitExecutable, signal }); const repoName = basename(repoRoot); - const sourceCapability = createGitRevisionSourceCapability( + const sourceCapability = await createGitRevisionSourceCapabilityAsync( input, input.ref ?? "stash@{0}", repoRoot, gitExecutable, + signal, ); return { repoRoot, sourceLabel: repoRoot, title: input.ref ? `${repoName} stash ${input.ref}` : `${repoName} stash`, - patchText: runGitText({ + patchText: await runGitTextAsync({ input, args: buildGitStashShowArgs( input, - resolveGitColorMovedOptions(input, { cwd, gitExecutable }), + await resolveGitColorMovedOptionsAsync(input, { cwd, gitExecutable, signal }), ), cwd, gitExecutable, + signal, }), ...sourceCapability, }; diff --git a/src/extensions/default/vcs/jujutsu/commands.ts b/src/extensions/default/vcs/jujutsu/commands.ts index 39c2272a4..daa3989c6 100644 --- a/src/extensions/default/vcs/jujutsu/commands.ts +++ b/src/extensions/default/vcs/jujutsu/commands.ts @@ -6,6 +6,7 @@ import { } from "hunkdiff/extension"; import { normalizePathForOS } from "../../../../lib/osPath"; import { describeDiffTargets } from "../diffRange"; +import { runAbortableCommand } from "../asyncProcess"; export type JjBackedInput = ExtensionVcsDiffInput | ExtensionVcsShowInput; @@ -14,6 +15,7 @@ export interface RunJjTextOptions { args: string[]; cwd?: string; jjExecutable?: string; + signal?: AbortSignal; } /** Identifies the reviewed new commit and every commit JJ used to build the old side. */ @@ -250,6 +252,31 @@ export function runJjText(options: RunJjTextOptions) { return runJjCommand(options).stdout; } +/** Run a Jujutsu command asynchronously so embedded review preparation stays cancellable. */ +export async function runJjTextAsync({ + input, + args, + cwd = process.cwd(), + jjExecutable = "jj", + signal, +}: RunJjTextOptions): Promise { + const command = [jjExecutable, "--no-pager", "--color", "never", ...args]; + let result: Awaited>; + try { + result = await runAbortableCommand(command, { cwd, signal }); + } catch (error) { + if (signal?.aborted) signal.throwIfAborted(); + throw translateJjSpawnFailure(input, error, jjExecutable); + } + if (result.exitCode !== 0) { + throw translateJjExitFailure( + input, + result.stderr.trim() || `Command failed: ${command.join(" ")}`, + ); + } + return result.stdout; +} + /** * Resolve a JJ revset once so the patch and later source reads use the same commit. * @@ -332,6 +359,61 @@ export function resolveJjRangeEndpoints( return { newCommitId: toCommitIds[0]!, oldCommitIds: [fromCommitIds[0]!] }; } +/** Resolve immutable JJ endpoints without blocking renderer input. */ +export async function resolveJjDiffEndpointsAsync( + input: JjBackedInput, + revset: string, + options: Omit = {}, +): Promise { + const commitIds = parseJjCommitIds( + await runJjTextAsync({ + input, + args: ["log", "--no-graph", "-r", revset, "-T", JjCommitIdTemplate], + ...options, + }), + ); + if (commitIds.length !== 1) return undefined; + const commitId = commitIds[0]!; + const parentCommitIds = parseJjCommitIds( + await runJjTextAsync({ + input, + args: [ + "log", + "--no-graph", + "--ignore-working-copy", + "-r", + `${commitId}-`, + "-T", + JjCommitIdTemplate, + ], + ...options, + }), + ); + return { newCommitId: commitId, oldCommitIds: parentCommitIds.sort() }; +} + +/** Resolve two immutable JJ range endpoints without blocking renderer input. */ +export async function resolveJjRangeEndpointsAsync( + input: ExtensionVcsDiffInput, + endpoints: ExtensionVcsRangeEndpoints, + options: Omit = {}, +): Promise { + const from = requireJjRevisionArg(input, endpoints.from); + const to = requireJjRevisionArg(input, endpoints.to); + const resolveOne = async (revset: string) => + parseJjCommitIds( + await runJjTextAsync({ + input, + args: ["log", "--no-graph", "-r", revset, "-T", JjCommitIdTemplate], + ...options, + }), + ); + const fromCommitIds = await resolveOne(from); + const toCommitIds = await resolveOne(to); + if (fromCommitIds.length !== 1 || toCommitIds.length !== 1) return undefined; + return { newCommitId: toCommitIds[0]!, oldCommitIds: [fromCommitIds[0]!] }; +} + export function resolveJjRepoRoot( input: JjBackedInput, options: Omit = {}, @@ -343,3 +425,11 @@ export function resolveJjRepoRoot( }).trim(); return normalizePathForOS(repoRoot); } + +/** Resolve the JJ repository root without blocking renderer input. */ +export async function resolveJjRepoRootAsync( + input: JjBackedInput, + options: Omit = {}, +) { + return normalizePathForOS((await runJjTextAsync({ input, args: ["root"], ...options })).trim()); +} diff --git a/src/extensions/default/vcs/jujutsu/index.ts b/src/extensions/default/vcs/jujutsu/index.ts index 0e0d7e9c7..cc065bc65 100644 --- a/src/extensions/default/vcs/jujutsu/index.ts +++ b/src/extensions/default/vcs/jujutsu/index.ts @@ -4,10 +4,11 @@ import { buildJjDiffArgs, buildJjShowArgs, createJjStagedError, - resolveJjDiffEndpoints, - resolveJjRangeEndpoints, - resolveJjRepoRoot, + resolveJjDiffEndpointsAsync, + resolveJjRangeEndpointsAsync, + resolveJjRepoRootAsync, runJjText, + runJjTextAsync, type JjDiffEndpoints, } from "./commands"; import { openJjHistory } from "./history"; @@ -154,15 +155,23 @@ export function createJjVcsAdapter({ jjExecutable = "jj" }: Readonly { + const command = [slExecutable, "--noninteractive", "--color", "never", ...args]; + let result: Awaited>; + try { + result = await runAbortableCommand(command, { cwd, signal }); + } catch (error) { + if (signal?.aborted) signal.throwIfAborted(); + throw translateSlSpawnFailure(input, error, slExecutable); + } + if (result.exitCode !== 0) { + throw translateSlExitFailure( + input, + result.stderr.trim() || `Command failed: ${command.join(" ")}`, + ); + } + return result.stdout; +} + /** Return whether working-copy review should synthesize unknown Sapling files into the patch stream. */ function shouldIncludeUntrackedFiles(input: ExtensionVcsDiffInput) { return !input.staged && !input.rangeEndpoints && input.options.excludeUntracked !== true; @@ -309,6 +336,34 @@ export function listSlUntrackedFiles( ); } +/** Return unknown files without blocking review preparation. */ +export async function listSlUntrackedFilesAsync( + input: ExtensionVcsDiffInput, + { + cwd = process.cwd(), + repoRoot, + slExecutable = "sl", + signal, + }: Omit & { repoRoot?: string } = {}, +) { + validateSlDiffEndpoints(input); + if (!shouldIncludeUntrackedFiles(input)) return []; + const statusText = await runSlTextAsync({ + input, + args: buildSlStatusArgs(input), + cwd, + slExecutable, + signal, + }); + const untrackedFiles = parseUntrackedFilePaths(statusText); + if (untrackedFiles.length === 0) return []; + const normalizedRepoRoot = + repoRoot ?? (await resolveSlRepoRootAsync(input, { cwd, slExecutable, signal })); + return untrackedFiles.filter((filePath) => + isReviewableUntrackedPath(normalizedRepoRoot, filePath), + ); +} + /** Resolve the repo root by running `sl root`. */ export function resolveSlRepoRoot( input: SlBackedInput, @@ -321,3 +376,11 @@ export function resolveSlRepoRoot( }).trim(); return normalizePathForOS(repoRoot); } + +/** Resolve the Sapling repository root without blocking renderer input. */ +export async function resolveSlRepoRootAsync( + input: SlBackedInput, + options: Omit = {}, +) { + return normalizePathForOS((await runSlTextAsync({ input, args: ["root"], ...options })).trim()); +} diff --git a/src/extensions/default/vcs/sapling/index.test.ts b/src/extensions/default/vcs/sapling/index.test.ts index 04e0031e2..20dad2254 100644 --- a/src/extensions/default/vcs/sapling/index.test.ts +++ b/src/extensions/default/vcs/sapling/index.test.ts @@ -165,14 +165,29 @@ describe("SaplingVcsAdapter without the sl binary", () => { test("adapter range loads do not probe working-copy unknown files", async () => { const repo = createTempDir("hunk-sl-adapter-range-untracked-"); const commands: string[][] = []; - const mutableBun = Bun as unknown as { spawnSync: typeof Bun.spawnSync }; + const mutableBun = Bun as unknown as { + spawnSync: typeof Bun.spawnSync; + spawn: typeof Bun.spawn; + }; const originalSpawnSync = mutableBun.spawnSync; + const originalSpawn = mutableBun.spawn; mutableBun.spawnSync = ((command: string[]) => { commands.push(command); const operation = command.slice(4); const stdout = operation[0] === "root" ? `${repo}\n` : ""; return { exitCode: 0, stdout: Buffer.from(stdout), stderr: Buffer.from("") }; }) as typeof Bun.spawnSync; + mutableBun.spawn = ((command: string[]) => { + commands.push(command); + const operation = command.slice(4); + const stdout = operation[0] === "root" ? `${repo}\n` : ""; + return { + stdout: new Blob([stdout]).stream(), + stderr: new Blob([""]).stream(), + exited: Promise.resolve(0), + kill() {}, + }; + }) as unknown as typeof Bun.spawn; try { const input = { @@ -192,6 +207,7 @@ describe("SaplingVcsAdapter without the sl binary", () => { ).toBe(true); } finally { mutableBun.spawnSync = originalSpawnSync; + mutableBun.spawn = originalSpawn; } }); diff --git a/src/extensions/default/vcs/sapling/index.ts b/src/extensions/default/vcs/sapling/index.ts index 37eacb1df..fcfe201ff 100644 --- a/src/extensions/default/vcs/sapling/index.ts +++ b/src/extensions/default/vcs/sapling/index.ts @@ -5,8 +5,11 @@ import { buildSlShowArgs, createSlStagedError, listSlUntrackedFiles, + listSlUntrackedFilesAsync, resolveSlRepoRoot, + resolveSlRepoRootAsync, runSlText, + runSlTextAsync, } from "./commands"; import { describeDiffRange } from "../diffRange"; import { @@ -76,20 +79,20 @@ export const SaplingVcsAdapter = { detectionPriority: HUNK_VCS_DETECTION_BASELINE_PRIORITY + 100, operations: { "working-tree-diff": { - async load(input, { cwd }) { + async load(input, { cwd, signal }) { if (input.staged) { throw createSlStagedError(input); } const diffArgs = buildSlDiffArgs(input); - const repoRoot = resolveSlRepoRoot(input, { cwd }); + const repoRoot = await resolveSlRepoRootAsync(input, { cwd, signal }); const repoName = basename(repoRoot); const range = describeDiffRange(input); return { repoRoot, sourceLabel: repoRoot, title: range ? `${repoName} ${range}` : `${repoName} working copy`, - patchText: runSlText({ input, args: diffArgs, cwd }), - untrackedPaths: listSlUntrackedFiles(input, { cwd, repoRoot }), + patchText: await runSlTextAsync({ input, args: diffArgs, cwd, signal }), + untrackedPaths: await listSlUntrackedFilesAsync(input, { cwd, repoRoot, signal }), }; }, watchSignature(input, { cwd }) { @@ -102,15 +105,15 @@ export const SaplingVcsAdapter = { }, }, "revision-show": { - async load(input, { cwd }) { - const repoRoot = resolveSlRepoRoot(input, { cwd }); + async load(input, { cwd, signal }) { + const repoRoot = await resolveSlRepoRootAsync(input, { cwd, signal }); const repoName = basename(repoRoot); const revset = input.ref ?? "."; return { repoRoot, sourceLabel: repoRoot, title: `${repoName} show ${revset}`, - patchText: runSlText({ input, args: buildSlShowArgs(input), cwd }), + patchText: await runSlTextAsync({ input, args: buildSlShowArgs(input), cwd, signal }), }; }, watchSignature(input, { cwd }) { diff --git a/src/main.tsx b/src/main.tsx index 23732524f..9ebf7fa7f 100644 --- a/src/main.tsx +++ b/src/main.tsx @@ -7,10 +7,6 @@ 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(); @@ -132,17 +128,10 @@ async function main() { } // OpenTUI stays behind the interactive plan so headless commands never materialize its embedded - // 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. + // 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. 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 { await runInteractiveApp(startupPlan); } catch (error) { @@ -154,7 +143,7 @@ async function main() { } } -await main().catch(async (error) => { - if (!(await reportTerminalHandoffFailure(error))) process.stderr.write(formatCliError(error)); +await main().catch((error) => { + process.stderr.write(formatCliError(error)); process.exitCode = 1; }); diff --git a/src/ui/App.tsx b/src/ui/App.tsx index c845e48a1..1f3ad5030 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -53,6 +53,7 @@ import { resolveCodeViewportWidth, } from "./diff/codeColumns"; import { useAppKeyboardShortcuts } from "./hooks/useAppKeyboardShortcuts"; +import { useIntermediateRenderAfterMount } from "./hooks/useIntermediateRenderAfterMount"; import { useCurrentReviewRefreshController } from "./hooks/useCurrentReviewRefreshController"; import { useExtensionCommandRunner } from "./hooks/useExtensionCommandRunner"; import { useExtensionDialogController } from "./hooks/useExtensionDialogController"; @@ -134,22 +135,29 @@ function clamp(value: number, min: number, max: number) { /** Orchestrate global app state, layout, navigation, and pane coordination. */ export function App({ bootstrap, + canReloadExtensions = true, hostClient, noticeText, onQuit = () => process.exit(0), + onFirstFrameReady, onRegisterWorkspaceRefreshRequest, onReloadSession, onRequestExtensionReviewReload, onWorkspaceWriteCompleted, reviewProducer, runWorkspaceWrite, + returnToHistory = process.env.HUNK_RETURN_TO_HISTORY === "1", watchRuntime, workspaceFileWriter, }: { bootstrap: AppBootstrap; + /** Whether this surface may replace the session-owned extension registry. */ + canReloadExtensions?: boolean; hostClient?: HunkSessionBrokerClient; noticeText?: string | null; onQuit?: () => void; + /** Report once OpenTUI has committed the review's first requested frame. */ + onFirstFrameReady?: () => void; /** Register the mounted review descriptor AppHost should reconcile after a completed write. */ onRegisterWorkspaceRefreshRequest: (request: WorkspaceRefreshRequest) => () => void; onReloadSession: ( @@ -166,6 +174,8 @@ export function App({ reviewProducer?: ReviewProducer; /** Start and track one irreversible write, or refuse it once graceful shutdown begins. */ runWorkspaceWrite: WorkspaceWriteRunner; + /** Present quit as returning to the owning history surface. */ + returnToHistory?: boolean; watchRuntime?: WatchedInputRuntime; workspaceFileWriter?: WorkspaceFileWriter; }) { @@ -362,8 +372,7 @@ export function App({ configPath: bootstrap.viewPreferencesConfigPath, pagerMode, promptSaveViewPreferences: - bootstrap.input.options.promptSaveViewPreferences !== false && - process.env.HUNK_RETURN_TO_HISTORY !== "1", + bootstrap.input.options.promptSaveViewPreferences !== false && !returnToHistory, transientViewPreferences: extensionSessionOptions.transientViewPreferences, onQuit, showNotice: showSessionNotice, @@ -742,11 +751,27 @@ export function App({ ), [diffContentWidth, maxLineNumberDigits, resolvedLayout, showLineNumbers], ); + // Redraw subsequent geometry changes without clearing a review mounted into an existing root. + useIntermediateRenderAfterMount( + renderer, + [renderSidebar, resolvedLayout, terminal.height, terminal.width, wrapLines], + Boolean(onFirstFrameReady), + ); + const firstFrameReportedRef = useRef(false); useEffect(() => { - // Force an intermediate redraw when app geometry or row-wrapping changes so pane relayout - // feels immediate after toggling split/stack or line wrapping. - renderer.intermediateRender(); - }, [renderer, renderSidebar, resolvedLayout, terminal.height, terminal.width, wrapLines]); + if (!onFirstFrameReady || firstFrameReportedRef.current) return; + let active = true; + renderer.requestRender(); + void renderer.idle().then(() => { + if (active && !firstFrameReportedRef.current) { + firstFrameReportedRef.current = true; + onFirstFrameReady(); + } + }); + return () => { + active = false; + }; + }, [onFirstFrameReady, renderer]); /** Scroll the main review pane by line steps, viewport fractions, or whole-content jumps. */ const scrollDiff = ( @@ -896,7 +921,7 @@ export function App({ extensionTrustPromptRoot, trustRepoExtensions, } = useExtensionTrustController({ - canRefreshCurrentInput, + canRefreshCurrentInput: canRefreshCurrentInput && canReloadExtensions, pagerMode, pendingRepoRoot: pendingTrustRepoRoot, refreshCurrentInput, @@ -1079,7 +1104,7 @@ export function App({ triggerEditSelectedFile, triggerRefreshCurrentInput, }).map((command) => - process.env.HUNK_RETURN_TO_HISTORY === "1" && command.id === "hunk.app.quit" + returnToHistory && command.id === "hunk.app.quit" ? { ...command, title: "Back to history" } : command, ), @@ -1354,6 +1379,7 @@ export function App({ pagerMode={pagerMode} screenTop={diffPaneScreenTop} showTopChrome={showMenuBar} + skipInitialIntermediateRender={Boolean(onFirstFrameReady)} headerLabelWidth={diffHeaderLabelWidth} headerStatsWidth={diffHeaderStatsWidth} layout={resolvedLayout} diff --git a/src/ui/AppHost.dynamic-mount.test.tsx b/src/ui/AppHost.dynamic-mount.test.tsx new file mode 100644 index 000000000..b6461df9b --- /dev/null +++ b/src/ui/AppHost.dynamic-mount.test.tsx @@ -0,0 +1,61 @@ +import { expect, mock, test } from "bun:test"; +import { testRender } from "@opentui/react/test-utils"; +import { act, useState } from "react"; +import { createTestVcsAppBootstrap } from "../../test/helpers/app-bootstrap"; +import { createTestDiffFile } from "../../test/helpers/diff-helpers"; +import { AppHost } from "./AppHost"; + +mock.restore(); + +let mountReview: (() => void) | undefined; + +/** Mount AppHost after another surface has already committed into the same stable root. */ +function DynamicReviewHost({ onReady }: { onReady: () => void }) { + const [mounted, setMounted] = useState(false); + mountReview = () => setMounted(true); + if (!mounted) return History surface; + return ( + + ); +} + +test("AppHost paints and reports readiness when mounted after a retained surface", async () => { + const ready = mock(() => undefined); + const setup = await testRender(, { + width: 100, + height: 18, + }); + try { + await act(async () => { + await setup.renderOnce(); + }); + expect(setup.captureCharFrame()).toContain("History surface"); + + act(() => mountReview?.()); + await Bun.sleep(20); + await setup.renderOnce(); + await Bun.sleep(20); + await setup.renderOnce(); + + const frame = setup.captureCharFrame(); + expect(frame).toContain("dynamic.ts"); + expect(ready).toHaveBeenCalledTimes(1); + } finally { + setup.renderer.destroy(); + mountReview = undefined; + } +}); diff --git a/src/ui/AppHost.keybindings.test.tsx b/src/ui/AppHost.keybindings.test.tsx index 1416daf55..6e03fd1f1 100644 --- a/src/ui/AppHost.keybindings.test.tsx +++ b/src/ui/AppHost.keybindings.test.tsx @@ -10,6 +10,7 @@ import { resolveConfiguredCliInput } from "../core/run/config"; import { getBundledVcsCatalog } from "../app/vcsCatalog"; import { loadAppBootstrap } from "../core/changeset/loaders"; import type { AppBootstrap } from "../core/bootstrap"; +import { retireExtensionLoadResult } from "../extensions/events"; import { createEmptyExtensionLoadResult } from "../extensions/types"; import { AppHost } from "./AppHost"; @@ -97,6 +98,7 @@ async function withAppHost( bootstrap: AppBootstrap, body: (setup: Awaited>, quits: () => number) => Promise, externalQuitSignal?: AbortSignal, + extensionOwnership: "owned" | "borrowed" = "owned", ) { let quitCount = 0; const setup = await testRender( @@ -104,6 +106,7 @@ async function withAppHost( bootstrap={bootstrap} externalQuitSignal={externalQuitSignal} onQuit={() => (quitCount += 1)} + extensionOwnership={extensionOwnership} />, { width: 120, height: 24 }, ); @@ -322,6 +325,49 @@ describe("user keybindings", () => { }); }); + test("borrows one history extension authority across repeated review mounts", async () => { + const repo = createTestRepo("hunk-keybindings-borrowed-extensions-"); + const bootstrap = await launchWithConfig(repo, ""); + const extensions = createEmptyExtensionLoadResult(repo); + const seen: string[] = []; + extensions.registry.eventHandlers.startup.push({ + extensionId: "stateful", + handler: () => { + seen.push("startup"); + }, + }); + extensions.registry.eventHandlers.changeset_loaded.push({ + extensionId: "stateful", + handler: () => { + seen.push("changeset"); + }, + }); + extensions.registry.eventHandlers.shutdown.push({ + extensionId: "stateful", + handler: () => { + seen.push("shutdown"); + }, + }); + bootstrap.extensions = extensions; + + for (let generation = 0; generation < 2; generation += 1) { + await withAppHost( + bootstrap, + async (setup, quits) => { + await act(async () => setup.mockInput.typeText("q")); + await flush(setup); + expect(quits()).toBe(1); + }, + undefined, + "borrowed", + ); + } + + expect(seen).toEqual(["changeset", "changeset"]); + await retireExtensionLoadResult(extensions); + expect(seen).toEqual(["changeset", "changeset", "shutdown"]); + }); + test("retires extensions before an external terminal interrupt quits", async () => { const repo = createTestRepo("hunk-keybindings-interrupt-shutdown-"); const bootstrap = await launchWithConfig(repo, ""); diff --git a/src/ui/AppHost.tsx b/src/ui/AppHost.tsx index 9c83727c2..959973c4b 100644 --- a/src/ui/AppHost.tsx +++ b/src/ui/AppHost.tsx @@ -62,6 +62,9 @@ export function AppHost({ hostClient, onQuit = () => process.exit(0), onActiveBootstrapChange, + onFirstFrameReady, + returnToHistory = false, + extensionOwnership = "owned", reviewProducer, startupNoticeResolver, watchRuntime, @@ -75,6 +78,12 @@ export function AppHost({ onQuit?: () => void; /** Observe the bootstrap after its matching App commit; used by mounted host tests. */ onActiveBootstrapChange?: (bootstrap: AppBootstrap) => void; + /** Report once the dynamically mounted review has committed its first requested frame. */ + onFirstFrameReady?: () => void; + /** Present quit as returning to an owning history surface. */ + returnToHistory?: boolean; + /** Whether this surface may retire and restart its initial extension authority. */ + extensionOwnership?: "owned" | "borrowed"; /** * The producer whose generations this host publishes. Supplied by the process that * built the initial registration from its first publication; a host mounted without one @@ -111,9 +120,14 @@ export function AppHost({ }), ); const [appVersion, setAppVersion] = useState(0); - // Extensions outlive App remounts, and a trust grant can replace the whole - // load result mid-session, so the host owns them rather than the bootstrap. + // Extensions outlive App remounts. Standalone hosts own replacement and shutdown; + // embedded reviews borrow the history workspace's initial authority. const extensionsRef = useRef(initialBootstrap.extensions as ExtensionLoadResult | undefined); + const borrowedExtensionRegistryRef = useRef( + extensionOwnership === "borrowed" + ? (initialBootstrap.extensions as ExtensionLoadResult | undefined)?.registry + : undefined, + ); // Experimental capabilities are launch authority: remote/watch reloads may replace content, // but opting in or out requires starting a new Hunk process. const launchExperimental = initialBootstrap.input.options.experimental === true; @@ -172,9 +186,11 @@ export function AppHost({ // leases are live here; passive UI events still wait until this order lands. if (initialExtensionStartupPendingRef.current) { initialExtensionStartupPendingRef.current = false; - emitExtensionEvent(extensionsRef.current, "startup", { - cwd: initialBootstrap.reloadContext.cwd, - }); + if (extensionOwnership === "owned") { + emitExtensionEvent(extensionsRef.current, "startup", { + cwd: initialBootstrap.reloadContext.cwd, + }); + } emitExtensionEvent(extensionsRef.current, "changeset_loaded", { changeset: initialBootstrap.changeset, }); @@ -197,7 +213,7 @@ export function AppHost({ reason: pending.reason, }); pending.resolveMounted(); - }, [activeBootstrap, initialBootstrap.reloadContext.cwd]); + }, [activeBootstrap, extensionOwnership, initialBootstrap.reloadContext.cwd]); /** Track one prepared registry until it is either adopted or fully retired. */ const trackPreparedExtensionReplacement = useCallback((result: ExtensionLoadResult) => { @@ -206,6 +222,7 @@ export function AppHost({ /** Track every registry retirement until its shared shutdown completion settles. */ const retireOwnedExtensionLoadResult = useCallback((result: ExtensionLoadResult | undefined) => { + if (result?.registry === borrowedExtensionRegistryRef.current) return Promise.resolve(); const retirement = retireExtensionLoadResult(result); pendingExtensionRetirementsRef.current.add(retirement); void retirement.then( @@ -297,7 +314,10 @@ export function AppHost({ } let replacementExtensions: ExtensionLoadResult | undefined; - if (options?.reloadExtensions || cwd !== extensionsCwdRef.current) { + if ( + extensionOwnership === "owned" && + (options?.reloadExtensions || cwd !== extensionsCwdRef.current) + ) { try { const resolvedExtensions = await resolveConfiguredExtensions({ runtimeInput, @@ -460,6 +480,7 @@ export function AppHost({ }, [ adoptPreparedExtensionReplacement, + extensionOwnership, hostClient, launchExperimental, launchFast, @@ -610,9 +631,12 @@ export function AppHost({ { - renderer.intermediateRender(); - }, [renderer, pinnedHeaderFileId]); + useIntermediateRenderAfterMount(renderer, [pinnedHeaderFileId], skipInitialIntermediateRender); const fullFileRenderItems = useMemo( (): FileRenderWindowItem[] => diff --git a/src/ui/history/types.ts b/src/ui/history/types.ts index fa808bae7..045b9916d 100644 --- a/src/ui/history/types.ts +++ b/src/ui/history/types.ts @@ -1,5 +1,6 @@ import type { HistoryCommandInput } from "../../core/run/commandInputs"; import type { VcsHistorySource } from "../../core/vcs/types"; +import type { ExtensionLoadResult } from "../../extensions/types"; import type { ExtensionVcsHistoryCommit, ExtensionVcsHistoryReviewAction, @@ -13,9 +14,13 @@ export interface HistoryRuntime { source: VcsHistorySource; providerId: string; providerName: string; + /** Invocation cwd used to resolve explicit extension paths for embedded reviews. */ + startupCwd?: string; repoRoot: string; notices: readonly string[]; customThemes: readonly NamedCustomThemeConfig[]; + /** History-owned extension authority borrowed by embedded reviews. */ + extensionSession?: ExtensionLoadResult; planReview( commit: ExtensionVcsHistoryCommit, options?: ExtensionVcsHistoryReviewOptions, diff --git a/src/ui/hooks/useIntermediateRenderAfterMount.test.tsx b/src/ui/hooks/useIntermediateRenderAfterMount.test.tsx new file mode 100644 index 000000000..02015641e --- /dev/null +++ b/src/ui/hooks/useIntermediateRenderAfterMount.test.tsx @@ -0,0 +1,39 @@ +import { expect, mock, test } from "bun:test"; +import { act, useState } from "react"; +import { testRender } from "@opentui/react/test-utils"; +import { useRenderer } from "@opentui/react"; +import { useIntermediateRenderAfterMount } from "./useIntermediateRenderAfterMount"; + +mock.restore(); + +let changeGeometry: (() => void) | undefined; + +/** Exercise the post-mount redraw hook through a committed geometry change. */ +function TestPostMountRender() { + const renderer = useRenderer(); + const [geometry, setGeometry] = useState(1); + changeGeometry = () => setGeometry((value) => value + 1); + useIntermediateRenderAfterMount(renderer, [geometry]); + return {geometry}; +} + +test("post-mount intermediate rendering skips initial dynamic mount and redraws later geometry", async () => { + const setup = await testRender(, { width: 20, height: 4 }); + const intermediateRender = mock(() => undefined); + setup.renderer.intermediateRender = intermediateRender; + try { + await act(async () => { + await setup.renderOnce(); + }); + expect(intermediateRender).not.toHaveBeenCalled(); + + await act(async () => { + changeGeometry?.(); + await setup.renderOnce(); + }); + expect(intermediateRender).toHaveBeenCalledTimes(1); + } finally { + setup.renderer.destroy(); + changeGeometry = undefined; + } +}); diff --git a/src/ui/hooks/useIntermediateRenderAfterMount.ts b/src/ui/hooks/useIntermediateRenderAfterMount.ts new file mode 100644 index 000000000..a04118320 --- /dev/null +++ b/src/ui/hooks/useIntermediateRenderAfterMount.ts @@ -0,0 +1,18 @@ +import type { CliRenderer } from "@opentui/core"; +import { useLayoutEffect, useRef, type DependencyList } from "react"; + +/** Request an intermediate redraw only after a component's initial committed layout. */ +export function useIntermediateRenderAfterMount( + renderer: Pick, + dependencies: DependencyList, + skipInitial = true, +) { + const mountedRef = useRef(false); + useLayoutEffect(() => { + if (!mountedRef.current) { + mountedRef.current = true; + if (skipInitial) return; + } + renderer.intermediateRender(); + }, [renderer, skipInitial, ...dependencies]); +} diff --git a/src/ui/log/LogApp.tsx b/src/ui/log/LogApp.tsx index 38090f7b5..b03bc09e6 100644 --- a/src/ui/log/LogApp.tsx +++ b/src/ui/log/LogApp.tsx @@ -102,9 +102,12 @@ export function LogApp({ : null, ); try { + const action = await planned; + // Commit the loading surface before in-process provider startup performs synchronous probes. + await new Promise((resolve) => setImmediate(resolve)); await onOutcome({ kind: "open-review", - action: await planned, + action, themeId: themeController.themeId, themeMode: terminalThemeMode, }); diff --git a/src/ui/log/LogSessionHost.tsx b/src/ui/log/LogSessionHost.tsx new file mode 100644 index 000000000..86c91bc07 --- /dev/null +++ b/src/ui/log/LogSessionHost.tsx @@ -0,0 +1,143 @@ +import { useCallback, useEffect, useRef, useState } from "react"; +import { retireExtensionLoadResult } from "../../extensions/events"; +import { resolveStartupUpdateNotice } from "../../core/process/updateNotice"; +import type { HistoryRuntime } from "../history/types"; +import { AppHost } from "../AppHost"; +import { + createReviewSessionRuntime, + prepareEmbeddedHistoryReview, + type EmbeddedHistoryReview, + type ReviewSessionRuntime, +} from "../runInteractiveApp"; +import { interactiveLogUsesColor } from "./colorPolicy"; +import { LogApp, type LogAppOutcome } from "./LogApp"; +import { LogController } from "./controller"; + +interface MountedReview { + plan: EmbeddedHistoryReview; + runtime: ReviewSessionRuntime; + instanceId: number; +} + +/** Route history and fresh review sessions through one stable React and terminal renderer root. */ +export function LogSessionHost({ + controller, + runtime, + externalQuitSignal, + onQuit, +}: { + controller: LogController; + runtime: HistoryRuntime; + externalQuitSignal: AbortSignal; + onQuit: (exitCode?: number) => void; +}) { + const [review, setReview] = useState(null); + const reviewRef = useRef(review); + reviewRef.current = review; + const [preparing, setPreparing] = useState(false); + const preparingRef = useRef(preparing); + preparingRef.current = preparing; + const nextInstanceRef = useRef(1); + const preparationControllerRef = useRef(null); + + const retireReview = useCallback(() => { + const current = reviewRef.current; + if (!current) return; + reviewRef.current = null; + current.runtime.stop(); + setReview(null); + if (externalQuitSignal.aborted) onQuit(); + }, [externalQuitSignal, onQuit]); + + const handleLogOutcome = async (outcome: LogAppOutcome) => { + if (outcome.kind === "quit") { + preparationControllerRef.current?.abort( + new Error("History review preparation was cancelled."), + ); + onQuit(outcome.exitCode); + return; + } + if (preparingRef.current || reviewRef.current) return; + preparingRef.current = true; + setPreparing(true); + const preparationController = new AbortController(); + preparationControllerRef.current = preparationController; + const preparationSignal = AbortSignal.any([externalQuitSignal, preparationController.signal]); + let plan: EmbeddedHistoryReview | undefined; + try { + plan = await prepareEmbeddedHistoryReview(runtime, outcome.action, { + themeId: outcome.themeId, + themeMode: outcome.themeMode, + signal: preparationSignal, + }); + preparationSignal.throwIfAborted(); + const reviewRuntime = createReviewSessionRuntime( + plan.bootstrap, + runtime.startupCwd ?? runtime.repoRoot, + ); + const mounted = { + plan, + runtime: reviewRuntime, + instanceId: nextInstanceRef.current++, + } satisfies MountedReview; + reviewRef.current = mounted; + setReview(mounted); + } catch (error) { + if (plan && !plan.borrowsExtensions && !reviewRef.current) { + await retireExtensionLoadResult(plan.bootstrap.extensions); + } + if (preparationSignal.aborted) { + if (externalQuitSignal.aborted) onQuit(); + } else throw error; + } finally { + if (preparationControllerRef.current === preparationController) { + preparationControllerRef.current = null; + } + preparingRef.current = false; + setPreparing(false); + } + }; + + useEffect( + () => () => { + preparationControllerRef.current?.abort( + new Error("History review host unmounted during preparation."), + ); + }, + [], + ); + + useEffect(() => { + if (reviewRef.current || preparing) return; + const requestQuit = () => onQuit(); + if (externalQuitSignal.aborted) requestQuit(); + else externalQuitSignal.addEventListener("abort", requestQuit, { once: true }); + return () => externalQuitSignal.removeEventListener("abort", requestQuit); + }, [externalQuitSignal, onQuit, preparing, review]); + + if (review) { + return ( + undefined} + returnToHistory + extensionOwnership={review.plan.borrowsExtensions ? "borrowed" : "owned"} + reviewProducer={review.runtime.reviewProducer} + startupNoticeResolver={resolveStartupUpdateNotice} + /> + ); + } + + return ( + + ); +} diff --git a/src/ui/log/embeddedReview.test.ts b/src/ui/log/embeddedReview.test.ts new file mode 100644 index 000000000..539a776fc --- /dev/null +++ b/src/ui/log/embeddedReview.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, test } from "bun:test"; +import { resolve } from "node:path"; +import type { HistoryRuntime } from "../history/types"; +import { prepareEmbeddedHistoryReview } from "../runInteractiveApp"; + +/** Provide only the provider-neutral fields embedded review startup consumes. */ +function createTestRuntime() { + const extensionSession = { registry: {} }; + return { + repoRoot: resolve("repository"), + startupCwd: resolve("invocation"), + providerId: "opaque-vcs", + input: { extensionPaths: ["extensions/provider.ts"], extensionsEnabled: true }, + extensionSession, + } as unknown as HistoryRuntime; +} + +describe("embedded history review bootstrap", () => { + test("preserves opaque actions, invocation-relative extensions, cwd, theme, and signal", async () => { + const abort = new AbortController(); + let captured: { argv: string[]; deps: Record } | undefined; + const runtime = createTestRuntime(); + const result = await prepareEmbeddedHistoryReview( + runtime, + { kind: "revision-show", revisionId: "--opaque:id" }, + { + themeId: "github-dark", + themeMode: "dark", + signal: abort.signal, + env: {}, + prepareStartupPlanImpl: (async (argv: string[], deps: Record) => { + captured = { argv, deps }; + return { + kind: "app", + bootstrap: { extensions: runtime.extensionSession }, + cliInput: {}, + controllingTerminal: null, + }; + }) as never, + }, + ); + + expect(result.bootstrap).toBeDefined(); + expect(captured?.argv).toContain(resolve("invocation", "extensions/provider.ts")); + expect(captured?.deps).toMatchObject({ + cwd: resolve("invocation"), + terminalThemeMode: "dark", + signal: abort.signal, + }); + expect(captured?.deps.borrowedExtensionLoad).toBe(runtime.extensionSession); + expect(result.borrowsExtensions).toBe(true); + expect(captured?.argv.join(" ")).not.toContain("--opaque:id"); + }); + + test("refuses an already-cancelled bootstrap before startup", async () => { + const abort = new AbortController(); + abort.abort(); + let called = false; + await expect( + prepareEmbeddedHistoryReview( + createTestRuntime(), + { kind: "revision-show", revisionId: "opaque" }, + { + signal: abort.signal, + prepareStartupPlanImpl: (async () => { + called = true; + return { kind: "help", text: "unexpected" }; + }) as never, + }, + ), + ).rejects.toThrow(); + expect(called).toBe(false); + }); +}); diff --git a/src/ui/log/reviewLaunch.test.ts b/src/ui/log/reviewLaunch.test.ts deleted file mode 100644 index f4f9e5d17..000000000 --- a/src/ui/log/reviewLaunch.test.ts +++ /dev/null @@ -1,127 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import type { ChildProcess, spawn } from "node:child_process"; -import { EventEmitter } from "node:events"; -import { PassThrough } from "node:stream"; -import type { HistoryRuntime } from "../history/types"; -import { prepareHistoryReview } from "./reviewLaunch"; - -/** Provide the launch fields used by the process adapter without constructing a repository cursor. */ -function createTestRuntime() { - return { - repoRoot: "/repo", - providerId: "opaque-vcs", - input: { extensionPaths: [], extensionsEnabled: false }, - } as unknown as HistoryRuntime; -} - -/** Emulate the bounded ChildProcess surface used by the handoff orchestration. */ -function createTestChild({ ignoreTerm = false }: { ignoreTerm?: boolean } = {}) { - const child = new EventEmitter() as ChildProcess & { - sent: unknown[]; - exitCode: number | null; - signalCode: NodeJS.Signals | null; - connected: boolean; - }; - child.sent = []; - child.exitCode = null; - child.signalCode = null; - child.connected = true; - child.stderr = new PassThrough(); - child.send = ((message: unknown, callback?: (error: Error | 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 f982678d9..ddbcd01cd 100644 --- a/src/ui/log/reviewLaunch.ts +++ b/src/ui/log/reviewLaunch.ts @@ -1,183 +1,7 @@ -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. */ +/** Convert a provider-owned review declaration into one option-safe internal 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]; } - -/** 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, - 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), - ]); - const args = [ - ...current.args, - ...historyReviewArgs(action), - "--vcs", - runtime.providerId, - ...(themeId ? ["--theme", themeId] : []), - ...(runtime.input.extensionsEnabled ? extensionArgs : ["--no-extensions"]), - ]; - const child = spawnImpl(current.command, args, { - cwd: runtime.repoRoot, - env: terminalHandoffEnv({ ...env, HUNK_RETURN_TO_HISTORY: "1" }, themeMode), - stdio: ["inherit", "inherit", "pipe", "ipc"], - }); - 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 518d7b532..bdb59d737 100644 --- a/src/ui/log/runInteractiveLog.tsx +++ b/src/ui/log/runInteractiveLog.tsx @@ -12,11 +12,10 @@ import { installTerminalDisconnectSupport, type TerminalDisconnectSupport, } from "../../core/process/terminal"; -import { LogApp, type LogAppOutcome } from "./LogApp"; -import { LogController } from "./controller"; -import { prepareHistoryReview, type PreparedHistoryReview } from "./reviewLaunch"; +import { disposeHighlightWorker } from "../diff/worker"; import type { HistoryRuntime } from "../history/types"; -import { interactiveLogUsesColor } from "./colorPolicy"; +import { LogController } from "./controller"; +import { LogSessionHost } from "./LogSessionHost"; const LOG_SHUTDOWN_SIGNALS: NodeJS.Signals[] = process.platform === "win32" @@ -28,134 +27,86 @@ export function logSignalExitCode(signal: NodeJS.Signals) { return signal === "SIGINT" ? 130 : signal === "SIGHUP" ? 129 : 143; } -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, +/** Browse history and fresh commit reviews inside one renderer and one stable React root. */ +export async function runInteractiveLog( runtime: HistoryRuntime, - stdin: NodeJS.ReadStream, - stdout: NodeJS.WriteStream, + { + stdin = process.stdin, + stdout = process.stdout, + }: { stdin?: NodeJS.ReadStream; stdout?: NodeJS.WriteStream } = {}, ) { - const renderer = await createCliRenderer({ - stdin, - stdout, - useMouse: true, - screenMode: "alternate-screen", - exitOnCtrlC: false, - exitSignals: [], - openConsoleOnError: true, - }); - let root: ReturnType; - try { - root = createRoot(renderer); - } catch (error) { - renderer.destroy(); - throw error; + if (!stdin.isTTY || !stdout.isTTY || typeof stdin.setRawMode !== "function") { + await runtime.close(); + throw new HunkUserError("The `hunk log` browser requires a terminal.", [ + "Use `hunk log --static` to force scrollback output.", + ]); } + + const controller = new LogController(runtime); + const quitController = new AbortController(); + let renderer: Awaited> | undefined; + let root: ReturnType | undefined; + let interrupt: JobControlInterruptSupport = { dispose: () => undefined }; + let suspend: JobControlSuspendSupport = { dispose: () => undefined }; + let disconnect: TerminalDisconnectSupport = { dispose: () => undefined }; let settled = false; - let settle!: (outcome: MountedLogOutcome) => void; - const outcome = new Promise((resolve) => { - settle = resolve; + let finish!: (exitCode?: number) => void; + const outcome = new Promise((resolve) => { + finish = (exitCode) => { + if (settled) return; + settled = true; + resolve(exitCode); + }; }); - 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 = () => quitController.abort(); + const requestInterrupt = () => { + process.exitCode = 130; + quitController.abort(); }; - const requestQuit = () => finish({ kind: "quit" }); - const requestInterrupt = () => finish({ kind: "quit", exitCode: 130 }); const signalHandlers = new Map void>( LOG_SHUTDOWN_SIGNALS.map((signal) => [ signal, - () => - finish({ - kind: "quit", - exitCode: logSignalExitCode(signal), - }), + () => { + process.exitCode = logSignalExitCode(signal); + quitController.abort(); + }, ]), ); - for (const [signal, handler] of signalHandlers) process.once(signal, handler); - let interrupt: JobControlInterruptSupport = { dispose: () => undefined }; - let suspend: JobControlSuspendSupport = { dispose: () => undefined }; - let disconnect: TerminalDisconnectSupport = { dispose: () => undefined }; + try { + await controller.loadMore(); + renderer = await createCliRenderer({ + stdin, + stdout, + useMouse: true, + screenMode: "alternate-screen", + exitOnCtrlC: false, + exitSignals: [], + openConsoleOnError: true, + }); + root = createRoot(renderer); interrupt = installJobControlInterruptSupport(renderer, requestInterrupt); suspend = installJobControlSuspendSupport(renderer); disconnect = installTerminalDisconnectSupport(stdin, requestQuit); + for (const [signal, handler] of signalHandlers) process.once(signal, handler); root.render( - , ); - return await outcome; + const exitCode = await outcome; + if (exitCode !== undefined) process.exitCode = exitCode; } finally { - if (!preparedLaunch) launchAbort.abort(); for (const [signal, handler] of signalHandlers) process.off(signal, handler); interrupt.dispose(); suspend.dispose(); disconnect.dispose(); - shutdownSession({ root, renderer, exit: () => undefined }); - } -} - -/** Browse history in shared desktop chrome, yielding fully to each child review. */ -export async function runInteractiveLog( - runtime: HistoryRuntime, - { - stdin = process.stdin, - stdout = process.stdout, - }: { stdin?: NodeJS.ReadStream; stdout?: NodeJS.WriteStream } = {}, -) { - if (!stdin.isTTY || !stdout.isTTY || typeof stdin.setRawMode !== "function") { - await runtime.close(); - throw new HunkUserError("The `hunk log` browser requires a terminal.", [ - "Use `hunk log --static` to force scrollback output.", - ]); - } - - const controller = new LogController(runtime); - try { - await controller.loadMore(); - for (;;) { - const outcome = await mountLogSurface(controller, runtime, stdin, stdout); - if (outcome.kind === "quit") { - if (outcome.exitCode !== undefined) process.exitCode = outcome.exitCode; - return; - } - try { - 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)); - } - } - } finally { + disposeHighlightWorker(); + if (root && renderer) shutdownSession({ root, renderer, exit: () => undefined }); + else renderer?.destroy(); await controller.close(); } } diff --git a/src/ui/runInteractiveApp.tsx b/src/ui/runInteractiveApp.tsx index ed1a7e00c..05af1108f 100644 --- a/src/ui/runInteractiveApp.tsx +++ b/src/ui/runInteractiveApp.tsx @@ -1,6 +1,7 @@ import { createNativeSessionBrokerLifecycleClock } from "@hunk/session-broker"; import { createCliRenderer } from "@opentui/core"; import { createRoot } from "@opentui/react"; +import { resolve } from "node:path"; import { installJobControlInterruptSupport, installJobControlSuspendSupport, @@ -16,6 +17,7 @@ import { } from "../core/process/terminal"; import type { AppBootstrap } from "../core/bootstrap"; import { resolveStartupUpdateNotice } from "../core/process/updateNotice"; +import { prepareStartupPlan } from "../app/startup"; import { ReviewProducer } from "../app/review/producer"; import { createInitialSessionSnapshot, @@ -23,6 +25,9 @@ import { } from "../app/session/registration"; import { SessionBrokerClient } from "../session/broker/brokerClient"; import { reportHunkSessionBrokerLifecycleDefect } from "../session/broker/lifecycleDefect"; +import type { ExtensionVcsHistoryReviewAction } from "../extension-api/types"; +import type { HistoryRuntime } from "./history/types"; +import { historyReviewArgs } from "./log/reviewLaunch"; import { AppHost } from "./AppHost"; import { disposeHighlightWorker } from "./diff/worker"; import { retireExtensionLoadResult } from "../extensions/events"; @@ -33,20 +38,23 @@ export interface InteractiveAppInput { controllingTerminal: ControllingTerminal | null; } -// Leave fatal process faults to their default OS disposition. -const APP_SHUTDOWN_SIGNALS: NodeJS.Signals[] = - process.platform === "win32" - ? ["SIGINT", "SIGTERM", "SIGBREAK"] - : ["SIGINT", "SIGTERM", "SIGHUP", "SIGQUIT", "SIGPIPE"]; +export interface ReviewSessionRuntime { + hostClient: SessionBrokerClient; + reviewProducer: ReviewProducer; + stop(): void; +} -/** Load and run the OpenTUI review app after startup has selected an interactive plan. */ -export async function runInteractiveApp({ - bootstrap, - controllingTerminal, -}: InteractiveAppInput): Promise { - // One producer owns this review's generations for the life of the process: the - // registration and the first snapshot are projections of its first publication, and every - // reload publishes the next one through the same object. +export interface EmbeddedHistoryReview { + bootstrap: AppBootstrap; + /** The history runtime owns this bootstrap's extension registry. */ + borrowsExtensions: boolean; +} + +/** Create broker and producer resources for one independently mountable review surface. */ +export function createReviewSessionRuntime( + bootstrap: AppBootstrap, + cwd = process.cwd(), +): ReviewSessionRuntime { const reviewProducer = new ReviewProducer({ files: bootstrap.changeset.files, sourceLabel: bootstrap.changeset.sourceLabel, @@ -54,11 +62,93 @@ export async function runInteractiveApp({ const publication = reviewProducer.getPublication(); const lifecycleClock = createNativeSessionBrokerLifecycleClock(); const hostClient = new SessionBrokerClient( - createSessionRegistration(bootstrap, publication), + createSessionRegistration(bootstrap, publication, cwd), createInitialSessionSnapshot(bootstrap, publication), { lifecycleClock, onDefect: reportHunkSessionBrokerLifecycleDefect }, ); hostClient.start(); + let stopped = false; + return { + hostClient, + reviewProducer, + stop() { + if (stopped) return; + stopped = true; + hostClient.stop(); + }, + }; +} + +/** Bootstrap one provider-planned history review without creating or claiming a renderer. */ +export async function prepareEmbeddedHistoryReview( + runtime: HistoryRuntime, + action: ExtensionVcsHistoryReviewAction, + { + themeId, + themeMode, + signal, + env = process.env, + prepareStartupPlanImpl = prepareStartupPlan, + }: { + themeId?: string; + themeMode?: "dark" | "light"; + signal?: AbortSignal; + env?: NodeJS.ProcessEnv; + prepareStartupPlanImpl?: typeof prepareStartupPlan; + } = {}, +): Promise { + signal?.throwIfAborted(); + const startupCwd = runtime.startupCwd ?? runtime.repoRoot; + const extensionArgs = runtime.input.extensionPaths.flatMap((path) => [ + "--extension", + resolve(startupCwd, path), + ]); + const args = [ + ...historyReviewArgs(action), + "--vcs", + runtime.providerId, + ...(themeId ? ["--theme", themeId] : []), + ...(runtime.input.extensionsEnabled ? extensionArgs : ["--no-extensions"]), + ]; + const plan = await prepareStartupPlanImpl(["hunk", "hunk", ...args], { + cwd: startupCwd, + env, + signal, + borrowedExtensionLoad: runtime.extensionSession, + stdinIsTTY: true, + stdoutIsTTY: true, + terminalThemeMode: themeMode, + }); + if (signal?.aborted && plan.kind === "app") { + plan.controllingTerminal?.close(); + if (plan.bootstrap.extensions !== runtime.extensionSession) { + await retireExtensionLoadResult(plan.bootstrap.extensions); + } + signal.throwIfAborted(); + } + if (plan.kind !== "app") { + throw new Error("The selected commit did not produce an interactive review."); + } + plan.controllingTerminal?.close(); + return { + bootstrap: plan.bootstrap as AppBootstrap, + borrowsExtensions: plan.bootstrap.extensions === runtime.extensionSession, + }; +} + +// Leave fatal process faults to their default OS disposition. +const APP_SHUTDOWN_SIGNALS: NodeJS.Signals[] = + process.platform === "win32" + ? ["SIGINT", "SIGTERM", "SIGBREAK"] + : ["SIGINT", "SIGTERM", "SIGHUP", "SIGQUIT", "SIGPIPE"]; + +/** Load and run the OpenTUI review app after startup has selected an interactive plan. */ +export async function runInteractiveApp({ + bootstrap, + controllingTerminal, +}: InteractiveAppInput): Promise { + const reviewSession = createReviewSessionRuntime(bootstrap, bootstrap.reloadContext.cwd); + const { hostClient, reviewProducer } = reviewSession; // Keep OpenTUI's platform-safe threading default (enabled on macOS, disabled on Linux). const rendererStdin = controllingTerminal?.stdin ?? process.stdin; @@ -78,7 +168,7 @@ export async function runInteractiveApp({ onDestroy: () => controllingTerminal?.close(), }); } catch (error) { - hostClient.stop(); + reviewSession.stop(); controllingTerminal?.close(); await retireExtensionLoadResult(bootstrap.extensions); throw error; @@ -89,7 +179,7 @@ export async function runInteractiveApp({ try { root = createRoot(appRenderer); } catch (error) { - hostClient.stop(); + reviewSession.stop(); appRenderer.destroy(); controllingTerminal?.close(); await retireExtensionLoadResult(bootstrap.extensions); @@ -119,7 +209,7 @@ export async function runInteractiveApp({ jobControlInterruptSupport.dispose(); jobControlSuspendSupport.dispose(); terminalDisconnectSupport.dispose(); - hostClient.stop(); + reviewSession.stop(); // Release the syntax worker here rather than from the executable entrypoint: this function // returns once the app is mounted, so an entrypoint-side dispose would fire before the first // eligible diff ever asked for the worker. diff --git a/test/pty/log-integration.test.ts b/test/pty/log-integration.test.ts index 8e1528b97..75c789572 100644 --- a/test/pty/log-integration.test.ts +++ b/test/pty/log-integration.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { createPtyHarness, rightmostColumnOf } from "./harness"; @@ -71,6 +71,46 @@ afterEach(() => { }); describe("interactive hunk log", () => { + test("cancels a slow bundled Git review without blocking terminal input", async () => { + const cwd = createHistoryRepo(); + const binDir = mkdtempSync(join(tmpdir(), "hunk-slow-git-bin-")); + tempDirs.push(binDir); + const gitPath = Bun.which("git"); + if (!gitPath) throw new Error("Git is required for the PTY history fixture."); + const wrapper = join(binDir, "git"); + writeFileSync( + wrapper, + `#!/bin/sh\ncase " $* " in *" show "*|*" diff "*) sleep 10 ;; esac\nexec ${JSON.stringify(gitPath)} "$@"\n`, + ); + chmodSync(wrapper, 0o755); + const session = await harness.launchHunk({ + args: ["log", "--color", "never", "--no-extensions"], + cwd, + cols: 100, + rows: 20, + env: { PATH: `${binDir}:${process.env.PATH ?? ""}` }, + }); + + try { + await session.waitForText(/Second history commit/, { timeout: 15_000 }); + await session.press("enter"); + await session.waitForText(/Preparing review/, { timeout: 5_000 }); + const cancelledAt = Date.now(); + session.writeRaw("\x03"); + while ( + !session.getRawOutput().slice(-2_000).includes("\x1b[?1049l") && + Date.now() - cancelledAt < 3_000 + ) { + await Bun.sleep(25); + } + expect(Date.now() - cancelledAt).toBeLessThan(3_000); + expect(session.getRawOutput().slice(-2_000)).toContain("\x1b[?1049l"); + expect(session.getRawOutput()).not.toContain("historyValue = 'second'"); + } finally { + session.writeRaw("\x03"); + } + }); + test("opens the selected immutable commit and returns to the retained history", async () => { const cwd = createHistoryRepo(); const session = await harness.launchHunk({ @@ -105,23 +145,23 @@ describe("interactive hunk log", () => { const firstRow = history.split("\n")[firstRowIndex] ?? ""; const commitColumn = firstRow.search(/[0-9a-f]{8}\s+⧉\s*$/); expect(commitColumn).toBeGreaterThan(0); + const transitionOutputStart = session.getRawOutput().length; 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, }); expect(review).toContain("history.ts"); + expect(session.getRawOutput().slice(transitionOutputStart)).not.toContain("\x1b[?1049l"); + const returnOutputStart = session.getRawOutput().length; await session.press("q"); const returned = await session.waitForText(/Second history commit/, { timeout: 15_000, }); expect(returned).toContain("Enter open"); + expect(session.getRawOutput().slice(returnOutputStart)).not.toContain("\x1b[?1049l"); // The adjacent icon copies without opening the review. session.writeRaw(