fix(F0SearchInput): focus declaratively instead of on a timer - #5194
Open
albertcalasanzs wants to merge 10 commits into
Open
fix(F0SearchInput): focus declaratively instead of on a timer#5194albertcalasanzs wants to merge 10 commits into
albertcalasanzs wants to merge 10 commits into
Conversation
Implemented-with: factorial-dev-workflow/frontend
#5095 replaced the original 50ms `setInterval` with a one-shot `setTimeout(50)`, which stops the runaway loop. It also taught `focusSelectedItem` to leave an already-focused descendant alone — and that guard makes the remaining delay unnecessary. Effects run child-before-parent, so the input can just take focus on mount: it owns focus before any ancestor open-focus effect runs, and the guard then backs off. This removes the 0-50ms window in which `autoFocus` had not happened yet, and drops the timeout, the `focus` listener and the cancel bookkeeping that kept them in sync. It also covers the `autoFocus` false -> true transition on an input that never unmounted, which is how F0Select drives it (`autoFocus={!asList && !isFiltersOpenLocal}`). React's native `autoFocus` only fires when the DOM node is created, so an effect has to exist for that case at all; it was previously untested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xd5igNn7FDNAGFdtjHFLH
Contributor
🔍 Review policy: Code changeDefault rule: any other change needs one approval from f0-devs (rule 4). Required approvals
How this was decided
Policy source: |
albertcalasanzs
marked this pull request as ready for review
August 20, 2026 17:56
Contributor
📦 Alpha Package Version PublishedUse Use |
Contributor
🔍 Visual review for your branch is published 🔍Here are the links to: |
1 task
Contributor
♿ Accessibility (axe) — components changed in this PR2 issues across 2 stories — all non-blocking (
Scope: only stories in the files/component folders this PR changed. It can't yet flag downstream ripple from shared-code/token changes, or diff against |
Contributor
Coverage Report for packages/react
File Coverage
|
||||||||||||||||||||||||||||||||||||||
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
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.
Description
F0SearchInput'sautoFocusfocused on a timer instead of taking focus once. #5095 already replaced the original 50 mssetIntervalwith a one-shotsetTimeout(50), which stops the runaway loop — this removes the remaining timer entirely.It is a simplification of #5095, not a correction of it: #5095 added the very guard that makes the delay unnecessary.
Type of change
Implementation details
What changes relative to #5095
#5095 taught
focusSelectedItemto return early when focus is already on a descendant of the content. Given that guard, the input no longer has to win a race, so it no longer needs a delay:Effects run child-before-parent, so the input owns focus before any ancestor's open-focus effect runs; the guard then backs off. Three things follow:
autoFocushad provably not happened yet at mount — the new testfocuses on mount without waiting for any timerfails on this branch's base and passes with the change. In a Select that window is when the selected option holds focus, so a fast typist's first keystrokes did not land in the search box.F0SearchInput'ssetTimeout(50)andSelectContentImpl'ssetTimeout(0)were tuned against each other. Both orderings happen to converge today thanks to the guard, but nothing in the code says so, and the 50 ms is unexplained.focuslistener that cancelled it, and the bookkeeping keeping those in sync.The
autoFocusfalse → true transitionF0Select drives
autoFocus={!asList && !isFiltersOpenLocal}, soautoFocusturns back on for an input that never unmounted — when the filters panel closes. React's nativeautoFocusonly fires when the DOM node is created and cannot cover that, which is why an effect has to exist here at all rather than just forwarding the attribute. That behaviour was untested; it now has a test.How this code came to be
Context for reviewers, because the original intent is not recoverable from the code:
feat(select): primitive multiselectfocusSelectedItem()onisPositionedand "we prevent open autofocus because we manually focus the selected item".feat: add support for source prop for "in" filterfix:bullets, incl. "search working again"), four artifacts appear together: the 50 mssetInterval, afocus()inside the debouncedonChange,tabIndex={-1}, andkey="search-input".fix: improve select glitchy behavioruseRef— functionally a no-op, but it made the code read as deliberate.F1SearchBox→F0SearchInput,experimental/→components/. The interval rides along untouched: a private workaround becomes public API.feat(ai): move One chat navigation into the sidebarChatHeaderSearchpassesautoFocus— the obvious prop name for "focus the search box". Chat renders inline in a header: no portal, no focus scope, nothing to race. It inherited a workaround for a problem it does not have.Nobody made a bad call; the defect lives in the seams. The tell that this was accretion rather than design is
tabIndex={-1}sitting beside the interval — the component simultaneously declared "you cannot reach me with Tab" and "I will seize focus twenty times a second". Those cannot both be intentional.The user-visible consequence in chat: while the header search was open, focus returned to the search input within 50 ms of any click, so the message composer could not be focused and Radix popovers closed on focus-out almost immediately.
Test plan
Red/green verified locally. With the source change reverted and only the new tests applied, both new assertions fail; with the change applied, both pass.
New tests:
advanceTimersByTime; pins the absence of a timing dependencyExisting coverage unchanged — 85 passed, 1 pre-existing skip across
F0SearchInput,F0Select,ui/Select, including the guarantees added by #5095:pnpm tscclean,oxlint0 warnings,oxfmtapplied.Manual verification in a standalone harness (200-option Select with
showSearchBox, every 5th option disabled, plus an 8,000-message F0Chat), with a probe readingdocument.activeElementand countingfocusoutonrole="searchbox":ArrowDownstill moves into the options (the keyboard carve-out from feat(F0Select): add inline variant #5095)focusin/secis 0 (was ~20) and the composer can be focused and typed intoDeliberately not included
onChangeLocal's focus restore (the remainingsetTimeout). feat(F0Select): add inline variant #5095'sshouldRestoreFocusguard already stops it stealing focus. It may now be redundant, but it compensates for a theft during theonChangere-render and proving that needs its own evidence.tabIndex={-1}andkey="search-input"— the other two artifacts from feat: add support for source prop for "in" filter #2884. Both look vestigial andtabIndex={-1}is arguably an a11y bug, but neither is load-bearing here.handleItemLeave. It is called unconditionally by a pointer move over a disabled option and by a scroll indicator's pointer move, so in principle it can pull focus out of the search box. In practice neither path is reachable in F0Select today: disabled options getdata-disabledand thereforepointer-events-none, and F0'sSelectContentnever renders the Radix scroll buttons (it usesScrollArea+useVirtualizer). A guard there would be speculative hardening, and a unit test for it only fails because jsdom ignorespointer-events. Worth knowing if scroll buttons are ever rendered.