fix(dialog): honour disableClose changes while the dialog is open by mnkprs · Pull Request #1743 · spartan-ng/spartan

mnkprs · 2026-09-11T14:26:44Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our
guidelines: https://github.com/spartan-ng/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Which package are you modifying?

Primitives

Others

What is the current behavior?

Closes #1682

BrnDialog.open() passes _options() by value to BrnDialogService.open(), and BrnDialogRef.dismiss() reads disableClose and closeOnOutsidePointerEvents from that frozen initialOptions snapshot. Changes to the [disableClose] binding while the dialog is open never reach the ref, so:

<hlm-sheet [disableClose]="form.dirty()"> opened pristine captures false; once the user edits the form, Escape or a backdrop click still dismisses the sheet and silently discards the edits.
A dialog opened with disableClose true can never be dismissed by the user, even after the binding turns false.

This is a regression of #882 (fixed in #1203 via updateOptions()), reintroduced when the June dialog/overlay lifecycle rework replaced that mechanism with dismiss() + initialOptions. panelClass and overlayClass already forward live updates to the open ref through setPanelClass/setOverlayClass; disableClose was the odd one out.

What is the new behavior?

BrnDialogRef keeps disableClose and closeOnOutsidePointerEvents as mutable state (seeded from the open-time options) and exposes setDisableClose() / setCloseOnOutsidePointerEvents(). dismiss() reads that state at dismiss time.
BrnDialog forwards both signal inputs to the open ref through effects (_syncDisableClose, _syncCloseOnOutsidePointerEvents), mirroring the existing _syncPanelClass / _syncOverlayClass. BrnSheet and BrnAlertDialog inherit the fix.
Programmatic close() and brnDialogClose buttons are unaffected, as before.

Regression coverage in libs/brain/dialog/src/lib/brn-dialog.spec.ts (real-browser vitest, like the other brain specs):

Escape and backdrop clicks are blocked once disableClose turns on after open, and work again once it turns off.
Escape works once disableClose turns off after opening with it on.
A closeOnOutsidePointerEvents change after open is honoured for backdrop clicks while Escape still closes.
close() still works while disableClose is on.

The first four specs fail on current main (they see the stale snapshot) and pass with this change.

Does this PR introduce a breaking change?

Yes
No

Other information

