fix: use native Clipboard API in useClipboard, fall back to copy-to-clipboard - #1287
Conversation
…lipboard copy-to-clipboard does a synchronous document.body.appendChild/removeChild of a hidden node on every copy. On /buy's payment-details view (11 copy buttons across payment-info-buy.tsx) that raw DOM write is a plausible collision window with anything else touching the page at the same instant (browser extensions, translators), and is the only unmanaged DOM write reachable from that screen. navigator.clipboard.writeText needs no DOM node at all; fall back to the old path only where it's unavailable.
The existing suite only ever exercised the copy-to-clipboard fallback since jsdom has no navigator.clipboard. Add cases for the primary path and for falling back when writeText rejects.
The 500ms reset used to fire unconditionally, so on the writeText rejection path the checkmark could revert before the copy-to-clipboard fallback (and its possible window.prompt()) even ran.
Neither async-path test checked isCopying, so it stayed green against the pre-fix code that reset it before the write actually settled. Verified by mutation: these two fail on the unconditional-setTimeout version and pass on the current .finally()-sequenced one.
|
4 review passes before this was ready — each fix commit was re-reviewed fresh:
All green since. |
|
@mara-steiner please check |
|
All three addressed:
Locally on the new head: lint and prettier clean, 74 suites / 801 tests green, hook coverage still 100% on all four metrics. Thanks for the premise check — good to have the |
|
2 review passes on the review-response commits before settling:
CI is green on 9d8a2ed. |
copy() in the navigator.clipboard-unavailable branch could throw (copy-to-clipboard's own removeChild is unguarded) and skip resetIsCopying, leaving isCopying stuck true. The async branch was already protected via .finally(); mirror that for the sync branch. Also restores the original navigator.clipboard property descriptor in tests instead of deleting it, so a future jsdom that ships a Clipboard implementation isn't silently stripped between tests.
|
@mara-steiner addressed at dbea1b2: the sync fallback branch now runs |
|
@mara-steiner please re-review! |
|
@mara-steiner please check |
Summary
useClipboard(src/hooks/clipboard.hook.ts) copies via thecopy-to-clipboardpackage, which on every call does a synchronous, non-React DOM write:document.body.appendChilda hidden node →execCommand('copy')→document.body.removeChildit, entirely self-contained (it never touches a node React itself is tracking).On
/buy, the payment-details view wires 11 separate copy buttons to this hook (payment-info-buy.tsx), each followed about a second later by a React-driven re-render (the button icon swapping back). Since the client-error boundary started reporting (#1227), telemetry has recorded a recurring crash on that route:The stack is entirely inside React's own DOM-commit internals — the signature of something outside React's control having removed or moved a node React still expected to own.
copy-to-clipboard's write doesn't collide with that node directly; the theory is that it opens a window, once per click, during which something else touching the page (a browser extension is the leading candidate) could interfere with nearby DOM structure before React's own follow-up commit runs. It's the only unmanaged DOM mutation reachable from/buy— everything else in that render tree, including the QR code, goes through React.This PR switches
useClipboardtonavigator.clipboard.writeText, which needs no DOM node at all, and falls back to the existingcopy-to-clipboardpath only when the Clipboard API is unavailable or the write is rejected.What this is and isn't
/buyfor the primary path. It has not been proven to be the actual cause of the crash — no direct interference signal was captured, this is the only concrete lever available given the rest of the render tree is plain React. Not strictly true for the fallback: whenwriteTextis unavailable or rejects, the fallback still routes throughcopy-to-clipboardand carries the sameremoveChild-based DOM write this PR otherwise removes.navigator.clipboardunavailable) now runscopy(text)insidetry/finally, soisCopyingstill resets even if that call throws — mirroring the guarantee the async branch already has via.finally(resetIsCopying).useClipboard's public interface (copy,isCopying) is unchanged, so the hook's other callers (payment-info-sell/-content, sell/swap-completion, connect-cli, user-data-panel, tfa, support-dashboard, payment-link-pos and the realunit screens) keep working as before.copyfromcopy-to-clipboarddirectly and keep the unchanged synchronous DOM write — the account, settings, transaction, payment-routes, invoice, payment-link and blockchain-tx screens, safe/receive, cointracking, the recommendations section andqr-code.tsx(QrCopy). None of them sit on the/buypayment-details view, so they are out of scope here..catch(() => copy(text))) runs from a promise-rejection microtask rather than synchronously inside the click handler. On a browser wherewriteTextrejects, this could run outside the strict synchronous-user-gesture windowexecCommand('copy')prefers on some engines —copy-to-clipboard's own internal fallback chain (execCommand→clipboardData→window.prompt()) still applies if that happens, so the failure mode degrades to a manual-copy prompt rather than silently dropping. If that last-resort fallback itself throws, the chain now terminates in a deliberate swallow (.catch(() => undefined)) instead of escaping as an unhandled rejection. Not chasing this further here: on a top-level document awriteTextrejection is uncommon, and the only way to fully close it would be reintroducing a synchronous DOM write on every click — the exact thing this PR removes. One deployment where the rejection is not uncommon: a cross-origin Iframe embed withoutallow="clipboard-write"never gets the native path, so every copy takes the fallback there — docs: set clipboard-write permission on the Iframe example #1296 adds the attribute to the documented Iframe example.key={index}map inexchange-rate.tsxnoted during investigation of this crash — that's a separate, lower-confidence smell that isn't itself known to cause this class of error.Test plan
npm test— 74 suites / 802 tests pass, including new coverage for both thewriteTextpath and its fallback: an explicitnavigator.clipboard-undefined context assertingcopy-to-clipboardis called (and that the reset still fires there), a throwing-fallback case asserting the chain settles without an unhandled rejection and still resetsisCopying, and a case wherecopy-to-clipboarditself throws synchronously, assertingisCopyingstill resets — all verified by mutation (removing the trailing.catch, the else-branch reset, or the sync branch'stry/finallyeach fail the suite)npm run linton changed filessrc/hooks/clipboard.hook.ts— 100% stmt/branch/func/line/buyin a real browser before merge