diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 929ee361ea..60eb9e88a6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -3,6 +3,7 @@ ## [Unreleased] - Added `app.edits.expand` (`ctrl+j`) to toggle edit diffs; diffs are now shown only by this toggle, and `ctrl+o` no longer affects them. +- Changed edit rendering so the `╰─ +N -M` summary line is always visible and `ctrl+j` toggles the diff inline beneath it, indented to the summary text. - Fixed fullscreen wheel scrolling in Ghostty while retaining application link clicks; set `terminal.fullscreenMouse` to `false` to use native Cmd-click instead. - Changed the agents view to sort idle and inactive sessions by last message time, newest first, while keeping running agents in stable creation order. - Fixed `openai-codex` models being invisible to `rlm` subagents and `find_models` because model discovery reported Prime Agent's own version as the Codex client version ([#1375](https://github.com/PrimeIntellect-ai/prime-agent/pull/1375) by [@bilelrais](https://github.com/bilelrais)). diff --git a/packages/coding-agent/src/core/tools/edit.ts b/packages/coding-agent/src/core/tools/edit.ts index 229ea1cb04..e0eb0502f0 100644 --- a/packages/coding-agent/src/core/tools/edit.ts +++ b/packages/coding-agent/src/core/tools/edit.ts @@ -1,10 +1,14 @@ import type { AgentTool } from "@earendil-works/pi-agent-core"; -import { Box, Container, Spacer, Text } from "@earendil-works/pi-tui"; +import { Box, type Component, Container, Spacer, Text, wrapTextWithAnsi } from "@earendil-works/pi-tui"; import { constants } from "fs"; import { access as fsAccess, readFile as fsReadFile, writeFile as fsWriteFile } from "fs/promises"; import { type Static, Type } from "typebox"; import { renderDiff } from "../../modes/interactive/components/diff.js"; -import { expandCollapseHint } from "../../modes/interactive/components/keybinding-hints.js"; +import { + countChangedLines, + FILE_CHANGE_DIFF_INDENT, + formatFileChangeSummaryLine, +} from "../../modes/interactive/components/edit-summary.js"; import type { ToolDefinition } from "../extensions/types.js"; import { applyEditsToNormalizedContent, @@ -141,7 +145,6 @@ type EditCallRenderComponent = Box & { previewArgsKey?: string; previewPending?: boolean; settledError?: boolean; - resultSettled?: boolean; }; function createEditCallRenderComponent(): EditCallRenderComponent { @@ -150,7 +153,6 @@ function createEditCallRenderComponent(): EditCallRenderComponent { previewArgsKey: undefined as string | undefined, previewPending: false, settledError: false, - resultSettled: false, }); } @@ -238,50 +240,82 @@ function getEditHeaderBg( settledError: boolean | undefined, theme: typeof import("../../modes/interactive/theme/theme.js").theme, ): (text: string) => string { + if (settledError || (preview && "error" in preview)) { + return (text: string) => theme.bg("toolErrorBg", text); + } if (preview) { - if ("error" in preview) { - return (text: string) => theme.bg("toolErrorBg", text); - } return (text: string) => theme.bg("toolSuccessBg", text); } - if (settledError) { - return (text: string) => theme.bg("toolErrorBg", text); - } return (text: string) => theme.bg("toolPendingBg", text); } +// Width-aware `╰─ +N -M` summary plus optional indented diff rows: the +// summary truncates to one row and wrapped diff lines keep the indent column. +class EditChangeSummaryComponent implements Component { + constructor( + private readonly rawPath: string, + private readonly cwd: string, + private readonly change: { added: number; removed: number }, + private readonly diffsExpanded: boolean | undefined, + private readonly diffLines: readonly string[] | undefined, + ) {} + + render(width: number): string[] { + const safeWidth = Math.max(1, width); + const lines = [formatFileChangeSummaryLine(this.rawPath, this.cwd, this.change, this.diffsExpanded, safeWidth)]; + if (this.diffLines !== undefined) { + const indent = FILE_CHANGE_DIFF_INDENT.slice(0, Math.max(0, safeWidth - 1)); + const contentWidth = Math.max(1, safeWidth - indent.length); + for (const line of this.diffLines) { + for (const row of wrapTextWithAnsi(line, contentWidth)) { + lines.push(`${indent}${row}`); + } + } + } + return lines; + } + + invalidate(): void {} +} + function buildEditCallComponent( component: EditCallRenderComponent, args: RenderableEditArgs | undefined, theme: typeof import("../../modes/interactive/theme/theme.js").theme, expanded: boolean, - showExpandHint: boolean, + cwd: string, ): EditCallRenderComponent { component.setBgFn(getEditHeaderBg(component.preview, component.settledError, theme)); component.clear(); - const canExpand = component.preview !== undefined && !("error" in component.preview); - // Collapsed rows normally carry the ctrl+j hint on the `╰─ path +N -M` - // summary line instead of the header — but that summary only mounts after a - // settled successful result. Until then (preview-only) and on error rows the - // header keeps the hint, so an expandable diff always advertises the key. - const hasSummaryLine = component.resultSettled === true; - const expandHint = - canExpand && showExpandHint && (expanded || !hasSummaryLine) - ? `${theme.fg("dim", " · ")}${expandCollapseHint("app.edits.expand", expanded)}` - : ""; - component.addChild(new Text(`${formatEditCall(args, theme)}${expandHint}`, 0, 0)); - - const body = - component.preview && - ("error" in component.preview - ? theme.fg("error", component.preview.error) - : expanded - ? renderDiff(component.preview.diff) - : undefined); - if (body) { + component.addChild(new Text(formatEditCall(args, theme), 0, 0)); + + if (component.preview && "error" in component.preview) { component.addChild(new Spacer(1)); - component.addChild(new Text(body, 0, 0)); + component.addChild(new Text(theme.fg("error", component.preview.error), 0, 0)); + return component; } + // A failed execution must not present the predicted diff as applied changes. + if (!component.preview || component.settledError) { + return component; + } + + // The `╰─ +N -M` summary line renders in both states; ctrl+j only + // attaches or removes the indented diff lines underneath it. + const rawPath = str(args?.file_path ?? args?.path); + const change = countChangedLines(component.preview.diff); + component.addChild(new Spacer(1)); + component.addChild( + new EditChangeSummaryComponent( + rawPath ?? "...", + cwd, + change, + // The ctrl+j hint renders on every edit summary row (unlike the ctrl+o + // hint, which the latest tool row owns), matching thinking and + // agent-message hints. + expanded, + expanded ? renderDiff(component.preview.diff).split("\n") : undefined, + ), + ); return component; } @@ -456,7 +490,7 @@ export function createEditToolDefinition( }); } - return buildEditCallComponent(component, args, theme, context.expanded, context.showExpandHint !== false); + return buildEditCallComponent(component, args, theme, context.expanded, context.cwd); }, renderResult(result, _options, theme, context) { const callComponent = context.state.callComponent; @@ -480,20 +514,13 @@ export function createEditToolDefinition( callComponent.settledError = context.isError; changed = true; } - // Mirrors the FileChangeSummaryComponent mount condition: any result - // with a countable diff and no error mounts the summary line. - const summaryMounts = !context.isError && typeof resultDiff === "string"; - if (callComponent.resultSettled !== summaryMounts) { - callComponent.resultSettled = summaryMounts; - changed = true; - } if (changed) { buildEditCallComponent( callComponent, context.args as RenderableEditArgs | undefined, theme, context.expanded, - context.showExpandHint !== false, + context.cwd, ); } } diff --git a/packages/coding-agent/src/modes/interactive/components/edit-summary.ts b/packages/coding-agent/src/modes/interactive/components/edit-summary.ts index 97c03c1e62..dacf64e261 100644 --- a/packages/coding-agent/src/modes/interactive/components/edit-summary.ts +++ b/packages/coding-agent/src/modes/interactive/components/edit-summary.ts @@ -1,7 +1,7 @@ import { isAbsolute } from "node:path"; import type { AgentMessage } from "@earendil-works/pi-agent-core"; import type { ToolResultMessage } from "@earendil-works/pi-ai"; -import { type Component, truncateToWidth, visibleWidth } from "@earendil-works/pi-tui"; +import { truncateToWidth, visibleWidth } from "@earendil-works/pi-tui"; import type { EditToolDetails } from "../../../core/tools/edit.js"; import { generateDiffString } from "../../../core/tools/edit-diff.js"; import type { IpythonToolDetails } from "../../../core/tools/ipython.js"; @@ -16,7 +16,7 @@ export interface FileChangeSummary { removed: number; } -function countChangedLines(diff: string): { added: number; removed: number } { +export function countChangedLines(diff: string): { added: number; removed: number } { let added = 0; let removed = 0; for (const line of diff.split("\n")) { @@ -80,7 +80,12 @@ export function mergeTurnFileChanges( } } -function counts(change: Pick): string { +/** Dim gutter that anchors every per-file change summary line. */ +const FILE_CHANGE_SUMMARY_PREFIX = " ╰─ "; +/** Indent that aligns diff rows with the summary line's text column. */ +export const FILE_CHANGE_DIFF_INDENT = " ".repeat(visibleWidth(FILE_CHANGE_SUMMARY_PREFIX)); + +function formatChangeCounts(change: Pick): string { return `${theme.fg("toolDiffAdded", `+${change.added}`)} ${theme.fg("toolDiffRemoved", `-${change.removed}`)}`; } @@ -91,29 +96,33 @@ function formatFileChangePath(path: string, cwd: string): string { return formatPathRelativeToCwdOrAbsolute(canonicalizePath(resolvedPath), canonicalizePath(cwd)); } -export class FileChangeSummaryComponent implements Component { - constructor( - private readonly changes: readonly FileChangeSummary[], - private readonly cwd: string, - private readonly diffsExpanded?: boolean, - ) {} - - render(width: number): string[] { - const safeWidth = Math.max(1, width); - const prefix = theme.fg("dim", " ╰─ "); - const hint = - this.diffsExpanded === undefined - ? "" - : `${theme.fg("dim", " · ")}${expandCollapseHint("app.edits.expand", this.diffsExpanded)}`; - return this.changes.map((change, index) => { - const suffix = `${theme.fg("dim", " ")}${counts(change)}${index === this.changes.length - 1 ? hint : ""}`; - const available = Math.max(1, safeWidth - visibleWidth(prefix) - visibleWidth(suffix)); - const path = truncateToWidth(formatFileChangePath(change.path, this.cwd), available, "…"); - return truncateToWidth(`${prefix}${theme.fg("muted", path)}${suffix}`, safeWidth, ""); - }); - } - - invalidate(): void {} +/** + * One ` ╰─ +N -M` row, truncated to width; the path renders relative + * to cwd where possible and the hint renders only when diffsExpanded is defined. + */ +export function formatFileChangeSummaryLine( + rawPath: string, + cwd: string | undefined, + change: Pick, + diffsExpanded: boolean | undefined, + width: number, +): string { + const prefix = theme.fg("dim", FILE_CHANGE_SUMMARY_PREFIX); + const hint = + diffsExpanded === undefined + ? "" + : `${theme.fg("dim", " · ")}${expandCollapseHint("app.edits.expand", diffsExpanded)}`; + // Size the path against the wider hint variant ("to collapse") so toggling + // ctrl+j never re-truncates it — the summary line is a stable anchor. + const widestHint = + diffsExpanded === undefined ? "" : `${theme.fg("dim", " · ")}${expandCollapseHint("app.edits.expand", true)}`; + const counts = `${theme.fg("dim", " ")}${formatChangeCounts(change)}`; + const suffix = `${counts}${hint}`; + const safeWidth = Math.max(1, width); + const available = Math.max(1, safeWidth - visibleWidth(prefix) - visibleWidth(counts) - visibleWidth(widestHint)); + const displayPath = cwd === undefined ? rawPath : formatFileChangePath(rawPath, cwd); + const path = truncateToWidth(displayPath, available, "…"); + return truncateToWidth(`${prefix}${theme.fg("muted", path)}${suffix}`, safeWidth, ""); } export function formatTotalChangeSummary(changes: readonly FileChangeSummary[]): string { @@ -122,5 +131,5 @@ export function formatTotalChangeSummary(changes: readonly FileChangeSummary[]): { added: 0, removed: 0 }, ); const files = `${changes.length} file${changes.length === 1 ? "" : "s"} changed`; - return `${theme.fg("muted", files)}${theme.fg("dim", " | ")}${counts(totals)}`; + return `${theme.fg("muted", files)}${theme.fg("dim", " | ")}${formatChangeCounts(totals)}`; } diff --git a/packages/coding-agent/src/modes/interactive/components/ipython-cell.ts b/packages/coding-agent/src/modes/interactive/components/ipython-cell.ts index 98986ab16e..68a8e50396 100644 --- a/packages/coding-agent/src/modes/interactive/components/ipython-cell.ts +++ b/packages/coding-agent/src/modes/interactive/components/ipython-cell.ts @@ -1,4 +1,3 @@ -import { isAbsolute, relative } from "node:path"; import { type Component, truncateToWidth, @@ -10,12 +9,12 @@ import { formatAgentMessageParticipant } from "../../../core/agent-messages.js"; import { previewIpythonCode } from "../../../core/tools/code-preview.js"; import { generateDiffString } from "../../../core/tools/edit-diff.js"; import { parseIpythonBashCell } from "../../../core/tools/ipython-cell-code.js"; -import { shortenPath } from "../../../core/tools/render-utils.js"; import { getLanguageFromPath, highlightCode, theme } from "../theme/theme.js"; import { getWorkingPulseFrame, WORKING_ICON_FRAMES, workingIconFrame } from "../theme/working-icon.js"; import { agentMessageBodyLines, agentMessagePreview, agentMessageSummaryLine } from "./agent-message.js"; import { normalizeErrorDetails, summarizeErrorDetails } from "./collapsible-error.js"; import { renderDiffSeparator, renderRichDiff } from "./diff.js"; +import { countChangedLines, FILE_CHANGE_DIFF_INDENT, formatFileChangeSummaryLine } from "./edit-summary.js"; import { expandCollapseHint } from "./keybinding-hints.js"; export interface IPythonCellContentBlock { @@ -285,18 +284,6 @@ function formatDuration(durationMs: number | undefined): string | undefined { return `${(durationMs / 1000).toFixed(1)}s`; } -// Relative to the session cwd when nested under it, else the absolute path. -function displayEditPath(path: string, cwd: string | undefined): string { - if (cwd && isAbsolute(path)) { - const rel = relative(cwd, path); - if (rel && !rel.startsWith("..") && !isAbsolute(rel)) { - return rel; - } - return shortenPath(path); - } - return path; -} - function isImageBlock(block: IPythonCellContentBlock): boolean { return block.type === "image" && typeof block.data === "string" && typeof block.mimeType === "string"; } @@ -384,8 +371,8 @@ export class IPythonCellComponent implements Component { const lines = [truncateToWidth(` ${this.collapsedLine(details)}`, safeWidth, "")]; const hasCode = this.state.expanded ? this.renderCode(lines, safeWidth) : false; - if ((details.diffs?.length ?? 0) > 0 && this.state.editDiffsExpanded) { - this.renderDiffs(lines, safeWidth, details.diffs ?? [], this.marker(details)); + if ((details.diffs?.length ?? 0) > 0) { + this.renderDiffs(lines, safeWidth, details.diffs ?? [], hasCode); } if ((details.sentAgentMessages?.length ?? 0) > 0) { this.renderSentAgentMessages(lines, safeWidth, details.sentAgentMessages ?? []); @@ -429,11 +416,6 @@ export class IPythonCellComponent implements Component { if (this.state.showExpandHint !== false) { parts.push(expandCollapseHint("app.tools.expand", this.state.expanded === true)); - // Expanded diffs replace the summary line that normally carries the - // ctrl+j cue, so the header advertises the collapse key instead. - if (this.state.editDiffsExpanded && (details.diffs?.length ?? 0) > 0) { - parts.push(expandCollapseHint("app.edits.expand", true)); - } } return parts.join(theme.fg("dim", " · ")); } @@ -670,16 +652,22 @@ export class IPythonCellComponent implements Component { } } - private renderDiffs(lines: string[], width: number, diffs: readonly DiffDisplay[], marker: string): void { + // The `╰─ +N -M` summary line renders in both states; ctrl+j only + // attaches or removes the indented diff rows underneath it. + private renderDiffs(lines: string[], width: number, diffs: readonly DiffDisplay[], hasCode: boolean): void { const diffsByPath = new Map(); for (const diff of diffs) { const existing = diffsByPath.get(diff.path); if (existing) existing.push(diff); else diffsByPath.set(diff.path, [diff]); } - for (const [path, edits] of diffsByPath) { + if (hasCode) { this.addPlain(lines, ""); - this.renderFileDiff(lines, width, path, edits, marker); + } + let index = 0; + for (const [path, edits] of diffsByPath) { + index += 1; + this.renderFileDiff(lines, width, path, edits, index === diffsByPath.size); } } @@ -688,33 +676,37 @@ export class IPythonCellComponent implements Component { width: number, path: string, edits: readonly DiffDisplay[], - marker: string, + showHint: boolean, ): void { const language = getLanguageFromPath(path); + // Diff rows align with the summary line's text column (after the `╰─ ` gutter). + const indent = FILE_CHANGE_DIFF_INDENT.slice(0, Math.max(0, width - 1)); + const contentWidth = Math.max(1, width - indent.length); let added = 0; let removed = 0; const rows: string[] = []; edits.forEach((edit, index) => { const { diff: diffText } = generateDiffString(edit.oldStr, edit.newStr, 4, edit.startLine ?? 1); - for (const row of diffText.split("\n")) { - if (row.startsWith("+")) added++; - else if (row.startsWith("-")) removed++; + const counts = countChangedLines(diffText); + added += counts.added; + removed += counts.removed; + if (!this.state.editDiffsExpanded) { + return; } if (index > 0) { - rows.push(renderDiffSeparator(width)); + rows.push(`${indent}${renderDiffSeparator(contentWidth)}`); } // Append, not spread: a huge edit's diff can exceed the JS arg-count limit. - for (const row of renderRichDiff(diffText, width, { language })) { - rows.push(row); + for (const row of renderRichDiff(diffText, contentWidth, { language })) { + rows.push(`${indent}${row}`); } }); - const counts = `${theme.fg("toolDiffAdded", `+${added}`)} ${theme.fg("toolDiffRemoved", `-${removed}`)}`; - const displayPath = displayEditPath(path, this.state.cwd); - // Truncate the path (not the counts) so it can't push the header past width. - const fixed = visibleWidth(marker) + 1 + 2 + visibleWidth(counts); - const shownPath = truncateToWidth(displayPath, Math.max(1, width - 1 - fixed), "…"); - this.addPlain(lines, `${marker} ${shownPath} ${counts}`); + // Unlike the ctrl+o hint (latest tool row only), the ctrl+j hint renders on + // every tool row, matching the thinking and agent-message hints. Within a + // row it renders once, on the last file's summary line (showHint). + const hint = showHint ? this.state.editDiffsExpanded === true : undefined; + lines.push(formatFileChangeSummaryLine(path, this.state.cwd, { added, removed }, hint, width)); for (const row of rows) { lines.push(row); diff --git a/packages/coding-agent/src/modes/interactive/components/tool-execution.ts b/packages/coding-agent/src/modes/interactive/components/tool-execution.ts index 0182bfbd42..3785b4c64d 100644 --- a/packages/coding-agent/src/modes/interactive/components/tool-execution.ts +++ b/packages/coding-agent/src/modes/interactive/components/tool-execution.ts @@ -9,7 +9,6 @@ import { getTextOutput as getRenderedTextOutput } from "../../../core/tools/rend import type { AgentConnectionToolDefinition } from "../../agent-connection/index.js"; import { type Theme, theme } from "../theme/theme.js"; import { getWorkingPulseFrame, workingIconFrame } from "../theme/working-icon.js"; -import { FileChangeSummaryComponent, getToolFileChanges } from "./edit-summary.js"; import { getIpythonCodeFromArgs, IPythonCellComponent } from "./ipython-cell.js"; import { ToolPanel } from "./tool-panel.js"; @@ -415,21 +414,6 @@ export class ToolExecutionComponent extends Container { } } - if (!this.editDiffsExpanded && this.result && (this.isBuiltInEditTool() || this.shouldUseIpythonRenderer())) { - const changes = getToolFileChanges(this.toolName, this.args, this.result, this.cwd); - if (changes.length > 0) { - const container = this.usesSelfRenderShell() ? this.selfRenderContainer : this.contentPanel; - container.addChild( - new FileChangeSummaryComponent( - changes, - this.cwd, - this.showExpandHint ? this.editDiffsExpanded : undefined, - ), - ); - hasContent = true; - } - } - if (this.hasRendererDefinition() && !hasContent && this.imageComponents.length === 0) { this.hideComponent = true; } diff --git a/packages/coding-agent/test/edit-summary.test.ts b/packages/coding-agent/test/edit-summary.test.ts index a5fb9ac27e..0f2aab8537 100644 --- a/packages/coding-agent/test/edit-summary.test.ts +++ b/packages/coding-agent/test/edit-summary.test.ts @@ -5,6 +5,7 @@ import type { AssistantMessage, ToolResultMessage, Usage } from "@earendil-works import stripAnsi from "strip-ansi"; import { beforeAll, describe, expect, test } from "vitest"; import { + formatFileChangeSummaryLine, formatTotalChangeSummary, getToolFileChanges, mergeTurnFileChanges, @@ -136,3 +137,19 @@ describe("edit summaries", () => { } }); }); + +describe("formatFileChangeSummaryLine", () => { + beforeAll(() => initTheme("dark")); + + test("keeps the truncated path stable when the ctrl+j hint flips", () => { + const change = { added: 3, removed: 1 }; + const path = "src/some/deeply/nested/directory/with-a-long-file-name.ts"; + const width = 44; + const pathPart = (line: string) => stripAnsi(line).replace(/\s*\+\d+ -\d+.*$/, ""); + const expanded = formatFileChangeSummaryLine(path, undefined, change, true, width); + const collapsed = formatFileChangeSummaryLine(path, undefined, change, false, width); + expect(stripAnsi(expanded)).toContain("…"); + // "to expand" vs "to collapse" differ in width; the path must not re-truncate. + expect(pathPart(expanded)).toBe(pathPart(collapsed)); + }); +}); diff --git a/packages/coding-agent/test/ipython-cell-diff.test.ts b/packages/coding-agent/test/ipython-cell-diff.test.ts index de525987d7..1492f4a0fb 100644 --- a/packages/coding-agent/test/ipython-cell-diff.test.ts +++ b/packages/coding-agent/test/ipython-cell-diff.test.ts @@ -1,3 +1,6 @@ +import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { visibleWidth } from "@earendil-works/pi-tui"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { renderRichDiff } from "../src/modes/interactive/components/diff.js"; @@ -118,29 +121,39 @@ describe("IPythonCellComponent diff rendering", () => { expect(diffRows.some(hasBackground)).toBe(true); }); - it("prefixes the header with the cell's status marker and aligns it with the summary line", () => { - const done = renderCell({ + it("always shows the summary line and indents diff rows to its text column", () => { + const state = { code: "await edit(...)", details: { status: "ok", diffs: [{ path: "a.ts", oldStr: "x", newStr: "X", startLine: 1 }] }, executionStarted: true, argsComplete: true, - expanded: true, - editDiffsExpanded: true, - }).split("\n"); - // Summary line and header share the same single-space indent. - expect(done[0]).toMatch(/^ ✓ python/); - expect(done.find((l) => l.includes("a.ts"))).toMatch(/^ ✓ a\.ts/); + expanded: false, + }; - const failed = renderCell({ - code: "await edit(...)", - details: { status: "error", diffs: [{ path: "a.ts", oldStr: "x", newStr: "X", startLine: 1 }] }, - executionStarted: true, - argsComplete: true, - expanded: true, - editDiffsExpanded: true, - isError: true, - }); - expect(failed).toMatch(/✗ a\.ts/); + const hidden = renderCell({ ...state, editDiffsExpanded: false }).split("\n"); + const hiddenSummary = hidden.find((l) => l.includes("╰─ a.ts")); + expect(hiddenSummary).toMatch(/^ {4}╰─ a\.ts \+1 -1 · .*to expand\)$/); + + // The ctrl+j hint is not owned by the latest tool row: it stays visible + // on rows whose ctrl+o hint is suppressed (showExpandHint=false). + const notLatest = renderCell({ ...state, editDiffsExpanded: false, showExpandHint: false }).split("\n"); + expect(notLatest.find((l) => l.includes("╰─ a.ts"))).toMatch(/to expand\)$/); + expect(hidden.some((l) => /1 - .*x/.test(l))).toBe(false); + + const shown = renderCell({ ...state, editDiffsExpanded: true }).split("\n"); + const summary = shown.find((l) => l.includes("╰─ a.ts")); + expect(summary).toMatch(/^ {4}╰─ a\.ts \+1 -1 · .*to collapse\)$/); + // Diff rows align with the summary's text column (after the ` ╰─ ` gutter). + const textColumn = (summary ?? "").indexOf("a.ts"); + const removed = shown.find((l) => /1 - .*x/.test(l)); + const added = shown.find((l) => /1 \+ .*X/.test(l)); + expect(removed).toBeDefined(); + expect(added).toBeDefined(); + for (const row of [removed ?? "", added ?? ""]) { + expect(row.startsWith(" ".repeat(textColumn))).toBe(true); + } + // Toggling only adds the diff rows underneath; the summary line is stable. + expect(shown.filter((l) => !/\d+ [-+ ] /.test(l) && !l.includes("⋮")).length).toBe(hidden.length); }); it("renders an edit path relative to the session cwd, or absolute when outside it", () => { @@ -169,6 +182,33 @@ describe("IPythonCellComponent diff rendering", () => { expect(outside).toContain("/etc/hosts"); }); + it("formats the summary path like the built-in edit tool, resolving symlinked cwds", () => { + const root = mkdtempSync(join(tmpdir(), "ipython-cell-diff-symlink-")); + try { + const realCwd = join(root, "real"); + const linkedCwd = join(root, "linked"); + mkdirSync(realCwd); + writeFileSync(join(realCwd, "same.ts"), "x"); + symlinkSync(realCwd, linkedCwd, "dir"); + const out = renderCell({ + code: "await edit(...)", + cwd: linkedCwd, + details: { + status: "ok", + diffs: [{ path: join(realpathSync(realCwd), "same.ts"), oldStr: "x", newStr: "X", startLine: 1 }], + }, + executionStarted: true, + argsComplete: true, + expanded: false, + editDiffsExpanded: false, + }); + // Same cwd-relative output the built-in edit tool's formatFileChangePath gives. + expect(out).toContain("╰─ same.ts +1 -1"); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + it("wraps a long diff line across rows without truncating or overflowing the width", () => { const width = 92; const longLine = `const x = ${Array.from({ length: 30 }, (_, i) => `arg${i}`).join(", ")};`; @@ -191,7 +231,7 @@ describe("IPythonCellComponent diff rendering", () => { expect(lines.filter((line) => /arg\d/.test(stripAnsi(line))).length).toBeGreaterThan(1); }); - it("truncates a long header path so it never overflows the width, keeping the counts", () => { + it("truncates a long summary path so it never overflows the width, keeping the counts", () => { const width = 40; const longPath = `src/${"very-long-directory-name/".repeat(8)}file.ts`; const lines = new IPythonCellComponent({ @@ -203,30 +243,35 @@ describe("IPythonCellComponent diff rendering", () => { editDiffsExpanded: true, }).render(width); expect(lines.every((line) => visibleWidth(line) <= width)).toBe(true); - const header = lines.map(stripAnsi).find((line) => line.includes("…")); - expect(header).toBeDefined(); - // The +/- counts survive truncation; only the path is shortened. - expect(header).toMatch(/\+1 -1/); + const summary = lines.map(stripAnsi).find((line) => line.includes("…")); + expect(summary).toBeDefined(); + // The +/- counts and hint survive truncation; only the path is shortened. + expect(summary).toMatch(/\+1 -1 · /); }); - it("advertises the collapse key on the cell header when diffs are expanded", () => { - const render = (editDiffsExpanded: boolean) => - new IPythonCellComponent({ - code: "await edit(...)", - details: { status: "ok", diffs: [{ path: "a.ts", oldStr: "x", newStr: "X", startLine: 1 }] }, - executionStarted: true, - argsComplete: true, - expanded: false, - editDiffsExpanded, - }).render(120); - const header = (lines: string[]) => stripAnsi(lines[0] ?? ""); - // Expanded diffs: the header carries the ctrl+j cue (the summary line is gone). - expect(header(render(true)).match(/to collapse/g)).toHaveLength(1); - // Collapsed diffs: no ctrl+j cue on the header (the summary line carries it). - expect(header(render(false))).not.toContain("to collapse"); + it("advertises the collapse key once per cell when diffs are expanded", () => { + const lines = new IPythonCellComponent({ + code: "await edit(...)", + details: { + status: "ok", + diffs: [ + { path: "a.ts", oldStr: "x", newStr: "X", startLine: 1 }, + { path: "b.ts", oldStr: "y", newStr: "Y", startLine: 1 }, + ], + }, + executionStarted: true, + argsComplete: true, + expanded: true, + editDiffsExpanded: true, + }).render(120); + const plain = lines.map(stripAnsi); + const hinted = plain.filter((line) => line.includes("to collapse") && /[+]\d+ -\d+/.test(line)); + // Exactly one file row (the last file's) carries the ctrl+j cue. + expect(hinted).toHaveLength(1); + expect(hinted[0]).toContain("b.ts"); }); - it("never overflows a narrow pane when expanded diffs add the header hint", () => { + it("never overflows a narrow pane when expanded diffs render", () => { const width = 24; const lines = new IPythonCellComponent({ code: "await edit(...)", @@ -289,7 +334,7 @@ describe("IPythonCellComponent diff rendering", () => { expect(out.findIndex((line) => line.includes("a.ts"))).toBeGreaterThan(2); }); - it("shows the full diff when collapsed", () => { + it("keeps the summary line but hides diff rows when edit diffs are collapsed", () => { const collapsed = renderCell({ code: "await edit(...)", details: { status: "ok", diffs: [{ path: "big.py", oldStr: "old", newStr: "NEW", startLine: 1 }] }, @@ -297,8 +342,8 @@ describe("IPythonCellComponent diff rendering", () => { argsComplete: true, expanded: false, }); + expect(collapsed).toContain("╰─ big.py +1 -1"); expect(collapsed).toContain("to expand"); - expect(collapsed).not.toContain("big.py"); expect(collapsed).not.toContain("old"); expect(collapsed).not.toContain("NEW"); }); @@ -319,7 +364,7 @@ describe("IPythonCellComponent diff rendering", () => { expect(expanded).toContain("a.py"); const expandedLines = expanded.split("\n"); expect(expandedLines.findIndex((line) => line.includes("hidden_side_effect ="))).toBeLessThan( - expandedLines.findIndex((line) => /✓ a\.py\s+\+1 -1/.test(line)), + expandedLines.findIndex((line) => /╰─ a\.py \+1 -1/.test(line)), ); }); diff --git a/packages/coding-agent/test/tool-execution-component.test.ts b/packages/coding-agent/test/tool-execution-component.test.ts index 57688465dd..7e00a2dbad 100644 --- a/packages/coding-agent/test/tool-execution-component.test.ts +++ b/packages/coding-agent/test/tool-execution-component.test.ts @@ -1,4 +1,5 @@ import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { Container, resetCapabilitiesCache, setCapabilities, Text, TUI } from "@earendil-works/pi-tui"; @@ -11,7 +12,7 @@ import { type BashOperations, createBashTool, createBashToolDefinition } from ". import { createEditToolDefinition } from "../src/core/tools/edit.js"; import { createAgentConnectionToolDefinition } from "../src/modes/agent-connection/tool-definition.js"; import { ToolExecutionComponent } from "../src/modes/interactive/components/tool-execution.js"; -import { initTheme } from "../src/modes/interactive/theme/theme.js"; +import { initTheme, theme } from "../src/modes/interactive/theme/theme.js"; import { getWorkingPulseFrame, workingIconFrame } from "../src/modes/interactive/theme/working-icon.js"; function createBaseToolDefinition(name = "custom_tool"): ToolDefinition { @@ -455,10 +456,16 @@ describe("ToolExecutionComponent parity", () => { expect(collapsed.split("to expand").length - 1).toBe(1); component.setEditDiffsExpanded(true); - const withDiffs = stripAnsi(component.render(120).join("\n")); - expect(withDiffs).toContain("-1 before"); - expect(withDiffs).toContain("+1 after"); - expect(withDiffs).not.toContain("+1 -1"); + const withDiffLines = stripAnsi(component.render(120).join("\n")).split("\n"); + // The summary line stays put; the diff renders under it, indented to its text column. + const summaryIndex = withDiffLines.findIndex((line) => line.includes("╰─ README.md +1 -1")); + expect(summaryIndex).toBeGreaterThanOrEqual(0); + expect(withDiffLines[summaryIndex]).toContain("to collapse"); + const textColumn = withDiffLines[summaryIndex].indexOf("README.md"); + const removed = withDiffLines.find((line) => line.includes("-1 before")); + const added = withDiffLines.find((line) => line.includes("+1 after")); + expect(removed?.startsWith(" ".repeat(textColumn))).toBe(true); + expect(added?.startsWith(" ".repeat(textColumn))).toBe(true); component.setEditDiffsExpanded(false); const collapsedAgain = stripAnsi(component.render(120).join("\n")); @@ -466,7 +473,111 @@ describe("ToolExecutionComponent parity", () => { expect(collapsedAgain).toContain("+1 -1"); }); - test("keeps the ctrl+j hint on the header while no summary line renders", async () => { + test("suppresses the built-in edit summary and diff when execution fails", async () => { + const dir = await mkdtemp(join(tmpdir(), "pi-edit-failed-")); + try { + const filePath = join(dir, "sample.txt"); + await writeFile(filePath, "before\n", "utf8"); + const component = new ToolExecutionComponent( + "edit", + "tool-4err", + { path: filePath, edits: [{ oldText: "before", newText: "after" }] }, + + {}, + createEditToolDefinition(dir), + createFakeTui(), + dir, + ); + + component.setEditDiffsExpanded(true); + const deadline = Date.now() + 2000; + while (Date.now() < deadline && !stripAnsi(component.render(120).join("\n")).includes("+1 -1")) { + component.setArgsComplete(); + await new Promise((resolve) => setTimeout(resolve, 5)); + } + expect(stripAnsi(component.render(120).join("\n"))).toContain("+1 -1"); + + component.updateResult( + { content: [{ type: "text", text: "Could not edit file: sample.txt." }], isError: true }, + false, + ); + const rendered = stripAnsi(component.render(120).join("\n")); + expect(rendered).not.toContain("╰─"); + expect(rendered).not.toContain("+1 -1"); + expect(rendered).not.toContain("-1 before"); + expect(rendered).not.toContain("+1 after"); + + // The header must switch to the error background even though the + // async preview itself succeeded before execution failed. + const rawRendered = component.render(120).join("\n"); + expect(rawRendered).toContain(theme.bg("toolErrorBg", "").slice(0, -"\x1b[49m".length)); + expect(rawRendered).not.toContain(theme.bg("toolSuccessBg", "").slice(0, -"\x1b[49m".length)); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("built-in edit summary truncates a long path to one row and keeps counts and hint", () => { + const longPath = "deeply/nested/directory/structure/with/a/really/long/file-name-that-overflows.md"; + const component = new ToolExecutionComponent( + "edit", + "tool-4f", + { path: longPath, oldText: "before", newText: "after" }, + {}, + createEditToolDefinition(process.cwd()), + createFakeTui(), + process.cwd(), + ); + component.updateResult( + { content: [], details: { diff: "-1 before\n+1 after", firstChangedLine: 1 }, isError: false }, + false, + ); + const lines = stripAnsi(component.render(40).join("\n")).split("\n"); + const summaryLines = lines.filter((line) => line.includes("╰─")); + expect(summaryLines.length).toBe(1); + expect(summaryLines[0]).toContain("…"); + expect(summaryLines[0]).toContain("+1 -1"); + expect(summaryLines[0]).toContain("to expand"); + for (const line of lines) { + expect(line.length).toBeLessThanOrEqual(40); + } + }); + + test("built-in edit diff rows keep the summary text column when a diff line wraps", () => { + const component = new ToolExecutionComponent( + "edit", + "tool-4g", + { path: "README.md", oldText: "before", newText: "after" }, + {}, + createEditToolDefinition(process.cwd()), + createFakeTui(), + process.cwd(), + ); + const longLine = + "alpha beta gamma delta epsilon zeta eta theta iota kappa lambda mu nu xi omicron pi rho sigma tau"; + component.updateResult( + { content: [], details: { diff: `+1 ${longLine}`, firstChangedLine: 1 }, isError: false }, + false, + ); + component.setEditDiffsExpanded(true); + const lines = stripAnsi(component.render(60).join("\n")).split("\n"); + const summaryIndex = lines.findIndex((line) => line.includes("╰─ README.md")); + expect(summaryIndex).toBeGreaterThanOrEqual(0); + const textColumn = lines[summaryIndex].indexOf("README.md"); + const diffRows: string[] = []; + for (let i = summaryIndex + 1; i < lines.length && lines[i].trim() !== ""; i++) { + diffRows.push(lines[i]); + } + // The single logical diff line wraps; every continuation row stays anchored at the text column. + expect(diffRows.length).toBeGreaterThan(1); + for (const row of diffRows) { + expect(row.startsWith(" ".repeat(textColumn))).toBe(true); + expect(row[textColumn]).not.toBe(" "); + } + expect(diffRows.join(" ")).toContain("tau"); + }); + + test("renders exactly one ctrl+j hint before and after the result lands", async () => { const dir = mkdtempSync(join(tmpdir(), "edit-hint-")); const filePath = join(dir, "sample.txt"); writeFileSync(filePath, "before\n"); @@ -486,12 +597,12 @@ describe("ToolExecutionComponent parity", () => { await vi.waitFor(() => { expect(stripAnsi(component.render(120).join("\n"))).toContain("to expand"); }); - // Pre-result: no summary line exists yet, so the header carries the hint. + // Pre-result: the preview's summary line already carries the hint. const preResult = stripAnsi(component.render(120).join("\n")); - expect(preResult).not.toContain("╰─"); + expect(preResult.split("\n").find((line) => line.includes("╰─"))).toContain("to expand"); expect(preResult.split("to expand").length - 1).toBe(1); - // Once a successful result lands, the summary line takes over the hint. + // A successful result keeps a single hint on the summary line. component.updateResult( { content: [], details: { diff: "-1 before\n+1 after", firstChangedLine: 1 }, isError: false }, false, @@ -781,8 +892,11 @@ describe("ToolExecutionComponent parity", () => { const withDiffs = stripAnsi(component.render(120).join("\n")); const withDiffLines = withDiffs.split("\n"); expect(withDiffLines.findIndex((line) => line.includes("hidden_side_effect ="))).toBeLessThan( - withDiffLines.findIndex((line) => /✓ README\.md\s+\+1 -1/.test(line)), + withDiffLines.findIndex((line) => line.includes("╰─ README.md +1 -1")), ); - expect(withDiffs).not.toContain("╰─ README.md +1 -1"); + // Exactly one summary line — the cell owns the block; no extra component doubles it. + expect(withDiffLines.filter((line) => line.includes("╰─ README.md +1 -1")).length).toBe(1); + expect(withDiffs).toMatch(/1 - before/); + expect(withDiffs).toMatch(/1 \+ after/); }); });