From 446716d2794cb5fc2348939b484b304d72f8e345 Mon Sep 17 00:00:00 2001 From: AstroHan Date: Fri, 10 Jul 2026 17:07:16 +0800 Subject: [PATCH 01/18] fix(ui): keep tool diagnostics collapsed by default --- ...automation-result-preview-contract.test.ts | 8 +- .../disclosure-collapsible-contract.test.ts | 15 +- .../text-swap-min-width-contract.test.ts | 27 +-- .../tool-activity-presentation.test.ts | 100 +++++++++++ packages/ui/src/primitives/chat.tsx | 18 +- packages/ui/src/tool-activity.tsx | 158 +++++++----------- packages/ui/src/tool-activity/presentation.ts | 56 +++++++ 7 files changed, 235 insertions(+), 147 deletions(-) create mode 100644 packages/ui/src/__tests__/tool-activity-presentation.test.ts create mode 100644 packages/ui/src/tool-activity/presentation.ts diff --git a/apps/desktop/src/main/__tests__/automation-result-preview-contract.test.ts b/apps/desktop/src/main/__tests__/automation-result-preview-contract.test.ts index d3c0c987c9..c8e407874d 100644 --- a/apps/desktop/src/main/__tests__/automation-result-preview-contract.test.ts +++ b/apps/desktop/src/main/__tests__/automation-result-preview-contract.test.ts @@ -42,10 +42,10 @@ function renderAutomationResult(text: string): string { const item: ToolActivityItem = { toolUseId: 'tu-1', toolName: AUTOMATION_TOOL_NAME, - // 'running' keeps the collapsible card open by default so the static - // markup includes the body (a settled card collapses and Base UI unmounts - // closed panel content — nothing to assert on). - status: 'running', + // This test exercises the result parser, not disclosure defaults. Use an + // attention state so Base UI mounts the panel in static markup; ordinary + // running tools now stay collapsed until the user asks for diagnostics. + status: 'errored', args: { mode: 'create' }, result: { kind: 'text', text }, }; diff --git a/apps/desktop/src/main/__tests__/disclosure-collapsible-contract.test.ts b/apps/desktop/src/main/__tests__/disclosure-collapsible-contract.test.ts index e3342b1151..feac2498d6 100644 --- a/apps/desktop/src/main/__tests__/disclosure-collapsible-contract.test.ts +++ b/apps/desktop/src/main/__tests__/disclosure-collapsible-contract.test.ts @@ -56,18 +56,11 @@ describe('PR-DISCLOSURE-COLLAPSIBLE-0 contract', () => { } }); - it('tool-activity Collapsible is controlled (open follows item.status), not defaultOpen', async () => { - // A `defaultOpen` card decides open only on first render, so a card that - // defaults open while pending/running would NOT auto-collapse when it - // settles to completed/interrupted — the pre-Collapsible `
` re-evaluated open every render. The - // controlled form (open + onOpenChange, re-synced via useEffect on - // [item.status]) restores that: status change collapses/expands the card, - // the user can still toggle in between. + it('tool-activity Collapsible is controlled by the shared disclosure state, not defaultOpen', async () => { const src = await readFile(resolve(REPO_ROOT, 'packages/ui/src/tool-activity.tsx'), 'utf8'); - assert.ok(!/defaultOpen=/.test(src), 'tool-activity must not use defaultOpen (a running card that defaults open would not auto-collapse when it settles); use controlled open that follows item.status'); + assert.ok(!/defaultOpen=/.test(src), 'tool-activity must not use defaultOpen; shared disclosure state preserves manual choices and surfaces new attention states'); assert.match(src, /\bonOpenChange\b/, 'tool-activity Collapsible must be controlled via onOpenChange'); - assert.match(src, /useEffect\([^]*\[item\.status\]/, 'tool-activity must re-sync open when item.status changes (useEffect on [item.status])'); + assert.match(src, /useToolDisclosure/, 'tool-activity must route open state through the shared disclosure controller'); }); }); @@ -80,4 +73,4 @@ describe('disclosure-collapsible negative cases', () => { const withImport = 'import { Collapsible } from "@maka/ui";\nexport function X() { return ; }'; assert.ok(COLLAPSIBLE_IMPORT_RE.test(withImport), 'a Collapsible import must match'); }); -}); \ No newline at end of file +}); diff --git a/apps/desktop/src/main/__tests__/text-swap-min-width-contract.test.ts b/apps/desktop/src/main/__tests__/text-swap-min-width-contract.test.ts index ec9020af31..c62cdb850c 100644 --- a/apps/desktop/src/main/__tests__/text-swap-min-width-contract.test.ts +++ b/apps/desktop/src/main/__tests__/text-swap-min-width-contract.test.ts @@ -108,13 +108,6 @@ const TEXT_SWAP_BUTTONS: Array<{ file: string; onClick: string; minW: string; no { file: 'apps/desktop/src/renderer/error-boundary.tsx', onClick: 'onClick={this.handleCopyReport}', minW: '5.5rem', note: '复制诊断信息 ↔ 复制中… ↔ 已复制 ↔ 复制失败' }, ]; -// Chat stream-count variant lock: the min-w-[Nrem] -// declaration lives in the variant definition (chat.tsx), not at the call -// site, so we pin the literal declaration substrings. -const CHAT_VARIANT_LOCKS: Array<{ file: string; substr: string; note: string }> = [ - { file: 'packages/ui/src/primitives/chat.tsx', substr: 'min-w-[5rem] [font-variant-numeric:tabular-nums]', note: 'streamVariants count (stdout/stderr/已脱敏 N)' }, -]; - const BUTTON_OPEN_RE = /<(?:Ui)?Button\b/g; // --- Heuristic scan (DISCOVERY, scoped to PR3 files) ------------------------ @@ -173,24 +166,6 @@ describe('PR-ANTI-LAYOUT-SHIFT-TEXT-SWAP-0 contract', () => { } }); - it('chat stream-count variants keep their min-w declarations', async () => { - const byFile = new Map(); - for (const l of CHAT_VARIANT_LOCKS) { - const arr = byFile.get(l.file) ?? []; - arr.push(l); - byFile.set(l.file, arr); - } - for (const [file, locks] of byFile) { - const src = await readFile(resolve(REPO_ROOT, file), 'utf8'); - for (const { substr, note } of locks) { - assert.ok( - src.includes(substr), - `${file}: missing variant declaration "${substr}" (${note})`, - ); - } - } - }); - it('no state-swap Button with a string-ternary child slips through without min-w-[Nrem] + whitelist value pin', () => { // Two failure modes, both must fail closed: // (a) no min-w-[Nrem] at all — the width lock is missing; @@ -269,4 +244,4 @@ describe('PR-ANTI-LAYOUT-SHIFT-TEXT-SWAP-0 contract', () => { 'STRING_TERNARY_RE must NOT match a computed-label child ({label}); if it did, the scan would wrongly claim to cover computed-label buttons. They must stay hand-pinned in TEXT_SWAP_BUTTONS.', ); }); -}); \ No newline at end of file +}); diff --git a/packages/ui/src/__tests__/tool-activity-presentation.test.ts b/packages/ui/src/__tests__/tool-activity-presentation.test.ts new file mode 100644 index 0000000000..753e2cf056 --- /dev/null +++ b/packages/ui/src/__tests__/tool-activity-presentation.test.ts @@ -0,0 +1,100 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { createElement } from 'react'; +import { renderToStaticMarkup } from 'react-dom/server'; +import { ToolActivity, ToolTrow } from '../tool-activity.js'; +import { + createToolDisclosureState, + deriveToolActivityPresentation, + setToolDisclosureOpen, + syncToolDisclosureState, +} from '../tool-activity/presentation.js'; +import type { ToolActivityItem } from '../materialize.js'; + +function renderTool(item: ToolActivityItem): string { + return renderToStaticMarkup(createElement(ToolTrow, { items: [item] })); +} + +describe('tool activity presentation', () => { + it('keeps a running command detail collapsed by default', () => { + const markup = renderTool({ + toolUseId: 'tool-running', + toolName: 'Bash', + intent: '检查当前项目结构', + status: 'running', + args: { command: 'Get-ChildItem -Recurse -Depth 1' }, + outputChunks: [ + { seq: 1, stream: 'stdout', text: 'packages\n', redacted: false, createdAt: 1 }, + ], + }); + + assert.doesNotMatch(markup, /Get-ChildItem/); + assert.doesNotMatch(markup, /实时输出/); + assert.match(markup, /检查当前项目结构/); + }); + + it('preserves a manual expansion across ordinary status changes', () => { + const running: ToolActivityItem = { + toolUseId: 'tool-manual', + toolName: 'Bash', + status: 'running', + args: { command: 'npm test' }, + }; + const completed: ToolActivityItem = { + ...running, + status: 'completed', + }; + const initial = createToolDisclosureState(deriveToolActivityPresentation(running)); + const expanded = setToolDisclosureOpen(initial, true); + + assert.deepEqual( + syncToolDisclosureState(expanded, deriveToolActivityPresentation(completed)), + { open: true, manuallySet: true }, + ); + }); + + it('opens a newly errored tool even after an earlier manual collapse', () => { + const running: ToolActivityItem = { + toolUseId: 'tool-error', + toolName: 'Bash', + status: 'running', + args: { command: 'npm test' }, + }; + const errored: ToolActivityItem = { + ...running, + status: 'errored', + }; + const collapsed = setToolDisclosureOpen( + createToolDisclosureState(deriveToolActivityPresentation(running)), + false, + ); + + assert.deepEqual( + syncToolDisclosureState(collapsed, deriveToolActivityPresentation(errored)), + { open: true, manuallySet: false }, + ); + }); + + it('shows diagnostic flags without exposing transport chunk counts', () => { + const markup = renderToStaticMarkup(createElement(ToolActivity, { + items: [{ + toolUseId: 'tool-output', + toolName: 'Bash', + status: 'errored', + args: { command: 'npm test' }, + outputChunks: [ + { seq: 1, stream: 'stdout', text: 'one\n', redacted: false, createdAt: 1 }, + { seq: 2, stream: 'stdout', text: 'two\n', redacted: true, createdAt: 2 }, + { seq: 3, stream: 'stderr', text: 'failed\n', redacted: false, createdAt: 3 }, + ], + outputTruncated: true, + } satisfies ToolActivityItem], + })); + + assert.doesNotMatch(markup, /stdout\s+2/i); + assert.doesNotMatch(markup, /stderr\s+1/i); + assert.match(markup, /stderr/i); + assert.match(markup, /已脱敏/); + assert.match(markup, /已截断/); + }); +}); diff --git a/packages/ui/src/primitives/chat.tsx b/packages/ui/src/primitives/chat.tsx index 6a9cf7684b..91b28e8f0e 100644 --- a/packages/ui/src/primitives/chat.tsx +++ b/packages/ui/src/primitives/chat.tsx @@ -254,7 +254,7 @@ export function Marker({ * Tool live-output stream shell (issue #332, PR3). * * Retires the bespoke `.maka-tool-output-stream-*` shell CSS (the panel, - * header, counts row, scrolling body, and chunk/tag spans in + * header, diagnostic flags, scrolling body, and chunk/tag spans in * `styles/tool-stream.css`), moving each onto this Tailwind substrate. Every * value is a LITERAL arbitrary utility that compiles 1:1 to the declaration it * replaces, so the cva source string IS the computed-style proof (the cascade @@ -286,16 +286,12 @@ const streamVariants = cva("", { "flex items-center justify-between gap-3 px-2.5 py-1.5 border-b border-[var(--border)] bg-[var(--foreground-3)] text-xs uppercase tracking-[0.06em] text-[color:var(--muted-foreground)]", // `.maka-tool-output-stream-label` label: "inline-flex items-center gap-1.5", - // `.maka-tool-output-stream-counts` - counts: "inline-flex items-center gap-2.5", - // `.maka-tool-output-stream-counts span` (tabular-nums on every count) plus - // the `[data-stream=stderr]` / `[data-redacted]` / `[data-truncated]` - // recolors. The `已截断` pill (`data-truncated`) gets the warning chrome the - // old `span[data-truncated="true"]` rule supplied; the inert - // `.maka-tool-output-stream-truncated-tag` class (no rule of its own) is - // dropped. - count: - "min-w-[5rem] [font-variant-numeric:tabular-nums]" + // Diagnostic flags replace transport chunk counts. A stream's internal + // delivery granularity is not user progress; only stderr, redaction, and + // truncation facts belong in this header. + flags: "inline-flex items-center gap-2.5", + flag: + "whitespace-nowrap" + " data-[stream=stderr]:text-[color:var(--destructive-text)]" + " data-[redacted=true]:text-[color:var(--warning-text,var(--info-text))]" + " data-[truncated=true]:rounded-[var(--radius-control)] data-[truncated=true]:border data-[truncated=true]:border-[oklch(from_var(--warning)_l_c_h_/_0.30)] data-[truncated=true]:bg-[oklch(from_var(--warning)_l_c_h_/_0.06)] data-[truncated=true]:px-1 data-[truncated=true]:text-[color:var(--warning-text,var(--info-text))] data-[truncated=true]:cursor-help", diff --git a/packages/ui/src/tool-activity.tsx b/packages/ui/src/tool-activity.tsx index de2ab1809f..dcf079d5cb 100644 --- a/packages/ui/src/tool-activity.tsx +++ b/packages/ui/src/tool-activity.tsx @@ -23,40 +23,28 @@ import { activeTrowTool, isTrowRunning, summarizeTrowTools, - trowActivityKind, trowNeedsAttention, type TrowActivityKind, } from './tool-activity/trow-summary.js'; import { deriveToolRowMotion, isToolRowRunning } from './tool-activity/tool-row-motion.js'; +import { + createToolDisclosureState, + deriveToolActivityPresentation, + isConnectorTool, + resolveToolDisplayName, + setToolDisclosureOpen, + syncToolDisclosureState, + type ToolActivityPresentation, +} from './tool-activity/presentation.js'; import { Alert, AlertAction, AlertDescription, AlertTitle } from './primitives/alert.js'; import { Collapsible, CollapsibleTrigger, CollapsiblePanel } from './primitives/collapsible.js'; import { LiveIndicator, previewVariants, streamVariants, TextShimmer, toolVariants } from './primitives/chat.js'; import { redactSecrets } from './redact.js'; import { Button as UiButton, cn } from './ui.js'; -import { describeLoadToolResult, formatRedactedJson, formatToolIntent, loadToolDisplayName } from './tool-format.js'; +import { describeLoadToolResult, formatRedactedJson, formatToolIntent } from './tool-format.js'; import { formatDuration, formatUserVisibleToolText } from './tool-activity/preview-utils.js'; import { ToolResultPreview } from './tool-activity/tool-result-preview.js'; -// Mirror of runtime's LOAD_TOOLS_NAME. @maka/ui must not depend on @maka/runtime, -// so the always-on group-activation connector's name is duplicated here as the -// single hook for its friendly, locale-aware presentation. The pre-unification -// name `load_tool` (PR #30) is also matched — it shipped and returns the same -// `{ loaded: [...] }` shape, so replayed old sessions still render friendly. -// `connect_tool_source` (PR #34) is intentionally NOT here: it never shipped and -// its `{ tools: [...] }` result shape this card does not render. -const CONNECTOR_TOOL_NAMES: ReadonlySet = new Set(['load_tools', 'load_tool']); - -function isConnectorTool(name: string): boolean { - return CONNECTOR_TOOL_NAMES.has(name); -} - -/** Friendly tool name: an explicit displayName wins; the connector gets a localized name. */ -function resolveToolDisplayName(item: ToolActivityItem): string { - if (item.displayName) return item.displayName; - if (isConnectorTool(item.toolName)) return loadToolDisplayName(detectUiLocale()); - return item.toolName; -} - /** Friendly card for a `load_tools` result; falls back to JSON on unexpected shapes. */ function LoadToolResultPreview(props: { args: unknown; value: unknown }) { const desc = describeLoadToolResult(props.args, props.value, detectUiLocale()); @@ -169,16 +157,15 @@ const STATUS_LABEL: Record = { interrupted: '已中断', }; -function isOpenByDefault(status: ToolActivityItem['status']): boolean { - // Show details inline while the call is in flight or blocking the user; also - // for errored calls so the failure is visible without an extra click. Settled - // success / interruption collapse so completed history doesn't drown the chat. - return ( - status === 'pending' || - status === 'waiting_permission' || - status === 'running' || - status === 'errored' - ); +function useToolDisclosure(presentation: ToolActivityPresentation) { + const [disclosure, setDisclosure] = useState(() => createToolDisclosureState(presentation)); + useEffect(() => { + setDisclosure((current) => syncToolDisclosureState(current, presentation)); + }, [presentation.needsAttention]); + return { + open: disclosure.open, + setOpen: (open: boolean) => setDisclosure((current) => setToolDisclosureOpen(current, open)), + }; } function extractErrorText(result: ToolActivityItem['result']): string { @@ -238,25 +225,19 @@ export function ToolActivity(props: { items: ToolActivityItem[] }) { } function ToolActivityCard({ item }: { item: ToolActivityItem }) { - // Controlled open that follows item.status: a card that defaults open while - // pending/running auto-collapses when it settles to completed/interrupted - // (restoring the pre-Collapsible native-disclosure behavior, where - // open={isOpenByDefault(status)} re-evaluated every render). The user can - // still toggle in between — onOpenChange updates local state, and the next - // status change re-syncs. See disclosure-collapsible-contract: defaultOpen - // is banned here. - const [open, setOpen] = useState(isOpenByDefault(item.status)); - useEffect(() => { - setOpen(isOpenByDefault(item.status)); - }, [item.status]); + // Ordinary work stays summarized. A new permission/error state opens the + // diagnostics, while an explicit user toggle survives later ordinary status + // changes. See disclosure-collapsible-contract: defaultOpen is banned here. + const presentation = deriveToolActivityPresentation(item); + const disclosure = useToolDisclosure(presentation); const duration = formatDuration(item.durationMs); return (