[alert dialog] Fix handle defaults

[atomiks]

This is part of the Codex component behavior and test coverage sweep. It fixes Alert Dialog behavior/API issues found during review and adds focused regression coverage for the affected paths.

Root cause

Alert Dialog reused Dialog store and handle paths that did not always enforce alert-dialog defaults, and the shared manual-unmount flag was not reset after actionsRef.unmount().

Changes

• Fix the Alert Dialog behavior covered by the review findings.
• Keep Alert Dialog on the shared Dialog root hook so it only supplies the alert-specific invariants without adding an extra Dialog wrapper component.
• Add regression tests for the failing or under-tested interaction paths.
• Update docs/API coverage for the public handle contract.

Original findings addressed

• [P2] Public AlertDialog.Handle can silently create a non-alert dialog.
• [P2] defaultOpen is ignored when AlertDialog.Root uses a handle.
• [P2] preventUnmountOnClose() state survives manual unmount and breaks later closes.

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

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

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

commit: 4533cf5

[code-infra-dashboard]

Bundle size

Bundle	Parsed size	Gzip size
@base-ui/react	▼-523B(-0.11%)	▼-1B(0.00%)

Details of bundle changes

Performance

Total duration: 1,162.73 ms -9.02 ms^(-0.8%) | Renders: 50 ⁽⁺⁰⁾ | Paint: 1,781.85 ms +2.75 ms^(+0.2%)

Test	Duration	Renders
Mixed surface mount (app-like density)	80.13 ms 🔺+13.83 ms(+20.9%)	5 (+0)

11 tests within noise — details

---

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

[netlify]

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	4533cf5
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/6a0bf6f678ef2500088b4dff
😎 Deploy Preview	https://deploy-preview-4834--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.

[flaviendelangle]

PR #4834 Review Summary — [alert dialog] Fix handle defaults

Critical Issues

1. Invalid test assertion (pr-test-analyzer, code-reviewer) — at AlertDialogRoot.test.tsx:205, expect(viewport).toContain(popup) is the Vitest core matcher (strings/arrays/iterables), not the jest-dom DOM-containment matcher (toContainElement). The code-reviewer reports it passes in jsdom because Vitest's toContain has a DOM-node branch that calls actual.contains(item) — so this is an important discrepancy in the two analyses; verification recommended (run the test and inspect Vitest's matcher source). If toContainElement is the correct API per jest-dom, swap it.

2. AlertDialogHandle is structurally identical to DialogHandle — type narrowing is illusory (type-design-analyzer, silent-failure-hunter) — the new class adds no instance members, so TS structural typing accepts a plain DialogHandle anywhere AlertDialogHandle is expected. The "fix" of narrowing AlertDialogRootProps.handle and AlertDialogTriggerProps.handle to AlertDialogHandle provides no real safety. Fix: add a private brand (declare private readonly __alertDialog: unique symbol;) on AlertDialogHandle.

Important Issues

3. No direct regression test for the original finding #1 (pr-test-analyzer) — the PR claims to fix "public AlertDialog.Handle can silently create a non-alert dialog", but every existing test uses AlertDialog.createHandle() (which already builds an alert-defaulted store). Removing the new this.store.update(alertDialogState) line at handle.ts:16 would not fail any current test. Add a test that constructs new AlertDialog.Handle(new DialogStore()) with a plain store and asserts role='alertdialog', modal, no pointer dismissal.

4. No test file for AlertDialogTrigger (pr-test-analyzer) — per AGENTS.md convention, components get co-located .test.tsx files.

5. Silent overwrite of caller-supplied store in AlertDialogHandle constructor (silent-failure-hunter) — handle.ts:14-17 unconditionally calls this.store.update(alertDialogState), silently mutating a foreign store's modal/disablePointerDismissal/role. Add a dev-mode warning when the supplied store has conflicting state.

6. useRenderDialogRoot writes to store during render (silent-failure-hunter) — useRenderDialogRoot.tsx:44-56 the alert branch always calls store.update(rootState) synchronously during render (via useOnFirstRender), which can interact badly with concurrent/strict mode. Consider moving to useIsoLayoutEffect.

7. Race in forceUnmount resetting preventUnmountingOnClose (silent-failure-hunter) — popupStoreUtils.ts:264-274 unconditionally resets the flag; a previously-armed animation-completion listener firing after a user re-opens then re-closes with preventUnmountOnClose() could force-unmount despite user intent. Gate the onComplete callback with a fresh store.state.preventUnmountingOnClose check, or attach the reset to an explicit "manual unmount" code path.

