refactor(ui): centralize tool call detail presentation

[colaforniaw]

Summary

Tool call details currently select presentation ownership in several places. ToolCallDetail, ToolResultPreview, and resultOwnsOwnPanel must agree on panel routing, quiet JSON composition, and live-to-settled behavior.

Route each expanded detail through one internal describeToolCall decision. The decision now owns body selection, invocation/title/args composition, permission-denied hiding, connector load cards, and the live-to-settled switch. Reuse one quiet JSON presentation and remove the duplicate owned-panel rule.

Rendered behavior remains unchanged across owned and shared panels, connector tools, streaming output, cancellation, bypass, and sandbox failures.

Fixes #5403

Verification

• npm run lint passed.
• npm run format:check passed.
• npm --workspace @maka/ui run build passed.
• npm --workspace @maka/ui run typecheck passed.
• tool-activity-presentation.test.ts passed: 30/30.
• npx knip --workspace apps/desktop passed.
• npx knip --workspace packages/ui passed.
• Biome checks on the changed files and git diff --check passed.
• The commit hook passed the ASF header audit and protocol epoch guard.
• Root npm run build and npm run typecheck were attempted but do not complete on the current origin/main. Unrelated baseline errors include implicit any failures in runtime tests and existing CLI/Desktop protocol type drift around transcript overlay and bootstrap fields.

Draft follow-up

• Attach before/after screenshots for representative tool-call details. No visual change is intended.
• Obtain the required independent human review before merge.

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex implemented the refactor, added the presentation coverage, performed adversarial review and design ablation, ran local verification, and prepared this PR. The commit includes a Generated-by: OpenAI Codex trailer.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[hqhq1025]

No P0-P3 findings at a8827f2d133221603f921f8c1bf2b53aa422e13b.

This change centralizes the mutually exclusive tool-detail selection in describeToolCall() and leaves ToolCallDetailBody as the renderer for that decision. The connector-owned panel, live stream, shared/owned settled result, quiet JSON, args-only, permission-denied, bypass, sandbox-denial, cancellation, and PTY-control paths retain their previous precedence and presentation.

I compared the exact parent and this head with an independent server-render matrix covering 32,256 combinations of locale, result kind, args shape, tool name, status, observed activity, and live chunks. After normalizing React-generated aria-describedby IDs, there were zero markup differences and no render exceptions. The new presentation suite also passes unchanged on the exact parent (28/28), which is consistent with this refactor's stated zero-behavior-change contract.

Validation completed with build:test, full workspace typecheck, UI 507/507, the focused presentation suite 28/28, lint, format, ASF headers, changed-file Biome, UI/Desktop Knip, and git diff --check. A conflict-free synthetic merge onto current main (c980b93a37ad38fb78f255b6fffcf844e125f64c) also passed build, typecheck, and the focused suite; hosted test and label are green. I did not independently exercise packaged macOS/Windows Electron or a native screen-reader session.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

No P0-P3 findings at dc1324677391256d3106c8188215d9dc928c991f.

The rebased four-file change still centralizes mutually exclusive tool-detail selection in describeToolCall() and leaves ToolCallDetailBody to render that decision. The connector-owned panel, live stream, shared/owned settled result, quiet JSON, args-only, permission-denied, bypass, sandbox-denial, cancellation, and PTY-control paths retain their prior precedence and presentation.

For this exact head, I compared the current base and PR using an independent server-render matrix covering 32,256 combinations of locale, result kind, args shape, tool name, status, observed activity, and live chunks. After normalizing React-generated IDs, there were zero markup differences and no render exceptions. The four PR files are also byte-identical to the previously reviewed patch after rebasing, and the current focused presentation suite passes 28/28.

Local validation passed with a clean install, build:test, full workspace typecheck, UI 498/498, focused presentation 28/28, lint, format, ASF headers, changed-file Biome, UI/Desktop Knip, git diff --check, Storybook production build, and three consecutive transcript geometry runs. The hosted test check is currently red because one geometry run observed three cold-reader slips where the gate allows one; the same exact-head scenario passed all three local repetitions, and the exact base's hosted test is green, so the available evidence does not attribute that isolated failure to this refactor. The gate remains red and should be rerun before merge.

I did not independently exercise packaged macOS/Windows Electron or a native screen-reader session.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

Reviewed exact head bc706c3a62af708061fc8d9068283a8f63d50c84.

I found no P0-P3 issue. This force-updated commit has the same complete tree object (57f858e6b2c7ab17da0722a48a35adf8ae0422d2) as the previously reviewed head dc1324677391256d3106c8188215d9dc928c991f; git range-diff reports the refactor patch unchanged, and all four affected UI file blobs are identical. The production behavior therefore remains the reviewed zero-behavior-change refactor: describeToolCall() centrally selects connector-owned, live, shared/owned settled, quiet JSON, args-only, permission, sandbox, cancellation, and PTY-control presentations, while the detail body renders that decision.

The identical tree previously passed a clean install, build:test, full workspace typecheck, UI 498/498, focused presentation 28/28, lint, format, ASF headers, changed-file Biome, UI/Desktop Knip, git diff --check, Storybook production build, three consecutive transcript-geometry runs, and an independent 32,256-case SSR differential with zero normalized markup differences or render exceptions. The new exact head's hosted test check is now green, resolving the prior head's unclosed geometry gate.

