fix-forward #2968 (tsk-xi3mcb): the isSecureContext conjunct makes copyText refuse a working clipboard (red), and the execCommand fallback can reject instead of returning false - #2977
Conversation
Copy buttons called navigator.clipboard.writeText directly, which throws
TypeError on plain-HTTP LAN origins where the API is unavailable. Some
call sites swallowed the error silently, so the button appeared to work
but copied nothing.
Created desktop/src/lib/clipboard.ts with a secure-context gate and
document.execCommand fallback (mirroring the existing InstallHelperPanel
pattern). Replaced all 20 navigator.clipboard occurrences across 14
source files to use the shared helper. Failed copies now surface an
error to the user instead of being swallowed by bare .catch() blocks.
Tests added:
- CodeBlock: copies in non-secure context (fallback path)
- CodeBlock: surfaces copy-failed error to the user
- ClusterApp: does not report success when clipboard is unavailable
RED-FIRST proof:
```
FAIL src/components/__tests__/CodeBlock.test.tsx > CodeBlock > copies in a non-secure context
AssertionError: expected \"vi.fn()\" to be called with arguments: [ \"copy\" ]
Number of calls: 0
FAIL src/components/__tests__/CodeBlock.test.tsx > CodeBlock > a failed copy surfaces an error to the user
TestingLibraryElementError: Unable to find an element with the text: /copy failed/i.
FAIL src/apps/__tests__/ClusterApp.test.tsx > ClusterApp clipboard in non-secure context > copy button does not report success when nothing was copied
expected document not to contain element, found <button ...>Copied</button> instead
Test Files 2 failed (2)
Tests 3 failed | 6 passed (9)
```
GREEN:
```
Test Files 2 passed (2)
Tests 9 passed (9)
```
Defect 1: the guard is stricter than the capability - navigator.clipboard is only exposed by the browser in a secure context, so the window.isSecureContext conjunct buys nothing and only adds a way to refuse a clipboard that demonstrably works - Drop && window.isSecureContext, rely on the optional-chained call plus the existing catch Defect 2: stale duplicate test file - desktop/src/components/CodeBlock.test.tsx was left untested by PR #2968, which only updated the __tests__/ sibling - Fold any unique assertions into __tests__/CodeBlock.test.tsx and delete the older file Defect 3: fallbackCopy can reject - fallbackCopy wraps document.execCommand("copy") in try/finally with NO catch, so a throwing execCommand propagates out of copyText as a rejected promise - Make it try/catch returning false, so copyText NEVER rejects - While in there: fallbackCopy calls textarea.focus() and never restores focus, so a keyboard user loses their place after every copy; save document.activeElement and restore it after removing the textarea ```text FAIL src/lib/clipboard.test.ts > copyText > uses navigator.clipboard.writeText when available and returns true, even when isSecureContext is false FAIL src/lib/clipboard.test.ts > copyText > returns false when execCommand throws, without rejecting Error: copy failed src/lib/clipboard.test.ts:52:15 50| Object.defineProperty(document, "execCommand", { 51| value: () => { 52| throw new Error("copy failed"); 53| }, ... Test Files 1 failed (1) Tests 2 failed | 1 passed (3) ``` ```text Test Files 2 passed (2) Tests 11 passed (11) ``` Carry changelog.d/tsk-xi3mcb-clipboard-helper.md from BASE; do not duplicate.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 21 seconds. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (20)
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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| textarea.focus(); | ||
| textarea.select(); | ||
| let ok = false; | ||
| const previouslyFocused = document.activeElement; |
There was a problem hiding this comment.
CRITICAL: previouslyFocused is captured after textarea.focus(), so focus restoration targets the detached textarea instead of the user's original focus.
| const previouslyFocused = document.activeElement; | |
| const previouslyFocused = document.activeElement; | |
| textarea.focus(); | |
| textarea.select(); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try { | ||
| await navigator.clipboard.writeText(code); | ||
| const ok = await copyText(code); | ||
| if (ok) { |
There was a problem hiding this comment.
WARNING: copyError is never cleared when a copy succeeds. If a previous copy failed, the "Copy failed" state lingers until its timeout fires even after the user successfully copies again.
| if (ok) { | |
| if (ok) { | |
| setCopied(true); | |
| setCopyError(false); | |
| setTimeout(() => setCopied(false), 1500); | |
| } else { |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 1500); | ||
| const ok = await copyText(lines.join("\n")); | ||
| if (ok) { |
There was a problem hiding this comment.
WARNING: copyError is never cleared when a copy succeeds. If a previous copy failed, the "Copy failed" state lingers until its timeout fires even after the user successfully copies again.
| if (ok) { | |
| if (ok) { | |
| setCopied(true); | |
| setCopyError(false); | |
| setTimeout(() => setCopied(false), 1500); | |
| } else { |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash:free · Input: 0 · Output: 0 · Cached: 0 |
|
The three defects are correctly diagnosed and correctly fixed — dropping the 1. CI is red on a test this PR's own fix contradicts
This is not a flake and not unrelated — it is the direct consequence of Defect 1. That test sets Object.defineProperty(window, "isSecureContext", { value: false, … });
writeText = vi.fn().mockResolvedValue(undefined); // a WORKING clipboard
Object.defineProperty(navigator, "clipboard", { value: { writeText }, … });
The fix belongs in this PR, since this PR is what makes it fail. Make the stub the real shape of Object.defineProperty(navigator, "clipboard", { value: undefined, configurable: true });Then the test exercises what it is named for: no clipboard → Please keep the test rather than deleting it — with that one-line change it is the red-proof for 2.
|
fix-forward #2977 (tsk-4smbaw): the ClusterApp red-proof stubs a WORKING clipboard in a non-secure context, so dropping the isSecureContext conjunct turns it red; and commit-message.txt is committed at the repo root
1 participant
CARD TITLE (intent, not commit subject): fix-forward #2968 (tsk-xi3mcb): the isSecureContext conjunct makes copyText refuse a working clipboard (red), and the execCommand fallback can reject instead of returning false
Autonomous build of board card tsk-4smbaw.
REVISION: built on
exec/tsk-xi3mcb(cut ate43bddc1a4fe5a2569f5eddee77887c87476b923), not ondev. That branch'scommits are ancestors of this one. Verified by
git merge-base --is-ancestorbefore the PR was opened.
Defect 1: the guard is stricter than the capability
Defect 2: stale duplicate test file
Defect 3: fallbackCopy can reject
Carry changelog.d/tsk-xi3mcb-clipboard-helper.md from BASE; do not duplicate.
Files:
desktop/src/apps/chat/MessageList.tsx | 7 +--
desktop/src/components/CodeBlock.test.tsx | 27 ----------
desktop/src/components/CodeBlock.tsx | 18 +++++--
desktop/src/components/TaosAssistantPanel.tsx | 7 ++-
.../src/components/tests/CodeBlock.test.tsx | 50 ++++++++++++++++++
desktop/src/lib/clipboard.test.ts | 61 ++++++++++++++++++++++
desktop/src/lib/clipboard.ts | 39 ++++++++++++++
20 files changed, 319 insertions(+), 78 deletions(-)