Skip to content

fix: guard clipboard copy actions#2292

Open
AjTheSpidey wants to merge 3 commits into
hyperdxio:mainfrom
AjTheSpidey:codex/clipboard-copy-fallback
Open

fix: guard clipboard copy actions#2292
AjTheSpidey wants to merge 3 commits into
hyperdxio:mainfrom
AjTheSpidey:codex/clipboard-copy-fallback

Conversation

@AjTheSpidey
Copy link
Copy Markdown

Summary

Fixes #2135.

Adds a shared copyTextToClipboard helper for the trace/event copy actions. It now:

  • uses navigator.clipboard.writeText when the browser permits it
  • falls back to a hidden textarea + document.execCommand('copy') path when the Clipboard API is unavailable, such as insecure/non-HTTPS contexts
  • shows a clear error notification if both copy methods fail instead of throwing Cannot read properties of undefined (reading 'writeText')

Screenshots or video

No screenshot included. This changes the failure/fallback path for browser clipboard permissions rather than the normal visible layout.

How to test on Vercel preview

Preview routes: /search

Steps:

  1. Open a search result row with event or trace attributes.
  2. Use the copy button for a row value or row JSON.
  3. Verify copying succeeds in secure contexts, or shows Could not access the clipboard. Check browser permissions or use HTTPS. if clipboard access is unavailable.

References

  • Linear Issue: N/A
  • Related PRs: N/A

Validation

  • yarn build:common-utils
  • yarn workspace @hyperdx/app jest --runTestsByPath src/utils/clipboard.test.ts src/components/DBRowJsonViewer.test.tsx --coverage=false (passes; existing Mantine notification act(...) warnings are printed)
  • yarn workspace @hyperdx/app exec tsc --noEmit
  • yarn workspace @hyperdx/app exec npx eslint --quiet src/utils/clipboard.ts src/utils/clipboard.test.ts src/components/DBRowJsonViewer.tsx src/components/DBTable/DBRowTableFieldWithPopover.tsx src/components/DBTable/DBRowTableRowButtons.tsx

Note: full yarn workspace @hyperdx/app lint is blocked locally on Windows by unrelated CRLF Prettier errors across existing files in the checkout.

@vercel
Copy link
Copy Markdown

vercel Bot commented May 15, 2026

@AjTheSpidey is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 15, 2026

🦋 Changeset detected

Latest commit: 345161c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

PR Review

✅ No critical issues found.

Solid, well-scoped fix for #2135. A few minor observations (non-blocking):

  • ⚠️ In DBRowTableRowButtons.tsx:71-76, the outer try/catch now only protects the JSON serialization (since copyTextToClipboard never throws), but shows CLIPBOARD_ERROR_MESSAGE for any failure. → Consider a distinct message for serialization errors, or narrow the try scope to just the JSON work.
  • ⚠️ copyTextWithTextarea calls textArea.select() followed by setSelectionRange(0, length) while readonly is set — works in modern browsers but iOS Safari historically requires the input to be contentEditable and non-readonly for execCommand('copy') to succeed. → Acceptable since iOS Safari supports the async Clipboard API; just noting the fallback path won't help iOS specifically.
  • ℹ️ Test file lives at src/utils/__tests__/clipboard.test.ts which matches the project convention (the PR description mentions src/utils/clipboard.test.ts — minor description inaccuracy only).
  • ℹ️ Good test coverage including selection restoration and the dual fallback failure path. Nice.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

Deep Review

✅ No critical issues found.

🟡 P2 — recommended

  • packages/app/src/components/DBRowJsonViewer.tsx:226 — Three new if (!copied) notifications.show({ color: 'red', … }) branches were added to HyperJsonMenu, handleCopyObject, and the Copy Value menu item, but no test exercises the failure path or asserts the success-only side effects do not fire.
    • Fix: Add component tests that stub copyTextToClipboard to resolve false and assert the red notification fires while setIsCopied/green toast do not.
    • testing, maintainability
  • packages/app/src/components/DBTable/DBRowTableFieldWithPopover.tsx:91copyFieldValue's new failure branch and the absence of the setIsCopied(true) transition on failure are untested; a regression that always flips isCopied would pass.
    • Fix: Add a test that forces copyTextToClipboard to resolve false and asserts notifications.show was called and isCopied stayed false.
    • testing
  • packages/app/src/components/DBTable/DBRowTableRowButtons.tsx:61 — The two new if (!copied) notification branches in copyRowData and copyRowUrl, plus the new red-toast catch in copyRowData, have no component-level coverage.
    • Fix: Add tests that drive both failure branches and the catch path.
    • testing
