Skip to content

fix(ui): preserve search selection when scope narrows - #4308

Merged
yannickmonney merged 1 commit into
mainfrom
fix/search-selection-after-filter
Oct 4, 2026
Merged

yannickmonney merged 1 commit into
mainfrom
fix/search-selection-after-filter

Conversation

@yannickmonney

Copy link
Copy Markdown
Contributor

What changed

  • Clamp the active search row when a scope/source update removes results.
  • Keep aria-activedescendant and Enter aligned with the remaining visible option.
  • Add a regression covering Everything → Chats narrowing after keyboard navigation.

Verification

  • bun run --filter @tale/ui test -- src/components/search/search-command.test.tsx --maxWorkers=1 --reporter=verbose (22 passed)
  • bunx oxlint --type-aware packages/ui/src/components/search/use-search-command.ts packages/ui/src/components/search/search-command.test.tsx
  • bun run --filter @tale/ui typecheck
  • bunx oxfmt --check packages/ui/src/components/search/use-search-command.ts packages/ui/src/components/search/search-command.test.tsx
  • bun run lint:conflicts
  • No EN/DE/FR, docs, security, or migration files changed; no corresponding sweep was needed.
  • Browser/E2E, backend, full platform suite, and full workspace checks were not run per task limits.

Closes #3612

@yannickmonney

Copy link
Copy Markdown
Contributor Author

Independent review of PR #4308 at 0c40c5efadfebff0e80a0ffe623a10fcc9ccab8b: accepted at source level, light only. CI hasn't run yet.

The brief's checks

