fix(ui): make copy-to-clipboard buttons work in non-secure contexts - #3531
joshdutcher wants to merge 2 commits into
Conversation
Greptile SummaryConsolidates 15 scattered Confidence Score: 5/5Safe to merge — all remaining findings are P2 improvement suggestions with no regressions to existing behaviour. The change is purely additive in the non-secure-context path; secure-context behaviour is byte-for-byte identical to before. The one P2 flag (native API rejection not cascading to execCommand) is intentional and documented in the test suite. 346/346 tests pass, TypeScript is clean, and the fallback logic was already shipping inline across two components. ui/src/lib/clipboard.ts — the native-API rejection fallback question is worth a quick decision before the utility is used by future callers. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/lib/clipboard.ts
Line: 19-28
Comment:
**Native API rejection doesn't cascade to execCommand fallback**
If `navigator.clipboard.writeText` is present but rejects (e.g. the user clicked "Deny" on the browser's clipboard-permission prompt), the function rejects immediately without attempting the `execCommand` path. The test `"propagates rejection from the native API"` explicitly encodes this — so it's intentional — but it means the utility can fail silently in a secure context even when `execCommand` would have worked. Consider catching and retrying via the fallback, or at least documenting the intentional behaviour in the JSDoc above:
```typescript
try {
await navigator.clipboard.writeText(text);
return;
} catch {
// fall through to execCommand fallback
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(ui): make all copy-to-clipboard butt..." | Re-trigger Greptile |
| export async function copyTextToClipboard(text: string): Promise<void> { | ||
| if ( | ||
| typeof navigator !== "undefined" && | ||
| navigator.clipboard && | ||
| typeof navigator.clipboard.writeText === "function" && | ||
| (typeof window === "undefined" || window.isSecureContext) | ||
| ) { | ||
| await navigator.clipboard.writeText(text); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Native API rejection doesn't cascade to execCommand fallback
If navigator.clipboard.writeText is present but rejects (e.g. the user clicked "Deny" on the browser's clipboard-permission prompt), the function rejects immediately without attempting the execCommand path. The test "propagates rejection from the native API" explicitly encodes this — so it's intentional — but it means the utility can fail silently in a secure context even when execCommand would have worked. Consider catching and retrying via the fallback, or at least documenting the intentional behaviour in the JSDoc above:
try {
await navigator.clipboard.writeText(text);
return;
} catch {
// fall through to execCommand fallback
}Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/lib/clipboard.ts
Line: 19-28
Comment:
**Native API rejection doesn't cascade to execCommand fallback**
If `navigator.clipboard.writeText` is present but rejects (e.g. the user clicked "Deny" on the browser's clipboard-permission prompt), the function rejects immediately without attempting the `execCommand` path. The test `"propagates rejection from the native API"` explicitly encodes this — so it's intentional — but it means the utility can fail silently in a secure context even when `execCommand` would have worked. Consider catching and retrying via the fallback, or at least documenting the intentional behaviour in the JSDoc above:
```typescript
try {
await navigator.clipboard.writeText(text);
return;
} catch {
// fall through to execCommand fallback
}
```
How can I resolve this? If you propose a fix, please make it concise.The Clipboard API (`navigator.clipboard.writeText`) is only available in
secure contexts — HTTPS or HTTP loopback (`localhost` / `127.0.0.1`).
Self-hosted Paperclip deployments reached over plain HTTP on a LAN
hostname, or over a private-network hostname behind Tailscale, Wireguard,
a VPN, or an SSH tunnel, all count as non-secure from the browser's
point of view — `navigator.clipboard` is `undefined` — and 14 call sites
across the UI were invoking it directly with no fallback.
On the sites wrapped in `try/catch`, the synchronous `TypeError` was
swallowed silently. On the sites using bare `.then()`, the throw escaped
into React's internal handler before the `.then` callback could run. In
both cases the state update that would animate the icon
(`setCopied(true)` / `setStatus("copied")`) never executed, so the user
saw no feedback at all: click, nothing happens, clipboard unchanged,
icon static, no toast — indistinguishable from a disabled button. This
broke every copy icon across the app for self-hosted users not on
HTTPS: Copy Agent ID, agent token copy, invite snippet copy, workspace
path copy, issue identifier and folder copy, document body copy,
routine secret copy, worktree name copy, code-block / message /
context-menu copies in chat transcripts, and the full-issue markdown
copy.
Two components already had their own inlined execCommand fallback —
`CopyText` (via paperclipai#2472 and its iteration commits 5583307, b64607c,
944a724) and the `copyTextWithFallback` helper inside `CommentThread`
(via 1e4ccb2 "Improve mobile comment copy button feedback"). This
consolidates both into a single shared `copyTextToClipboard` utility at
`ui/src/lib/clipboard.ts`, migrates all 14 unmigrated call sites, and
hardens the fallback vs. the previous inlined versions:
- Explicit `focus({ preventScroll: true })` before `.select()`, which
some browsers require before they'll honor a programmatic selection.
- `setSelectionRange(0, length)` as a secondary selection path.
- On-screen-but-invisible positioning (opacity: 0, zIndex: -1, 1×1px)
instead of `left: -9999px`, since some engines refuse to include
off-screen elements in the document selection.
- Restores focus to the previously-focused element after copying, so a
user mid-typing isn't bumped out of their textarea.
No changes to secure-context behavior: when `navigator.clipboard` is
available the utility calls straight through. All 346 existing UI tests
pass; the new `ui/src/lib/clipboard.test.ts` adds 9 tests covering the
native-API path, fallback mounting/selecting/cleanup, execCommand
failure propagation, and focus restoration.
Fixes paperclipai#3529.
cafd262 to
514b6dd
Compare
Addresses Greptile P2 note on the initial commit: if `navigator.clipboard.writeText` is present but rejects (transient NotAllowedError from focus shifts, Safari timing quirks, etc.), fall through to the execCommand fallback instead of surfacing the rejection. Both paths use the same user-gesture gate, so if the fallback succeeds the user gets their copy through. Updated JSDoc to document the cascade. Replaced the "propagates rejection from the native API" test with two tests covering the new behavior: cascade on native rejection, and reject only when both paths fail.
|
Good call — applied in 8abefe7. The native-API call is now wrapped in try/catch and falls through to |
…#1832) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The board UI shows the workspace attached to an issue in `ui/src/components/IssueWorkspaceCard.tsx` > - That card renders values such as the branch name and the workspace path through a small `CopyableInline` component, each with an icon-only copy button > - The button has a `title` attribute only. Screen readers do not announce `title` reliably. A screen reader user hears no useful name for the button, because the button contains an icon and no text > - The button also starts a 1.5 second `setTimeout` to reset its "copied" state. Nothing clears that timer. If the card unmounts first, the callback sets state on an unmounted component > - This pull request adds a dynamic `aria-label` to the button and clears the timer in a `useEffect` cleanup > - The benefit is a copy control that assistive technology can announce, and no stray timer after the card unmounts ## Linked Issues or Issue Description No existing GitHub issue covers this. The problem is described below with the fields from [`bug_report.yml`](.github/ISSUE_TEMPLATE/bug_report.yml). **What happened?** Open an issue that has a workspace attached. Tab to the copy button next to the branch or the workspace path in the workspace card. The screen reader announces an unlabeled button, because the button holds only a lucide `Copy` icon and a `title` attribute. Separately, copy a value and navigate away within 1.5 seconds. The pending `setTimeout` then calls `setCopied(false)` on an unmounted component. **Expected behavior** The copy button has an accessible name that says what it copies, and the name changes to confirm the copy. The reset timer is cleared when the component unmounts. **Steps to reproduce** 1. Run the app locally with `pnpm dev`. 2. Open an issue that has a workspace attached, so `IssueWorkspaceCard` renders. 3. Turn on a screen reader (VoiceOver, NVDA). 4. Tab to the copy button next to the workspace path or the branch name. The button has no useful accessible name. 5. Click the copy button, then navigate away from the issue in under 1.5 seconds. The reset timer is still pending. **Paperclip version or commit** Reproducible on `master` at this pull request's base commit. **Deployment mode** Local dev (pnpm dev). Related pull request, not a duplicate: #3531 makes copy-to-clipboard buttons work in non-secure contexts. That pull request changes the clipboard write path. This one changes the button label and the timer cleanup, so the two do not overlap. ## What Changed - Added an `aria-label` to the `CopyableInline` copy button in `ui/src/components/IssueWorkspaceCard.tsx`. The label reads `Copy <label>` (for example "Copy branch"), falls back to `Copy value` when the component gets no `label` prop, and changes to `Copied to clipboard` after a copy. - Added a `useEffect` cleanup that calls `clearTimeout(timerRef.current)` on unmount, so the 1.5 second reset timer cannot fire after the component unmounts. ## Verification - CI is green on this pull request. - Static check: `pnpm -r typecheck`. - Test suite: `pnpm test`. - Manual, screen reader: open an issue with a workspace, tab to the copy button next to the path or the branch, and confirm the announcement is "Copy path" or "Copy branch". Activate the button and confirm the announcement changes to "Copied to clipboard". - Manual, timer: click the copy button and navigate away from the issue immediately. Confirm the console shows no unmounted-component state update. ## Risks Low risk. The change adds one ARIA attribute and one unmount cleanup in a single presentational component. No behavior changes for mouse users, no API or schema change. `clearTimeout(undefined)` is a no-op, so the cleanup is safe when the user never copied. ## Model Used - Anthropic Claude Opus, model ID `claude-opus-4-6`, 200K context window, extended thinking enabled, with tool use for file edits. - Recorded by a maintainer while bringing this description up to the current template. The original description predates the Model Used requirement, so the author did not state a model. Author: please correct this line if the model was different. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Notes on the checklist: no test or documentation change applies to a two-line ARIA and cleanup fix in one component. The Greptile box stays unchecked until the current review round closes.
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Operators often open self-hosted Paperclip over plain HTTP on a LAN or private network. > - Browser Clipboard API writes are not reliable in that insecure context. > - Paperclip already has one shared helper with a legacy copy fallback, but many current copy actions bypass it. > - This pull request routes every core UI copy action and the first-party workspace-diff plugin through the shared helper. > - The benefit is consistent copy behavior on HTTPS, localhost, and plain-HTTP private deployments. ## Linked Issues or Issue Description Refs #3529. This change supersedes the stale prior attempt in #3531. Current master has more copy surfaces and a first-party plugin UI bridge that the prior branch does not cover. ## What Changed - Replaced direct Clipboard API writes and duplicate fallback implementations across the current core UI with `copyTextToClipboard`. - Added an HTTP-safe clipboard function to the plugin UI SDK and wired the host bridge to the same implementation. - Migrated the first-party workspace-diff plugin to the plugin SDK clipboard function. - Added unit coverage for native rejection fallback and plugin host delegation. - Added a source-level regression test that rejects new direct clipboard writes outside the shared implementation. - Documented the plugin UI clipboard function. ## Verification - `NODE_ENV=test pnpm exec vitest run ...` for 14 affected suites: 164 tests passed. - `pnpm exec vitest run tests/ui-clipboard.test.ts` in `packages/plugins/sdk`: 1 test passed. - `NODE_ENV=test pnpm -r typecheck`: passed for 31 workspace projects. - `NODE_ENV=test pnpm test:run`: passed. - `NODE_ENV=production pnpm build`: passed. - `pnpm check:token-gates`: passed with all gates clean. ## Risks Low risk. Secure contexts still use the modern Clipboard API. Plain HTTP and rejected modern writes use the existing `execCommand("copy")` fallback. That API is deprecated, but it is the compatibility path required for insecure contexts. The change has no schema, API, or visual design effect. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI Codex, `gpt-5.6-sol`. The runtime did not expose a context-window size. Reasoning, tool use, repository editing, test execution, and GitHub CLI access were enabled. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
…paperclipai#1832) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The board UI shows the workspace attached to an issue in `ui/src/components/IssueWorkspaceCard.tsx` > - That card renders values such as the branch name and the workspace path through a small `CopyableInline` component, each with an icon-only copy button > - The button has a `title` attribute only. Screen readers do not announce `title` reliably. A screen reader user hears no useful name for the button, because the button contains an icon and no text > - The button also starts a 1.5 second `setTimeout` to reset its "copied" state. Nothing clears that timer. If the card unmounts first, the callback sets state on an unmounted component > - This pull request adds a dynamic `aria-label` to the button and clears the timer in a `useEffect` cleanup > - The benefit is a copy control that assistive technology can announce, and no stray timer after the card unmounts ## Linked Issues or Issue Description No existing GitHub issue covers this. The problem is described below with the fields from [`bug_report.yml`](.github/ISSUE_TEMPLATE/bug_report.yml). **What happened?** Open an issue that has a workspace attached. Tab to the copy button next to the branch or the workspace path in the workspace card. The screen reader announces an unlabeled button, because the button holds only a lucide `Copy` icon and a `title` attribute. Separately, copy a value and navigate away within 1.5 seconds. The pending `setTimeout` then calls `setCopied(false)` on an unmounted component. **Expected behavior** The copy button has an accessible name that says what it copies, and the name changes to confirm the copy. The reset timer is cleared when the component unmounts. **Steps to reproduce** 1. Run the app locally with `pnpm dev`. 2. Open an issue that has a workspace attached, so `IssueWorkspaceCard` renders. 3. Turn on a screen reader (VoiceOver, NVDA). 4. Tab to the copy button next to the workspace path or the branch name. The button has no useful accessible name. 5. Click the copy button, then navigate away from the issue in under 1.5 seconds. The reset timer is still pending. **Paperclip version or commit** Reproducible on `master` at this pull request's base commit. **Deployment mode** Local dev (pnpm dev). Related pull request, not a duplicate: paperclipai#3531 makes copy-to-clipboard buttons work in non-secure contexts. That pull request changes the clipboard write path. This one changes the button label and the timer cleanup, so the two do not overlap. ## What Changed - Added an `aria-label` to the `CopyableInline` copy button in `ui/src/components/IssueWorkspaceCard.tsx`. The label reads `Copy <label>` (for example "Copy branch"), falls back to `Copy value` when the component gets no `label` prop, and changes to `Copied to clipboard` after a copy. - Added a `useEffect` cleanup that calls `clearTimeout(timerRef.current)` on unmount, so the 1.5 second reset timer cannot fire after the component unmounts. ## Verification - CI is green on this pull request. - Static check: `pnpm -r typecheck`. - Test suite: `pnpm test`. - Manual, screen reader: open an issue with a workspace, tab to the copy button next to the path or the branch, and confirm the announcement is "Copy path" or "Copy branch". Activate the button and confirm the announcement changes to "Copied to clipboard". - Manual, timer: click the copy button and navigate away from the issue immediately. Confirm the console shows no unmounted-component state update. ## Risks Low risk. The change adds one ARIA attribute and one unmount cleanup in a single presentational component. No behavior changes for mouse users, no API or schema change. `clearTimeout(undefined)` is a no-op, so the cleanup is safe when the user never copied. ## Model Used - Anthropic Claude Opus, model ID `claude-opus-4-6`, 200K context window, extended thinking enabled, with tool use for file edits. - Recorded by a maintainer while bringing this description up to the current template. The original description predates the Model Used requirement, so the author did not state a model. Author: please correct this line if the model was different. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Notes on the checklist: no test or documentation change applies to a two-line ARIA and cleanup fix in one component. The Greptile box stays unchecked until the current review round closes. (cherry picked from commit 54e2031)
Fixes #3529.
Thinking Path
What Changed
ui/src/lib/clipboard.tsexportingcopyTextToClipboard(text: string): Promise<void>. Usesnavigator.clipboard.writeTextwhen bothnavigator.clipboardandwindow.isSecureContextare truthy; otherwise falls back to a transient on-screen-but-invisible textarea +document.execCommand("copy"). Rejects on failure so callers can surface a toast if they want to.focus({ preventScroll: true })before.select()(some browsers won't honor programmatic selection without focus first),setSelectionRange(0, length)as a secondary selection path, on-screen-but-opacity: 0+zIndex: -1+pointerEvents: nonepositioning instead ofleft: -9999px(off-screen elements are silently excluded from the document selection in some engines), and restore-focus to the previously-focused element so users mid-typing aren't bumped out of their inputs.ui/src/lib/clipboard.test.ts— 9 tests covering the native-API happy path, the fallback mounting/selecting/removing the textarea,execCommandfailure propagation,execCommand-throws cleanup, focus restoration, and the "secure context but no navigator.clipboard" degenerate case (jsdom-style).CopyText.tsxandCommentThread.tsxnow import the shared utility and drop their inlined fallbacks. Their existing tests continue to pass unchanged.navigator.clipboard.writeText(...)call sites acrossui/src/components/andui/src/pages/migrated tocopyTextToClipboard(...). No behavioral changes beyond "now actually copies in non-secure contexts." Toast and copied-state feedback paths are untouched.Verification
Automated:
cd ui && pnpm tsc --noEmit→ clean (exit 0).cd ui && pnpm vitest run→ 346/346 tests pass, 65/65 files, including the new 9 clipboard tests and the pre-existingCommentThread.test.tsx(which has its ownexecCommandmocking).Manual (on a self-hosted Paperclip reached over plain HTTP from another machine on the LAN):
ui-dist/index.htmlwith an equivalent inline polyfill and restarting the service: click "Copy Agent ID" → paste elsewhere confirms the UUID is on the clipboard. Spot-checked several other previously-broken buttons (workspace path copy, invite snippet copy, agent token copy on rotation) — all now land text on the clipboard.http://localhost:3100(wherenavigator.clipboardis defined): utility short-circuits tonavigator.clipboard.writeText, no textarea is ever mounted. Thedoes not mount any DOM nodesunit test asserts this invariant so it can't silently regress.CopyText, and "Copy as markdown" on comments which usesCopyMarkdownButton) continue to work identically after the refactor — their inlined fallbacks were replaced with calls to the shared utility, not behavior-changed.Risks
Low. The fallback path is strictly opt-in-on-failure — it only runs when
navigator.clipboardis missing orwindow.isSecureContextis false, so secure-context (HTTPS/localhost) behavior is identical to before this PR. The execCommand path is the same technique already shipping inline inCopyTextandCommentThread; we're not introducing a new clipboard mechanism, just centralizing and hardening one.execCommand("copy")is deprecated but universally implemented (MDN flags it as deprecated for new code, not broken for existing calls). No schema, API, or public-contract changes.Focus restoration is the one new behavior beyond the existing fallbacks — if a user had a textarea/input focused and clicked a copy button, focus now returns there after the copy (previously the off-screen textarea would steal focus). That's an ergonomic improvement rather than a regression risk, but worth flagging.
Model Used
Claude,
claude-opus-4-6(1M context window), with extended tool use for file reads, writes, and running vitest/tsc. No reasoning mode beyond the default.Checklist