perf(ui): cache search keyboard navigation targets#2492
perf(ui): cache search keyboard navigation targets#2492trivikr wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaced per-keypress global DOM queries with a component-scoped cached list of focusable result elements backed by a MutationObserver that schedules RAF-batched refreshes. Keydown navigation reads from the cached list; observer lifecycle and RAF cancellation are started when the results ref is available and cleaned up on unmount. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (keyboard)
participant Comp as SearchComponent
participant Results as ResultsContainer<br/>`<section ref="resultsContainerRef">`
participant Observer as MutationObserver
participant DOM as DOM (focusable elements)
Note over Comp,Results: Mount / init
Comp->>Results: initial querySelectorAll([data-result-index],[data-suggestion-index])
Comp->>Comp: populate cached focusableElements
Comp->>Observer: observe Results (subtree, attributes, childList)
Note over User,Comp: Keyboard navigation
User->>Comp: ArrowUp/ArrowDown keydown
Comp->>Comp: read cached focusableElements -> move focus
Note over Results,Observer: Results update / visibility change
Results->>Observer: mutation events
Observer->>Comp: schedule RAF to refresh cache
Comp->>Results: re-query focusable elements -> update cached focusableElements
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ 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
🤖 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/pages/search.vue`:
- Around line 519-536: The deferred refresh can run after the results section
has been torn down; inside the watcher callback that currently calls nextTick(()
=> scheduleFocusableElementsRefresh()), guard before scheduling by checking the
relevant DOM/component ref (e.g., the results container ref or root ref used by
scheduleFocusableElementsRefresh) is still non-null/not unmounted; if the ref is
null, bail and do not call scheduleFocusableElementsRefresh. Update the watcher
closure (the block referencing nextTick and scheduleFocusableElementsRefresh) to
perform this null-check so the rAF is never queued for a removed element.
🪄 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: fc324e9e-b4ce-4cfd-b1e0-a11af619e087
📒 Files selected for processing (1)
app/pages/search.vue
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/pages/search.vue (2)
741-746: Redundant RAF cancellation afterstopObservingFocusableElements().Lines 743-746 cancel
refreshFocusableElementsFrameand null it, butstopObservingFocusableElements()on line 742 already does this (see lines 446-449). This duplication can be removed.Suggested cleanup
onBeforeUnmount(() => { stopObservingFocusableElements() - if (refreshFocusableElementsFrame != null) { - window.cancelAnimationFrame(refreshFocusableElementsFrame) - refreshFocusableElementsFrame = null - } updateLiveRegionMobile.cancel() updateLiveRegionDesktop.cancel() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/search.vue` around lines 741 - 746, The onBeforeUnmount hook contains redundant cancellation of refreshFocusableElementsFrame after calling stopObservingFocusableElements(); remove the explicit window.cancelAnimationFrame(refreshFocusableElementsFrame) and the refreshFocusableElementsFrame = null lines in the onBeforeUnmount block since stopObservingFocusableElements() already handles cancelling and clearing refreshFocusableElementsFrame (refer to stopObservingFocusableElements and refreshFocusableElementsFrame in the same file and the onBeforeUnmount hook).
470-473: Consider removing the redundant scheduled refresh.After the synchronous
refreshFocusableElements()call on line 472, the immediatescheduleFocusableElementsRefresh()on line 473 appears unnecessary. The cache is already populated, and the MutationObserver will handle any subsequent DOM changes. The extra RAF will effectively no-op, but removing it would simplify the logic.Suggested simplification
// Perform an initial synchronous refresh so focusableElements is populated // before any immediate key handling (ArrowUp/ArrowDown) occurs. refreshFocusableElements() - scheduleFocusableElementsRefresh() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/search.vue` around lines 470 - 473, The initial synchronous call to refreshFocusableElements() already populates the focusableElements cache, so remove the redundant scheduled refresh by deleting the call to scheduleFocusableElementsRefresh() immediately after refreshFocusableElements(); rely on the existing MutationObserver and scheduleFocusableElementsRefresh() when DOM changes occur rather than invoking it synchronously here (adjust any comments accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/pages/search.vue`:
- Around line 741-746: The onBeforeUnmount hook contains redundant cancellation
of refreshFocusableElementsFrame after calling stopObservingFocusableElements();
remove the explicit window.cancelAnimationFrame(refreshFocusableElementsFrame)
and the refreshFocusableElementsFrame = null lines in the onBeforeUnmount block
since stopObservingFocusableElements() already handles cancelling and clearing
refreshFocusableElementsFrame (refer to stopObservingFocusableElements and
refreshFocusableElementsFrame in the same file and the onBeforeUnmount hook).
- Around line 470-473: The initial synchronous call to
refreshFocusableElements() already populates the focusableElements cache, so
remove the redundant scheduled refresh by deleting the call to
scheduleFocusableElementsRefresh() immediately after refreshFocusableElements();
rely on the existing MutationObserver and scheduleFocusableElementsRefresh()
when DOM changes occur rather than invoking it synchronously here (adjust any
comments accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ace559c-15a4-4342-9712-943bb4675beb
📒 Files selected for processing (1)
app/pages/search.vue
serhalp
added
needs review
…sly on start (#7) Co-authored-by: trivikr <16024985+trivikr@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
trivikr
force-pushed
the
get-focusable-elements
branch
from
edf1725 to
c2e8d16
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/pages/search.vue (1)
514-516: Consider collapsing theonMountedand theresultsContainerRefwatcher.
startObservingFocusableElements()is invoked both fromonMounted(Line 620-622) and fromwatch(resultsContainerRef, …)(Line 514-516). WhencommittedQueryis truthy on initial mount, the template ref is assigned during the mount cycle and the watcher will fire in addition toonMounted, causing two consecutive stop-then-start cycles. It's safe because the helper is idempotent, but theonMountedhook is redundant — the watcher alone already covers both the initial null→element transition and subsequent toggles of the results<section>.♻️ Suggested simplification
-watch(resultsContainerRef, () => { - startObservingFocusableElements() -}) +watch( + resultsContainerRef, + () => { + startObservingFocusableElements() + }, + { flush: 'post', immediate: true }, +)And drop the dedicated mount hook:
-onMounted(() => { - startObservingFocusableElements() -})
flush: 'post'also guarantees the DOM is in its updated state when the handler runs, which matches whatrefreshFocusableElements()assumes.Also applies to: 620-622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/search.vue` around lines 514 - 516, The onMounted hook calling startObservingFocusableElements is redundant because the watch(resultsContainerRef, ...) already handles the initial null→element transition and subsequent changes; remove the onMounted(...) invocation and rely solely on watch(resultsContainerRef, handler, { flush: 'post' }) so the handler runs after DOM updates and startObservingFocusableElements is invoked exactly once when the ref becomes available; keep the existing watcher and ensure it uses flush: 'post' to match refreshFocusableElements() expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/pages/search.vue`:
- Around line 514-516: The onMounted hook calling
startObservingFocusableElements is redundant because the
watch(resultsContainerRef, ...) already handles the initial null→element
transition and subsequent changes; remove the onMounted(...) invocation and rely
solely on watch(resultsContainerRef, handler, { flush: 'post' }) so the handler
runs after DOM updates and startObservingFocusableElements is invoked exactly
once when the ref becomes available; keep the existing watcher and ensure it
uses flush: 'post' to match refreshFocusableElements() expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2befb513-407c-46d5-918d-f92651424222
📒 Files selected for processing (1)
app/pages/search.vue
🔗 Linked issue
N/A
🧭 Context
The search page supports keyboard navigation across suggestions and package results with
ArrowUp,ArrowDown, andEnter.That navigation path was doing repeated DOM queries and per-key sorting to rebuild the list of selectable elements on every key press. On larger result sets, that adds unnecessary work to a hot interaction path and can make keyboard movement through results feel sluggish.
While optimizing that path, a follow-up issue showed up: the cached focus targets were being populated on the next animation frame, which left a brief window where results were visible but keyboard navigation had no cached targets yet. In practice, if a user pressed an arrow key immediately after results rendered, nothing could happen. There was also a teardown gap where a pending animation frame could refill the cache after observation had stopped.
📚 Description
This change updates search keyboard navigation to cache focusable result elements instead of querying and sorting the DOM on every key press.
It introduces a scoped results container ref, maintains the cached focus target list with a
MutationObserver, and refreshes that cache when relevant search UI state changes. This keeps navigation work lightweight while preserving the existing DOM-order behavior of suggestions first, then package results.