[tooltip] Reset preventUnmountOnClose on reopen

[michaldudak]

Summary

• Reset the tooltip store's prevented-unmount flag when the popup opens again.
• Prevent one preventUnmountOnClose() close from making later closes stay mounted forever.
• Add regression coverage for a prevented close followed by a normal close.

Bug

Calling preventUnmountOnClose() latches preventUnmountingOnClose permanently. After one prevented close, later normal closes can keep the tooltip positioner mounted forever instead of unmounting after close completion.

Reproduction

• Before: https://codesandbox.io/s/rdf8dy?file=/src/App.tsx
• After: https://codesandbox.io/s/r4wzg3?file=/src/App.tsx

Test plan

• pnpm test:jsdom TooltipRoot --no-watch
• pnpm test:chromium TooltipRoot --no-watch

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/react@4885
  

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/utils@4885
  

commit: 7bc2d79

[code-infra-dashboard]

Bundle size

Bundle	Parsed size	Gzip size
@base-ui/react	🔺+90B(+0.02%)	🔺+16B(+0.01%)

Details of bundle changes

Performance

Total duration: 1,191.82 ms -37.92 ms^(-3.1%) | Renders: 50 ⁽⁺⁰⁾ | Paint: 1,796.01 ms -87.32 ms^(-4.6%)

No significant changes — details

---

Check out the code infra dashboard for more information about this PR.

[netlify]

✅ Deploy Preview for base-ui ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	7bc2d79
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/6a1957f0ac7410000877323f
😎 Deploy Preview	https://deploy-preview-4885--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[michaldudak]

Codex Review (GPT-5.5)

Reviewed the full PR diff for #4885 against freshly fetched upstream/master. Dominant type: bug-fix; the patch is narrow and targets the latched preventUnmountOnClose() state after a prevented tooltip close.

1. Bugs / Issues (None)

I did not find any concrete branch-relevant bugs or regressions in the implementation.

Root Cause & Patch Assessment

The fix resets preventUnmountingOnClose only on a closed-to-open transition, which preserves the current prevented close while preventing the flag from leaking into later close cycles. Keeping the reset inside useOpenStateTransitions also makes the shared popup lifecycle behavior consistent for the other popup primitives that use the same utility.

Repo Conventions / Cleanliness Assessment

The change follows the repo’s hook utility guidance by using useIsoLayoutEffect, and the dependency list is explicit. No public API, docs-generated types, or error messages were changed.

Test Coverage Assessment

I ran pnpm test:jsdom TooltipRoot --no-watch, pnpm test:chromium TooltipRoot --no-watch, pnpm eslint, and git diff --check; all passed. The new regression test runs through the existing contained, detached, and multiple-detached tooltip variants, which is good coverage for the changed lifecycle path. I did not separately transplant the new test onto upstream/master to prove the red/green failure at base.

Recommendation

Approve ✅

The patch addresses the root cause cleanly, the regression coverage matches the reported failure, and the targeted validation passed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54f7fc63ae

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7bc2d7902c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Clear prevented-close latch while still open

When a close request is canceled after calling details.preventUnmountOnClose(), setOpen returns before changing open, so the next render still has open === true and previousOpenRef.current === true. Because this branch only clears on a false→true transition, the preventUnmountingOnClose latch remains set while the popup stays open; a later normal close will then skip useOpenChangeComplete and leave the popup mounted even though no close was prevented. The previous open ? false : ... sync cleared this stale latch on any open render.

Useful? React with 👍 / 👎.

[flaviendelangle]

Any reason not to use useValueAsRef here?

[flaviendelangle]

PR #4885 Review Summary — [tooltip] Reset preventUnmountOnClose on reopen

Four specialized agents reviewed this PR in parallel. All ran type-checking and the relevant test suites (Tooltip, Popover, Menu, Dialog, AlertDialog, PreviewCard) — all pass.

Critical Issues

1. Test suppresses a real production React warning (silent-failure-hunter + code-reviewer + pr-test-analyzer)

File: packages/react/src/tooltip/root/TooltipRoot.test.tsx:341-353

The new tooltip test installs a console.error spy that silently swallows:

Cannot update a component (TooltipTrigger) while rendering a different component (TooltipRoot)

The silent-failure-hunter empirically verified the warning is real and stack-traced it to:

• useOnFirstRender at TooltipRoot.tsx:56-63 synchronously calls store.update({open: true, ...}) during render
• A detached Tooltip.Trigger sibling subscribed to the same store gets a setState-during-render notification — exactly the anti-pattern React's warning catches

The Popover version of the test avoids this entirely by placing the Trigger inside the Root. The same defect exists in PopoverRoot.tsx:46-53 and MenuRoot.tsx:115, but the tooltip test scenario is the only one that exposes it.

