Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,178.33 ms -66.36 ms(-5.3%) | Renders: 76 (+0) | Paint: 1,914.99 ms -119.90 ms(-5.9%)
14 tests within noise — details 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. |
LukasTy
added
component: menu
`open` reaches the popup store synchronously, while `mounted` and `transitionStatus` sync in a layout effect. A retained popup therefore renders one commit as open but not yet starting, so a transition driven by `open && transitionStatus !== 'starting'` enters twice, and completion can fire before the enter animation finishes. Derive the starting status in `popupStoreSelectors` instead of `MenuStore`. The defect is in the shared ordering, so this fixes Menu, Popover, Dialog, Tooltip and Preview Card together. Select and Combobox keep their own selectors and do not have the defect. Add unit coverage for the selector and a Popover browser regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`waitFor` on a running 200ms transition needed a poll to land inside that window. Once it closed, `getAnimations()` stayed empty and the wait failed at timeout instead of retrying. Use a transition that outlasts every phase, then finish the animations to end the phase. This also removes the `pause()` workaround for the reversed exit. Stop asserting inside the `onOpenChangeComplete` mock. The library calls it from `flushSync` inside a promise continuation, so a throw escaped as an unhandled rejection rather than failing the test, and the check was vacuous on close. The test body already asserts the same thing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasTy
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The focused selector correction matches the lifecycle behavior and is comprehensively covered by unit and browser regressions.
Pull request overview
Keeps popup transition state consistent when reopening retained popups.
Changes:
- Derives
startingwhile an open popup lifecycle is not mounted. - Adds selector unit tests and browser regressions for Menu and Popover.
File summaries
| File | Description |
|---|---|
packages/react/src/utils/popups/store.ts |
Corrects the shared transition selector. |
packages/react/src/utils/popups/store.test.ts |
Covers selector lifecycle states. |
packages/react/src/popover/popup/PopoverPopupTransitionState.test.tsx |
Tests retained Popover reopen cycles. |
packages/react/src/menu/popup/MenuPopupTransitionState.test.tsx |
Tests Menu lifecycle combinations and exit reversal. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
LukasTy
marked this pull request as ready for review
September 18, 2026 10:59
LukasTy
requested review from
atomiks,
colmtuite,
flaviendelangle,
jjenzz and
michaldudak
as code owners
September 18, 2026 10:59
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.
Noticed while working on mui/material-ui#48823.
openreaches a popup store synchronously, whilemountedandtransitionStatusare synced in a layout effect. A popup that stays in the DOM while closed therefore renders one commit as open but not yet starting. A transition driven byopen && transitionStatus !== 'starting'then enters twice, andonOpenChangeCompletecan fire before the enter animation finishes.Derive the starting status while the lifecycle is not mounted. This matches
useTransitionStatusand keeps the existing mounting and completion logic.The defect is in the shared ordering rather than in Menu, so the fix lives in
popupStoreSelectorsand covers Menu, Popover, Dialog, Tooltip and Preview Card together. Select and Combobox declare their own selectors and do not have the defect: they holdopenin React state and synchronizeopen,mountedandtransitionStatusin one store update.Tests
packages/react/src/utils/popups/store.test.ts. It covers the uncontrolled and controlled starting windows, pass-through once mounted, the reversed exit that must keepending, and the closed states. This one runs in the JSDOM job.The browser tests use a transition that outlasts every phase, then finish the animations to end each phase. The Menu exit-reversal case waits for the same popup to have
data-openbefore finishing the animation. This prevents the helper from finishing the exit before the reopen handler runs. No assertion depends on catching a short animation window.Validation
Latest validation at
f17a8227a:Earlier validation runs:
Reverting the selector fails 2 of the 5 new unit tests, the 4 retained-popup Menu cases, and the Popover case. The remaining cases pass either way by design: without a retained popup the affected window never renders.
Chromium: 1,788 tests pass across Popover, Dialog, Alert Dialog, Tooltip, Preview Card and Drawer. 1,119 pass across Menu and Popover.
JSDOM: 1,897 tests pass across the same component trees, plus 134 store tests.
TypeScript, ESLint, stylelint, and Prettier pass.
I have followed the PR section of the contributing guide.