fix(ui): arrow key navigation for org page#2339
fix(ui): arrow key navigation for org page#2339tylersayshi wants to merge 6 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Hello! Thank you for opening your first PR to npmx, @tylersayshi! 🚀 Here’s what will happen next:
|
|
also, great meeting y'all at https://atmosphereconf.org - thanks for npmx ❤️ |
- adding a composable to handle arrows on org page - shared composable back with search results page fixes npmx-dev#2338
tylersayshi
force-pushed
the
tyler-fix-org-keys
branch
from
cc67c76 to
9439b16
Compare
tylersayshi
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds keyboard navigation for result lists via a new composable Suggested reviewers
🚥 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)
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✅ All modified and coverable lines are covered by tests. 📢 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/search.vue (1)
461-487:⚠️ Potential issue | 🟡 MinorScope Enter handling to the actual search input.
The Enter handler currently matches any
<input>element on the page. Please check the header search box explicitly before reading its value to prevent pressing Enter in other focused inputs from rewritingcommittedQuerywith unintended data.Suggested approach
function handleSearchInputEnter(e: KeyboardEvent) { if (!keyboardShortcuts.value) { return } + const activeElement = document.activeElement // If the active element is an input, navigate to exact match or wait for results - if (e.key === 'Enter' && document.activeElement?.tagName === 'INPUT') { + if ( + e.key === 'Enter' && + activeElement instanceof HTMLInputElement && + activeElement.matches('#header-search, input[name="q"]') + ) { // Get value directly from input (not from route query, which may be debounced) - const inputValue = (document.activeElement as HTMLInputElement).value.trim() + const inputValue = activeElement.value.trim()
🧹 Nitpick comments (1)
test/e2e/interactions.spec.ts (1)
313-362: Add one regression case for focused controls.The new handlers are document-scoped, but these tests only drive the page from neutral focus. A small case that presses ArrowDown on the org sort control and Enter on a toolbar input on the search page would catch the two easiest regressions here.
Also applies to: 411-445
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f006bc64-b295-4607-b776-f0360ae50e9a
📒 Files selected for processing (6)
app/components/BaseCard.vueapp/components/Package/Card.vueapp/composables/useResultsKeyboardNavigation.tsapp/pages/org/[org].vueapp/pages/search.vuetest/e2e/interactions.spec.ts
| tabindex="0" | ||
| :data-result-index="index" | ||
| class="group bg-bg-subtle border border-border rounded-lg p-4 sm:p-6 transition-[border-color,background-color] duration-200 hover:(border-border-hover bg-bg-muted) cursor-pointer relative focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-bg focus-visible:ring-offset-2 focus-visible:ring-accent focus-visible:bg-bg-muted focus-visible:border-accent/50" |
There was a problem hiding this comment.
Don't make every BaseCard instance a tabbable result wrapper.
app/components/SearchSuggestionCard.vue:14-23 still keeps the real interactive target on its inner NuxtLink. With this change, suggestions gain an extra wrapper tab stop, and that link no longer inherits the old card ring because the wrapper now styles only focus-visible while the link itself uses focus-visible:outline-none. Please gate the wrapper tabindex to cards that actually participate in result navigation, or keep the card's focus-within treatment for child-focused consumers.
Possible direction
- tabindex="0"
+ :tabindex="index != null ? 0 : undefined"| function handleKeydown(e: KeyboardEvent) { | ||
| // Only handle arrow keys and Enter | ||
| if (!['ArrowDown', 'ArrowUp', 'Enter'].includes(e.key)) { | ||
| return | ||
| } | ||
|
|
||
| if (!keyboardShortcuts.value) { | ||
| return | ||
| } | ||
|
|
||
| const elements = getFocusableElements() | ||
| const currentIndex = elements.findIndex(el => el === document.activeElement) | ||
|
|
||
| if (e.key === 'ArrowDown') { | ||
| // If there are results available, handle navigation | ||
| if (elements.length > 0) { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
|
|
||
| // If no result is focused, focus the first one | ||
| if (currentIndex < 0) { | ||
| const firstEl = elements[0] | ||
| if (firstEl) focusElement(firstEl) | ||
| return | ||
| } | ||
|
|
||
| // If a result is already focused, move to the next one | ||
| const nextIndex = Math.min(currentIndex + 1, elements.length - 1) | ||
| const el = elements[nextIndex] | ||
| if (el) focusElement(el) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if (e.key === 'ArrowUp') { | ||
| // Only intercept if a result is already focused | ||
| if (currentIndex >= 0) { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
|
|
||
| // At first result | ||
| if (currentIndex === 0) { | ||
| // Call custom callback if provided (e.g., return focus to search input) | ||
| if (options?.onArrowUpAtStart) { | ||
| options.onArrowUpAtStart() | ||
| } | ||
| return | ||
| } | ||
| const nextIndex = currentIndex - 1 | ||
| const el = elements[nextIndex] | ||
| if (el) focusElement(el) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if (e.key === 'Enter') { | ||
| // Handle Enter on focused card - click the main link inside | ||
| if (document.activeElement && elements.includes(document.activeElement as HTMLElement)) { | ||
| const card = document.activeElement as HTMLElement | ||
| // Find the first link inside the card and click it | ||
| const link = card.querySelector('a') | ||
| if (link) { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| link.click() | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Register keyboard event listeners using useEventListener for better control | ||
| // Use capture phase to intercept before other handlers | ||
| useEventListener(document, 'keydown', handleKeydown, { capture: true }) |
There was a problem hiding this comment.
Don't steal ArrowUp/ArrowDown from native controls.
This handler runs at document scope and calls preventDefault() before the page's own controls can react. On both app/pages/org/[org].vue:267-288 and app/pages/search.vue:700-724, pressing ArrowDown inside the toolbar's selects or text inputs will jump to the first result instead of moving the caret or opening the control. Please ignore interactive targets unless focus is already on a result element or on an explicitly allowed hand-off source.
Possible direction
export function useResultsKeyboardNavigation(options?: {
includeSuggestions?: boolean
onArrowUpAtStart?: () => void
+ canStartFrom?: (target: HTMLElement | null) => boolean
}) {
@@
const elements = getFocusableElements()
const currentIndex = elements.findIndex(el => el === document.activeElement)
+ const target = e.target instanceof HTMLElement ? e.target : null
+ const isInteractiveTarget = !!target?.closest(
+ 'input, textarea, select, button, [contenteditable="true"]',
+ )
+
+ if (currentIndex < 0 && isInteractiveTarget && !options?.canStartFrom?.(target)) {
+ return
+ }There was a problem hiding this comment.
I believe stealing the arrow keys is desired in this instance, but anyone please tell me if I'm wrong here 👍
There was a problem hiding this comment.
The Comment image shows an organization page at localhost:3000/org/atcute in a npm-like UI. Visible elements include:
- Top bar with a search input labeled “/ search packages …”, and actions: compare, settings, connect.
- Org header with avatar “A”, handle “@ATCUTE”, “58 public packages”, and an “npm” badge with “58,784 / week”.
- A PACKAGES section: “58 packages”, a “Filters” button, a sort control showing “Downloads/wk” (native select), and list/grid view toggles.
- A list of package cards (e.g.,
@atcute/time-ms,@atcute/bluesky) each with description, date, version, and weekly downloads.
@tylersayshi I love the consistency goal, but stealing ArrowUp/ArrowDown from native controls (inputs/selects/textarea/contenteditable) at the document level will block expected keyboard behavior (caret movement, opening selects, adjusting ranges) and hurts accessibility. Right now, ArrowDown is intercepted even when focus is inside the sort select or the search box, because we call preventDefault in capture before those controls can react.
A small compromise keeps the desired list navigation while preserving native control behavior:
- Only start navigation when:
- focus is already on a result element, or
- the current target is explicitly whitelisted (e.g., the search input on /search).
- Otherwise, let native controls handle Arrow keys.
Minimal patch to the composable
// app/composables/useResultsKeyboardNavigation.ts
export function useResultsKeyboardNavigation(options?: {
includeSuggestions?: boolean
onArrowUpAtStart?: () => void
canStartFrom?: (target: HTMLElement | null) => boolean
}) {
// ...
function handleKeydown(e: KeyboardEvent) {
if (!['ArrowDown', 'ArrowUp', 'Enter'].includes(e.key) || !keyboardShortcuts.value) return
const elements = getFocusableElements()
const currentIndex = elements.findIndex(el => el === document.activeElement)
const target = e.target instanceof HTMLElement ? e.target : null
const isInteractiveTarget = !!target?.closest('input, textarea, select, button, [contenteditable="true"]')
// Don’t hijack Arrow keys from interactive widgets unless navigation is already active
// or the caller explicitly allows starting from this target.
if (currentIndex < 0 && isInteractiveTarget && !options?.canStartFrom?.(target)) {
return
}
// ...existing ArrowDown/ArrowUp/Enter logic unchanged...
}
useEventListener(document, 'keydown', handleKeydown, { capture: true })
}Call-site usage
- Search page: allow starting from the search input only (so ArrowDown in the box moves to first result as today).
useResultsKeyboardNavigation({
includeSuggestions: true,
onArrowUpAtStart: focusSearchInput,
canStartFrom: (el) => !!el?.closest('input[type="search"], [data-search-input="true"]'),
})- Org page: don’t pass canStartFrom so Arrow keys inside “Downloads/wk” select, Filters inputs, etc., keep their native behavior; navigation still works once focus is on a result card.
Optional guard
- Also ignore modified keys to avoid surprises:
if (e.altKey || e.ctrlKey || e.metaKey) returnTests to lock behavior
- Add e2e asserting:
- ArrowDown inside the sort does not move focus to the first result on /org/atcute. ArrowDown in the search input on /search starts navigation (whitelisted). ArrowDown on the document body starts at first result when nothing interactive is focused. If you’d prefer truly global “steal-from-anywhere,” I can roll with that, but it will block selects and text inputs across the page. The above tweak preserves your intended UX on /search while keeping native controls functional on the org page. Want me to prep a patch? 🧠 Learnings used Learnt from: btea Repo: npmx-dev/npmx.dev PR: 1739 File: app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue:259-262 Timestamp: 2026-02-28T07:17:28.172Z Learning: In app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue, the scroll-to-top button for the code preview container should be shown on both desktop and touch devices (gated only by scroll position using SCROLL_TO_TOP_THRESHOLD), not restricted to touch devices. This is different from the window-level ScrollToTop.client.vue component which is touch-device-only, because the code container scroll-to-top helps users navigate long code files where keyboard shortcuts are less practical for container-specific scrolling. Learnt from: graphieros Repo: npmx-dev/npmx.dev PR: 2273 File: app/components/Package/TrendsChart.vue:1648-1648 Timestamp: 2026-03-25T12:31:33.715Z Learning: In `app/components/Package/TrendsChart.vue`, the `tabindex` values on the view-toggle tab buttons (combined/split chart layout) are intentionally inverted from the standard WAI-ARIA pattern. The **selected** tab receives `tabindex="-1"` and the **unselected** tab receives `tabindex="0"`. Do not flag this as a bug or suggest reversing the values to match the WAI-ARIA tabs pattern. Learnt from: userquin Repo: npmx-dev/npmx.dev PR: 1596 File: app/pages/about.vue:318-341 Timestamp: 2026-02-23T05:31:58.922Z Learning: In the npmx.dev project, the contributor popover on the about page is intentionally a mouse-only progressive enhancement. Keyboard support (focus/blur with full popover interaction) was tested but rejected due to Firefox performance issues (3-4 second lag with per-contributor popovers in the Top Layer) and implementation complexity (aria-controls, aria-expanded, focus trapping, Escape handling). All contributor information remains accessible via the clickable link to the GitHub profile. Learnt from: ShroXd Repo: npmx-dev/npmx.dev PR: 2115 File: app/pages/search.vue:365-368 Timestamp: 2026-03-17T06:53:49.002Z Learning: In npmx-dev/npmx.dev (app/pages/search.vue), the `showClaimPrompt` computed intentionally returns `true` during a `status === 'pending'` fetch even when `avail.name` doesn't match `committedQuery.value.trim()`. This is by design to prevent the claim prompt from flickering/disappearing while the user is typing (avoiding layout shifts). The claim button is separately guarded with `:disabled="status === 'pending'"` to prevent accidental clicks on stale data. Do not suggest adding an `avail.name === committedQuery` guard to the pending branch. Learnt from: danielroe Repo: npmx-dev/npmx.dev PR: 1204 File: app/composables/npm/useUserPackages.ts:40-41 Timestamp: 2026-02-08T13:24:06.104Z Learning: In Nuxt 3, when using useAsyncData or useLazyAsyncData, allow and rely on reactive keys by typing them as MaybeRefOrGetter<string>. Keys can be a string, a Ref<string>, a ComputedRef<string>, or a getter () => string. Reactive keys should automatically trigger refetches when their value changes. In reviews, verify that data scripts pass a MaybeRefOrGetter<string> (not a plain string-only key if reactivity is intended), and that the logic handles dynamic key changes without stale data. Learnt from: MatteoGabriele Repo: npmx-dev/npmx.dev PR: 1922 File: app/composables/usePackageListPreferences.ts:52-53 Timestamp: 2026-03-05T10:14:50.799Z Learning: In Nuxt projects (here npmx-dev/npmx.dev), exports from shared/types/* (e.g., PAGE_SIZE_OPTIONS, DEFAULT_COLUMNS, DEFAULT_PREFERENCES, PageSize) are auto-imported by Nuxt for composables and components. Do not add explicit import statements for these constants/types when using files under shared/types/, and rely on the auto-imported bindings in files under app/composables (and similarly in components). This pattern applies to all TS files within app/composables that reference these shared/types exports.
tylersayshi
force-pushed
the
tyler-fix-org-keys
branch
from
ac52d75 to
848a1ad
Compare
1 participant
🔗 Linked issue
resolves #2338
🧭 Context
This adds the key control for the org page like on https://npmx.dev/org/atcute
📚 Description