Skip to content

fix(ui): fall back to legacy copy in insecure browser contexts - #1174

Merged
backnotprop merged 2 commits into
mainfrom
fix/1173-clipboard-insecure-context
Aug 3, 2026
Merged

fix(ui): fall back to legacy copy in insecure browser contexts#1174
backnotprop merged 2 commits into
mainfrom
fix/1173-clipboard-insecure-context

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

Closes #1173

Bug: navigator.clipboard only exists in secure browser contexts. Remote-mode Plannotator serves plain HTTP on a non-localhost host, so navigator.clipboard is undefined there and every bare navigator.clipboard.writeText(...) call threw TypeError: Cannot read properties of undefined (reading 'writeText'). Reported from annotate mode on 0.25.1; it affected every copy button. Remote-mode HTTP is the main affected environment.

Fix: New copyTextToClipboard(text): Promise in packages/ui/utils/clipboard.ts. It tries the async Clipboard API (guarded against synchronous throws), falls back to the existing copy-event / execCommand path, returns whether the copy actually happened, and never throws. All 27 bare call sites across packages/ui, packages/editor, and packages/review-editor now go through it, preserving each site's UX (Copied states only on success, existing error toasts and console errors on failure, fire-and-forget sites stay fire-and-forget). GoalSetupSurface gains the fallback and keeps its error message for the all-strategies-failed case. copyTextPreservingFocus keeps its exported signature and behavior unchanged.

Tests: DOM-gated unit tests (packages/ui/utils/clipboard.test.ts, registered in .github/workflows/test.yml) cover: Clipboard API absent (falls back, returns the execCommand result), writeText rejecting, writeText throwing synchronously, writeText resolving (no fallback invoked), everything failing (resolves false, never throws), and the fallback copy event carrying the text payload. Typecheck, full bun test, and the CI DOM test set all pass.

navigator.clipboard only exists in secure contexts. Remote mode serves
plain HTTP on a non-localhost host, so every bare
navigator.clipboard.writeText call threw TypeError and copy buttons
silently broke.

Add copyTextToClipboard(text): Promise<boolean> to
packages/ui/utils/clipboard.ts: it tries the async Clipboard API
(guarded against synchronous throws), falls back to the existing
copy-event plus execCommand path, reports success as a boolean, and
never throws. copyTextWithFallback now returns whether the copy
happened and accepts an optional focusOwner; copyTextPreservingFocus
keeps its exported signature and behavior unchanged.

Route all bare call sites through the helper, preserving each site's
UX: Copied states only flip on success, error toasts and console
errors remain for the failure case, fire-and-forget sites stay
fire-and-forget. GoalSetupSurface gains the fallback and keeps its
error surface for the all-strategies-failed case.

Add DOM-gated unit tests for the helper and register them in CI.

Closes #1173
@backnotprop

Copy link
Copy Markdown
Owner Author

Adversarial review (at 45a60cba)

Verdict: core fix is correct and the load-bearing property holds, verified empirically: with navigator.clipboard undefined, the fallback runs in the same synchronous turn as the click handler, so the user-gesture window is preserved on the exact path issue #1173 hits. copyTextPreservingFocus is byte-identical to main (diffed), so the published @plannotator/ui surface is untouched. All 27 conversions were diffed against main; toasts, timeouts, and state ordering match. Typecheck clean; new tests skip cleanly without DOM and pass with it; full DOM CI set 160 pass / 0 fail.

Two findings should be fixed before merge; both are small.

1. The fallback textarea closes popout dialogs mid-copy (TablePopout.tsx:103,112, CodeFilePopout.tsx:461). The fallback appends a textarea to document.body and focuses it. PopoutDialog closes on focus-out unless the focus target matches ANNOTATION_SELECTORS, and a bare body-level textarea matches none of them. In remote mode over HTTP, clicking "Copy as markdown" in a table popout copies the text and then closes the popout underneath the click, and the "Copied!" state lands on a closed dialog. PR-introduced: these sites had no textarea fallback on main. Cleanest fix: tag the fallback textarea with a marker attribute and add it to ANNOTATION_SELECTORS. AnnotationToolbar is not affected (its button sits inside .annotation-toolbar).

2. Quick copy now shows "Copied" on failure (packages/editor/App.tsx:4728 via AnnotationPanel.tsx:249). onQuickCopy is typed Promise<void>, so the boolean is discarded and the panel unconditionally sets the copied state. On main a failure rejected and the checkmark never showed. Fix: widen the prop to Promise<boolean> (additive on the published seam) and gate the state.

Worth taking in the same pass:

3. The fallback can report success when nothing was copied (clipboard.ts:12-24, confirmed with a probe): the copy handler sets copied = true even when execCommand returns false, and when clipboardData is null the handler still flags success after preventDefault() suppressed the native copy. Inherited from main, but the boolean now drives every "Copied!" badge. One-line tighten: only set the flag when setData actually ran.

