Skip to content

fix(Modal): keep close confirmation armed after an intermediate form submit - #2779

Merged
Lisa18289 merged 2 commits into
mainfrom
claude/modal-close-confirmation-bug-2798f5
Aug 7, 2026
Merged

fix(Modal): keep close confirmation armed after an intermediate form submit#2779
Lisa18289 merged 2 commits into
mainfrom
claude/modal-close-confirmation-bug-2798f5

Conversation

@Lisa18289

Copy link
Copy Markdown
Member

Closes #2775

Problem

Form.handleSubmit called modalController.confirmClose() on every successful submit, setting closeIsConfirmed = true. That flag was only cleared once a close actually went through, so a submit that did not close the Modal left behind a permanently valid "close without asking" grant. A wizard advancing a step via SubmitButton was unprotected from then on: Escape and backdrop click closed the Modal silently, even though the Form was still mounted and still isDirty.

The grant was correct in intent — after saving, "discard unsaved changes?" would be nonsense — but unbounded in scope, while what it needs to cover is exactly one close: the one caused by that submit.

Solution

Scope the grant to the operation that may legitimately close the Modal, as sketched under "Proposed direction" in the issue (the explicit-lifetime alternative).

  • OverlayController.grantCloseWithoutConfirmation() returns a disposer. setOpen() honours an active grant the same way it honours bypassConfirmation.
  • Form wraps the submit flow and — separately — the after-submit callback invoked from onAfterSuccessFeedback, releasing each grant as soon as that operation has settled (synchronously, or via .finally() for an awaited onSubmit). Two independent grants avoid waiting on a callback that may never fire, e.g. when the form is submitted via Enter instead of a SubmitButton.
  • closeIsConfirmed is now consumed on every close attempt instead of only on a successful close, so it cannot survive a close aborted by an onClose handler either — the same latch shape, one level down.

bypassConfirmation and the Action closeModal behaviour are unchanged.

Acceptance criteria

  • A submit that does not close the Modal leaves the confirmation armed — Modal keeps requiring confirmation after a submit that does not close it
  • A submit that does close the Modal still closes without a prompt — Modal closed by its own Form submit needs no confirmation, covering all four paths: synchronously in onSubmit, after an awaited async onSubmit, and in the after-submit callback with and without SubmitButton success feedback
  • bypassConfirmation and Action closeModal unchanged — covered by the existing suite
  • Browser test in Modal.browser.test.tsx alongside the existing confirmation tests

Both tests were verified to actually bite: each of the three grant sites was neutered individually, and in every case exactly the matching variant failed.

Docs

overview.mdx claimed the confirmation is gone "nach einem erfolgreichen Submit" — a description of the bug. It now states that a submit which closes the Modal needs no prompt, while a Modal that stays open after a submit remains protected.

Note on OverlayController

OverlayController is exported wholesale from the . entry, so grantCloseWithoutConfirmation() becomes public API. Additive and non-breaking; happy to move it behind an internal surface if you would rather not expose it.

Verification

components browser tests (222), unit (156), test:compile, pnpm lint (0 errors), and pnpm build with no generated-code drift — all green.

The local test:visual run for remote-react-components shows 26 failures on macOS, but that set is byte-identical with and without this change (verified over repeated runs with baseline cleanup in between) — pre-existing darwin baseline drift, unrelated. This change alters no rendered output, so no visual test was added; adding the run-visual-tests label to confirm against the Linux baselines would still be worthwhile.

Out of scope

The issue's "Related observation 1" — Form overruling <Modal confirmOnClose> when the global flag is on — appears already resolved: confirmOnCloseSources is keyed per source and confirmOnCloseEnabled combines them with .some(Boolean), i.e. the OR semantic the issue asks for. The existing test for it only runs with the flag off, though, so a flag-on case would be cheap insurance. Observation 2 (per-step <Form> losing dirty state) is untouched.

🤖 Generated with Claude Code

Form.handleSubmit called modalController.confirmClose() on every successful
submit, which set closeIsConfirmed = true. That flag was only cleared once a
close actually went through, so a submit that did not close the Modal left
behind a permanently valid "close without asking" grant. Wizards that advance
a step via SubmitButton were unprotected from then on: Escape and backdrop
click closed the Modal silently while the Form was still dirty.

Replace the unbounded latch with a grant that is scoped to the operation that
may legitimately close the Modal:

- OverlayController gains grantCloseWithoutConfirmation(), returning a disposer.
  setOpen() honours an active grant the same way it honours bypassConfirmation.
- Form wraps the submit flow and – separately – the after-submit callback run
  from onAfterSuccessFeedback, releasing each grant as soon as that operation
  has settled (synchronously, or via .finally() for an awaited onSubmit).
- closeIsConfirmed is now consumed on every close attempt instead of only on a
  successful close, so it cannot survive an aborted close either.

bypassConfirmation and the Action closeModal behaviour are unchanged.

Fixes #2775

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Lisa18289
Lisa18289 marked this pull request as draft August 5, 2026 06:17
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Coverage Report for ./packages/components/

Status Category Percentage Covered / Total
🔵 Lines 85.52% 579 / 677
🔵 Statements 85.42% 592 / 693
🔵 Functions 87.75% 129 / 147
🔵 Branches 74.43% 262 / 352
File CoverageNo changed files found.
Generated in workflow #6037 for commit f76cbd6 by the Vitest Coverage Report Action

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🚀 Preview Deployment

Preview environments are ready:

Type URL
docs pr-2779.docs.review.flow-components.de
storybook pr-2779.storybook.review.flow-components.de

Images:

  • docs: ghcr.io/mittwald/flow/docs:pr-2779
  • storybook: ghcr.io/mittwald/flow/storybook:pr-2779

@Lisa18289 Lisa18289 self-assigned this Aug 6, 2026
@Lisa18289
Lisa18289 marked this pull request as ready for review August 6, 2026 06:05
…nfirmation-bug-2798f5

# Conflicts:
#	apps/docs/src/content/04-components/overlays/modal/overview.mdx
#	packages/components/src/integrations/react-hook-form/components/Form/Form.tsx
@Lisa18289
Lisa18289 merged commit 9138e11 into main Aug 7, 2026
10 checks passed
@Lisa18289
Lisa18289 deleted the claude/modal-close-confirmation-bug-2798f5 branch August 7, 2026 10:36
Jan-Eimertenbrink added a commit that referenced this pull request Aug 10, 2026
Resolve the one modify/delete conflict on
apps/docs/src/content/04-components/overlays/modal/overview.mdx: this
branch consolidated the modal tab files into a single index.mdx, while
main updated the confirmOnClose explanation (#2779). Keep the deletion of
overview.mdx and fold main's revised wording — the reset-only phrasing
plus the new submit-closes vs. modal-stays-open paragraph — into
modal/index.mdx.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modal: close confirmation is permanently disarmed by an intermediate form submit (multi-step wizards)

2 participants