Check Result Evidence
After Everything → Chats, the active index is clamped or reset ✅ Clamped to the last remaining row; 0 when nothing is left P1, P10
aria-activedescendant names an existing option ✅ The id resolves to a role="option" inside the listbox the input controls, and it is the only aria-selected="true" P1, P10
Enter opens a visible result ✅ P1: onSelect(chat-1). P10: navigate({to:'/dashboard/$id/chat/$threadId', params:{id:'org-1', threadId:'thread-1'}}) P1, P10
The ArrowDown control (#3612 step 4) ✅ At the head and on main P2, P11
The query-change reset ✅ At the head and on main P3
The shared hook is safe for its other consumers ✅ useSearchCommand has one caller, SearchCommand, which the docs DocsSearchDialog and the platform SidebarSearchCommand use. A growing list (a loadMore append) keeps the active row (P4), and an in-range shrink keeps the index (P5) Suites below
The regression fails on main ✅ The PR suite with main's hook: 1 failed / 21 passed, with aria-activedescendant="_r_26_-4" where "_r_26_-0" was expected Runs below

Runs

Installed Node 24.21.0, Vitest with jsdom, one worker.

Suite Head Main's hook swapped in
PR search-command.test.tsx 22/22 21/22 (the new test fails)
Reviewer probes P1–P9 (packages/ui) 9/9 5/9 (P1, P6, P7 and P9 fail)
Reviewer probes P10–P11 (platform) 2/2 1/2 (P10 fails)

P10 follows #3612's steps exactly, through the real SidebarSearchCommand:

  1. Press Ctrl+K and type a query.
  2. Press ArrowDown four times, to the fifth row (the chat).
  3. Click the Chats chip. Focus moves to the chip.
  4. Click back into the input and press Enter.

Only useBackendQuery and useChatQuery are faked: they return four project hits and one chat. With main's hook, P10 reproduces the issue's "Actual": aria-activedescendant is _r_1_-4, a removed node, and the remaining chat is aria-selected="false".

The other probes:

  • P6: the list shrinks through an empty loading state.
  • P7: ArrowUp after a narrowing from the same source.
  • P8: the list narrows to no results, then widens back.
  • P9: logs [activeIndex, visualResults.length] on every render.

Consumer suites at the head:

  • The docs search dialog and layout, plus the rest of the search folder: 154/154 (11 files).
  • The platform sidebar search, sidebar, search source and result target: 13/13.

Mutants: each one removes one half of the fix

  • Mutant A drops the sync useEffect. All 31 tests still pass. Only P8's record changes: after an empty interim, row 4 comes back, as on main.
  • Mutant B returns the raw activeIndex. The PR's 22 tests still pass; only P9 fails, because the first render after narrowing returns [4,1].

Non-blocking notes

  • N1, test strength. The regression pins the fix as a whole, but not the same-render clamp its comment promises ("Keep the selection valid in the same render…").
    • Mutant B passes all 22 PR tests, because waitFor lets the sync effect repair the state first.
    • A per-render assertion like P9 would pin it.
    • If that half were lost, a data-driven narrowing would leave one committed render with a stale aria-activedescendant, and a key pressed in that gap would read the stale index.
  • N2, the precondition. expect.stringContaining('4') is weak. Option ids carry a useId prefix (_r_26_-4, _r_1i_-0), and a prefix containing a 4 passes the check even if ArrowDown never moved. The test would then pass on main too. Asserting the fifth option's exact id, as P1 does, closes that.
  • N3, missing tests. Bug: Search loses its keyboard selection after narrowing Everything to Chats #3612's ArrowDown control and the query-change reset have no committed test. The gap predates this PR. P2, P3 and P11 show both work at the head and on main.
  • N4, a behaviour change within the contract. After an empty interim, rows that come back start at row 0 (P8: 0 at the head, 4 on main). That is the contract's "reset to the first row".

Composition and static checks

  • With main dcfca9819: merge-tree is clean.
  • With the overlapping PR fix(platform): preserve search privacy and keyboard focus #4214 (TALE-540): it keys SearchCommand by org and user, not by scope. A trial merge in the review worktree merges the code cleanly, and its sidebar tests plus P10–P11 pass 13/13. The only conflict is automation.md, which fix(platform): preserve search privacy and keyboard focus #4214 also has against main.
  • Lint and types: oxlint --type-aware --type-check reports 0 diagnostics on both changed files. The type check is real: it reported a planted TS2322.
  • Format and commit: oxfmt --check is clean, and commitlint passes on both the commit and the PR title.

Unrun

  • A real-browser keyboard run of the Everything → Chats path. All the proof here is jsdom.

  • Screen-reader speech for the active option after narrowing.

  • Bug: Search loses its keyboard selection after narrowing Everything to Chats #3612's acceptance step, "verify the corrected behavior in the local UI before landing". The PR body says the author didn't run it, and this review didn't either, so it is still a landing condition.

  • Browser and E2E suites, the full packages/ui and platform suites, whole-workspace tsc, SAST and knip.

  • CI: at 15:31Z only the five Resolve source jobs had passed, and 16 checks were still queued. All five workflows for 0c40c5efa have been queued since 13:57:06Z:

    • Checks 37207457926
    • E2E 37207457928
    • Commitlint 37207457919
    • SAST 37207457941
    • Build 37207457956

    At 15:24Z, 256 runs were queued repo-wide and 2 were in progress. CI isn't green, and I didn't rerun or cancel anything.

Root's 08:15Z rule

There is no blocking finding; N1–N4 are non-blocking. This ACCEPT binds to 0c40c5ef only, and a new head needs a fresh review.

I did not merge, push, move a card, rerun or cancel CI, or act on a native review. TALE-157's pending human review 957f5616 is untouched.

TALE-687's delivery box has this record, both probes, the mutant diffs, logs for each run and the reproduction script run-probes.sh, which re-ran the whole matrix in 62 s.

@yannickmonney
yannickmonney merged commit 8f4d1a3 into main Oct 4, 2026
62 checks passed
@yannickmonney
yannickmonney deleted the fix/search-selection-after-filter branch October 4, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Search loses its keyboard selection after narrowing Everything to Chats

1 participant