Docs unchanged: the dialog page's "Dismiss Options" section already describes disableClose / closeOnOutsidePointerEvents as live inputs; this PR makes that true while the dialog is open.
BrnOverlayRef.dismiss() (popover, tooltip, hover-card) reads its initialOptions the same way, so [disableClose] on those primitives has the same limitation. Left out of this PR to keep it scoped to Dialog: disableClose is snapshotted at open() - dynamic updates ignored (regression of #882, previously fixed by #1203) #1682; happy to follow up if you want the overlay primitive aligned too.
Verified locally: pnpm nx format:check --base=origin/main, pnpm nx affected -t lint, pnpm nx run-many -t test -p brain helm, pnpm nx run-many -t build -p brain helm.

Summary by CodeRabbit

New Features
- Dialog dismissal settings can now be updated while a dialog is open. Changes immediately apply to Escape-key presses and backdrop clicks.
- Dialogs now attach to the specified target when one is provided, rather than using the origin as a fallback.
Bug Fixes
- Programmatic closing and other dismissal behavior now respect updated settings while the dialog is open.

greptile-apps · 2026-09-11T14:28:57Z

Confidence Score: 5/5

The PR appears safe to merge; dismissal inputs are propagated through the existing reactive-reference pattern without bypassing dialog lifecycle or overlay-stack protections.

Summary

Stores disableClose and closeOnOutsidePointerEvents as mutable per-dialog reference state.
Synchronizes declarative input changes into the active dialog reference.
Adds browser-level coverage for Escape, backdrop, and programmatic close behavior.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Inputs["Dialog inputs<br/>disableClose / closeOnOutsidePointerEvents"] --> Effects["Reactive synchronization effects"]
  Effects --> Ref["BrnDialogRef mutable dismissal state"]
  Escape["Escape"] --> Dismiss["dismiss(reason)"]
  Backdrop["Backdrop click"] --> Dismiss
  Outside["Outside pointer event"] --> Dismiss
  Dismiss --> Ref
  Ref --> Decision{"Dismissal allowed?"}
  Decision -->|Yes| Close["Close dialog"]
  Decision -->|No| Open["Keep dialog open"]

_{Reviews (1) · Last reviewed commit: "fix(dialog): honour disableClose changes..."}

coderabbitai · 2026-09-11T14:31:05Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 9dc29097-1964-4bff-999e-4d8b0f1fa48e

📥 Commits

Reviewing files that changed from the base of the PR and between 3295fa7 and b1b43d1.

📒 Files selected for processing (2)

libs/brain/dialog/src/lib/brn-dialog-ref.ts
libs/brain/dialog/src/lib/brn-dialog.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The dialog now applies live disableClose and closeOnOutsidePointerEvents values while open. BrnDialogRef exposes setters, and BrnDialog synchronizes input changes. getAttachTo now prefers the attachTo input over the origin fallback. Tests cover dismissal behavior.

Changes

Dynamic dialog dismissal options

Layer / File(s)	Summary
Mutable dialog reference options `libs/brain/dialog/src/lib/brn-dialog-ref.ts`	`BrnDialogRef` stores mutable dismissal options, uses them when dismissing, and exposes setters for runtime updates.
Dialog synchronization and regression coverage `libs/brain/dialog/src/lib/brn-dialog.ts`, `libs/brain/dialog/src/lib/brn-dialog.spec.ts`	`BrnDialog` forwards live input changes to the active reference and prefers `attachTo` over the origin fallback. Tests cover Escape, backdrop clicks, and programmatic closing.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to b1b43

Live dismissal settings are applied when dialogs open and change, and no introduced positioning regression was found. The PR is ready for normal merge checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to b1b43

The changes are confined to dialog behavior. Dismissal remains subject to the configured controls, but applications that specify both an attachment target and an origin may see different positioning and outside-click behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The evidenced exposure is dialog instances using live dismissal inputs or supplying both attachment targets. The changed target can affect overlay routing, but the reference-level dismissal checks still apply.

Trust Boundaries and Controls

observed — disableClose blocks user dismissal; closeOnOutsidePointerEvents additionally blocks outside and backdrop dismissal, but does not block Escape or programmatic close.

Resilience and Maintainability Implications

observed — Regression tests cover changes to dismissal inputs while open and confirm that programmatic close still succeeds when user dismissal is disabled.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: applying disableClose updates while the dialog is open. It is concise and specific.
Linked Issues check	✅ Passed	The PR satisfies the coding requirements in issue `#1682`. `BrnDialog` forwards live `disableClose` and `closeOnOutsidePointerEvents` values to the open `BrnDialogRef` through reactive synchronization. …
Out of Scope Changes check	✅ Passed	The changes remain within issue `#1682` scope. The mutable dismissal state, synchronization effects, and regression tests directly address stale options during an open dialog. The `closeOnOutsidePointer…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

`BrnDialogRef.dismiss()` read `disableClose` and `closeOnOutsidePointerEvents` from the options snapshot taken when the dialog opened, so a binding such as `[disableClose]="form.dirty()"` that changed afterwards was ignored: Escape and backdrop clicks still dismissed the dialog and discarded the user's edits, and a dialog opened with `disableClose` on could never be dismissed. `panelClass` and `overlayClass` already forward live updates to the open ref; `disableClose` was the odd one out. Forward both inputs to the open ref through effects, mirroring the existing panel/overlay class sync, and read them at dismiss time. Programmatic `close()` is unaffected. Closes spartan-ng#1682

mnkprs force-pushed the fix/dialog-disable-close-1682 branch 2 times, most recently from 0f6600e to 3295fa7 Compare September 14, 2026 01:23

mnkprs force-pushed the fix/dialog-disable-close-1682 branch from 3295fa7 to b1b43d1 Compare September 25, 2026 19:01

Navigation Menu

fix(dialog): honour disableClose changes while the dialog is open - #1743

fix(dialog): honour disableClose changes while the dialog is open#1743
mnkprs wants to merge 1 commit into
spartan-ng:mainfrom
mnkprs:fix/dialog-disable-close-1682

mnkprs commented Sep 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

greptile-apps Bot commented Sep 11, 2026

Uh oh!

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mnkprs commented Sep 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

Which package are you modifying?

Primitives

Others

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Summary by CodeRabbit

Uh oh!

greptile-apps Bot commented Sep 11, 2026

Confidence Score: 5/5

Summary

Diagram

Uh oh!

coderabbitai Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Security Architecture Review

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mnkprs commented Sep 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading