This repository was archived by the owner on Sep 4, 2026. It is now read-only.
feat(chat): make MCP and tool rows legible end to end (parser + UI, flag-gated) - #387
Merged
Conversation
ElbertePlinio
force-pushed
the
feat-parser-tool-detail
branch
from
July 26, 2026 01:43
a48e624 to
8eada38
Compare
ElbertePlinio
force-pushed
the
feat-parser-tool-detail
branch
2 times, most recently
from
July 26, 2026 01:57
dcd64df to
53825a9
Compare
ElbertePlinio
force-pushed
the
feat-parser-tool-detail
branch
from
July 26, 2026 02:04
53825a9 to
866360d
Compare
…tatus Shared parser slice for #362 and #365 — both issues named `claude_stream.rs`'s `RememberedTool` and the Bash-only guard in `map_tool_result` as one surface to serialize rather than duplicate, so this does that half once. **Nothing non-Bash ever completed.** `map_tool_result` returned `None` unless the tool was Bash, so every other tool was emitted `InProgress` and never updated — permanently "running" long after it finished. Lifting the guard alone would not have helped: `RememberedTool` collapsed everything non-Bash to a bare `Other`, discarding the name, so there was nothing left to route a completion to. It now retains the tool's identity — Bash, an MCP server/tool pair, or a named tool — and `map_tool_result` dispatches on it. **MCP rows were starved on purpose.** The MCP branch emitted `detail: None` while the non-MCP branch immediately below it attached `compact_input_summary(&input)`. That is why an MCP call rendered as one dead grey line: no args, no result, no completion, disabled chevron. **The generic row starved on a four-key allowlist.** `file_path`, `notebook_path`, `pattern`, `description` — `AskUserQuestion`, `Task`, `Skill`, `Glob`, `ToolSearch` and `Read`-with-offset carry none of them, so `detail` was `None` and the chevron was a dead button. Known keys still win (they are the human-meaningful ones); anything else falls back to a compact rendering of the whole input. Both payloads are capped in Rust, not the view, because these rows are persisted. Argument summaries take the FRONT of the string — the meaningful part of an argument list is at its head, unlike a command's output tail — and are capped far shorter, since they ride on a single collapsed row. Deliberately NOT in this slice, so the two follow-on PRs stay reviewable: `tool_progress` elapsed time and `system`/`status` compacting/requesting (#365 B1), which add a new event variant, and the frontend half of both issues — the timeline item's `status`/`detail` fields, the itemId update path, and the card wiring. Refs #362, #365
The parser half of this change starts emitting completion events for MCP calls. `reduceMcpToolCall` appended unconditionally — it had no itemId match path, unlike its `reduceToolUse` sibling — so a resolving call would visibly double instead of updating. That makes the two halves inseparable: shipping the parser without this turns 'the row never finishes' into 'the row finishes twice'. Refs #362
…ggle (#390) Frontend half of #362 and #365, behind the new default-off `mcpToolDetail` flag. Stacked on the parser slice, which is what makes these events exist. The wire event has always carried `status` and `detail` — `lib/agentChat.ts` declares both on `mcpToolCall` and `toolUse`. The timeline item type did not, so the reducer read them and threw them away, and `ChatTimeline` rendered `<McpCard server tool />` with no `detail` at all. With `detail` undefined `canExpand()` is false, which is exactly the reported symptom: a disabled chevron on a dead grey row. Both item types now carry `status` and `detail`, both optional so a row persisted before the parser learned to resolve non-Bash tools still renders. A completion that carries no result keeps the arg summary already on the row — resolving must never blank what the row was showing. Expansion moves into the hoisted `RowExpansion` map, the same contract `CommandCard` and `ThinkingBubble` use, so the toggle survives the virtualizer disposing the row on scroll-back. That was listed in #372 as a separate annoyance; it comes free here because these cards had to take `open`/`onToggle` anyway. Status renders through `StatusPill`, matching `CommandCard` rather than adding new chrome — bracket-cornered mono, never a filled chip. The two new render branches are extracted out of `renderItem` because adding them inline pushed it past the complexity cap. `flags.test.ts` enumerates the registry, so the new flag is asserted there rather than silently appearing. Refs #362, #365
ElbertePlinio
force-pushed
the
feat-parser-tool-detail
branch
from
July 26, 2026 02:11
98b0ce4 to
50d08a0
Compare
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The backend and frontend halves of #362 and #365, consolidated. #390 merged into this branch, so it ships as one gated unit rather than leaving the parser emitting events nothing renders.
Nothing non-Bash ever completed
Every other tool was emitted
InProgressand never updated — permanently "working" long after it finished.Lifting that guard alone would not have helped:
RememberedToolwas{ Bash, Other }, discarding the name, so there was nothing to route a completion to. It now retains identity (Bash,Mcp { server, tool },Other(name)). Both issues named this type as the thing whichever PR landed first must own.MCP rows were starved on purpose
The MCP branch emitted
detail: Nonewhile the non-MCP branch immediately below it attachedcompact_input_summary(&input). That asymmetry is why an MCP call rendered as one dead grey line with a disabled chevron.The generic row starved on a four-key allowlist
file_path,notebook_path,pattern,description.AskUserQuestion,Task,Skill,Glob,ToolSearchandRead-with-offset carry none of them. Known keys still win; anything else falls back to a capped rendering of the whole input.The frontend threw away what did arrive
lib/agentChat.tshas always declaredstatusanddetailon both events. The timeline item type did not, so the reducer read them and dropped them, andChatTimelinerendered<McpCard server tool />with nodetail— which is precisely whycanExpand()was false.Now: both item types carry
statusanddetail(optional, so rows persisted before this still render); a completion carrying no result keeps the arg summary already on the row; expansion moves into the hoistedRowExpansionmap so the toggle survives scroll-back; status renders throughStatusPill, not new chrome.reduceMcpToolCallgained the itemId match path — it appended unconditionally, unlike itsreduceToolUsesibling. Without it these new completions turn "the row never finishes" into "the row finishes twice". That is why these commits cannot be separated.Capping
In Rust, not the view, because rows are persisted. Output tails keep the 2000-char tail; argument summaries take the head at 300 — the meaningful part of an argument list is at its front, and it rides on one collapsed row.
Flag
mcpToolDetail, default off. A persisted row's shape changed and the parser only emits completions on newer builds, somainstays releasable while this is dark — the same precedent aschangesReview.flags.test.tsenumerates the registry, so the flag is asserted rather than silently appearing.Tests
Every one verified load-bearing.
Rust (8): MCP arg summary +
InProgress → Completed; failed MCP resolvesFailed; a generic tool completes under its own name; Bash still maps toCommandDone; an orphantool_resultstill ignored; summary falls back to whole input, prefers a known key, is capped, and yieldsNoneon empty. Restoring the Bash-only guard fails exactly the three completion tests.Frontend (5): MCP status/detail carried and resolving in place; a
detail: nullcompletion keeping the arg summary; a generic tool reachingfailed; no duplicate row; distinct calls still appending separately.Validation
cargo test -p pickforge-core— 738 passedcargo clippy -p pickforge-core --all-targets -- -D warnings— cleanbun run test:unit— 1606 passedbunx tsc --noEmitclean;bun run lintcleanStill open on these two issues
Stated rather than implied — #365's headline complaint is not fixed here:
tool_progresselapsed time andsystem/statuscompacting/requesting need a new event variant inevent.rs. Without them,WorkingRowstill names nothing and shows no elapsed time.pickforge-lanesrendering lane cards inline. Open decision (live vs snapshot) still unresolved.COMPACT_LINE_PXholds. Re-measure when elapsed lands.Refs #362, #365