diff --git a/.changeset/fuzzy-agents-guide.md b/.changeset/fuzzy-agents-guide.md new file mode 100644 index 000000000..54cf4e668 --- /dev/null +++ b/.changeset/fuzzy-agents-guide.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Add custom React/OpenTUI dialog surfaces to the extension API and run Hunk's Agent Skill onboarding as a bundled extension. 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..5e637a04f 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 @@ -235,12 +236,14 @@ modal keys also remain outside the table and therefore outside the event. `ctx.dialogs` is the one place extension code can interrupt the user, so its ordering and settlement live outside React in `src/ui/lib/extensionDialogs.ts` — one FIFO queue per App instance, minting a -per-extension `dialogs` object, normalizing (and sanitizing) extension-authored -text into a request the host draws, and answering by request id so a duplicated +per-extension `dialogs` object, normalizing host-rendered prompts or retaining a +custom component request, and answering by request id so a duplicated Enter cannot spill onto whatever was queued behind. App subscribes with `useSyncExternalStore`, renders the current request through `src/ui/components/chrome/ExtensionDialog.tsx` (confirm reuses `ConfirmDialog`; -select and input are `ModalFrame` surfaces), and unmount calls `shutdown()` so +select and input are `ModalFrame` surfaces; `open` mounts a guarded public +React/OpenTUI component inside exact clamped bounds), and +unmount calls `shutdown()` so every pending and queued dialog resolves its cancel value instead of leaving a handler awaiting forever. Key precedence in `useAppKeyboardShortcuts` places dialogs below Hunk's own app-critical prompts (repo trust, save-on-quit) and @@ -253,8 +256,9 @@ must not be able to impersonate Hunk. The host derives the extension's trusted bundled origin from registry metadata and omits the redundant marker only for Hunk-owned bundled UI. `src/ui/lib/modalGeometry.ts` clamps the frame before extension text is wrapped or windowed, so measurement and rendering use the -same terminal width; body/options yield rows to a pinned mouse-clickable action -footer on short terminals. +same terminal width. Custom components receive the remaining exact rectangle +after required attribution and own layout within it; Hunk retains Escape, +clipboard mediation, queue settlement, and render-failure containment. Lifecycle and bus handlers receive that same attributed dialog queue plus the same guarded live navigation commands use. `App` installs both through the @@ -272,10 +276,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..9e38ecaf5 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -280,9 +280,12 @@ 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 `17`). Branch on it if you want +one file to support several Hunk versions. Version 17 adds custom React/OpenTUI +dialog surfaces; version 16 added 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 +470,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 +589,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 +612,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 @@ -1608,12 +1625,13 @@ your extension. #### Asking the user -`ctx.dialogs` puts a question on screen and waits for the answer. Three shapes, -all promise-returning: +`ctx.dialogs` puts a modal surface on screen and waits for it to settle. Four +shapes, all promise-returning: - `confirm({ title, body?, confirmLabel?, cancelLabel? })` → `true` or `false` - `select({ title, options })` → the chosen string, or `null` - `input({ title, placeholder?, initial? })` → the typed string, or `null` +- `open({ title, width?, height?, component })` → `void` when closed ```ts hunk.registerCommand( @@ -1657,18 +1675,74 @@ hunk.registerCommand({ id: "pick-hunk", title: "Pick a hunk", key: "ctrl+k" }, a }); ``` -Hunk draws the dialog, not you: your text fills the title, body, and choices, -and dialogs from installed extensions carry an `ext ` attribution line -— the same marker `notify` toasts use — so a third-party prompt can never present -itself as Hunk asking. Hunk's own bundled extensions omit that redundant marker. +`open` mounts a React/OpenTUI component in an exact host-owned rectangle, like +`registerPane` inside modal chrome. `width` and `height` request the preferred +component size (defaults `64×12`, maximum `240×100`); Hunk clamps both to the +terminal before passing the resulting dimensions, semantic theme, +`copySupported`, and guarded `actions` to the component. Escape stays +host-owned. Other keys reach the component, and `actions.close()` resolves the +promise. + +```tsx +import { useKeyboard } from "@opentui/react"; +import { matchesKey, type ExtensionDialogProps } from "hunkdiff/extension"; + +const prompt = "Review the current Hunk session. Focus on correctness."; + +function AgentSetupDialog({ actions, copySupported, theme }: ExtensionDialogProps) { + const copy = () => { + actions.notify(actions.copy(prompt) ? "Copied agent prompt" : "Clipboard copy failed"); + }; + useKeyboard((key) => { + if (!copySupported || !matchesKey("c", key)) return; + key.preventDefault(); + key.stopPropagation(); + copy(); + }); + + return ( + + {prompt} + + + {copySupported ? "Copy prompt" : "Copy unavailable"} + + + + ); +} + +hunk.registerCommand({ id: "agent-setup", title: "Agent setup" }, async (ctx) => { + await ctx.dialogs.open({ + title: "Agent setup", + width: 64, + height: 6, + component: AgentSetupDialog, + }); +}); +``` + +`actions.copy(text)` uses Hunk's OSC 52 integration, strips terminal control +sequences, expands tabs to four spaces, and returns whether the renderer accepted +the bounded payload (maximum 16,384 JavaScript string code units). +`actions.notify(message)` shows a short host status message, and +`actions.close()` dismisses the modal. A render failure is contained to the +component and leaves a dismissible fallback. + +Component dialogs are trusted extension code, just like pane components: Hunk +cannot verify that an arbitrary surface visually discloses what it passes to +`actions.copy`. Hunk owns the frame, title, bounds, Escape handling, and an +`ext ` attribution line for installed extensions. Hunk's bundled UI +omits that redundant marker. One dialog is on screen at a time. Concurrent requests queue in call order, -across extensions too, so a second question waits its turn instead of replacing -the first. While a dialog is up it owns the keyboard: Escape cancels (`false`, -or `null`), Enter accepts — the confirm action, the highlighted option, or the -typed text — and review shortcuts stay suppressed underneath. Confirm dialogs -also answer to `y`/`n`, select dialogs to `↑`/`↓`, and every dialog's actions -and rows are clickable. +across extensions too, so a second modal waits its turn instead of replacing +the first. While a dialog is up it owns the keyboard: Escape cancels (`false` +or `null`) or closes a component dialog, Enter accepts the confirm action, +highlighted option, or typed text, and review shortcuts stay suppressed +underneath. Component-dialog keys other than Escape reach the mounted surface. +Confirm dialogs also answer to `y`/`n`, +select dialogs to `↑`/`↓`, and every dialog's actions and rows are clickable. Two things resolve a dialog without the user: the session moving on, and bad arguments. A session reload — the refresh key, a watch-triggered reload, an @@ -1679,6 +1753,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 +1802,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 +1834,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 +1920,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..6cb56abd5 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 `17`) | `hunk.apiVersion` | Registration is only valid while the factory runs — Hunk seals the API object afterwards. @@ -160,11 +160,16 @@ 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` plus `open` for a custom OpenTUI component, + 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`. +- **Dialog components** get exact clamped dimensions, semantic `theme`, clipboard + availability, and guarded `close`/`copy`/`notify` actions. Escape remains host-owned. - **File-view `layout`** gets `file`, `width`, `signal`, `changes`, and a lazy `readDocument(side)`. - **File-view `mode` handlers** get `ctx.file` and `ctx.fileViews`. `onKey`, @@ -214,8 +219,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 +258,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..73bb76fe4 100644 --- a/src/extension-api/index.ts +++ b/src/extension-api/index.ts @@ -95,6 +95,10 @@ export type { ExtensionReviewSnapshotNote, ExtensionReviewSnapshotNoteAnchor, ExtensionConfirmOptions, + ExtensionDialogActions, + ExtensionDialogComponent, + ExtensionDialogOptions, + ExtensionDialogProps, ExtensionDialogs, ExtensionInputOptions, ExtensionSelectOptions, @@ -137,6 +141,7 @@ export type { ExtensionVcsFileChangeType, ExtensionVcsFileSide, ExtensionVcsFileSourceReader, + ExtensionVcsFileSourcePathResolver, ExtensionVcsFileSourceRequest, ExtensionVcsFileSourceResult, ExtensionVcsFileSourceTooLarge, @@ -155,6 +160,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..53acaabd8 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 = 17; 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. * @@ -1621,18 +1631,61 @@ export interface ExtensionInputOptions { initial?: string; } +/** Actions available while an extension-owned dialog component is mounted. */ +export interface ExtensionDialogActions { + /** Close this dialog and resolve its `open` promise. */ + close(): void; + /** + * Copy terminal-safe text through Hunk's OSC 52 integration. + * + * Hunk strips terminal control sequences, expands tabs to four spaces, and + * rejects empty or oversized payloads. Returns false when copying is + * unsupported, refused, or no longer belongs to the mounted dialog. + */ + copy(text: string): boolean; + /** Show one short host status message while this dialog remains current. */ + notify(message: string): void; +} + +/** Everything an extension-owned dialog component receives. */ +export interface ExtensionDialogProps { + /** Exact host-owned component width after terminal clamping. */ + readonly width: number; + /** Exact host-owned component height after terminal clamping and attribution. */ + readonly height: number; + readonly theme: ExtensionPaintTheme; + /** Whether Hunk's renderer currently supports clipboard writes. */ + readonly copySupported: boolean; + readonly actions: ExtensionDialogActions; +} + +/** A React/OpenTUI component mounted inside a host-owned modal frame. */ +export type ExtensionDialogComponent = (props: ExtensionDialogProps) => unknown; + +/** One extension-owned modal surface opened from a command or event handler. */ +export interface ExtensionDialogOptions { + title: string; + /** Preferred component width in terminal cells. Defaults to 64; maximum 240. */ + width?: number; + /** Preferred component height in terminal rows. Defaults to 12; maximum 100. */ + height?: number; + component: ExtensionDialogComponent; +} + /** - * Ask the user questions from a command handler, one modal at a time. + * Present modal interactions from a command handler, one at a time. * - * Every dialog is drawn by Hunk, not by the extension. Dialogs from installed - * extensions carry an attribution line naming their source, so a third-party - * prompt cannot present itself as Hunk asking; Hunk-owned bundled extensions + * Hunk draws every frame and every confirm/select/input surface; `open` mounts + * extension-owned content inside that frame. Dialogs from installed extensions + * carry an attribution line naming their source; Hunk-owned bundled extensions * omit that redundant marker. Only one dialog is on screen at a time: * concurrent requests queue in call order (FIFO), including across extensions, * so a second question waits for the first to be answered rather than replacing it. * - * Escape always cancels, resolving the cancel value (`false`, or `null`). - * Enter accepts: the confirm action, the highlighted option, or the typed text. + * Escape always dismisses, resolving the cancel value (`false`, `null`, or + * `undefined`). Enter accepts: the confirm action, the highlighted option, or + * the typed text. Open component dialogs remain mounted until they call + * `actions.close()` or the user presses Escape. * A session reload — the refresh key, a watch-triggered reload, an agent * command — cancels open and queued dialogs the same way: the review they * asked about is being replaced. A dialog raised while the app is tearing @@ -1640,8 +1693,9 @@ export interface ExtensionInputOptions { * never left hanging. * * Bad arguments are a programming error rather than a user answer, so they - * reject instead of resolving: a missing or blank `title`, or a `select` with - * no options. Because a dialog call is only useful awaited, the rejection + * reject instead of resolving: a missing or blank `title`, a `select` with no + * options, or invalid component-dialog dimensions. Because a dialog + * call is only useful awaited, the rejection * surfaces through the same path as any other handler failure — a warning toast * naming the extension. */ @@ -1652,6 +1706,8 @@ export interface ExtensionDialogs { select(options: ExtensionSelectOptions): Promise; /** Resolves the submitted text, or null on cancel/escape. */ input(options: ExtensionInputOptions): Promise; + /** Mount an extension-owned React/OpenTUI surface inside a host-owned modal. */ + open(options: ExtensionDialogOptions): Promise; } /** One whole-document replacement an extension asks the host to write. */ @@ -1675,6 +1731,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 +1803,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 +1881,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/agentSkill/index.test.ts b/src/extensions/default/ui/agentSkill/index.test.ts new file mode 100644 index 000000000..556b3fa77 --- /dev/null +++ b/src/extensions/default/ui/agentSkill/index.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, mock, test } from "bun:test"; +import type { ExtensionCommandContext } from "hunkdiff/extension"; +import { getBundledUIRegistry } from ".."; +import { AgentSkillDialog, BUNDLED_AGENT_SKILL_COMMAND_FULL_ID } from "."; + +/** Return the agent-skill registration from the process-static bundled UI registry. */ +function getBundledAgentSkillCommand() { + const registered = getBundledUIRegistry().commands.find( + ({ extensionId, command }) => + `${extensionId}.${command.id}` === BUNDLED_AGENT_SKILL_COMMAND_FULL_ID, + ); + if (!registered) throw new Error("Bundled agent skill command is missing."); + return registered; +} + +describe("bundled agent skill extension", () => { + test("registers the shared Hunk command identity without owning its host menu shell", () => { + const registered = getBundledAgentSkillCommand(); + + expect(registered.extensionId).toBe("hunk"); + expect(registered.command).toEqual({ + id: "app.openAgentSkill", + title: "Show setup guidance for reviewing with an agent", + }); + }); + + test("opens its onboarding through the public component dialog", async () => { + const open = mock(async () => {}); + const context = { dialogs: { open } } as unknown as ExtensionCommandContext; + + await getBundledAgentSkillCommand().handler(context); + + expect(open).toHaveBeenCalledWith({ + title: "Agent skill", + width: 80, + height: 9, + component: AgentSkillDialog, + }); + }); +}); diff --git a/src/extensions/default/ui/agentSkill/index.tsx b/src/extensions/default/ui/agentSkill/index.tsx new file mode 100644 index 000000000..2859e8775 --- /dev/null +++ b/src/extensions/default/ui/agentSkill/index.tsx @@ -0,0 +1,136 @@ +import type { MouseEvent as TuiMouseEvent } from "@opentui/core"; +import { useKeyboard } from "@opentui/react"; +import { matchesKey, type ExtensionDialogProps, type ExtensionFactory } from "hunkdiff/extension"; + +export const AGENT_SKILL_COMMAND = "hunk skill path"; +export const AGENT_SKILL_PROMPT_ROWS = [ + "Load the Hunk skill and use it for this review.", + "Run `hunk skill path` to get the skill path.", +] as const; +export const AGENT_SKILL_PROMPT = AGENT_SKILL_PROMPT_ROWS.join(" "); +export const BUNDLED_AGENT_SKILL_COMMAND_ID = "app.openAgentSkill"; +export const BUNDLED_AGENT_SKILL_COMMAND_FULL_ID = `hunk.${BUNDLED_AGENT_SKILL_COMMAND_ID}`; + +const AGENT_SKILL_BODY = "Teach your agent how to review this Hunk session."; +const AGENT_SKILL_DIALOG_WIDTH = 80; +const AGENT_SKILL_DIALOG_HEIGHT = 9; + +/** Wrap Hunk-owned ASCII prose to one component rectangle. */ +function wrapWords(text: string, width: number) { + const safeWidth = Math.max(1, width); + const lines: string[] = []; + let current = ""; + for (const word of text.split(" ")) { + if (word.length > safeWidth) { + if (current) lines.push(current); + for (let offset = 0; offset < word.length; offset += safeWidth) { + lines.push(word.slice(offset, offset + safeWidth)); + } + current = ""; + continue; + } + const next = current ? `${current} ${word}` : word; + if (next.length <= safeWidth) { + current = next; + } else { + lines.push(current); + current = word; + } + } + if (current) lines.push(current); + return lines; +} + +/** Render Agent Skill onboarding through the public custom-dialog contract. */ +export function AgentSkillDialog({ + actions, + copySupported, + height, + theme, + width, +}: ExtensionDialogProps) { + const bodyLines = wrapWords(AGENT_SKILL_BODY, width); + const promptWidth = Math.max(1, width - 4); + const promptLines = AGENT_SKILL_PROMPT_ROWS.flatMap((line) => wrapWords(line, promptWidth)); + const requiredHeight = bodyLines.length + promptLines.length + 6; + const copyExposed = width >= 5 && height >= requiredHeight; + + const copyPrompt = () => { + const copied = actions.copy(AGENT_SKILL_PROMPT); + actions.notify(copied ? "Copied agent skill prompt to clipboard" : "Clipboard copy failed"); + }; + + useKeyboard((key) => { + if (!copySupported || !copyExposed || !matchesKey("c", key)) return; + key.preventDefault(); + key.stopPropagation(); + copyPrompt(); + }); + + return ( + + {bodyLines.map((line, index) => ( + + {line} + + ))} + + + Prompt + + + + {promptLines.map((line, index) => ( + + {line} + + ))} + + + + + { + event.stopPropagation(); + if (copySupported && copyExposed) copyPrompt(); + }} + > + + {copySupported ? " ⧉ Copy prompt " : " Copy unavailable "} + + + + + ); +} + +/** Register Hunk's agent onboarding guidance through the public dialog contract. */ +const registerBundledAgentSkill: ExtensionFactory = (hunk) => { + hunk.registerCommand( + { + id: BUNDLED_AGENT_SKILL_COMMAND_ID, + title: "Show setup guidance for reviewing with an agent", + }, + async (ctx) => { + await ctx.dialogs.open({ + title: "Agent skill", + width: AGENT_SKILL_DIALOG_WIDTH, + height: AGENT_SKILL_DIALOG_HEIGHT, + component: AgentSkillDialog, + }); + }, + ); +}; + +export default registerBundledAgentSkill; 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..f69f76117 100644 --- a/src/extensions/default/ui/index.test.ts +++ b/src/extensions/default/ui/index.test.ts @@ -1,10 +1,18 @@ import { describe, expect, test } from "bun:test"; import { getBundledUIRegistry } from "."; import { paneKey } from "../../apply"; +import { BUNDLED_AGENT_SKILL_COMMAND_FULL_ID } from "./agentSkill"; +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 workflow commands", () => { + 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, BUNDLED_AGENT_SKILL_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..f6616b6a0 100644 --- a/src/extensions/default/ui/index.ts +++ b/src/extensions/default/ui/index.ts @@ -6,30 +6,53 @@ import { type ExtensionLoadIssue, type ExtensionRegistry, } from "../../types"; +import registerBundledEditor, { BUNDLED_EDITOR_COMMAND_FULL_ID } from "./editor"; +import registerBundledAgentSkill, { BUNDLED_AGENT_SKILL_COMMAND_FULL_ID } from "./agentSkill"; 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); + registerBundledAgentSkill(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, + ); + const agentSkillCommandRegistered = registry.commands.some( + ({ extensionId, command }) => + `${extensionId}.${command.id}` === BUNDLED_AGENT_SKILL_COMMAND_FULL_ID, + ); + if ( + issues.length > 0 || + !filesPaneRegistered || + !editorCommandRegistered || + !agentSkillCommandRegistered + ) { + 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/events.test.ts b/src/extensions/events.test.ts index f0ae2bad4..ff0823b15 100644 --- a/src/extensions/events.test.ts +++ b/src/extensions/events.test.ts @@ -195,6 +195,7 @@ describe("extension event dispatch", () => { confirm: async () => false, select: async () => null, input: async () => null, + open: async () => {}, }, events: { emit: () => {} }, }; diff --git a/src/extensions/events.ts b/src/extensions/events.ts index 4d3bfa636..97be5f1eb 100644 --- a/src/extensions/events.ts +++ b/src/extensions/events.ts @@ -343,6 +343,9 @@ function unavailableDialogs(result: ExtensionLoadResult, extensionId: string): E unavailable(); return null; }, + open: async () => { + unavailable(); + }, }; } diff --git a/src/extensions/types.ts b/src/extensions/types.ts index 1a399b57f..cdb4bb4f3 100644 --- a/src/extensions/types.ts +++ b/src/extensions/types.ts @@ -44,6 +44,10 @@ export type { ExtensionContext, ExtensionCustomEventHandler, ExtensionDiffFile, + ExtensionDialogActions, + ExtensionDialogComponent, + ExtensionDialogOptions, + ExtensionDialogProps, ExtensionEventBus, ExtensionEventContext, ExtensionEventHandler, @@ -74,6 +78,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..dea80ef85 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -1,6 +1,7 @@ import type { BoxRenderable, MouseEvent as TuiMouseEvent, + Renderable, ScrollBoxRenderable, } from "@opentui/core"; import { useRenderer, useTerminalDimensions } from "@opentui/react"; @@ -18,7 +19,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 +35,9 @@ import { } from "../extensions/apply"; import { projectExtensionReviewNotes } from "../extensions/reviewSnapshot"; import type { ExtensionNotifyType, ExtensionLoadResult } from "../extensions/types"; +import { getBundledUIRegistry } from "../extensions/default/ui"; +import { BUNDLED_AGENT_SKILL_COMMAND_FULL_ID } from "../extensions/default/ui/agentSkill"; +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 +57,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"; @@ -86,6 +90,7 @@ import { } from "./lib/appCommands"; import { buildAppMenus } from "./lib/appMenus"; import { buildExtensionAppCommands, extensionCommandKeyDefaults } from "./lib/extensionCommands"; +import { normalizeExtensionDialogClipboardText } from "./lib/extensionDialogs"; import type { CurrentLineAlignment } from "./lib/hunkScroll"; import type { LineCursor } from "./lib/lineCursors"; import { useFilePresentationController } from "./fileViews/useFilePresentationController"; @@ -103,7 +108,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"; @@ -111,9 +115,6 @@ type FocusArea = "files" | "filter" | "note"; const FAST_CODE_HORIZONTAL_SCROLL_COLUMNS = 8; -const LazyAgentSkillDialog = lazy(async () => ({ - default: (await import("./components/chrome/AgentSkillDialog")).AgentSkillDialog, -})); const LazyHelpDialog = lazy(async () => ({ default: (await import("./components/chrome/HelpDialog")).HelpDialog, })); @@ -204,6 +205,17 @@ export function App({ const wrapToggleScrollTopRef = useRef(null); const layoutToggleScrollTopRef = useRef(null); const cancelCopySelectionRef = useRef<(() => void) | null>(null); + const activeReviewGenerationRef = useRef(bootstrap); + const renderedReviewGenerationRef = useRef(bootstrap); + renderedReviewGenerationRef.current = bootstrap; + const bundledDialogsLiveRef = useRef(false); + useLayoutEffect(() => { + activeReviewGenerationRef.current = bootstrap; + bundledDialogsLiveRef.current = true; + return () => { + bundledDialogsLiveRef.current = false; + }; + }, [bootstrap]); const [layoutToggleRequestId, setLayoutToggleRequestId] = useState(0); const [scrollEdgeRequest, setScrollEdgeRequest] = useState<{ id: number; @@ -223,7 +235,6 @@ export function App({ const [showHunkHeaders, setShowHunkHeaders] = useState(bootstrap.initialShowHunkHeaders ?? true); const [showMenuBar, setShowMenuBar] = useState(bootstrap.initialShowMenuBar ?? true); const [showHelp, setShowHelp] = useState(false); - const [showAgentSkill, setShowAgentSkill] = useState(false); const [focusArea, setFocusArea] = useState("files"); const { text: sessionNoticeText, show: showSessionNotice } = useTimedNotice(4_000); const extensions = bootstrap.extensions as ExtensionLoadResult | undefined; @@ -481,8 +492,13 @@ export function App({ const { accept: acceptExtensionDialog, + acceptRequest: acceptExtensionDialogRequest, cancel: cancelExtensionDialog, + cancelRequest: cancelExtensionDialogRequest, + cancelAll: cancelAllExtensionDialogs, createDialogs: createQueuedExtensionDialogs, + getCurrentRequest: getCurrentExtensionDialogRequest, + isCurrentRequestLive: isCurrentExtensionDialogRequestLive, inputValue: extensionDialogInputValue, moveSelection: moveExtensionDialogSelection, pickOption: setExtensionDialogSelectedIndex, @@ -491,19 +507,40 @@ 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) => { const lease = createReviewCapabilityLease(); - const bundled = extensions?.registry.extensions.some( - (metadata) => metadata.id === extensionId && metadata.origin === "bundled", - ); + const bundled = [ + ...getBundledUIRegistry().extensions, + ...(extensions?.registry.extensions ?? []), + ].some((metadata) => metadata.id === extensionId && metadata.origin === "bundled"); return createQueuedExtensionDialogs(extensionId, { - isLive: lease.isLive, + // Bundled registrations are process-static rather than owned by the + // reloadable user-extension registry, but their review-scoped controls + // still retire when the mounted review changes. + isLive: () => + renderedReviewGenerationRef.current === bootstrap && + !extensionAppController.isAppActive() && + (bundled + ? bundledDialogsLiveRef.current && activeReviewGenerationRef.current === bootstrap + : lease.isLive()), showAttribution: !bundled, }); }, - [createQueuedExtensionDialogs, createReviewCapabilityLease, extensions], + [ + bootstrap, + createQueuedExtensionDialogs, + createReviewCapabilityLease, + extensionAppController, + extensions, + ], ); const extensionWorkspaceController = useExtensionWorkspaceControls({ @@ -511,12 +548,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 +568,7 @@ export function App({ createKeyboardModeControls, createLineHighlightControls, createNavigation: createExtensionNavigation, + createOpenInApp: extensionAppController.createOpenInApp, createPaneControls, createReviewControls: createExtensionReviewControls, createWorkspaceControls: extensionWorkspaceController.createWorkspaceControls, @@ -538,6 +576,29 @@ 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; + }, []); + + const bundledAgentSkillCommand = useMemo(() => { + const command = resolveExtensionCommands(getBundledUIRegistry()).commands.find( + ({ extensionId, command: registration }) => + `${extensionId}.${registration.id}` === BUNDLED_AGENT_SKILL_COMMAND_FULL_ID, + ); + if (!command) throw new Error("Bundled agent skill 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,59 +945,105 @@ 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(); + /** Delegate the shared host command shell to the bundled agent skill extension. */ + const openAgentSkill = useCallback(() => { + runExtensionCommand(bundledAgentSkillCommand); + }, [bundledAgentSkillCommand, runExtensionCommand]); + + const extensionDialogCopySupported = + (renderer.isOsc52Supported?.() ?? false) && typeof renderer.copyToClipboardOSC52 === "function"; + const extensionDialogActive = extensionDialog !== null; + const extensionOpenDialogActive = extensionDialog?.kind === "open"; + const extensionOpenDialogFocusLeaseRef = useRef<{ + active: boolean; + previous: Renderable | null; + }>({ active: false, previous: null }); + if (extensionOpenDialogActive && !extensionOpenDialogFocusLeaseRef.current.active) { + // Capture before the custom tree commits and takes focus. OpenTUI exposes + // one process-wide focus owner, so restoring this exact renderable also + // preserves imperative review-scroll focus outside pager mode. + extensionOpenDialogFocusLeaseRef.current = { + active: true, + previous: renderer.currentFocusedRenderable, + }; + } + useLayoutEffect(() => { + const lease = extensionOpenDialogFocusLeaseRef.current; + if (!extensionDialog && lease.active) { + extensionOpenDialogFocusLeaseRef.current = { active: false, previous: null }; + // `focus()` is inert when teardown already destroyed the old owner. + lease.previous?.focus(); } - }, [ - activeLineCursor, - bootstrap.changeset.sourceLabel, - bootstrap.input.kind, - canRefreshCurrentInput, - renderer, - review.selectedHunk, - selectedFile, - showSessionNotice, - triggerRefreshCurrentInput, - ]); + }, [extensionDialog]); + + /** Copy text only for the custom dialog that still owns the mounted component. */ + const copyExtensionDialogText = useCallback( + (requestId: number, text: string) => { + const current = getCurrentExtensionDialogRequest(); + if ( + current?.kind !== "open" || + current.id !== requestId || + !isCurrentExtensionDialogRequestLive(requestId) || + !(renderer.isOsc52Supported?.() ?? false) || + typeof renderer.copyToClipboardOSC52 !== "function" + ) { + return false; + } - /** Close the agent skill setup overlay. */ - const closeAgentSkill = useCallback(() => { - setShowAgentSkill(false); - }, []); + const normalized = normalizeExtensionDialogClipboardText(text); + return normalized !== null && renderer.copyToClipboardOSC52(normalized); + }, + [getCurrentExtensionDialogRequest, isCurrentExtensionDialogRequestLive, renderer], + ); - /** Open the agent skill setup overlay. */ - const openAgentSkill = useCallback(() => { - setShowAgentSkill(true); - }, []); + /** Show status only for the custom dialog that still owns the mounted component. */ + const notifyExtensionDialog = useCallback( + (requestId: number, message: string) => { + const current = getCurrentExtensionDialogRequest(); + if ( + current?.kind !== "open" || + current.id !== requestId || + !isCurrentExtensionDialogRequestLive(requestId) + ) { + return; + } + const safeMessage = sanitizeTerminalLine(message).trim(); + if (!safeMessage) return; + showTransientNotice( + current.showAttribution ? `Extension ${current.extensionId}: ${safeMessage}` : safeMessage, + ); + }, + [getCurrentExtensionDialogRequest, isCurrentExtensionDialogRequestLive, showTransientNotice], + ); - /** Copy the agent skill prompt through the terminal clipboard integration. */ - const copyAgentSkillPrompt = useCallback(async () => { - const { AGENT_SKILL_PROMPT } = await import("./components/chrome/AgentSkillDialog"); - if (renderer.isOsc52Supported?.() && typeof renderer.copyToClipboardOSC52 === "function") { - renderer.copyToClipboardOSC52(AGENT_SKILL_PROMPT); - showTransientNotice("Copied agent skill prompt to clipboard"); - return; - } + /** Close only the custom dialog whose mounted component owns this action. */ + const closeExtensionDialogComponent = useCallback( + (requestId: number) => { + if (isCurrentExtensionDialogRequestLive(requestId)) { + cancelExtensionDialogRequest(requestId); + } + }, + [cancelExtensionDialogRequest, isCurrentExtensionDialogRequestLive], + ); - showTransientNotice("Clipboard copy unsupported in this terminal (enable OSC 52)"); - }, [renderer, showTransientNotice]); + /** Contain one custom component failure and leave its host frame dismissible. */ + const reportExtensionDialogRenderFailure = useCallback( + (requestId: number, error: unknown) => { + const current = getCurrentExtensionDialogRequest(); + if ( + current?.kind !== "open" || + current.id !== requestId || + !isCurrentExtensionDialogRequestLive(requestId) + ) { + return; + } + const detail = error instanceof Error ? error.message || error.name : String(error); + showSessionNotice( + `Extension ${current.extensionId} dialog failed rendering • ${sanitizeTerminalLine(detail)}`, + ); + }, + [getCurrentExtensionDialogRequest, isCurrentExtensionDialogRequestLive, showSessionNotice], + ); /** Toggle the modal keyboard help overlay. */ const toggleHelp = useCallback(() => { @@ -1121,7 +1228,6 @@ export function App({ useAppKeyboardShortcuts({ activeMenuId, activateCurrentMenuItem, - closeAgentSkill, closeHelp, closeMenu, acceptThemeSelector, @@ -1130,7 +1236,7 @@ export function App({ closeExtensionTrustPrompt, commands: appCommands, denyRepoExtensions, - extensionDialog, + getExtensionDialog: getCurrentExtensionDialogRequest, acceptExtensionDialog, cancelExtensionDialog, moveExtensionDialogSelection, @@ -1152,7 +1258,6 @@ export function App({ neverAskToSaveViewPreferencesAndQuit, closeSaveConfigPrompt, saveDraftNote, - showAgentSkill, showHelp, switchMenu, toggleFocusArea, @@ -1338,7 +1443,8 @@ export function App({ selectedHunkIndex={selectedHunkIndex} scrollToNote={review.scrollToNote} draftNote={review.draftNote} - draftNoteFocused={focusArea === "note"} + draftNoteFocused={focusArea === "note" && !extensionDialogActive} + keyboardFocusBlocked={extensionDialogActive} separatorWidth={diffSeparatorWidth} showAgentNotes={showAgentNotes} showLineNumbers={showLineNumbers} @@ -1401,7 +1507,7 @@ export function App({ {statusBarVisible ? ( ) : null} - {showAgentSkill ? ( - - - - ) : null} - {showHelp ? ( ) : null} 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..a7330184d 100644 --- a/src/ui/AppHost.extension-dialogs.test.tsx +++ b/src/ui/AppHost.extension-dialogs.test.tsx @@ -3,6 +3,7 @@ import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, test } from "bun:test"; +import { KeyEvent, type ParsedKey } from "@opentui/core"; import { testRender } from "@opentui/react/test-utils"; import { act } from "react"; import { removeTestDirectory } from "../../test/helpers/filesystem"; @@ -77,6 +78,23 @@ async function flush(setup: Awaited>) { }); } +/** Publish one key synchronously, used for same-input-flush coverage. */ +function testKeyEvent(fields: Partial) { + return new KeyEvent({ + name: "", + sequence: "", + raw: "", + ctrl: false, + meta: false, + option: false, + shift: false, + number: false, + eventType: "press", + source: "raw", + ...fields, + }); +} + /** Render frames until a condition holds, and fail loudly when it never does. */ async function flushUntil( setup: Awaited>, @@ -204,6 +222,31 @@ function writeDialogFixture(extPath: string, logPath: string, askSource: string) writeFileSync( extPath, `import { appendFileSync } from "node:fs";\n` + + `import { createElement, useState } from "react";\n` + + `import { useKeyboard } from "@opentui/react";\n` + + `import { matchesKey } from "hunkdiff/extension";\n` + + `function createCopyDialog(body, label, text) {\n` + + ` return function CopyDialog({ actions, copySupported, height, theme, width }) {\n` + + ` const copyExposed = width >= 5 && height >= 4;\n` + + ` const copy = () => {\n` + + ` const copied = actions.copy(text);\n` + + ` actions.notify(copied ? "Copied custom content to clipboard" : "Clipboard copy failed");\n` + + ` };\n` + + ` useKeyboard((key) => {\n` + + ` if (!copySupported || !copyExposed || !matchesKey("c", key)) return;\n` + + ` key.preventDefault();\n` + + ` key.stopPropagation();\n` + + ` copy();\n` + + ` });\n` + + ` return createElement("box", { style: { width, height, flexDirection: "column", overflow: "hidden" } },\n` + + ` createElement("box", { style: { width: "100%", height: 1 } }, createElement("text", { fg: theme.text }, body)),\n` + + ` createElement("box", { style: { width: "100%", height: 1 } }, createElement("text", { fg: theme.badgeNeutral }, label)),\n` + + ` createElement("box", { style: { width: "100%", height: 1 } }, createElement("text", { fg: theme.text }, text)),\n` + + ` createElement("box", { style: { width: "100%", height: 1, backgroundColor: copySupported ? theme.accentMuted : theme.panelAlt }, onMouseUp: (event) => { event.stopPropagation(); if (copySupported && copyExposed) copy(); } },\n` + + ` createElement("text", { fg: copySupported ? theme.text : theme.muted }, copySupported ? " ⧉ Copy " + label.toLowerCase() + " " : " Copy unavailable "))\n` + + ` );\n` + + ` };\n` + + `}\n` + `export default function (hunk) {\n` + ` hunk.registerCommand({ id: "ask", title: "Ask", key: "y" }, async (ctx) => {\n` + ` const answer = await ${askSource};\n` + @@ -214,6 +257,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. @@ -275,6 +360,628 @@ describe("extension dialogs", () => { }); }); + test("owns later keys when a command opens and cancels a dialog in one input flush", async () => { + const repo = createTestRepo("hunk-ext-dialog-same-flush-"); + const extDir = createTempDir("hunk-ext-dialog-same-flush-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture(extPath, logPath, `ctx.dialogs.confirm({ title: "Same flush?" })`); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup, quits) => { + await act(async () => { + setup.renderer.keyInput.emit( + "keypress", + testKeyEvent({ name: "y", sequence: "y", raw: "y" }), + ); + setup.renderer.keyInput.emit( + "keypress", + testKeyEvent({ name: "q", sequence: "q", raw: "q" }), + ); + setup.renderer.keyInput.emit( + "keypress", + testKeyEvent({ name: "escape", sequence: "\u001b", raw: "\u001b" }), + ); + }); + + await flushUntil( + setup, + () => readProbeLog(logPath).includes("answer false"), + "the same-flush Escape to cancel the queued dialog", + ); + expect(quits()).toBe(0); + expect(setup.captureCharFrame()).not.toContain("Same flush?"); + }); + }); + + test("moves and accepts a newly opened select dialog in one input flush", async () => { + const repo = createTestRepo("hunk-ext-dialog-select-same-flush-"); + const extDir = createTempDir("hunk-ext-dialog-select-same-flush-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.select({ title: "Same flush choice", options: ["one", "two"] })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + await act(async () => { + setup.renderer.keyInput.emit( + "keypress", + testKeyEvent({ name: "y", sequence: "y", raw: "y" }), + ); + setup.renderer.keyInput.emit("keypress", testKeyEvent({ name: "down" })); + setup.renderer.keyInput.emit("keypress", testKeyEvent({ name: "return" })); + }); + + await flushUntil( + setup, + () => readProbeLog(logPath).includes("answer two"), + "the same-flush selection to resolve", + ); + expect(setup.captureCharFrame()).not.toContain("Same flush choice"); + }); + }); + + test("a component dialog renders an OpenTUI surface and closes only on escape", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-"); + const extDir = createTempDir("hunk-ext-dialog-open-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.open({ title: "Agent setup", width: 46, height: 6, component: createCopyDialog("Teach your agent how to review this Hunk session.", "Prompt", "Load the Hunk skill and use it for this review. Run hunk skill path to get the skill path.") })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost( + bootstrap, + async (setup) => { + const copied: string[] = []; + setup.renderer.isOsc52Supported = () => true; + setup.renderer.copyToClipboardOSC52 = (text: string) => { + copied.push(text); + return true; + }; + + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Agent setup"), + "the component dialog to open", + ); + + const frame = setup.captureCharFrame(); + expect(frame).toContain("Teach your agent"); + expect(frame).toContain("Prompt"); + expect(frame).toContain("Load the Hunk skill"); + expect(frame).toContain("ext ext"); + + await act(async () => { + await setup.mockInput.pressEnter(); + await setup.mockInput.typeText("c"); + }); + await flush(setup); + expect(setup.captureCharFrame()).toContain("Agent setup"); + expect(copied).toEqual([ + "Load the Hunk skill and use it for this review. Run hunk skill path to get the skill path.", + ]); + + const copyAction = findTextPosition(setup.captureCharFrame(), "Copy prompt"); + expect(copyAction).not.toBeNull(); + await act(async () => { + await setup.mockMouse.click(copyAction!.x, copyAction!.y); + }); + expect(copied).toHaveLength(2); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("answer undefined"), + "the component-dialog handler to finish", + ); + }, + undefined, + { width: 50, height: 20 }, + ); + }); + + test("preserves focus owned by an input inside a component dialog", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-input-"); + const extDir = createTempDir("hunk-ext-dialog-open-input-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.open({ title: "Custom input", width: 36, height: 4, component: function InputDialog({ theme, width }) { const [value, setValue] = useState(""); return createElement("input", { focused: true, width, value, onInput: setValue, textColor: theme.text }); } })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup, quits) => { + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Custom input"), + "the custom input dialog to open", + ); + + await act(async () => { + await setup.mockInput.typeText("quick-fix"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("quick-fix"), + "typing to reach the extension-owned input", + ); + expect(quits()).toBe(0); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + await flushUntil( + setup, + () => + readProbeLog(logPath).includes("answer undefined") && + !setup.captureCharFrame().includes("Custom input"), + "the custom input dialog to settle and close", + ); + }); + }); + + test("keeps a promoted input focused after a component dialog closes", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-input-promotion-"); + const extDir = createTempDir("hunk-ext-dialog-open-input-promotion-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeFileSync( + extPath, + `import { appendFileSync } from "node:fs";\n` + + `import { createElement } from "react";\n` + + `import { useKeyboard } from "@opentui/react";\n` + + `import { matchesKey } from "hunkdiff/extension";\n` + + `function FirstDialog({ actions, theme }) {\n` + + ` useKeyboard((key) => { if (matchesKey("x", key)) actions.close(); });\n` + + ` return createElement("text", { fg: theme.text }, "Press x for input");\n` + + `}\n` + + `export default function (hunk) {\n` + + ` hunk.registerCommand({ id: "ask", title: "Ask", key: "y" }, async (ctx) => {\n` + + ` const opened = ctx.dialogs.open({ title: "First component", component: FirstDialog });\n` + + ` const typed = ctx.dialogs.input({ title: "Promoted input" });\n` + + ` const results = await Promise.all([opened, typed]);\n` + + ` appendFileSync(${JSON.stringify(logPath)}, "answer " + String(results[1]) + "\\n");\n` + + ` });\n` + + `}\n`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + bootstrap.input.options.pager = true; + await withAppHost(bootstrap, async (setup) => { + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Press x for input"), + "the first component dialog to open", + ); + + await act(async () => { + await setup.mockInput.typeText("x"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Promoted input"), + "the queued input dialog to be promoted", + ); + expect(setup.renderer.currentFocusedRenderable?.constructor.name).toBe("InputRenderable"); + + await act(async () => { + await setup.mockInput.typeText("quick-fix"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("quick-fix"), + "typing to remain with the promoted input", + ); + await act(async () => { + await setup.mockInput.pressEnter(); + }); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("answer quick-fix"), + "the promoted input to resolve its typed value", + ); + }); + }); + + test("clips a custom component to the bounded rectangle beneath attribution", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-short-"); + const extDir = createTempDir("hunk-ext-dialog-open-short-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.open({ title: "Agent setup", width: 46, height: 6, component: createCopyDialog("Teach your agent how to review this Hunk session.", "Prompt", "Load the Hunk skill and use it for this review. Run hunk skill path to get the skill path.") })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost( + bootstrap, + async (setup) => { + const copied: string[] = []; + setup.renderer.isOsc52Supported = () => true; + setup.renderer.copyToClipboardOSC52 = (text: string) => { + copied.push(text); + return true; + }; + + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Agent setup"), + "the constrained component dialog to open", + ); + + const frame = setup.captureCharFrame(); + expect(frame).toContain("ext ext"); + expect(frame).not.toContain("Copy prompt"); + + await act(async () => { + await setup.mockInput.typeText("c"); + }); + await flush(setup); + expect(copied).toEqual([]); + expect(setup.captureCharFrame()).toContain("Agent setup"); + }, + undefined, + { width: 50, height: 12 }, + ); + }); + + test("an unavailable component copy action is visible but inert", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-unavailable-"); + const extDir = createTempDir("hunk-ext-dialog-open-unavailable-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.open({ title: "Copy setup", width: 46, height: 6, component: createCopyDialog("Copy this text.", "Prompt", "copy me") })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + const copied: string[] = []; + setup.renderer.isOsc52Supported = () => false; + setup.renderer.copyToClipboardOSC52 = (text: string) => { + copied.push(text); + return true; + }; + + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Copy unavailable"), + "the unavailable copy state to render", + ); + + const unavailable = findTextPosition(setup.captureCharFrame(), "Copy unavailable"); + expect(unavailable).not.toBeNull(); + await act(async () => { + await setup.mockInput.typeText("c"); + await setup.mockMouse.click(unavailable!.x, unavailable!.y); + }); + await flush(setup); + + expect(copied).toEqual([]); + expect(setup.captureCharFrame()).toContain("Copy setup"); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("answer undefined"), + "the unavailable-copy component dialog to close", + ); + }); + }); + + test("reports a clipboard write rejected by the renderer as a failure", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-copy-failure-"); + const extDir = createTempDir("hunk-ext-dialog-open-copy-failure-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.open({ title: "Copy setup", width: 46, height: 6, component: createCopyDialog("Copy this text.", "Prompt", "copy me") })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + let attempts = 0; + setup.renderer.isOsc52Supported = () => true; + setup.renderer.copyToClipboardOSC52 = () => { + attempts += 1; + return false; + }; + + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Copy prompt"), + "the copy action to render", + ); + + await act(async () => { + await setup.mockInput.typeText("c"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Clipboard copy failed"), + "the copy failure notice", + ); + + expect(attempts).toBe(1); + expect(setup.captureCharFrame()).not.toContain("Copied custom content to clipboard"); + }); + }); + + test("contains a custom component render failure inside its dismissible frame", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-render-failure-"); + const extDir = createTempDir("hunk-ext-dialog-open-render-failure-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.open({ title: "Broken surface", width: 30, height: 4, component: ({ actions }) => { const retained = actions; setTimeout(() => { appendFileSync(${JSON.stringify(logPath)}, "failed-copy " + String(retained.copy("stale")) + "\\n"); retained.notify("stale failure notice"); retained.close(); appendFileSync(${JSON.stringify(logPath)}, "failed-actions-called\\n"); }, 10); throw new Error("surface exploded"); } })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + const copied: string[] = []; + setup.renderer.isOsc52Supported = () => true; + setup.renderer.copyToClipboardOSC52 = (text: string) => { + copied.push(text); + return true; + }; + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Dialog unavailable"), + "the custom-dialog fallback to render", + ); + + const failed = setup.captureCharFrame(); + expect(failed).toContain("Broken surface"); + expect(failed).toContain("surface exploded"); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("failed-actions-called"), + "the retained failed-component actions to run", + ); + expect(readProbeLog(logPath)).toContain("failed-copy false"); + expect(copied).toEqual([]); + expect(setup.captureCharFrame()).toContain("Dialog unavailable"); + expect(setup.captureCharFrame()).not.toContain("stale failure notice"); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("answer undefined"), + "the failed component dialog to close", + ); + }); + }); + + test("keeps a custom component mounted through a zero-row terminal allocation", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-zero-height-"); + const extDir = createTempDir("hunk-ext-dialog-open-zero-height-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.open({ title: "Resize surface", width: 30, height: 4, component: function StatefulDialog({ height, theme }) { const [value, setValue] = useState("initial"); useKeyboard((key) => { if (matchesKey("x", key)) setValue("preserved"); }); return createElement("text", { fg: theme.text }, value + " at " + height); } })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("initial at 4"), + "the stateful component to open", + ); + + await act(async () => { + await setup.mockInput.typeText("x"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("preserved at 4"), + "the stateful component to update before shrinking", + ); + + await act(async () => { + await setup.resize(80, 8); + }); + await flush(setup); + expect(setup.captureCharFrame()).not.toContain("preserved at"); + + await act(async () => { + await setup.resize(80, 20); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("preserved at 4"), + "the zero-row component state to survive the resize", + ); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("answer undefined"), + "the resized component dialog to close", + ); + }); + }); + + test("routes component keys without exposing the review and honors guarded close", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-close-"); + const extDir = createTempDir("hunk-ext-dialog-open-close-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeDialogFixture( + extPath, + logPath, + `ctx.dialogs.open({ title: "Component keys", width: 30, height: 4, component: function KeyDialog({ actions, theme }) { useKeyboard((key) => { if (matchesKey("x", key)) actions.close(); }); return createElement("text", { fg: theme.text }, "Press x to close"); } })`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Press x to close"), + "the keyed component dialog to open", + ); + + await act(async () => { + await setup.mockInput.typeText("x"); + }); + await flushUntil( + setup, + () => + readProbeLog(logPath).includes("answer undefined") && + !setup.captureCharFrame().includes("Component keys"), + "the component action to settle and close its dialog", + ); + }); + }); + + test("remounts reused components and retires actions when the queue advances", async () => { + const repo = createTestRepo("hunk-ext-dialog-open-lease-"); + const extDir = createTempDir("hunk-ext-dialog-open-lease-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeFileSync( + extPath, + `import { appendFileSync } from "node:fs";\n` + + `import { createElement, useState } from "react";\n` + + `import { useKeyboard } from "@opentui/react";\n` + + `import { matchesKey } from "hunkdiff/extension";\n` + + `let mounts = 0;\n` + + `let firstActions;\n` + + `function SharedDialog({ actions, theme }) {\n` + + ` const [mount] = useState(() => ++mounts);\n` + + ` if (mount === 1) firstActions = actions;\n` + + ` useKeyboard((key) => {\n` + + ` if (matchesKey("x", key)) actions.close();\n` + + ` if (matchesKey("z", key) && firstActions) {\n` + + ` appendFileSync(${JSON.stringify(logPath)}, "stale-copy " + String(firstActions.copy("stale")) + "\\n");\n` + + ` firstActions.notify("stale notification");\n` + + ` firstActions.close();\n` + + ` }\n` + + ` });\n` + + ` return createElement("text", { fg: theme.text }, "mount " + mount);\n` + + `}\n` + + `export default function (hunk) {\n` + + ` hunk.registerCommand({ id: "ask", title: "Ask", key: "y" }, async (ctx) => {\n` + + ` await Promise.all([\n` + + ` ctx.dialogs.open({ title: "First surface", component: SharedDialog }),\n` + + ` ctx.dialogs.open({ title: "Second surface", component: SharedDialog }),\n` + + ` ]);\n` + + ` appendFileSync(${JSON.stringify(logPath)}, "settled\\n");\n` + + ` });\n` + + `}\n`, + ); + + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost(bootstrap, async (setup) => { + const copied: string[] = []; + setup.renderer.isOsc52Supported = () => true; + setup.renderer.copyToClipboardOSC52 = (text: string) => { + copied.push(text); + return true; + }; + + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("mount 1"), + "the first reused component to mount", + ); + + await act(async () => { + await setup.mockInput.typeText("x"); + }); + await flushUntil( + setup, + () => { + const frame = setup.captureCharFrame(); + return frame.includes("Second surface") && frame.includes("mount 2"); + }, + "the reused component to remount for the promoted request", + ); + + await act(async () => { + await setup.mockInput.typeText("z"); + }); + await flushUntil( + setup, + () => readProbeLog(logPath).includes("stale-copy false"), + "the retired copy action to report that it is inert", + ); + const promotedFrame = setup.captureCharFrame(); + expect(promotedFrame).toContain("Second surface"); + expect(promotedFrame).not.toContain("stale notification"); + expect(copied).toEqual([]); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + await flushUntil( + setup, + () => + readProbeLog(logPath).includes("settled") && + !setup.captureCharFrame().includes("Second surface"), + "the promoted component dialog to settle and close", + ); + }); + }); + test("keeps confirm actions visible when wrapped prose exceeds a short terminal", async () => { const repo = createTestRepo("hunk-ext-dialog-short-confirm-"); const extDir = createTempDir("hunk-ext-dialog-short-confirm-ext-"); @@ -528,6 +1235,68 @@ describe("extension dialogs", () => { ); }); + test("retires component actions before a soft-reload layout cleanup", async () => { + const repo = createTestRepo("hunk-ext-dialog-reload-action-lease-"); + const extDir = createTempDir("hunk-ext-dialog-reload-action-lease-ext-"); + const logPath = join(extDir, "probe.log"); + const extPath = join(extDir, "ext.ts"); + writeFileSync( + extPath, + `import { appendFileSync } from "node:fs";\n` + + `import { createElement, useLayoutEffect } from "react";\n` + + `function CleanupDialog({ actions, theme }) {\n` + + ` useLayoutEffect(() => () => {\n` + + ` appendFileSync(${JSON.stringify(logPath)}, "cleanup-copy " + String(actions.copy("stale")) + "\\n");\n` + + ` actions.notify("stale cleanup notice");\n` + + ` actions.close();\n` + + ` }, []);\n` + + ` return createElement("text", { fg: theme.text }, "Reload cleanup probe");\n` + + `}\n` + + `export default function (hunk) {\n` + + ` hunk.registerCommand({ id: "ask", title: "Ask", key: "y" }, async (ctx) => {\n` + + ` await ctx.dialogs.open({ title: "Reload action lease", component: CleanupDialog });\n` + + ` appendFileSync(${JSON.stringify(logPath)}, "settled\\n");\n` + + ` });\n` + + `}\n`, + ); + + const broker = createTestBrokerClient(); + const bootstrap = await launchWithExtension(repo, extPath); + await withAppHost( + bootstrap, + async (setup) => { + const copied: string[] = []; + setup.renderer.isOsc52Supported = () => true; + setup.renderer.copyToClipboardOSC52 = (text: string) => { + copied.push(text); + return true; + }; + await act(async () => { + await setup.mockInput.typeText("y"); + }); + await flushUntil( + setup, + () => setup.captureCharFrame().includes("Reload cleanup probe"), + "the cleanup-probe component dialog to open", + ); + + await broker.reload({ kind: "vcs", staged: false, options: {} }); + await flushUntil( + setup, + () => { + const events = readProbeLog(logPath); + return events.includes("cleanup-copy false") && events.includes("settled"); + }, + "the stale cleanup actions to retire before reload teardown", + ); + + expect(copied).toEqual([]); + expect(setup.captureCharFrame()).not.toContain("stale cleanup notice"); + }, + broker.client, + ); + }); + test("keeps a dialog opened by the replacement generation's reload lifecycle", async () => { const repo = createTestRepo("hunk-ext-dialog-reload-lifecycle-"); const extDir = createTempDir("hunk-ext-dialog-reload-lifecycle-ext-"); diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index e7c707496..4b5f3862d 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -18,7 +18,7 @@ import { createTestVcsAppBootstrap } from "../../test/helpers/app-bootstrap"; import { capturedTestColorToHex } from "../../test/helpers/test-color-helpers"; import { createTestDiffFile as buildTestDiffFile, lines } from "../../test/helpers/diff-helpers"; import { createEmptyExtensionLoadResult } from "../extensions/types"; -import { AGENT_SKILL_COMMAND, AGENT_SKILL_PROMPT } from "./components/chrome/AgentSkillDialog"; +import { AGENT_SKILL_COMMAND, AGENT_SKILL_PROMPT } from "../extensions/default/ui/agentSkill"; import { App } from "./App"; import { availableThemes, resolveTheme } from "./themes"; @@ -1932,6 +1932,12 @@ describe("App interactions", () => { width: 120, height: 24, }); + const copied: string[] = []; + setup.renderer.isOsc52Supported = () => true; + setup.renderer.copyToClipboardOSC52 = (text: string) => { + copied.push(text); + return true; + }; try { await flush(setup); @@ -1975,6 +1981,17 @@ describe("App interactions", () => { expect(frame).toContain(AGENT_SKILL_COMMAND); expect(frame).toContain("Copy"); + await act(async () => { + await setup.mockInput.typeText("c"); + }); + frame = await waitForFrame( + setup, + (currentFrame) => currentFrame.includes("Copied agent skill prompt to clipboard"), + 12, + ); + expect(copied).toEqual([AGENT_SKILL_PROMPT]); + expect(frame).toContain("Copied agent skill prompt to clipboard"); + await act(async () => { await setup.mockInput.pressEscape(); }); diff --git a/src/ui/AppHost.key-routing.test.tsx b/src/ui/AppHost.key-routing.test.tsx index 3cc571e23..9a9e4a707 100644 --- a/src/ui/AppHost.key-routing.test.tsx +++ b/src/ui/AppHost.key-routing.test.tsx @@ -278,4 +278,72 @@ describe("UI key routing with a focused scroll box", () => { rmSync(root, { recursive: true, force: true }); } }); + + test("an open component dialog isolates unhandled keys from the focused review", async () => { + const root = mkdtempSync(join(tmpdir(), "hunk-key-routing-dialog-")); + const extension = join(root, "custom-dialog"); + mkdirSync(extension, { recursive: true }); + writeFileSync( + join(extension, "package.json"), + JSON.stringify({ + name: "custom-dialog", + private: true, + hunk: { extensions: ["./index.ts"] }, + }), + ); + writeFileSync( + join(extension, "index.ts"), + `import { createElement } from "react"; +export default function (hunk) { + hunk.registerCommand({ id: "open", title: "Open", key: "y" }, (ctx) => + ctx.dialogs.open({ + title: "Focused custom surface", + component: ({ theme }) => createElement("text", { fg: theme.text }, "Unhandled keys stay here"), + }), + ); +} +`, + ); + const extensions = await loadStartupExtensions({ + cliExtensionPaths: [extension], + cwd: root, + env: { XDG_CONFIG_HOME: root } as NodeJS.ProcessEnv, + extensions: { enabled: true, extensionConfigs: {}, paths: [], repoPaths: [] }, + }); + const bootstrap = createScrollableBootstrap(); + bootstrap.extensions = extensions; + const setup = await testRender( {}} />, { + width: 120, + height: 24, + }); + + try { + await waitForFrame(setup, (frame) => frame.includes("big.ts"), 12); + const scrollBox = findReviewScrollBox(setup.renderer.root); + if (!scrollBox) { + throw new Error("No scrollable review scroll box found in the rendered app."); + } + await act(async () => { + scrollBox.focus(); + await setup.mockInput.typeText("y"); + }); + await waitForFrame(setup, (frame) => frame.includes("Focused custom surface"), 12); + const scrollTopBefore = scrollBox.scrollTop; + + await act(async () => setup.mockInput.typeText("j")); + await flush(setup); + + expect(scrollBox.scrollTop).toBe(scrollTopBefore); + expect(setup.captureCharFrame()).toContain("Focused custom surface"); + + await act(async () => setup.mockInput.pressEscape()); + await waitForFrame(setup, (frame) => !frame.includes("Focused custom surface"), 12); + expect(setup.renderer.currentFocusedRenderable).toBe(scrollBox); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + rmSync(root, { recursive: true, force: true }); + } + }); }); diff --git a/src/ui/components/chrome/AgentSkillDialog.tsx b/src/ui/components/chrome/AgentSkillDialog.tsx deleted file mode 100644 index 52ccb038a..000000000 --- a/src/ui/components/chrome/AgentSkillDialog.tsx +++ /dev/null @@ -1,96 +0,0 @@ -import type { MouseEvent as TuiMouseEvent } from "@opentui/core"; -import { fitText, padText } from "../../lib/text"; -import type { AppTheme } from "../../themes"; -import { ModalFrame } from "./ModalFrame"; - -export const AGENT_SKILL_COMMAND = "hunk skill path"; -export const AGENT_SKILL_PROMPT_ROWS = [ - "Load the Hunk skill and use it for this review.", - "Run `hunk skill path` to get the skill path.", -]; -export const AGENT_SKILL_PROMPT = AGENT_SKILL_PROMPT_ROWS.join(" "); - -/** Render copyable setup guidance for connecting an agent to the live Hunk session. */ -export function AgentSkillDialog({ - copySupported, - terminalHeight, - terminalWidth, - theme, - onClose, - onCopyPrompt, -}: { - copySupported: boolean; - terminalHeight: number; - terminalWidth: number; - theme: AppTheme; - onClose: () => void; - onCopyPrompt: () => void; -}) { - const width = Math.min(84, Math.max(58, terminalWidth - 8)); - const bodyWidth = Math.max(1, width - 4); - const promptWidth = Math.max(1, bodyWidth - 4); - const promptRows = AGENT_SKILL_PROMPT_ROWS; - const cardWidth = Math.max(1, bodyWidth - 4); - const cardTextWidth = Math.max(1, cardWidth - 4); - const requiredModalHeight = promptRows.length + 11; - const modalHeight = Math.min(requiredModalHeight, Math.max(10, terminalHeight - 2)); - - const copyLabel = copySupported ? " ⧉ Copy prompt " : " Copy unavailable "; - return ( - - - - - {fitText("Teach your agent how to review this Hunk session.", bodyWidth)} - - - - - {fitText("Prompt", promptWidth)} - - - - {promptRows.map((line, index) => ( - - {fitText(line, cardTextWidth)} - - ))} - - - - - { - event.stopPropagation(); - if (copySupported) { - onCopyPrompt(); - } - }} - > - {copyLabel} - - {padText("", Math.max(1, bodyWidth - copyLabel.length))} - - - - ); -} diff --git a/src/ui/components/chrome/ExtensionDialog.tsx b/src/ui/components/chrome/ExtensionDialog.tsx index 5a79b6b79..2898f950e 100644 --- a/src/ui/components/chrome/ExtensionDialog.tsx +++ b/src/ui/components/chrome/ExtensionDialog.tsx @@ -1,13 +1,18 @@ -import type { MouseEvent as TuiMouseEvent } from "@opentui/core"; +import type { BoxRenderable, MouseEvent as TuiMouseEvent, Renderable } from "@opentui/core"; +import { useRenderer } from "@opentui/react"; +import { Component, useLayoutEffect, useMemo, useRef, type ReactNode } from "react"; +import type { ExtensionDialogActions, ExtensionDialogProps } from "../../../extension-api/types"; import type { ExtensionDialogRequest, ExtensionInputDialogRequest, + ExtensionOpenDialogRequest, ExtensionSelectDialogRequest, } from "../../lib/extensionDialogs"; +import { planExtensionOpenDialog, windowDialogText } from "../../lib/extensionDialogGeometry"; import { extensionToastPrefix } from "../../lib/extensionNotifications"; -import { windowDialogText } from "../../lib/extensionDialogGeometry"; import { listWindowStart } from "../../lib/listWindow"; import { MODAL_FRAME_CHROME_ROWS, resolveModalGeometry } from "../../lib/modalGeometry"; +import { toExtensionPaintTheme } from "../../lib/extensionPaintTheme"; import { fitText, padText } from "../../lib/text"; import type { AppTheme } from "../../themes"; import { ConfirmDialog, confirmDialogHeight, DialogActionRow } from "./ConfirmDialog"; @@ -36,37 +41,74 @@ function attributionText(extensionId: string, width: number) { return fitText(`${extensionToastPrefix()} ${extensionId}`, width); } +/** Report whether one focused renderable belongs to a custom dialog's bounded root. */ +function isWithinRenderable(root: Renderable, candidate: Renderable | null) { + let current = candidate; + while (current) { + if (current === root) return true; + current = current.parent; + } + return false; +} + export function ExtensionDialog({ + copySupported, inputValue, - onAccept, - onCancel, - onChangeInput, - onPickOption, + onAcceptRequest, + onCancelRequest, + onChangeInputRequest, + onClose, + onCopy, + onNotify, + onPickOptionRequest, + onRenderFailure, request, selectedIndex, terminalHeight, terminalWidth, theme, }: { + copySupported: boolean; /** Live text of an input dialog's field; ignored by the other kinds. */ inputValue: string; - onAccept: (selectedIndexOverride?: number) => void; - onCancel: () => void; - onChangeInput: (value: string) => void; + onAcceptRequest: (requestId: number, selectedIndexOverride?: number) => void; + onCancelRequest: (requestId: number) => void; + onChangeInputRequest: (requestId: number, value: string) => void; + onClose: (requestId: number) => void; + onCopy: (requestId: number, text: string) => boolean; + onNotify: (requestId: number, message: string) => void; /** Highlight one option row without accepting it, mirroring the theme selector. */ - onPickOption: (index: number) => void; + onPickOptionRequest: (requestId: number, index: number) => void; + onRenderFailure: (requestId: number, error: unknown) => void; request: ExtensionDialogRequest; selectedIndex: number; terminalHeight: number; terminalWidth: number; theme: AppTheme; }) { + if (request.kind === "open") { + return ( + onCancelRequest(request.id)} + onClose={onClose} + onCopy={onCopy} + onNotify={onNotify} + onRenderFailure={onRenderFailure} + request={request} + terminalHeight={terminalHeight} + terminalWidth={terminalWidth} + theme={theme} + /> + ); + } + if (request.kind === "select") { return ( onAcceptRequest(request.id, selectedIndexOverride)} + onCancel={() => onCancelRequest(request.id)} + onPickOption={(index) => onPickOptionRequest(request.id, index)} request={request} selectedIndex={selectedIndex} terminalHeight={terminalHeight} @@ -80,9 +122,9 @@ export function ExtensionDialog({ return ( onAcceptRequest(request.id)} + onCancel={() => onCancelRequest(request.id)} + onChangeInput={(value) => onChangeInputRequest(request.id, value)} request={request} terminalHeight={terminalHeight} terminalWidth={terminalWidth} @@ -109,8 +151,16 @@ export function ExtensionDialog({ return ( onAcceptRequest(request.id), + }, + { + keyLabel: "esc/n", + label: request.cancelLabel, + run: () => onCancelRequest(request.id), + }, ]} height={confirmDialogHeight(visibleBody.lines.length + attributionRows + attributionGapRows)} terminalHeight={terminalHeight} @@ -118,7 +168,7 @@ export function ExtensionDialog({ theme={theme} title={request.title} width={frame.width} - onClose={onCancel} + onClose={() => onCancelRequest(request.id)} > {attributionRows > 0 ? ( @@ -136,6 +186,151 @@ export function ExtensionDialog({ ); } +/** Contain a custom dialog's render failure to its request identity. */ +class ExtensionDialogErrorBoundary extends Component< + { + request: ExtensionOpenDialogRequest; + fallback: ReactNode; + onError: (error: unknown) => void; + retireActions: () => void; + children: ReactNode; + }, + { failed: boolean; request: ExtensionOpenDialogRequest | null } +> { + override state = { failed: false, request: null as ExtensionOpenDialogRequest | null }; + static getDerivedStateFromError() { + return { failed: true }; + } + static getDerivedStateFromProps( + props: { request: ExtensionOpenDialogRequest }, + state: { failed: boolean; request: ExtensionOpenDialogRequest | null }, + ) { + return props.request !== state.request ? { request: props.request, failed: false } : null; + } + override componentDidCatch(error: unknown) { + this.props.retireActions(); + this.props.onError(error); + } + override componentWillUnmount() { + this.props.retireActions(); + } + override render() { + return this.state.failed ? this.props.fallback : this.props.children; + } +} + +/** Mount an extension-owned component inside host-controlled modal chrome. */ +function ExtensionOpenDialog({ + copySupported, + onCancel, + onClose, + onCopy, + onNotify, + onRenderFailure, + request, + terminalHeight, + terminalWidth, + theme, +}: { + copySupported: boolean; + onCancel: () => void; + onClose: (requestId: number) => void; + onCopy: (requestId: number, text: string) => boolean; + onNotify: (requestId: number, message: string) => void; + onRenderFailure: (requestId: number, error: unknown) => void; + request: ExtensionOpenDialogRequest; + terminalHeight: number; + terminalWidth: number; + theme: AppTheme; +}) { + const renderer = useRenderer(); + const componentRootRef = useRef(null); + const layout = planExtensionOpenDialog(request, terminalWidth, terminalHeight); + const { attributionGapRows, attributionRows, bodyWidth, componentHeight, frame } = layout; + const publicTheme = useMemo(() => toExtensionPaintTheme(theme), [theme]); + const actionLease = request.actionLease; + const actions = useMemo( + () => + Object.freeze({ + close: () => { + if (actionLease.active) onClose(request.id); + }, + copy: (text: string) => actionLease.active && onCopy(request.id, text), + notify: (message: string) => { + if (actionLease.active) onNotify(request.id, message); + }, + }), + [actionLease, onClose, onCopy, onNotify, request.id], + ); + const viewProps: ExtensionDialogProps = { + width: bodyWidth, + height: componentHeight, + theme: publicTheme, + copySupported, + actions, + }; + const View = request.component as (props: ExtensionDialogProps) => ReactNode; + const componentBox = (children: ReactNode, fallback = false) => ( + 0} + style={{ + width: bodyWidth, + height: componentHeight, + flexShrink: 0, + overflow: "hidden", + flexDirection: "column", + backgroundColor: theme.panel, + }} + > + {children} + + ); + + useLayoutEffect(() => { + const root = componentRootRef.current; + if (root && !isWithinRenderable(root, renderer.currentFocusedRenderable)) { + // A nested input that focused itself during mount wins. Otherwise the + // bounded root traps unhandled keys before they reach the review. + root.focus(); + } + }, [renderer, request.id]); + + return ( + + {attributionRows > 0 ? ( + + {layout.attributionText} + + ) : null} + {attributionGapRows > 0 ? : null} + Dialog unavailable, true)} + retireActions={() => { + actionLease.active = false; + }} + onError={(error) => { + onRenderFailure(request.id, error); + }} + > + {componentBox()} + + + ); +} + /** Render a select dialog as a keyboard- and mouse-driven option list. */ function ExtensionSelectDialog({ onAccept, diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index fe8b6f44c..bd105ac92 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -318,6 +318,7 @@ export function DiffPane({ draftNoteFocused = false, separatorWidth, pagerMode = false, + keyboardFocusBlocked = false, copyDecorations = false, screenTop = 0, showTopChrome, @@ -387,6 +388,8 @@ export function DiffPane({ draftNoteFocused?: boolean; separatorWidth: number; pagerMode?: boolean; + /** Prevent a modal custom surface from forwarding unhandled keys to the review stream. */ + keyboardFocusBlocked?: boolean; copyDecorations?: boolean; screenTop?: number; showTopChrome?: boolean; @@ -2710,7 +2713,7 @@ export function DiffPane({ height="100%" scrollY={true} viewportCulling={true} - focused={pagerMode} + focused={pagerMode && !keyboardFocusBlocked} onMouseDown={beginCopySelection} onMouseDrag={updateCopySelection} onMouseDragEnd={endCopySelection} 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/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index ac288555c..79bad6d6f 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -23,7 +23,6 @@ type FocusArea = "files" | "filter" | "note"; export interface UseAppKeyboardShortcutsOptions { activeMenuId: MenuId | null; activateCurrentMenuItem: () => void; - closeAgentSkill: () => void; closeHelp: () => void; closeMenu: () => void; acceptThemeSelector: () => void; @@ -36,8 +35,8 @@ export interface UseAppKeyboardShortcutsOptions { */ commands: readonly AppCommand[]; denyRepoExtensions: () => void; - /** The extension dialog currently on screen, or `null` when none is. */ - extensionDialog: ExtensionDialogRequest | null; + /** Read the live queued dialog; several keys can arrive before React renders it. */ + getExtensionDialog: () => ExtensionDialogRequest | null; acceptExtensionDialog: () => void; cancelExtensionDialog: () => void; moveExtensionDialogSelection: (delta: number) => void; @@ -69,7 +68,6 @@ export interface UseAppKeyboardShortcutsOptions { neverAskToSaveViewPreferencesAndQuit: () => void; closeSaveConfigPrompt: () => void; saveDraftNote: () => void; - showAgentSkill: boolean; showHelp: boolean; switchMenu: (delta: number) => void; toggleFocusArea: () => void; @@ -98,7 +96,6 @@ export interface UseAppKeyboardShortcutsOptions { export function useAppKeyboardShortcuts({ activeMenuId, activateCurrentMenuItem, - closeAgentSkill, closeHelp, closeMenu, acceptThemeSelector, @@ -107,7 +104,7 @@ export function useAppKeyboardShortcuts({ closeExtensionTrustPrompt, commands, denyRepoExtensions, - extensionDialog, + getExtensionDialog, acceptExtensionDialog, cancelExtensionDialog, moveExtensionDialogSelection, @@ -129,7 +126,6 @@ export function useAppKeyboardShortcuts({ neverAskToSaveViewPreferencesAndQuit, closeSaveConfigPrompt, saveDraftNote, - showAgentSkill, showHelp, switchMenu, toggleFocusArea, @@ -138,12 +134,11 @@ export function useAppKeyboardShortcuts({ const activeMenuIdRef = useRef(activeMenuId); const commandsRef = useRef(commands); const focusAreaRef = useRef(focusArea); - const showAgentSkillRef = useRef(showAgentSkill); const showHelpRef = useRef(showHelp); const saveConfigPromptOpenRef = useRef(saveConfigPromptOpen); const themeSelectorOpenRef = useRef(themeSelectorOpen); const extensionTrustPromptOpenRef = useRef(extensionTrustPromptOpen); - const extensionDialogRef = useRef(extensionDialog); + const getExtensionDialogRef = useRef(getExtensionDialog); // The mode callbacks read live App state (which mode is running, its context), // so they are reached through refs rather than captured when the chain is built. const isFileViewModeActiveRef = useRef(isFileViewModeActive); @@ -152,7 +147,7 @@ export function useAppKeyboardShortcuts({ const isKeyboardModeActiveRef = useRef(isKeyboardModeActive); const exitKeyboardModeRef = useRef(exitKeyboardMode); const sendKeyboardModeKeyRef = useRef(sendKeyboardModeKey); - // These three close over live dialog state (the highlighted option, the typed + // These callbacks close over live dialog state (the highlighted option, the typed // text), so they are read through refs rather than captured once. const acceptExtensionDialogRef = useRef(acceptExtensionDialog); const cancelExtensionDialogRef = useRef(cancelExtensionDialog); @@ -161,12 +156,11 @@ export function useAppKeyboardShortcuts({ activeMenuIdRef.current = activeMenuId; commandsRef.current = commands; focusAreaRef.current = focusArea; - showAgentSkillRef.current = showAgentSkill; showHelpRef.current = showHelp; saveConfigPromptOpenRef.current = saveConfigPromptOpen; themeSelectorOpenRef.current = themeSelectorOpen; extensionTrustPromptOpenRef.current = extensionTrustPromptOpen; - extensionDialogRef.current = extensionDialog; + getExtensionDialogRef.current = getExtensionDialog; isFileViewModeActiveRef.current = isFileViewModeActive; exitFileViewModeRef.current = exitFileViewMode; sendFileViewModeKeyRef.current = sendFileViewModeKey; @@ -229,17 +223,12 @@ export function useAppKeyboardShortcuts({ return "mine"; }; - /** Escape closes the topmost open overlay (agent skill, then help). */ + /** Escape closes Hunk's help overlay. */ const handleDialogShortcut = (key: KeyEvent): KeyOwner => { if (!isEscapeKey(key)) { return "notMine"; } - if (showAgentSkillRef.current) { - closeAgentSkill(); - return "mine"; - } - if (showHelpRef.current) { closeHelp(); return "mine"; @@ -322,12 +311,12 @@ export function useAppKeyboardShortcuts({ * extension may not outrank them — and above menus, help, and the command * table. * - * The input kind is the one non-modal-shaped answer: keys it does not act on - * are the text the user is typing into the dialog's focused field, so they - * are the focused widget's, not swallowed. + * Input and open dialogs leave unclaimed keys for their focused surface. The + * open dialog's bounded root takes focus even when its component has no input, + * so those keys cannot reach a previously focused review widget behind it. */ const handleExtensionDialogShortcut = (key: KeyEvent): KeyOwner => { - const dialog = extensionDialogRef.current; + const dialog = getExtensionDialogRef.current(); if (!dialog) { return "notMine"; } @@ -338,8 +327,11 @@ export function useAppKeyboardShortcuts({ } if (key.name === "return" || key.name === "enter") { - acceptExtensionDialogRef.current(); - return "mine"; + if (dialog.kind !== "open") { + acceptExtensionDialogRef.current(); + return "mine"; + } + return "focused"; } if (dialog.kind === "select") { @@ -370,7 +362,7 @@ export function useAppKeyboardShortcuts({ } } - return dialog.kind === "input" ? "focused" : "mine"; + return dialog.kind === "input" || dialog.kind === "open" ? "focused" : "mine"; }; /** Own every key while the theme selector is up; it is a modal surface. */ 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.test.tsx b/src/ui/hooks/useExtensionDialogController.test.tsx index 1f12e4a11..d95232d7b 100644 --- a/src/ui/hooks/useExtensionDialogController.test.tsx +++ b/src/ui/hooks/useExtensionDialogController.test.tsx @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { testRender } from "@opentui/react/test-utils"; -import { act, useState } from "react"; +import { act, useLayoutEffect, useState } from "react"; import { useExtensionDialogController } from "./useExtensionDialogController"; /** Mount the controller with a replaceable review-generation token. */ @@ -57,7 +57,9 @@ describe("useExtensionDialogController", () => { expect(harness.controller().selectedIndex).toBe(0); expect(harness.controller().inputValue).toBe("feature/base"); - await act(async () => harness.controller().updateInput("feature/typed")); + await act(async () => + harness.controller().updateInput(harness.controller().request!.id, "feature/typed"), + ); await act(async () => harness.controller().accept()); expect(await typed).toBe("feature/typed"); await flush(harness.setup); @@ -67,6 +69,43 @@ describe("useExtensionDialogController", () => { } }); + test("ignores controls retained from a rendered request after promotion", async () => { + const harness = await renderController(); + const dialogs = harness.controller().createDialogs("probe"); + let selected!: Promise; + let typed!: Promise; + + try { + await act(async () => { + selected = dialogs.select({ title: "First", options: ["one", "two"] }); + typed = dialogs.input({ title: "Second", initial: "initial" }); + }); + await flush(harness.setup); + const firstId = harness.controller().request!.id; + + await act(async () => { + harness.controller().pickOption(firstId, 1); + harness.controller().acceptRequest(firstId); + harness.controller().pickOption(firstId, 0); + harness.controller().updateInput(firstId, "stale"); + harness.controller().acceptRequest(firstId); + }); + + expect(await selected).toBe("two"); + expect(harness.controller().getCurrentRequest()).toMatchObject({ + kind: "input", + title: "Second", + }); + await flush(harness.setup); + expect(harness.controller().inputValue).toBe("initial"); + + await act(async () => harness.controller().cancel()); + expect(await typed).toBeNull(); + } finally { + await act(async () => harness.setup.renderer.destroy()); + } + }); + test("cancels pending requests when a soft reload replaces the review", async () => { const harness = await renderController(); const dialogs = harness.controller().createDialogs("probe"); @@ -117,4 +156,41 @@ describe("useExtensionDialogController", () => { expect(await selected).toBeNull(); expect(await dialogs.input({ title: "Too late?" })).toBeNull(); }); + + test("retires the current request before child layout cleanup", async () => { + let controller!: ReturnType; + let requestDuringCleanup: unknown = "not cleaned"; + + function CleanupProbe() { + useLayoutEffect( + () => () => { + requestDuringCleanup = controller.getCurrentRequest(); + }, + [], + ); + return null; + } + + function Harness() { + controller = useExtensionDialogController({ reviewGeneration: "review" }); + return ; + } + + const setup = await testRender(, { width: 40, height: 4 }); + await act(async () => setup.renderOnce()); + let pending!: Promise; + await act(async () => { + pending = controller.createDialogs("probe").open({ + title: "Open", + component: () => null, + }); + }); + await act(async () => setup.renderOnce()); + expect(controller.getCurrentRequest()).toMatchObject({ kind: "open" }); + + await act(async () => setup.renderer.destroy()); + + expect(requestDuringCleanup).toBeNull(); + expect(await pending).toBeUndefined(); + }); }); diff --git a/src/ui/hooks/useExtensionDialogController.ts b/src/ui/hooks/useExtensionDialogController.ts index 0ac595b72..265fa4c2a 100644 --- a/src/ui/hooks/useExtensionDialogController.ts +++ b/src/ui/hooks/useExtensionDialogController.ts @@ -1,4 +1,11 @@ -import { useEffect, useLayoutEffect, useRef, useState, useSyncExternalStore } from "react"; +import { + useCallback, + useEffect, + useLayoutEffect, + useRef, + useState, + useSyncExternalStore, +} from "react"; import { createExtensionDialogQueue, type ExtensionDialogQueue, @@ -8,15 +15,25 @@ import { export interface ExtensionDialogController { /** Build the dialog capability one extension command receives. */ createDialogs: ExtensionDialogQueue["createDialogs"]; + /** Read the queue's live request rather than a render-time snapshot. */ + getCurrentRequest: ExtensionDialogQueue["current"]; + /** Check request identity and generation liveness at the moment an action runs. */ + isCurrentRequestLive: ExtensionDialogQueue["isCurrentLive"]; + /** Cancel one request only while it remains at the front of the queue. */ + cancelRequest: ExtensionDialogQueue["cancel"]; /** Request currently visible to the user. */ request: ExtensionDialogRequest | null; selectedIndex: number; inputValue: string; accept: (selectedIndexOverride?: number) => void; + /** Accept only the rendered request with this id; stale controls are ignored. */ + acceptRequest: (requestId: number, 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; + pickOption: (requestId: number, index: number) => void; + updateInput: (requestId: number, value: string) => void; } /** Own the React state and lifetime of one App instance's extension-dialog queue. */ @@ -30,14 +47,27 @@ export function useExtensionDialogController({ const request = useSyncExternalStore(queue.subscribe, queue.current, queue.current); const [selectedIndex, setSelectedIndex] = useState(0); const [inputValue, setInputValue] = useState(""); - const requestId = request?.id ?? null; - const initialInput = request?.kind === "input" ? request.initial : ""; + const selectedIndexRef = useRef(0); + const inputValueRef = useRef(""); + const answerRequestIdRef = useRef(null); + /** Align mutable answer state before another key can arrive ahead of React. */ + const alignAnswerState = useCallback((active: ExtensionDialogRequest | null) => { + const activeId = active?.id ?? null; + if (answerRequestIdRef.current === activeId) return; + + answerRequestIdRef.current = activeId; + selectedIndexRef.current = 0; + inputValueRef.current = active?.kind === "input" ? active.initial : ""; + }, []); + + alignAnswerState(request); useEffect(() => { // A promoted queued request must never inherit the previous request's answer state. - setSelectedIndex(0); - setInputValue(initialInput); - }, [initialInput, requestId]); + alignAnswerState(request); + setSelectedIndex(selectedIndexRef.current); + setInputValue(inputValueRef.current); + }, [alignAnswerState, request]); const previousReviewGenerationRef = useRef(reviewGeneration); useLayoutEffect(() => { @@ -50,45 +80,105 @@ export function useExtensionDialogController({ } }, [queue, reviewGeneration]); - useEffect(() => { - // Settle every pending handler when this App instance leaves the review tree. + useLayoutEffect(() => { + // Retire capabilities before custom component layout cleanups can retain or + // invoke actions from a dialog whose App is already leaving the tree. return () => queue.shutdown(); }, [queue]); - /** Answer the visible request with the state appropriate to its dialog kind. */ - const accept = (selectedIndexOverride?: number) => { - if (!request) return; + /** Answer one rendered request only while it remains current. */ + const acceptRequest = useCallback( + (requestId: number, selectedIndexOverride?: number) => { + const active = queue.current(); + alignAnswerState(active); + if (!active || active.id !== requestId) return; - if (request.kind === "select") { - queue.accept(request.id, request.options[selectedIndexOverride ?? selectedIndex]); - return; - } + if (active.kind === "select") { + queue.accept(active.id, active.options[selectedIndexOverride ?? selectedIndexRef.current]); + alignAnswerState(queue.current()); + return; + } - queue.accept(request.id, request.kind === "input" ? inputValue : undefined); + queue.accept(active.id, active.kind === "input" ? inputValueRef.current : undefined); + alignAnswerState(queue.current()); + }, + [alignAnswerState, queue], + ); + + /** Answer the live request with the state appropriate to its dialog kind. */ + const accept = (selectedIndexOverride?: number) => { + const active = queue.current(); + if (active) acceptRequest(active.id, selectedIndexOverride); }; /** Dismiss the visible request with its kind-specific cancel value. */ const cancel = () => { - if (request) queue.cancel(request.id); + const active = queue.current(); + if (active) queue.cancel(active.id); + alignAnswerState(queue.current()); }; /** Move a select request's highlight, wrapping at both ends. */ const moveSelection = (delta: number) => { - if (request?.kind !== "select") return; + const active = queue.current(); + alignAnswerState(active); + if (active?.kind !== "select") return; + + const optionCount = active.options.length; + const next = (selectedIndexRef.current + delta + optionCount) % optionCount; + selectedIndexRef.current = next; + setSelectedIndex(next); + }; + + /** Cancel one live request and synchronously prepare any promoted answer state. */ + const cancelRequest = useCallback( + (id: number) => { + queue.cancel(id); + alignAnswerState(queue.current()); + }, + [alignAnswerState, queue], + ); + + /** Drain every request and synchronously clear its mutable answer state. */ + const cancelAll = useCallback(() => { + queue.cancelAll(); + alignAnswerState(queue.current()); + }, [alignAnswerState, queue]); + + /** Select one option while keeping the same-flush answer state current. */ + const pickOption = (requestId: number, index: number) => { + const active = queue.current(); + alignAnswerState(active); + if (active?.kind !== "select" || active.id !== requestId) return; + + selectedIndexRef.current = index; + setSelectedIndex(index); + }; + + /** Update input text in both React and same-flush answer state. */ + const updateInput = (requestId: number, value: string) => { + const active = queue.current(); + alignAnswerState(active); + if (active?.kind !== "input" || active.id !== requestId) return; - const optionCount = request.options.length; - setSelectedIndex((current) => (current + delta + optionCount) % optionCount); + inputValueRef.current = value; + setInputValue(value); }; return { createDialogs: queue.createDialogs, + getCurrentRequest: queue.current, + isCurrentRequestLive: queue.isCurrentLive, + cancelRequest, request, selectedIndex, inputValue, accept, + acceptRequest, cancel, + cancelAll, moveSelection, - pickOption: setSelectedIndex, - updateInput: setInputValue, + pickOption, + updateInput, }; } 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/extensionDialogGeometry.test.ts b/src/ui/lib/extensionDialogGeometry.test.ts index c7ed4ba19..c96001017 100644 --- a/src/ui/lib/extensionDialogGeometry.test.ts +++ b/src/ui/lib/extensionDialogGeometry.test.ts @@ -1,5 +1,19 @@ import { describe, expect, test } from "bun:test"; -import { windowDialogText } from "./extensionDialogGeometry"; +import type { ExtensionOpenDialogRequest } from "./extensionDialogs"; +import { planExtensionOpenDialog, windowDialogText } from "./extensionDialogGeometry"; + +const TestDialog = () => null; +const openRequest = { + id: 1, + kind: "open", + extensionId: "example", + showAttribution: true, + title: "Custom surface", + width: 64, + height: 12, + component: TestDialog, + actionLease: { active: true }, +} satisfies ExtensionOpenDialogRequest; describe("windowDialogText", () => { test("wraps prose within the available terminal-cell rows", () => { @@ -17,3 +31,35 @@ describe("windowDialogText", () => { expect(windowDialogText(["overflow"], 3, 0)).toEqual({ lines: [], truncated: true }); }); }); + +describe("planExtensionOpenDialog", () => { + test("gives the component its preferred rectangle after host attribution", () => { + const layout = planExtensionOpenDialog(openRequest, 120, 40); + + expect(layout.frame).toMatchObject({ width: 68, height: 19 }); + expect(layout.bodyWidth).toBe(64); + expect(layout.componentHeight).toBe(12); + expect(layout.attributionRows).toBe(1); + expect(layout.attributionGapRows).toBe(1); + expect(layout.attributionText).toBe("ext example"); + }); + + test("clamps the component rectangle while preserving attribution first", () => { + const layout = planExtensionOpenDialog(openRequest, 50, 12); + + expect(layout.frame).toMatchObject({ width: 48, height: 10 }); + expect(layout.bodyWidth).toBe(44); + expect(layout.attributionRows).toBe(1); + expect(layout.attributionGapRows).toBe(1); + expect(layout.componentHeight).toBe(3); + }); + + test("gives bundled components the attribution rows they do not need", () => { + const layout = planExtensionOpenDialog({ ...openRequest, showAttribution: false }, 120, 40); + + expect(layout.frame.height).toBe(17); + expect(layout.attributionRows).toBe(0); + expect(layout.attributionGapRows).toBe(0); + expect(layout.componentHeight).toBe(12); + }); +}); diff --git a/src/ui/lib/extensionDialogGeometry.ts b/src/ui/lib/extensionDialogGeometry.ts index 795b8c66c..cd1bb710e 100644 --- a/src/ui/lib/extensionDialogGeometry.ts +++ b/src/ui/lib/extensionDialogGeometry.ts @@ -1,4 +1,7 @@ -import { wrapText } from "./text"; +import { fitText, wrapText } from "./text"; +import { extensionToastPrefix } from "./extensionNotifications"; +import { MODAL_FRAME_CHROME_ROWS, resolveModalGeometry } from "./modalGeometry"; +import type { ExtensionOpenDialogRequest } from "./extensionDialogs"; /** Wrapped body rows that fit one modal body allocation. */ export interface WindowedDialogText { @@ -25,3 +28,42 @@ export function windowDialogText( truncated: true, }; } + +/** Concrete host frame and component rectangle for one open dialog. */ +export interface ExtensionOpenDialogLayout { + frame: { width: number; height: number }; + bodyWidth: number; + componentHeight: number; + attributionText: string; + attributionRows: number; + attributionGapRows: number; +} + +/** Clamp an extension-owned component while preserving host attribution above it. */ +export function planExtensionOpenDialog( + request: ExtensionOpenDialogRequest, + terminalWidth: number, + terminalHeight: number, +): ExtensionOpenDialogLayout { + const attributionRequestRows = request.showAttribution ? 2 : 0; + const frame = resolveModalGeometry({ + width: request.width + 4, + height: request.height + MODAL_FRAME_CHROME_ROWS + attributionRequestRows, + terminalWidth, + terminalHeight, + }); + const bodyWidth = Math.max(1, frame.width - 4); + const contentRows = Math.max(0, frame.height - MODAL_FRAME_CHROME_ROWS); + const attributionRows = request.showAttribution && contentRows > 0 ? 1 : 0; + const attributionGapRows = attributionRows > 0 && contentRows > 1 ? 1 : 0; + const componentHeight = Math.max(0, contentRows - attributionRows - attributionGapRows); + + return { + frame, + bodyWidth, + componentHeight, + attributionText: fitText(`${extensionToastPrefix()} ${request.extensionId}`, bodyWidth), + attributionRows, + attributionGapRows, + }; +} diff --git a/src/ui/lib/extensionDialogs.test.ts b/src/ui/lib/extensionDialogs.test.ts index 2b5b4ab8c..4a37ebeb1 100644 --- a/src/ui/lib/extensionDialogs.test.ts +++ b/src/ui/lib/extensionDialogs.test.ts @@ -1,5 +1,10 @@ import { describe, expect, test } from "bun:test"; -import { createExtensionDialogQueue } from "./extensionDialogs"; +import { + createExtensionDialogQueue, + normalizeExtensionDialogClipboardText, +} from "./extensionDialogs"; + +const TestDialog = () => null; describe("createExtensionDialogQueue", () => { test("shows one dialog at a time and queues the rest in call order", async () => { @@ -50,6 +55,12 @@ describe("createExtensionDialogQueue", () => { const valueless = dialogs.select({ title: "Which?", options: ["a"] }); queue.accept(queue.current()!.id); expect(await valueless).toBeNull(); + + const opened = dialogs.open({ title: "Guide", component: TestDialog }); + queue.accept(queue.current()!.id); + expect(queue.current()).toMatchObject({ kind: "open", title: "Guide" }); + queue.cancel(queue.current()!.id); + expect(await opened).toBeUndefined(); }); test("ignores an answer aimed at a dialog that is no longer current", async () => { @@ -115,6 +126,32 @@ describe("createExtensionDialogQueue", () => { expect(queue.current()).toMatchObject({ title: "Pick", options: ["opt"] }); }); + test("carries a custom component and default rectangle into the request", () => { + const queue = createExtensionDialogQueue(); + const dialogs = queue.createDialogs("guide"); + + void dialogs.open({ + title: "Setup", + component: TestDialog, + }); + + expect(queue.current()).toMatchObject({ + kind: "open", + width: 64, + height: 12, + component: TestDialog, + }); + }); + + test("normalizes bounded custom-dialog clipboard payloads", () => { + expect(normalizeExtensionDialogClipboardText("copy\t\u001b[31mexactly\u001b[0m")).toBe( + "copy exactly", + ); + expect(normalizeExtensionDialogClipboardText("")).toBeNull(); + expect(normalizeExtensionDialogClipboardText("x".repeat(16_385))).toBeNull(); + expect(normalizeExtensionDialogClipboardText("\t".repeat(4_097))).toBeNull(); + }); + test("sanitizes an input dialog's starting text without trimming it", () => { const queue = createExtensionDialogQueue(); const dialogs = queue.createDialogs("hostile"); @@ -206,7 +243,7 @@ describe("createExtensionDialogQueue", () => { expect(await second).toBe(false); }); - test("rejects a blank title and a select with no options", async () => { + test("rejects blank sanitized titles and malformed select options", async () => { const queue = createExtensionDialogQueue(); const dialogs = queue.createDialogs("probe"); @@ -221,6 +258,36 @@ describe("createExtensionDialogQueue", () => { await expect(dialogs.select({ title: "Which?", options: [] })).rejects.toThrow( /at least one option/, ); + await expect(dialogs.confirm({ title: "\u001b[31m\u001b[0m" })).rejects.toThrow( + /non-empty title after terminal sanitization/, + ); + await expect( + dialogs.select({ title: "Which?", options: ["\u001b]0;pwned\u0007"] }), + ).rejects.toThrow(/remain non-empty after sanitization/); + await expect(dialogs.select({ title: "Which?", options: [" "] })).rejects.toThrow( + /remain non-empty after sanitization/, + ); + const sparseOptions = Array.from({ length: 2 }, () => "one"); + delete sparseOptions[0]; + sparseOptions[1] = "one"; + await expect(dialogs.select({ title: "Which?", options: sparseOptions })).rejects.toThrow( + /dense array of strings/, + ); + await expect(dialogs.open({ title: "No component", component: null as never })).rejects.toThrow( + /component function/, + ); + await expect( + dialogs.open({ title: "Bad width", width: 0, component: TestDialog }), + ).rejects.toThrow(/width must be an integer from 1 to 240/); + await expect( + dialogs.open({ title: "Wide", width: 241, component: TestDialog }), + ).rejects.toThrow(/width must be an integer from 1 to 240/); + await expect( + dialogs.open({ title: "Bad height", height: 1.5, component: TestDialog }), + ).rejects.toThrow(/height must be an integer from 1 to 100/); + await expect( + dialogs.open({ title: "Tall", height: 101, component: TestDialog }), + ).rejects.toThrow(/height must be an integer from 1 to 100/); await expect( dialogs.select({ title: "Which?", options: [1 as unknown as string] }), ).rejects.toThrow(/must all be strings/); diff --git a/src/ui/lib/extensionDialogs.ts b/src/ui/lib/extensionDialogs.ts index d97d5b9ae..6c9a77050 100644 --- a/src/ui/lib/extensionDialogs.ts +++ b/src/ui/lib/extensionDialogs.ts @@ -1,7 +1,7 @@ /** * The queue behind `ctx.dialogs`, kept free of React on purpose. * - * Extensions ask questions from async handlers, so the interesting behavior is + * Extensions open modal surfaces from async handlers, so the interesting behavior is * ordering and settlement — one dialog on screen at a time, later requests * waiting their turn, everything still waiting resolving its cancel value when * the session goes away. None of that is rendering, so it lives here as plain @@ -11,11 +11,12 @@ import type { ExtensionConfirmOptions, + ExtensionDialogOptions, ExtensionDialogs, ExtensionInputOptions, ExtensionSelectOptions, } from "../../extension-api/types"; -import { sanitizeTerminalLine } from "../../lib/terminalText"; +import { sanitizeTerminalLine, sanitizeTerminalText } from "../../lib/terminalText"; /** Default label for the accepting action of a confirm dialog. */ const DEFAULT_CONFIRM_LABEL = "ok"; @@ -26,6 +27,17 @@ const DEFAULT_CANCEL_LABEL = "cancel"; /** Body lines one confirm dialog may show; beyond this the modal stops being a prompt. */ const MAX_CONFIRM_BODY_LINES = 6; +/** Default extension-owned component rectangle. */ +const DEFAULT_OPEN_DIALOG_WIDTH = 64; +const DEFAULT_OPEN_DIALOG_HEIGHT = 12; + +/** Bounds keep one request from retaining absurd off-screen geometry. */ +const MAX_OPEN_DIALOG_WIDTH = 240; +const MAX_OPEN_DIALOG_HEIGHT = 100; + +/** Clipboard text is bounded before it reaches the terminal's OSC 52 channel. */ +const MAX_DIALOG_COPY_TEXT_LENGTH = 16_384; + /** What every queued dialog carries, whatever kind it is. */ interface ExtensionDialogRequestBase { /** @@ -61,14 +73,25 @@ export interface ExtensionInputDialogRequest extends ExtensionDialogRequestBase initial: string; } +/** One extension-owned component the host should mount in a modal frame. */ +export interface ExtensionOpenDialogRequest extends ExtensionDialogRequestBase { + kind: "open"; + width: number; + height: number; + component: ExtensionDialogOptions["component"]; + /** Shared across React render retries so every retained action can be retired together. */ + actionLease: { active: boolean }; +} + /** One dialog the host should draw, normalized from what an extension asked for. */ export type ExtensionDialogRequest = | ExtensionConfirmDialogRequest | ExtensionSelectDialogRequest - | ExtensionInputDialogRequest; + | ExtensionInputDialogRequest + | ExtensionOpenDialogRequest; /** What a dialog hands back to the awaiting handler. */ -type ExtensionDialogResult = boolean | string | null; +type ExtensionDialogResult = boolean | string | null | undefined; /** The host-side controller for every extension dialog in one session. */ export interface ExtensionDialogQueue { @@ -79,11 +102,14 @@ export interface ExtensionDialogQueue { ): ExtensionDialogs; /** The dialog that should be on screen, or `null` when none is. */ current(): ExtensionDialogRequest | null; + /** Whether this id is still the current request and its owning capability remains live. */ + isCurrentLive(id: number): boolean; /** * Accept the dialog with this id. * * A confirm resolves `true`. A select or input resolves `value`; without one - * there is nothing to hand back, so it settles as a cancel instead. + * there is nothing to hand back, so it settles as a cancel instead. Open + * component dialogs ignore acceptance and remain visible until cancelled. * * Answering anything but the current dialog is ignored: an answer computed * for a dialog the queue has already moved past — a repeated key, a late @@ -123,7 +149,12 @@ function normalizeTitle(method: string, title: unknown) { invalid(method, "requires a non-empty title."); } - return sanitizeTerminalLine(title.trim()); + const normalized = sanitizeTerminalLine(title.trim()).trim(); + if (normalized.length === 0) { + invalid(method, "requires a non-empty title after terminal sanitization."); + } + + return normalized; } /** Normalize an optional extension-authored label, falling back to Hunk's own. */ @@ -132,7 +163,7 @@ function normalizeLabel(label: unknown, fallback: string) { return fallback; } - return sanitizeTerminalLine(label.trim()); + return sanitizeTerminalLine(label.trim()).trim() || fallback; } /** @@ -143,30 +174,68 @@ function normalizeLabel(label: unknown, fallback: string) { * dialog text is third-party and routinely carries repo-controlled fragments, * exactly like toast text. */ -function normalizeBodyLines(body: unknown) { +function normalizeBodyLines(body: unknown, maxLines = MAX_CONFIRM_BODY_LINES) { if (typeof body !== "string" || body.length === 0) { return []; } return body .split("\n") - .slice(0, MAX_CONFIRM_BODY_LINES) + .slice(0, maxLines) .map((line) => sanitizeTerminalLine(line)); } +/** Normalize one preferred component dimension, or reject it. */ +function normalizeOpenDialogDimension( + name: "width" | "height", + value: unknown, + fallback: number, + maximum: number, +) { + if (value === undefined) return fallback; + if (!Number.isInteger(value) || (value as number) < 1 || (value as number) > maximum) { + invalid("open", `${name} must be an integer from 1 to ${maximum}.`); + } + return value as number; +} + +/** Normalize one custom-dialog clipboard payload, or reject it without throwing. */ +export function normalizeExtensionDialogClipboardText(text: unknown): string | null { + if (typeof text !== "string" || text.length === 0 || text.length > MAX_DIALOG_COPY_TEXT_LENGTH) { + return null; + } + + const normalized = sanitizeTerminalText(text).replaceAll("\t", " "); + return normalized.length > 0 && normalized.length <= MAX_DIALOG_COPY_TEXT_LENGTH + ? normalized + : null; +} + /** Normalize the choices of a select dialog, or reject them. */ function normalizeOptions(options: unknown) { if (!Array.isArray(options) || options.length === 0) { invalid("select", "requires at least one option."); } - return options.map((option) => { + const normalizedOptions: string[] = []; + for (let index = 0; index < options.length; index += 1) { + if (!Object.hasOwn(options, index)) { + invalid("select", "options must be a dense array of strings."); + } + const option = options[index]; if (typeof option !== "string") { - invalid("select", "options must all be strings."); + invalid("select", "options must all be strings that remain non-empty after sanitization."); } - return sanitizeTerminalLine(option); - }); + const normalized = sanitizeTerminalLine(option).trim(); + if (normalized.length === 0) { + invalid("select", "options must all be strings that remain non-empty after sanitization."); + } + + normalizedOptions.push(normalized); + } + + return normalizedOptions; } /** @@ -194,9 +263,14 @@ export function createExtensionDialogQueue(): ExtensionDialogQueue { } }; - /** The cancel value one request resolves with: `false` for confirm, `null` otherwise. */ + /** The cancel value one request resolves with. */ const cancelValueFor = (request: ExtensionDialogRequest): ExtensionDialogResult => - request.kind === "confirm" ? false : null; + request.kind === "confirm" ? false : request.kind === "open" ? undefined : null; + + /** Retire component actions before settling or removing their request. */ + const retireActions = (request: ExtensionDialogRequest) => { + if (request.kind === "open") request.actionLease.active = false; + }; /** * Queue one request and hand back the promise its handler awaits. @@ -231,6 +305,7 @@ export function createExtensionDialogQueue(): ExtensionDialogQueue { const drainPending = () => { const drained = pending.splice(0); for (const entry of drained) { + retireActions(entry.request); entry.settle(cancelValueFor(entry.request)); } @@ -246,6 +321,7 @@ export function createExtensionDialogQueue(): ExtensionDialogQueue { return; } + retireActions(active.request); active.settle(value); notify(); }; @@ -310,6 +386,39 @@ export function createExtensionDialogQueue(): ExtensionDialogQueue { isLive, ); }, + async open(options: ExtensionDialogOptions) { + const title = normalizeTitle("open", options?.title); + if (typeof options?.component !== "function") { + invalid("open", "requires a component function."); + } + const width = normalizeOpenDialogDimension( + "width", + options.width, + DEFAULT_OPEN_DIALOG_WIDTH, + MAX_OPEN_DIALOG_WIDTH, + ); + const height = normalizeOpenDialogDimension( + "height", + options.height, + DEFAULT_OPEN_DIALOG_HEIGHT, + MAX_OPEN_DIALOG_HEIGHT, + ); + await enqueue( + (id) => ({ + kind: "open", + id, + extensionId, + showAttribution, + title, + width, + height, + component: options.component, + actionLease: { active: true }, + }), + undefined, + isLive, + ); + }, }; }, @@ -317,6 +426,11 @@ export function createExtensionDialogQueue(): ExtensionDialogQueue { return pending[0]?.request ?? null; }, + isCurrentLive(id: number) { + const active = pending[0]; + return active?.request.id === id && active.isLive(); + }, + accept(id: number, value?: string) { const active = pending[0]; if (!active || active.request.id !== id) { @@ -333,6 +447,10 @@ export function createExtensionDialogQueue(): ExtensionDialogQueue { return; } + if (active.request.kind === "open") { + return; + } + settleCurrent(value ?? null); }, 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/chrome.test.ts b/test/pty/chrome.test.ts index 0eea9d935..7271c1907 100644 --- a/test/pty/chrome.test.ts +++ b/test/pty/chrome.test.ts @@ -84,6 +84,48 @@ describe("PTY chrome", () => { } }); + test("the Agent menu opens bundled skill guidance as a component dialog", async () => { + const fixture = harness.createTwoFileRepoFixture(); + const session = await harness.launchHunk({ + args: ["diff", "--mode", "split"], + cwd: fixture.dir, + cols: 120, + rows: 24, + }); + + try { + await session.waitForText(/View\s+Navigate\s+Agent\s+Help/, { timeout: 15_000 }); + await session.click(/Agent/, { first: true }); + const menu = await session.waitForText(/Agent skill/, { timeout: 5_000 }); + expect(menu).toContain("Next annotated file"); + + await session.click(/Agent skill/); + const info = await harness.waitForSnapshot( + session, + (text) => + text.includes("Teach your agent how to review this Hunk session.") && + text.includes("hunk skill path") && + text.includes("⧉ Copy prompt"), + 5_000, + ); + expect(info).not.toContain("ext hunk"); + + await session.press("enter"); + const stillOpen = await session.text({ immediate: true }); + expect(stillOpen).toContain("Teach your agent how to review this Hunk session."); + + await session.press("escape"); + const closed = await harness.waitForSnapshot( + session, + (text) => !text.includes("Teach your agent how to review this Hunk session."), + 5_000, + ); + expect(closed).toContain("alpha.ts"); + } finally { + session.close(); + } + }); + test("rapid theme preview key repeats keep the selector responsive", async () => { const initialThemeId = "github-dark-default"; const themes = availableThemes(); 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..786d4a7a5 100644 --- a/website/src/content/docs/docs/extend/extension-api.md +++ b/website/src/content/docs/docs/extend/extension-api.md @@ -7,9 +7,12 @@ 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 `17`). Branch on it if you want +one file to support several Hunk versions. Version 17 adds custom React/OpenTUI +dialog surfaces; version 16 added 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 @@ -282,11 +285,12 @@ A handler may be async; a failure becomes a warning naming your extension. ### Asking the user -`ctx.dialogs` puts a question on screen and waits for the answer. Three methods, all return promises: +`ctx.dialogs` puts a modal surface on screen and waits for it to settle. Four methods return promises: - `confirm({ title, body?, confirmLabel?, cancelLabel? })` → `true` or `false` - `select({ title, options })` → the chosen string, or `null` - `input({ title, placeholder?, initial? })` → the typed string, or `null` +- `open({ title, width?, height?, component })` → `void` when closed ```ts hunk.registerCommand( @@ -308,6 +312,47 @@ hunk.registerCommand( ); ``` +`open` mounts a React/OpenTUI component in an exact host-owned rectangle, like +`registerPane` inside modal chrome. Preferred `width` and `height` default to +`64×12` and are clamped to the terminal. The component receives the resulting +dimensions, semantic theme, clipboard availability, and guarded `close`, `copy`, +and `notify` actions. Escape stays host-owned; other keys reach the component. + +```tsx +import type { ExtensionDialogProps } from "hunkdiff/extension"; + +const prompt = "Review the current Hunk session. Focus on correctness."; + +function AgentSetupDialog({ actions, copySupported, theme }: ExtensionDialogProps) { + const copy = () => { + actions.notify(actions.copy(prompt) ? "Copied agent prompt" : "Clipboard copy failed"); + }; + return ( + + {prompt} + + Copy prompt + + + ); +} + +hunk.registerCommand({ id: "agent-setup", title: "Agent setup" }, async (ctx) => { + await ctx.dialogs.open({ + title: "Agent setup", + width: 64, + height: 6, + component: AgentSetupDialog, + }); +}); +``` + +Component dialogs are trusted extension code like panes. Hunk cannot verify +what an arbitrary component visibly discloses before it calls `actions.copy`. +Hunk still owns bounds, frame chrome, attribution, Escape handling, queueing, +and render-failure containment. Clipboard payloads are sanitized and limited to +16,384 JavaScript string code units. + `select` fits acting on part of the selection — asking which hunk to jump to, then navigating there: ```ts @@ -329,9 +374,20 @@ hunk.registerCommand({ id: "pick-hunk", title: "Pick a hunk", key: "ctrl+k" }, a }); ``` -Hunk draws the dialog; your text fills the title, body, and choices. Dialogs from installed extensions carry an `ext ` attribution line — the same marker `notify` toasts use — so a third-party prompt cannot present itself as Hunk asking. Hunk's own bundled extensions omit that redundant marker. +Hunk draws every frame and every confirm/select/input surface. Dialogs from installed extensions carry an `ext ` attribution line — the same marker `notify` toasts use. Hunk's own bundled extensions omit that redundant marker. + +One dialog shows at a time; concurrent requests queue in call order, across extensions. Escape cancels (`false` or `null`) or closes a component dialog. Enter accepts host-rendered interactive dialogs; component-dialog keys other than Escape reach the mounted surface. Confirm dialogs also answer to `y`/`n`, select dialogs to `↑`/`↓`, and host-rendered actions are clickable. A session reload cancels open and queued dialogs, and a dialog pending at shutdown resolves its cancel value. -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 @@ -340,6 +396,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 +412,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.