diff --git a/.changeset/fuzzy-editors-dock.md b/.changeset/fuzzy-editors-dock.md new file mode 100644 index 000000000..1ce88f96b --- /dev/null +++ b/.changeset/fuzzy-editors-dock.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Let extension commands temporarily hand Hunk's terminal to an application, resolve filesystem-attested review locations, and run Hunk's responsive open-in-editor workflow as a bundled extension. diff --git a/docs/extension-architecture.md b/docs/extension-architecture.md index e386a2ff4..edd7c28d4 100644 --- a/docs/extension-architecture.md +++ b/docs/extension-architecture.md @@ -20,11 +20,12 @@ object and registry collection (`src/extensions/runExtension.ts`): by the app composition root (`app/vcsCatalog.ts`) and loaded synchronously before config resolution, so backends exist without making core import the extension host. `default/ui/index.ts` is deliberately not part of that list: - it synchronously loads the bundled files pane through `runExtensionFactory` - only where the app resolves UI panes. + it synchronously loads the bundled files pane and editor command through + `runExtensionFactory` only where the interactive app resolves UI contributions. -Git and the built-in file navigation use the public `registerVcsAdapter` and -`registerPane` paths. The external [Hunk Lens](https://github.com/modem-dev/hunk-lens) +Git, built-in file navigation, and open-in-editor workflow use the public +`registerVcsAdapter`, `registerPane`, and `registerCommand` paths. The external +[Hunk Lens](https://github.com/modem-dev/hunk-lens) extension exercises current-line pane paint through that same public contract. Bundled extensions are implicitly trusted and stay loaded under @@ -272,10 +273,20 @@ inert before shutdown begins. Session behavior requests are registry data too: presentation view changes ephemeral without teaching `App` about an extension id. +`src/ui/hooks/useExtensionAppController.ts` owns `ctx.openInApp`. Command-scoped +leases refuse stale handoffs, one shared lock prevents overlapping applications, +and renderer suspension always resumes in `finally` unless the renderer was +destroyed. The extension owns execution and application-specific metadata; +Hunk's bundled editor command consumes the same public callback and explicitly +refreshes after a successful edit. Dialog admission and workspace writes consult +the same ownership state so host UI cannot deadlock behind a suspended renderer. + `src/ui/lib/extensionWorkspace.ts` owns the policy for `ctx.workspace`. Reads resolve reviewed file ids through the existing source fetcher, which retains ownership of caching and size limits. Missing or unreadable sources become -`null`. +`null`. Location resolution maps reviewed file ids and source addresses onto +attested on-disk paths and lines using per-side provenance supplied by loaders +and VCS adapters plus the authoritative parsed hunk. Writes are limited to reloadable working-tree reviews and reviewed paths inside the review root. App supplies the current input, unfiltered changeset, and root diff --git a/docs/extensions.md b/docs/extensions.md index 549810a68..1c74845d6 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -280,9 +280,11 @@ new instances and run that shutdown/startup pair around the replacement. ### `hunk.apiVersion` -The API generation this Hunk speaks (currently `15`). Branch on it if you want -one file to support several Hunk versions. Version 15 adds `{ side, line }` to -opted-in pane `currentLine` paint; version 14 added structured `rangeEndpoints` +The API generation this Hunk speaks (currently `16`). Branch on it if you want +one file to support several Hunk versions. Version 16 adds temporary application +handoffs and on-disk location resolution to command handlers; version 15 added +`{ side, line }` to opted-in pane +`currentLine` paint; version 14 added structured `rangeEndpoints` to two-revision VCS diff requests; version 13 added saved-note parent identities and committed note-edit events; version 12 adds responsive fractional pane sizing; version 11 added the `"dim"` line-highlight tone; version 10 added generic top-level CLI commands; version 9 @@ -467,12 +469,13 @@ instead of a crash. A `load` result is patch text plus how to label it. Everything else on it is optional, and each optional field buys one thing: -| Field | What it adds | -| ---------------- | ------------------------------------------------------------------ | -| `untrackedPaths` | files your VCS calls unknown, synthesized into added-file diffs | -| `readFileSource` | exact whole-file contents, for context expansion and highlighting | -| `sourceCacheKey` | stable source-snapshot identity for highlight reuse across reloads | -| `extraFiles` | files reviewed outside the patch, including skipped placeholders | +| Field | What it adds | +| ----------------------- | ------------------------------------------------------------------ | +| `untrackedPaths` | files your VCS calls unknown, synthesized into added-file diffs | +| `readFileSource` | exact whole-file contents, for context expansion and highlighting | +| `resolveFileSourcePath` | exact filesystem provenance for application location handoff | +| `sourceCacheKey` | stable source-snapshot identity for highlight reuse across reloads | +| `extraFiles` | files reviewed outside the patch, including skipped placeholders | `untrackedPaths` is the shorthand: list the repo-root-relative paths your VCS reports as unknown and Hunk synthesizes the added-file diffs for you, skipping @@ -585,6 +588,10 @@ async load(input, ctx) { } return changeType === "deleted" ? null : hgCat(newRev, path); }, + resolveFileSourcePath: ({ path, changeType, side }) => { + if (side !== "new" || changeType === "deleted" || input.range) return null; + return join(ctx.cwd, path); + }, }; } ``` @@ -604,6 +611,15 @@ stable identity and Hunk will invalidate conservatively. Leaving `readFileSource` off is fine: Hunk falls back to the content the patch itself carries, which renders the same diff with less context available. +`resolveFileSourcePath` is separate from source reads because a binary or skipped +file can still have a real path. Return an absolute path only when that exact +reviewed side is backed by the filesystem. Return `null` for absent sides and +for index, revision, stash, patch, merged, or other virtual sources, even when a +same-named working-tree file exists. Hunk uses this provenance for +`ctx.workspace.resolveLocation`; it never invents a checkout path for historical +content. Direct file and difftool comparisons retain their concrete input paths +independently of their display names. + #### Files outside the patch `extraFiles` lists files to review that your `patchText` does not contain, in @@ -1679,6 +1695,47 @@ the same way, and a request made after that point cancels immediately. A blank answer from the user, so the promise **rejects**; like any other handler failure, that surfaces as a warning naming your extension. +#### Temporary applications + +`ctx.openInApp(callback)` temporarily replaces Hunk with an application your +extension runs. Hunk suspends its renderer before calling you and restores the +review in `finally` after your callback returns or throws: + +```ts +async function runProjectTool(metadata: { file: string | undefined; line: number | undefined }) { + // Spawn an interactive process with inherited stdio and encode metadata however the app expects. + return { exitCode: 0, metadata }; +} + +hunk.registerCommand({ id: "open-tool", title: "Open project tool", key: "f8" }, async (ctx) => { + const file = ctx.selection.file; + const location = file + ? ctx.workspace.resolveLocation({ + fileId: file.id, + ...(ctx.selection.hunkIndex === null ? {} : { hunkIndex: ctx.selection.hunkIndex }), + ...(ctx.selection.currentLine === null ? {} : { line: ctx.selection.currentLine }), + }) + : null; + const result = await ctx.openInApp(() => + runProjectTool({ + file: location?.path, + line: location?.line, + }), + ); + if (result.exitCode !== 0) ctx.notify(`Tool exited with status ${result.exitCode}`, "error"); +}); +``` + +The extension owns process execution and decides how to pass file, line, hunk, +or extension state through arguments, environment, files, or an application-specific +protocol. Hunk only owns terminal suspension and restoration. One application +may own the terminal at a time; concurrent calls and controls retained past a +review reload reject without invoking the callback. The callback's value and +error pass through unchanged. Host-presented dialogs cancel immediately and +workspace writes return `unavailable` while the callback owns the terminal, so +do not await Hunk UI from inside it. Non-interactive workspace reads and location +resolution remain available. + #### Workspace documents `ctx.workspace` reads full documents from the current review and can replace an @@ -1687,6 +1744,7 @@ eligible working-tree file. | Method | Result | | -------------------------------------- | ------------------------------------------------- | | `readDocument(fileId, "old" \| "new")` | The reviewed source text, or `null` | +| `resolveLocation({ fileId, ... })` | Absolute on-disk `{ path, line }`, or `null` | | `canWriteDocument(fileId)` | Whether the review and file allow writes | | `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | @@ -1718,6 +1776,17 @@ returns `null` when the file or side is absent, no source is available, reading fails, or the document exceeds Hunk's size limit. Reads never prompt. An invalid side rejects the promise. +`resolveLocation` turns a reviewed file id and optional `hunkIndex` and +`{ side, line }` into an attested absolute path and one-based line on disk. Hunk +uses parsed hunk metadata to map old-side deletions onto a filesystem-backed new +side, so extensions can pass accurate locations to editors, debuggers, browsers, +or other applications without interpreting opaque diff metadata. Direct file +comparisons retain their concrete input paths, including the old path for a +deleted-file comparison. Index, revision, stash, patch, merged, absent, and +other virtual sides return `null` instead of borrowing a same-named checkout +file. Missing hunks and stale controls also return `null`; malformed source +addresses reject. + Writes require all of the following: - an unstaged working-tree review (`hunk diff` with no revision range) @@ -1793,9 +1862,10 @@ ready resolve to their cancel value with a warning rather than opening later. Controls retained across a review or extension-registry replacement expire: navigation and pane mutations warn and do nothing, dialogs resolve to their normal cancel value, and workspace reads or not-yet-started writes return -`null`/`unavailable` instead of acting on replacement content. Once a consented -filesystem write starts, it reports its actual outcome and success reconciles -the review then active. +`null`/`unavailable` instead of acting on replacement content. A stale +`openInApp` callback rejects before taking terminal ownership. +Once a consented filesystem write starts, it reports its actual outcome and +success reconciles the review then active. | Event | Payload | When | | ---------------------- | ----------------------- | --------------------------------------------------------- | diff --git a/skills/hunk-extensions/SKILL.md b/skills/hunk-extensions/SKILL.md index 9bb72282d..a024daf5e 100644 --- a/skills/hunk-extensions/SKILL.md +++ b/skills/hunk-extensions/SKILL.md @@ -110,7 +110,7 @@ bad or duplicate id is skipped with a startup notice. | Coordinate with another loaded extension | `hunk.events.emit` / `hunk.events.on` | | Read user-supplied settings | `hunk.config` (`[extension.]` table) | | Snapshot stable files and every saved review note | `ctx.review.snapshot()` in a command | -| Branch on the API generation (currently `15`) | `hunk.apiVersion` | +| Branch on the API generation (currently `16`) | `hunk.apiVersion` | Registration is only valid while the factory runs — Hunk seals the API object afterwards. @@ -160,8 +160,10 @@ transform — gets `ctx.cwd` and `ctx.notify(message, type?)`. A file view's (`isEnabled`/`execute` for public semantic `hunk.*` commands), `ctx.keyboardModes` (enter/exit/probe this extension's session modes), `ctx.review` (deeply immutable snapshots of stable files and complete saved store notes), - `ctx.dialogs` (`confirm`/`select`/`input`, queued and attributed), and - `ctx.workspace` (`readDocument`, `canWriteDocument`, `writeDocument` with consent). + `ctx.dialogs` (`confirm`/`select`/`input`, queued and attributed), + `ctx.openInApp` (temporary terminal ownership around extension-run applications), + and `ctx.workspace` (`readDocument`, `resolveLocation`, `canWriteDocument`, + `writeDocument` with consent). - **Pane components** get frozen `files`, selection, placement, exact dimensions, optional `currentLine` paint (with `{ side, line }` when opted in), semantic `theme`, resolved `keybindings`, and guarded navigation/notification `actions`. @@ -214,8 +216,9 @@ Most extension bugs are one of these: `review-note-navigator` shows how to join stable note ids and file keys back to guarded navigation after awaiting a selector; file filters can still refuse hidden targets. - **Retained review controls expire on reload.** An old handler cannot control - replacement content: pane/navigation calls become inert, dialogs cancel, and - workspace reads or not-yet-started writes return `null`/`unavailable`. A + replacement content: pane/navigation calls become inert, dialogs cancel, + stale app handoffs reject, and workspace reads or not-yet-started writes + return `null`/`unavailable`. A consented write already in progress reports its real outcome, holds graceful exit until it settles, and reconciles the active review on success. `shutdown` runs after revocation, so use it only @@ -252,10 +255,13 @@ Most extension bugs are one of these: - **Failures are contained, not sandboxed.** A throwing factory is rolled back to zero registrations and a throwing handler is a warning naming the extension — containment against bugs, not against code that should not have been loaded. -- **The API touches nothing outside the review.** No clipboard, no filesystem, no - process surface beyond `ctx.workspace` — an extension is ordinary code, so shell - out for the rest. Never write to stdout: the renderer owns it. For the same - reason `hunk.log` is collected as diagnostics and printed nowhere; `ctx.notify` +- **Application execution stays extension-owned.** Extensions are ordinary + trusted code and may spawn processes; use `ctx.openInApp` when one needs the + terminal so Hunk suspends and restores its renderer. `ctx.workspace.resolveLocation` + maps only filesystem-attested reviewed sides to app-ready paths and lines. + Dialogs cancel and writes refuse while an app owns the terminal, so do not + await host UI inside the callback. Never write to stdout while + Hunk owns the terminal; `hunk.log` is collected as diagnostics and `ctx.notify` is how a user hears from you. - **`HunkExtensionUserError`** (detected structurally by `name`) buys the full treatment — message plus `suggestions`, no stack trace — only from a VCS adapter diff --git a/src/core/changeset/diffFile.test.ts b/src/core/changeset/diffFile.test.ts index d137fd25e..2fe9fb59c 100644 --- a/src/core/changeset/diffFile.test.ts +++ b/src/core/changeset/diffFile.test.ts @@ -105,6 +105,23 @@ describe("buildDiffFile", () => { isBinary: false, }); }); + + test("retains source paths for binary files independently of source fetching", () => { + let fetched = false; + const file = buildDiffFile(metadata, "Binary files a/x and b/x differ\n", 0, "src", null, { + sourceFetcherBuilder: () => { + fetched = true; + return undefined; + }, + sourcePathBuilder: (context) => { + expect(context.isBinary).toBe(true); + return { old: "/repo/old.png", new: "/repo/new.png" }; + }, + }); + + expect(fetched).toBe(true); + expect(file.sourcePaths).toEqual({ old: "/repo/old.png", new: "/repo/new.png" }); + }); }); describe("change-block line pairing", () => { diff --git a/src/core/changeset/diffFile.ts b/src/core/changeset/diffFile.ts index dfb603df1..97049c5ba 100644 --- a/src/core/changeset/diffFile.ts +++ b/src/core/changeset/diffFile.ts @@ -3,7 +3,7 @@ import { findSidecarFileContext } from "./sidecar"; import { patchLooksBinary } from "./binary"; import { fileLanguageForPath } from "./fileLanguageLookup"; import { normalizeDiffMetadataPaths, normalizeDiffPath } from "./diffPaths"; -import type { FileSourceFetcher } from "./fileSource"; +import type { FileSourceFetcher, FileSourcePaths } from "./fileSource"; import type { DiffFile, DiffLineMoveKinds, SidecarContext } from "./model"; /** Count visible additions and deletions from parsed diff metadata. */ @@ -36,6 +36,7 @@ export interface BuildDiffFileOptions { previousPath?: string; isBinary?: boolean; sourceFetcherBuilder?: (file: DiffFileSourceContext) => FileSourceFetcher | undefined; + sourcePathBuilder?: (file: DiffFileSourceContext) => FileSourcePaths | undefined; isTooLarge?: boolean; stats?: DiffFile["stats"]; statsTruncated?: boolean; @@ -55,6 +56,7 @@ export function buildDiffFile( previousPath, isBinary, sourceFetcherBuilder, + sourcePathBuilder, isTooLarge, stats, statsTruncated, @@ -69,13 +71,15 @@ export function buildDiffFile( : (normalizeDiffPath(previousPath) ?? normalizedMetadata.prevName); const resolvedIsBinary = isBinary ?? patchLooksBinary(patch); const language = fileLanguageForPath(path); - const sourceFetcher = sourceFetcherBuilder?.({ + const sourceContext = { path, previousPath: resolvedPreviousPath, type: normalizedMetadata.type, isUntracked: Boolean(isUntracked), isBinary: resolvedIsBinary, - }); + } satisfies DiffFileSourceContext; + const sourceFetcher = sourceFetcherBuilder?.(sourceContext); + const sourcePaths = sourcePathBuilder?.(sourceContext); return { id: `${sourcePrefix}:${index}:${path}`, @@ -93,6 +97,7 @@ export function buildDiffFile( isTooLarge, statsTruncated, sourceFetcher, + sourcePaths, }; } diff --git a/src/core/changeset/fileSource.test.ts b/src/core/changeset/fileSource.test.ts index 00424460d..8b2302aee 100644 --- a/src/core/changeset/fileSource.test.ts +++ b/src/core/changeset/fileSource.test.ts @@ -2,7 +2,11 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { createFileSourceFetcher, SourceTextTooLargeError } from "./fileSource"; +import { + createFileSourceFetcher, + fileSourcePathsForSpecs, + SourceTextTooLargeError, +} from "./fileSource"; const tempDirs: string[] = []; @@ -22,6 +26,15 @@ afterEach(() => { }); describe("createFileSourceFetcher", () => { + test("projects only filesystem-backed specs to source paths", () => { + expect( + fileSourcePathsForSpecs({ + old: { kind: "none" }, + new: { kind: "fs", absolutePath: join("/repo", "after.txt") }, + }), + ).toEqual({ old: null, new: join("/repo", "after.txt") }); + }); + test("reads fs paths for old and new sides", async () => { const dir = createTempDir("hunk-source-fs-"); const left = join(dir, "before.txt"); diff --git a/src/core/changeset/fileSource.ts b/src/core/changeset/fileSource.ts index b654fdfd8..b45131892 100644 --- a/src/core/changeset/fileSource.ts +++ b/src/core/changeset/fileSource.ts @@ -15,6 +15,18 @@ export type FileSourceSpec = { kind: "none" } | { kind: "fs"; absolutePath: stri export type FileSourceSide = "old" | "new"; +/** Exact filesystem paths for the reviewed sides, or null when a side is not filesystem-backed. */ +export interface FileSourcePaths { + readonly old: string | null; + readonly new: string | null; +} + +/** Generic source specs for both sides of one reviewed file. */ +export interface FileSourceSpecs { + old: FileSourceSpec; + new: FileSourceSpec; +} + export interface FileSourceFetcher { /** Stable identity for source state not already represented by the file's patch. */ readonly cacheKey?: string; @@ -39,11 +51,6 @@ export interface FileSourceFetcherOptions { maxSourceBytes?: number; } -interface ResolvedSpecs { - old: FileSourceSpec; - new: FileSourceSpec; -} - async function readFsSpec( spec: Extract, maxSourceBytes: number, @@ -67,9 +74,17 @@ export async function readFileSourceSpec( return readFsSpec(spec, maxSourceBytes); } +/** Project source specs to the exact paths of only their filesystem-backed sides. */ +export function fileSourcePathsForSpecs(specs: FileSourceSpecs): FileSourcePaths { + return { + old: specs.old.kind === "fs" ? specs.old.absolutePath : null, + new: specs.new.kind === "fs" ? specs.new.absolutePath : null, + }; +} + /** Build a per-file source fetcher that caches each side's resolved text. */ export function createFileSourceFetcher( - specs: ResolvedSpecs, + specs: FileSourceSpecs, { maxSourceBytes = DEFAULT_SOURCE_TEXT_MAX_BYTES }: Readonly = {}, ): FileSourceFetcher { const cache = new Map(); diff --git a/src/core/changeset/fromPatch.ts b/src/core/changeset/fromPatch.ts index b07a1c354..a88a5afea 100644 --- a/src/core/changeset/fromPatch.ts +++ b/src/core/changeset/fromPatch.ts @@ -128,7 +128,7 @@ export function changesetFromPatch( title: string, sourceLabel: string, sidecar: SidecarContext | null, - perFileOptions?: Pick, + perFileOptions?: Pick, ): Changeset { const lineMoveKinds = collectLineMoveKinds(patchText); const sanitizedPatch = sanitizePatch(patchText); diff --git a/src/core/changeset/loaders.test.ts b/src/core/changeset/loaders.test.ts index 9950f19da..b9e008d78 100644 --- a/src/core/changeset/loaders.test.ts +++ b/src/core/changeset/loaders.test.ts @@ -398,6 +398,8 @@ describe("loadAppBootstrap", () => { expect(bootstrap.changeset.files[0]?.previousPath).toBe("before.png"); expect(bootstrap.changeset.files[0]?.isBinary).toBe(true); expect(bootstrap.changeset.files[0]?.metadata.hunks).toHaveLength(0); + expect(bootstrap.changeset.files[0]?.sourceFetcher).toBeUndefined(); + expect(bootstrap.changeset.files[0]?.sourcePaths).toEqual({ old: left, new: right }); }); test("marks git binary diffs as skipped binary content", async () => { @@ -513,6 +515,10 @@ describe("loadAppBootstrap", () => { }); expect(bootstrap.changeset.files[0]?.metadata.hunks).toHaveLength(0); expect(bootstrap.changeset.files[0]?.sourceFetcher).toBeUndefined(); + expect(bootstrap.changeset.files[0]?.sourcePaths?.old).toBeNull(); + expect(normalizeComparablePath(bootstrap.changeset.files[0]!.sourcePaths!.new!)).toBe( + normalizeComparablePath(join(dir, "large.txt")), + ); }); test("keeps generated large untracked files as skipped placeholders", async () => { @@ -539,6 +545,10 @@ describe("loadAppBootstrap", () => { expect(bootstrap.changeset.files[0]?.statsTruncated).toBe(false); expect(bootstrap.changeset.files[0]?.metadata.hunks).toHaveLength(0); expect(bootstrap.changeset.files[0]?.sourceFetcher).toBeUndefined(); + expect(bootstrap.changeset.files[0]?.sourcePaths?.old).toBeNull(); + expect(normalizeComparablePath(bootstrap.changeset.files[0]!.sourcePaths!.new!)).toBe( + normalizeComparablePath(join(dir, "large.txt")), + ); }); test("caps skipped untracked-file stats when byte-size detection would require a full huge read", async () => { @@ -1853,8 +1863,59 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(file?.sourceFetcher).toBeDefined(); expect(await file?.sourceFetcher?.getFullText("old")).toBe("old\n"); expect(await file?.sourceFetcher?.getFullText("new")).toBe("new\n"); + expect(file?.sourcePaths).toEqual({ old: left, new: right }); + }); + + test("difftool keeps concrete side paths instead of its display path", async () => { + const dir = createTempDir("hunk-source-difftool-"); + const left = join(dir, "before.ts"); + const right = join(dir, "after.ts"); + writeFileSync(left, "old\n"); + writeFileSync(right, "new\n"); + + const bootstrap = await loadAppBootstrap({ + kind: "difftool", + left, + right, + path: "display/renamed.ts", + options: {}, + }); + + expect(bootstrap.changeset.files[0]?.path).toBe("display/renamed.ts"); + expect(bootstrap.changeset.files[0]?.sourcePaths).toEqual({ old: left, new: right }); }); + test.skipIf(platform() === "win32")( + "marks /dev/null as an absent direct-comparison side", + async () => { + const dir = createTempDir("hunk-source-dev-null-"); + const right = join(dir, "added.ts"); + writeFileSync(right, "new\n"); + + const bootstrap = await loadAppBootstrap({ + kind: "diff", + left: "/dev/null", + right, + options: {}, + }); + const file = bootstrap.changeset.files[0]; + + expect(file?.metadata.type).toBe("new"); + expect(file?.sourcePaths).toEqual({ old: null, new: right }); + expect(await file?.sourceFetcher?.getFullText("old")).toBeNull(); + + const deleted = await loadAppBootstrap({ + kind: "diff", + left: right, + right: "/dev/null", + options: {}, + }); + expect(deleted.changeset.files[0]?.metadata.type).toBe("deleted"); + expect(deleted.changeset.files[0]?.sourcePaths).toEqual({ old: right, new: null }); + expect(await deleted.changeset.files[0]?.sourceFetcher?.getFullText("new")).toBeNull(); + }, + ); + test("git working-tree diffs read the new side from the working tree and the old side from the index", async () => { const dir = createTempRepo("hunk-source-git-wt-"); writeFileSync(join(dir, "value.txt"), "first\n"); @@ -1873,6 +1934,10 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(file?.sourceFetcher).toBeDefined(); expect(await file?.sourceFetcher?.getFullText("new")).toBe("second\n"); expect(await file?.sourceFetcher?.getFullText("old")).toBe("first\n"); + expect(file?.sourcePaths?.old).toBeNull(); + expect(normalizeComparablePath(file!.sourcePaths!.new!)).toBe( + normalizeComparablePath(join(dir, "value.txt")), + ); }); test("git source fetchers use the custom git executable from bootstrap loading", async () => { @@ -1979,6 +2044,7 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(file?.sourceFetcher).toBeDefined(); expect(await file?.sourceFetcher?.getFullText("new")).toBe("second\n"); expect(await file?.sourceFetcher?.getFullText("old")).toBe("first\n"); + expect(file?.sourcePaths).toBeUndefined(); }); test("`hunk show ` refuses to expand source blobs above the source cap", async () => { @@ -2072,6 +2138,10 @@ describe("loadAppBootstrap source fetcher attachment", () => { expect(untracked?.sourceFetcher).toBeDefined(); expect(await untracked?.sourceFetcher?.getFullText("new")).toBe("added contents\n"); expect(await untracked?.sourceFetcher?.getFullText("old")).toBeNull(); + expect(untracked?.sourcePaths?.old).toBeNull(); + expect(normalizeComparablePath(untracked!.sourcePaths!.new!)).toBe( + normalizeComparablePath(join(dir, "added.txt")), + ); }); test("deleted Unicode files attach a fetcher with new=null and old source", async () => { diff --git a/src/core/changeset/loaders.ts b/src/core/changeset/loaders.ts index aece09bc4..2e919bd0b 100644 --- a/src/core/changeset/loaders.ts +++ b/src/core/changeset/loaders.ts @@ -12,7 +12,12 @@ import { resolve as resolvePath } from "node:path"; import { findSidecarFileContext, loadSidecarContext } from "./sidecar"; import { createSkippedBinaryMetadata, isProbablyBinaryFile } from "./binary"; import { buildDiffFile, type BuildDiffFileOptions, type DiffFileSourceContext } from "./diffFile"; -import { createFileSourceFetcher, type FileSourceSpec } from "./fileSource"; +import { + createFileSourceFetcher, + fileSourcePathsForSpecs, + type FileSourceSpec, + type FileSourceSpecs, +} from "./fileSource"; import { changesetFromPatch } from "./fromPatch"; import { DEFAULT_FILE_GAP, DEFAULT_HUNK_GAP } from "../run/reviewGap"; @@ -52,14 +57,9 @@ function basename(path: string) { return path.split(/[\\/]/).filter(Boolean).pop() ?? path; } -interface ResolvedFileSourceSpecs { - old: FileSourceSpec; - new: FileSourceSpec; -} - /** Build a binary-aware source-fetcher factory from per-file source specs. */ function createSourceFetcherBuilder( - resolveSpecs: (file: DiffFileSourceContext) => ResolvedFileSourceSpecs | undefined, + resolveSpecs: (file: DiffFileSourceContext) => FileSourceSpecs | undefined, ): NonNullable { return (file) => { if (file.isBinary) { @@ -71,6 +71,21 @@ function createSourceFetcherBuilder( }; } +/** Build exact filesystem provenance from per-file source specs. */ +function createSourcePathBuilder( + resolveSpecs: (file: DiffFileSourceContext) => FileSourceSpecs | undefined, +): NonNullable { + return (file) => { + const specs = resolveSpecs(file); + return specs ? fileSourcePathsForSpecs(specs) : undefined; + }; +} + +/** Represent `/dev/null` as an absent side and every other resolved path as filesystem-backed. */ +function directFileSourceSpec(absolutePath: string): FileSourceSpec { + return absolutePath === "/dev/null" ? { kind: "none" } : { kind: "fs", absolutePath }; +} + /** Reorder files to follow agent-context narrative order when a sidecar provides one. */ export function orderDiffFiles(files: DiffFile[], sidecar: SidecarContext | null) { if (!sidecar || sidecar.files.length === 0) { @@ -132,6 +147,7 @@ function buildBinaryFileDiffChangeset( leftPath: string, rightPath: string, sidecar: SidecarContext | null, + sourceSpecs: FileSourceSpecs, ) { return { id: `pair:${displayPath}`, @@ -148,6 +164,7 @@ function buildBinaryFileDiffChangeset( { previousPath: basename(input.left), isBinary: true, + sourcePathBuilder: createSourcePathBuilder(() => sourceSpecs), }, ), ], @@ -164,6 +181,10 @@ async function loadFileDiffChangeset( const rightPath = resolvePath(cwd, input.right); const displayPath = input.kind === "difftool" ? (input.path ?? basename(input.right)) : basename(input.right); + const sourceSpecs = { + old: directFileSourceSpec(leftPath), + new: directFileSourceSpec(rightPath), + } satisfies FileSourceSpecs; const title = input.kind === "difftool" ? `git difftool: ${displayPath}` @@ -172,7 +193,15 @@ async function loadFileDiffChangeset( : `${basename(input.left)} ↔ ${basename(input.right)}`; if (isProbablyBinaryFile(leftPath) || isProbablyBinaryFile(rightPath)) { - return buildBinaryFileDiffChangeset(input, displayPath, title, leftPath, rightPath, sidecar); + return buildBinaryFileDiffChangeset( + input, + displayPath, + title, + leftPath, + rightPath, + sidecar, + sourceSpecs, + ); } const leftText = await Bun.file(leftPath).text(); @@ -201,10 +230,8 @@ async function loadFileDiffChangeset( files: [ buildDiffFile(metadata, patch, 0, displayPath, sidecar, { previousPath: basename(input.left), - sourceFetcherBuilder: createSourceFetcherBuilder(() => ({ - old: { kind: "fs", absolutePath: leftPath }, - new: { kind: "fs", absolutePath: rightPath }, - })), + sourceFetcherBuilder: createSourceFetcherBuilder(() => sourceSpecs), + sourcePathBuilder: createSourcePathBuilder(() => sourceSpecs), }), ], } satisfies Changeset; @@ -225,7 +252,12 @@ async function loadVcsChangeset( result.title, result.sourceLabel, sidecar, - result.sourceFetcherBuilder ? { sourceFetcherBuilder: result.sourceFetcherBuilder } : undefined, + result.sourceFetcherBuilder || result.sourcePathBuilder + ? { + sourceFetcherBuilder: result.sourceFetcherBuilder, + sourcePathBuilder: result.sourcePathBuilder, + } + : undefined, ); // Two published ways to review a file the patch does not contain, and both // land here: `untrackedPaths`, where an adapter names what its VCS considers diff --git a/src/core/changeset/model.ts b/src/core/changeset/model.ts index 2fa4c8750..4ade602ac 100644 --- a/src/core/changeset/model.ts +++ b/src/core/changeset/model.ts @@ -9,7 +9,7 @@ */ import type { FileDiffMetadata } from "@pierre/diffs"; import type { AgentFileContext } from "../../extension-api/types"; -import type { FileSourceFetcher } from "./fileSource"; +import type { FileSourceFetcher, FileSourcePaths } from "./fileSource"; /** One loaded review sidecar: the changeset summary plus every annotated file it names. */ export interface SidecarContext { @@ -38,6 +38,8 @@ export interface DiffFile { // Optional capability for fetching the file's full text on either side. // Loaders attach this when source content is reachable; absent when not. sourceFetcher?: FileSourceFetcher; + // Exact on-disk provenance for filesystem-backed reviewed sides. + sourcePaths?: FileSourcePaths; } export type DiffLineMoveKind = "moved"; diff --git a/src/core/vcs/types.ts b/src/core/vcs/types.ts index bb056911c..15cbfe108 100644 --- a/src/core/vcs/types.ts +++ b/src/core/vcs/types.ts @@ -56,6 +56,8 @@ export interface VcsPatchResult { untrackedPaths?: string[]; /** Exact old/new content lookups, built from the result's `readFileSource`. */ sourceFetcherBuilder?: BuildDiffFileOptions["sourceFetcherBuilder"]; + /** Exact filesystem paths, built from the result's `resolveFileSourcePath`. */ + sourcePathBuilder?: BuildDiffFileOptions["sourcePathBuilder"]; /** Diff files built from the result's declarative `extraFiles` entries. */ extraFiles?: DiffFile[]; } diff --git a/src/core/vcs/untracked.test.ts b/src/core/vcs/untracked.test.ts index a8032ec42..d7bbbf5ac 100644 --- a/src/core/vcs/untracked.test.ts +++ b/src/core/vcs/untracked.test.ts @@ -28,6 +28,7 @@ describe("buildFilesystemUntrackedDiffFile", () => { expect(file.metadata.type).toBe("new"); expect(file.metadata.hunks).toHaveLength(0); + expect(file.sourcePaths).toEqual({ old: null, new: join(repoRoot, "empty.txt") }); expect(file.patch).toBe( [ "diff --git a/empty.txt b/empty.txt", diff --git a/src/core/vcs/untracked.ts b/src/core/vcs/untracked.ts index ea5187aa7..f50e92b67 100644 --- a/src/core/vcs/untracked.ts +++ b/src/core/vcs/untracked.ts @@ -22,12 +22,14 @@ export function buildSkippedLargeUntrackedDiffFile( index: number, sourcePrefix: string, largeFileCheck: LargeFileCheck, + absolutePath: string, ) { return buildDiffFile(createSkippedLargeMetadata(filePath, "new"), "", index, sourcePrefix, null, { isTooLarge: true, isUntracked: true, stats: largeFileCheck.stats, statsTruncated: largeFileCheck.statsTruncated, + sourcePathBuilder: () => ({ old: null, new: absolutePath }), }); } @@ -86,12 +88,19 @@ export function buildFilesystemUntrackedDiffFile( // patch already carries the only content a symlink has. return buildDiffFile(parseSingleFilePatch(patch, filePath), patch, index, sourcePrefix, null, { isUntracked: true, + sourcePathBuilder: () => ({ old: null, new: absolutePath }), }); } const largeFileCheck = inspectLargeUntrackedFile(repoRoot, filePath); if (largeFileCheck.shouldSkip) { - return buildSkippedLargeUntrackedDiffFile(filePath, index, sourcePrefix, largeFileCheck); + return buildSkippedLargeUntrackedDiffFile( + filePath, + index, + sourcePrefix, + largeFileCheck, + absolutePath, + ); } if (isProbablyBinaryFile(absolutePath)) { @@ -101,7 +110,11 @@ export function buildFilesystemUntrackedDiffFile( index, sourcePrefix, null, - { isBinary: true, isUntracked: true }, + { + isBinary: true, + isUntracked: true, + sourcePathBuilder: () => ({ old: null, new: absolutePath }), + }, ); } @@ -128,5 +141,6 @@ export function buildFilesystemUntrackedDiffFile( old: { kind: "none" }, new: { kind: "fs", absolutePath }, }), + sourcePathBuilder: () => ({ old: null, new: absolutePath }), }); } diff --git a/src/extension-api/index.ts b/src/extension-api/index.ts index dbc86e2fe..f3f6d11a0 100644 --- a/src/extension-api/index.ts +++ b/src/extension-api/index.ts @@ -137,6 +137,7 @@ export type { ExtensionVcsFileChangeType, ExtensionVcsFileSide, ExtensionVcsFileSourceReader, + ExtensionVcsFileSourcePathResolver, ExtensionVcsFileSourceRequest, ExtensionVcsFileSourceResult, ExtensionVcsFileSourceTooLarge, @@ -155,6 +156,8 @@ export type { ExtensionVcsWatchTarget, ExtensionVcsWatchTargetSource, ExtensionWorkspace, + ExtensionWorkspaceLocation, + ExtensionWorkspaceLocationRequest, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, HunkExtensionAPI, diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index 345d29820..3268f97d6 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -21,7 +21,7 @@ * Extensions can branch on `hunk.apiVersion` so a newer Hunk can keep loading * older extensions without guessing at their expectations. */ -export const HUNK_EXTENSION_API_VERSION = 15; +export const HUNK_EXTENSION_API_VERSION = 16; export type HunkExtensionApiVersion = typeof HUNK_EXTENSION_API_VERSION; export type ExtensionNotifyType = "info" | "warning" | "error"; @@ -797,6 +797,11 @@ export type ExtensionVcsFileSourceReader = ( request: ExtensionVcsFileSourceRequest, ) => Promise; +/** Resolve one reviewed side to its exact absolute path when it is filesystem-backed. */ +export type ExtensionVcsFileSourcePathResolver = ( + request: ExtensionVcsFileSourceRequest, +) => string | null; + /* -------------------------------------------------------------------------- */ /* Extra reviewed files */ /* -------------------------------------------------------------------------- */ @@ -875,6 +880,11 @@ export interface ExtensionVcsPatchResult { * carries, which renders the same diff with less context available. */ readFileSource?: ExtensionVcsFileSourceReader; + /** + * Return the exact absolute path for a filesystem-backed side, or `null` for + * absent, index, historical, patch, and other virtual sources. + */ + resolveFileSourcePath?: ExtensionVcsFileSourcePathResolver; /** * Opaque stable identity for source state not already represented by each file's patch. * @@ -1675,6 +1685,24 @@ export type ExtensionWorkspaceWriteResult = | { ok: true } | { ok: false; reason: "unavailable" | "cancelled" | "failed"; detail: string }; +/** A reviewed source address an extension wants to pass to an application. */ +export interface ExtensionWorkspaceLocationRequest { + /** The reviewed file, by its `ExtensionDiffFile.id`. */ + fileId: string; + /** Hunk used to map an old-side line onto the corresponding on-disk file. */ + hunkIndex?: number; + /** Exact source line to prefer over the hunk's first line. */ + line?: { side: ExtensionFileSide; line: number }; +} + +/** The on-disk path and line represented by a reviewed source address. */ +export interface ExtensionWorkspaceLocation { + /** Absolute on-disk path corresponding to the reviewed source. */ + path: string; + /** One-based line in the file on disk. */ + line: number; +} + /** * The reviewed files as whole documents, read and written through the host. * @@ -1729,6 +1757,13 @@ export interface ExtensionWorkspace { * the pairing this exists for. */ readDocument(fileId: string, side: ExtensionFileSide): Promise; + /** + * Resolve review metadata into the corresponding path and line on disk. + * + * Returns `null` when the input has no attested path, the file or hunk is + * unavailable, or the review generation expires. Malformed source addresses reject. + */ + resolveLocation(request: ExtensionWorkspaceLocationRequest): ExtensionWorkspaceLocation | null; /** * Whether `writeDocument` could currently succeed for this reviewed file. * @@ -1800,6 +1835,17 @@ export interface ExtensionSessionOptions { export interface ExtensionCommandContext extends ExtensionContext { /** Live access to the public built-in command table. */ readonly commands: ExtensionCommandControls; + /** + * Temporarily hand Hunk's terminal to an application run by this extension. + * + * Hunk suspends its renderer before calling `run` and restores the review in + * `finally` after `run` settles. The extension owns execution, metadata, + * arguments, environment, and exit handling. Calls reject after this review + * generation expires or while another application already owns the terminal. + * Host-presented dialogs cancel and workspace writes are unavailable while + * `run` owns the terminal; document reads and location resolution remain available. + */ + openInApp(run: () => Result | PromiseLike): Promise; /** Session keyboard modes registered by this command's owning extension. */ readonly keyboardModes: ExtensionKeyboardModeControls; /** Session panes registered by this command's owning extension. */ diff --git a/src/extensions/default/ui/editor/editorApp.test.ts b/src/extensions/default/ui/editor/editorApp.test.ts new file mode 100644 index 000000000..ac5e625e8 --- /dev/null +++ b/src/extensions/default/ui/editor/editorApp.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, test } from "bun:test"; +import { buildEditorCommand, editorUsesTerminal } from "./editorApp"; + +describe("bundled editor app", () => { + test("builds editor-specific line arguments without a shell", () => { + expect( + buildEditorCommand({ + editor: '"C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd" --wait', + filePath: "C:\\repo\\file with spaces.ts", + line: 7, + }), + ).toEqual({ + command: "C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd", + args: ["--wait", "--goto", "C:\\repo\\file with spaces.ts:7"], + }); + expect( + buildEditorCommand({ editor: "nvim --clean", filePath: "/repo/a.ts", line: 12 }), + ).toEqual({ command: "nvim", args: ["--clean", "+12", "/repo/a.ts"] }); + expect( + buildEditorCommand({ editor: "code --reuse-window", filePath: "/repo/a.ts", line: 9 }), + ).toEqual({ + command: "code", + args: ["--reuse-window", "--wait", "--goto", "/repo/a.ts:9"], + }); + expect(buildEditorCommand({ editor: "cursor -w", filePath: "/repo/a.ts", line: 9 })).toEqual({ + command: "cursor", + args: ["-w", "--goto", "/repo/a.ts:9"], + }); + }); + + test("hands terminal editors to Hunk's app lifecycle but leaves GUI editors visible", () => { + expect(editorUsesTerminal("nvim --clean")).toBe(true); + expect(editorUsesTerminal('"C:\\Program Files\\Cursor\\cursor.exe" --wait')).toBe(false); + expect(editorUsesTerminal("code-insiders --wait")).toBe(false); + }); +}); diff --git a/src/extensions/default/ui/editor/editorApp.ts b/src/extensions/default/ui/editor/editorApp.ts new file mode 100644 index 000000000..53ff7ba68 --- /dev/null +++ b/src/extensions/default/ui/editor/editorApp.ts @@ -0,0 +1,87 @@ +import { existsSync } from "node:fs"; +import { basename, win32 } from "node:path"; + +export interface EditorCommand { + command: string; + args: string[]; +} + +/** Split the user's editor command without involving a shell. */ +function splitEditorCommand(editor: string) { + return ( + editor + .match(/(?:[^\s"']+|"(?:\\.|[^"])*"|'(?:\\.|[^'])*')+/g) + ?.map((token) => token.replace(/^(["'])(.*)\1$/, "$2")) ?? [] + ); +} + +/** Return the executable basename used to select an editor's line-address syntax. */ +function editorProgram(editor: string) { + const [firstToken = ""] = splitEditorCommand(editor); + return basename(win32.basename(firstToken)) + .replace(/\.(?:cmd|exe)$/i, "") + .toLowerCase(); +} + +const VI_STYLE_EDITORS = ["vim", "nvim", "vi"]; +const CODE_STYLE_EDITORS = ["code", "code-insiders", "cursor"]; + +/** Report whether this editor expects to own the current terminal. */ +export function editorUsesTerminal(editor: string) { + return !CODE_STYLE_EDITORS.includes(editorProgram(editor)); +} + +/** Build an editor process invocation without shell quoting. */ +export function buildEditorCommand({ + editor, + filePath, + line, +}: { + editor: string; + filePath: string; + line: number; +}): EditorCommand { + const [command = "", ...editorArgs] = splitEditorCommand(editor); + const program = editorProgram(editor); + + if (VI_STYLE_EDITORS.includes(program)) { + return { command, args: [...editorArgs, `+${line}`, filePath] }; + } + if (CODE_STYLE_EDITORS.includes(program)) { + const waitArgs = editorArgs.includes("--wait") || editorArgs.includes("-w") ? [] : ["--wait"]; + return { + command, + args: [...editorArgs, ...waitArgs, "--goto", `${filePath}:${line}`], + }; + } + if (program === "hx") { + return { command, args: [...editorArgs, `${filePath}:${line}`] }; + } + return { command, args: [...editorArgs, filePath] }; +} + +/** Validate one resolved location and turn it into an editor invocation. */ +export function editorCommandForLocation({ + editor, + line, + path, + reviewPath, +}: { + editor: string; + line: number; + path: string; + reviewPath: string; +}): { ok: true; command: EditorCommand } | { ok: false; detail: string } { + if (!existsSync(path)) { + return { ok: false, detail: `Cannot edit ${reviewPath}: file does not exist on disk.` }; + } + + return { + ok: true, + command: buildEditorCommand({ + editor, + filePath: path, + line, + }), + }; +} diff --git a/src/extensions/default/ui/editor/index.test.ts b/src/extensions/default/ui/editor/index.test.ts new file mode 100644 index 000000000..831ff19c3 --- /dev/null +++ b/src/extensions/default/ui/editor/index.test.ts @@ -0,0 +1,162 @@ +import { afterEach, describe, expect, mock, test } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { ExtensionCommandContext } from "hunkdiff/extension"; +import { getBundledUIRegistry } from ".."; +import { BUNDLED_EDITOR_COMMAND_FULL_ID } from "."; + +const originalEditor = process.env.EDITOR; +const originalSpawn = Bun.spawn; +const tempDirs: string[] = []; + +afterEach(() => { + if (originalEditor === undefined) delete process.env.EDITOR; + else process.env.EDITOR = originalEditor; + (Bun as unknown as { spawn: typeof Bun.spawn }).spawn = originalSpawn; + for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +/** Replace Bun's asynchronous process launcher through one narrowly typed test seam. */ +function mockSpawn(implementation: (command: string[]) => { exited: Promise }) { + (Bun as unknown as { spawn: typeof Bun.spawn }).spawn = + implementation as unknown as typeof Bun.spawn; +} + +/** Create a promise whose completion one editor test controls. */ +function createDeferredExit() { + let resolve!: (exitCode: number) => void; + const exited = new Promise((settle) => { + resolve = settle; + }); + return { exited, resolve }; +} + +/** Return the editor registration from the process-static bundled UI registry. */ +function getBundledEditorCommand() { + const registered = getBundledUIRegistry().commands.find( + ({ extensionId, command }) => `${extensionId}.${command.id}` === BUNDLED_EDITOR_COMMAND_FULL_ID, + ); + if (!registered) throw new Error("Bundled editor command is missing."); + return registered; +} + +/** Build a frozen public selection for one file that exists in a temporary workspace. */ +function createEditorContext() { + const cwd = mkdtempSync(join(tmpdir(), "hunk-bundled-editor-")); + tempDirs.push(cwd); + writeFileSync(join(cwd, "alpha.ts"), "one\ntwo\nthree\n"); + const execute = mock(() => true); + const notify = mock(() => {}); + const openInApp = mock(async (run: () => Result | PromiseLike) => await run()); + const context = { + commands: { execute }, + cwd, + notify, + openInApp, + selection: { + file: { + id: "alpha", + path: "alpha.ts", + changeType: "change", + hunks: [{ index: 0, header: "@@", oldRange: [1, 3], newRange: [1, 3] }], + }, + hunkIndex: 0, + currentLine: { side: "new", line: 2 }, + }, + workspace: { + resolveLocation: () => ({ path: join(cwd, "alpha.ts"), line: 2 }), + }, + } as unknown as ExtensionCommandContext; + return { context, cwd, execute, notify, openInApp }; +} + +describe("bundled editor extension", () => { + test("registers the shared Hunk command identity without owning its host key shell", () => { + const registered = getBundledEditorCommand(); + + expect(registered.extensionId).toBe("hunk"); + expect(registered.command).toEqual({ + id: "review.editSelectedFile", + title: "Open the selected file in your editor", + }); + }); + + test("awaits a terminal editor asynchronously inside a generic app handoff", async () => { + const { context, cwd, execute, notify, openInApp } = createEditorContext(); + process.env.EDITOR = "vim --clean"; + const spawnCalls: string[][] = []; + const exit = createDeferredExit(); + mockSpawn((command) => { + spawnCalls.push(command); + return { exited: exit.exited }; + }); + + const pending = getBundledEditorCommand().handler(context); + + expect(openInApp).toHaveBeenCalledTimes(1); + expect(spawnCalls).toEqual([["vim", "--clean", "+2", join(cwd, "alpha.ts")]]); + expect(execute).not.toHaveBeenCalled(); + + exit.resolve(0); + await pending; + + expect(execute).toHaveBeenCalledWith("hunk.app.refresh"); + expect(notify).not.toHaveBeenCalled(); + }); + + test("reports editor failures after Hunk restores its view", async () => { + const { context, notify } = createEditorContext(); + process.env.EDITOR = "vim"; + mockSpawn(() => ({ exited: Promise.resolve(2) })); + + await getBundledEditorCommand().handler(context); + + expect(notify).toHaveBeenCalledWith("Editor exited with status 2.", "error"); + }); + + test("keeps GUI editors responsive, waits before refreshing, and refuses overlap", async () => { + const { context, cwd, execute, notify, openInApp } = createEditorContext(); + process.env.EDITOR = "code --reuse-window"; + const spawnCalls: string[][] = []; + const exit = createDeferredExit(); + mockSpawn((command) => { + spawnCalls.push(command); + return { exited: exit.exited }; + }); + + const pending = getBundledEditorCommand().handler(context); + + expect(openInApp).not.toHaveBeenCalled(); + expect(spawnCalls).toEqual([ + ["code", "--reuse-window", "--wait", "--goto", `${join(cwd, "alpha.ts")}:2`], + ]); + expect(execute).not.toHaveBeenCalled(); + + // The first asynchronous child remains pending without blocking a second command dispatch. + await getBundledEditorCommand().handler(context); + expect(spawnCalls).toHaveLength(1); + expect(notify).toHaveBeenCalledWith("An editor is already open.", "warning"); + + exit.resolve(0); + await pending; + + expect(execute).toHaveBeenCalledWith("hunk.app.refresh"); + }); + + test("releases bundled editor ownership when asynchronous process launch fails", async () => { + const { context, notify } = createEditorContext(); + process.env.EDITOR = "code"; + let launches = 0; + mockSpawn(() => { + launches += 1; + throw new Error("missing executable"); + }); + + await getBundledEditorCommand().handler(context); + await getBundledEditorCommand().handler(context); + + expect(launches).toBe(2); + expect(notify).toHaveBeenCalledWith("Failed to launch editor: missing executable", "error"); + }); +}); diff --git a/src/extensions/default/ui/editor/index.ts b/src/extensions/default/ui/editor/index.ts new file mode 100644 index 000000000..a2a572b25 --- /dev/null +++ b/src/extensions/default/ui/editor/index.ts @@ -0,0 +1,83 @@ +import type { ExtensionFactory } from "hunkdiff/extension"; +import { editorCommandForLocation, editorUsesTerminal } from "./editorApp"; + +export const BUNDLED_EDITOR_COMMAND_ID = "review.editSelectedFile"; +export const BUNDLED_EDITOR_COMMAND_FULL_ID = `hunk.${BUNDLED_EDITOR_COMMAND_ID}`; + +/** Register Hunk's editor workflow through the public app-handoff contract. */ +const registerBundledEditor: ExtensionFactory = (hunk) => { + let editorOpen = false; + + hunk.registerCommand( + { + id: BUNDLED_EDITOR_COMMAND_ID, + title: "Open the selected file in your editor", + }, + async (ctx) => { + const editor = process.env.EDITOR?.trim(); + if (!editor) { + ctx.notify("$EDITOR is not set.", "warning"); + return; + } + + const file = ctx.selection.file; + if (!file) { + ctx.notify("No file selected.", "warning"); + return; + } + const location = ctx.workspace.resolveLocation({ + fileId: file.id, + ...(ctx.selection.hunkIndex === null ? {} : { hunkIndex: ctx.selection.hunkIndex }), + ...(ctx.selection.currentLine === null ? {} : { line: ctx.selection.currentLine }), + }); + if (!location) { + ctx.notify(`Cannot resolve ${file.path} on disk.`, "warning"); + return; + } + const selected = editorCommandForLocation({ + editor, + ...location, + reviewPath: file.path, + }); + if (!selected.ok) { + ctx.notify(selected.detail, "warning"); + return; + } + + if (editorOpen) { + ctx.notify("An editor is already open.", "warning"); + return; + } + + editorOpen = true; + try { + const runEditor = async () => { + const child = Bun.spawn([selected.command.command, ...selected.command.args], { + stdin: "inherit", + stdout: "inherit", + stderr: "inherit", + }); + return await child.exited; + }; + const exitCode = editorUsesTerminal(editor) + ? await ctx.openInApp(runEditor) + : await runEditor(); + + if (exitCode !== 0) { + ctx.notify(`Editor exited with status ${exitCode}.`, "error"); + return; + } + ctx.commands.execute("hunk.app.refresh"); + } catch (error) { + ctx.notify( + `Failed to launch editor: ${error instanceof Error ? error.message : String(error)}`, + "error", + ); + } finally { + editorOpen = false; + } + }, + ); +}; + +export default registerBundledEditor; diff --git a/src/extensions/default/ui/index.test.ts b/src/extensions/default/ui/index.test.ts index c3472717f..f9cced6b4 100644 --- a/src/extensions/default/ui/index.test.ts +++ b/src/extensions/default/ui/index.test.ts @@ -1,10 +1,17 @@ import { describe, expect, test } from "bun:test"; import { getBundledUIRegistry } from "."; import { paneKey } from "../../apply"; +import { BUNDLED_EDITOR_COMMAND_FULL_ID } from "./editor"; describe("bundled UI registry", () => { - test("registers only the built-in files pane", () => { - const panes = getBundledUIRegistry().panes; + test("registers the built-in files pane and editor command", () => { + const registry = getBundledUIRegistry(); + const panes = registry.panes; expect(panes.map(paneKey)).toEqual(["hunk:files"]); + expect( + registry.commands.map(({ extensionId, command }) => `${extensionId}.${command.id}`), + ).toEqual([BUNDLED_EDITOR_COMMAND_FULL_ID]); + expect(registry.extensions).toHaveLength(1); + expect(registry.extensions[0]?.origin).toBe("bundled"); }); }); diff --git a/src/extensions/default/ui/index.ts b/src/extensions/default/ui/index.ts index b5575104b..8e86df792 100644 --- a/src/extensions/default/ui/index.ts +++ b/src/extensions/default/ui/index.ts @@ -6,30 +6,42 @@ import { type ExtensionLoadIssue, type ExtensionRegistry, } from "../../types"; +import registerBundledEditor, { BUNDLED_EDITOR_COMMAND_FULL_ID } from "./editor"; import registerBundledSidebar from "./sidebar"; -const factories: readonly [string, ExtensionFactory][] = [["files", registerBundledSidebar]]; let cachedRegistry: ExtensionRegistry | undefined; +/** Register Hunk's default terminal surfaces through one bundled extension identity. */ +const registerBundledUI: ExtensionFactory = (hunk) => { + registerBundledSidebar(hunk); + registerBundledEditor(hunk); +}; + /** Load bundled UI registrations through the public factory path, once per process. */ export function getBundledUIRegistry(): ExtensionRegistry { if (cachedRegistry) return cachedRegistry; const registry = createEmptyExtensionRegistry(); const issues: ExtensionLoadIssue[] = []; - for (const [id, factory] of factories) { - runExtensionFactory({ - metadata: { - id: HUNK_VENDOR_EXTENSION_ID, - sourcePath: `hunk:bundled/ui/${id}`, - origin: "bundled", - }, - registry, - issues, - factory, - }); - } - if (issues.length > 0 || registry.panes.length !== factories.length) { - throw new Error(`Bundled UI failed to register: ${issues[0]?.message ?? "missing pane"}`); + runExtensionFactory({ + metadata: { + id: HUNK_VENDOR_EXTENSION_ID, + sourcePath: "hunk:bundled/ui", + origin: "bundled", + }, + registry, + issues, + factory: registerBundledUI, + }); + const filesPaneRegistered = registry.panes.some( + ({ extensionId, pane }) => extensionId === HUNK_VENDOR_EXTENSION_ID && pane.id === "files", + ); + const editorCommandRegistered = registry.commands.some( + ({ extensionId, command }) => `${extensionId}.${command.id}` === BUNDLED_EDITOR_COMMAND_FULL_ID, + ); + if (issues.length > 0 || !filesPaneRegistered || !editorCommandRegistered) { + throw new Error( + `Bundled UI failed to register: ${issues[0]?.message ?? "missing required contribution"}`, + ); } cachedRegistry = registry; return registry; diff --git a/src/extensions/default/vcs/git/index.test.ts b/src/extensions/default/vcs/git/index.test.ts index fc43fe21c..e34101ef7 100644 --- a/src/extensions/default/vcs/git/index.test.ts +++ b/src/extensions/default/vcs/git/index.test.ts @@ -132,6 +132,18 @@ describe("GitVcsAdapter", () => { const trackedFile = { path: "tracked.txt", changeType: "change", isUntracked: false } as const; expect(await readSource?.({ ...trackedFile, side: "old" })).toBe("old\n"); expect(await readSource?.({ ...trackedFile, side: "new" })).toBe("new\n"); + expect(result.resolveFileSourcePath?.({ ...trackedFile, side: "old" })).toBeNull(); + expect( + normalizeComparablePath(result.resolveFileSourcePath!({ ...trackedFile, side: "new" })!), + ).toBe(normalizeComparablePath(join(repo, "tracked.txt"))); + expect( + result.resolveFileSourcePath?.({ ...trackedFile, changeType: "new", side: "old" }), + ).toBeNull(); + expect( + result.resolveFileSourcePath?.({ ...trackedFile, changeType: "deleted", side: "new" }), + ).toBeNull(); + rmSync(join(repo, "tracked.txt")); + expect(result.resolveFileSourcePath?.({ ...trackedFile, side: "new" })).toBeNull(); git(repo, "add", "tracked.txt"); const changedIndexResult = await GitVcsAdapter.operations["working-tree-diff"]!.load(input, { @@ -169,6 +181,8 @@ describe("GitVcsAdapter", () => { expect(result.untrackedPaths).toEqual([]); expect(await result.readFileSource?.({ ...file, side: "old" })).toBe("old\ncontext\n"); expect(await result.readFileSource?.({ ...file, side: "new" })).toBe("new\ncontext\n"); + expect(result.resolveFileSourcePath?.({ ...file, side: "old" })).toBeNull(); + expect(result.resolveFileSourcePath?.({ ...file, side: "new" })).toBeNull(); }); test("loads revision and stash patches through adapter operations", async () => { @@ -196,6 +210,8 @@ describe("GitVcsAdapter", () => { const showFile = { path: "file.txt", changeType: "change", isUntracked: false } as const; expect(await showResult.readFileSource?.({ ...showFile, side: "old" })).toBe("one\n"); expect(await showResult.readFileSource?.({ ...showFile, side: "new" })).toBe("two\n"); + expect(showResult.resolveFileSourcePath?.({ ...showFile, side: "old" })).toBeNull(); + expect(showResult.resolveFileSourcePath?.({ ...showFile, side: "new" })).toBeNull(); writeFileSync(join(repo, "file.txt"), "three\n"); git(repo, "stash", "push", "-m", "adapter stash"); diff --git a/src/extensions/default/vcs/git/index.ts b/src/extensions/default/vcs/git/index.ts index d3a1b27ff..d2ee70395 100644 --- a/src/extensions/default/vcs/git/index.ts +++ b/src/extensions/default/vcs/git/index.ts @@ -19,7 +19,7 @@ import { type GitBackedInput, type GitDiffEndpoints, } from "./commands"; -import { gitEndpointSourceSpec, readGitFileSource } from "./source"; +import { gitEndpointSourceSpec, gitFileSourcePath, readGitFileSource } from "./source"; import { describeDiffRange } from "../diffRange"; import { HUNK_VCS_DETECTION_BASELINE_PRIORITY, @@ -28,6 +28,8 @@ import { type ExtensionVcsDirectoryTreeWatchTarget, type ExtensionVcsExtraFile, type ExtensionVcsFileSourceReader, + type ExtensionVcsFileSourcePathResolver, + type ExtensionVcsFileSourceRequest, type ExtensionVcsWatchPlan, type HunkExtensionAPI, } from "hunkdiff/extension"; @@ -83,9 +85,34 @@ export function statSignature(path: string) { /** Exact source reader plus a stable identity for its complete old/new snapshot. */ interface GitSourceCapability { readFileSource: ExtensionVcsFileSourceReader; + resolveFileSourcePath: ExtensionVcsFileSourcePathResolver; sourceCacheKey: string; } +/** Resolve one file side to the exact Git source spec used for both reads and paths. */ +function gitFileSourceSpecForRequest( + request: ExtensionVcsFileSourceRequest, + repoRoot: string, + endpoints: GitDiffEndpoints, +) { + const spec = + request.side === "old" + ? request.changeType === "new" + ? ({ kind: "none" } as const) + : gitEndpointSourceSpec(endpoints.old, repoRoot, request.previousPath ?? request.path) + : request.changeType === "deleted" + ? ({ kind: "none" } as const) + : gitEndpointSourceSpec(endpoints.new, repoRoot, request.path); + + if (spec.kind !== "fs") return spec; + try { + fs.lstatSync(spec.absolutePath); + return spec; + } catch { + return { kind: "none" } as const; + } +} + /** Hash semantic index entries so filesystem-stat refreshes do not defeat cache reuse. */ function gitIndexCacheKey(input: GitBackedInput, repoRoot: string, gitExecutable: string) { const entries = runGitText({ @@ -130,26 +157,12 @@ function createGitSourceCapability( gitEndpointCacheKey(endpoints.old, indexCacheKey), 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, - }, - ); - } - - return changeType === "deleted" - ? Promise.resolve(null) - : readGitFileSource(gitEndpointSourceSpec(endpoints.new, repoRoot, path), { - gitExecutable, - }); - }, + readFileSource: (request) => + readGitFileSource(gitFileSourceSpecForRequest(request, repoRoot, endpoints), { + gitExecutable, + }), + resolveFileSourcePath: (request) => + gitFileSourcePath(gitFileSourceSpecForRequest(request, repoRoot, endpoints)), }; } diff --git a/src/extensions/default/vcs/git/source.test.ts b/src/extensions/default/vcs/git/source.test.ts index f97eb2029..f07166fa3 100644 --- a/src/extensions/default/vcs/git/source.test.ts +++ b/src/extensions/default/vcs/git/source.test.ts @@ -2,7 +2,7 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { gitEndpointSourceSpec, readGitFileSource } from "./source"; +import { gitEndpointSourceSpec, gitFileSourcePath, readGitFileSource } from "./source"; const tempDirs: string[] = []; @@ -73,6 +73,17 @@ describe("gitEndpointSourceSpec", () => { absolutePath: join("/repo", "a.ts"), }); }); + + test("exposes paths only for filesystem specs", () => { + expect(gitFileSourcePath({ kind: "fs", absolutePath: join("/repo", "a.ts") })).toBe( + join("/repo", "a.ts"), + ); + expect(gitFileSourcePath({ kind: "git-index", repoRoot: "/repo", path: "a.ts" })).toBeNull(); + expect( + gitFileSourcePath({ kind: "git-blob", repoRoot: "/repo", ref: "HEAD", path: "a.ts" }), + ).toBeNull(); + expect(gitFileSourcePath({ kind: "none" })).toBeNull(); + }); }); describe("Git source reading", () => { diff --git a/src/extensions/default/vcs/git/source.ts b/src/extensions/default/vcs/git/source.ts index b5a767291..23e5632a2 100644 --- a/src/extensions/default/vcs/git/source.ts +++ b/src/extensions/default/vcs/git/source.ts @@ -32,6 +32,11 @@ export interface GitFileSourceOptions { maxSourceBytes?: number; } +/** Return the exact path only when one Git source spec names the live filesystem. */ +export function gitFileSourcePath(spec: GitFileSourceSpec) { + return spec.kind === "fs" ? spec.absolutePath : null; +} + /** Convert one Git diff endpoint into the corresponding source lookup. */ export function gitEndpointSourceSpec( endpoint: GitDiffEndpoint, diff --git a/src/extensions/default/vcs/jujutsu/index.test.ts b/src/extensions/default/vcs/jujutsu/index.test.ts index 0cf270698..231dddbe5 100644 --- a/src/extensions/default/vcs/jujutsu/index.test.ts +++ b/src/extensions/default/vcs/jujutsu/index.test.ts @@ -130,6 +130,12 @@ describe("JjVcsAdapter", () => { } as const; expect(await diffResult.readFileSource?.({ ...reviewedFile, side: "old" })).toBe("one\n"); expect(await diffResult.readFileSource?.({ ...reviewedFile, side: "new" })).toBe("two\n"); + expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "old" })).toBeNull(); + expect( + normalizeComparablePath( + diffResult.resolveFileSourcePath!({ ...reviewedFile, side: "new" })!, + ), + ).toBe(normalizeComparablePath(join(repo, "file.txt"))); const equivalentDiffResult = await JjVcsAdapter.operations["working-tree-diff"]!.load( diffInput, { cwd: repo }, @@ -150,6 +156,7 @@ describe("JjVcsAdapter", () => { expect(showResult.sourceCacheKey).toContain("jj-source-v1"); expect(await showResult.readFileSource?.({ ...reviewedFile, side: "old" })).toBe("one\n"); expect(await showResult.readFileSource?.({ ...reviewedFile, side: "new" })).toBe("two\n"); + expect("resolveFileSourcePath" in showResult).toBe(false); // Lazy source reads stay attached to the revision that produced the patch, // even after `@` is resnapshotted with different working-copy contents. @@ -195,6 +202,7 @@ describe("JjVcsAdapter", () => { expect(result.patchText).toContain("+two"); expect(await result.readFileSource?.({ ...file, side: "old" })).toBe("one\ncontext\n"); expect(await result.readFileSource?.({ ...file, side: "new" })).toBe("two\ncontext\n"); + expect(result.resolveFileSourcePath).toBeUndefined(); writeFileSync(join(repo, "file.txt"), "three\ncontext\n"); expect( diff --git a/src/extensions/default/vcs/jujutsu/index.ts b/src/extensions/default/vcs/jujutsu/index.ts index 48018f053..10036b4fe 100644 --- a/src/extensions/default/vcs/jujutsu/index.ts +++ b/src/extensions/default/vcs/jujutsu/index.ts @@ -12,6 +12,7 @@ import { } from "./commands"; import { readJjFileSource } from "./source"; import { describeDiffRange } from "../diffRange"; +import { createWorkingTreeSourcePathResolver } from "../workingTreeSource"; import { HUNK_VCS_DETECTION_BASELINE_PRIORITY, type ExtensionVcsAdapter, @@ -165,6 +166,9 @@ export function createJjVcsAdapter({ jjExecutable = "jj" }: Readonly { expect(diffResult.title).toContain("working copy"); expect(diffResult.patchText).toContain("diff --git a/file.txt b/file.txt"); expect(diffResult.patchText).toContain("+two"); + const reviewedFile = { + path: "file.txt", + changeType: "change", + isUntracked: false, + } as const; + expect(diffResult.resolveFileSourcePath?.({ ...reviewedFile, side: "old" })).toBeNull(); + expect( + normalizeComparablePath( + diffResult.resolveFileSourcePath!({ ...reviewedFile, side: "new" })!, + ), + ).toBe(normalizeComparablePath(join(repo, "file.txt"))); const showInput = { kind: "show", @@ -130,6 +141,7 @@ describe("SaplingVcsAdapter", () => { expect(showResult.title).toContain("show ."); expect(showResult.patchText).toContain("diff --git a/file.txt b/file.txt"); + expect("resolveFileSourcePath" in showResult).toBe(false); expect( SaplingVcsAdapter.operations["working-tree-diff"]!.watchSignature!(diffInput, { cwd: repo, @@ -186,6 +198,7 @@ describe("SaplingVcsAdapter without the sl binary", () => { }); expect(result.untrackedPaths).toEqual([]); + expect(result.resolveFileSourcePath).toBeUndefined(); expect(commands.some((command) => command.includes("status"))).toBe(false); expect( commands.some((command) => command.includes("main") && command.includes("feature")), diff --git a/src/extensions/default/vcs/sapling/index.ts b/src/extensions/default/vcs/sapling/index.ts index 37eacb1df..28f31defe 100644 --- a/src/extensions/default/vcs/sapling/index.ts +++ b/src/extensions/default/vcs/sapling/index.ts @@ -9,6 +9,7 @@ import { runSlText, } from "./commands"; import { describeDiffRange } from "../diffRange"; +import { createWorkingTreeSourcePathResolver } from "../workingTreeSource"; import { HUNK_VCS_DETECTION_BASELINE_PRIORITY, type ExtensionVcsAdapter, @@ -90,6 +91,9 @@ export const SaplingVcsAdapter = { title: range ? `${repoName} ${range}` : `${repoName} working copy`, patchText: runSlText({ input, args: diffArgs, cwd }), untrackedPaths: listSlUntrackedFiles(input, { cwd, repoRoot }), + ...(!input.range && !input.rangeEndpoints + ? { resolveFileSourcePath: createWorkingTreeSourcePathResolver(repoRoot) } + : {}), }; }, watchSignature(input, { cwd }) { diff --git a/src/extensions/default/vcs/workingTreeSource.test.ts b/src/extensions/default/vcs/workingTreeSource.test.ts new file mode 100644 index 000000000..d1a3bf3d2 --- /dev/null +++ b/src/extensions/default/vcs/workingTreeSource.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, test } from "bun:test"; +import { join } from "node:path"; +import { createWorkingTreeSourcePathResolver } from "./workingTreeSource"; + +describe("createWorkingTreeSourcePathResolver", () => { + const resolvePath = createWorkingTreeSourcePathResolver("/repo"); + const file = { path: "src/a.ts", changeType: "change", isUntracked: false } as const; + + test("resolves only present new sides", () => { + expect(resolvePath({ ...file, side: "new" })).toBe(join("/repo", "src/a.ts")); + expect(resolvePath({ ...file, side: "old" })).toBeNull(); + expect(resolvePath({ ...file, changeType: "deleted", side: "new" })).toBeNull(); + }); +}); diff --git a/src/extensions/default/vcs/workingTreeSource.ts b/src/extensions/default/vcs/workingTreeSource.ts new file mode 100644 index 000000000..223713731 --- /dev/null +++ b/src/extensions/default/vcs/workingTreeSource.ts @@ -0,0 +1,15 @@ +import { join } from "node:path"; +import type { + ExtensionVcsFileSourcePathResolver, + ExtensionVcsFileSourceRequest, +} from "hunkdiff/extension"; + +/** Resolve only a present new side to its path in a provider's live working tree. */ +export function createWorkingTreeSourcePathResolver( + repoRoot: string, +): ExtensionVcsFileSourcePathResolver { + return (request: ExtensionVcsFileSourceRequest) => + request.side === "new" && request.changeType !== "deleted" + ? join(repoRoot, request.path) + : null; +} diff --git a/src/extensions/types.ts b/src/extensions/types.ts index 1a399b57f..9f9523288 100644 --- a/src/extensions/types.ts +++ b/src/extensions/types.ts @@ -74,6 +74,8 @@ export type { ExtensionThemeConfig, ExtensionVcsAdapter, ExtensionWorkspace, + ExtensionWorkspaceLocation, + ExtensionWorkspaceLocationRequest, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, HunkExtensionAPI, diff --git a/src/extensions/vcsPatchResult.test.ts b/src/extensions/vcsPatchResult.test.ts index fa22cc56a..b901a39da 100644 --- a/src/extensions/vcsPatchResult.test.ts +++ b/src/extensions/vcsPatchResult.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { join, resolve } from "node:path"; import { toInternalVcsPatchResult } from "./vcsPatchResult"; import { HunkExtensionUserError } from "../extension-api/types"; import { HunkUserError, toUserFacingError } from "../core/run/errors"; @@ -148,6 +149,63 @@ describe("published source readers", () => { }); }); +describe("published source path resolvers", () => { + test("become per-file absolute source paths without requiring a text reader", () => { + const requests: ExtensionVcsFileSourceRequest[] = []; + const root = resolve("repo"); + const result = toInternalVcsPatchResult( + baseResult({ + resolveFileSourcePath: (request) => { + requests.push(request); + return request.side === "new" ? join(root, request.path) : null; + }, + }), + ); + const sourcePaths = result.sourcePathBuilder?.({ + path: "src/a.ts", + previousPath: "src/old.ts", + type: "rename-changed", + isUntracked: false, + isBinary: true, + }); + + expect(result.sourceFetcherBuilder).toBeUndefined(); + expect(sourcePaths).toEqual({ old: null, new: join(root, "src/a.ts") }); + expect(requests).toEqual([ + { + path: "src/a.ts", + previousPath: "src/old.ts", + changeType: "rename-changed", + isUntracked: false, + side: "old", + }, + { + path: "src/a.ts", + previousPath: "src/old.ts", + changeType: "rename-changed", + isUntracked: false, + side: "new", + }, + ]); + }); + + test("drops relative and wholly unavailable resolver answers", () => { + const relative = toInternalVcsPatchResult( + baseResult({ resolveFileSourcePath: () => "relative/a.ts" }), + ); + const unavailable = toInternalVcsPatchResult(baseResult({ resolveFileSourcePath: () => null })); + const file = { + path: "a.ts", + type: "change", + isUntracked: false, + isBinary: false, + } as const; + + expect(relative.sourcePathBuilder?.(file)).toBeUndefined(); + expect(unavailable.sourcePathBuilder?.(file)).toBeUndefined(); + }); +}); + describe("published extra files", () => { test("build a diff file from a one-file patch, labeled with the declared path", () => { const result = toInternalVcsPatchResult( @@ -170,6 +228,7 @@ describe("published extra files", () => { }); test("build a placeholder for a skipped file with no content to read", () => { + const sourcePath = resolve("repo", "generated.txt"); const result = toInternalVcsPatchResult( baseResult({ extraFiles: [ @@ -184,6 +243,7 @@ describe("published extra files", () => { }, ], readFileSource: async () => "unreachable", + resolveFileSourcePath: ({ side }) => (side === "new" ? sourcePath : null), }), ); @@ -195,6 +255,7 @@ describe("published extra files", () => { expect(file?.statsTruncated).toBe(true); expect(file?.metadata.hunks).toHaveLength(0); expect(file?.sourceFetcher).toBeUndefined(); + expect(file?.sourcePaths).toEqual({ old: null, new: sourcePath }); }); test("defaults a skipped file to a modification with no counted lines", () => { diff --git a/src/extensions/vcsPatchResult.ts b/src/extensions/vcsPatchResult.ts index 540697358..03a0cbe35 100644 --- a/src/extensions/vcsPatchResult.ts +++ b/src/extensions/vcsPatchResult.ts @@ -3,6 +3,7 @@ import { createSkippedLargeMetadata, type BuildDiffFileOptions, } from "../core/changeset/diffFile"; +import { isAbsolute } from "node:path"; import { parseSingleFilePatch } from "../core/patch/singleFile"; import { DEFAULT_SOURCE_TEXT_MAX_BYTES, @@ -14,6 +15,8 @@ import type { VcsPatchResult } from "../core/vcs/types"; import type { ExtensionVcsExtraFile, ExtensionVcsFileSourceReader, + ExtensionVcsFileSourcePathResolver, + ExtensionVcsFileSourceRequest, ExtensionVcsPatchResult, } from "../extension-api/types"; @@ -27,6 +30,21 @@ import type { */ type SourceFetcherBuilder = NonNullable; +type SourcePathBuilder = NonNullable; + +/** Build one public source request from normalized per-file context. */ +function sourceRequest( + file: Parameters[0], + side: FileSourceSide, +): ExtensionVcsFileSourceRequest { + return { + path: file.path, + previousPath: file.previousPath, + changeType: file.type, + isUntracked: file.isUntracked, + side, + }; +} /** * Adapt a published per-file source reader to the internal per-file fetcher. @@ -61,13 +79,7 @@ function toSourceFetcherBuilder( throw new SourceTextTooLargeError(cachedLimit); } - const result = await read({ - path: file.path, - previousPath: file.previousPath, - changeType: file.type, - isUntracked: file.isUntracked, - side, - }); + const result = await read(sourceRequest(file, side)); if (typeof result === "object" && result !== null) { if (result.kind === "too-large") { const maxBytes = @@ -89,6 +101,19 @@ function toSourceFetcherBuilder( }; } +/** Adapt a published path resolver to normalized per-file filesystem provenance. */ +function toSourcePathBuilder(resolvePath: ExtensionVcsFileSourcePathResolver): SourcePathBuilder { + return (file) => { + const resolveSide = (side: FileSourceSide) => { + const path = resolvePath(sourceRequest(file, side)); + return path !== null && isAbsolute(path) ? path : null; + }; + const old = resolveSide("old"); + const next = resolveSide("new"); + return old === null && next === null ? undefined : { old, new: next }; + }; +} + /** * Build the diff model for one file an adapter reported outside its patch text. * @@ -101,6 +126,7 @@ function toInternalExtraFile( index: number, sourcePrefix: string, sourceFetcherBuilder: SourceFetcherBuilder | undefined, + sourcePathBuilder: SourcePathBuilder | undefined, ): DiffFile { if (entry.kind === "skipped") { return buildDiffFile( @@ -115,6 +141,7 @@ function toInternalExtraFile( isTooLarge: true, stats: entry.stats, statsTruncated: entry.statsTruncated, + sourcePathBuilder, }, ); } @@ -129,6 +156,7 @@ function toInternalExtraFile( previousPath: entry.previousPath, isUntracked: entry.isUntracked, sourceFetcherBuilder, + sourcePathBuilder, }, ); } @@ -138,6 +166,9 @@ export function toInternalVcsPatchResult(result: ExtensionVcsPatchResult): VcsPa const sourceFetcherBuilder = result.readFileSource ? toSourceFetcherBuilder(result.readFileSource, result.sourceCacheKey) : undefined; + const sourcePathBuilder = result.resolveFileSourcePath + ? toSourcePathBuilder(result.resolveFileSourcePath) + : undefined; return { repoRoot: result.repoRoot, @@ -146,8 +177,9 @@ export function toInternalVcsPatchResult(result: ExtensionVcsPatchResult): VcsPa patchText: result.patchText, untrackedPaths: result.untrackedPaths, sourceFetcherBuilder, + sourcePathBuilder, extraFiles: result.extraFiles?.map((entry, index) => - toInternalExtraFile(entry, index, result.repoRoot, sourceFetcherBuilder), + toInternalExtraFile(entry, index, result.repoRoot, sourceFetcherBuilder, sourcePathBuilder), ), }; } diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 9c5cdd1d1..b1076cfdb 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -18,7 +18,6 @@ import type { PersistedViewPreferences } from "../core/run/config"; import { experimentalFeatureEnabled, resolveExperimentalDiffFiles } from "../core/run/experimental"; import { DEFAULT_FILE_GAP, DEFAULT_HUNK_GAP } from "../core/run/reviewGap"; import { DEFAULT_TAB_WIDTH } from "../core/run/tabWidth"; -import { isVcsReviewInput } from "../core/vcs"; import type { AppBootstrap } from "../core/bootstrap"; import { selectActiveEditableReviewNoteId, @@ -35,6 +34,8 @@ import { } from "../extensions/apply"; import { projectExtensionReviewNotes } from "../extensions/reviewSnapshot"; import type { ExtensionNotifyType, ExtensionLoadResult } from "../extensions/types"; +import { getBundledUIRegistry } from "../extensions/default/ui"; +import { BUNDLED_EDITOR_COMMAND_FULL_ID } from "../extensions/default/ui/editor"; import type { ReviewProducer } from "../app/review/producer"; import type { HunkSessionBrokerClient } from "../session/broker/brokerClient"; import type { ReloadedSessionResult, ReloadSessionOptions } from "../session/types"; @@ -54,6 +55,7 @@ import { import { useAppKeyboardShortcuts } from "./hooks/useAppKeyboardShortcuts"; import { useCurrentReviewRefreshController } from "./hooks/useCurrentReviewRefreshController"; import { useExtensionCommandRunner } from "./hooks/useExtensionCommandRunner"; +import { useExtensionAppController } from "./hooks/useExtensionAppController"; import { useExtensionDialogController } from "./hooks/useExtensionDialogController"; import { useExtensionEventContextProvider } from "./hooks/useExtensionEventContextProvider"; import { useExtensionNotifications } from "./hooks/useExtensionNotifications"; @@ -103,7 +105,6 @@ import { import { HUNK_FILES_PANE_KEY } from "../extensions/extensionIds"; import { maxFileHeaderStatsWidth } from "./lib/fileHeader"; import { setMouseCapture } from "./lib/mouseCapture"; -import { openSelectedFileInEditor } from "./lib/openInEditor"; import { resolveResponsiveLayout } from "./lib/responsive"; import type { WorkspaceRefreshRequest } from "./currentReviewRefresh"; @@ -482,6 +483,7 @@ export function App({ const { accept: acceptExtensionDialog, cancel: cancelExtensionDialog, + cancelAll: cancelAllExtensionDialogs, createDialogs: createQueuedExtensionDialogs, inputValue: extensionDialogInputValue, moveSelection: moveExtensionDialogSelection, @@ -491,6 +493,12 @@ export function App({ updateInput: setExtensionDialogInputValue, } = useExtensionDialogController({ reviewGeneration: bootstrap }); + const extensionAppController = useExtensionAppController({ + createReviewCapabilityLease, + onOwnershipStarted: cancelAllExtensionDialogs, + renderer, + }); + /** Keep third-party dialog attribution while presenting bundled extensions as native Hunk UI. */ const createExtensionDialogs = useCallback( (extensionId: string) => { @@ -499,11 +507,11 @@ export function App({ (metadata) => metadata.id === extensionId && metadata.origin === "bundled", ); return createQueuedExtensionDialogs(extensionId, { - isLive: lease.isLive, + isLive: () => lease.isLive() && !extensionAppController.isAppActive(), showAttribution: !bundled, }); }, - [createQueuedExtensionDialogs, createReviewCapabilityLease, extensions], + [createQueuedExtensionDialogs, createReviewCapabilityLease, extensionAppController, extensions], ); const extensionWorkspaceController = useExtensionWorkspaceControls({ @@ -511,12 +519,12 @@ export function App({ createReviewCapabilityLease, files: reviewFiles, input: bootstrap.input, + isAppActive: extensionAppController.isAppActive, onWorkspaceWriteCompleted, root: bootstrap.reloadContext.repoRoot ?? bootstrap.reloadContext.cwd, runWorkspaceWrite, workspaceFileWriter, }); - useExtensionEventContextProvider({ createDialogs: createExtensionDialogs, createNavigation: createExtensionNavigation, @@ -531,6 +539,7 @@ export function App({ createKeyboardModeControls, createLineHighlightControls, createNavigation: createExtensionNavigation, + createOpenInApp: extensionAppController.createOpenInApp, createPaneControls, createReviewControls: createExtensionReviewControls, createWorkspaceControls: extensionWorkspaceController.createWorkspaceControls, @@ -538,6 +547,20 @@ export function App({ getSelection: getExtensionSelection, }); + const bundledEditorCommand = useMemo(() => { + const command = resolveExtensionCommands(getBundledUIRegistry()).commands.find( + ({ extensionId, command: registration }) => + `${extensionId}.${registration.id}` === BUNDLED_EDITOR_COMMAND_FULL_ID, + ); + if (!command) throw new Error("Bundled editor command is not registered."); + return command; + }, []); + + /** Delegate the shared host command shell to the bundled editor extension. */ + const triggerEditSelectedFile = useCallback(() => { + runExtensionCommand(bundledEditorCommand); + }, [bundledEditorCommand, runExtensionCommand]); + const registeredExtensionCommands = useMemo( () => (extensions ? resolveExtensionCommands(extensions.registry).commands : []), [extensions], @@ -884,38 +907,6 @@ export function App({ showNotice: showSessionNotice, }); - const triggerEditSelectedFile = useCallback(() => { - const basePath = isVcsReviewInput(bootstrap.input) - ? bootstrap.changeset.sourceLabel - : undefined; - const message = openSelectedFileInEditor({ - basePath, - file: selectedFile, - lineCursor: activeLineCursor, - renderer, - selectedHunk: review.selectedHunk, - }); - - if (message) { - showSessionNotice(message); - return; - } - - if (canRefreshCurrentInput) { - triggerRefreshCurrentInput(); - } - }, [ - activeLineCursor, - bootstrap.changeset.sourceLabel, - bootstrap.input.kind, - canRefreshCurrentInput, - renderer, - review.selectedHunk, - selectedFile, - showSessionNotice, - triggerRefreshCurrentInput, - ]); - /** Close the agent skill setup overlay. */ const closeAgentSkill = useCallback(() => { setShowAgentSkill(false); diff --git a/src/ui/AppHost.edit-in-editor.test.tsx b/src/ui/AppHost.edit-in-editor.test.tsx index d4f5a3d91..d983c4740 100644 --- a/src/ui/AppHost.edit-in-editor.test.tsx +++ b/src/ui/AppHost.edit-in-editor.test.tsx @@ -5,6 +5,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { act } from "react"; import type { AppBootstrap } from "../core/bootstrap"; +import { createEmptyExtensionLoadResult } from "../extensions/types"; import { createTestVcsAppBootstrap } from "../../test/helpers/app-bootstrap"; import { createTestDiffFile, lines } from "../../test/helpers/diff-helpers"; @@ -28,7 +29,7 @@ const AFTER = lines( ); const originalEditor = process.env.EDITOR; -const originalSpawnSync = Bun.spawnSync; +const originalSpawn = Bun.spawn; const tempDirs: string[] = []; let setup: Awaited> | undefined; @@ -40,28 +41,35 @@ function createTempWorkspace() { return dir; } -function mockSpawnSync(implementation: typeof Bun.spawnSync) { - const mutableBun = Bun as unknown as { spawnSync: typeof Bun.spawnSync }; - mutableBun.spawnSync = implementation; +function mockSpawn(implementation: (commands: string[]) => { exited: Promise }) { + const mutableBun = Bun as unknown as { spawn: typeof Bun.spawn }; + mutableBun.spawn = implementation as unknown as typeof Bun.spawn; } /** Bootstrap one working-tree review whose file really exists under `sourceLabel`. */ -function createEditorBootstrap(sourceLabel: string): AppBootstrap { - return createTestVcsAppBootstrap({ - changesetId: "changeset:edit-in-editor", - initialMode: "stack", - sourceLabel, - files: [ - createTestDiffFile({ - after: AFTER, - agent: false, - before: BEFORE, - context: 3, - id: "sample", - path: "sample.ts", - }), - ], - }); +function createEditorBootstrap(sourceLabel: string, repoRoot = sourceLabel): AppBootstrap { + return { + ...createTestVcsAppBootstrap({ + changesetId: "changeset:edit-in-editor", + initialMode: "stack", + sourceLabel, + files: [ + { + ...createTestDiffFile({ + after: AFTER, + agent: false, + before: BEFORE, + context: 3, + id: "sample", + path: "sample.ts", + }), + sourcePaths: { old: null, new: join(repoRoot, "sample.ts") }, + }, + ], + }), + extensions: createEmptyExtensionLoadResult(repoRoot), + reloadContext: { cwd: repoRoot, repoRoot }, + }; } async function flush(target: Awaited>) { @@ -99,7 +107,7 @@ afterEach(async () => { } else { process.env.EDITOR = originalEditor; } - mockSpawnSync(originalSpawnSync); + (Bun as unknown as { spawn: typeof Bun.spawn }).spawn = originalSpawn; while (tempDirs.length > 0) { const dir = tempDirs.pop(); @@ -119,7 +127,7 @@ describe("AppHost edit-selected-file shortcut", () => { await pressKeys(setup, "e"); - // openSelectedFileInEditor returns "$EDITOR is not set." which shows as a session notice. + // The bundled editor extension owns editor configuration and reports its refusal. expect(setup.captureCharFrame()).toContain("EDITOR is not set"); }); @@ -128,12 +136,15 @@ describe("AppHost edit-selected-file shortcut", () => { process.env.EDITOR = "vim"; const spawnCalls: string[][] = []; - mockSpawnSync(((cmds: string[]) => { + mockSpawn((cmds) => { spawnCalls.push(cmds); - return { exitCode: 1 }; - }) as unknown as typeof Bun.spawnSync); + return { exited: Promise.resolve(1) }; + }); - setup = await testRender(, WIDE); + setup = await testRender( + , + WIDE, + ); await flush(setup); // The hunk starts at line 1; step down onto the changed line, then one line past it. diff --git a/src/ui/AppHost.extension-dialogs.test.tsx b/src/ui/AppHost.extension-dialogs.test.tsx index 7df60410d..84da7f025 100644 --- a/src/ui/AppHost.extension-dialogs.test.tsx +++ b/src/ui/AppHost.extension-dialogs.test.tsx @@ -214,6 +214,48 @@ function writeDialogFixture(extPath: string, logPath: string, askSource: string) } describe("extension dialogs", () => { + test("dialogs cancel immediately instead of queueing while an application owns the terminal", async () => { + const repo = createTestRepo("hunk-ext-dialog-app-owner-"); + const extDir = createTempDir("hunk-ext-dialog-app-owner-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeFileSync( + extPath, + `import { appendFileSync } from "node:fs";\n` + + `export default function (hunk) {\n` + + ` hunk.registerCommand({ id: "ask-in-app", title: "Ask in app", key: "y" }, async (ctx) => {\n` + + ` const pending = ctx.dialogs.confirm({ title: "Already queued confirm" });\n` + + ` const answers = await ctx.openInApp(async () => await Promise.all([\n` + + ` pending,\n` + + ` ctx.dialogs.confirm({ title: "Invisible confirm" }),\n` + + ` ctx.dialogs.select({ title: "Invisible select", options: ["one"] }),\n` + + ` ctx.dialogs.input({ title: "Invisible input" }),\n` + + ` ]));\n` + + ` appendFileSync(${JSON.stringify(logPath)}, JSON.stringify(answers) + "\\n");\n` + + ` });\n` + + `}\n`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("[false,false,null,null]"), + "the app-owned dialogs to resolve their cancel values", + ); + + const frame = setup.captureCharFrame(); + expect(frame).toContain("alpha.txt"); + expect(frame).not.toContain("Already queued confirm"); + expect(frame).not.toContain("Invisible confirm"); + expect(frame).not.toContain("Invisible select"); + expect(frame).not.toContain("Invisible input"); + }); + }); + test("a confirm dialog renders with attribution and resolves true on enter", async () => { const repo = createTestRepo("hunk-ext-dialog-confirm-"); // Outside the repo, so the fixture and its log never join the review. diff --git a/src/ui/currentReviewRefresh.ts b/src/ui/currentReviewRefresh.ts index debdf5ba9..96b82bd28 100644 --- a/src/ui/currentReviewRefresh.ts +++ b/src/ui/currentReviewRefresh.ts @@ -1,8 +1,8 @@ /** * Describes how the currently mounted review can be rebuilt from its original input. * - * Manual refresh, watch mode, editor return, extension trust reloads, and completed workspace - * writes all reuse this descriptor. It reapplies live view options so a soft reload does not + * Manual refresh, watch mode, extension trust reloads, and completed workspace writes all reuse + * this descriptor. It reapplies live view options so a soft reload does not * fall back to launch-time settings, and it supplies a source path only for VCS-backed reviews. * * Stdin-backed inputs remain non-reloadable because refreshing must not attempt to reread diff --git a/src/ui/hooks/useCurrentReviewRefreshController.ts b/src/ui/hooks/useCurrentReviewRefreshController.ts index 05c78046a..1afa12717 100644 --- a/src/ui/hooks/useCurrentReviewRefreshController.ts +++ b/src/ui/hooks/useCurrentReviewRefreshController.ts @@ -1,8 +1,8 @@ /** * Coordinates every in-session refresh of the currently mounted review. * - * Watch changes, manual commands, editor return, extension trust grants, and completed workspace - * writes converge on the same reloadable review descriptor. This hook derives and registers that + * Watch changes, manual commands, extension trust grants, and completed workspace writes converge + * on the same reloadable review descriptor. This hook derives and registers that * descriptor, connects watch notifications to refreshes, and exposes stable refresh callbacks to * App. * diff --git a/src/ui/hooks/useExtensionAppController.test.tsx b/src/ui/hooks/useExtensionAppController.test.tsx new file mode 100644 index 000000000..6fccb4e63 --- /dev/null +++ b/src/ui/hooks/useExtensionAppController.test.tsx @@ -0,0 +1,243 @@ +import { describe, expect, mock, test } from "bun:test"; +import { testRender } from "@opentui/react/test-utils"; +import { act } from "react"; +import { useExtensionAppController } from "./useExtensionAppController"; + +/** Build one renderer identity whose terminal ownership can outlive hook mounts. */ +function createTestAppRenderer(suspend: () => void = () => {}) { + return { + destroyed: false, + resume: mock(() => {}), + suspend: mock(suspend), + renderer: null as unknown as { + readonly isDestroyed: boolean; + resume: () => void; + suspend: () => void; + }, + }; +} + +/** Mount app controls with mutable review authority and a traced renderer. */ +async function renderController( + appRenderer = createTestAppRenderer(), + onOwnershipStarted = mock(() => {}), +) { + let live = true; + let controller!: ReturnType; + appRenderer.renderer ||= { + get isDestroyed() { + return appRenderer.destroyed; + }, + suspend: appRenderer.suspend, + resume: appRenderer.resume, + }; + + function Harness() { + controller = useExtensionAppController({ + createReviewCapabilityLease: () => ({ isLive: () => live }), + onOwnershipStarted, + renderer: appRenderer.renderer, + }); + return null; + } + + const setup = await testRender(, { width: 20, height: 2 }); + await act(async () => setup.renderOnce()); + return { + controller: () => controller, + destroyRenderer: () => { + appRenderer.destroyed = true; + }, + onOwnershipStarted, + resume: appRenderer.resume, + retire: () => { + live = false; + }, + setup, + suspend: appRenderer.suspend, + }; +} + +describe("useExtensionAppController", () => { + test("exposes renderer ownership while suspended and passes the application result through", async () => { + const harness = await renderController(); + const openInApp = harness.controller().createOpenInApp(); + const calls: string[] = []; + let finish!: (value: number) => void; + const result = new Promise((resolve) => { + finish = resolve; + }); + + try { + expect(harness.controller().isAppActive()).toBe(false); + const active = openInApp(async () => { + calls.push("app"); + expect(harness.controller().isAppActive()).toBe(true); + return await result; + }); + + expect(harness.controller().isAppActive()).toBe(true); + expect(calls).toEqual(["app"]); + expect(harness.onOwnershipStarted).toHaveBeenCalledTimes(1); + expect(harness.suspend).toHaveBeenCalledTimes(1); + expect(harness.resume).not.toHaveBeenCalled(); + + finish(42); + await expect(active).resolves.toBe(42); + + expect(harness.controller().isAppActive()).toBe(false); + expect(harness.resume).toHaveBeenCalledTimes(1); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("restores after application failures without replacing their error", async () => { + const harness = await renderController(); + const openInApp = harness.controller().createOpenInApp(); + const failure = new Error("app failed"); + + try { + await expect( + openInApp(() => { + throw failure; + }), + ).rejects.toBe(failure); + expect(harness.controller().isAppActive()).toBe(false); + expect(harness.resume).toHaveBeenCalledTimes(1); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("refuses stale and concurrent handoffs before invoking extension code", async () => { + const harness = await renderController(); + const stale = harness.controller().createOpenInApp(); + harness.retire(); + + try { + let staleRuns = 0; + await expect( + stale(() => { + staleRuns += 1; + }), + ).rejects.toThrow("after the review reloads"); + expect(staleRuns).toBe(0); + + const currentHarness = await renderController(); + try { + let finish!: () => void; + const waiting = new Promise((resolve) => { + finish = resolve; + }); + const first = currentHarness.controller().createOpenInApp(); + const second = currentHarness.controller().createOpenInApp(); + const active = first(async () => await waiting); + await expect(second(() => "never")).rejects.toThrow("another application owns"); + finish(); + await active; + } finally { + await act(async () => currentHarness.setup.renderer.destroy()); + } + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("keeps terminal ownership across controller remounts", async () => { + const renderer = createTestAppRenderer(); + const firstHarness = await renderController(renderer); + const secondHarness = await renderController(renderer); + let finish!: () => void; + const waiting = new Promise((resolve) => { + finish = resolve; + }); + + try { + const active = firstHarness.controller().createOpenInApp()(async () => await waiting); + expect(firstHarness.controller().isAppActive()).toBe(true); + expect(secondHarness.controller().isAppActive()).toBe(true); + await expect(secondHarness.controller().createOpenInApp()(() => "never")).rejects.toThrow( + "another application owns", + ); + finish(); + await active; + expect(firstHarness.controller().isAppActive()).toBe(false); + expect(secondHarness.controller().isAppActive()).toBe(false); + expect(renderer.suspend).toHaveBeenCalledTimes(1); + expect(renderer.resume).toHaveBeenCalledTimes(1); + } finally { + await act(async () => firstHarness.setup.renderer.destroy()); + await act(async () => secondHarness.setup.renderer.destroy()); + } + }); + + test("does not resume a renderer destroyed while the app owns the terminal", async () => { + const harness = await renderController(); + const openInApp = harness.controller().createOpenInApp(); + + try { + await openInApp(() => harness.destroyRenderer()); + expect(harness.resume).not.toHaveBeenCalled(); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("releases renderer ownership when suspension fails before the callback runs", async () => { + let suspensionAttempts = 0; + const renderer = createTestAppRenderer(() => { + suspensionAttempts += 1; + if (suspensionAttempts === 1) throw new Error("suspend failed"); + }); + const harness = await renderController(renderer); + let runs = 0; + + try { + await expect( + harness.controller().createOpenInApp()(() => { + runs += 1; + }), + ).rejects.toThrow("suspend failed"); + expect(runs).toBe(0); + expect(harness.controller().isAppActive()).toBe(false); + + await expect(harness.controller().createOpenInApp()(() => "restored")).resolves.toBe( + "restored", + ); + expect(harness.controller().isAppActive()).toBe(false); + expect(harness.resume).toHaveBeenCalledTimes(1); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + + test("does not replace the application's result when renderer restoration fails", async () => { + let controller!: ReturnType; + function Harness() { + controller = useExtensionAppController({ + createReviewCapabilityLease: () => ({ isLive: () => true }), + onOwnershipStarted: () => {}, + renderer: { + isDestroyed: false, + suspend: () => {}, + resume: () => { + throw new Error("resume failed"); + }, + }, + }); + return null; + } + const setup = await testRender(, { width: 20, height: 2 }); + await act(async () => setup.renderOnce()); + const originalError = console.error; + console.error = mock(() => {}); + try { + await expect(controller.createOpenInApp()(() => "app result")).resolves.toBe("app result"); + expect(console.error).toHaveBeenCalled(); + } finally { + console.error = originalError; + await act(async () => setup.renderer.destroy()); + } + }); +}); diff --git a/src/ui/hooks/useExtensionAppController.ts b/src/ui/hooks/useExtensionAppController.ts new file mode 100644 index 000000000..843886f7b --- /dev/null +++ b/src/ui/hooks/useExtensionAppController.ts @@ -0,0 +1,61 @@ +import type { CliRenderer } from "@opentui/core"; +import { useCallback, useMemo } from "react"; +import type { ExtensionCommandContext } from "../../extension-api/types"; +import type { ExtensionCapabilityLease } from "../lib/extensionCapabilityLease"; + +/** The terminal handoff capability installed on one extension command context. */ +export type ExtensionOpenInApp = ExtensionCommandContext["openInApp"]; + +const activeAppByRenderer = new WeakMap(); + +/** Build command-scoped app handoffs around one renderer's terminal ownership. */ +export function useExtensionAppController({ + createReviewCapabilityLease, + onOwnershipStarted, + renderer, +}: { + createReviewCapabilityLease: () => ExtensionCapabilityLease; + /** Settle host UI before the renderer gives the terminal away. */ + onOwnershipStarted: () => void; + renderer: Pick; +}) { + /** Report whether an extension application currently owns this renderer's terminal. */ + const isAppActive = useCallback(() => activeAppByRenderer.has(renderer), [renderer]); + + const createOpenInApp = useCallback((): ExtensionOpenInApp => { + const lease = createReviewCapabilityLease(); + return async (run: () => Result | PromiseLike): Promise => { + if (typeof run !== "function") { + throw new Error("openInApp requires an application callback."); + } + if (!lease.isLive()) { + throw new Error("openInApp is unavailable after the review reloads."); + } + if (isAppActive()) { + throw new Error("openInApp is unavailable while another application owns the terminal."); + } + + const ownership = {}; + activeAppByRenderer.set(renderer, ownership); + let suspended = false; + try { + onOwnershipStarted(); + renderer.suspend(); + suspended = true; + return await run(); + } finally { + const stillOwnsTerminal = activeAppByRenderer.get(renderer) === ownership; + if (stillOwnsTerminal) activeAppByRenderer.delete(renderer); + if (stillOwnsTerminal && suspended && !renderer.isDestroyed) { + try { + renderer.resume(); + } catch (error) { + console.error("Failed to restore Hunk after an extension application.", error); + } + } + } + }; + }, [createReviewCapabilityLease, isAppActive, onOwnershipStarted, renderer]); + + return useMemo(() => ({ createOpenInApp, isAppActive }), [createOpenInApp, isAppActive]); +} diff --git a/src/ui/hooks/useExtensionCommandRunner.test.tsx b/src/ui/hooks/useExtensionCommandRunner.test.tsx index 39949918c..85c530d2d 100644 --- a/src/ui/hooks/useExtensionCommandRunner.test.tsx +++ b/src/ui/hooks/useExtensionCommandRunner.test.tsx @@ -31,6 +31,7 @@ const selection = Object.freeze({ hunkIndex: null, currentLine: null, }) as ExtensionReviewSelection; +const openInApp = async (run: () => Result | PromiseLike) => await run(); /** Mount the command runner and expose its stable invocation callback. */ async function renderRunner({ @@ -50,6 +51,7 @@ async function renderRunner({ createKeyboardModeControls: () => keyboardModes, createLineHighlightControls: () => highlights, createNavigation: () => navigation, + createOpenInApp: () => openInApp, createPaneControls: createPanes, createReviewControls: () => review, createWorkspaceControls: () => workspace, @@ -92,6 +94,7 @@ describe("useExtensionCommandRunner", () => { highlights, keyboardModes, navigation, + openInApp, panes, review, selection, diff --git a/src/ui/hooks/useExtensionCommandRunner.ts b/src/ui/hooks/useExtensionCommandRunner.ts index f1812d1ca..14014c1ec 100644 --- a/src/ui/hooks/useExtensionCommandRunner.ts +++ b/src/ui/hooks/useExtensionCommandRunner.ts @@ -22,6 +22,7 @@ import type { ExtensionWorkspace, } from "../../extension-api/types"; import type { ExtensionLoadResult, RegisteredCommand } from "../../extensions/types"; +import type { ExtensionOpenInApp } from "./useExtensionAppController"; /** Describe an extension command failure without assuming an Error instance. */ function commandFailureMessage(registered: RegisteredCommand, error: unknown) { @@ -39,6 +40,7 @@ export function useExtensionCommandRunner({ createKeyboardModeControls, createLineHighlightControls, createNavigation, + createOpenInApp, createPaneControls, createReviewControls, createWorkspaceControls, @@ -54,6 +56,7 @@ export function useExtensionCommandRunner({ ) => ExtensionKeyboardModeControls; createLineHighlightControls: (extensionId: string) => ExtensionLineHighlightControls; createNavigation: (extensionId: string) => ExtensionReviewNavigation; + createOpenInApp: () => ExtensionOpenInApp; createPaneControls: (extensionId: string) => ExtensionPaneControls; createReviewControls: () => ExtensionReviewControls; createWorkspaceControls: (extensionId: string) => ExtensionWorkspace; @@ -74,6 +77,7 @@ export function useExtensionCommandRunner({ commands: commandControls, keyboardModes: createKeyboardModeControls(registered.extensionId, extensions?.registry), notify: (message, type) => extensions?.context.notify(message, type), + openInApp: createOpenInApp(), panes, sidebars: panes, fileViews: createFileViewControls(registered.extensionId), @@ -101,6 +105,7 @@ export function useExtensionCommandRunner({ createKeyboardModeControls, createLineHighlightControls, createNavigation, + createOpenInApp, createPaneControls, createReviewControls, createWorkspaceControls, diff --git a/src/ui/hooks/useExtensionDialogController.ts b/src/ui/hooks/useExtensionDialogController.ts index 0ac595b72..6fd1b925c 100644 --- a/src/ui/hooks/useExtensionDialogController.ts +++ b/src/ui/hooks/useExtensionDialogController.ts @@ -14,6 +14,8 @@ export interface ExtensionDialogController { inputValue: string; accept: (selectedIndexOverride?: number) => void; cancel: () => void; + /** Cancel the visible request and every queued request with their kind-specific values. */ + cancelAll: () => void; moveSelection: (delta: number) => void; pickOption: (index: number) => void; updateInput: (value: string) => void; @@ -87,6 +89,7 @@ export function useExtensionDialogController({ inputValue, accept, cancel, + cancelAll: queue.cancelAll, moveSelection, pickOption: setSelectedIndex, updateInput: setInputValue, diff --git a/src/ui/hooks/useExtensionWorkspaceControls.test.tsx b/src/ui/hooks/useExtensionWorkspaceControls.test.tsx index 68ed4158d..f8878eacb 100644 --- a/src/ui/hooks/useExtensionWorkspaceControls.test.tsx +++ b/src/ui/hooks/useExtensionWorkspaceControls.test.tsx @@ -7,6 +7,7 @@ import { act, useState } from "react"; import type { CliInput } from "../../core/run/commandInputs"; import type { ExtensionConfirmOptions } from "../../extension-api/types"; import type { WorkspaceFileSource } from "../lib/extensionWorkspace"; +import { createTestDiffFile } from "../../../test/helpers/diff-helpers"; import { useExtensionWorkspaceControls, type WorkspaceFileWriter, @@ -18,6 +19,11 @@ const EXPIRED = { reason: "unavailable", detail: "The review reloaded before this extension operation could finish.", } as const; +const APP_ACTIVE = { + ok: false, + reason: "unavailable", + detail: "Workspace writes are unavailable while another application owns the terminal.", +} as const; const WRITABLE_INPUT: CliInput = { kind: "vcs", staged: false, options: {} }; const tempDirs: string[] = []; @@ -35,10 +41,9 @@ function createTestRoot() { /** Build one reviewed file carrying an optional full-source reader. */ function createTestFile(overrides: Partial = {}): WorkspaceFileSource { + const diff = createTestDiffFile({ id: "alpha", path: "alpha.txt" }); return { - id: "alpha", - path: "alpha.txt", - metadata: { type: "change" }, + ...diff, sourceFetcher: { getFullText: async (side) => `${side} alpha` }, ...overrides, }; @@ -66,6 +71,7 @@ async function renderController({ workspaceFileWriter?: WorkspaceFileWriter; } = {}) { let live = true; + let appActive = false; let controller!: ReturnType; let replaceInputs!: (next: { files: readonly WorkspaceFileSource[]; @@ -77,6 +83,7 @@ async function renderController({ confirm: (options: ExtensionConfirmOptions) => confirm(options, extensionId), }); const createReviewCapabilityLease = () => ({ isLive: () => live }); + const isAppActive = () => appActive; function Harness() { const [liveInputs, setLiveInputs] = useState({ files, input, root }); @@ -87,6 +94,7 @@ async function renderController({ createExtensionDialogs, createReviewCapabilityLease, ...liveInputs, + isAppActive, onWorkspaceWriteCompleted, runWorkspaceWrite, workspaceFileWriter, @@ -97,7 +105,13 @@ async function renderController({ const setup = await testRender(, { width: 20, height: 4 }); await act(async () => setup.renderOnce()); return { + claimApp: () => { + appActive = true; + }, controller: () => controller, + releaseApp: () => { + appActive = false; + }, replaceInputs: async (next: { files: readonly WorkspaceFileSource[]; input: CliInput; @@ -223,6 +237,30 @@ describe("useExtensionWorkspaceControls reads", () => { await destroy(harness.setup); } }); + + test("resolves live app locations and makes retained resolvers inert", async () => { + const root = createTestRoot(); + const harness = await renderController({ + files: [ + createTestFile({ + sourcePaths: { old: null, new: join(root, "alpha.txt") }, + }), + ], + root, + }); + const workspace = harness.controller().createWorkspaceControls("probe"); + + try { + expect(workspace.resolveLocation({ fileId: "alpha" })).toEqual({ + path: join(root, "alpha.txt"), + line: 1, + }); + harness.retire(); + expect(workspace.resolveLocation({ fileId: "alpha" })).toBeNull(); + } finally { + await destroy(harness.setup); + } + }); }); describe("useExtensionWorkspaceControls lifecycle", () => { @@ -282,6 +320,140 @@ describe("useExtensionWorkspaceControls lifecycle", () => { }); describe("useExtensionWorkspaceControls writes", () => { + test("refuses writes during app ownership without disabling reads or retained controls", async () => { + const root = createTestRoot(); + let prompts = 0; + let writes = 0; + const harness = await renderController({ + files: [ + createTestFile({ + sourcePaths: { old: null, new: join(root, "alpha.txt") }, + }), + ], + root, + confirm: async () => { + prompts += 1; + return true; + }, + workspaceFileWriter: async () => { + writes += 1; + }, + }); + const workspace = harness.controller().createWorkspaceControls("probe"); + harness.claimApp(); + + try { + expect(workspace.canWriteDocument("alpha")).toBe(false); + await expect( + workspace.writeDocument({ fileId: "alpha", text: "replacement" }), + ).resolves.toEqual(APP_ACTIVE); + await expect(workspace.writeDocument({ fileId: "", text: "replacement" })).rejects.toThrow( + "non-empty fileId", + ); + await expect(workspace.readDocument("alpha", "new")).resolves.toBe("new alpha"); + expect(workspace.resolveLocation({ fileId: "alpha" })).toEqual({ + path: join(root, "alpha.txt"), + line: 1, + }); + expect(prompts).toBe(0); + expect(writes).toBe(0); + + harness.releaseApp(); + expect(workspace.canWriteDocument("alpha")).toBe(true); + await expect( + workspace.writeDocument({ fileId: "alpha", text: "replacement" }), + ).resolves.toEqual({ ok: true }); + expect(prompts).toBe(1); + expect(writes).toBe(1); + } finally { + await destroy(harness.setup); + } + }); + + test("refuses ownership acquired during verification or consent and preserves stale precedence", async () => { + let prompts = 0; + const verifyingHarness = await renderController({ + confirm: async () => { + prompts += 1; + return true; + }, + }); + + try { + const pending = verifyingHarness + .controller() + .createWorkspaceControls("probe") + .writeDocument({ fileId: "alpha", text: "replacement" }); + verifyingHarness.claimApp(); + await expect(pending).resolves.toEqual(APP_ACTIVE); + expect(prompts).toBe(0); + } finally { + await destroy(verifyingHarness.setup); + } + + let confirmStarted!: () => void; + const started = new Promise((resolve) => { + confirmStarted = resolve; + }); + let resolveConfirm!: (confirmed: boolean) => void; + const confirmation = new Promise((resolve) => { + resolveConfirm = resolve; + }); + let writes = 0; + const consentHarness = await renderController({ + confirm: async () => { + confirmStarted(); + return await confirmation; + }, + workspaceFileWriter: async () => { + writes += 1; + }, + }); + + try { + const pending = consentHarness + .controller() + .createWorkspaceControls("probe") + .writeDocument({ fileId: "alpha", text: "replacement" }); + await started; + consentHarness.claimApp(); + resolveConfirm(true); + await expect(pending).resolves.toEqual(APP_ACTIVE); + expect(writes).toBe(0); + } finally { + await destroy(consentHarness.setup); + } + + let staleConfirmStarted!: () => void; + const staleStarted = new Promise((resolve) => { + staleConfirmStarted = resolve; + }); + let resolveStaleConfirm!: (confirmed: boolean) => void; + const staleConfirmation = new Promise((resolve) => { + resolveStaleConfirm = resolve; + }); + const staleHarness = await renderController({ + confirm: async () => { + staleConfirmStarted(); + return await staleConfirmation; + }, + }); + + try { + const pending = staleHarness + .controller() + .createWorkspaceControls("probe") + .writeDocument({ fileId: "alpha", text: "replacement" }); + await staleStarted; + staleHarness.claimApp(); + staleHarness.retire(); + resolveStaleConfirm(true); + await expect(pending).resolves.toEqual(EXPIRED); + } finally { + await destroy(staleHarness.setup); + } + }); + test("throws for malformed requests and refuses unwritable reviews without prompting", async () => { let prompts = 0; const harness = await renderController({ @@ -557,6 +729,7 @@ describe("useExtensionWorkspaceControls writes", () => { .writeDocument({ fileId: "alpha", text: "replacement\n" }); await started; harness.retire(); + harness.claimApp(); finishWrite(); await expect(pending).resolves.toEqual({ ok: true }); diff --git a/src/ui/hooks/useExtensionWorkspaceControls.ts b/src/ui/hooks/useExtensionWorkspaceControls.ts index a3e1e7624..caafb8600 100644 --- a/src/ui/hooks/useExtensionWorkspaceControls.ts +++ b/src/ui/hooks/useExtensionWorkspaceControls.ts @@ -10,13 +10,16 @@ import type { ExtensionDialogs, ExtensionFileSide, ExtensionWorkspace, + ExtensionWorkspaceLocationRequest, ExtensionWorkspaceWriteRequest, ExtensionWorkspaceWriteResult, } from "../../extension-api/types"; import type { ExtensionCapabilityLease } from "../lib/extensionCapabilityLease"; import { + normalizeWorkspaceLocationRequest, normalizeWorkspaceWriteRequest, resolveExtensionWorkspaceRead, + resolveExtensionWorkspaceLocation, resolveExtensionWorkspaceWriteTarget, type WorkspaceFileSource, } from "../lib/extensionWorkspace"; @@ -48,12 +51,22 @@ function expiredWorkspaceWrite(): ExtensionWorkspaceWriteResult { }; } +/** Describe a write that cannot ask for consent while another application owns the terminal. */ +function appActiveWorkspaceWrite(): ExtensionWorkspaceWriteResult { + return { + ok: false, + reason: "unavailable", + detail: "Workspace writes are unavailable while another application owns the terminal.", + }; +} + /** Own live reviewed-document inputs and host-mediated extension workspace operations. */ export function useExtensionWorkspaceControls({ createExtensionDialogs, createReviewCapabilityLease, files, input, + isAppActive, onWorkspaceWriteCompleted, root, runWorkspaceWrite, @@ -67,6 +80,8 @@ export function useExtensionWorkspaceControls({ files: readonly WorkspaceFileSource[]; /** The current CLI review input that decides whether writes are meaningful. */ input: CliInput; + /** Whether an extension application currently owns the terminal renderer. */ + isAppActive: () => boolean; /** Reconcile the review currently mounted by the host after a successful write. */ onWorkspaceWriteCompleted: () => void; /** The current repository root, or the review's working directory. */ @@ -83,6 +98,12 @@ export function useExtensionWorkspaceControls({ const lease = createReviewCapabilityLease(); const resolveTarget = (fileId: string) => resolveExtensionWorkspaceWriteTarget({ fileId, ...liveInputsRef.current }); + const writeAuthorityRefusal = () => { + // Stale review authority remains the more fundamental refusal when both conditions apply. + if (!lease.isLive()) return expiredWorkspaceWrite(); + if (isAppActive()) return appActiveWorkspaceWrite(); + return null; + }; return { async readDocument(fileId: string, side: ExtensionFileSide) { @@ -98,16 +119,30 @@ export function useExtensionWorkspaceControls({ const document = read ? await read().catch(() => null) : null; return lease.isLive() ? document : null; }, + resolveLocation(request: ExtensionWorkspaceLocationRequest) { + const normalized = normalizeWorkspaceLocationRequest(request); + if (!lease.isLive()) return null; + return resolveExtensionWorkspaceLocation({ + files: liveInputsRef.current.files, + request: normalized, + }); + }, canWriteDocument(fileId: string) { // An affordance probe answers false rather than throwing for malformed ids. - return lease.isLive() && typeof fileId === "string" && resolveTarget(fileId).writable; + return ( + lease.isLive() && + !isAppActive() && + typeof fileId === "string" && + resolveTarget(fileId).writable + ); }, async writeDocument( request: ExtensionWorkspaceWriteRequest, ): Promise { // Malformed requests are extension bugs, including after authority expires. const { fileId, text } = normalizeWorkspaceWriteRequest(request); - if (!lease.isLive()) return expiredWorkspaceWrite(); + const initialRefusal = writeAuthorityRefusal(); + if (initialRefusal) return initialRefusal; const target = resolveTarget(fileId); if (!target.writable) { @@ -123,7 +158,8 @@ export function useExtensionWorkspaceControls({ root, }); const refusal = await verifyTarget(); - if (!lease.isLive()) return expiredWorkspaceWrite(); + const verifiedRefusal = writeAuthorityRefusal(); + if (verifiedRefusal) return verifiedRefusal; if (refusal) { return { ok: false, reason: "unavailable", detail: refusal }; } @@ -133,7 +169,8 @@ export function useExtensionWorkspaceControls({ body: `Extension ${extensionId} will replace this file's contents on disk.`, confirmLabel: "write", }); - if (!lease.isLive()) return expiredWorkspaceWrite(); + const consentRefusal = writeAuthorityRefusal(); + if (consentRefusal) return consentRefusal; if (!confirmed) { return { ok: false, @@ -143,13 +180,15 @@ export function useExtensionWorkspaceControls({ } const changedTargetRefusal = await verifyTarget(); - if (!lease.isLive()) return expiredWorkspaceWrite(); + const reverifiedRefusal = writeAuthorityRefusal(); + if (reverifiedRefusal) return reverifiedRefusal; if (changedTargetRefusal) { return { ok: false, reason: "unavailable", detail: changedTargetRefusal }; } // Authority remains revocable until the host atomically starts the filesystem write. - if (!lease.isLive()) return expiredWorkspaceWrite(); + const writeBoundaryRefusal = writeAuthorityRefusal(); + if (writeBoundaryRefusal) return writeBoundaryRefusal; try { const started = await runWorkspaceWrite(() => workspaceFileWriter(target.absolutePath, text), @@ -174,6 +213,7 @@ export function useExtensionWorkspaceControls({ [ createExtensionDialogs, createReviewCapabilityLease, + isAppActive, onWorkspaceWriteCompleted, runWorkspaceWrite, workspaceFileWriter, diff --git a/src/ui/lib/extensionWorkspace.test.ts b/src/ui/lib/extensionWorkspace.test.ts index 587e84130..f10832432 100644 --- a/src/ui/lib/extensionWorkspace.test.ts +++ b/src/ui/lib/extensionWorkspace.test.ts @@ -1,16 +1,18 @@ import { join, resolve, sep } from "node:path"; import { describe, expect, test } from "bun:test"; +import { createTestDiffFile } from "../../../test/helpers/diff-helpers"; import type { CliInput, CommonOptions } from "../../core/run/commandInputs"; import { + normalizeWorkspaceLocationRequest, normalizeWorkspaceWriteRequest, resolveExtensionWorkspaceRead, + resolveExtensionWorkspaceLocation, resolveExtensionWorkspaceWriteTarget, type WorkspaceFileSource, } from "./extensionWorkspace"; const ROOT = resolve(sep, "repo"); const NO_OPTIONS: CommonOptions = {}; - /** One reviewed file as the workspace policy sees it, changed unless told otherwise. */ function createTestWorkspaceFile( overrides: Partial = {}, @@ -240,3 +242,164 @@ describe("extension workspace write requests", () => { ); }); }); + +describe("extension workspace locations", () => { + test("uses new-side provenance and maps old-side lines from parsed hunk metadata", () => { + const file = { + ...createTestDiffFile({ + id: "alpha", + path: "packages/app/alpha.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + }), + sourcePaths: { + old: join(ROOT, "archive", "alpha.ts"), + new: join(ROOT, "packages", "app", "alpha.ts"), + }, + }; + + expect( + resolveExtensionWorkspaceLocation({ + files: [file], + request: { fileId: "alpha", hunkIndex: 0, line: { side: "old", line: 3 } }, + }), + ).toEqual({ path: join(ROOT, "packages", "app", "alpha.ts"), line: 2 }); + }); + + test("rejects malformed source addresses and returns null for unavailable metadata", () => { + expect(() => normalizeWorkspaceLocationRequest(undefined)).toThrow("non-empty fileId"); + expect(() => normalizeWorkspaceLocationRequest({ fileId: "alpha", hunkIndex: -1 })).toThrow( + "non-negative integer", + ); + expect(() => + normalizeWorkspaceLocationRequest({ + fileId: "alpha", + line: { side: "both", line: 1 }, + }), + ).toThrow('line.side must be "old" or "new"'); + expect( + resolveExtensionWorkspaceLocation({ + files: [createTestWorkspaceFile({ metadata: undefined })], + request: { fileId: "alpha" }, + }), + ).toBeNull(); + }); + + test("preserves old-side offsets through context and multi-line replacements", () => { + const removed = { + ...createTestDiffFile({ + id: "removed", + path: "removed.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + context: 1, + }), + sourcePaths: { old: null, new: join(ROOT, "removed.ts") }, + }; + const replaced = { + ...createTestDiffFile({ + id: "replaced", + path: "replaced.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nTWO\nTHREE\nfour\n", + }), + sourcePaths: { old: null, new: join(ROOT, "replaced.ts") }, + }; + + expect( + resolveExtensionWorkspaceLocation({ + files: [removed], + request: { fileId: "removed", hunkIndex: 0, line: { side: "old", line: 3 } }, + })?.line, + ).toBe(2); + expect( + resolveExtensionWorkspaceLocation({ + files: [replaced], + request: { fileId: "replaced", hunkIndex: 0, line: { side: "old", line: 3 } }, + })?.line, + ).toBe(3); + }); + + test("uses concrete provenance instead of reviewed display paths", () => { + const file = { + ...createTestDiffFile({ id: "alpha", path: "display/after.ts" }), + sourcePaths: { + old: join(ROOT, "concrete", "before.ts"), + new: join(ROOT, "concrete", "after.ts"), + }, + }; + + expect( + resolveExtensionWorkspaceLocation({ + files: [file], + request: { fileId: "alpha", line: { side: "new", line: 2 } }, + }), + ).toEqual({ path: join(ROOT, "concrete", "after.ts"), line: 2 }); + expect( + resolveExtensionWorkspaceLocation({ + files: [{ ...file, sourcePaths: undefined }], + request: { fileId: "alpha" }, + }), + ).toBeNull(); + }); + + test("resolves deleted direct comparisons to their old-side source", () => { + const file = { + ...createTestDiffFile({ + id: "deleted", + path: "deleted.ts", + before: "one\ntwo\n", + after: "", + }), + sourcePaths: { old: join(ROOT, "archive", "deleted.ts"), new: null }, + }; + + expect( + resolveExtensionWorkspaceLocation({ + files: [file], + request: { fileId: "deleted", hunkIndex: 0, line: { side: "old", line: 2 } }, + }), + ).toEqual({ path: join(ROOT, "archive", "deleted.ts"), line: 2 }); + }); + + test("rejects requested absent sides before considering opposite-side provenance", () => { + const added = { + ...createTestDiffFile({ id: "added", before: "", after: "new\n" }), + sourcePaths: { old: null, new: join(ROOT, "added.ts") }, + }; + const deleted = { + ...createTestDiffFile({ id: "deleted", before: "old\n", after: "" }), + sourcePaths: { old: join(ROOT, "deleted.ts"), new: null }, + }; + + expect( + resolveExtensionWorkspaceLocation({ + files: [added], + request: { fileId: "added", line: { side: "old", line: 1 } }, + }), + ).toBeNull(); + expect( + resolveExtensionWorkspaceLocation({ + files: [deleted], + request: { fileId: "deleted", line: { side: "new", line: 1 } }, + }), + ).toBeNull(); + }); + + test("returns null when the relevant side is virtual or its path is not absolute", () => { + const file = createTestDiffFile({ id: "alpha" }); + + expect( + resolveExtensionWorkspaceLocation({ + files: [{ ...file, sourcePaths: { old: join(ROOT, "old.ts"), new: null } }], + request: { fileId: "alpha", hunkIndex: 0, line: { side: "old", line: 1 } }, + }), + ).toBeNull(); + expect( + resolveExtensionWorkspaceLocation({ + files: [{ ...file, sourcePaths: { old: null, new: "relative/alpha.ts" } }], + request: { fileId: "alpha" }, + }), + ).toBeNull(); + }); +}); diff --git a/src/ui/lib/extensionWorkspace.ts b/src/ui/lib/extensionWorkspace.ts index 1d1e76cd9..9d555abbb 100644 --- a/src/ui/lib/extensionWorkspace.ts +++ b/src/ui/lib/extensionWorkspace.ts @@ -19,10 +19,11 @@ import { isAbsolute, relative, resolve, sep } from "node:path"; import { normalizeDiffPath } from "../../core/changeset/diffPaths"; -import type { FileSourceSide } from "../../core/changeset/fileSource"; +import type { FileSourcePaths, FileSourceSide } from "../../core/changeset/fileSource"; import { canReloadInput } from "../../core/run/inputReload"; import type { CliInput } from "../../core/run/commandInputs"; import { readMetadataChangeType } from "../../extensions/events"; +import type { ExtensionWorkspaceLocation } from "../../extension-api/types"; /** * The slice of one reviewed file the workspace policy inspects. @@ -40,6 +41,8 @@ export interface WorkspaceFileSource { isTooLarge?: boolean; /** Absent when the loader had no reachable source for this file. */ sourceFetcher?: { getFullText(side: FileSourceSide): Promise }; + /** Exact absolute paths for only the sides backed by the live filesystem. */ + sourcePaths?: FileSourcePaths; } /** One reviewed document side's read, already bound to the file that answers it. */ @@ -66,6 +69,141 @@ export interface WorkspaceWriteRequestFields { text: string; } +/** A validated reviewed source address ready for workspace resolution. */ +export interface WorkspaceLocationRequestFields { + fileId: string; + hunkIndex?: number; + line?: { side: FileSourceSide; line: number }; +} + +interface WorkspaceLocationHunk { + deletionStart: number; + deletionCount: number; + additionStart: number; + additionCount: number; + hunkContent: Array< + { type: "context"; lines: number } | { type: "change"; deletions: number; additions: number } + >; +} + +interface WorkspaceLocationMetadata { + type: string; + hunks: WorkspaceLocationHunk[]; +} + +/** Reject malformed app-location metadata before it reaches workspace state. */ +export function normalizeWorkspaceLocationRequest( + request: unknown, +): WorkspaceLocationRequestFields { + const fields = request as Partial | null | undefined; + if (typeof fields?.fileId !== "string" || fields.fileId.length === 0) { + throw new Error("workspace.resolveLocation requires a non-empty fileId."); + } + if ( + fields.hunkIndex !== undefined && + (!Number.isInteger(fields.hunkIndex) || fields.hunkIndex < 0) + ) { + throw new Error("workspace.resolveLocation hunkIndex must be a non-negative integer."); + } + if (fields.line !== undefined) { + if (fields.line.side !== "old" && fields.line.side !== "new") { + throw new Error('workspace.resolveLocation line.side must be "old" or "new".'); + } + if (!Number.isInteger(fields.line.line) || fields.line.line < 1) { + throw new Error("workspace.resolveLocation line.line must be a positive integer."); + } + } + return { + fileId: fields.fileId, + ...(fields.hunkIndex === undefined ? {} : { hunkIndex: fields.hunkIndex }), + ...(fields.line === undefined + ? {} + : { line: { side: fields.line.side, line: fields.line.line } }), + }; +} + +/** Translate one old-side line to its corresponding filesystem-backed new-side line. */ +function lineOnDisk(hunk: WorkspaceLocationHunk, deletionLine: number) { + let deletionCursor = hunk.deletionStart; + let additionCursor = hunk.additionCount === 0 ? hunk.additionStart + 1 : hunk.additionStart; + + for (const content of hunk.hunkContent) { + if (content.type === "context") { + if (deletionLine < deletionCursor + content.lines) { + return additionCursor + (deletionLine - deletionCursor); + } + deletionCursor += content.lines; + additionCursor += content.lines; + continue; + } + if (deletionLine < deletionCursor + content.deletions) { + const offset = Math.min(deletionLine - deletionCursor, Math.max(content.additions - 1, 0)); + return additionCursor + offset; + } + deletionCursor += content.deletions; + additionCursor += content.additions; + } + return additionCursor; +} + +/** Resolve a reviewed source address against the authoritative parsed diff. */ +export function resolveExtensionWorkspaceLocation({ + files, + request, +}: { + files: readonly WorkspaceFileSource[]; + request: WorkspaceLocationRequestFields; +}): ExtensionWorkspaceLocation | null { + const file = files.find((candidate) => candidate.id === request.fileId); + if (!file) return null; + const metadata = file.metadata as Partial | undefined; + if (!metadata || typeof metadata.type !== "string" || !Array.isArray(metadata.hunks)) return null; + + const deleted = metadata.type === "deleted"; + if ( + (metadata.type === "new" && request.line?.side === "old") || + (deleted && request.line?.side === "new") + ) { + return null; + } + + let hunkIndex = request.hunkIndex; + if (request.line?.side === "old" && metadata.type !== "deleted") { + hunkIndex ??= metadata.hunks.findIndex( + (hunk) => + request.line!.line >= hunk.deletionStart && + request.line!.line < hunk.deletionStart + hunk.deletionCount, + ); + if (hunkIndex < 0) hunkIndex = undefined; + } + const hunk = hunkIndex === undefined ? undefined : metadata.hunks[hunkIndex]; + if (hunkIndex !== undefined && !hunk) return null; + + let line: number; + if (request.line?.side === (deleted ? "old" : "new")) { + line = request.line.line; + } else if (request.line && !deleted) { + if ( + !hunk || + request.line.line < hunk.deletionStart || + request.line.line >= hunk.deletionStart + hunk.deletionCount + ) { + return null; + } + line = lineOnDisk(hunk, request.line.line); + } else { + line = deleted ? (hunk?.deletionStart ?? 1) : (hunk?.additionStart ?? 1); + } + + const filePath = file.sourcePaths?.[deleted ? "old" : "new"]; + if (!filePath || !isAbsolute(filePath)) return null; + + return { + path: filePath, + line: Math.max(1, line), + }; +} + /** * Name what this session is reviewing when it is not the working tree. * diff --git a/src/ui/lib/openInEditor.test.ts b/src/ui/lib/openInEditor.test.ts deleted file mode 100644 index 16d099fb5..000000000 --- a/src/ui/lib/openInEditor.test.ts +++ /dev/null @@ -1,482 +0,0 @@ -import { afterEach, describe, expect, mock, test } from "bun:test"; -import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; -import { join, resolve } from "node:path"; -import { createTestDiffFile } from "../../../test/helpers/diff-helpers"; -import { - buildEditorCommand, - openSelectedFileInEditor, - resolveEditableFilePath, - shouldSuspendForEditor, -} from "./openInEditor"; - -const originalEditor = process.env.EDITOR; -const originalSpawnSync = Bun.spawnSync; -const tempDirs: string[] = []; - -function createTempDir() { - const dir = realpathSync(mkdtempSync(join(tmpdir(), "hunk-open-editor-"))); - tempDirs.push(dir); - return dir; -} - -function restoreEditorEnv() { - if (originalEditor === undefined) { - delete process.env.EDITOR; - } else { - process.env.EDITOR = originalEditor; - } -} - -function mockSpawnSync( - implementation: (cmds: string[], options?: Parameters[1]) => unknown, -) { - const mutableBun = Bun as unknown as { spawnSync: typeof Bun.spawnSync }; - mutableBun.spawnSync = implementation as typeof Bun.spawnSync; -} - -function createRenderer() { - return { - isDestroyed: false, - resume: mock(() => {}), - suspend: mock(() => {}), - }; -} - -afterEach(() => { - restoreEditorEnv(); - mockSpawnSync(originalSpawnSync); - - while (tempDirs.length > 0) { - const dir = tempDirs.pop(); - if (dir) { - rmSync(dir, { recursive: true, force: true }); - } - } -}); - -describe("open in editor helpers", () => { - test("builds vi-style editor args without shell quoting", () => { - expect( - buildEditorCommand({ - editor: "nvim", - filePath: "/tmp/project/file with spaces's.ts", - line: 12, - }), - ).toEqual({ - command: "nvim", - args: ["+12", "/tmp/project/file with spaces's.ts"], - }); - }); - - test("preserves editor flags before appending the target file", () => { - expect( - buildEditorCommand({ - editor: "code --reuse-window", - filePath: "/tmp/project/example.ts", - line: 4, - }), - ).toEqual({ - command: "code", - args: ["--reuse-window", "--goto", "/tmp/project/example.ts:4"], - }); - }); - - test("handles quoted editor commands and Windows executable paths", () => { - expect( - buildEditorCommand({ - editor: '"C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd" --wait', - filePath: "C:\\Users\\Duarte\\repo\\file with spaces.ts", - line: 7, - }), - ).toEqual({ - command: "C:\\Program Files\\Microsoft VS Code\\bin\\code.cmd", - args: ["--wait", "--goto", "C:\\Users\\Duarte\\repo\\file with spaces.ts:7"], - }); - }); - - test("defaults unknown editors to opening the file path only", () => { - expect( - buildEditorCommand({ - editor: "zed --new-window", - filePath: "/tmp/project/example.ts", - line: 4, - }), - ).toEqual({ - command: "zed", - args: ["--new-window", "/tmp/project/example.ts"], - }); - }); - - test("does not suspend for code-style GUI editors", () => { - expect(shouldSuspendForEditor("code --reuse-window")).toBe(false); - expect(shouldSuspendForEditor('"C:\\Program Files\\Cursor\\cursor.exe"')).toBe(false); - expect(shouldSuspendForEditor("nvim")).toBe(true); - }); - - test("resolves repo-relative diff paths from the diff source path", () => { - expect(resolveEditableFilePath("src/main.tsx", "/tmp/project")).toBe( - resolve("/tmp/project", "src/main.tsx"), - ); - }); - - test("returns an error when no file is selected", () => { - const renderer = createRenderer(); - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - expect( - openSelectedFileInEditor({ - file: undefined, - renderer, - selectedHunk: undefined, - }), - ).toBe("No file selected."); - - expect(spawnCalls).toEqual([]); - expect(renderer.suspend).not.toHaveBeenCalled(); - expect(renderer.resume).not.toHaveBeenCalled(); - }); - - test("returns an error when $EDITOR is unset", () => { - const renderer = createRenderer(); - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - delete process.env.EDITOR; - - expect( - openSelectedFileInEditor({ - file: createTestDiffFile({ path: "missing-editor.ts" }), - renderer, - selectedHunk: undefined, - }), - ).toBe("$EDITOR is not set."); - - expect(spawnCalls).toEqual([]); - expect(renderer.suspend).not.toHaveBeenCalled(); - expect(renderer.resume).not.toHaveBeenCalled(); - }); - - test("returns an error when the file does not exist on disk", () => { - const renderer = createRenderer(); - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - process.env.EDITOR = "nvim"; - - expect( - openSelectedFileInEditor({ - basePath: createTempDir(), - file: createTestDiffFile({ path: "missing-on-disk.ts" }), - renderer, - selectedHunk: undefined, - }), - ).toBe("Cannot edit missing-on-disk.ts: file does not exist on disk."); - - expect(spawnCalls).toEqual([]); - expect(renderer.suspend).not.toHaveBeenCalled(); - expect(renderer.resume).not.toHaveBeenCalled(); - }); - - test("spawns terminal editors with suspend and resume around a successful edit", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "nvim --clean"; - - const spawnCalls: Array<{ - cmds: string[]; - options: Parameters[1] | undefined; - }> = []; - mockSpawnSync((cmds, options) => { - spawnCalls.push({ cmds, options }); - return { exitCode: 0 }; - }); - - const renderer = createRenderer(); - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - renderer, - selectedHunk: undefined, - }), - ).toBeNull(); - - expect(spawnCalls).toEqual([ - { - cmds: ["nvim", "--clean", "+1", join(basePath, "example.ts")], - options: { stdin: "inherit", stdout: "inherit", stderr: "inherit" }, - }, - ]); - expect(renderer.suspend).toHaveBeenCalledTimes(1); - expect(renderer.resume).toHaveBeenCalledTimes(1); - }); - - test("opens the current line instead of the selected hunk start", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: file.id, - hunkIndex: 1, - target: { side: "new", line: 3 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[0], - }), - ).toBeNull(); - - expect(spawnCalls).toEqual([["vim", "+3", join(basePath, "example.ts")]]); - }); - - test("maps an old-side current line onto the line on disk", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "one\nfour\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ - path: "example.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nfour\n", - }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: file.id, - hunkIndex: 0, - target: { side: "old", line: 3 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[0], - }), - ).toBeNull(); - - expect(spawnCalls).toEqual([["vim", "+2", join(basePath, "example.ts")]]); - }); - - test("walks leading context when mapping an old-side current line", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "one\nfour\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ - path: "example.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nfour\n", - context: 1, - }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: file.id, - hunkIndex: 0, - target: { side: "old", line: 3 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[0], - }), - ).toBeNull(); - - // Old line 3 ("three") was removed, so the editor lands on the line that now follows "one". - expect(spawnCalls).toEqual([["vim", "+2", join(basePath, "example.ts")]]); - }); - - test("preserves the deleted line's offset within a multi-line replacement", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "one\nTWO\nTHREE\nfour\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ - path: "example.ts", - before: "one\ntwo\nthree\nfour\n", - after: "one\nTWO\nTHREE\nfour\n", - }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: file.id, - hunkIndex: 0, - target: { side: "old", line: 3 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[0], - }), - ).toBeNull(); - - // Old line 3 ("three") is the second of two replaced lines, so the editor - // lands on the second replacement line ("THREE") rather than the first. - expect(spawnCalls).toEqual([["vim", "+3", join(basePath, "example.ts")]]); - }); - - test("falls back to the selected hunk when the cursor is in another file", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - lineCursor: { - fileId: "other-file", - hunkIndex: 0, - target: { side: "new", line: 42 }, - }, - renderer: createRenderer(), - selectedHunk: file.metadata.hunks[1], - }), - ).toBeNull(); - - expect(spawnCalls).toEqual([ - ["vim", `+${file.metadata.hunks[1]!.additionStart}`, join(basePath, "example.ts")], - ]); - }); - - test("uses deletion line numbers for deleted files", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "deleted.ts"), "const old = true;\n"); - process.env.EDITOR = "vim"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 0 }; - }); - - const baseFile = createTestDiffFile({ path: "deleted.ts" }); - const file = { - ...baseFile, - metadata: { - ...baseFile.metadata, - type: "deleted" as const, - }, - }; - const selectedHunk = { - ...file.metadata.hunks[0]!, - additionStart: 2, - deletionStart: 9, - }; - - expect( - openSelectedFileInEditor({ - basePath, - file, - renderer: createRenderer(), - selectedHunk, - }), - ).toBeNull(); - - expect(spawnCalls).toEqual([["vim", "+9", join(basePath, "deleted.ts")]]); - }); - - test("does not suspend GUI editors and reports non-zero exits", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "code --wait"; - - const spawnCalls: string[][] = []; - mockSpawnSync((cmds) => { - spawnCalls.push(cmds); - return { exitCode: 2 }; - }); - - const renderer = createRenderer(); - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - renderer, - selectedHunk: file.metadata.hunks[0], - }), - ).toBe("Editor exited with status 2."); - - expect(spawnCalls).toEqual([["code", "--wait", "--goto", `${join(basePath, "example.ts")}:1`]]); - expect(renderer.suspend).not.toHaveBeenCalled(); - expect(renderer.resume).not.toHaveBeenCalled(); - }); - - test("resumes after spawn failures and reports launch errors", () => { - const basePath = createTempDir(); - writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); - process.env.EDITOR = "vi"; - - mockSpawnSync(() => { - throw new Error("boom"); - }); - - const renderer = createRenderer(); - const file = createTestDiffFile({ path: "example.ts" }); - - expect( - openSelectedFileInEditor({ - basePath, - file, - renderer, - selectedHunk: file.metadata.hunks[0], - }), - ).toBe("Failed to launch editor: boom"); - - expect(renderer.suspend).toHaveBeenCalledTimes(1); - expect(renderer.resume).toHaveBeenCalledTimes(1); - }); -}); diff --git a/src/ui/lib/openInEditor.ts b/src/ui/lib/openInEditor.ts deleted file mode 100644 index 21f0ed2dc..000000000 --- a/src/ui/lib/openInEditor.ts +++ /dev/null @@ -1,208 +0,0 @@ -import { existsSync } from "node:fs"; -import { basename, resolve, win32 } from "node:path"; -import type { CliRenderer } from "@opentui/core"; -import type { DiffFile } from "../../core/changeset/model"; -import type { LineCursor } from "./lineCursors"; - -export interface EditorCommand { - command: string; - args: string[]; -} - -type DiffHunk = DiffFile["metadata"]["hunks"][number]; - -/** The review stream's current line, minus the geometry fields this module never reads. */ -export type EditorLineCursor = Pick; - -/** - * Translate an old-side line to the line it maps to in the file on disk. - * - * Deleted lines have no on-disk counterpart, so they resolve to the position their - * replacement occupies, which is where the reader expects the editor to land. - */ -function deletionLineToFileLine(hunk: DiffHunk, deletionLine: number) { - let deletionCursor = hunk.deletionStart; - // A zero-count side names the line before the change, so step past it to land inside the file. - let additionCursor = hunk.additionCount === 0 ? hunk.additionStart + 1 : hunk.additionStart; - - for (const content of hunk.hunkContent) { - if (content.type === "context") { - if (deletionLine < deletionCursor + content.lines) { - return additionCursor + (deletionLine - deletionCursor); - } - - deletionCursor += content.lines; - additionCursor += content.lines; - continue; - } - - if (deletionLine < deletionCursor + content.deletions) { - // Land on the corresponding replacement line, clamped to the last one this block adds. - const offset = Math.min(deletionLine - deletionCursor, Math.max(content.additions - 1, 0)); - return additionCursor + offset; - } - - deletionCursor += content.deletions; - additionCursor += content.additions; - } - - return additionCursor; -} - -/** Prefer the current line over the selected hunk's first line. */ -function selectedLine( - file: DiffFile, - selectedHunk: DiffHunk | undefined, - lineCursor: EditorLineCursor | null | undefined, -) { - // Deleted files are opened against their pre-change content, every other file against its new one. - const isDeleted = file.metadata.type === "deleted"; - const diskSide = isDeleted ? "old" : "new"; - const cursor = lineCursor?.fileId === file.id ? lineCursor : undefined; - - if (cursor) { - if (cursor.target.side === diskSide) { - return cursor.target.line; - } - - const cursorHunk = file.metadata.hunks[cursor.hunkIndex]; - if (!isDeleted && cursorHunk) { - return deletionLineToFileLine(cursorHunk, cursor.target.line); - } - } - - if (isDeleted) { - return selectedHunk?.deletionStart ?? 1; - } - - return selectedHunk?.additionStart ?? 1; -} - -function splitEditorCommand(editor: string) { - return ( - editor - .match(/(?:[^\s"']+|"(?:\\.|[^"])*"|'(?:\\.|[^'])*')+/g) - ?.map((token) => token.replace(/^(["'])(.*)\1$/, "$2")) ?? [] - ); -} - -function editorProgram(editor: string) { - const [firstToken = ""] = splitEditorCommand(editor); - return basename(win32.basename(firstToken)) - .replace(/\.(?:cmd|exe)$/i, "") - .toLowerCase(); -} - -const VI_STYLE_EDITORS = ["vim", "nvim", "vi"]; -const CODE_STYLE_EDITORS = ["code", "code-insiders", "cursor"]; - -/** Suspend for terminal editors. */ -export function shouldSuspendForEditor(editor: string) { - const program = editorProgram(editor); - if (CODE_STYLE_EDITORS.includes(program)) { - return false; - } - - return true; -} - -/** Build an editor process invocation without shell quoting so paths stay cross-platform. */ -export function buildEditorCommand({ - editor, - filePath, - line, -}: { - editor: string; - filePath: string; - line: number; -}): EditorCommand { - const [command = "", ...editorArgs] = splitEditorCommand(editor); - const program = editorProgram(editor); - - if (VI_STYLE_EDITORS.includes(program)) { - return { command, args: [...editorArgs, `+${line}`, filePath] }; - } - - if (CODE_STYLE_EDITORS.includes(program)) { - return { command, args: [...editorArgs, "--goto", `${filePath}:${line}`] }; - } - - if (program == "hx") { - return { command, args: [...editorArgs, `${filePath}:${line}`] }; - } - - return { command, args: [...editorArgs, filePath] }; -} - -/** Resolve diff paths relative to their source repo instead of the launch cwd. */ -export function resolveEditableFilePath(filePath: string, basePath = process.cwd()) { - return resolve(basePath, filePath); -} - -/** Open the selected file in $EDITOR, suspending TUI for terminal editors. */ -export function openSelectedFileInEditor({ - basePath, - file, - lineCursor, - renderer, - selectedHunk, -}: { - basePath?: string; - file: DiffFile | undefined; - lineCursor?: EditorLineCursor | null; - renderer: Pick; - selectedHunk: DiffHunk | undefined; -}) { - if (!file) { - return "No file selected."; - } - - const editor = process.env.EDITOR?.trim(); - if (!editor) { - return "$EDITOR is not set."; - } - - const absolutePath = resolveEditableFilePath(file.path, basePath); - if (!existsSync(absolutePath)) { - return `Cannot edit ${file.path}: file does not exist on disk.`; - } - - const line = Math.max(1, selectedLine(file, selectedHunk, lineCursor)); - const command = buildEditorCommand({ - editor, - filePath: absolutePath, - line, - }); - - const shouldSuspend = shouldSuspendForEditor(editor); - if (shouldSuspend) { - renderer.suspend(); - } - - let exitCode = 0; - let failureMessage: string | null = null; - try { - const result = Bun.spawnSync([command.command, ...command.args], { - stdin: "inherit", - stdout: "inherit", - stderr: "inherit", - }); - exitCode = result.exitCode; - } catch (error) { - failureMessage = error instanceof Error ? error.message : String(error); - } - - if (shouldSuspend && !renderer.isDestroyed) { - renderer.resume(); - } - - if (failureMessage) { - return `Failed to launch editor: ${failureMessage}`; - } - - if (exitCode !== 0) { - return `Editor exited with status ${exitCode}.`; - } - - return null; -} diff --git a/test/pty/extensions-integration.test.ts b/test/pty/extensions-integration.test.ts index ac4467832..8bf421078 100644 --- a/test/pty/extensions-integration.test.ts +++ b/test/pty/extensions-integration.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; -import { existsSync, readFileSync } from "node:fs"; +import { existsSync, readFileSync, writeFileSync } from "node:fs"; import { join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { createPtyHarness, dragMouse, lineIndexOf } from "./harness"; @@ -256,6 +256,27 @@ const DIALOG_EXTENSION_SOURCE = `export default function (hunk) { } `; +const APP_HANDOFF_CHILD_SOURCE = ` +process.stdout.write("\\x1b[2J\\x1b[HCHILD APP ACTIVE\\nreturning to Hunk\\n"); +while (!(await Bun.file(".hunk-child-release").exists())) await Bun.sleep(25); +`; + +/** An extension that gives a child process exclusive use of the real terminal. */ +const APP_HANDOFF_EXTENSION_SOURCE = `export default function (hunk) { + hunk.registerCommand({ id: "open-child", title: "Open child app", key: "y" }, async (ctx) => { + const exitCode = await ctx.openInApp(() => { + const child = Bun.spawnSync([process.execPath, "-e", ${JSON.stringify(APP_HANDOFF_CHILD_SOURCE)}], { + stdin: "inherit", + stdout: "inherit", + stderr: "inherit", + }); + return child.exitCode; + }); + ctx.notify("CHILD APP EXITED " + exitCode); + }); +} +`; + describe("PTY extensions", () => { test("trust prompt runs repo extensions after the user trusts the repository", async () => { const configHome = harness.createIsolatedConfigHome(); @@ -632,6 +653,47 @@ describe("PTY extensions", () => { } }); + test("an extension app takes over the terminal and returns to the review", async () => { + const configHome = harness.createIsolatedConfigHome(); + const fixture = harness.createRepoExtensionFixture(APP_HANDOFF_EXTENSION_SOURCE); + const session = await harness.launchHunk({ + args: [ + "diff", + "--mode", + "stack", + "--extension", + join(fixture.dir, ".hunk", "extensions", "fixture.ts"), + ], + cwd: fixture.dir, + cols: 80, + rows: 20, + env: { XDG_CONFIG_HOME: configHome }, + }); + + try { + await harness.waitForSnapshot(session, (text) => text.includes("alpha.ts"), 20_000); + await harness.ensureKeyboardIsLive(session); + + await session.press("y"); + const childFrame = await harness.waitForSnapshot( + session, + (text) => text.includes("CHILD APP ACTIVE") && text.includes("returning to Hunk"), + 20_000, + ); + expect(childFrame).not.toContain("alpha.ts"); + writeFileSync(join(fixture.dir, ".hunk-child-release"), "release"); + + const restoredFrame = await harness.waitForSnapshot( + session, + (text) => text.includes("alpha.ts") && text.includes("CHILD APP EXITED 0"), + 20_000, + ); + expect(restoredFrame).not.toContain("CHILD APP ACTIVE"); + } finally { + session.close(); + } + }); + test("the real review note navigator inventories and reveals a saved user note", async () => { const configHome = harness.createIsolatedConfigHome(); const fixture = harness.createBottomClampedRepoFixture(); diff --git a/website/src/content/docs/docs/extend/extension-api.md b/website/src/content/docs/docs/extend/extension-api.md index c376bbc25..f37846ef1 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -7,9 +7,11 @@ The extension factory receives one API object. Registration calls are only valid ## `hunk.apiVersion` -The API generation this Hunk speaks (currently `15`). Branch on it if you want -one file to support several Hunk versions. Version 15 adds `{ side, line }` to -opted-in pane `currentLine` paint; version 14 added structured two-revision +The API generation this Hunk speaks (currently `16`). Branch on it if you want +one file to support several Hunk versions. Version 16 adds temporary application +handoffs and on-disk location resolution to command handlers; version 15 added +`{ side, line }` to opted-in pane +`currentLine` paint; version 14 added structured two-revision VCS diff endpoints; version 13 added saved-note parent identities and committed note-edit events; version 12 added responsive fractional pane sizing; version 11 added the `dim` line-highlight tone; version 10 added @@ -333,6 +335,17 @@ Hunk draws the dialog; your text fills the title, body, and choices. Dialogs fro One dialog shows at a time; concurrent requests queue in call order, across extensions. Escape cancels (`false` or `null`), Enter accepts; confirm dialogs also answer to `y`/`n`, select dialogs to `↑`/`↓`, and everything is clickable. A session reload cancels open and queued dialogs, and a dialog pending at shutdown resolves its cancel value. +### Temporary applications + +`ctx.openInApp(callback)` temporarily hands Hunk's terminal to application code +owned by your command. Hunk suspends its renderer, awaits the callback, and +restores the review in `finally`. Your extension owns execution and how file, +line, hunk, or application state reaches the child process. One app can own the +terminal at a time, and stale or concurrent handoffs reject before the callback +runs. Host-presented dialogs cancel immediately and workspace writes are +unavailable while the callback owns the terminal; reads and location resolution +remain available. + ### Workspace documents `ctx.workspace` reads full documents from the current review and writes eligible working-tree files. @@ -340,6 +353,7 @@ One dialog shows at a time; concurrent requests queue in call order, across exte | Method | Result | | -------------------------------------- | ------------------------------------------------- | | `readDocument(fileId, "old" \| "new")` | Reviewed source text or `null` | +| `resolveLocation({ fileId, ... })` | Absolute on-disk `{ path, line }` or `null` | | `canWriteDocument(fileId)` | Whether review policy allows a write | | `writeDocument({ fileId, text })` | `{ ok: true }` or `{ ok: false, reason, detail }` | @@ -355,13 +369,15 @@ if (file && ctx.workspace.canWriteDocument(file.id)) { Reads return the source represented by the review, including historical content in revision and stash reviews. Missing, unreadable, or oversized sources return `null`; reads never prompt. +`resolveLocation` maps a reviewed file id and optional hunk/source line onto an attested absolute path and line on disk. Direct comparisons retain their concrete input path. Index, revision, stash, patch, merged, absent, and other virtual sides return `null` rather than borrowing a same-named checkout file. Hunk uses parsed hunk metadata for old-side mapping; missing hunks and stale locations also return `null`. + Writes require a reloadable, unstaged working-tree review and a writable reviewed-file id. Hunk verifies the target, asks for attributed consent, verifies it again, writes it, and reloads the review. Other review kinds and deleted, binary, oversized, missing, symlinked, or root-escaping targets return `unavailable`. Cancellation returns `cancelled`; an attempted write failure returns `failed` with a displayable `detail`. `canWriteDocument` does not inspect the filesystem, so `writeDocument` can still refuse a changed target. See the [full workspace guide](https://github.com/modem-dev/hunk/blob/main/docs/extensions.md#workspace-documents) for lifecycle and error details. ## `hunk.on(event, handler)` -Subscribe to a lifecycle or UI event. Handlers may be async; Hunk never blocks the UI waiting for one. Every handler receives `ctx.panes`, live `ctx.navigation`, and attributed `ctx.dialogs` alongside `cwd` and `notify`, so a `startup` handler can present one focused welcome dialog and navigate to its first example without a keypress. `ctx.sidebars` is deprecated. Controls retained across a review or extension-registry replacement expire instead of controlling the replacement UI; workspace reads and writes that have not started return `null`/`unavailable`. Once a consented filesystem write starts, it reports its actual outcome, graceful shutdown waits for it, and success reconciles the review then active. +Subscribe to a lifecycle or UI event. Handlers may be async; Hunk never blocks the UI waiting for one. Every handler receives `ctx.panes`, live `ctx.navigation`, and attributed `ctx.dialogs` alongside `cwd` and `notify`, so a `startup` handler can present one focused welcome dialog and navigate to its first example without a keypress. `ctx.sidebars` is deprecated. Controls retained across a review or extension-registry replacement expire instead of controlling the replacement UI; workspace reads and writes that have not started return `null`/`unavailable`. A stale `openInApp` callback rejects before taking terminal ownership. Once a consented filesystem write starts, it reports its actual outcome, graceful shutdown waits for it, and success reconciles the review then active. | Event | Payload | When | | ---------------------- | ----------------------- | -------------------------------------------------------- | diff --git a/website/src/content/docs/docs/extend/vcs-adapters.md b/website/src/content/docs/docs/extend/vcs-adapters.md index 7a4522245..8313eef57 100644 --- a/website/src/content/docs/docs/extend/vcs-adapters.md +++ b/website/src/content/docs/docs/extend/vcs-adapters.md @@ -32,11 +32,12 @@ The ids Hunk ships with — `git`, `jj`, and `sl` — are reserved. An adapter t A `load` result is patch text plus how to label it. Everything else on it is optional, and each optional field buys one thing: -| Field | What it adds | -| ---------------- | ----------------------------------------------------------------- | -| `untrackedPaths` | files your VCS calls unknown, synthesized into added-file diffs | -| `readFileSource` | exact whole-file contents, for context expansion and highlighting | -| `extraFiles` | files reviewed outside the patch, including skipped placeholders | +| Field | What it adds | +| ----------------------- | ----------------------------------------------------------------- | +| `untrackedPaths` | files your VCS calls unknown, synthesized into added-file diffs | +| `readFileSource` | exact whole-file contents, for context expansion and highlighting | +| `resolveFileSourcePath` | exact filesystem provenance for application location handoff | +| `extraFiles` | files reviewed outside the patch, including skipped placeholders | `untrackedPaths` is the shorthand: list the repo-root-relative paths your VCS reports as unknown and Hunk synthesizes the added-file diffs for you, skipping binaries and files too large to render. Honor `input.options.excludeUntracked` when you do, so `--exclude-untracked` still means what it says. The other two are covered below. @@ -114,12 +115,18 @@ async load(input, ctx) { } return changeType === "deleted" ? null : hgCat(newRev, path); }, + resolveFileSourcePath: ({ path, changeType, side }) => { + if (side !== "new" || changeType === "deleted" || input.range) return null; + return join(ctx.cwd, path); + }, }; } ``` Return `null` for a side that has no content — the old side of an added file, a path the revision never contained — rather than throwing. Return `{ kind: "too-large", maxBytes }` when fetching the source would exceed your resource limit; Hunk shows expansion as unavailable without treating the result as an extension failure. Hunk calls the reader **at most once per file and side** and caches what it resolves, so you do not need your own cache, and it never calls it for a file the diff reports as binary. Leaving `readFileSource` off is fine: Hunk falls back to the content the patch itself carries, which renders the same diff with less context available. +`resolveFileSourcePath` is independent of source reads because binary and skipped files can still have real paths. Return an absolute path only when that exact reviewed side is filesystem-backed. Return `null` for index, revision, stash, patch, merged, absent, and other virtual sides even if a same-named checkout file exists. Hunk uses this provenance for `ctx.workspace.resolveLocation` and never derives historical paths from display names. + ## Files outside the patch `extraFiles` lists files to review that your `patchText` does not contain, in the order they should appear. Each entry is one of two kinds, and Hunk builds the diff model for both — you describe files, you never assemble them.