fix(search): snappier, keyboard-navigable In/From filter submenu#2643
Merged
gbirman merged 15 commits intoApr 17, 2026
Merged
Conversation
This reverts commit 93eebc4.
Replaces the custom list + manual keyboard handler in SearchableFilterSubmenu with a shared Combobox-based component, matching the UX of the In/From chips. Fixes laggy/flaky arrow navigation by delegating keyboard nav, scroll-into-view, and multi-select state to Kobalte's Combobox. The DropdownMenu.Sub wrapper is kept so hover-open, arrow nav between In and From, and Left-to-close still work natively. Focus on the search input is pinned deterministically via onFocusIn redirect.
…tart Previously only closed if the search field was empty. Now it also closes when the caret is at position 0, even with search text present, matching standard menu-submenu behavior.
Swap the non-virtualized Kobalte Combobox.Listbox for a virtua/solid Virtualizer inside Combobox.Listbox so only visible rows render. Wire up scrollToItem so Kobalte's keyboard nav still scrolls the highlighted row into view. Precompute lowercased labels so per-keystroke filtering doesn't toLowerCase every option.
gbirman
force-pushed
the
macro-cz9w4xGnpcUj9ynWJ6cVm-fix-broken-updown-arrow-keys-in-search-menu-filter
branch
from
8566ce9 to
bb2f727
Compare
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughReplaced inline searchable multi-select implementations with a shared Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app/packages/app/component/next-soup/soup-view/filters-bar/active-filter-chips.tsx (1)
310-385: 🧹 Nitpick | 🔵 TrivialDedupe the searchable-vs-non-searchable chip selection.
Lines 310-341 and 343-384 duplicate the same
Show when={filter.searchableOptions}branch — first standalone, then wrapped in a span that tacks on theClear allbutton for the last chip. Extract a small helper (renderChip(filter)) that returns either<SearchableFilterChip>or<FilterChip>once, then reuse it in both the last-chip and non-last-chip paths:♻️ Sketch
const renderChip = (filter: ActiveFilter) => ( <Show when={filter.searchableOptions} fallback={ <FilterChip filter={filter} onRemove={() => props.onRemove(filter.optionId)} onReplace={(newOptionId) => props.onReplace(filter.optionId, newOptionId)} isOptionActive={props.isOptionActive} chipClass={props.chipClass} hideCategoryLabel={props.hideCategoryLabel} /> } > <SearchableFilterChip filter={filter} onRemove={() => filter.onRemove ? filter.onRemove() : props.onRemove(filter.optionId) } chipClass={props.chipClass} hideCategoryLabel={props.hideCategoryLabel} /> </Show> );Then the outer
<Show>just decides whether to wrap the singlerenderChip(filter)in a<span>with theClear allbutton.As per coding guidelines: "Eliminate redundant code and abstractions" / "Consolidate related logic into cohesive functions or code blocks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/next-soup/soup-view/filters-bar/active-filter-chips.tsx` around lines 310 - 385, The duplicated Show branch for searchable vs non-searchable chips should be extracted into a small helper function (e.g., renderChip(filter: ActiveFilter)) that returns the Show(...) which chooses between SearchableFilterChip and FilterChip (preserving props: onRemove logic, onReplace, isOptionActive, chipClass, hideCategoryLabel). Replace both occurrences inside the outer Show (the standalone branch and the span-wrapped branch that includes the Clear all Button) with a call to renderChip(filter) so the Clear all button remains only in the last-chip path while the chip rendering logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@js/app/packages/app/component/next-soup/soup-view/filters-bar/searchable-multi-select.tsx`:
- Around line 234-236: The createEffect that watches props.isOpen and calls
setSearchQuery('') should be removed because it improperly derives internal
signal state from a prop; instead clear the searchQuery at the submenu close
handlers (e.g., call setSearchQuery('') from the component/method that
implements onRequestClose or the subcomponent's onOpenChange) or bind the
input's value to props.isOpen so the input is keyed by the open state; locate
the createEffect block referencing props.isOpen and setSearchQuery and move the
reset logic into the onRequestClose/onOpenChange handler or change the input
value to depend on props.isOpen/searchQuery accordingly.
- Around line 185-197: The empty-state fallback currently always prints No
options match "{searchQuery()}" and thus shows No options match "" when the
source options list is empty; update the Show fallback logic around
filteredOptions() so it branches on searchQuery().trim(): if
searchQuery().trim() === "" show a different message like "No options available"
(or similar) when the original options() list is empty, otherwise keep the "No
options match \"{searchQuery()}\"" message for a non-empty query that filtered
everything out; apply the same change to the inline variant referenced in the
file (the other Show fallback at the later block) and use the existing symbols
filteredOptions(), searchQuery(), Show and VirtualizedListbox to locate and
modify the code.
- Around line 165-183: The component renders two Combobox.Inputs causing
ref/focus conflicts; update SearchableMultiSelect to use exactly one input
inside Combobox.Control by removing the sr-only Combobox.Input currently nested
in Combobox.Control and moving the visible search input (the Combobox.Input
currently inside Combobox.Content paired with SearchIcon and
placeholder={props.placeholder}) into Combobox.Control (or conversely ensure the
visible input is the only input and remove the hidden one), keeping
props.children placement and the search input's classes/placeholder so
filtering, keyboard navigation and focus are handled by a single Combobox.Input
instance.
In
`@js/app/packages/app/component/next-soup/soup-view/filters-bar/unified-filter-dropdown.tsx`:
- Around line 450-452: The small wrapper function setAssigneeIds simply forwards
its argument to setAssigneeFilter; remove setAssigneeIds and wire
setAssigneeFilter directly where setAssigneeIds is passed (e.g., as the onChange
handler in the dropdown), and also remove the other identical wrapper occurrence
around setAssigneeFilter (the one mentioned near the second occurrence) so
callers use setAssigneeFilter directly.
- Around line 352-361: The onFocusIn handler on DropdownMenu.SubContent can miss
the input because inputRef may not be set yet; update the handler in
unified-filter-dropdown.tsx to, when e.target === e.currentTarget, defer
focusing the SearchableMultiSelectInline input via a timing fallback (e.g.,
double requestAnimationFrame or setTimeout 0 then focus) that checks inputRef
and calls inputRef.focus() once available; keep the immediate focus attempt but
add the deferred attempts so arrow-key navigation lands in the combobox input
reliably on first open.
---
Outside diff comments:
In
`@js/app/packages/app/component/next-soup/soup-view/filters-bar/active-filter-chips.tsx`:
- Around line 310-385: The duplicated Show branch for searchable vs
non-searchable chips should be extracted into a small helper function (e.g.,
renderChip(filter: ActiveFilter)) that returns the Show(...) which chooses
between SearchableFilterChip and FilterChip (preserving props: onRemove logic,
onReplace, isOptionActive, chipClass, hideCategoryLabel). Replace both
occurrences inside the outer Show (the standalone branch and the span-wrapped
branch that includes the Clear all Button) with a call to renderChip(filter) so
the Clear all button remains only in the last-chip path while the chip rendering
logic is centralized.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9218db2b-01f7-4078-8e8b-266eee095d46
📒 Files selected for processing (3)
js/app/packages/app/component/next-soup/soup-view/filters-bar/active-filter-chips.tsxjs/app/packages/app/component/next-soup/soup-view/filters-bar/searchable-multi-select.tsxjs/app/packages/app/component/next-soup/soup-view/filters-bar/unified-filter-dropdown.tsx
Comment on lines
+165
to
+183
| <Combobox.Control class="flex"> | ||
| {props.children} | ||
| <Combobox.Input class="sr-only" /> | ||
| </Combobox.Control> | ||
|
|
||
| <Combobox.Portal> | ||
| <Combobox.Content | ||
| class={cn( | ||
| 'z-action-menu bg-surface-0 border border-edge-muted rounded-sm shadow-md w-[260px] max-w-[90vw] overflow-hidden', | ||
| props.contentClass | ||
| )} | ||
| > | ||
| <div class="flex items-center gap-2 px-3 py-2 border-b border-edge-muted"> | ||
| <SearchIcon class="size-3.5 text-ink-muted shrink-0" /> | ||
| <Combobox.Input | ||
| class="flex-1 min-w-0 text-xs bg-transparent outline-none caret-accent placeholder:text-ink-faint" | ||
| placeholder={props.placeholder ?? 'Search...'} | ||
| /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Kobalte Combobox support rendering two Combobox.Input components — one inside Combobox.Control and another inside Combobox.Content?
💡 Result:
No, Kobalte Combobox does not support rendering two Combobox.Input components—one inside Combobox.Control and another inside Combobox.Content. The official documentation specifies a single Combobox.Input within Combobox.Control for filtering options and reflecting the selected value, with Combobox.Content containing only Combobox.Listbox for options. All examples and anatomy diagrams show exactly one Input in Control. No sources mention or demonstrate multiple Inputs, and the component's design follows standard ARIA combobox patterns with a single input element.
Citations:
- 1: https://kobalte.dev/docs/core/components/combobox/
- 2: [Combobox] ctrl+a in text input replaces selection when
multiple=truekobaltedev/kobalte#499
Remove the sr-only Combobox.Input inside Combobox.Control — Kobalte Combobox only supports a single input.
SearchableMultiSelect renders two Combobox.Input instances (line 167 and lines 179–182), but Kobalte Combobox's design requires exactly one input inside Combobox.Control. The visible input in Combobox.Content violates the component architecture. This can cause ref collisions, broken keyboard navigation, and unpredictable focus behavior.
Hoist the search field into Combobox.Control as the sole input, or remove the sr-only input and rely on the visible input for filtering and accessibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/app/component/next-soup/soup-view/filters-bar/searchable-multi-select.tsx`
around lines 165 - 183, The component renders two Combobox.Inputs causing
ref/focus conflicts; update SearchableMultiSelect to use exactly one input
inside Combobox.Control by removing the sr-only Combobox.Input currently nested
in Combobox.Control and moving the visible search input (the Combobox.Input
currently inside Combobox.Content paired with SearchIcon and
placeholder={props.placeholder}) into Combobox.Control (or conversely ensure the
visible input is the only input and remove the hidden one), keeping
props.children placement and the search input's classes/placeholder so
filtering, keyboard navigation and focus are handled by a single Combobox.Input
instance.
The activeFilters memo was rebuilding ActiveFilter objects on every run, so <For>'s reference-based diff treated each selection toggle as a new item and remounted the chip — taking its Combobox (open state, search text) down with it. The previous fix papered over this with a module-level Map that cached open state across remounts. Now ActiveFilter.optionId and optionLabel are accessors so values can update reactively, and activeFilters caches chip objects by a stable key (categoryLabel[+optionId]). Same reference across runs means <For> keeps the chip mounted, and the Combobox's internal state survives naturally — no persistent-state Map needed.
Previously we pre-filtered the options list before passing it to Kobalte's Combobox. When a user typed to narrow the list, Kobalte's internal `allOptions` only contained the visible slice — so its `getOptionsFromValues` dropped any selected key not in that slice. Selecting "Gabriel Birman" while filtered to "gab" silently removed "macro" from the selection. Now we pass the full options list to Combobox and let Kobalte's built-in `contains` filter narrow the visible items via `inputValue`. Our own searchQuery is kept only for the "no options match" fallback message.
…ty selection Two issues in the active-filter chips: 1. After reload, the In/From chip labels showed raw uuids instead of display names because the label lookup used quickAccess.getById — a plain-Map read that isn't reactive. The accessor was built once and never re-ran when senders/channels finished loading. Now label lookup derives from the reactive channelOptions/senderOptions accessors, so the accessor re-runs and the label updates to the display name as soon as data is available. 2. Toggling off the last selection in a searchable chip immediately hid the chip (and with it, the open popup), so a user mid-way through swapping A→B would lose the menu. Added an isPopupOpen signal to each chip that owns one; activeFilters keeps the chip in the output array while either the selection is non-empty OR the popup is still open. The chip is only evicted once the user explicitly closes the popup.
…e closes the other
…ering one closes the other" This reverts commit 4f65333.
…vering one closes the other" This reverts commit 9290c7b.
…balte grace polygon
… + trivial wrapper
gbirman
deleted the
macro-cz9w4xGnpcUj9ynWJ6cVm-fix-broken-updown-arrow-keys-in-search-menu-filter
branch
This was referenced Apr 24, 2026
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.
Clean up search filter components