This head is directly based on current main 672d82731a638e45e3ec87022eabdcf70a5a90be, and GitHub reports MERGEABLE. I did not independently exercise packaged macOS/Windows Electron or a native screen-reader session; final acceptance remains a human decision.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[Astro-Han]

Thanks for splitting this out — the single-decision shape is much easier to reason about than three places that had to agree. I checked behavior parity first, since the claim is "no rendered change"; I could not construct a reachable input where the output differs. Nits below.

Parity checks I ran

Owned-panel routing is equivalent. The ownedPanel IIFE (packages/ui/src/tool-activity.tsx:217-233) is the deleted resultOwnsOwnPanel (packages/ui/src/tool-activity/result-projection.ts:71-88 on main) verbatim: connector + json first, then the same six-kind switch. ownsPanel = ownedPanel !== undefined || requiresBypass (:234) matches the old resultOwnsOwnPanel(item) || requiresBypass, and showResult / displayResult / invocationLine / showLiveStream (:235-248) keep their old guards, so invocation suppression and the live-to-settled switch are unchanged.

The new if-chain reproduces the old IIFE's priority. Walking the reachable combinations against main's shared-well block (packages/ui/src/tool-activity.tsx:286-320): json result → :295 (same title, because describeQuietJsonPresentation keeps the headline !== invocationLine rule); settled non-owned result → :305; raw args dump → :319; invocation-line fallback → :329. :319 is unreachable when showResult is true (:305 catches it first), which is exactly the old !showResult guard on argsBody (main:289-292). The item.args && truthiness test also matches the old hasSharedPanelContent disjunct (main:243), so falsy-but-defined args still render nothing rather than a （空） block. One nice side effect: the old code could emit an empty <div data-slot="tool-output" class="maka-tool-output-stack"> when the IIFE returned null; that's gone.

Per-tool special-casing survives. WriteStdin + shell_run still reaches PtyControlPreview, because the owned branch passes toolName/args (:269-280) and ToolResultPreview keys off them (packages/ui/src/tool-activity/tool-result-preview.tsx:219); ptyControlResult still suppresses only the sandbox banner (:251-255); the load-tool renderer is still selected only for connector tools, and passing item.result instead of displayResult is safe because withLiveStreamFallback is the identity for json.

The props widening is inert. The shared branch now passes toolName/args/shellRunSource (:305-317) where main's shared call site passed none (main:309-315). Those three are read only in the shell_run branch of ToolResultPreview, and shell_run is always owned, so the shared branch can never observe them. No API break either: ToolResultPreview's props are untouched, and result-projection.ts isn't re-exported from index.ts/components.tsx, so dropping resultOwnsOwnPanel breaks no consumer.

No accessibility delta. actionIdentity is computed once (:426-431) from the same two parts as outputActionIdentity, so every copy button's aria-label (Copy: <tool>) is unchanged, and data-slot/data-kind are still emitted by the same components in the same places.

Nits

1. The argsOnly branch has no test, and the test named for it doesn't reach it. tool-activity.tsx:319-327 requires !invocationLine, but the new case uses args: { alpha: 1, beta: true } (packages/ui/src/__tests__/tool-activity-presentation.test.ts:692-702). formatToolInvocationLine falls through to its last resort and returns alpha: 1 / beta: true (packages/core/src/tool-quiet-preview.ts:326), so !invocationLine is false and the render lands on the invocation-line branch (:329). /alpha/, /beta/ and the panel count all match either way — the test would still pass with :319-327 deleted. That branch is reachable in production: projectToolActivityArgs projects todo_write args to {} (packages/core/src/tool-activity-args.ts:185), which produces no key/value lines → undefined invocation line → the language="json" （空） dump. Using args: {} (or an array) there would cover it.

2. Three of the six owned kinds are absent from the family test. :222-228 still routes web_search, web_search_error and rive_workflow to bespoke cards that emit no data-slot="tool-output" (tool-result-preview.tsx:759, :768, :821, :718). The parametrized list (tool-activity-presentation.test.ts:496-599) covers file_diff / terminal / shell_run pipes+pty only, so the "never nest these in the shared well" invariant is unasserted for exactly the kinds that would silently gain a .maka-tool-output-stack wrapper (which carries margin-top: 4px) if someone dropped them from the switch. Adding them with an expected count of 0 plus doesNotMatch(/maka-tool-output-stack/) would close that gap.

3. describeQuietJsonPresentation stops one step short of unifying. tool-result-preview.tsx:239-240 runs body and title through formatUserVisibleToolText, while the quietText arm of ToolCallDetailBody (tool-activity.tsx:362-369) passes them through raw. That matches main (main:295-305 skipped it for the shared quiet panel too), so it isn't a regression — but the helper's contract is now "returns text that some callers localize and others don't", which is the kind of drift this refactor is meant to remove. A one-line comment saying the asymmetry is deliberate would be enough.

4. nit: outputPanelCount (tool-activity-presentation.test.ts:44-46) duplicates the inline markup.match(/data-slot="tool-output"/g) idiom already used at :408 and :440; reusing the helper there keeps one spelling.

5. nit: both checklist boxes at the bottom of the description are unchecked even though the Verification section reports lint/format/typecheck/tests green — worth ticking or calling out what's outstanding, since the before/after screenshots follow-up is still open too.