diff --git a/AGENTS.md b/AGENTS.md index 2cd2ffa..6abd1c9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,7 +40,7 @@ CLI input - CLI entrypoints: `diff`, `show`, `stash show`, `patch`, `pager`, `difftool`. - All input sources normalize into one internal changeset model. -- Pager mode has two paths: full diff UI for patch-like stdin, plain-text fallback for non-diff pager content. +- Pager mode resolves to one of four `StartupPlan` outcomes (the single enumeration of pager behavior): full diff UI for patch-like stdin on a usable TTY, plain-text fallback for non-diff pager content, a static ANSI diff for captured pager hosts (LazyGit etc.: `TERM=dumb` + a non-TTY pipe), and raw passthrough for any other non-TTY/dumb pipe. - View defaults are layered through built-ins, user config, repo `.dunk/config.toml`, command sections, pager sections, and CLI flags. - Review comments live in one committed `.dunk/comments.json` per repo. Watch mode picks up changes so a coding agent and a human reviewer can ping-pong on the same review. - Prefer one source of truth for each user-visible behavior. When rendering, navigation, scrolling, or note placement share the same model, derive them from the same planning layer rather than maintaining parallel implementations. diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ea1461..da9203c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and - `watch = true` in `.dunk/config.toml` (or `~/.config/dunk/config.toml`) now enables watch mode without passing `--watch` every time. The `--watch`/absence of it on the command line still overrides the config value. - Ctrl-Z suspends `dunk` as a normal shell job (the terminal is restored, the process group gets SIGTSTP, and `fg` resumes the renderer). Previously Ctrl-Z was swallowed in raw mode and `dunk` could not be suspended. No-op on Windows. - `,` and `.` jump to the previous / next file in the review stream (clamped at the ends), so you can skip whole files without stepping through every hunk. Shown in `?` help. +- Captured pager hosts (LazyGit, `git -c core.pager="dunk pager" log -p`, anything that pipes a custom pager into its own panel with `TERM=dumb`) now get a static, colored ANSI diff instead of a broken alt-screen attempt. Other non-TTY / dumb-terminal pipes echo the raw patch verbatim so the pager pipeline keeps working. ### Removed diff --git a/src/core/startup.test.ts b/src/core/startup.test.ts index 0d4a160..4861687 100644 --- a/src/core/startup.test.ts +++ b/src/core/startup.test.ts @@ -56,6 +56,10 @@ describe("startup planning", () => { parseCliImpl: async () => ({ kind: "pager", options: { theme: "paper" } }), readStdinText: async () => "diff --git a/a.ts b/a.ts\n@@ -1 +1 @@\n-old\n+new\n", looksLikePatchInputImpl: () => true, + // The interactive TUI path requires a usable TTY and a non-captured, + // non-dumb terminal; without these the pager now falls back instead. + stdoutIsTTY: true, + env: { TERM: "xterm-256color" }, resolveRuntimeCliInputImpl(input) { seenInputs.push(input); return input; @@ -88,6 +92,83 @@ describe("startup planning", () => { expect(seenInputs).toHaveLength(3); }); + test("renders a static diff for a captured pager host", async () => { + const plan = await prepareStartupPlan(["bun", "dunk", "pager"], { + parseCliImpl: async () => ({ kind: "pager", options: {} }), + readStdinText: async () => "diff --git a/a.ts b/a.ts\n@@ -1 +1 @@\n-old\n+new\n", + looksLikePatchInputImpl: () => true, + // LazyGit-style: TERM=dumb + GIT_PAGER, stdout is a non-TTY pipe. This + // must beat the generic non-TTY passthrough below. + stdoutIsTTY: false, + env: { TERM: "dumb", GIT_PAGER: "dunk" }, + resolveRuntimeCliInputImpl: (input) => input, + resolveConfiguredCliInputImpl: (input) => ({ input }) as never, + loadAppBootstrapImpl: async (input) => createBootstrap(input), + }); + + expect(plan.kind).toBe("static-diff-pager"); + if (plan.kind !== "static-diff-pager") { + throw new Error("Expected static-diff-pager plan."); + } + expect(plan.bootstrap.input).toMatchObject({ kind: "patch", file: "-" }); + }); + + test.each([ + { name: "LAZYGIT marker", env: { TERM: "dumb", LAZYGIT_STATE: "1" } }, + { name: "GIT_PAGER", env: { TERM: "dumb", GIT_PAGER: "dunk" } }, + { name: "lv filter mode", env: { TERM: "dumb", LV: "-c" } }, + ])("captured-host detection fires for $name", async ({ env }) => { + const plan = await prepareStartupPlan(["bun", "dunk", "pager"], { + parseCliImpl: async () => ({ kind: "pager", options: {} }), + readStdinText: async () => "diff --git a/a.ts b/a.ts\n@@ -1 +1 @@\n-old\n+new\n", + looksLikePatchInputImpl: () => true, + stdoutIsTTY: false, + env, + resolveRuntimeCliInputImpl: (input) => input, + resolveConfiguredCliInputImpl: (input) => ({ input }) as never, + loadAppBootstrapImpl: async (input) => createBootstrap(input), + }); + + expect(plan.kind).toBe("static-diff-pager"); + }); + + test("GIT_PAGER without TERM=dumb is not a captured host (no false positive)", async () => { + // A globally-exported GIT_PAGER must not force static output for a normal + // pager pipe; the TERM=dumb gate keeps interactive use on the TUI path. + const plan = await prepareStartupPlan(["bun", "dunk", "pager"], { + parseCliImpl: async () => ({ kind: "pager", options: {} }), + readStdinText: async () => "diff --git a/a.ts b/a.ts\n@@ -1 +1 @@\n-old\n+new\n", + looksLikePatchInputImpl: () => true, + stdoutIsTTY: false, + env: { TERM: "xterm-256color", GIT_PAGER: "dunk" }, + loadAppBootstrapImpl: async () => { + throw new Error("should not bootstrap"); + }, + }); + + expect(plan.kind).toBe("passthrough"); + }); + + test("echoes the raw patch when there is no usable TTY and no captured host", async () => { + const stdinText = "diff --git a/a.ts b/a.ts\n@@ -1 +1 @@\n-old\n+new\n"; + let bootstrapped = false; + + const plan = await prepareStartupPlan(["bun", "dunk", "pager"], { + parseCliImpl: async () => ({ kind: "pager", options: {} }), + readStdinText: async () => stdinText, + looksLikePatchInputImpl: () => true, + stdoutIsTTY: false, + env: { TERM: "xterm-256color" }, + loadAppBootstrapImpl: async () => { + bootstrapped = true; + throw new Error("should not bootstrap for passthrough"); + }, + }); + + expect(plan).toEqual({ kind: "passthrough", text: stdinText }); + expect(bootstrapped).toBe(false); + }); + test("rejects watch mode for stdin-backed patch inputs", async () => { const cliInput: CliInput = { kind: "patch", diff --git a/src/core/startup.ts b/src/core/startup.ts index edc1e3d..25d718f 100644 --- a/src/core/startup.ts +++ b/src/core/startup.ts @@ -20,6 +20,14 @@ export type StartupPlan = kind: "plain-text-pager"; text: string; } + | { + kind: "passthrough"; + text: string; + } + | { + kind: "static-diff-pager"; + bootstrap: AppBootstrap; + } | { kind: "app"; bootstrap: AppBootstrap; @@ -36,6 +44,28 @@ export interface StartupDeps { loadAppBootstrapImpl?: typeof loadAppBootstrap; usesPipedPatchInputImpl?: typeof usesPipedPatchInput; openControllingTerminalImpl?: typeof openControllingTerminal; + stdoutIsTTY?: boolean; + env?: NodeJS.ProcessEnv; +} + +/** + * Detect a captured pager host (LazyGit, `git -c core.pager=dunk log -p`) + * that pipes our output into its own panel while advertising `TERM=dumb`. + * Such hosts give a non-TTY stdout, so this must be checked before the + * generic non-TTY passthrough or the static path would never fire for them. + */ +function isCapturedPagerHost(env: NodeJS.ProcessEnv) { + // Mirrors upstream hunk's captured-host signal set, all gated on TERM=dumb: + // `LV=-c` (the `lv` pager invoked in filtered/captured mode), `GIT_PAGER` + // (git driving us as a custom pager, e.g. `git -c core.pager=dunk log -p`), + // and any `LAZYGIT*` var (LazyGit's diff panel). Misses degrade safely to + // passthrough; a false positive would need TERM=dumb on a real TTY. + return ( + env.TERM === "dumb" && + (env.LV === "-c" || + Boolean(env.GIT_PAGER) || + Object.keys(env).some((key) => key.startsWith("LAZYGIT"))) + ); } /** @@ -51,6 +81,7 @@ export async function prepareStartupPlan( ): Promise { const parseCliImpl = deps.parseCliImpl ?? parseCli; let parsedCliInput = await parseCliImpl(argv); + let staticPager = false; if (parsedCliInput.kind === "help") { return { @@ -73,6 +104,22 @@ export async function prepareStartupPlan( }; } + const env = deps.env ?? process.env; + const stdoutIsTTY = deps.stdoutIsTTY ?? Boolean(process.stdout.isTTY); + const capturedPagerHost = isCapturedPagerHost(env); + + // Known captured hosts get a static ANSI render even though their stdout + // is a non-TTY pipe — so this check must precede the generic passthrough. + // Anything else without a usable TTY (or a dumb terminal) just echoes the + // raw patch so the pager pipeline keeps working. + if (!capturedPagerHost && (!stdoutIsTTY || env.TERM === "dumb")) { + return { + kind: "passthrough", + text: stdinText, + }; + } + staticPager = capturedPagerHost; + parsedCliInput = { kind: "patch", file: "-", @@ -116,6 +163,14 @@ export async function prepareStartupPlan( } const bootstrap = await loadAppBootstrapImpl(cliInput); + + if (staticPager) { + return { + kind: "static-diff-pager", + bootstrap, + }; + } + const controllingTerminal = usesPipedPatchInputImpl(cliInput) ? openControllingTerminalImpl() : null; diff --git a/src/main.tsx b/src/main.tsx index 55fb84f..22a3363 100644 --- a/src/main.tsx +++ b/src/main.tsx @@ -18,6 +18,22 @@ async function main() { process.exit(0); } + if (startupPlan.kind === "passthrough") { + // Captured non-TTY pipe we can't usefully render in: echo the patch back + // verbatim so the host's pager pipeline keeps working. (UTF-8 text — the + // same assumption as the plain-text pager path.) + await Bun.write(Bun.stdout, startupPlan.text); + process.exit(0); + } + + if (startupPlan.kind === "static-diff-pager") { + const { renderStaticDiffPager } = await import("./ui/staticDiffPager"); + // Bun.write drains before resolving, so the full diff reaches the host + // before exit (process.exit would otherwise truncate a large diff). + await Bun.write(Bun.stdout, renderStaticDiffPager(startupPlan.bootstrap)); + process.exit(0); + } + if (startupPlan.kind !== "app") { throw new Error("Unreachable startup plan."); } diff --git a/src/ui/staticDiffPager.test.ts b/src/ui/staticDiffPager.test.ts new file mode 100644 index 0000000..5023fc8 --- /dev/null +++ b/src/ui/staticDiffPager.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, test } from "bun:test"; +import type { AppBootstrap, CliInput, DiffFile } from "../core/types"; +import { createTestDiffFile, createTestAnnotations } from "../../test/helpers/diff-helpers"; +import { renderStaticDiffPager, staticStackPalette } from "./staticDiffPager"; +import { resolveTheme, THEMES } from "./themes"; + +const PATCH_INPUT: CliInput = { kind: "patch", file: "-", options: {} }; + +function ansiBg(hex: string) { + const value = hex.replace(/^#/, ""); + const r = Number.parseInt(value.slice(0, 2), 16); + const g = Number.parseInt(value.slice(2, 4), 16); + const b = Number.parseInt(value.slice(4, 6), 16); + return `\x1b[48;2;${r};${g};${b}m`; +} + +function bootstrapFor(files: DiffFile[], overrides: Partial = {}): AppBootstrap { + return { + input: PATCH_INPUT, + changeset: { id: "test", sourceLabel: "test", title: "test", files }, + initialMode: "stack", + initialTheme: "graphite", + ...overrides, + }; +} + +describe("renderStaticDiffPager", () => { + test("renders the file path, both diff sides, and the hunk header", () => { + const file = createTestDiffFile({ context: 1 }); + const output = renderStaticDiffPager(bootstrapFor([file])); + + expect(output).toContain("example.ts"); + expect(output).toContain("@@"); + expect(output).toContain("alpha = 10"); // added side + expect(output).toContain("alpha = 1;"); // removed side + }); + + test("colors come from the resolved theme, not hardcoded values", () => { + const file = createTestDiffFile({ context: 0 }); + const output = renderStaticDiffPager(bootstrapFor([file])); + const theme = resolveTheme("graphite", null); + + // An added line must carry the theme's addition background, proving the + // static mapping reads the same AppTheme fields as the interactive + // renderer's stackCellPalette rather than inventing its own palette. + expect(output).toContain(ansiBg(theme.addedBg)); + expect(output).toContain(ansiBg(theme.removedBg)); + }); + + test("omits hunk headers when disabled", () => { + const file = createTestDiffFile({ context: 0 }); + const withHeaders = renderStaticDiffPager(bootstrapFor([file])); + const withoutHeaders = renderStaticDiffPager( + bootstrapFor([file], { initialShowHunkHeaders: false }), + ); + + expect(withHeaders).toContain("@@"); + expect(withoutHeaders).not.toContain("@@"); + }); + + test("never renders the comment overlay (pager content is transient)", () => { + const file = createTestDiffFile({ annotations: createTestAnnotations("example.ts") }); + const output = renderStaticDiffPager(bootstrapFor([file])); + + expect(file.annotations.length).toBeGreaterThan(0); + expect(output).not.toContain("Annotation for example.ts"); + }); + + test("renders an empty changeset without throwing", () => { + expect(renderStaticDiffPager(bootstrapFor([]))).toBe("\n"); + }); + + // Enforces the "mirrors stackCellPalette" invariant as a test, not just a + // comment: the static palette must select the exact AppTheme fields the + // interactive renderer uses, for every built-in theme. + test("the static palette reads the same theme fields across all built-in themes", () => { + for (const theme of THEMES) { + expect(staticStackPalette("addition", theme)).toEqual({ + contentBg: theme.addedBg, + signColor: theme.addedSignColor, + }); + expect(staticStackPalette("deletion", theme)).toEqual({ + contentBg: theme.removedBg, + signColor: theme.removedSignColor, + }); + expect(staticStackPalette("context", theme)).toEqual({ + contentBg: theme.contextBg, + signColor: theme.muted, + }); + } + }); + + // The ANSI serializer only colors strict 6-hex values; a theme using + // shorthand/8-digit would silently render colorless. Pin every consumed + // field across all built-in themes. + test("every theme color the static pager consumes is a 6-digit hex value", () => { + const sixHex = /^#[0-9a-f]{6}$/i; + for (const theme of THEMES) { + for (const field of [ + theme.addedBg, + theme.removedBg, + theme.contextBg, + theme.addedSignColor, + theme.removedSignColor, + theme.muted, + theme.lineNumberFg, + theme.lineNumberBg, + theme.badgeNeutral, + theme.panelAlt, + theme.text, + ]) { + expect(field).toMatch(sixHex); + } + } + }); +}); diff --git a/src/ui/staticDiffPager.ts b/src/ui/staticDiffPager.ts new file mode 100644 index 0000000..6d331cf --- /dev/null +++ b/src/ui/staticDiffPager.ts @@ -0,0 +1,147 @@ +/** + * Non-interactive `dunk pager` renderer for captured pager hosts. + * + * dunk's normal pager integration is a full-screen interactive TUI: Git pipes + * patch text on stdin and dunk opens the controlling terminal for input. Tools + * like LazyGit instead invoke a custom pager inside their own panel and + * advertise a constrained environment (`TERM=dumb`, a non-TTY pipe). Launching + * the TUI there corrupts the host panel or produces no usable output. + * + * This module is the fallback output adapter for those contexts. It is a thin + * serializer: it reuses dunk's normal parse/plan stack (`loadAppBootstrap` → + * Pierre `buildStackRows`) and only turns the resulting stack rows into ANSI + * text. It deliberately does NOT introduce a second diff parser or review + * model, does not render the comment overlay (pager content is transient), and + * skips syntax highlighting so a host that re-spawns the pager on every + * selection stays snappy. All colors come from `themes.ts` — the single source + * of truth shared with the interactive renderer (see `stackCellPalette` in + * `src/ui/diff/renderRows.tsx`, which this mirrors for the static path). + * + * Known fidelity gap vs. the TUI: a fully blank added/removed line has no + * spans, so its add/remove background does not extend across the row here. + * Diff intent is still readable from the colored sign column. + */ +import type { AppBootstrap } from "../core/types"; +import { buildStackRows, type DiffRow, type RenderSpan } from "./diff/pierre"; +import { resolveTheme, type AppTheme } from "./themes"; + +const RESET = "\x1b[0m"; + +/** Build one ANSI truecolor SGR code from a six-digit hex color. */ +function ansiColor(channel: "fg" | "bg", hex: string | undefined) { + const normalized = hex?.replace(/^#/, ""); + if (!normalized || !/^[0-9a-f]{6}$/i.test(normalized)) { + return ""; + } + + const red = Number.parseInt(normalized.slice(0, 2), 16); + const green = Number.parseInt(normalized.slice(2, 4), 16); + const blue = Number.parseInt(normalized.slice(4, 6), 16); + return `\x1b[${channel === "fg" ? 38 : 48};2;${red};${green};${blue}m`; +} + +/** Wrap one text fragment in foreground/background ANSI colors. */ +function colorText(text: string, fg?: string, bg?: string) { + if (!text) { + return ""; + } + const prefix = `${ansiColor("fg", fg)}${ansiColor("bg", bg)}`; + return prefix ? `${prefix}${text}${RESET}` : text; +} + +/** Serialize highlighted code spans, falling back to the row background. */ +function serializeSpans(spans: RenderSpan[], rowBg: string) { + return spans.map((span) => colorText(span.text, span.fg, span.bg ?? rowBg)).join(""); +} + +/** + * Stack-view colors per diff cell kind. Mirrors `stackCellPalette` in + * `renderRows.tsx`; both must read the same `AppTheme` fields so the static + * pager and the interactive renderer never drift apart. + */ +export function staticStackPalette(kind: "context" | "addition" | "deletion", theme: AppTheme) { + if (kind === "addition") { + return { contentBg: theme.addedBg, signColor: theme.addedSignColor }; + } + if (kind === "deletion") { + return { contentBg: theme.removedBg, signColor: theme.removedSignColor }; + } + return { contentBg: theme.contextBg, signColor: theme.muted }; +} + +const RAIL_MARKER = "▌"; + +/** Render a header-like row (collapsed gap / hunk header) as ANSI text. */ +function renderHeaderLikeRow(text: string, fg: string, theme: AppTheme) { + return ( + colorText(RAIL_MARKER, theme.lineNumberFg, theme.panelAlt) + + colorText(text.trimEnd(), fg, theme.panelAlt) + ); +} + +/** Render one stacked diff row as ANSI text, or null to skip it. */ +function renderStaticRow( + row: DiffRow, + theme: AppTheme, + showLineNumbers: boolean, + showHunkHeaders: boolean, +): string | null { + if (row.type === "collapsed") { + return renderHeaderLikeRow(`··· ${row.text} ···`, theme.muted, theme); + } + + if (row.type === "hunk-header") { + return showHunkHeaders ? renderHeaderLikeRow(row.text, theme.badgeNeutral, theme) : null; + } + + if (row.type !== "stack-line") { + // Split rows never reach the static pager (it always builds stack rows). + return null; + } + + const { cell } = row; + const palette = staticStackPalette(cell.kind, theme); + const rail = colorText(RAIL_MARKER, palette.signColor, palette.contentBg); + + let gutter = ""; + if (showLineNumbers) { + const oldNumber = cell.oldLineNumber ? String(cell.oldLineNumber) : ""; + const newNumber = cell.newLineNumber ? String(cell.newLineNumber) : ""; + gutter = colorText( + ` ${oldNumber.padStart(5)} ${newNumber.padStart(5)} `, + theme.lineNumberFg, + theme.lineNumberBg, + ); + } + + const sign = colorText(`${cell.sign || " "} `, palette.signColor, palette.contentBg); + const content = serializeSpans(cell.spans, palette.contentBg); + return `${rail}${gutter}${sign}${content}`; +} + +/** + * Render a resolved bootstrap as a static ANSI diff for a captured pager host. + * Reuses the normal parse/plan path; never throws past the caller's fallback. + */ +export function renderStaticDiffPager(bootstrap: AppBootstrap): string { + const theme = resolveTheme(bootstrap.initialTheme ?? "graphite", null); + const showLineNumbers = bootstrap.initialShowLineNumbers ?? false; + const showHunkHeaders = bootstrap.initialShowHunkHeaders ?? true; + + const lines: string[] = []; + for (const file of bootstrap.changeset.files) { + lines.push(colorText(` ${file.path} `, theme.text, theme.panelAlt)); + // Static output skips syntax highlighting (null): a captured host + // re-spawns the pager per selection, so per-invocation WASM init would + // dominate. Diff intent still reads from add/remove backgrounds. + for (const row of buildStackRows(file, null, theme)) { + const rendered = renderStaticRow(row, theme, showLineNumbers, showHunkHeaders); + if (rendered !== null) { + lines.push(rendered); + } + } + lines.push(""); + } + + return `${lines.join("\n")}\n`; +} diff --git a/test/cli/entrypoint.test.ts b/test/cli/entrypoint.test.ts index 62d521b..d5c8ff0 100644 --- a/test/cli/entrypoint.test.ts +++ b/test/cli/entrypoint.test.ts @@ -224,4 +224,48 @@ describe("CLI entrypoint contracts", () => { rmSync(repoDir, { recursive: true, force: true }); } }); + + const SAMPLE_PATCH = + "diff --git a/a.ts b/a.ts\n" + + "index 0000000..1111111 100644\n" + + "--- a/a.ts\n" + + "+++ b/a.ts\n" + + "@@ -1,2 +1,2 @@\n" + + "-const old = 1;\n" + + "+const renewed = 2;\n" + + " const stable = true;\n"; + + test("a captured pager host gets a static ANSI diff, not a TUI takeover", () => { + const proc = Bun.spawnSync([bunExecutable, "run", "src/main.tsx", "pager"], { + cwd: process.cwd(), + stdin: Buffer.from(SAMPLE_PATCH), + stdout: "pipe", + stderr: "pipe", + env: { ...process.env, TERM: "dumb", GIT_PAGER: "dunk" }, + }); + + const stdout = Buffer.from(proc.stdout).toString("utf8"); + + expect(proc.exitCode).toBe(0); + expect(stdout).not.toContain("[?1049h"); // never enters the alt screen + expect(stdout).toContain("[48;2;"); // truecolor backgrounds from the theme + expect(stdout).toContain("const renewed = 2;"); + expect(stdout).toContain("const old = 1;"); + }); + + test("a plain non-TTY pipe echoes the raw patch verbatim", () => { + const proc = Bun.spawnSync([bunExecutable, "run", "src/main.tsx", "pager"], { + cwd: process.cwd(), + stdin: Buffer.from(SAMPLE_PATCH), + stdout: "pipe", + stderr: "pipe", + env: { ...process.env, TERM: "xterm-256color", GIT_PAGER: undefined }, + }); + + const stdout = Buffer.from(proc.stdout).toString("utf8"); + + expect(proc.exitCode).toBe(0); + expect(stdout).toBe(SAMPLE_PATCH); + expect(stdout).not.toContain("[?1049h"); + }); });