🔵 P3 nitpicks (10)
  • packages/app/src/components/DBTable/DBRowTableRowButtons.tsx:33 — The outer try/catch now reports CLIPBOARD_ERROR_MESSAGE for what can only be a JSON serialization defect, since copyTextToClipboard no longer throws; the toast misattributes the failure.
    • Fix: Either drop the outer try/catch or change the catch message to reflect a serialization failure rather than a clipboard one.
    • maintainability, kieran-typescript
  • packages/app/src/components/DBTable/DBRowTableRowButtons.tsx:79copyRowUrl lost its previous try/catch, so a throw from getRowWhere(row) or new URL(window.location.href) now becomes an unhandled rejection with no toast, while sibling copyRowData still wraps in a try/catch.
    • Fix: Wrap copyRowUrl in the same try/catch shape as copyRowData for consistent failure surfacing.
    • correctness, julik-frontend-races
  • packages/app/src/utils/clipboard.ts:53document.body.removeChild and previousActiveElement.focus run unwrapped inside the finally; if either throws, the function rejects instead of resolving false, and callers without try/catch get an unhandled rejection with no toast.
    • Fix: Wrap the cleanup statements in their own try/catch so the helper always honors its Promise<boolean> contract.
    • correctness
  • packages/app/src/utils/clipboard.ts:1 — The if (!copied) notifications.show({ color: 'red', message: CLIPBOARD_ERROR_MESSAGE }); return; block is duplicated across five call sites; the helper exporting only a message string keeps the boilerplate one bug away from drifting.
    • Fix: Expose a copyWithFeedback(text, { successMessage }) wrapper that owns both toasts, or move the failure toast into the helper.
    • maintainability
  • packages/app/src/utils/clipboard.ts:4Promise<boolean> discards the failure reason, so every caller reuses the same "Check browser permissions or use HTTPS" copy even when the cause was a denied permission or execCommand rejection.
    • Fix: Return a discriminated { ok: true } | { ok: false; reason: 'unavailable' | 'denied' | 'exec-failed' } so callers can tailor the message.
    • maintainability, kieran-typescript
  • packages/app/src/utils/clipboard.ts:5navigator.clipboard?.writeText will throw ReferenceError if the helper is ever imported into an environment without navigator (SSR, workers); the optional chaining only protects against missing properties.
    • Fix: Add a typeof navigator === 'undefined' guard before the property access.
    • correctness
  • packages/app/src/utils/clipboard.ts:39document.activeElement instanceof HTMLElement skips focusable SVG (and MathML) elements; focus is silently dropped after the fallback when the user had a focusable SVG node selected.
    • Fix: Widen the narrowing to include SVGElement, or feature-detect a focus method via the HTMLOrSVGElement mixin.
    • kieran-typescript
  • packages/app/src/components/DBTable/DBRowTableFieldWithPopover.tsx:103 — Each click schedules a fresh 2 s setTimeout without clearing the previous one, so a second click within the window can clear isCopied ~100 ms later; same pattern at DBRowTableRowButtons.tsx:70 and :96.
    • Fix: Stash the timeout id in a useRef and clearTimeout it before scheduling a new one (and on unmount).
    • correctness, julik-frontend-races
  • packages/app/src/components/DBTable/DBRowTableFieldWithPopover.tsx:39 — The existing cleanup effect tracks hover timers in refs but the new copy setTimeout is untracked, so a popover unmount within 2 s of a copy fires setIsCopied on a dead component (dev warning, leaked timer).
    • Fix: Add a third ref for the copy timer and clear it alongside the existing cleanup.
    • julik-frontend-races
  • packages/app/src/utils/__tests__/clipboard.test.ts:110jest.spyOn(selection!, 'addRange') mixes a non-null assertion with ?.-guarded calls elsewhere in the same test; a future JSDOM that returns null would mask the real cause inside spyOn.
    • Fix: Replace ! with an explicit guard that throws a clear assertion if selection is null.
    • kieran-typescript

Reviewers (5): correctness, testing, maintainability, julik-frontend-races, kieran-typescript.

Testing gaps:

  • No component-level coverage for the new if (!copied) failure-notification branches in any of the three refactored components.
  • No coverage for the previousActiveElement/document.contains focus-restoration path or the typeof document === 'undefined' SSR guard in clipboard.ts.
  • No test for the writeText-rejects-then-fallback-also-fails compound path, nor for copyTextToClipboard('') with an empty string.
  • The copyRowData outer catch in DBRowTableRowButtons.tsx is currently unreachable from any test; coverage would either confirm or refute it.

@AjTheSpidey AjTheSpidey force-pushed the codex/clipboard-copy-fallback branch from d7405ce to 87f079b Compare May 16, 2026 10:23
@AjTheSpidey
Copy link
Copy Markdown
Author

Pushed an update after rebasing on latest main.

I changed the clipboard helper so it only uses the textarea fallback when the Clipboard API is missing. If navigator.clipboard.writeText exists but rejects, it now returns false instead of trying the fallback after the async rejection. I also added coverage for that case and for execCommand throwing while making sure the temporary textarea is removed.

Checked locally:

  • jest src/utils/clipboard.test.ts --runInBand
  • focused eslint on the touched app files
  • tsc --noEmit --pretty false
  • git diff --check

@AjTheSpidey
Copy link
Copy Markdown
Author

Pushed another small follow-up for the deep-review notes.

This now tries the textarea fallback if navigator.clipboard.writeText rejects, moves the util test into src/utils/tests, adds coverage for selection restore / cleanup, keeps the row JSON copy path guarded, and updates the changeset quote style.

Checked locally:

  • jest src/utils/tests/clipboard.test.ts --runInBand
  • focused eslint on the touched app files
  • tsc --noEmit --pretty false
  • git diff --check

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.

Copying trace attributes/columns as JSON doesn't work

1 participant