fix(web): address diff card review — split terminator, error arm, wire narrowing

- DiffBlock: an empty side contributes zero lines and a trailing newline is a
  terminator, so a create ending in a newline draws one added line (not a
  phantom empty one) and a full deletion draws no phantom + line.
- diffCardModel: narrow the wire diffs payload (card is the only validated
  field) so a malformed diff card falls back to the generic path instead of
  throwing inside DiffBlock.
- FileMutationRow: surface the result text when an errored mutation has no diff
  card, so a failed edit/write is more than a red dot.
- copyText ends its closed union on assertNever.
- Docs: drop the "bridge relativizes" claim, record the file-count divergence
  from the TUI footer, correct the built-boot overclaim, note why the row title
  outranks the view title, and make fixture turn 67 args self-consistent.
- Tests: terminator/empty-side/interior-blank rows, wire-narrowing null arms,
  the error-text arm and its name/code fallback, stopped state, no-path summary,
  and the registration/disposal shape.
This commit is contained in:
Chinesezjc
2026-07-30 18:32:55 +08:00
parent d7e46bea35
commit 8c5c4b46c8
10 changed files with 221 additions and 19 deletions

View File

@@ -238,7 +238,7 @@ function buildAlphaLog(): SessionEvent[] {
// the presenter reads to emit the two-hunk sample: the card draws one path
// header, the first hunk, a `⋯` gap, then the second (the same-file
// second-hunk arm turns 62/63 cannot reach).
toolTurn(67, 'edit', '{"file_path":"src/config.ts","old_string":"multi","new_string":"multi"}', '已编辑')
toolTurn(67, 'edit', '{"file_path":"src/config.ts","old_string":"const timeout = 30","new_string":"const timeout = 60"}', '已编辑')
// Turn 64: one run_code turn with three logged sub-dispatches — the Code
// Mode acceptance surface (parent code row + nested native-identical rows,
// including an isError sub-call and a bash sub-call that must hit the same

View File

@@ -7,7 +7,7 @@
* call this, so the hunks they show are derived once.
* @module
*/
import type { DiffBlockProps } from '@deepseek-ai/dsh-client-ui-primitives'
import type { DiffBlockProps, DiffHunk } from '@deepseek-ai/dsh-client-ui-primitives'
import type { ToolCallBlock } from './tool-call-model.ts'
/**
@@ -35,6 +35,30 @@ export interface DiffCardModel {
card: Pick<DiffBlockProps, 'diffs'>
}
/**
* Narrow a wire `card:'diff'` view's `diffs` to well-formed hunks. The event
* view crosses the wire and `toolEventViewSchema` validates only the `card`
* string, so a version mismatch or an anomalous plugin can deliver a `diff` card
* whose `diffs` is absent, not an array, or carries malformed hunks. Returning
* null for any of those routes the block to the generic path instead of letting
* DiffBlock's `for...of`/`split` throw and crash the row or the details panel.
* @param diffs - the view's `diffs` field, unverified.
* @returns the validated hunks, or null when the payload is not usable.
*/
function narrowDiffs(diffs: unknown): DiffHunk[] | null {
if (!Array.isArray(diffs) || diffs.length === 0) return null
const out: DiffHunk[] = []
for (const hunk of diffs) {
if (typeof hunk !== 'object' || hunk === null) return null
const { path, oldText, newText } = hunk as Record<string, unknown>
if (typeof path !== 'string') return null
if (oldText !== null && typeof oldText !== 'string') return null
if (typeof newText !== 'string') return null
out.push({ path, oldText, newText })
}
return out
}
/**
* Derive the diff-card props for a tool call, or null when this call is not a
* diff card and belongs on the generic path.
@@ -49,6 +73,14 @@ export interface DiffCardModel {
* be trusted to be one of the compiled variants — and a settled call whose
* result view is generic (how write/edit keep their execution errors on the
* generic path).
*
* This derivation consumes only `diffs`; the render intent's `title` field is
* deliberately dropped. The row supplies its own title (`Edit`/`Write · path`
* from the args) and that outranks the view's `title`, matching the TUI diff
* branch, which likewise draws no view title. A tool that names its own diff
* header therefore does not surface that text on the Web row — an accepted
* product choice, recorded here as the one asymmetry with the terminal card,
* whose derivation does consume the view's title.
* @param block - RunningToolCall or ToolResultNode off the snapshot caches.
* @returns the diff-card props, or null for the generic path.
*/
@@ -56,11 +88,13 @@ export function diffCardModel(block: ToolCallBlock): DiffCardModel | null {
if (!('kind' in block)) {
// Running: the call view may carry the intended diff; the result is absent.
const call = block.callView?.card === 'diff' ? block.callView : null
return call === null ? null : { card: { diffs: call.diffs } }
const diffs = call === null ? null : narrowDiffs(call.diffs)
return diffs === null ? null : { card: { diffs } }
}
// Settled: the result view's applied hunks replace the call-time diff. A
// window that dropped the call head leaves only the result, which still
// renders — the result view carries the whole change.
const result = block.resultView?.card === 'diff' ? block.resultView : null
return result === null ? null : { card: { diffs: result.diffs } }
const diffs = result === null ? null : narrowDiffs(result.diffs)
return diffs === null ? null : { card: { diffs } }
}

View File

@@ -117,3 +117,14 @@
clip: rect(0 0 0 0);
white-space: nowrap;
}
/* The result text for an errored mutation, indented to the card's own column
(the diff card's inset) and in the error tone, since it stands in for the diff
card the failure path does not produce. */
.failure {
margin: 4px 0 4px 22px;
white-space: pre-wrap;
overflow-wrap: anywhere;
font: var(--dsw-font-xs-13);
color: var(--dsw-alias-state-error-primary);
}

