fix(console): exclude toggle labels from trace message copy#464
Conversation
Teach CopyButton to accept a sourceRef and walk the DOM itself, skipping any subtree marked with the data-copy-ignore attribute. Trace message toggles, the Reasoning disclosure trigger, and tool-call/other-part label elements are annotated so their text no longer leaks into the clipboard. The walker uses textContent semantics so collapsed/hidden content is still copied in full. Fixes #460 Co-authored-by: Heinrich Wendel <heiwen@users.noreply.github.com>
- Collapse `value` and `sourceRef` into a single `value: string | (() => string) | RefObject<HTMLElement | null>` prop on CopyButton.
- Delete `getNodeText` from Code.tsx; pass a ref on the rendered `<pre>` to CopyButton instead, so the text lives once in the DOM and the copy string is only built on click.
- Update trace details.tsx to use `value={contentRef}` in place of `sourceRef`.
Co-authored-by: Heinrich Wendel <heiwen@users.noreply.github.com>
📝 WalkthroughWalkthroughCopyButton now accepts element refs and extracts clipboard text by walking the referenced DOM (skipping nodes marked ChangesCopy Button Ref-Based Text Extraction
Sequence DiagramsequenceDiagram
participant User
participant CopyButton
participant contentRef
participant collectCopyText
participant Clipboard
User->>CopyButton: clicks copy
CopyButton->>contentRef: read ref
CopyButton->>collectCopyText: traverse DOM subtree
collectCopyText->>collectCopyText: skip nodes with data-copy-ignore
collectCopyText-->>CopyButton: resolved text
CopyButton->>Clipboard: writeText(resolved text)
Clipboard-->>User: content copied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26fa2ddde8
ℹ️ 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".
| .replaceAll(/\n{3,}/g, "\n\n") | ||
| .trim(); |
There was a problem hiding this comment.
The new DOM walker normalizes output with .replaceAll(/\n{3,}/g, "\n\n").trim(), which mutates copied text by removing leading/trailing whitespace and collapsing intentional blank lines. This now affects CodeBlock because it was switched to CopyButton value={preRef} in this commit, so pasted code can differ from the rendered snippet (for example, snippets with meaningful leading indentation at the start, trailing newline, or 3+ consecutive blank lines). Copy actions should preserve exact text rather than rewriting whitespace.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/console/app/routes/_shell.agent.$agentSlug.branch.$branchSlug.traces.($traceId)/details.tsx (1)
370-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
data-copy-ignorefrom tool-call names and part-type labels; they are semantic content, not UI affordances.When copying message content, the tool call name and part type are essential context that originates from
part.nameandpart.typein the message data structure. Currently,data-copy-ignorestrips these labels, leaving only the JSON payload with no indication of which tool was invoked or what type of part it represents.The toggle buttons (
More/LessinCollapsibleTextandCollapsibleCode, and theReasoningdisclosure trigger) correctly usedata-copy-ignoresince they are UI affordances. Tool names and part types, however, are data content and should remain in the clipboard text.Proposed change
{toolCalls.map((tc, index) => ( <div key={`tool-call:${index}`} className="space-y-2"> - <Badge variant="outline" data-copy-ignore> + <Badge variant="outline"> <Wrench className="size-3" /> {tc.name} </Badge> <CollapsibleCode code={tc.arguments} maxLength={300} /> </div> ))} {otherParts.map((part, index) => ( <div key={`${part.type}:${index}`} className="space-y-2"> - <div - data-copy-ignore - className="text-xs font-medium text-muted-foreground uppercase" - > + <div className="text-xs font-medium text-muted-foreground uppercase"> {part.type} </div> <CollapsibleCode code={part.value} maxLength={300} /> </div> ))}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/console/app/routes/_shell.agent`.$agentSlug.branch.$branchSlug.traces.($traceId)/details.tsx around lines 370 - 390, Remove the data-copy-ignore attribute from the tool call name Badge and the part-type label div so semantic content is included when copying; locate the JSX rendering toolCalls (Badge with {tc.name}) and otherParts (the div showing {part.type}) and delete the data-copy-ignore prop there, keeping data-copy-ignore only on UI affordance elements like the CollapsibleCode/CollapsibleText "More/Less" toggles and the Reasoning disclosure trigger.
🧹 Nitpick comments (2)
apps/console/app/routes/_shell.agent.$agentSlug.branch.$branchSlug.traces.($traceId)/details.tsx (1)
347-349: 💤 Low valueCopy on the empty-message branch silently writes
"".When this branch renders,
contentRefis never attached andresolveText()returns"", so clicking copy succeeds with an empty clipboard. Minor UX issue; consider hidingCopyButton(or disabling it) when there is no content to copy.♻️ Optional — gate the copy button on having content
- <div className="flex items-center justify-between"> + {const hasContent = reasoning || texts.length > 0 || toolCalls.length > 0 || otherParts.length > 0;} + <div className="flex items-center justify-between"> <div className="flex items-center gap-2"> ... </div> - <CopyButton value={contentRef} /> + {hasContent && <CopyButton value={contentRef} />} </div>(Alternatively, lift the
hasContentboolean to replace the inline boolean expression on line 347.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/console/app/routes/_shell.agent`.$agentSlug.branch.$branchSlug.traces.($traceId)/details.tsx around lines 347 - 349, The empty-message branch renders "(no message)" but still allows CopyButton to copy an empty string because contentRef is not attached and resolveText() returns ""; update the conditional that renders the CopyButton (or lift/create a hasContent boolean) to check for actual content before rendering/enabling CopyButton — use contentRef, resolveText(), texts, toolCalls, otherParts and reasoning to compute hasContent and either hide or disable the CopyButton in the branch that currently renders the "(no message)" paragraph so copy is impossible when there is nothing to copy.packages/shared-ui/src/components/Code.tsx (1)
13-36: 💤 Low valueLGTM — clean migration from
getNodeTextto ref-based copy.Passing
preRefdirectly toCopyButtonremoves the separate React-node-to-text traversal in favor of the rendered<pre>subtree, which is exactly what the newCopyButtonAPI expects. Code content is preserved verbatim (text nodes'\ns are kept;<pre>is recognized as a block but the wrapping newlines are stripped by the final.trim()).Minor nit:
className=""at Line 26 is a no-op and can be omitted for symmetry with the consumers that pass real classes.✨ Tiny cleanup
- <CopyButton value={preRef} className="" /> + <CopyButton value={preRef} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared-ui/src/components/Code.tsx` around lines 13 - 36, Remove the no-op className="" prop passed to CopyButton in the title branch: locate the conditional that renders CopyButton alongside the title (uses title ? ... : ...), and update the CopyButton call that currently includes className="" to omit the empty prop so it matches the other branch that passes a real class; the CopyButton and preRef identifiers are the references to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@apps/console/app/routes/_shell.agent`.$agentSlug.branch.$branchSlug.traces.($traceId)/details.tsx:
- Around line 370-390: Remove the data-copy-ignore attribute from the tool call
name Badge and the part-type label div so semantic content is included when
copying; locate the JSX rendering toolCalls (Badge with {tc.name}) and
otherParts (the div showing {part.type}) and delete the data-copy-ignore prop
there, keeping data-copy-ignore only on UI affordance elements like the
CollapsibleCode/CollapsibleText "More/Less" toggles and the Reasoning disclosure
trigger.
---
Nitpick comments:
In
`@apps/console/app/routes/_shell.agent`.$agentSlug.branch.$branchSlug.traces.($traceId)/details.tsx:
- Around line 347-349: The empty-message branch renders "(no message)" but still
allows CopyButton to copy an empty string because contentRef is not attached and
resolveText() returns ""; update the conditional that renders the CopyButton (or
lift/create a hasContent boolean) to check for actual content before
rendering/enabling CopyButton — use contentRef, resolveText(), texts, toolCalls,
otherParts and reasoning to compute hasContent and either hide or disable the
CopyButton in the branch that currently renders the "(no message)" paragraph so
copy is impossible when there is nothing to copy.
In `@packages/shared-ui/src/components/Code.tsx`:
- Around line 13-36: Remove the no-op className="" prop passed to CopyButton in
the title branch: locate the conditional that renders CopyButton alongside the
title (uses title ? ... : ...), and update the CopyButton call that currently
includes className="" to omit the empty prop so it matches the other branch that
passes a real class; the CopyButton and preRef identifiers are the references to
update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4aa71b8b-3289-4a08-ae48-b23df2efd70b
📒 Files selected for processing (3)
apps/console/app/routes/_shell.agent.$agentSlug.branch.$branchSlug.traces.($traceId)/details.tsxpackages/shared-ui/src/components/Code.tsxpackages/shared-ui/src/components/CopyButton.tsx
|
@claude what do you think about the codereview comments? |
|
Claude finished @heiwen's task in 1m 48s —— View job My take on the review commentsI read through both reviewer comments and the code. Here's my honest assessment, ranked by what I'd actually act on: 1. Codex P1 — whitespace normalization clobbers code ✅ Legit, worth fixing
This is a real bug now that
The whitespace normalization makes sense for prose/message copies where block-level walking produces spurious double-newlines, but it's wrong for a
I'd lean toward the targeted fix — it's smaller and the 2. CodeRabbit "Major" —
|
| Comment | Verdict |
|---|---|
| Codex: whitespace normalization breaks code copies | Fix — real bug for CodeBlock |
| CodeRabbit: include tool names / part types in copy | Skip — intentional per your test plan |
| CodeRabbit: empty-message copy button | Skip / cosmetic |
CodeRabbit: empty className="" |
Optional cleanup |
Want me to push the fix for the Codex whitespace issue? I'd special-case <pre> root to preserve verbatim text.
· branch claude-issue-460-20260511-0724
|
@claude how was the whitespace normalization handled before? |
|
Claude finished @heiwen's task in 1m 25s —— View job Whitespace normalization — before vs. afterThere was no whitespace normalization before. The two old call sites each got it "for free" in different ways, neither of which collapsed blank lines or trimmed: 1. <CopyButton value={getNodeText(children)} />
const getNodeText = (node: React.ReactNode): string => {
if (typeof node === "string" || typeof node === "number") return String(node);
if (Array.isArray(node)) return React.Children.toArray(node).map(getNodeText).join("");
if (React.isValidElement(node)) return getNodeText(node.props.children);
return "";
};It just concatenated the string/number leaves of the React tree, verbatim — leading indent, trailing 2. <CopyButton value={() => contentRef.current?.innerText ?? ""} />This delegated to the browser's
So in both old paths, blank-line groupings and leading/trailing whitespace were preserved. The new Why it matters for the Codex findingFor function collectCopyText(root: HTMLElement): string {
if (root.tagName === "PRE") return root.textContent ?? "";
// …existing walker + normalize
}That restores the old Want me to push that fix? |
|
@claude why do we need the BLOCK_TAG_PATTERN in the first place, can't we simplify that? |
|
Claude finished @heiwen's task in 2m 5s —— View job Why
|
|
@claude it feels more complicated than it needs to be. why not simply walk the whole HTML tree recursively, like it was done previously in getNodeText()? |
|
Claude finished @heiwen's task in 2m 55s —— View job Why not just
|
|
@claude ok, go with this version: |
|
Claude finished @heiwen's task in 2m 11s —— View job Simplified
|
Drop the over-broad block-tag regex and the chunks/pushBlockBreak bookkeeping. Pre-formatted roots short-circuit through textContent so CodeBlock copies preserve indentation and trailing newlines verbatim instead of being mangled by the trim/collapse pass. Co-authored-by: Heinrich Wendel <heiwen@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared-ui/src/components/CopyButton.tsx (1)
15-31: 💤 Low valueNested
<pre>loses verbatim formatting.The PRE short-circuit only fires when PRE is the root. When a
<pre>is nested inside a non-PRE root (e.g., a message that embeds a code block), the walker recurses into it and the outerreplaceAll(/\n+/g, "\n")collapses the code block's blank/indent-preserving newlines. For the current call sites (CodeBlock passes the<pre>directly; MessageBlock content typically lacks nested<pre>) this isn't user‑visible today, but it's a latent foot‑gun if message rendering ever embeds rendered code blocks.Consider short‑circuiting nested PREs to
textContentas well, and emitting block separators around the result so surrounding newlines are preserved:♻️ Proposed adjustment
const walk = (node: Node): string => { if (node.nodeType === Node.TEXT_NODE) return node.nodeValue ?? ""; if (node.nodeType !== Node.ELEMENT_NODE) return ""; const el = node as HTMLElement; if (el.hasAttribute(COPY_IGNORE_ATTR)) return ""; if (el.tagName === "BR") return "\n"; + if (el.tagName === "PRE") return `\n${el.textContent ?? ""}\n`; const inner = Array.from(el.childNodes) .map((child) => walk(child)) .join(""); return BLOCK_TAGS.has(el.tagName) ? `\n${inner}\n` : inner; };Note: preserving internal newlines of a nested
<pre>also requires changing the final normalization to not collapse them (e.g., only trim, or only collapse 3+ newlines into 2). Worth deciding based on whether nested<pre>is actually expected in MessageBlock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared-ui/src/components/CopyButton.tsx` around lines 15 - 31, The walker in collectCopyText currently only short-circuits when the root is PRE, so nested <pre> elements lose verbatim newlines when the final replaceAll collapses runs of newlines; update the walker to treat nested PREs specially by returning the pre's full textContent wrapped with block separators (e.g., return `\n${el.textContent ?? ""}\n` when el.tagName === "PRE") and keep respecting COPY_IGNORE_ATTR and BR handling, and then change the final normalization from replaceAll(/\n+/g, "\n") to one that preserves internal pre spacing (for example only trim() or collapse 3+ newlines to two using replaceAll(/\n{3,}/g, "\n\n")). Ensure references: collectCopyText, COPY_IGNORE_ATTR, BLOCK_TAGS, and the walk inner function are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/shared-ui/src/components/CopyButton.tsx`:
- Around line 15-31: The walker in collectCopyText currently only short-circuits
when the root is PRE, so nested <pre> elements lose verbatim newlines when the
final replaceAll collapses runs of newlines; update the walker to treat nested
PREs specially by returning the pre's full textContent wrapped with block
separators (e.g., return `\n${el.textContent ?? ""}\n` when el.tagName ===
"PRE") and keep respecting COPY_IGNORE_ATTR and BR handling, and then change the
final normalization from replaceAll(/\n+/g, "\n") to one that preserves internal
pre spacing (for example only trim() or collapse 3+ newlines to two using
replaceAll(/\n{3,}/g, "\n\n")). Ensure references: collectCopyText,
COPY_IGNORE_ATTR, BLOCK_TAGS, and the walk inner function are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a630a56f-b577-48ec-a692-e4516128162e
📒 Files selected for processing (1)
packages/shared-ui/src/components/CopyButton.tsx
|
@claude ok, so we are using the DOM representation now. Would the alternative be to use the virtual dom and if so how to do that without creating duplicate representations of everything just for the copy button? |
|
Claude finished @heiwen's task in 3m 12s —— View job Virtual DOM as the copy source — would it work, and at what cost?Short answer: there isn't really a "virtual DOM" you can walk from outside React. React's fiber tree is internal; the only public surfaces are the JSX you pass in (children/props of components you author) and what React renders out (the DOM). So "walk the vDOM" practically means one of three concrete things — and only one of them genuinely avoids duplicate representations. Option A — walk React children of a wrapper componentPut the content inside a wrapper: <Copyable>
<ReasoningPanel ... />
<CollapsibleText ... />
<CollapsibleCode ... />
</Copyable>
Option B —
|
|
@claude I won't the CopyButton to be generic, so that the client doesn't have to provide their own mapping logic every single time. but I also like to avoid the keepMounted, since that can slow down the frontend significantly. how? |
|
Claude finished @heiwen's task in 3m 55s —— View job Generic CopyButton without
|
|
@claude there must be another way. my constraints are clear (i) keep using the collapsible component, we don't have access to it's API (ii) don;t use keepMounted (iii) a generic way to pass the content into the copybutton [whether that's dom or virtual dom or whatever I don't know). (iv) no expensive duplication so it doesn't blow up memory or the like. this should be possible, since the content is already there somewhere. What's that way? (don't implement anything) |
|
Claude finished @heiwen's task in 4m 1s —— View job The way: register the JSX, don't render itYou named the constraint correctly — "the content is already there somewhere." It is. We just need to look at where, per case, and stop conflating "in memory" with "in the DOM." Where the text actually lives, per component
So actually only one of these three is the problem: Where
|
| Constraint | Met? | Why |
|---|---|---|
(i) Keep using Collapsible, no API access needed |
✅ | We don't touch base-ui. We just don't pass keepMounted and add a sibling marker. |
(ii) No keepMounted |
✅ | Collapsible unmounts freely. The text source is the JSX, not the DOM. |
| (iii) Generic CopyButton API | ✅ | Call sites unchanged: <CopyButton value={contentRef} />. The participation lives inside the unmounting component, not at consumer sites. |
| (iv) No expensive duplication | ✅ | children JSX is held once by ExpandableContent's props. The registry stores a closure reference (function + JSX pointer, no copy). The text string is built once, on click. No second mount, no second DOM tree, no effects re-run. |
Why this is not a reinvention of keepMounted
keepMountedkeeps a React subtree mounted: components run, effects fire, refs are taken, DOM nodes exist for every child. For a Reasoning panel with rich content, that means measurement, animation, motion observers, etc., all live in parallel with the visible copy.- The registry holds a JSX object reference — a static description of the tree, the same
{ type, props, children }data the React reconciler would consume if it mounted. Nothing runs. There's no DOM. There's no second instance. It's the same JS object the parent already holds.
That's the whole point: React always has two representations of any subtree — the JSX you write (cheap, declarative) and the rendered fiber/DOM tree (expensive, live). keepMounted keeps the second one alive when you don't need it. The slot trick lets us read from the first one instead.
Caveats worth naming
- The JSX walker only sees text passed as
children. If a child component computes text from props (<Greeting name="Bob" />→ "Hello, Bob"), the walker sees<Greeting name="Bob"/>and gets nothing. For ourExpandableContentusage today,childrenis<p>{reasoning}</p>— pure text — so this is a non-issue. For future containers that wrap arbitrary components and need this, those components would either pass strings as children, or themselves participate viauseCopySource. Same one-time cost, isolated to authors. - Slot ordering is preserved because the placeholder
<span data-copy-slot>sits at the JSX position where the content would render, so the DOM walker encounters it in correct order. - Three call sites for
useCopySourcewould feel like API surface; one call site does not.
TL;DR
CollapsibleText and CollapsibleCode don't actually need keepMounted — drop it from the PR's mental model, they were never the problem. ExpandableContent is the one container whose DOM disappears, but its children JSX lives in memory anyway. Read from that via a Context + slot-placeholder, and you get the generic CopyButton API, no keepMounted, and zero expensive duplication.
· branch claude-issue-460-20260511-0724
1 participant
Summary
CopyButtonto accept a DOM subtree via itsvalueprop (nowstring | () => string | RefObject<HTMLElement>) and walk the tree itself, skipping any subtree marked withdata-copy-ignore.data-copy-ignore.getNodeTextfromCode.tsxand pointCopyButtonat the<pre>ref so there's one copy of the text in the DOM and the clipboard string is only built on click.textContentsemantics so collapsed / hidden text is still copied in full (noline-clamptruncation, Reasoning panel useskeepMounted).Fixes #460
Test plan
CopyButtonsites (Raw JSON, metadata rows, alias paths, Input) still copy expected string.Generated with Claude Code
Summary by CodeRabbit