Repository navigation
feat(vscode): add copy content button to Documents viewer - #14634
Conversation
Adds a toolbar button and right-click context menu with Copy and Copy as Markdown actions for the Documents viewer panel content. Closes #14598
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The previously flagged issue (copy-confirmation timer cancelled before the clipboard write settled) is resolved in Files Reviewed (1 file)
Previous Review Summaries (6 snapshots, latest commit 17abdb9)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 17abdb9)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (6 files)
Previously raised findings are resolved: Fix these issues in Kilo Cloud Previous review (commit 7ee84c8)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (33 files)
Fix these issues in Kilo Cloud Previous review (commit 661fcbc)Status: No Issues Found | Recommendation: Merge The incremental commit Files Reviewed (1 file)
Previous review (commit 459d88a)Status: No Issues Found | Recommendation: Merge All four prior suggestions on Files Reviewed (1 changed file)
Previous review (commit 9fd3536)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (23 files)
Fix these issues in Kilo Cloud Previous review (commit 34ec938)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (23 files)
Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
- Fix Markdown fence language extraction to use the basename instead of splitting the full path, so files like LICENSE/Makefile or paths with dotted directories don't leak into the fence language. - Clear the copy-confirmation timer on cleanup and before re-copying. - Handle clipboard write rejection instead of always flashing success. - Hide the toolbar copy button for image documents, matching the existing content-area and context-menu image gating.
Copy as Markdown wrapped content in a fixed triple-backtick fence, which breaks if the copied content itself contains a run of three or more backticks (e.g. a nested code block). The fence now expands past the longest backtick run found in the content, per the CommonMark fenced code block rule.
kirillk
left a comment
kirillk
left a comment
There was a problem hiding this comment.
Review findings
-
High: this does not complete the linked issue. Issue #14598 requests document-tab actions for Copy Path, Copy Relative Path, Copy File Name, and Open in Editor. Tabs still receive only close actions in
DocumentPanel.tsx:331-355. This PR instead adds a context menu to the document body, so it should not close the issue as implemented. -
Medium: Copy Content is enabled while content is unavailable or stale. The toolbar only checks
!isImage()atDocumentPanel.tsx:299-309. Loading and errored documents pass that condition, whilecontent()converts missing content to an empty string. Opening a missing, binary, or oversized document leaves a working copy button that overwrites the clipboard with an empty string and reports success. During refresh, it can copy cached stale content. -
Medium: the custom menu regresses selection-based copying in the standalone Documents panel. The trigger at
DocumentPanel.tsx:370-406suppresses the native context menu, and Copy Content always copies the complete document. Previously, users could select one line and use the native right-click Copy action. This is specifically a standalone-panel regression; Agent Manager already suppresses native menus globally. -
Medium: Copy as Markdown can crash on an accepted large document.
fenceForspreads every backtick match intoMath.maxatDocumentPanel.tsx:64-67, while the document reader accepts files up to 2 MB. A valid file containing"` ".repeat(1_000_000)reproducesRangeError: Maximum call stack size exceeded. A loop or reduction avoids the argument limit. -
Low: Copy as Markdown adds an extra blank line when the source already ends in a newline.
DocumentPanel.tsx:76always inserts another newline before the closing fence. For example, content ending inconst x = 1\ngains an empty line inside the fenced block. -
Low: copy confirmation belongs to the panel rather than the document. Copy document A and switch to B within 1.5 seconds; B shows the check icon even though it was not copied. The state at
DocumentPanel.tsx:135-138is not reset by the tab-change effect.
Context-menu convention
Using @kilocode/kilo-ui/context-menu is consistent with existing Agent Manager UI. It is not the native VS Code webview pattern: VS Code recommends contributes.menus["webview/context"] with data-vscode-context, which integrates with the editor menu and preserves default selection-based Copy/Paste items. The custom menu is reasonable for Agent Manager, but the native mechanism is a better fit for the standalone DocumentViewerProvider and avoids the selection-copy regression.
No tests were added for the new serialization, loading/error gating, tab switching, or either host menu behavior.
kirillk's review found this PR did not implement the issue as scoped and introduced several regressions/bugs with the added document-body context menu and 'Copy as Markdown' feature. Address each point: - Remove the document-body right-click menu and 'Copy as Markdown'. It suppressed the native context menu in the standalone Documents webview, regressing selection-based Copy, and its fence-wrapping logic had unrelated bugs (stack overflow on huge inputs, spurious blank lines). This was scope creep beyond the linked issue. - Add the tab context menu actions the issue actually requested: Copy Path, Copy Relative Path, Copy File Name, and Open in Editor. Copy Path needs the workspace-absolute path, which the webview does not know, so it round-trips through a new 'document.copyPath' message (standalone DocumentViewerProvider) and a new 'agentManager.copyFilePath' message (Agent Manager), both resolving the path host-side and writing to the clipboard. - Gate the toolbar Copy Content button on loading/error state (not just image kind), so it cannot copy an empty or stale string while flashing success. - Reset the copy-confirmation state when the active tab changes, so switching documents right after a copy no longer shows a stale checkmark on the wrong tab. - Add unit tests for the new copyPath/openFile message plumbing.
|
@kirillk Thanks for the thorough review. I've addressed the points you outlined above and would appreciate another cycle-through 🙂 |
Select All previously selected the entire panel DOM (header title, tab labels, action icons), not just the file's text -- the core complaint in issue #14598. Intercept Cmd/Ctrl+A while focus is inside the panel and outside any editable field, and select only the rendered document content instead, matching what Copy Content copies.
…ocode into feat/documents-copy-content
|
Re-review findings at
The previous scope and context-menu findings are otherwise resolved: all requested tab actions are present, Copy as Markdown and the document-body custom menu were removed, standalone selected-text Copy is restored, loading/error gating is fixed, and immediate confirmation leakage is reset on tab changes. Validation passed for typecheck, lint, knip, whitespace checks, and 107 focused tests. All hosted PR checks are green. |
|
@brunoagatao it seems 1-3 are worth addressing |
- Copy Path/Relative Path/File Name and the tab label use the loaded document's canonical relative path, so absolute inputs work (and Windows separators no longer leak into names). - Ignore late clipboard completions after switching tabs. - Log clipboard failures instead of swallowing or dropping them.
- Track the copied file path instead of a boolean, removing the tab-switch reset and stale-completion guard. - Select-all: ignore stale (detached) content, Shift/Alt variants, and editable targets inside shadow roots. - Evaluate the tab menu's leading items once.
|
@kirillk Addressed 🙂 Thanks again for the feedback! |
What
Adds a copy affordance to the Kilo Documents viewer panel:
Why
Closes #14598. Users viewing a file in the Documents panel had no quick way to copy its content.
Notes
navigator.clipboard.writeTextdirectly in the sharedDocumentPanelcomponent, since it renders in both the standalone Documents webview and the Agent Manager inline host — this avoids duplicating a host-side clipboard handler for each.agentManager.documents.copy/ `agentManager.documents.copyAsMarkdown" strings to all locale files (existing i18n split test requires every agent-manager key to be present in every locale).