APP-2527 Phase 3: MCP JSON tree rendering#12829
Conversation
Replace the flat pretty-printed JSON text in MCP tool call detail view with an interactive, collapsible JSON tree. - Update json_tree.rs callbacks to accept &mut EventContext so callers can dispatch typed actions for toggle/copy-json interactions - Add CopyJsonToClipboard action to RequestedCommandViewAction to bridge EventContext right-click handlers to ViewContext clipboard writes - Add mcp_scroll_state field to RequestedCommandView to persist scroll position across renders - Replace should_render_mcp_content rendering: request section shows JSON tree (or '(no arguments)' placeholder), response section shows tree/error/cancelled once a finished result arrives, wrapped in a scrollable constrained to MAX_EDITOR_HEIGHT Co-Authored-By: Oz <oz-agent@warp.dev>
cephalonaut
left a comment
There was a problem hiding this comment.
Phase 3 Code Review — APP-2527 MCP JSON Tree Rendering
Verdict: MINOR — core implementation is correct; four items need resolution before merge.
✅ Passing criteria
should_render_mcp_contentblock fully replaced by Request + Response sections callingrender_json_tree✓- Request section renders the tree when args are known, or
"(no arguments)"placeholder whenmcp_requestisNone(Behavior 29) ✓ - Response section gated on
finished_result()only;mcp_result_to_renderablecorrectly dispatches toTree/Error/Cancelled(Behavior 28) ✓ - Tree body wrapped in
NewScrollable::vertical(Clipped) +ConstrainedBox::with_max_height(MAX_EDITOR_HEIGHT)(Behavior 17) ✓ mcp_scroll_state: ClippedScrollStateHandleadded to struct and initialized viaDefault::default()✓ToggleJsonNodecallbacks dispatchMcpTree::Request/McpTree::Responsecorrectly ✓on_copy_jsonserializes withserde_json::to_string_prettyand dispatchesCopyJsonToClipboard; handler callsctx.clipboard().write(...)(Behavior 27) ✓SelectableArea+mcp_content_selection_handlewraps the constrained scrollable (Behavior 25) ✓- Outer
Containerretainsbackground+ bottomCornerRadius;with_borderand margin structure unchanged (Behavior 33) ✓ - No hard-coded colors — all resolved through
JsonTreeColors::from_theme(theme)andtheme.ui_error_color()(Behavior 18–20) ✓ CopyJsonToClipboardusesClipboardContent::plain_textviactx.clipboard().write(...)— consistent with other copy surfaces ✓ToggleJsonStringhandler wired inhandle_actionbut not yet dispatched from UI — acceptable follow-up ✓- No spec/process references in comments ✓
- PR body includes full manual validation checklist from TECH.md Phase 3 ✓
Spec deviation — &mut EventContext in callbacks (evaluated as required): Adding EventContext as the first argument to on_toggle and on_copy_json is the minimal necessary change. Without it, closures captured inside Hoverable::on_click/on_right_click cannot call ctx.dispatch_typed_action(...). Alternatives (returning a Box<dyn Action>, using a channel) would be more complex and less idiomatic. No existing callers outside this PR are affected (the component was new in Phase 1). The change is correct and appropriate.
⚠️ Minor issues (must be resolved before merge)
1. No right-click context menu — Behavior 26 and PR checklist item 9 unmet
render_container_node and render_scalar_row invoke on_copy_clone directly on right-click rather than showing a Menu with "Copy" and "Copy JSON" items. PRODUCT.md Behavior 26 explicitly requires a context menu; TECH.md Design §E1 (recommended) specifies a warpui Menu. The PR's own checklist item — "Copy menu item is greyed out" — cannot be verified because there is no menu to inspect.
Text selection + Cmd+C still works via SelectableArea, so Behavior 25 is met for the keyboard path, but the right-click surface deviates from spec. This was not implemented in Phase 1 and Phase 3 inherits the gap.
2. Padding moved from outer Container to scroll child
The criterion is "Outer Container (padding, background, bottom corner radius) unchanged." INLINE_ACTION_HORIZONTAL_PADDING and REQUESTED_COMMAND_BODY_VERTICAL_PADDING were removed from the outer Container and applied to the inner Container that is the SingleAxisConfig::Clipped child. The background and corner radius correctly remain on the outer container.
Moving padding inside the scroll viewport is the correct UX approach (so the tree content is padded from the viewport edges, not the clip boundary), but it is a visual change: the scrollbar thumb now appears flush with the container edge rather than inset by INLINE_ACTION_HORIZONTAL_PADDING. Verify against the existing shell-command body rendering to ensure the visual treatment is consistent with the rest of the action container.
3. tree_font_size local duplicates private constant
// requested_command.rs (new)
let tree_font_size = 12.0_f32;json_tree.rs already defines const TREE_FONT_SIZE: f32 = 12.; but it is private. The local literal is used only for the "no arguments" / error / cancelled label text in requested_command.rs. Either make TREE_FONT_SIZE pub and import it, or document the value with a comment so the coupling is explicit and a future change to TREE_FONT_SIZE isn't silently missed here.
4. depth proxy in ToggleJsonNode handler is implicit
RequestedCommandViewAction::ToggleJsonNode { path, tree } => {
let depth = path.len(); // depth == path length by tree construction
...
}Using path.len() as depth is correct — the render tree is built such that depth always equals path length (root: [] / depth 0, first child: [Key] / depth 1, etc.). This invariant is implicit; a one-line comment (as shown) would prevent a future maintainer from wondering if the proxy is exact or approximate.
Reviewed against TECH.md Phase 3 criteria and PRODUCT.md.
cephalonaut
left a comment
There was a problem hiding this comment.
Phase 3 Static Verification — APP-2527 MCP JSON Tree Rendering
Static Checks
| Check | Result |
|---|---|
serde_json::to_string_pretty removed from flat render path |
✅ PASS — two remaining calls are correct (inside on_copy_req/on_copy_resp for Copy JSON) |
render_json_tree called twice (request + response) |
✅ PASS — lines 1562, 1621 |
NewScrollable + ConstrainedBox::with_max_height(MAX_EDITOR_HEIGHT) present |
✅ PASS |
Icon::ChevronDown/Icon::ChevronRight in json_tree.rs; no render_expansion_icon import |
✅ PASS |
No hard-coded color literals in json_tree.rs |
✅ PASS |
mcp_scroll_state: ClippedScrollStateHandle added and initialized |
✅ PASS |
on_toggle dispatches ToggleJsonNode { path, tree: McpTree::Request/Response } |
✅ PASS |
CopyJsonToClipboard uses ctx.clipboard().write(ClipboardContent::plain_text(...)) |
✅ PASS |
SelectableArea + mcp_content_selection_handle still wraps content |
✅ PASS |
Outer Container retains background and bottom corner radius (Behavior 33) |
✅ PASS |
| No hard-coded colors in new render path | ✅ PASS |
| Code comments: no spec/process references | ✅ PASS |
| PR body contains manual validation checklist | ✅ PASS |
cargo fmt clean |
✅ PASS |
ToggleJsonString defined + handler present but not wired in render |
✅ Acceptable follow-up |
Spec Deviation: &mut EventContext in callbacks
The change from Fn(Vec<PathSegment>, usize) to Fn(&mut EventContext, Vec<PathSegment>, usize) is the minimal necessary change: Hoverable click callbacks receive &mut EventContext, and callers need it to dispatch typed actions via ctx.dispatch_typed_action(...). Without it, a channel or side-channel workaround would be needed. There is only one caller (requested_command.rs), so no existing callers are broken. The design rationale is documented in the CopyJsonToClipboard doc comment. Acceptable.
Minor Items
M1 — No context menu on right-click (pre-existing from Phase 1, not a Phase 3 regression)
Right-click on a tree row directly dispatches CopyJsonToClipboard (copying JSON immediately) rather than showing a context menu with "Copy / Copy JSON" items as specified in PRODUCT.md Behavior 26 and Design §E1. The "Copy" item (greyed out when no selection) is absent from the right-click path. Confirmed that Phase 1 shipped the same behavior — this is a carry-forward from Phase 1, not introduced here. The text-selection Copy path still works via SelectableArea. Should be tracked as a follow-up for the full menu, not a blocker on this PR.
M2 — PR checklist missing screenshot item
The TECH.md Phase 3 checklist includes: "Screenshots of expanded tree in dark and light themes attached to the PR." This item is absent from the PR body checklist and no screenshots are attached.
Manual Testing Checklist
The following items cannot be verified statically and require a running Warp instance with a local MCP server configured:
- Chevrons expand/collapse nodes correctly; clicking anywhere on the row toggles (Behavior 9)
- Theme colors apply correctly in dark and light themes; switching theme recolors without restart (Behavior 18-19)
- Long-string elision shows truncated preview with expander — note:
ToggleJsonStringnot yet wired inrender_json_tree; long strings render but expander click is a no-op (known gap) - Right-click copies correct subtree JSON to clipboard (Behavior 27)
- Tree scrolls when taller than
MAX_EDITOR_HEIGHT; outer block list scroll unaffected (Behavior 17) -
SelectableArea+Hoverableright-click interaction does not conflict (TECH.md risk item) - Non-MCP actions (commands, file edits) are visually unchanged (Behavior 35)
- Error and cancelled tool calls show "Error: …" / "Cancelled" labels (Behavior 28)
- "(no arguments)" placeholder renders when
mcp_requestis absent (Behavior 29) - Scroll position preserved when re-entering MCP content (via
mcp_scroll_state) - Screenshots of expanded tree in dark and light themes
Verdict: MINOR — Core implementation correct; two minor items to address before merge: acknowledge the menu gap as a tracked follow-up, and add the screenshot checklist item to the PR body.
M1: Add right-click context menu with Copy/Copy JSON items - Add ShowMcpContextMenu, CopyMcpSelection, CloseMcpContextMenu actions - Add mcp_context_menu ViewHandle and mcp_context_menu_open flag to view - on_copy callbacks now dispatch ShowMcpContextMenu instead of directly copying JSON; the menu Copy JSON item dispatches CopyJsonToClipboard and Copy writes the current text selection - Wrap MCP section in SavePosition so menu can be anchored below it - Show menu via Dismiss+positioned overlay in root_stack M2: Move body padding to outer Container to match shell-command layout - Remove padding from inner scroll-child container - Add INLINE_ACTION_HORIZONTAL_PADDING and REQUESTED_COMMAND_BODY_VERTICAL_PADDING to the outer Container that wraps the SelectableArea M3: Export TREE_FONT_SIZE from json_tree.rs and use it in render - pub const TREE_FONT_SIZE to avoid silent duplication M4: Add comment explaining depth == path.len() in ToggleJsonNode handler Co-Authored-By: Oz <oz-agent@warp.dev>
cephalonaut
left a comment
There was a problem hiding this comment.
Re-review: M1–M4 status + one new blocker
All four requested fixes are correctly implemented. However, the rewrite of the MCP render block accidentally dropped a call that existed outside that block and is now dead code.
✅ M1 — Right-click context menu
ShowMcpContextMenu, CopyMcpSelection, and CloseMcpContextMenu action variants are present and fully handled. The menu is constructed with MenuVariant::Fixed, "Copy" is greyed out when there is no selection (with_disabled(!has_selection)), and "Copy JSON" always fires CopyJsonToClipboard. The menu is overlaid via root_stack.add_positioned_child anchored to the SavePosition ID of the MCP section. PASS.
✅ M2 — Padding on outer Container
INLINE_ACTION_HORIZONTAL_PADDING and REQUESTED_COMMAND_BODY_VERTICAL_PADDING are now on the outer Container that wraps selectable_content, not on the inner scroll child. Confirmed against the pre-fix commit (d97a2568e). PASS.
✅ M3 — TREE_FONT_SIZE made pub const
pub const TREE_FONT_SIZE: f32 = 12. in json_tree.rs, imported via use crate::ui_components::json_tree::{..., TREE_FONT_SIZE}; in requested_command.rs. No local duplicate. PASS.
✅ M4 — Depth comment
A node's depth in the tree always equals its path length: the root has an empty path (depth 0) and each level down adds one segment.
Present immediately above let depth = path.len();. PASS.
🚫 NEW BLOCKER — maybe_render_footer call was dropped
In the previous commit (d97a2568e), maybe_render_footer(app) was called after the if should_render_mcp_content { … } block, not inside it — rendering citations footers and autonomy speedbumps for all command types (MCP and shell commands alike). The PR removed that call entirely; maybe_render_footer is now dead code.
The impact: citations ("Copied from" / "Derived from" chips) and autonomy setting speedbumps (ShouldShowForAutoexecutingReadonlyCommands, ShouldShowForProfileCommandAutoexecution) will no longer appear on any requested-command block. The has_citations_footer variable still suppresses the header's rounded bottom corner even though there is no footer, causing a visual glitch on top of the functional regression.
The fix is straightforward — restore the call after the MCP content block:
} // end if should_render_mcp_content
if let Some(footer) = self.maybe_render_footer(app) {
content.add_child(Clipped::new(footer).finish());
}
let border_color = …Minor — action variant doc comments describe callers
The new doc comments on CopyJsonToClipboard, ShowMcpContextMenu, and CopyMcpSelection each contain a sentence describing which UI element triggers them ("Triggered from …", "Used by …"). Per the project comment policy, comments should take a local view and not describe the behavior of callers. These lines should be dropped from the doc comments. Not blocking by itself, but should be cleaned up alongside the footer fix.
- Restore the maybe_render_footer call after the MCP content block; it was accidentally dropped in the M1 rewrite, causing citation footers and autonomy speedbumps to stop rendering for all requested-command blocks - Remove caller-describing sentences from CopyJsonToClipboard, ShowMcpContextMenu, and CopyMcpSelection doc comments; keep only the local "why" perspective Co-Authored-By: Oz <oz-agent@warp.dev>
cephalonaut
left a comment
There was a problem hiding this comment.
Final re-review — PASS
Commit 3b54a1cf4 cleanly addresses both outstanding items. Only requested_command.rs was touched.
✅ Blocker resolved — maybe_render_footer restored
The call is back in exactly the right position: after the if should_render_mcp_content { … } closing brace and before let border_color. Citations footers and autonomy speedbumps will render correctly for all requested-command block types again.
✅ Minor resolved — doc comments no longer describe callers
CopyJsonToClipboard: caller-describing sentence removed; only the local "write to clipboard" description remains.ShowMcpContextMenu: revised to explain why thejson_textfield is carried, without referencing caller behavior.CopyMcpSelection: simplified to "Copy the currently selected MCP tree text to the clipboard."CloseMcpContextMenu: already clean.
No other code was changed. PR is ready to merge.
SelectableArea was intercepting mouse events on macOS, preventing chevron Hoverables from receiving clicks. The right-click context menu handles copy actions, so SelectableArea is no longer needed here. Also remove the now-unused SelectableArea import. Co-Authored-By: Oz <oz-agent@warp.dev>
Bug 1 fix — stable MouseStateHandle per tree row: Add RefCell<HashMap<Vec<PathSegment>, MouseStateHandle>> to JsonTreeState. Add mouse_state_for(&path) which creates and caches a handle on first access. Use state.mouse_state_for(&path) in render_container_node and render_scalar_row instead of the inline MouseStateHandle::default(). Inline default() discards click_count between LeftMouseDown -> ctx.notify() re-render -> LeftMouseUp, so click handlers never fire. Stable handles fix this. Restore SelectableArea around the tree; its depth-first dispatch lets Hoverable rows receive LeftMouseDown first, so clicks and drag-selection coexist correctly. Bug 2 fix — per-row context menu anchoring: Add position_id_prefix: &str to render_json_tree and thread it through render_value, render_container_node, render_scalar_row. Wrap each interactive row's Hoverable in a SavePosition keyed by prefix + path. Extend on_copy_json callback signature to carry the row's anchor ID. Add anchor_id field to ShowMcpContextMenu action and mcp_context_menu_anchor_id: Option<String> to the view struct. The root_stack menu overlay now anchors to the right-clicked row's SavePosition instead of the whole section. Co-Authored-By: Oz <oz-agent@warp.dev>
Default expansion: all nodes open
Change JsonTreeState::is_expanded to return true by default for all
depths (unwrap_or(true)). The tree is now fully open on first render;
the height cap + scroll keep it bounded. Update the three
depth-specific tests to reflect the new default.
Update PRODUCT.md B13/B14: remove 'nested collapsed by default' and
the open-question about auto-collapse caps.
Long string elision
Add string_mouse_states: RefCell<HashMap<...>> to JsonTreeState and
mouse_state_for_string(&path) accessor (stable handle for
long-string Hoverables, separate namespace from container handles).
Add on_toggle_string callback parameter to render_json_tree,
render_value (replaces the old build_string_value_text approach).
Add render_long_string_row: uses is_string_expanded to choose between
preview mode (first line + threshold chars + ellipsis + ChevronRight)
and expanded mode (full wrapped text + ChevronDown). Chevron occupies
the standard left position, making the column visually consistent
with container nodes.
Thread on_toggle_string_req / on_toggle_string_resp callbacks from
requested_command.rs; dispatch ToggleJsonString{tree:Request/Response}.
Co-Authored-By: Oz <oz-agent@warp.dev>
What
Replace the flat pretty-printed JSON text in the MCP tool call detail view with an interactive, collapsible JSON tree rendered by the
JsonTreeViewcomponent built in Phases 1 and 2.json_tree.rs: Updated
render_json_tree,render_value,render_container_node, andrender_scalar_rowcallback signatures to accept&mut EventContextas first argument, allowing callers to dispatch typed actions from Hoverable click handlers.requested_command.rs:
CopyJsonToClipboard { text: String }toRequestedCommandViewActionto bridge EventContext right-click handlers (which lack clipboard access) toViewContext::clipboard()inhandle_actionmcp_scroll_state: ClippedScrollStateHandlefield to preserve scroll position across rendersshould_render_mcp_contentbody: request section rendersrender_json_treewithmcp_request_tree_state(or a "(no arguments)" placeholder), response section renders tree/error/cancelled once a finished result is available, all wrapped in aNewScrollable+ConstrainedBox::with_max_height(MAX_EDITOR_HEIGHT)+SelectableAreaWhy
Replaces an unformatted flat JSON blob with an interactive tree that lets users collapse uninteresting subtrees and expand long strings on demand.
Agent Mode
Manual Validation Checklist (from TECH.md Phase 3)
Conversation: https://staging.warp.dev/conversation/3a48e515-b541-4d6d-bc95-0bd50f8b9124
Run: https://oz.staging.warp.dev/runs/019edff8-41ce-7cf2-891d-3e1c4bcd4608
This PR was generated with Oz.