perf(Collection): replace indexOf sort with node index map in getItems#2695
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR optimizes the item sorting logic in the Collection component by replacing repeated DOM node lookups with a precomputed index map, and updates the typo checker configuration to exempt the ChangesCollection Sorting and Configuration Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
getItems() sorted registered items into DOM order using Array.indexOf inside the sort comparator, scanning orderedNodes on every comparison (O(n² log n)). Build a node→index Map once and sort by lookup instead, giving O(n log n) with two O(n) passes — identical output. The `?? -1` fallback preserves prior behavior for items whose ref is not in the DOM query result (indexOf returned -1, sorting them first). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9e6826a to
e5e2fb9
Compare
The whole-repo typos scan flags the "BA"/"ba" typeahead search fixtures in useTypeahead.test.ts as a misspelling of "by"/"be". Allowlist "ba" in extend-words (case-insensitive, covers "BA" too), mirroring the existing "Januar" entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
getItems()sorted registered items into DOM order usingArray.indexOfinside the sort comparator, re-scanningorderedNodeson every comparison — O(n²·log n).getItems()is called from ~23 files on every arrow-key nav, typeahead keystroke, and highlight change (Listbox, Combobox, Select, Menu, Tabs…), so large collections paid hundreds of thousands of scans per keypress.This builds a node→index
Maponce and sorts by lookup — O(n·log n) with two O(n) passes, identical output. The?? -1fallback preserves prior behavior for items whoserefisn't in the DOM query result (detached nodes sort first, exactly asindexOfreturning-1did).Test Plan
Collection.test.tssafety net (test(shared): add characterization tests for core composables and Collection #2694) → 5/5 pass, unchangedsrc/Listbox src/Combobox src/Select src/Menu src/Tabs→ 125/125type-checkexit 0,pnpm lintexit 0git status→ onlyCollection.tsmodified🤖 Generated with Claude Code
Summary by CodeRabbit
Performance Improvements
Chores