Repository navigation
feat(explain): copy selected node details to clipboard (#1400) - #1424
NewtTheWolf merged 5 commits into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| <> | ||
| <Check size={12} className="text-accent-success" /> | ||
| <span className="text-accent-success text-[11px]"> | ||
| {t('editor.visualExplain.copied', 'Copied')} |
There was a problem hiding this comment.
WARNING: editor.visualExplain.copied is not defined in any locale file
The PR adds copyDetails, detailsCopied, copyDetailsFailed and nodeDetails to all 13 locales, but not copied. In en.json the editor.visualExplain block has no copied key, so non-English users will see the hard-coded English fallback Copied on the button. Add editor.visualExplain.copied to every locale (the PR description also claims locale coverage is complete).
| if (timerRef.current) { | ||
| clearTimeout(timerRef.current); | ||
| } | ||
| setCopied(true); |
There was a problem hiding this comment.
WARNING: False "Copied" feedback when no clipboard path is available
success starts as true. When onCopyDetails is not provided and navigator.clipboard?.writeText is missing (the else if at line 80 is false), nothing is copied but setCopied(true) still runs and the button shows a checkmark. Initialise success to false and set it to true only after a clipboard write actually succeeds (also set it to true when onCopyDetails returns anything other than false).
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Both earlier warnings stay resolved at current HEAD
Changes since the last review, in the explain-related files:
Files Reviewed (23 files in PR)
Previous Review Summaries (3 snapshots, latest commit af218f7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit af218f7)Status: No Issues Found | Recommendation: Merge Both previous warnings are resolved at commit
Files Reviewed (14 files in incremental diff; 21 total in PR)
Previous review (commit 33112bb)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file in incremental diff; 21 total in PR)
Fix these issues in Kilo Cloud Previous review (commit d8dfdd8)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (21 files)
Reviewed by claude-sonnet-5.5 · Input: 264 · Output: 3.3K · Cached: 773K Review guidance: REVIEW.md from base branch |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for sharing the formatter between the displayed details and clipboard text, and for routing the app copy action through platform.writeClipboard with error feedback. The 12 targeted web UI tests and typecheck pass.
Two follow-ups: avoid the synchronous setCopied call in useEffect, per .rules/react.md rule 2; also render Diagnostics once. getExplainNodeDetailSections now appends a Diagnostics section, while ExplainNodeDetails still renders ExplainDiagnosticList above all the returned sections.
These are follow-up nits, so I am approving this. I will leave the PR open for your manual checks before any merge.
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
I updated the branch against main and addressed both earlier nits: copy feedback is derived from the node whose copy succeeded, without a synchronous state reset in an effect, and diagnostics render once while remaining in the copied text.
The new component regressions pass, along with all 8,794 frontend tests and 220 explain-package tests. Lint and typecheck are green. Approved on f4ff540, subject to the current CI checks.
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Regenerated the embedded plugin guest runtime after the Explain component changes. The resulting Git blob matches the one CI expected, and two consecutive locked builds produced identical output. The 14 related frontend tests, nine Rust sandbox-related tests and explain-package typecheck pass. Approved on 49c9733.
Summary
Resolves #1400.
Adds a "Copy Details" action to the EXPLAIN node details panel, allowing users to copy the selected node's displayed fields and metrics as stable, structured plain text for investigation notes and sharing.
Key Changes
@tabularis/explain):formatExplainNodeDetailsandgetExplainNodeDetailSectionsinpackages/explain/src/nodeDetails.ts(exported from package index).packages/explain/src/components/ExplainNodeDetails.tsx):disabled={!node}).onCopyDetails), falling back to navigator clipboard if unprovided.packages/web-ui/src/components/explain/VisualExplainView.tsx):handleCopyNodeDetailsto the host application's platform clipboard adapter (platform.writeClipboard).detailsCopied) and error (copyDetailsFailed).copyDetails,detailsCopied,copyDetailsFailed,nodeDetails) to all 13 supported locales (de,en,es,fr,it,ja,ko,pt-BR,ru,tl,vi,zh,zh-TW).packages/explain/tests/nodeDetails.test.ts(estimated-only, analyzed, missing metrics, Unicode, custom translation, diagnostics, headers).packages/web-ui/tests/components/explain/VisualExplainView.test.tsxto verify disabled state with no node, successful clipboard copy with feedback, and clipboard rejection handling.Verification
pnpm typecheck&pnpm typecheck:explain(clean, 0 errors)vitest run packages/explain/(212/212 tests pass)vitest run packages/web-ui/tests/components/explain/(27/27 tests pass)eslint packages/explain packages/web-ui/src/components/explain(clean, 0 errors)node scripts/check-theme-tokens.mjs(OK)cargo check --no-default-featuresfromsrc-tauri(passed, exit 0)Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.