Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change keeps labels and separators in place while sorting matching options within their sections, with coverage for ordering and empty groups. No merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
commit: |
| // Sorting `result` itself would pull every structural item to the front, | ||
| // so only the matches are reordered and they refill the slots they held. | ||
| const matches = result.filter(({ score }) => score !== -1).sort((a, b) => a.score - b.score) | ||
| let index = 0 | ||
| return result.map(({ item, score }) => score === -1 ? item : matches[index++]!.item) |
There was a problem hiding this comment.
Sorting the whole group lets items cross a label or separator in a flat array, e.g. Aubergine ends up under Fruits when searching a. Sorting each run on its own avoids that:
| // Sorting `result` itself would pull every structural item to the front, | |
| // so only the matches are reordered and they refill the slots they held. | |
| const matches = result.filter(({ score }) => score !== -1).sort((a, b) => a.score - b.score) | |
| let index = 0 | |
| return result.map(({ item, score }) => score === -1 ? item : matches[index++]!.item) | |
| // Sort each run of matches on its own so items never cross a label or separator. | |
| let start = 0 | |
| for (let i = 0; i <= result.length; i++) { | |
| if (i === result.length || result[i]!.score === -1) { | |
| result.splice(start, i - start, ...result.slice(start, i).sort((a, b) => a.score - b.score)) | |
| start = i + 1 | |
| } | |
| } | |
| return result.map(({ item }) => item) |
There was a problem hiding this comment.
Applied your suggestion. You are right that the slot-refilling version let items cross a boundary: with [Fruits, 'Banana', Vegetables, 'Aubergine'] and a, it returned Aubergine under Fruits and Banana under Vegetables.
Added a test for exactly that case. It is the one the previous version failed while the other three stayed green, so the suite now pins the boundary rather than just the position of the labels.
One note while checking the loop: start = i behaves identically to start = i + 1, because a structural item scores -1, which is below every value score() returns (0, 1, 2), and sort is stable, so it stays at the front of the run either way. I kept your i + 1 since it says the intent. Full suite is 7452 passing, 6 skipped, and lint and typecheck are clean.
0976abf to
a9d68ac
Compare
This branch was successfully deployed
2 participants
🔗 Linked issue
None. Found while auditing
filterGroups.❓ Type of change
📚 Description
filterGroupsgives labels and separators the sentinel score-1, then sorts the whole group by score, so every structural item lands ahead of every match. Its own docstring promises the opposite: "Structural items (labels, separators) are kept at their relative position."A separator inside a flat
itemsarray is documented usage, and typing moves the rule to the top of the menu:Through SelectMenu, InputMenu, Listbox or DropdownMenu that renders a separator above the first option.
Only the matches are sorted now, and structural items keep the slots they hold in the filtered group. A leading label still comes out first because it was already first, so grouped items are unaffected.
One case I left alone: a separator whose surrounding items are all filtered out still renders.
filterGroups([['Apple', { type: 'separator' }, 'Carrot']], 'carrot', ...)returns[{ type: 'separator' }, 'Carrot']. Deciding when a separator has become redundant is a separate behaviour change.test/composables/useFilter.spec.tsis new and covers the ordering.📝 Checklist