Suggestions

8. Positional boolean args (code-reviewer, type-design-analyzer, comment-analyzer) — useRenderDialogRoot(props, false, true) is opaque; prefer mode: 'dialog' | 'drawer' | 'alertdialog' or an options object. Illegal combos (isDrawer && isAlertDialog) are currently representable.

9. alertDialogState duplicated (type-design-analyzer) — the same defaults are encoded once in handle.ts:4-8 and again as ternaries in useRenderDialogRoot.tsx:28-34. Consolidate.

10. Missing comments on load-bearing branches (comment-analyzer, code-reviewer) — the alert-only store.update(rootState) in useOnFirstRender and the duplicate this.store.update(alertDialogState) in the AlertDialogHandle constructor are both invisible-purpose code paths that future cleanups will likely remove. Add one-line comments explaining the invariant.

11. Inconsistent terminology cleanup in AlertDialogRoot JSDoc (comment-analyzer) — actionsRef and onOpenChange JSDoc at AlertDialogRoot.tsx:24-36 still says "the dialog" while the PR migrated trigger/handle docs to "the alert dialog".

12. Pre-existing trigger issues now natural to fix (code-reviewer) — aria-haspopup is hard-coded to 'dialog' in DialogTrigger, and error messages reference Dialog.Trigger even for alert callers. The dedicated AlertDialogTrigger makes this fixable, but the type-cast trick (DialogTrigger as AlertDialogTrigger) leaves no place to specialize.

13. Redundant update call when no store is passed (code-reviewer) — in AlertDialogHandle constructor, when store is undefined the freshly-built DialogStore already has alert defaults; the subsequent update is a no-op.

Strengths

• The three documented bugs are fixed correctly; the shared useRenderDialogRoot refactor matches AGENTS.md's "avoid duplicating logic" guideline.
• The preventUnmountingOnClose: false reset in popupStoreUtils.ts automatically fixes the same bug across Dialog, Drawer, Menu, Popover, PreviewCard, and Tooltip.
• Tests adopt React.createRef<AlertDialog.Root.Actions>() instead of ad-hoc mocks, exercising real public types.
• The "clears manual unmount state" test cleanly reproduces and verifies finding #3.
• AGENTS.md compliance (useTimeout/useStableCallback/useIsoLayoutEffect, shadow-DOM-safe utils, ownerDocument/Window) is clean.
• The code-reviewer ran pnpm typescript, pnpm eslint, and the relevant jsdom test suites — all pass.

Recommended Action

1. Verify the toContain assertion by running pnpm test:jsdom AlertDialog --no-watch and inspecting whether it actually fails when the assertion is broken (e.g., by temporarily changing the viewport to a sibling). If it doesn't, replace with toContainElement.
2. Add the brand to AlertDialogHandle — single line, makes the entire type-narrowing story real.
3. Add the missing regression test for new AlertDialog.Handle(plainStore) (covers original finding #1).
4. Add explanatory comments on the two load-bearing branches (handle.ts:16, useRenderDialogRoot.tsx alert branch).
5. Consider the positional-boolean → discriminated-mode refactor and the actionsRef/onOpenChange JSDoc terminology cleanup before merging.
6. Defer the race-condition fix in forceUnmount for a follow-up unless reproducible.

[atomiks]

@flaviendelangle Claude overstating a bit

Codex:

• toContain is valid here. Vitest’s toContain has a DOM Node branch that calls actual.contains(item), and this repo already uses the same pattern in DialogViewport.test.tsx.
• I’m not adding a separate AlertDialogTrigger.test.tsx; the component is a typed alias over DialogTrigger, and its Alert Dialog integration is covered through the root tests.
• I’m not warning when AlertDialogHandle overwrites a supplied store’s dialog state. Enforcing those invariants is the intended behavior here, not an exceptional condition.
• I’m keeping the first-render useOnFirstRender sync. This matches the repo’s existing root-store pattern and is needed so reused stores have alert-dialog invariants immediately on first render.
• I’m not changing the forceUnmount completion path without a reproducible race. The completion effect is gated by preventUnmountingOnClose, and stale animation listeners are aborted through effect cleanup.
• I’m keeping aria-haspopup="dialog". ARIA supports dialog for this value; there is no separate alertdialog popup token.