[popups] Fix positioning and viewport edge cases#4925
Conversation
atomiks
added
component: menu
commit: |
Bundle size
PerformanceTotal duration: 1,115.40 ms -106.36 ms(-8.7%) | Renders: 50 (+0) | Paint: 1,705.55 ms -153.17 ms(-8.2%)
9 tests within noise — details Check out the code infra dashboard for more information about this PR. |
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
PR #4925 Review — [popups] Fix positioning and viewport edge cases
Scope: 22 files, +276/−91. Three behavioral fixes (custom-anchor tracking, preventUnmountOnClose reset-on-reopen, disabled-hover) + a viewport type/namespace refactor + a docs/JSDoc correction. Reviewed by four specialized agents (code, tests, comments, types); their key claims were independently re-verified (typescript, docs:api, eslint, prettier, and regression runs that revert each fix).
Critical Issues (0)
None. All three fixes were empirically confirmed to fail on master and pass on the branch — they are genuine regression guards, not tautological.
Important Issues (1)
1. Menu's reset-on-reopen behavior is untested — MenuRoot.tsx:314
This PR replaced Menu's hand-written active-trigger block with the shared setPopupOpenState, which newly resets preventUnmountingOnClose = false on open. Menu fully supports preventUnmountOnClose (MenuRoot.tsx:248-250), making it the highest-regression-risk component — it's the only one whose call site is bespoke rather than a uniform store method (Dialog/Tooltip/PreviewCard all call setPopupOpenState identically inside their store classes). The only existing Menu test (MenuRoot.test.tsx:1562) covers the actionsRef.unmount() path, not reopen-then-normal-close.
→ Recommend adding a Menu analog of PopoverRoot.test.tsx:644: open → close once with a gated preventUnmountOnClose() (menu stays mounted) → reopen via trigger → normal-close → assert queryByRole('menu') is null.
Suggestions (3, all optional)
- PreviewCardViewport namespace order —
PreviewCardViewport.tsx:85-88re-exportsStatebeforeProps; the other three viewports usePropsthenState. Cosmetic (alias semantics are order-independent) but it propagates into the generatedpreview-card/types.mdordering. One-line tidy for intra-PR symmetry. - Direct coverage for Dialog/Tooltip/PreviewCard reset-on-reopen — currently rely on Popover as a proxy. Defensible since they share the store-level call site; a single mirrored test would close the loop cheaply.
- Undocumented "why" — the functional core of the bug fix (
popupStoreUtils.ts:124-126and theopen ? false : …sync at:270) has no comment. A one-liner like "Opening clears any pending prevent-unmount request from a previous close cycle" would help future maintainers.
⚠️ One agent finding to disregard
The type-design agent suggested collapsing the multi-line extends BaseUIComponentProps<…> in PreviewCardViewportProps onto one line. I ran pnpm prettier --check — it passes as-is; the wrap is forced by line length (PreviewCard's identifiers are longer than the siblings'). Collapsing it would fail formatting. Leave it.
Strengths
- Docs fix corrects a real, pre-existing error.
data-activation-directionwas documented astop/bottom, but the producer (usePopupViewport.tsx:285) emitsright/left+down/upwith an''fallback. The PR fixes the tokens, fixes a missing-$typo in the popover variant, adds the''option, and unifies all four files with the already-correct Menu definition. domReference→reference(useAnchorPositioning.ts:507) correctly tracks custom anchors forkeepMountedpopups (a customanchorsetselements.referenceviasetPositionReferencewhiledomReferencestays the trigger), and aligns the keepMounted path with the non-keepMountedwhileElementsMountedpath. Safe for virtual elements.!disabledhover guard (PopoverTrigger.tsx:83) brings Popover in line with Tooltip/Menu/SubmenuTrigger, which already guard this way; PreviewCard's<a>trigger has nodisabledconcept. No shared abstraction warranted for a one-token guard.- Type refactor finishes a partial migration to the codebase's dominant convention (~180 components use top-level interface + namespace alias);
instantunions are correctly differentiated per component (verified against each store), and thestringwidening foractivationDirectionmatches its runtime provider. - Verification clean:
pnpm typescript,pnpm docs:api("No files needed updating"),pnpm eslint,pnpm prettier, plus PopoverRoot/PopoverTrigger/PopoverPositioner/Menu/Tooltip/PreviewCard test suites all pass.
Recommended Action
- Add the Menu reset-on-reopen test (the one Important item).
- Optionally apply suggestions 1–3.
- Everything else is merge-ready.
acd92a9 to
11a1b53
Compare
✅ 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. |
3 participants
Updates shared popup behavior so keep-mounted/custom-anchor positioning, preventUnmountOnClose reopen cycles, and viewport API docs are handled consistently across popup components.
Changes