View File

@@ -39,6 +39,27 @@ function stateStatus(state: ToolRowState): string | null {
}
}
/**
* A settled result's text, flattened from its content blocks, for the arm that
* shows a failure the diff card cannot: write/edit return `undefined` from
* `presentResult` on `result.isError`, so an errored mutation has no diff card,
* and the keyed row is not a details-panel target. Without this the failure —
* an `old_string` that did not match, a permission denial — would read as a bare
* red dot with the model-facing error text nowhere on screen.
* @param block - the frozen call slice.
* @returns the result text, or null for a running call or an empty result.
*/
function errorText(block: ToolRowProps['block']): string | null {
if (!('kind' in block)) return null
const parts: string[] = []
for (const item of block.content) {
if (item.type === 'text') parts.push(item.text)
}
if (parts.length === 0 && block.error !== undefined) parts.push(`${block.error.name}: ${block.error.code}`)
const text = parts.join('\n')
return text === '' ? null : text
}
/**
* File-mutation row: icon + {Edit,Write} · {path} in the shared ToolRow chrome,
* with the applied diff resident below it. The summary is a path link (a file
@@ -50,6 +71,9 @@ export function FileMutationRow({ toolName, block, cwd, openFile }: ToolRowProps
const diff = diffCardModel(block)
const status = stateStatus(model.state)
const filePath = model.filePath
// An errored mutation has no diff card (presentResult returns undefined on
// isError); surface its result text so the failure is more than a red dot.
const failure = diff === null && model.state === 'error' ? errorText(block) : null
return (
<div className={css.card}>
<div className={css.root} data-variant={model.variant} data-state={model.state}>
@@ -72,6 +96,7 @@ export function FileMutationRow({ toolName, block, cwd, openFile }: ToolRowProps
{diff !== null && (
<DiffBlock {...diff.card} maxLines={CHAT_DIFF_MAX_LINES} className={css.diff} />
)}
{failure !== null && <div className={css.failure}>{failure}</div>}
</div>
)
}

View File

@@ -17,7 +17,7 @@ import { CHAT_DIFF_MAX_LINES, diffCardModel } from '../src/client/contract/diff-
import { createChatStore } from '../src/client/stores.ts'
import { GenericToolCard } from '../src/client/chat/GenericToolCard.tsx'
import { DetailsPanel } from '../src/client/skeleton/DetailsPanel.tsx'
import { FileMutationRow } from '../src/client/toolviews/file-mutation-row.tsx'
import { FileMutationRow, fileMutationToolview } from '../src/client/toolviews/file-mutation-row.tsx'
afterEach(cleanup)
@@ -86,6 +86,22 @@ describe('diffCardModel', () => {
callView: future, resultView: { card: 'chart' } as unknown as ToolResultView,
}))).toBeNull()
})
it('falls back to null for a malformed diff payload off the wire', () => {
// toolEventViewSchema validates only the `card` string, so a version
// mismatch can deliver a diff card with an unusable diffs field. Each shape
// routes to the generic path instead of throwing inside DiffBlock.
const bad = (diffs: unknown): ToolResultView => ({ card: 'diff', diffs } as unknown as ToolResultView)
expect(diffCardModel(settled({ resultView: bad(undefined) }))).toBeNull()
expect(diffCardModel(settled({ resultView: bad([]) }))).toBeNull()
expect(diffCardModel(settled({ resultView: bad('nope') }))).toBeNull()
expect(diffCardModel(settled({ resultView: bad([null]) }))).toBeNull()
expect(diffCardModel(settled({ resultView: bad([{ path: 1, oldText: null, newText: 'x' }]) }))).toBeNull()
expect(diffCardModel(settled({ resultView: bad([{ path: 'a', oldText: 5, newText: 'x' }]) }))).toBeNull()
expect(diffCardModel(settled({ resultView: bad([{ path: 'a', oldText: null, newText: 9 }]) }))).toBeNull()
// The running side narrows identically.
expect(diffCardModel(running({ callView: { card: 'diff', diffs: 'nope' } as unknown as ToolCallView }))).toBeNull()
})
})
describe('chat row diff body', () => {
@@ -176,6 +192,78 @@ describe('FileMutationRow diff card', () => {
const view = render(<FileMutationRow {...rowProps(settled({ callView: null, resultView: null }))} />)
expect(view.container.querySelector('[data-diff]')).toBeNull()
})
it('surfaces the result text when an errored mutation has no diff card', () => {
// write/edit return undefined from presentResult on isError, so the failure
// has no diff — the row shows the model-facing error text instead of a bare
// red dot.
const view = render(<FileMutationRow {...rowProps(settled({
isError: true, callView: null, resultView: null,
content: [{ type: 'text', text: 'old_string not found in notes/demo.txt' }],
}))} />)
expect(view.container.querySelector('[data-diff]')).toBeNull()
expect(view.getByText('old_string not found in notes/demo.txt')).toBeTruthy()
})
it('falls back to the error name/code when an errored result has no text block', () => {
const view = render(<FileMutationRow {...rowProps(settled({
isError: true, callView: null, resultView: null, content: [],
error: { name: 'ToolError', code: 'sandbox_denied' },
}))} />)
expect(view.getByText('ToolError: sandbox_denied')).toBeTruthy()
})
it('shows no failure text for a successful diff or a running call', () => {
const ok = render(<FileMutationRow {...rowProps(settled())} />)
expect(ok.container.querySelector('[class*="_failure_"]')).toBeNull()
cleanup()
const run = render(<FileMutationRow {...rowProps(running())} />)
expect(run.container.querySelector('[class*="_failure_"]')).toBeNull()
})
it('shows the stopped state when the call was interrupted', () => {
const view = render(<FileMutationRow {...rowProps(settled({
callView: null, resultView: null, isError: true,
error: { name: 'ToolError', code: 'interrupted' },
}))} />)
expect(view.container.querySelector('[data-state="stopped"]')).not.toBeNull()
// The visually-hidden status label carries the stopped semantic for AT.
expect(view.getByText('已停止')).toBeTruthy()
})
it('renders a plain summary span when the call carries no file path', () => {
// Empty args leave deriveFilePath undefined, so the summary is not a link.
const view = render(<FileMutationRow {...rowProps(settled({
call: { name: 'edit', argsRaw: '' }, callView: null, resultView: null,
}))} />)
expect(view.container.querySelector('[class*="_fileLink_"]')).toBeNull()
expect(view.container.querySelector('[class*="_summary_"]')).not.toBeNull()
})
})
describe('fileMutationToolview registration', () => {
it('registers one component under both edit and write, and each disposes', () => {
const registered: { key: string; disposed: boolean }[] = []
const disposers: (() => void)[] = []
const ctx = {
slots: {
register: ({ key }: { name: string; key: string }) => {
const entry = { key, disposed: false }
registered.push(entry)
const dispose = () => { entry.disposed = true }
disposers.push(dispose)
return dispose
},
},
}
fileMutationToolview.apply(ctx as never)
expect(registered.map(r => r.key).sort()).toEqual(['edit', 'write'])
// The registrant's inject seam is the load-order contract the row relies on.
expect(fileMutationToolview.inject).toEqual(['slots', 'conversation'])
// Disposal removes each contribution (packages/AGENTS.md registry contract).
for (const dispose of disposers) dispose()
expect(registered.every(r => r.disposed)).toBe(true)
})
})
describe('DetailsPanel diff Output section', () => {

View File

@@ -26,7 +26,7 @@ export const DEFAULT_DIFF_MAX_LINES = 16
* free of the tool contract (the terminal card's decoupling, applied to diffs).
*/
export interface DiffHunk {
/** The changed file's path (as the tool operated on it; the bridge relativizes it). */
/** The changed file's path, drawn verbatim as the hunk's header (the tool's model-facing path). */
path: string
/** Prior content, or `null` for a new file / an overwrite (nothing on the removed side). */
oldText: string | null
@@ -49,6 +49,12 @@ interface DiffRow {
text: string
}
/** Local exhaustiveness helper — this package does not depend on `dsh-llm`. */
/* v8 ignore next 3 -- closed-union backstop; only reached if a row kind is forged */
function assertNever(value: never): never {
throw new Error(`unreachable diff row kind: ${String(value)}`)
}
/** The dim class per row kind (path/gap chrome vs the diff's own +/- colors). */
const ROW_CLASS: Record<DiffRow['kind'], string | undefined> = {
path: css.path,
@@ -61,8 +67,10 @@ const ROW_CLASS: Record<DiffRow['kind'], string | undefined> = {
* Flatten the hunks into the body's rows plus the footer counts. A path header
* opens each new file; a same-file second hunk (a scattered edit) opens with a
* `⋯` gap instead of repeating the path. Every old-side line counts toward
* `removed` and every new-side line toward `added`, the same per-side line count
* the TUI footer draws, so the two front ends agree on a change's size.
* `removed` and every new-side line toward `added`. The file count is of
* DISTINCT paths, which is the one deliberate divergence from the TUI diff card:
* the TUI footer uses `diffs.length`, so two hunks in one file read there as
* `2 files`, whereas this counts the one file they belong to.
* @param diffs - the hunks to render.
* @returns the body rows, the +/- totals, and the distinct-file count.
*/
@@ -78,12 +86,12 @@ function buildRows(diffs: DiffHunk[]): { rows: DiffRow[]; added: number; removed
else rows.push({ kind: 'gap', text: '⋯' })
prevPath = diff.path
if (diff.oldText !== null) {
for (const line of diff.oldText.split('\n')) {
for (const line of contentLines(diff.oldText)) {
rows.push({ kind: 'del', text: line })
removed++
}
}
for (const line of diff.newText.split('\n')) {
for (const line of contentLines(diff.newText)) {
rows.push({ kind: 'add', text: line })
added++
}
@@ -91,6 +99,21 @@ function buildRows(diffs: DiffHunk[]): { rows: DiffRow[]; added: number; removed
return { rows, added, removed, files: paths.size }
}
/**
* Split a side's text into its content lines. Empty text is zero lines (a full
* deletion's `newText` or a create's absent `oldText` side draws nothing), and a
* single trailing newline is a line terminator rather than an extra empty line —
* the same terminator rule TerminalBlock applies to command output. An interior
* blank line (a genuine `\n\n`) survives.
* @param text - the removed or added side's text.
* @returns the content lines, without the terminating newline.
*/
function contentLines(text: string): string[] {
if (text === '') return []
const body = text.endsWith('\n') ? text.slice(0, -1) : text
return body.split('\n')
}
/**
* The diff text a reader copies: each row's `-`/`+`/path/gap prefix and its
* content, exactly what the card shows. The removed and added blocks are the
@@ -103,8 +126,9 @@ function copyText(rows: DiffRow[]): string {
switch (row.kind) {
case 'del': return `- ${row.text}`
case 'add': return `+ ${row.text}`
case 'path': return row.text
case 'gap': return row.text
default: return row.text
default: return assertNever(row.kind)
}
}).join('\n')
}

View File

@@ -76,6 +76,26 @@ describe('DiffBlock structure', () => {
const { container } = render(<DiffBlock diffs={[]} />)
expect(container.firstChild).toBeNull()
})
it('treats a trailing newline as a terminator, not an extra blank line', () => {
// A create whose newText ends in a newline is one added line, not two, and
// the footer counts one — the phantom `+ ` empty line the naive split drew.
const { container } = render(<DiffBlock diffs={[{ path: 'n.txt', oldText: null, newText: 'hello\n' }]} />)
expect(changeRows(container)).toEqual(['hello'])
expect(screen.getByText('└ +1 -0 · 1 file')).toBeTruthy()
})
it('renders a full deletion as removed-only with no phantom added line', () => {
// newText '' is zero added lines: an empty string must contribute nothing.
const { container } = render(<DiffBlock diffs={[{ path: 'gone.ts', oldText: 'a\nb', newText: '' }]} />)
expect(container.querySelectorAll('[class*="_add_"]').length).toBe(0)
expect(screen.getByText('└ +0 -2 · 1 file')).toBeTruthy()
})
it('keeps a genuine interior blank line', () => {
const { container } = render(<DiffBlock diffs={[{ path: 'a.ts', oldText: null, newText: 'x\n\ny' }]} />)
expect(container.querySelectorAll('[class*="_add_"]').length).toBe(3)
})
})
describe('DiffBlock footer', () => {