feat(frontend): chip-select component + global filter chips in topbar (#344 T1+T2) - #345
Conversation
…ssue #344 T1) First task of UI revamp phase 2 (#344). New `lib/chip-select.ts` component replaces native `<select>` for filter use-cases where we want a compact pill trigger that opens a custom listbox. Component shape: createChipSelect({ label, options, value, onChange, searchThreshold? }) → { root, setValue, getValue, setOptions } Features: - Pill-shaped trigger that mirrors the existing `.filter-group select` visual (so adopting it in T2 is a drop-in replacement). - Click-toggle + keyboard open (Enter / Space / ArrowDown on trigger). - Listbox menu opens below; flips upward when near viewport bottom. - Escape + click-outside both close the menu. - Arrow-key navigation through options; Enter selects the active one. - In-popover search input renders automatically when options.length > 8 (configurable via `searchThreshold`). Filters case-insensitively on label substring. "No matches" hint when filter excludes everything. - ARIA: trigger has `aria-haspopup="listbox"` + `aria-expanded`; menu has `role="listbox"`; options have `role="option"` + `aria-selected`. - All DOM construction via createElement + textContent (no innerHTML), matching the issue #340 XSS posture. NOT touched in this commit (per the plan's T1 → T2 ordering): - Existing `.filter-group select` callers in dashboard.ts / plans.ts / history.ts / recommendations.ts. Those swap over in T2. - Top-bar filter relocation. That's T2. Files: - frontend/src/lib/chip-select.ts (NEW, ~270 lines) - frontend/src/styles/components.css — appended ~110 lines of .chip-select* rules tied to the existing design tokens. - frontend/src/__tests__/chip-select.test.ts (NEW, 10 tests): trigger render, open/close, select fires onChange, Escape close, click-outside close, search-input threshold, case-insensitive filter, empty-result hint, setValue + setOptions roundtrip, ArrowDown keyboard open. Verified: - npm test: 1610/1610 pass (1598 baseline + 12 new tests). - npm run build: clean. Bundle 514 KiB (+0.4% from 512 baseline, well within ±10% gate). - npm test -- --coverage: All-files coverage 80.8% stmts / 82.5% lines (preserved vs 80.76 / 82.40 baseline).
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR introduces a reusable chip-select UI component and refactors provider/account filtering from per-tab DOM selects into a global topbar with reactive state subscriptions. It updates dashboard, history, and plans modules to read filter state from the shared state module instead of direct DOM reads, with comprehensive test coverage for all changes. ChangesGlobal filter chips and topbar integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
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 unit tests (beta)
Comment |
|
@coderabbitai review First of 4 PRs for issue #344. Self-contained new Focus areas:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 19605
1. ARIA —
|
| Concept | Correct attribute |
|---|---|
| Which option is selected | aria-selected="true" on that option |
| Which option has keyboard focus | aria-activedescendant="<option-id>" on trigger (or listbox) |
Minimal fix in renderOptions():
visible.forEach((opt, i) => {
const li = document.createElement('li');
const optId = `chip-select-opt-${cfg.label.toLowerCase().replace(/\s+/g, '-')}-${opt.value || 'empty'}-${i}`;
li.id = optId;
li.setAttribute('role', 'option');
// ...
li.setAttribute('aria-selected', opt.value === currentValue ? 'true' : 'false');
if (i === activeIndex) {
li.classList.add('active');
// set pointer on the trigger (or on the listbox element)
trigger.setAttribute('aria-activedescendant', optId);
}
// ...
});
// Clear when nothing is highlighted
if (activeIndex === -1) trigger.removeAttribute('aria-activedescendant');And in closeMenu():
trigger.removeAttribute('aria-activedescendant');3. Keyboard — ArrowDown double-fires on open
Two separate keydown listeners are registered on trigger. When the menu is closed and the user presses ArrowDown:
- Listener 1 fires →
openMenu()executes →isOpen = true,activeIndexis set to the position ofcurrentValue(say0),renderOptions()called. - Listener 2 fires on the same event →
isOpenis nowtrue→activeIndex = Math.min(0 + 1, …)→ highlight jumps to index 1 instead of staying at 0.
The highlight skips its intended initial position every time ArrowDown opens the menu.
Fix — split the intent: the first listener should only open when closed, and the second should only navigate when already open:
// Listener 1 — open only
trigger.addEventListener('keydown', (ev) => {
if (isOpen) return; // ← guard added
if (ev.key === 'Enter' || ev.key === ' ' || ev.key === 'ArrowDown') {
ev.preventDefault();
openMenu();
}
});
// Listener 2 — navigate only (isOpen guard already present, no change needed)4. Keyboard — Tab does not close the menu
The JSDoc says "Tab on menu → close (and let focus continue)" but there is no Tab handler on the search input (or anywhere else). After the user presses Tab in the search input:
- Focus moves to the next DOM element.
menustays visible (hiddenclass not added).aria-expandedstays"true".- The document-level
mousedown+escListenerremain registered.
Fix — add a keydown guard in the search handler:
search.addEventListener('keydown', (ev) => {
if (ev.key === 'Tab') {
closeMenu(); // don't prevent default — let Tab continue to next element
return;
}
// ... existing arrow/enter handling
});5. ARIA — "No matches" hint needs a valid role inside role="listbox"
const li = document.createElement('li');
li.classList.add('chip-select-empty');
li.textContent = 'No matches';
li.setAttribute('aria-disabled', 'true');
// ← no roleAn element inside a role="listbox" that isn't role="option" (or role="group") is invalid per ARIA ownership rules. Either give it role="option" aria-disabled="true" (making it an inert, announced option) or role="presentation" (purely visual). The former is more informative to AT users.
6. Click-outside timing — looks correct ✅
The mousedown-based outside listener is the right approach. Option selection uses mousedown + ev.preventDefault(), which fires and calls selectByValue → closeMenu before the document listener can act. root.contains(ev.target) correctly filters out intra-component clicks. No race condition observed.
7. Search threshold — defensible ✅
DEFAULT_SEARCH_THRESHOLD = 8 (search appears at 9+ options) is a widely used convention (Material, Headless UI both use 5–10). Checking options.length rather than visibleOptions().length ensures the input doesn't disappear mid-search. The searchThreshold escape hatch in the config is a good call.
Summary table
| # | Area | Severity | Quick fix |
|---|---|---|---|
| 1 | role="option" not owned by role="listbox" |
🔴 High | role="presentation" on ul or move role="listbox" to ul |
| 2 | Missing aria-activedescendant + option ids |
🔴 High | Generate ids in renderOptions, set attribute on trigger |
| 3 | ArrowDown double-fires on open | 🟠 Medium | Guard listener 1 with if (isOpen) return |
| 4 | Tab doesn't close menu | 🟠 Medium | Add Tab guard in search.keydown |
| 5 | "No matches" li has no role |
🟡 Low | Add role="option" aria-disabled="true" |
| 6 | Click-outside mousedown timing |
✅ OK | — |
| 7 | Search threshold | ✅ OK | — |
Relocate the Provider + Account filters from per-section <select> dropdowns (Home / Plans / Purchases) into a single pair of chip-selects in the global topbar. The page-level controls had drifted into three near-identical copies, and changing scope meant clicking the same filter on each tab in turn — the topbar now owns one source of truth. Each section subscribes to state.subscribeProvider / subscribeAccount and reloads itself when the global filter changes. Issue #185's ordering invariant (clear account state BEFORE awaiting the account-list refetch on provider change) moves from dashboard.ts to topbar-filters.ts so it stays at the mutation site. - new: frontend/src/topbar-filters.ts (initTopbarFilters + chip wiring) - new: subscribeProvider / subscribeAccount in state.ts (fire on change) - removed: per-section #dashboard-/plans-/history-{provider,account}-filter - new: topbar-filters.test.ts (Issue #185 regression coverage) Tests: 1609 passed / 1 skipped (dashboard's Issue #185 cases moved to topbar-filters.test.ts). Coverage 80.76% stmts / 82.54% lines (parity with the post-#343 baseline). Build 519 KiB (within ±10% of 512).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
pre-commit CI run 25790874399 passed on dc6ed40. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
frontend/src/__tests__/chip-select.test.ts (1)
217-230: ⚡ Quick winAdd keyboard regression tests for open-state navigation and Tab close.
Please add focused tests for: (1) ArrowDown opening without advancing past initial active option, and (2) Tab from
.chip-select-searchclosing the menu while allowing focus to proceed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/chip-select.test.ts` around lines 217 - 230, Add two focused keyboard regression tests in the chip-select test suite using createChipSelect: (1) test that pressing ArrowDown on the trigger opens the menu but does not advance the active option from the initial selection — query the trigger ('.chip-select'), open the menu via KeyboardEvent('keydown',{key:'ArrowDown',bubbles:true}), then assert the menu is visible and that the initially active option (inspect elements under '.chip-select-menu' or the option with an 'active' class/aria-selected) remains unchanged; (2) test that pressing Tab while focus is in the search input ('.chip-select-search') closes the menu but allows focus to move forward — focus the search element, open the menu, dispatch KeyboardEvent('keydown',{key:'Tab',bubbles:true}), then assert the menu is hidden and that document.activeElement is the next focusable element (or not the search input). Use createChipSelect to build the component and existing query selectors ('.chip-select', '.chip-select-menu', '.chip-select-search') to locate elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/history.ts`:
- Around line 70-72: setupHistoryHandlers currently calls
state.subscribeProvider and state.subscribeAccount each time without removing
previous listeners, causing duplicate loadHistory() calls; make these
subscriptions idempotent by storing the unsubscribe/cleanup functions returned
by state.subscribeProvider and state.subscribeAccount (or by tracking a boolean
flag) on the module scope and calling them before creating new subscriptions
inside setupHistoryHandlers, then subscribe with wrappers that call
loadHistory(); reference setupHistoryHandlers, state.subscribeProvider,
state.subscribeAccount, and loadHistory when locating and updating the
implementation.
In `@frontend/src/lib/chip-select.ts`:
- Around line 270-275: The ArrowDown key is being handled twice: the trigger
keydown listener calls openMenu() and another keydown listener immediately
advances activeIndex; to prevent skipping the initial highlight, when handling
ArrowDown in the trigger keydown handler (the handler that calls openMenu()),
after calling openMenu() call ev.stopImmediatePropagation() (and keep
ev.preventDefault()) so the subsequent keydown listener won't run; apply the
same change to the other symmetric trigger keydown block around the 303-309
region so both places stop immediate propagation for ArrowDown.
- Around line 283-298: In the search keydown handler (the listener attached with
search.addEventListener('keydown', ...)) add handling for the 'Tab' key so the
menu closes and normal focus movement is preserved: detect ev.key === 'Tab', do
NOT call ev.preventDefault(), and invoke the component's menu-close logic (e.g.,
call the existing close function or set the open state to false — whatever the
component uses to hide options) instead of letting the handler ignore it; keep
the rest of ArrowUp/ArrowDown/Enter behavior unchanged (references: activeIndex,
visibleOptions(), renderOptions(), selectByValue()).
- Around line 81-87: The trigger button created as "trigger" needs ARIA wiring:
assign a stable id to the listbox element and set
trigger.setAttribute('aria-controls', listboxId'); generate stable ids for each
option (e.g., `${baseId}-option-${index}`) when creating option elements, and
whenever the menu is opened or activeIndex changes update
trigger.setAttribute('aria-activedescendant', idOfActiveOption) or remove it
when closed; ensure the code paths that change activeIndex (keyboard handlers,
open/close logic) also call the update so assistive tech can see the highlighted
option.
- Around line 114-129: Move the listbox semantics from the wrapper div to the
actual options container: remove menu.setAttribute('role', 'listbox') and
instead call optionsList.setAttribute('role', 'listbox') while preserving the
aria-label (use cfg.label) on the listbox; also ensure the empty-state "No
matches" row created later is given role="option" (and tabindex/-selection
handling if applicable) so it is a valid child of the listbox. Locate the
variables menu and optionsList in chip-select.ts and update their attribute
calls accordingly, and update the code that renders the no-results row to add
role="option".
In `@frontend/src/plans.ts`:
- Around line 1116-1118: The setupPlanHandlers function currently calls
state.subscribeProvider and state.subscribeAccount each time without keeping
unsubscribe handles, causing duplicate listeners and multiple loadPlans()
invocations on re-entry; update setupPlanHandlers to store the unsubscribe
callbacks returned by state.subscribeProvider and state.subscribeAccount (or
keep them in a module-scope variable) and call those unsubscribe functions
before adding new subscriptions, or make setupPlanHandlers idempotent by
checking if subscriptions already exist and skipping re-subscription, ensuring
loadPlans() is only invoked once per actual state change.
In `@frontend/src/topbar-filters.ts`:
- Around line 38-57: populateAccountOptions can apply stale async responses when
multiple api.listAccounts calls overlap; add a request-sequencing guard:
introduce a module-scoped counter (e.g., let accountRequestCounter = 0),
increment it at the start of populateAccountOptions and capture its value in a
local const (e.g., const req = ++accountRequestCounter), then after awaiting
api.listAccounts(check) return early unless req === accountRequestCounter before
calling accountChip.setOptions; apply the same pattern to the other places in
this file that call api.listAccounts (the other populate/refresh functions
referencing accountChip).
---
Nitpick comments:
In `@frontend/src/__tests__/chip-select.test.ts`:
- Around line 217-230: Add two focused keyboard regression tests in the
chip-select test suite using createChipSelect: (1) test that pressing ArrowDown
on the trigger opens the menu but does not advance the active option from the
initial selection — query the trigger ('.chip-select'), open the menu via
KeyboardEvent('keydown',{key:'ArrowDown',bubbles:true}), then assert the menu is
visible and that the initially active option (inspect elements under
'.chip-select-menu' or the option with an 'active' class/aria-selected) remains
unchanged; (2) test that pressing Tab while focus is in the search input
('.chip-select-search') closes the menu but allows focus to move forward — focus
the search element, open the menu, dispatch
KeyboardEvent('keydown',{key:'Tab',bubbles:true}), then assert the menu is
hidden and that document.activeElement is the next focusable element (or not the
search input). Use createChipSelect to build the component and existing query
selectors ('.chip-select', '.chip-select-menu', '.chip-select-search') to locate
elements.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cc2c547-b8c9-4d51-83fa-ae7a158cd19b
📒 Files selected for processing (19)
frontend/src/__tests__/chip-select.test.tsfrontend/src/__tests__/dashboard.test.tsfrontend/src/__tests__/history-approve-button.test.tsfrontend/src/__tests__/history-cancel-button.test.tsfrontend/src/__tests__/history-retry-button.test.tsfrontend/src/__tests__/history.test.tsfrontend/src/__tests__/html.test.tsfrontend/src/__tests__/plans.test.tsfrontend/src/__tests__/topbar-filters.test.tsfrontend/src/dashboard.tsfrontend/src/history.tsfrontend/src/index.htmlfrontend/src/index.tsfrontend/src/lib/chip-select.tsfrontend/src/plans.tsfrontend/src/state.tsfrontend/src/styles/components.cssfrontend/src/styles/layout.cssfrontend/src/topbar-filters.ts
| export function setupHistoryHandlers(): void { | ||
| const providerFilter = document.getElementById('history-provider-filter') as HTMLSelectElement | null; | ||
| if (providerFilter) { | ||
| providerFilter.addEventListener('change', () => { | ||
| void populateHistoryAccountFilter(providerFilter.value); | ||
| }); | ||
| } | ||
| void populateHistoryAccountFilter(); | ||
| state.subscribeProvider(() => void loadHistory()); | ||
| state.subscribeAccount(() => void loadHistory()); |
There was a problem hiding this comment.
Make history filter subscriptions idempotent to avoid duplicate reloads.
Line 71 and Line 72 register new subscribers every call and never clean up old ones. If setupHistoryHandlers() is called again, each filter change can trigger multiple loadHistory() runs.
Suggested fix
+let unsubscribeHistoryProvider: (() => void) | null = null;
+let unsubscribeHistoryAccount: (() => void) | null = null;
+
export function setupHistoryHandlers(): void {
- state.subscribeProvider(() => void loadHistory());
- state.subscribeAccount(() => void loadHistory());
+ unsubscribeHistoryProvider?.();
+ unsubscribeHistoryAccount?.();
+ unsubscribeHistoryProvider = state.subscribeProvider(() => void loadHistory());
+ unsubscribeHistoryAccount = state.subscribeAccount(() => void loadHistory());
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/history.ts` around lines 70 - 72, setupHistoryHandlers currently
calls state.subscribeProvider and state.subscribeAccount each time without
removing previous listeners, causing duplicate loadHistory() calls; make these
subscriptions idempotent by storing the unsubscribe/cleanup functions returned
by state.subscribeProvider and state.subscribeAccount (or by tracking a boolean
flag) on the module scope and calling them before creating new subscriptions
inside setupHistoryHandlers, then subscribe with wrappers that call
loadHistory(); reference setupHistoryHandlers, state.subscribeProvider,
state.subscribeAccount, and loadHistory when locating and updating the
implementation.
| const trigger = document.createElement('button'); | ||
| trigger.type = 'button'; | ||
| trigger.classList.add('chip-select'); | ||
| trigger.setAttribute('aria-haspopup', 'listbox'); | ||
| trigger.setAttribute('aria-expanded', 'false'); | ||
| trigger.setAttribute('aria-label', cfg.label); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "chip-select.ts" --exec cat -n {} \;Repository: LeanerCloud/CUDly
Length of output: 13773
Add aria-activedescendant wiring for keyboard-highlighted option.
The highlighted option (activeIndex) changes visually but is invisible to assistive technology. Add stable option IDs and sync aria-activedescendant on the trigger while the menu is open and whenever keyboard navigation changes the active index. Also link the trigger to the listbox via aria-controls.
Suggested patch
export function createChipSelect(cfg: ChipSelectConfig): ChipSelectHandle {
+ const instanceId = `chip-select-${Math.random().toString(36).slice(2)}`;
@@
trigger.setAttribute('aria-haspopup', 'listbox');
trigger.setAttribute('aria-expanded', 'false');
trigger.setAttribute('aria-label', cfg.label);
+ trigger.setAttribute('aria-controls', `${instanceId}-listbox`);
@@
optionsList.classList.add('chip-select-options');
+ optionsList.id = `${instanceId}-listbox`;
@@
visible.forEach((opt, i) => {
const li = document.createElement('li');
+ li.id = `${instanceId}-opt-${i}`;
@@
if (i === activeIndex) li.classList.add('active');
@@
});
+ const activeEl = activeIndex >= 0
+ ? optionsList.querySelectorAll<HTMLElement>('.chip-select-option')[activeIndex]
+ : null;
+ if (activeEl) trigger.setAttribute('aria-activedescendant', activeEl.id);
+ else trigger.removeAttribute('aria-activedescendant');
}
@@
function closeMenu(): void {
@@
trigger.setAttribute('aria-expanded', 'false');
+ trigger.removeAttribute('aria-activedescendant');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/lib/chip-select.ts` around lines 81 - 87, The trigger button
created as "trigger" needs ARIA wiring: assign a stable id to the listbox
element and set trigger.setAttribute('aria-controls', listboxId'); generate
stable ids for each option (e.g., `${baseId}-option-${index}`) when creating
option elements, and whenever the menu is opened or activeIndex changes update
trigger.setAttribute('aria-activedescendant', idOfActiveOption) or remove it
when closed; ensure the code paths that change activeIndex (keyboard handlers,
open/close logic) also call the update so assistive tech can see the highlighted
option.
| export function setupPlanHandlers(): void { | ||
| const plansProviderFilter = document.getElementById('plans-provider-filter') as HTMLSelectElement | null; | ||
| if (plansProviderFilter) { | ||
| plansProviderFilter.addEventListener('change', () => { | ||
| void populatePlansAccountFilter(plansProviderFilter.value); | ||
| void loadPlans(); | ||
| }); | ||
| } | ||
|
|
||
| const plansAccountFilter = document.getElementById('plans-account-filter') as HTMLSelectElement | null; | ||
| if (plansAccountFilter) { | ||
| plansAccountFilter.addEventListener('change', () => void loadPlans()); | ||
| } | ||
|
|
||
| void populatePlansAccountFilter(); | ||
| state.subscribeProvider(() => void loadPlans()); | ||
| state.subscribeAccount(() => void loadPlans()); |
There was a problem hiding this comment.
Prevent duplicate provider/account subscriptions in repeated setup calls.
Line 1117 and Line 1118 add subscriptions without storing/cleaning unsubscribe callbacks. Re-entering setupPlanHandlers() will stack listeners and multiply loadPlans() calls per filter change.
Suggested fix
+let unsubscribePlanProvider: (() => void) | null = null;
+let unsubscribePlanAccount: (() => void) | null = null;
+
export function setupPlanHandlers(): void {
- state.subscribeProvider(() => void loadPlans());
- state.subscribeAccount(() => void loadPlans());
+ unsubscribePlanProvider?.();
+ unsubscribePlanAccount?.();
+ unsubscribePlanProvider = state.subscribeProvider(() => void loadPlans());
+ unsubscribePlanAccount = state.subscribeAccount(() => void loadPlans());
const providerSelect = document.getElementById('plan-provider') as HTMLSelectElement | null;
const serviceSelect = document.getElementById('plan-service') as HTMLSelectElement | null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/plans.ts` around lines 1116 - 1118, The setupPlanHandlers
function currently calls state.subscribeProvider and state.subscribeAccount each
time without keeping unsubscribe handles, causing duplicate listeners and
multiple loadPlans() invocations on re-entry; update setupPlanHandlers to store
the unsubscribe callbacks returned by state.subscribeProvider and
state.subscribeAccount (or keep them in a module-scope variable) and call those
unsubscribe functions before adding new subscriptions, or make setupPlanHandlers
idempotent by checking if subscriptions already exist and skipping
re-subscription, ensuring loadPlans() is only invoked once per actual state
change.
| async function populateAccountOptions(provider: string): Promise<void> { | ||
| if (!accountChip) return; | ||
| try { | ||
| const filter = | ||
| provider && provider !== '' && provider !== 'all' | ||
| ? { provider: provider as 'aws' | 'azure' | 'gcp' } | ||
| : undefined; | ||
| const accounts = await api.listAccounts(filter); | ||
| const options: ChipSelectOption[] = [ | ||
| { value: '', label: 'All Accounts' }, | ||
| ...accounts.map((a) => ({ | ||
| value: a.id, | ||
| label: `${a.name} (${a.external_id})`, | ||
| })), | ||
| ]; | ||
| accountChip.setOptions(options); | ||
| } catch (err) { | ||
| console.warn('topbar-filters: failed to list accounts for chip:', err); | ||
| accountChip.setOptions([{ value: '', label: 'All Accounts' }]); | ||
| } |
There was a problem hiding this comment.
Guard against stale async account-list responses.
Out-of-order listAccounts responses can overwrite the account chip with options for an older provider after the user already switched again.
Suggested patch
let providerChip: ChipSelectHandle | null = null;
let accountChip: ChipSelectHandle | null = null;
+let accountReqSeq = 0;
@@
async function populateAccountOptions(provider: string): Promise<void> {
if (!accountChip) return;
+ const reqSeq = ++accountReqSeq;
try {
@@
const accounts = await api.listAccounts(filter);
+ if (reqSeq !== accountReqSeq) return;
@@
accountChip.setOptions(options);
} catch (err) {
+ if (reqSeq !== accountReqSeq) return;
console.warn('topbar-filters: failed to list accounts for chip:', err);
accountChip.setOptions([{ value: '', label: 'All Accounts' }]);
}
}Also applies to: 92-94, 113-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/topbar-filters.ts` around lines 38 - 57, populateAccountOptions
can apply stale async responses when multiple api.listAccounts calls overlap;
add a request-sequencing guard: introduce a module-scoped counter (e.g., let
accountRequestCounter = 0), increment it at the start of populateAccountOptions
and capture its value in a local const (e.g., const req =
++accountRequestCounter), then after awaiting api.listAccounts(check) return
early unless req === accountRequestCounter before calling
accountChip.setOptions; apply the same pattern to the other places in this file
that call api.listAccounts (the other populate/refresh functions referencing
accountChip).
…ble-fire Address all five actionable items from the CodeRabbit review of PR #345: 1. ARIA ownership (High): move role="listbox" from the outer div to the ul so role="option" elements are directly owned by the listbox, per WAI-ARIA §6.6.12. 2. aria-activedescendant (High): generate stable ids for each option li and set aria-activedescendant on the trigger while the menu is open, pointing at the highlighted option. Clear it in closeMenu(). 3. ArrowDown double-fire (Medium): merge the two separate keydown listeners on the trigger into a single handler that captures wasOpen before any mutation, making open and navigate mutually exclusive for the same event. 4. Tab does not close menu (Medium): add a Tab guard in the search input keydown handler; closeMenu() without preventDefault so focus continues. 5. "No matches" role (Low): add role="option" + aria-selected="false" to the empty-result hint so it is a valid child of role="listbox". Add six regression tests covering all five fixes. Items 6 (click-outside timing) and 7 (search threshold) were confirmed correct by CodeRabbit and required no changes.
CodeRabbit review — pass 1 addressed (commit e5d253d)All five actionable items from the CR review have been fixed: Actionable fixes (commit
Confirmed no-action (per CR):
Coverage: 80.74% stmts / 82.51% lines (baseline: 80.76% / 82.40% — flat/improved). Six regression tests added for all five fixes. @coderabbitai review |
🧩 Analysis chainLet me read the updated implementation before re-reviewing. 🏁 Script executed: Length of output: 25214
The One minor observation for future hardening (not a blocker): the ✅ Pass 2 — nothing blocking merge. ✅ Actions performedReview triggered.
|
Address two actionable findings from the CodeRabbit pass-2 review: 1. plans.ts — setupPlanHandlers was adding new subscribeProvider / subscribeAccount listeners on every call without cleaning up the prior ones, so calling it more than once (tests, HMR) stacks duplicate loadPlans() invocations. Store the unsubscribe handles at module scope (_unsubProvider / _unsubAccount) and call them before re-registering. 2. topbar-filters.ts — populateAccountOptions had no guard against overlapping async api.listAccounts calls; a slower earlier response could overwrite the options set by a newer, faster response. Introduce a monotonic generation counter (_accountRequestGen); each call captures its generation before the await and discards the result if a newer call has already started.
CodeRabbit review — pass 2 addressed (commit 605240d)Actionable fixes:
Nitpick (already done in pass 1): ArrowDown double-fire and Tab regression tests were added in commit e5d253d. Coverage: 80.73% stmts / 82.51% lines (baseline: 80.76% / 82.40%). @coderabbitai review |
CR clean - ready for human reviewCodeRabbit pass 3 came back clean (no blocking issues). All actionable items from both review passes have been addressed across two fix commits:
Coverage: 80.73% stmts / 82.51% lines. 45 test suites green. |
🧩 Analysis chain🏁 Script executed: Length of output: 102 🏁 Script executed: Length of output: 53723
Two small observations, neither blocking:
✅ Pass 3 — nothing blocking merge. ✅ Actions performedReview triggered.
|
Summary
Issue #344 (UI revamp phase 2) — this PR bundles tasks T1 + T2 of the original 4-task plan:
chip-selectcomponent (custom popover listbox).<select>dropdowns into a single pair ofchip-selectchips in the global topbar.T3 (loading skeletons) and T4 (Plan-builder drawer) land in follow-up PRs against this branch.
T1 — chip-select component
frontend/src/lib/chip-select.ts— pill trigger + custom popover listbox. API:Behaviour: click-toggle / keyboard open (Enter / Space / ArrowDown), Esc + click-outside both close, arrow-key option navigation, in-popover search above 8 options, menu flips upward near viewport bottom, full ARIA (
aria-haspopup="listbox",aria-expanded,role="listbox"+role="option"+aria-selected), DOM viacreateElement+textContentonly (noinnerHTML).T2 — global filter chips in topbar
frontend/src/topbar-filters.tsmounts two chip-selects (Provider + Account) into a new#topbar-filtersslot in the topbar. Each section subscribes to filter changes viastate.subscribeProvider/state.subscribeAccountand reloads itself — no more three near-identical copies of the same filter on three different tabs.The provider/account
<select>dropdowns are deleted from#dashboard-controls,#plans-header, and#history-controlsinindex.html.dashboard.ts,plans.ts, andhistory.tslose their DOM-binding /populateAccountFiltercalls and gain a singlestate.subscribe*registration each. Issue #185's ordering invariant (clearstate.currentAccountIDsBEFORE awaiting the account-list refetch on provider change) moves fromdashboard.tstotopbar-filters.tsso it stays at the mutation site — regression test moves with it (__tests__/topbar-filters.test.ts).Verification
topbar-filters.test.ts).Not in this PR
Both will be stacked on top of this branch in follow-up PRs.
Summary by CodeRabbit
New Features
Tests