4. Pin the synchronous-fallback property in a test. None of the 7 tests fails if someone inserts an await before the fallback, which would silently kill the fix in real browsers while staying green. A 6-line test (call without awaiting, assert execCommand fired before return) covers it.

5. Nit: GoalSetupSurface now reports every failure as "Clipboard is unavailable in this browser", including permission-denied with the API present. Preserve the real error when one exists.

Noted, no action needed: share-link copy awaits a network round-trip before copying, so Safari's strict gesture window may still expire; not a regression (main threw a TypeError there) but #1173 is not fully closed for that one path. apps/skills/extra/plannotator-compound/assets/report-template.html has the same bug class in a generated artifact; follow-up material.

Also verified clean: no cross-listener hijack with Viewer's document copy listener (the fallback textarea is excluded by Viewer's own tag guard), zero bare navigator.clipboard left in packages/, clipboard reads unaffected (paste uses the native event and works in insecure contexts), no lockfile or artifact churn, tests excluded from the published package.

Review follow-ups for the insecure-context clipboard fallback:

1. Tag the fallback textarea with data-clipboard-fallback and whitelist
   it in PopoutDialog's ANNOTATION_SELECTORS so the transient focus
   shift during a fallback copy no longer closes popout dialogs
   (TablePopout copy buttons, CodeFilePopout copy contents).

2. Widen AnnotationPanel's onQuickCopy prop to Promise<void | boolean>.
   A false resolution now suppresses the Copied flash; void resolution
   stays success so existing hosts keep today's behavior. The editor
   quick-copy site returns the helper's boolean.

3. In copyTextWithFallback, only flag the copy-event path as success
   when clipboardData was present and setData actually ran, and only
   when execCommand also reported success. A null clipboardData no
   longer calls preventDefault, so the textarea retry still runs.

4. Add tests pinning that the fallback runs synchronously when
   navigator.clipboard is absent (execCommand fires before the call
   returns, keeping it inside the user-gesture window), that a copy
   event without clipboardData is not treated as success, and that the
   fallback textarea carries the PopoutDialog focus-out marker and is
   removed after the copy resolves.

5. GoalSetupSurface surfaces the real writeText rejection message when
   the Clipboard API exists but fails and the fallback also fails; the
   generic unavailable message is reserved for the API-absent case.
@backnotprop

Copy link
Copy Markdown
Owner Author

Delta re-review (at 9c54237a)

Verdict: merge as-is. All five fixes verified mechanically, not by reading.

  • Popout focus-out: the marker attribute is matched by both guard branches of PopoutDialog (target and relatedTarget run the same selector check), including the second focusout fired when the textarea is removed while focused. No legitimate close is suppressed: the only producer of the attribute appends and removes it within one synchronous function body, and a test asserts no marker element survives the copy. The ANNOTATION_SELECTORS export is purely additive.
  • onQuickCopy widening: proved additive with an isolated strict tsc check in both directions (host Promise<void> and Promise<boolean> both assign; narrowing is rejected, pinned with @ts-expect-error). PanelProps is not exported, so the breaking direction is unreachable for hosts. Void resolution keeps today's flash.
  • False-success paths: both original probes now resolve false, and the null-clipboardData case no longer suppresses the native copy. The && tighten cannot discard a real success: in the odd browser where the handler ran but execCommand reported false, the textarea retry performs its own copy, so the worst case is a false negative, strictly the safer direction.
  • Synchronicity test: mutation-tested. Inserting await Promise.resolve() before the fallback fails exactly that test and nothing else, so the property the fix rests on now has a real guard.
  • GoalSetupSurface: all four error paths traced; real rejection errors propagate again, generic message reserved for API-absent/total failure, gesture window preserved.

Typecheck (incl. strict-consumer) pass; clipboard tests 10/10 with DOM, 10 clean skips without; full CI DOM set 163/0.

One behavior delta to record: copyTextPreservingFocus is no longer byte-identical to main in one narrow branch. When a copy handler ran but execCommand reported false, main declared success and skipped the retry; it now takes the textarea retry (a redundant second copy of the same text, focus restoration verified intact). Benign and arguably a fix, noted here since the first commit advertised unchanged behavior.

Non-blocking notes for follow-up: the marker attribute literal lives in clipboard.ts and PopoutDialog.tsx independently (tied together only by the test, which is the right layering); GoalSetupSurface double-invokes writeText on the reject path and partially duplicates the helper's strategy ladder; the AnnotationPanel seam-contract test has no onQuickCopy coverage (pre-existing gap).

@backnotprop
backnotprop merged commit 463f6ed into main Aug 3, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clipboard copy actions fail when Plannotator runs in insecure browser contexts

1 participant