[internal] Clean up floating-ui-react hooks#4550
Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
02b5375 to
eb3eafa
Compare
flaviendelangle
left a comment
There was a problem hiding this comment.
PR Review: #4550 — [internal] Clean up floating-ui-react hooks
Branch: codex/clean-up-floating-ui-react-hooks → master
Stats: +420 / -466 across 14 files
Scope: packages/react/src/floating-ui-react/
Critical Issues (0 found)
None.
Important Issues (1 found)
1. useTypeahead: Removing setTypingChange loses deduplication guard
File: packages/react/src/floating-ui-react/hooks/useTypeahead.ts
The old setTypingChange wrapper used dataRef.current.typing to deduplicate calls — it only fired onTypingChange when the typing state actually transitioned (false→true or true→false). The PR removes both the guard and the typing field from ContextData, replacing all calls with direct onTypingChange?.(value).
This means onTypingChange?.(true) can now be called multiple times in succession during a single typing session (e.g., when typing two characters quickly, the intermediate mismatch triggers onTypingChange(false) then onTypingChange(true) again, whereas the old code would suppress the redundant true call).
Current consumers (MenuRoot, SelectRoot) just do typingRef.current = nextTyping, so the practical impact is minimal (idempotent ref assignment). However, this is a semantic change that could affect future consumers expecting transition-only callbacks.
Suggestion: Either keep a local boolean ref to preserve the dedup behavior:
const typingRef = React.useRef(false);
// Then wrap calls:
if (value !== typingRef.current) {
typingRef.current = value;
onTypingChange?.(value);
}Or explicitly document that onTypingChange may now fire redundantly and add a test covering multi-key typeahead to confirm this is acceptable.
Suggestions (3 found)
1. useClick: getNextOpen has an unclear boolean expression
The extracted getNextOpen helper preserves the original logic but the nested negation with ternary is hard to follow:
return (
(open && hasClickedOnInactiveTrigger) ||
!(open && toggle && (openEvent && stickIfOpen ? isClickLikeOpenEvent(openEvent.type) : true))
);Since it's now a named function, this was a good opportunity to use explicit branching:
if (open && hasClickedOnInactiveTrigger) return true;
if (!open) return true;
if (!toggle) return true;
if (stickIfOpen && openEvent) return !isClickLikeOpenEvent(openEvent.type);
return false;2. useHover: isClickLikeOpenEvent could use the shared helper
In useHover.ts, isClickLikeOpenEvent manually checks ['click', 'mousedown'].includes(...), while useHoverReferenceInteraction and useHoverFloatingInteraction already use the shared isClickLikeOpenEventShared from useHoverShared.ts. Aligning useHover to also use the shared helper would reduce duplication.
3. useTypeahead: reference/floating aliases are unnecessary
After the PR, the code creates sharedProps then assigns it to both reference and floating. The intermediate variables are trivial aliases — the return could directly use sharedProps:
return React.useMemo(
() => (enabled ? { reference: sharedProps, floating: sharedProps } : {}),
[enabled, sharedProps],
);Strengths
- Consistent hook ordering — props destructuring before store/context access is now uniform across all hooks, making them easier to scan.
- Good deduplication —
setOpenWithTouchDelay,getNextOpen,resetBlockedFocus,hasBlockingChild,isEventWithinOwnElements,openOnNavigationKeyDownall reduce repeated inline logic. - Correct dependency management — moving
closeWithDelay,handleInteractInside, and similar helpers into effect bodies is safe because the captured values (delayRef,store,timeout,instance) are all identity-stable refs/stores. No stale closure risk. - Type safety improvement —
closeOnReferencePressinuseDismissreplacesas anywith an explicitMouseEvent | PointerEvent | TouchEvent | KeyboardEventunion. getParentOrientationconverted touseStableCallback— follows the project's CLAUDE.md guidelines since it's called within an event handler.- Safe removal of
disabledIndicesReffromuseListNavigationdeps —useValueAsRefreturns an identity-stable ref; only.currentchanges. - Import reordering — alphabetical organization improves consistency.
- Deprecated
typingfield removal fromContextData— only read/written withinuseTypeahead.tsitself; no external consumers.
Recommended Action
- Decide on the
setTypingChangededup behavior — either restore deduplication with a local ref or confirm the change is intentional. - Consider the three suggestions above as optional polish.
- Existing tests should be run to confirm no regressions (
pnpm test:jsdom --no-watchandpnpm test:chromium --no-watchfor affected components).
This cleans up the internal floating-ui-react hooks so the shared interaction code is easier to scan and maintain without changing behavior.
Changes