fix(ui): simplify grouped tool activity#100318
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2fa982d97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const resultContent = hasToolResultBlock | ||
| ? resultOnlyContent |
There was a problem hiding this comment.
When a result message has array content with a tool_result block but keeps the call id/name on the top-level toolCallId/toolName fields, this branch copies the raw block and drops that metadata. extractToolCards accepts that shape before the merge, but the merged message is later re-extracted without resultMessage, so the result block no longer matches the call and renders as a second generic tool row instead of one coalesced execution. Enrich or synthesize the preserved result block with resultCard.callId/resultName before appending it.
Useful? React with 👍 / 👎.
|
Codex review: needs changes before merge. Reviewed July 5, 2026, 9:52 AM ET / 13:52 UTC. Summary PR surface: Source +133, Tests +376. Total +509 across 12 files. Reproducibility: yes. Source inspection shows a result message with an existing Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Normalize preserved result blocks at the merge boundary so every coalesced execution retains call id, tool name, output, media, canvas previews, and error state under one compact expandable row. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows a result message with an existing Is this the best way to solve the issue? No. The UI direction is reasonable, but the implementation is not the best fix until it enriches or synthesizes preserved result blocks before appending them. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ccdbb70cf09e. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +133, Tests +376. Total +509 across 12 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
|
Merged via squash.
|
What Problem This Solves
Grouped tool activity is visually noisy: one logical execution can appear as separate call and result rows, alternating rows use different card backgrounds, and labels repeat in forms such as
Bash bashorHeartbeat Respond heartbeat_respond. The group header also repeats the tool categories already visible below it.Why This Change Was Made
This normalizes provider tool identifiers and coalesces only adjacent, matching call/result pairs into one logical execution. The merged row preserves expandable output, non-text content, error state, and canvas previews. Tool rows now use the normalized tool role for flat shell chrome, redundant summary text is suppressed after case/separator normalization, and the activity header shows only the execution count.
The broader normalization and regression coverage are intentional: provider payloads use several call-ID spellings, and collapsing rows without retaining those identities can hide output or merge unrelated events.
Implementation and review were AI-assisted with Codex under maintainer direction.
User Impact
Tool activity is easier to scan: one transparent row per logical execution, one consistent compact rhythm, no duplicated label/detail text, and no repeated category list in the header. Users can still expand each row to inspect input, output, media, canvas content, and real errors; recovered failures remain visually neutral.
Evidence
Before and after
Verification
pnpm test ui/src/pages/chat/chat-thread.test.ts ui/src/pages/chat/components/chat-tool-cards.test.ts ui/src/pages/chat/components/chat-tool-cards.node.test.ts ui/src/pages/chat/components/chat-message.test.ts ui/src/pages/chat/chat-responsive.browser.test.ts— 5 files, 201 tests passedpnpm ui:build— passedOPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 CI=1 PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false pnpm check:changed -- <12 touched files>— passed.agents/skills/autoreview/scripts/autoreview --mode local— clean; no accepted/actionable findings remainLocal Testbox was unavailable because this machine has neither the Blacksmith binary nor an AWS execution role. The repository-native prepare/landing flow will require exact-head hosted proof before merge.