fix(clipboard): stop toasting a copy that never happened (BB-215) - #4947
Conversation
The Share modal's "Copy link" showed "Link copied" over an untouched
clipboard. `navigator.clipboard.writeText` rejects with NotAllowedError, the
fallback appends its scratch textarea to `<body>` — outside the Radix dialog —
and the dialog's focus trap pulls focus back off it the moment `select()` runs,
collapsing the selection. `execCommand("copy")` returns true anyway, so
`copyToClipboard` forwarded a success for a copy that took nothing.
Not specific to the share modal: every caller inside a dialog had the same
false positive, including ScenarioShareEmptyPanel, UrlElicitationConsent,
error-card and the json-editor copy buttons.
Append the textarea to the open dialog so the selection can take, and check
that the selection spans the textarea before trusting execCommand's return
value. The check measures against `textarea.value`, not the input string: a
textarea normalizes CRLF to LF, so comparing against `text.length` would
reject every multi-line copy made on Windows.
The two regression tests — "reports FAILURE when the fallback selection does
not take" and "puts the scratch textarea inside the open dialog" — both fail
against the previous implementation.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughThe clipboard fallback now appends its scratch textarea inside the nearest open dialog when available. It verifies that selection succeeded before running Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The clipboard fallback now reports failure when it cannot select text and works within dialog focus traps. No actionable merge-blocking risk remains. 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 |
Internal previewPreview URL: https://mcp-inspector-pr-4947.up.railway.app |
This branch was successfully deployed
3 participants
Fixes BB-215.
What broke
Open a User Testing study, click Share, click Copy link. A green "Link copied" toast appears and nothing is written to the clipboard.
The chain, all inside
client/src/lib/clipboard.ts:navigator.clipboard.writeText()gets the correct URL and rejects withNotAllowedError: Write permission denied.catchruns theexecCommandfallback, which appends its scratch<textarea>todocument.body— outside the Radix dialog.FocusScopesees focus leave the scope the momentselect()runs and pulls it back synchronously, collapsing the selection.document.execCommand("copy")returnstrueanyway, socopyToClipboardforwards a success andShareSection.handleCopyLinktoasts "Link copied".The file already guarded against the neighbouring mistake — its comment says that swallowing the result would make every caller show a success toast for a copy that never happened, and the code forwards the result faithfully. The problem was one layer deeper: the source itself lies.
Not specific to the share modal. Every caller of
copyToClipboardthat runs inside a dialog had the same false positive —ScenarioShareEmptyPanel,UrlElicitationConsent,error-card, thejson-editorcopy buttons.The fix
document.activeElement?.closest('[role="dialog"],[role="alertdialog"]')) instead of<body>, so the focus trap leaves the selection alone and the copy actually works.alertdialogis in the selector becauseShareSectionmounts anAlertDialogfor link rotation.execCommand. If it does not span the textarea, returnfalseand don't run the copy. This makes the failure honest even where the fallback still can't work.One deviation from the fix suggested on the ticket: the check measures against
textarea.value.length, nottext.length. A textarea normalizes CRLF to LF, so comparing against the input string would returnfalsefor every multi-line copy containing\r\n— trading a false positive for a false negative in the callers that copy JSON, commands and traces. Verified in jsdom:"a\r\nb"is 4 characters, and the textarea's value is 3.Tests
Three added to
clipboard.test.ts. The first two fail against the previous implementation:reports FAILURE when the fallback selection does not take— stubsselect()to a no-op whileexecCommandreturnstrue, which is exactly the shape of the bug.puts the scratch textarea inside the open dialog.still copies multi-line text with CRLF newlines— covers thetextarea.valuemeasurement.Out of scope
NetworkAccessError.tsxhas its own inline fallback with the sameif (document.execCommand("copy"))shape. It doesn't route throughcopyToClipboard, so this change doesn't reach it. It renders as a full-page error rather than inside a dialog, so the selection does take there and the BB-215 symptom doesn't apply — left alone rather than widened into this PR.Verification
Run on the merged tree (branch is up to date with
mainatab2a7b730), exit codes captured to a file rather than read through a pipe:npm run docs:check-tokensnpm run typechecknpm run typecheck:client -w @mcpjam/inspectorvitest run --project client1304 passed | 3 skipped, 0 assertion failuresnpm run build:inspectorclient/src/lib/__tests__/clipboard.test.tsBoth new regression tests were confirmed to fail against the pre-fix
clipboard.tsand pass after it, so they are testing the bug rather than the implementation.Two failures showed up locally that this branch does not cause:
npm testexits 1 on Windows in server-side suites (routes/web/*, theutils/harness/local/*sandbox and supervisor family,ws-native-fallback) plus theverifyscripts ofdiscord-app,surface-coreandslack-appon CRLF format checks. Nothing underserver/orcli/importslib/clipboard— this diff is two client files — and the merge-baseab2a7b730is green ontest.ymlin CI. Left untouched rather than reformatting packages this change has no business in.ScenarioChatPage.test.tsx > prompts the recipient when the server does require authorizationtimed out in the full client run and passes in isolation (exit 0). It asserts on an "Authorize again" button, andScenarioChatPagecallsnavigator.clipboard.writeTextdirectly without going throughcopyToClipboard.E2E not run locally: the diff touches neither routing, app boot, nor the OAuth debugger.
Summary by cubic
Fixes BB-215: the share modal's "Copy link" toasted success while leaving the clipboard untouched. Inside a Radix dialog, the
execCommandfallback parked its scratch textarea on<body>, the focus trap collapsed the selection, andexecCommandstill returnedtrue.The fallback now appends the textarea to the open dialog so the selection takes, and verifies the selection spans the textarea before trusting
execCommand's return value. Callers now getfalseinstead of a false positive.Bug Fixes
falseand logs a warning.textarea.value(normalizes CRLF to LF), so multi-line copies with Windows newlines still pass.Written for commit ab0c639. Summary will update on new commits.