fix: resolve readme copy functionality in Safari#2554
fix: resolve readme copy functionality in Safari#2554graphieros merged 8 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced the README "copy as Markdown" clipboard wiring on the package page with an async clipboard composable. The page now uses Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (click)"
participant Page as "Package Page\n(`copyReadme`)" rect rgba(52,152,219,0.5)
participant Composable as "useClipboardAsync\n(copy)" rect rgba(46,204,113,0.5)
participant Fetch as "fetchReadmeMarkdown()" rect rgba(155,89,182,0.5)
User->>Page: Click "Copy README"
Page->>Composable: call copy() (async provider)
Composable->>Fetch: invoke async fn
Fetch-->>Composable: returns markdown string
Composable->>Composable: create ClipboardItem(Blob(markdown))
Composable->>Composable: navigator.clipboard.write([item])
Composable-->>Page: set copied = true
Page-->>User: UI feedback (copied)
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/composables/useClipboardAsync.ts (1)
8-10:copiedDuringdefault of0makes the copied state unobservable.
copiedDuringis typed as required onUseClipboardAsyncOptionsbut read withoptions?.copiedDuring ?? 0. Two small issues:
- The type says required but the runtime treats it as optional — mark it optional (
copiedDuring?: number) to match usage.- A default of
0ms resetscopiedon the next tick, so any UI binding tocopiedwill effectively never seetrue. Consider a sensible default (e.g.1500) to match typical clipboard-feedback UX.♻️ Proposed change
type UseClipboardAsyncOptions = { - copiedDuring: number + copiedDuring?: number } @@ - const timeout = useTimeoutFn(() => (copied.value = false), options?.copiedDuring ?? 0, { + const timeout = useTimeoutFn(() => (copied.value = false), options?.copiedDuring ?? 1500, { immediate: false, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useClipboardAsync.ts` around lines 8 - 10, The UseClipboardAsyncOptions type currently declares copiedDuring as required but the runtime reads options?.copiedDuring ?? 0, and the 0ms default makes the copied state unobservable; update the type to make copiedDuring optional (copiedDuring?: number) and change the runtime default from 0 to a sensible value (e.g. 1500) where used in the useClipboardAsync composable (the options?.copiedDuring ?? 0 expression) so UI bindings can observe the copied state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/composables/useClipboardAsync.ts`:
- Around line 21-31: The copy function currently sets copied.value = true before
calling navigator.clipboard.write and does not handle its Promise; change copy
to return Promise<void>, create the ClipboardItem as you do but await
navigator.clipboard.write([...]) (or attach .then/.catch) and only set
copied.value = true and call timeout.start() after the write promise resolves;
in the catch handler reset copied.value = false, surface or rethrow the error so
callers can handle it, and ensure any rejection from fn() propagated by the
ClipboardItem is also handled via the awaited write call.
---
Nitpick comments:
In `@app/composables/useClipboardAsync.ts`:
- Around line 8-10: The UseClipboardAsyncOptions type currently declares
copiedDuring as required but the runtime reads options?.copiedDuring ?? 0, and
the 0ms default makes the copied state unobservable; update the type to make
copiedDuring optional (copiedDuring?: number) and change the runtime default
from 0 to a sensible value (e.g. 1500) where used in the useClipboardAsync
composable (the options?.copiedDuring ?? 0 expression) so UI bindings can
observe the copied state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce1e0991-99c1-47bb-af65-61ac19e0656f
📒 Files selected for processing (2)
app/composables/useClipboardAsync.tsapp/pages/package/[[org]]/[name].vue
✅ Files skipped from review due to trivial changes (1)
- app/pages/package/[[org]]/[name].vue
…ithub.com/MatteoGabriele/npmx.dev into fix/async-clipboard-not-working-in-safari
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.lighthouserc.cjs (1)
28-28: Unrelated config change; scope check.This Lighthouse CI readiness pattern tweak appears unrelated to the Safari clipboard fix described in the PR objectives. The broader regex is reasonable (matches Nuxt/Vite dev output), but consider splitting unrelated infra changes into their own PR for cleaner history, or clarify in the description why it's bundled here.
One minor note:
:3000will match any line containing that substring (including unrelated log output that happens to reference the port). If that's intentional as a catch-all, no action needed; otherwise anchoring to something likelocalhost:3000would be slightly more precise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.lighthouserc.cjs at line 28, The change to startServerReadyPattern in .lighthouserc.cjs is an unrelated infra tweak; either move this regex update into its own PR for cleaner history or document in the current PR why it’s bundled, and if you want the pattern to be more precise update startServerReadyPattern to anchor the port (for example use 'localhost:3000' or add word/line boundaries) instead of the generic ':3000' so it doesn’t accidentally match unrelated log lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.lighthouserc.cjs:
- Line 28: The change to startServerReadyPattern in .lighthouserc.cjs is an
unrelated infra tweak; either move this regex update into its own PR for cleaner
history or document in the current PR why it’s bundled, and if you want the
pattern to be more precise update startServerReadyPattern to anchor the port
(for example use 'localhost:3000' or add word/line boundaries) instead of the
generic ':3000' so it doesn’t accidentally match unrelated log lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d01b413f-4013-4e25-a883-4a700228ebc8
📒 Files selected for processing (1)
.lighthouserc.cjs
knowler
left a comment
knowler
left a comment
There was a problem hiding this comment.
Works! I remember running into this one time and using a similar solution.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Ty for solving this! Did you play around with useClipboardItems from vueuse? I tried to get it to work but I couldn't 🫠 |
I did, but I would crash on me, so I went the custom way. At least for the asynchronous operations. Perhaps opening an issue on vueuse could be an option as well. |
4 participants
🔗 Linked issue
resolves #2151
🧭 Context
The copy Readme button doesn't work in Safari
📚 Description
Unfortunately, for security reasons, Safari doesn't allow the Clipboard API to be used with async operations, so it silently fails without copying anything.
I've created a custom useClipboardAsync that addresses the security issues and handles the async call.