fix(web/agent-chat): default tool_call/tool_result off, add toolbar toggle#6388
fix(web/agent-chat): default tool_call/tool_result off, add toolbar toggle#6388singlerider wants to merge 3 commits intozeroclaw-labs:masterfrom
Conversation
β¦ript Tool execution is plumbing. The dashboard Agent chat was rendering every tool_call and tool_result frame as its own card in the message stream alongside user messages and the assistant reply, including errored tool calls (e.g. memory_recall returning "Provide at least 'query' ..."). That noise belongs in an activity surface, not the conversation transcript. Gate inline rendering of tool_call / tool_result behind a new showToolActivity flag, default false. Read from localStorage at 'zeroclaw_show_tool_activity'; flip to '1' to opt in. Not in this commit (deliberately separate so the immediate noise stops landing in the transcript before the larger UX work): - Toggle UI for showToolActivity (settings panel or chat-header control). - Routing tool activity into the existing collapsed "Thinking" strip so power users have a discoverable home for it. - Resyncing chatHistoryStorage so already-persisted tool_call cards on disk also disappear on reload (today they survive in localStorage if the user had the bug-rendered cards persisted before this fix). Refs zeroclaw-labs#6348.
Adds a discoverable Wrench button next to the existing Compact toggle that flips the same `zeroclaw_show_tool_activity` localStorage flag the gates already read. Operators no longer have to drop into devtools to opt back in to inline tool_call / tool_result cards. Audited the full WsMessage union while wiring the toggle: tool_call and tool_result are the only frames that should be gated as "plumbing." cron_result is asynchronous output from a job the operator explicitly scheduled, not internal-to-turn agent reasoning, so it stays in the transcript unconditionally. Updated the gate comment so it does not promise a deferred toggle UI that now exists. Refs zeroclaw-labs#6348
Comments now describe what the gate does, not what PR introduced it.
Audacity88
left a comment
There was a problem hiding this comment.
I reviewed the live PR state for #6388 at head f487936, the linked issue #6348, the current diff for AgentChat.tsx / i18n.ts, existing reviews/comments, and the failing CI job log. There are no prior reviews or inline threads yet. I did not run a local web build because the source-level issue below blocks the advertised toggle behavior.
π’ What looks good β default hiding matches the issue
Hiding tool_call and tool_result frames by default is the right direction for #6348. The distinction in the PR body also looks right: routine tool execution is chat plumbing, while cron_result is an explicit scheduled-job output and should remain visible in the transcript.
π΄ Blocking β the Wrench toggle uses stale state in the WebSocket handler
The new toggle changes React state, but the WebSocket message handler that gates tool_call and tool_result is installed once in the connection effect with an empty dependency list. That handler closes over the initial showToolActivity value from page load.
So with the default false setting, clicking the Wrench button changes the visible button state and localStorage, but the existing ws.onMessage callback still sees showToolActivity === false. Future tool_call and tool_result frames keep getting skipped until the component remounts or the socket handler is recreated. That breaks the second half of this PR: the discoverable opt-in toggle does not actually opt the current chat session back into inline tool activity.
Could you make the message handler read the current flag at message time? A small showToolActivityRef kept in sync with state would fit this pattern, or the socket callback can be safely reattached when the flag changes as long as it does not leak or reconnect unnecessarily.
π‘ Check status β the red 32-bit job looks like CI noise, but it still needs a rerun
The only failing required check I saw is Check (32-bit). Its log shows the job was canceled while installing gcc-multilib, before the Rust check reached this PR's code. Since this is a web-only diff, I do not think that failure is evidence against the patch itself, but the branch still needs a green required-gate rerun before merge.
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
Review β #6388 fix(web/agent-chat): default tool_call/tool_result off, add toolbar toggle
Verdict: comment
Reviewer: WareWolf-MoonWall
Head checked: f487936 (2/12 checks failing)
I read the full diff (AgentChat.tsx, i18n.ts), the PR body, the linked issue #6348, the existing review (Audacity88: CHANGES_REQUESTED), the failing CI log, and the current AgentChat.tsx source in the local codebase to verify the closure structure. I'm not approving over Audacity88's active block.
State of the queue: Audacity88 holds an active CHANGES_REQUESTED on the stale-closure bug. That finding is correct. I'm seconding it and adding a couple of supplementary observations.
On Audacity88's block β I agree and verified it
I read the useEffect(() => {...}, []) block in AgentChat.tsx. The WebSocket connection is established once, and ws.onMessage is assigned inside that effect. The new showToolActivity state is captured at the moment the effect runs β which is the initial render, where showToolActivity is false (the localStorage default). When the user clicks the Wrench toggle, setShowToolActivity updates the React state and re-renders the button correctly, but the existing ws.onMessage callback still holds the closure over the initial false value. New tool_call and tool_result frames keep getting dropped until the component remounts. The toggle is visually responsive but functionally inert for the current session.
The fix Audacity88 described is the right one. Here's the specific pattern, consistent with how pendingContentRef and capturedThinkingRef are already used in this file:
const showToolActivityRef = useRef(showToolActivity);
useEffect(() => {
showToolActivityRef.current = showToolActivity;
}, [showToolActivity]);Then inside ws.onMessage, replace if (!showToolActivity) with if (!showToolActivityRef.current). The ref sync effect is lightweight (no socket reconnect, no message loss), and it keeps the WS effect's [] dependency array intact β which is the right choice since reconnecting the socket every time the toggle changes would be disruptive.
Note: the default-off behavior (showToolActivity === false at mount) works correctly as shipped. The stale closure only affects the toggle-on path. This means the primary fix from #6348 β stopping the noise of tool activity cards in normal use β is already functional. The broken part is opt-in visibility, not the default behavior.
π‘ Warning β inline style prop is inconsistent with the rest of the toolbar
The new Wrench button uses:
style={{ padding: '0.3rem 0.75rem', borderRadius: '0.5rem' }}The existing Compact button alongside it uses only Tailwind class names for layout and sizing. The mixed approach (Tailwind + inline styles) is inconsistent and will make the button harder to retheme or restyle with the rest of the toolbar. The padding and border-radius values here correspond to standard Tailwind classes (px-3 py-1.5 rounded-lg or similar). Prefer using class names for consistency with the surrounding component.
π΅ Suggestion β button label text when default (off) is potentially confusing
When showToolActivity is false (the default), the button displays t('agent.tool_activity_show') β "Show tool activity". That's correct. But a first-time user looking at the toolbar might read "Show tool activity" as a status indicator (tool activity is showing) rather than a call to action (click to show tool activity). The aria-label correctly uses the action framing. The visible label does too, but it might be worth a quick UX check before landing. This is a minor π΅, not a blocker.
π‘ Warning β CI must be green before merge
Two checks are failing. Audacity88 identified the Check (32-bit) failure as CI infrastructure noise (cancelled during gcc-multilib install, never reached the Rust check). That analysis looks correct for a web-only diff. But both failing checks need a clean rerun result before this can merge, regardless of the root cause. Please trigger a rerun and confirm green.
π’ Praise β cron_result explicitly kept out of the gate
The PR body documents why cron_result is treated differently from tool_call/tool_result: it's the output of a job the operator scheduled, not internal agent reasoning. That distinction is correct and explicitly justified. Not gating it prevents silent loss of scheduled-job output when the toggle is off. The reasoning is right, and it's recorded where a future maintainer can find it.
π’ Praise β toggleToolActivity correctly uses functional update
The toggleToolActivity callback is:
const toggleToolActivity = useCallback(() => {
setShowToolActivity((prev) => {
const next = !prev;
try { localStorage.setItem(...); } catch { /* noop */ }
return next;
});
}, []);The empty dependency array is correct here because the functional update form (prev) => !prev doesn't close over showToolActivity β it always acts on the current state value. The localStorage write inside the updater is a side effect in an updater function, which React can call twice in StrictMode, but since writing the same derived value twice is idempotent in localStorage, this is fine in practice.
Template
The PR body fully fills the required sections. The cron_result audit rationale in the body is the kind of explicit scope reasoning that makes future changes safer. The rollback section correctly names both files and the localStorage escape hatch. The compatibility section is accurate.
Summary: Audacity88's stale-closure block is the right call and I've verified it in the source. The default-off behavior works today; it's the toggle-on path that needs the ref fix. Address that, clean up the inline style, trigger a CI rerun, and this is ready for re-review.
Summary
The dashboard's Agent chat was rendering every
tool_callandtool_resultWebSocket frame as a standalone card in the message stream, including errored tool calls (e.g.memory_recallreturning "Provide at least 'query' ..."), alongside the user message and the assistant reply. That noise belongs in an activity surface, not the conversation transcript.Two changes:
tool_callandtool_resultbehind ashowToolActivityflag (default false). The flag persists tolocalStorage.zeroclaw_show_tool_activityfor cross-session memory.aria-pressedsemantics andagent.tool_activity_show/agent.tool_activity_hidei18n strings. Operators can flip the gate without dropping into devtools.Audit of the full WsMessage union confirms
tool_callandtool_resultare the only frames that should be gated as plumbing.cron_resultis asynchronous output from a job the operator explicitly scheduled, not internal-to-turn agent reasoning, so it stays in the transcript unconditionally.Closes #6348
Validation Evidence
Builds cleanly (only the pre-existing chunk-size warnings).
Security & Privacy Impact
Compatibility
localStorage.zeroclaw_show_tool_activity = '1').Rollback
git revert <merge-sha>. Two-file diff (web/src/pages/AgentChat.tsx,web/src/lib/i18n.ts). Operators who specifically want the noisy cards back set the localStorage flag.