Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe package page navigation was changed to surface funding more prominently. The inline funding link was removed from the main links list (desktop and mobile). When a funding URL exists, a dedicated LinkBase button with a heart icon appears in the desktop top navigation; it includes the keyboard shortcut 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0166c68-4f3f-48d6-a2df-02c77ff5df2d
📒 Files selected for processing (1)
app/pages/package/[[org]]/[name].vue
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb1eaf14-5ea0-49d8-a65b-5c0441547d9c
📒 Files selected for processing (1)
app/pages/package/[[org]]/[name].vue
| onKeyStroke( | ||
| e => keyboardShortcuts.value && isKeyWithoutModifiers(e, 'f') && !isEditableElement(e.target), | ||
| e => { | ||
| if (!fundingUrl.value) return | ||
| e.preventDefault() | ||
| navigateTo(fundingUrl.value, { external: true, open: { target: '_blank' } }) | ||
| }, | ||
| { dedupe: true }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Possible LinkBase implementation =="
fd 'LinkBase*' . app components
echo
echo "== Existing URL sanitisation / external navigation helpers =="
rg -n -C3 --glob '**/*.{ts,vue}' \
'javascript:|data:|sanitize|safe.*url|allowed.*protocol|new URL\(|navigateTo\('Repository: npmx-dev/npmx.dev
Length of output: 50448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for LinkBase in app directory ==="
find app -name '*LinkBase*' -o -name '*link*' -type f | head -20
echo
echo "=== Looking for the actual file and funding shortcut code ==="
sed -n '730,745p' app/pages/package/[[org]]/[name].vue
echo
echo "=== Searching for URL validation utilities in app ==="
rg -n 'export.*function.*url|export.*const.*url' app --glob '**/*.ts' | grep -i 'valid\|safe\|allow'
echo
echo "=== Checking for any protocol validation utilities in app ==="
rg -n 'protocol.*http|https.*protocol' app --glob '**/*.{ts,vue}' -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for LinkBase as a component (case-insensitive) ==="
rg -i 'linkbase|link-base' app --glob '**/*.{vue,ts}' -l
echo
echo "=== Looking for Link components in app/components ==="
fd -e vue app/components | grep -i link
echo
echo "=== Reading usePermalink.ts ==="
head -80 app/composables/usePermalink.ts
echo
echo "=== Searching for any external link or safe URL patterns in app ==="
rg -n 'external.*true|open.*target|target.*blank' app --glob '**/*.vue' -B 2 -A 2 | head -40Repository: npmx-dev/npmx.dev
Length of output: 1694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Looking for Link component files ==="
find app/components/Link -type f 2>/dev/null | head -20
echo
echo "=== Reading Link component files ==="
cat app/components/Link/*.vue 2>/dev/null | head -150
echo
echo "=== Checking for LinkBase usage in the funding section ==="
sed -n '850,900p' app/pages/package/[[org]]/[name].vue
echo
echo "=== Looking for fundingUrl computation ==="
sed -n '520,550p' app/pages/package/[[org]]/[name].vueRepository: npmx-dev/npmx.dev
Length of output: 8066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify LinkBase component name (check if it's exported as LinkBase or something else) ==="
grep -n "export\|name:" app/components/Link/Base.vue | head -10
echo
echo "=== Check how LinkBase is imported/used in the page ==="
grep -n "import.*LinkBase\|LinkBase" app/pages/package/[[org]]/[name].vue | head -5
echo
echo "=== Verify navigateTo is being used and understand its behavior ==="
grep -n "navigateTo" app/pages/package/[[org]]/[name].vueRepository: npmx-dev/npmx.dev
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
echo "=== Check component registration in nuxt.config ==="
grep -n "LinkBase\|Link/Base" nuxt.config.ts app.vue 2>/dev/null || echo "Not found in those files"
echo
echo "=== Verify the page uses LinkBase and check surrounding context ==="
sed -n '1,50p' app/pages/package/[[org]]/[name].vue | grep -E "import|script"
echo
echo "=== Confirm navigateTo usage in shortcut handler ==="
sed -n '730,745p' app/pages/package/[[org]]/[name].vue
echo
echo "=== Check if there are any tests for the funding shortcut ==="
find test -name "*.spec.ts" -o -name "*.spec.vue" | xargs grep -l "fundingUrl\|funding.*shortcut" 2>/dev/null || echo "No tests found"Repository: npmx-dev/npmx.dev
Length of output: 1592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify that LinkBase is used for the funding button ==="
sed -n '865,885p' app/pages/package/[[org]]/[name].vue
echo
echo "=== Check what keyboardShortcutsEnabled is in LinkBase ==="
grep -n "keyboardShortcutsEnabled" app/components/Link/Base.vue
echo
echo "=== Verify aria-keyshortcuts handling in LinkBase ==="
grep -n "aria-keyshortcuts" app/components/Link/Base.vueRepository: npmx-dev/npmx.dev
Length of output: 1507
Add protocol validation to the funding keyboard shortcut.
The keyboard shortcut at lines 732–740 calls navigateTo(fundingUrl.value, { external: true, ... }) without protocol validation. Unlike the LinkBase button which restricts external links to http:, https:, and //, the shortcut bypasses this check. An unsafe protocol such as javascript: or data: in the package metadata would be reachable only via the global shortcut path. Validate the protocol before navigating, or explicitly allow only HTTP(S) schemes.
Suggested fix
onKeyStroke(
e => keyboardShortcuts.value && isKeyWithoutModifiers(e, 'f') && !isEditableElement(e.target),
e => {
- if (!fundingUrl.value) return
+ if (!fundingUrl.value) return
+
+ let safeFundingUrl: string | null = null
+ try {
+ const parsed = new URL(fundingUrl.value, window.location.origin)
+ if (parsed.protocol === 'http:' || parsed.protocol === 'https:') {
+ safeFundingUrl = parsed.toString()
+ }
+ } catch {
+ return
+ }
+
+ if (!safeFundingUrl) return
e.preventDefault()
- navigateTo(fundingUrl.value, { external: true, open: { target: '_blank' } })
+ navigateTo(safeFundingUrl, { external: true, open: { target: '_blank' } })
},
{ dedupe: true },
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onKeyStroke( | |
| e => keyboardShortcuts.value && isKeyWithoutModifiers(e, 'f') && !isEditableElement(e.target), | |
| e => { | |
| if (!fundingUrl.value) return | |
| e.preventDefault() | |
| navigateTo(fundingUrl.value, { external: true, open: { target: '_blank' } }) | |
| }, | |
| { dedupe: true }, | |
| ) | |
| onKeyStroke( | |
| e => keyboardShortcuts.value && isKeyWithoutModifiers(e, 'f') && !isEditableElement(e.target), | |
| e => { | |
| if (!fundingUrl.value) return | |
| let safeFundingUrl: string | null = null | |
| try { | |
| const parsed = new URL(fundingUrl.value, window.location.origin) | |
| if (parsed.protocol === 'http:' || parsed.protocol === 'https:') { | |
| safeFundingUrl = parsed.toString() | |
| } | |
| } catch { | |
| return | |
| } | |
| if (!safeFundingUrl) return | |
| e.preventDefault() | |
| navigateTo(safeFundingUrl, { external: true, open: { target: '_blank' } }) | |
| }, | |
| { dedupe: true }, | |
| ) |
| <LinkBase | ||
| variant="button-secondary" | ||
| v-if="fundingUrl" | ||
| :to="fundingUrl" | ||
| aria-keyshortcuts="f" | ||
| classicon="i-lucide:heart" | ||
| :title="$t('package.links.fund')" | ||
| > | ||
| <span class="max-sm:sr-only">{{ $t('package.links.fund') }}</span> | ||
| </LinkBase> |
There was a problem hiding this comment.
Only advertise the f shortcut when shortcuts are enabled.
This button always exposes aria-keyshortcuts="f", but the handler only runs when useKeyboardShortcuts() is enabled. When the setting is off, assistive tech is told about a shortcut that does nothing.
Proposed fix
<LinkBase
variant="button-secondary"
v-if="fundingUrl"
:to="fundingUrl"
- aria-keyshortcuts="f"
+ :aria-keyshortcuts="keyboardShortcuts ? 'f' : undefined"
classicon="i-lucide:heart"
:title="$t('package.links.fund')"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <LinkBase | |
| variant="button-secondary" | |
| v-if="fundingUrl" | |
| :to="fundingUrl" | |
| aria-keyshortcuts="f" | |
| classicon="i-lucide:heart" | |
| :title="$t('package.links.fund')" | |
| > | |
| <span class="max-sm:sr-only">{{ $t('package.links.fund') }}</span> | |
| </LinkBase> | |
| <LinkBase | |
| variant="button-secondary" | |
| v-if="fundingUrl" | |
| :to="fundingUrl" | |
| :aria-keyshortcuts="keyboardShortcuts ? 'f' : undefined" | |
| classicon="i-lucide:heart" | |
| :title="$t('package.links.fund')" | |
| > | |
| <span class="max-sm:sr-only">{{ $t('package.links.fund') }}</span> | |
| </LinkBase> |
|
Hey @parion, This looks really cool❤️ Can we change the order of button group? to this Instead of Fund coming first? Just my thought and perspective 😇 |
How's that looking? |
|
@parion This is great now🥳 |
|
that button bar is semantically only for things that are 'on npmx' rather than links out ... so I love the highlighting for funding, but I think we need another place for it |
3 participants
resolves #1835
🧭 Context
For the reasons given in the attached issue, the fund button needed more visibility on package pages. I moved the fund link from the external link list to the top button group. This also enables the
fkey to activate the fund link.Please provide any feedback on this design change! I'm noticing this button bar is getting pretty wide, I was considering scaling it down.
Desktop View
Mobile View