perf(ui): cache search keyboard navigation targets#2492
Open
trivikr wants to merge 5 commits intonpmx-dev:mainfrom
Open
perf(ui): cache search keyboard navigation targets#2492trivikr wants to merge 5 commits intonpmx-dev:mainfrom
trivikr wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
…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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaced per-keystroke global DOM queries with a component-scoped cached list of focusable result elements backed by a MutationObserver (RAF-batched refresh). Keydown navigation now uses the cached list; observer lifecycle is started when the results container 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\r`<section ref="resultsContainerRef">`
participant Observer as MutationObserver
participant DOM as DOM (focusable elements)
Note over Comp,Results: Mount / init
Comp->>Results: 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: use cached focusableElements to 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}
Contributor
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
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 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.