Risks: (a) The narrow substring filter ('Cannot update a component' + 'TooltipTrigger' + 'TooltipRoot') is broad enough to mask unrelated future regressions in this component pair. (b) Real users with detached triggers + shared handles + defaultOpen will see this warning in dev.

Recommendations:

• Preferred: Defer useOnFirstRender's store.update into a layout effect to eliminate the warning at its root.
• Minimum: Convert the suppression into an explicit expect(consoleError).toHaveBeenCalledWith(...) assertion documenting the known limitation, so future divergence is caught.

2. Tooltip test runs 3× redundantly inside describe.for (code-reviewer)

File: packages/react/src/tooltip/root/TooltipRoot.test.tsx:339-439

The new "unmounts on a normal close after a prevented close and initially open remount" test is inside the describe.for([contained / detached / multiple detached triggers]) block (line 38), but it ignores the TestTooltip parameter and creates its own hardcoded structure. It executes the same code path three times, adding ~600ms with no extra coverage. Move it to a dedicated outer describe, or refactor to use TestTooltip.

Important Issues

3. Subscribed vs direct state read inconsistency (silent-failure-hunter + code-reviewer)

File: packages/react/src/utils/popups/popupStoreUtils.ts:266-272

const preventUnmountingOnClose = store.useState('preventUnmountingOnClose');  // subscribed
const shouldClearPreventUnmountingOnClose =
  open && !previousOpenRef.current && store.state.preventUnmountingOnClose;   // direct

Both reads return the same value today, but mixing them is confusing and the direct store.state.x read during render bypasses the reactive subscription system. Use the subscribed value in both places.

4. Cross-component coverage gap (pr-test-analyzer)

The fix lives in shared useOpenStateTransitions (used by tooltip, popover, dialog, menu, preview-card). Tests are only added for tooltip + popover. Menu has a related test (MenuRoot.test.tsx:2318) but Dialog, AlertDialog, PreviewCard, and Drawer have no test for the defaultOpen remount scenario this fix targets. A future refactor that splits the hook could silently regress them.

5. Missing WHY comments (comment-analyzer)

Three load-bearing pieces of logic have no comments to defend them against future "cleanup":

• previousOpenRef = React.useRef(false) (line 267): the literal false initialization is the entire point of the fix — it makes defaultOpen=true remounts treated as open edges. A reader could "fix" it back to useRef(open) and break the regression test.
• Why the clear happens during render (lines 267-272): the previous version did this in an effect; the new version intentionally moves it to render so useOpenChangeComplete (line 297) sees the cleared value on the same render as the edge. Without a comment, this is invisible.
• Relationship to setPopupOpenState: the existing comment at popupStoreUtils.ts:125 ("Opening starts a new close cycle…") documents the same invariant. The new code is a second enforcement for paths that bypass setOpen (e.g. defaultOpen remount via useOnFirstRender). This connection should be made explicit.

Suggestions

• forceUnmount still explicitly resets preventUnmountingOnClose: false (line 286) — now potentially redundant. Either remove it with a comment explaining why, or keep it with a comment noting it covers the imperative actionsRef.unmount() path.
• JSDoc for useOpenStateTransitions (lines 249-259) is now incomplete — it no longer mentions that the hook synchronizes preventUnmountingOnClose.
• Console.error suppression filter is too lax: tighten to a single anchored regex and assert call count to catch additional unexpected warnings.
• Tighten waitFors in the first tooltip test to direct sync assertions since BASE_UI_ANIMATIONS_DISABLED makes the transitions synchronous.
• No test covers multiple prevented closes in a row (e.g., 3 prevented closes → unprevented close).
• No controlled-mode coverage of the remount path (both new tests use defaultOpen).

Strengths

• The fix is correct and surgically localized to shared infrastructure — automatically benefits all 5 popup components.
• useRef(false) initial value is the right choice (counter-intuitively).
• Layout-effect ordering is correct; no concurrent-mode / Strict Mode hazards.
• Tests follow CLAUDE.md/AGENTS.md conventions (no flushMicrotasks after render, Vitest-only matchers, run in jsdom).
• The first tooltip test ("does not prevent unmounting on later closes") cleanly isolates the regression with no console suppression.
• Test names are descriptive and behavior-oriented; tests don't depend on internal implementation details.
• All affected test suites pass (607+ tests across 5 components).

Recommended Action Plan

1. Critical: Address the console.error suppression — preferably fix useOnFirstRender setState-during-render, alternatively convert to an explicit assertion.
2. Critical: Move the second tooltip test out of the describe.for parametrized block.
3. Important: Normalize to using the subscribed preventUnmountingOnClose in both reads (lines 269 & 297).
4. Important: Add the three load-bearing WHY comments in popupStoreUtils.ts (initial ref value, render-time computation, link to setPopupOpenState invariant).
5. Consider: Add the prevented-close-and-remount regression test to Dialog/AlertDialog/PreviewCard suites.
6. Re-run review after critical fixes.