Feat: prevent closing with option for confirmation dialog by yonathan-utila · Pull Request #131 · ngneat/dialog

yonathan-utila · 2026-02-04T13:48:20Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: CONTRIBUTING.md#commit
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?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

bolt-new-by-stackblitz · 2026-02-04T13:48:24Z

Run & review this pull request in StackBlitz Codeflow.

yonathan-utila · 2026-02-04T13:48:36Z

apps/playground/src/app/app.component.ts

+      closeGuardAction: 'escape',
+    });
+
+    // guard will fire on esdc keypress


yonathan-utila · 2026-02-04T13:49:23Z

apps/playground/src/app/app.component.ts

+    ref?.beforeClose(async () => {
+      const confirmRef = this.showConfirmationDialog();
+      const result = await firstValueFrom(confirmRef.afterClosed$);
+      return result === true;


return result

yonathan-utila · 2026-02-04T13:49:48Z

apps/playground/src/index.html

  </head>
  <body>
-    <app-root />
+    <app-root></app-root>


redundant ? check this

yonathan-utila · 2026-02-04T13:49:58Z

apps/playground/src/main.ts


 bootstrapApplication(AppComponent, {
  providers: [
+    provideZoneChangeDetection(),


yonathan-utila · 2026-02-04T13:51:02Z

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

  beforeCloseGuards: GuardFN<unknown>[] = [];
  onClose: (result?: unknown) => void;
  onReset: (offset?: DragOffset) => void;
+  private currentCloseAction?: CloseGuardAction;


find better naming, activeGuardAction

yonathan-utila · 2026-02-04T13:52:25Z

.prettierignore

+.next
+out
+
+# Logs


remove that

yonathan-utila · 2026-02-04T13:53:18Z

tsconfig.base.json

    "importHelpers": true,
    "target": "ES2022",
    "lib": ["es2022", "dom"],
+    "skipLibCheck": true,


@NetanelBasal lets talk about this flag. I had some troubles while migrating because of some exposed angular module.

Copilot

Pull request overview

Adds a configurable close-guard mechanism to the dialog library so beforeClose guards can be limited to specific close triggers (ESC/backdrop/close button), and updates the playground to demonstrate confirmation-on-close behavior. This PR also includes a broad Angular/Nx/tooling upgrade.

Changes:

Introduce closeGuardAction/CloseGuardAction and thread close “action” through ESC/backdrop/close-button close paths to selectively run beforeClose guards.
Update playground with a confirmation dialog + demos for “ESC-only” and “all-actions” protection.
Upgrade Angular/Nx/tooling versions and adjust related workspace config (migrations, targets, formatting ignores).

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
tsconfig.base.json	Enables `skipLibCheck` during TS compilation.
package.json	Major Angular/Nx/tooling dependency updates and script changes.
migrations.json	Replaced/expanded migration entries (Nx migrations).
libs/dialog/src/public-api.ts	Exposes `CloseGuardAction` from the library public API.
libs/dialog/src/lib/types.ts	Adds `CloseGuardAction` type + `closeGuardAction` config option.
libs/dialog/src/lib/dialog.component.ts	Tracks close origin (ESC/backdrop) and passes action into `close()`.
libs/dialog/src/lib/dialog-ref.ts	Adds action-aware close handling and guard gating (`shouldRunGuards`).
apps/playground/src/main.ts	Adds `provideZoneChangeDetection()` to providers.
apps/playground/src/index.html	Fixes `<app-root>` element markup.
apps/playground/src/app/confirmation-dialog.component.ts	Adds a simple confirmation dialog component.
apps/playground/src/app/app.component.ts	Adds demos for action-specific close guards + confirmation flow.
apps/playground/src/app/app.component.html	Adds UI section to test close protection guards.
apps/playground/project.json	Updates serve target option from `browserTarget` to `buildTarget`.
.prettierignore	Expands ignored files (deps, builds, lockfiles, generated artifacts).

Comments suppressed due to low confidence (1)

libs/dialog/src/lib/dialog-ref.ts:49

InternalDialogRef.close() defaults action to 'closeButton', but close() is also used for programmatic closes (e.g., DialogService.closeAll() and any consumer calling ref.close()), so those will be treated as a close-button action too. If closeGuardAction: 'closeButton' is intended to mean an actual UI close-button click, consider adding a distinct action like 'programmatic' (and default to that), and pass 'closeButton' only from the dialog close button handler/directive.

  close(result?: unknown, action: CloseGuardAction = 'closeButton'): void {
    this.currentCloseAction = action;
    this.canClose(result)
      .pipe(filter<boolean>(Boolean))
      .subscribe({ next: () => this.onClose(result) });
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-04T15:43:26Z

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

@@ -55,6 +57,13 @@ export class InternalDialogRef extends DialogRef {
  }

  canClose(result: unknown): Observable<boolean> {
+    const shouldRunGuards = this.shouldRunGuards();
+
+    if (!shouldRunGuards) {
+      // skip checks, close the dialog right away
+      return of(true);
+    }
+
    const guards$ = this.beforeCloseGuards
      .map((guard) => guard(result))
      .filter((value) => value !== undefined && value !== true)
@@ -65,6 +74,17 @@ export class InternalDialogRef extends DialogRef {
    return merge(...guards$).pipe(defaultIfEmpty(true), first());
  }

+  private shouldRunGuards(): boolean {
+    const { closeGuardAction } = this.config;
+    // we want to make sure that user already using a guard but NOT using the closeActionGuard type, that their checks still run
+    if (!closeGuardAction || closeGuardAction === 'all') {
+      return true;
+    }
+
+    //else, if the user has provided a closeGuardAction, only run it if it matches the currentCloseAction
+    return closeGuardAction === this.currentCloseAction;
+  }


The new closeGuardAction behavior is not covered by existing unit tests. There are already extensive beforeClose tests in libs/dialog/src/lib/specs/dialog.service.spec.ts, but none verify that guards run only for the configured action (e.g., run on 'escape' but skip on 'backdrop'). Please add tests covering at least: action matches, action mismatches, and 'all' behavior.

Copilot · 2026-02-04T15:43:27Z

apps/playground/src/app/confirmation-dialog.component.ts

+export class ConfirmationDialogComponent {
+  message: string;
+
+  private dialogRef = inject(DialogRef<boolean>);
+


ConfirmationDialogComponent declares a message field but never uses it, and the dialog data passed in showConfirmationDialog() is also unused. Either remove the unused field/data, or render the message (e.g., read from dialogRef.data) so the component reflects the provided config.

Copilot · 2026-02-04T15:43:27Z

package.json

+    "@angular/animations": "21.1.2",
+    "@angular/common": "^21.1.2",
+    "@angular/compiler": "21.1.2",
+    "@angular/core": "21.1.2",
+    "@angular/forms": "21.1.2",
+    "@angular/platform-browser": "21.1.2",
+    "@angular/platform-browser-dynamic": "21.1.2",
+    "@angular/router": "21.1.2",


Angular framework packages should typically be kept on the exact same version range to avoid mismatched peer dependency resolutions. Here @angular/common is ^21.1.2 while most other @angular/* dependencies are pinned to 21.1.2. Align the version specifiers (either pin all to the same exact version or use the same range for all).

Copilot · 2026-02-04T15:43:27Z

libs/dialog/src/lib/dialog.component.ts

      fromEvent<KeyboardEvent>(this.document.body, 'keyup').pipe(
        filter(({ key }) => key === 'Escape'),
-        map(() => closeConfig.escape),
+        // wr now track what truggers the actual closing, esc or backdrop click so we can act accordingly with the closingGuards


The inline comment contains typos (e.g., "truggers") and is hard to read. Please rewrite it clearly (or remove it) so future maintainers can understand why the action is being tracked.

Suggested change

// wr now track what truggers the actual closing, esc or backdrop click so we can act accordingly with the closingGuards

// Track whether the dialog is closed via the Escape key or a backdrop click so close guards can react accordingly.

Copilot · 2026-02-04T15:43:28Z

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

+    // we want to make sure that user already using a guard but NOT using the closeActionGuard type, that their checks still run
+    if (!closeGuardAction || closeGuardAction === 'all') {
+      return true;
+    }
+
+    //else, if the user has provided a closeGuardAction, only run it if it matches the currentCloseAction


The comments in shouldRunGuards() have multiple typos and refer to a "closeActionGuard type" that doesn't exist in this code. Please fix the wording to accurately describe the closeGuardAction behavior to avoid confusion.

Suggested change

// we want to make sure that user already using a guard but NOT using the closeActionGuard type, that their checks still run

if (!closeGuardAction || closeGuardAction === 'all') {

return true;

}

//else, if the user has provided a closeGuardAction, only run it if it matches the currentCloseAction

// If no closeGuardAction is configured, or it is set to 'all', always run all beforeClose guards.

// This keeps the behavior consistent for users who were already using guards before closeGuardAction was introduced.

if (!closeGuardAction || closeGuardAction === 'all') {

return true;

}

// Otherwise, only run guards when the configured closeGuardAction matches the action that triggered close().

Copilot · 2026-02-04T15:43:28Z

apps/playground/src/app/app.component.ts

+      closeGuardAction: 'escape',
+    });
+
+    // guard will fire on esdc keypress


Typo in the comment: "esdc" should be "esc".

Suggested change

// guard will fire on esdc keypress

// guard will fire on esc keypress

Copilot · 2026-02-04T15:43:28Z

apps/playground/src/app/confirmation-dialog.component.ts

+  imports: [CommonModule],
+  template: `
+    <div style="padding: 10px">
+      <p>Are you sure you want to close this dialog ?</p>


Minor punctuation/spacing: there is an extra space before the question mark in the dialog text.

Suggested change

<p>Are you sure you want to close this dialog ?</p>

<p>Are you sure you want to close this dialog?</p>

Copilot · 2026-02-04T15:43:28Z

package.json

  "dependencies": {
-    "@angular/animations": "^17.0.4",
-    "@angular/common": "^17.0.4",
-    "@angular/compiler": "^17.0.4",
-    "@angular/core": "^17.0.4",
-    "@angular/forms": "^17.0.4",
-    "@angular/platform-browser": "^17.0.4",
-    "@angular/platform-browser-dynamic": "^17.0.4",
-    "@angular/router": "^17.0.4",
+    "@angular/animations": "21.1.2",
+    "@angular/common": "^21.1.2",
+    "@angular/compiler": "21.1.2",
+    "@angular/core": "21.1.2",
+    "@angular/forms": "21.1.2",
+    "@angular/platform-browser": "21.1.2",
+    "@angular/platform-browser-dynamic": "21.1.2",
+    "@angular/router": "21.1.2",
    "@ngneat/overview": "6.0.0",
    "rxjs": "~7.5.0",
    "tslib": "2.3.1",
-    "zone.js": "~0.14.2"
+    "zone.js": "0.16.0"
  },
  "devDependencies": {
-    "@angular-devkit/build-angular": "^17.0.3",
-    "@angular/cli": "^17.0.3",
-    "@angular/compiler-cli": "^17.0.4",
-    "@angular-devkit/core": "16.2.10",
-    "@angular-devkit/schematics": "16.2.10",
-    "@angular-eslint/eslint-plugin": "^16.3.1",
-    "@angular-eslint/eslint-plugin-template": "^16.3.1",
-    "@angular-eslint/template-parser": "^16.3.1",
+    "@angular-devkit/build-angular": "^21.1.2",
+    "@angular-devkit/core": "^21.1.2",
+    "@angular-devkit/schematics": "^21.1.2",
+    "@angular-eslint/eslint-plugin": "21.2.0",
+    "@angular-eslint/eslint-plugin-template": "21.2.0",
+    "@angular-eslint/template-parser": "21.2.0",
+    "@angular/cli": "^21.1.2",
+    "@angular/compiler-cli": "21.1.2",
    "@commitlint/cli": "18.4.1",
    "@commitlint/config-angular": "18.4.0",
    "@commitlint/config-conventional": "18.4.0",
-    "@jscutlery/semver": "4.0.0",
+    "@jscutlery/semver": "5.7.1",
    "@ngneat/spectator": "15.0.1",
-    "@nx/angular": "17.1.3",
-    "@nx/eslint-plugin": "17.1.3",
-    "@nx/js": "17.1.3",
-    "@nx/linter": "17.1.3",
-    "@nx/workspace": "17.1.3",
-    "@schematics/angular": "^15.2.7",
-    "@swc-node/register": "~1.6.7",
-    "@swc/core": "~1.3.85",
+    "@nx/angular": "22.4.5",
+    "@nx/eslint-plugin": "22.4.5",
+    "@nx/js": "22.4.5",
+    "@nx/workspace": "22.4.5",
+    "@schematics/angular": "^21.1.2",
+    "@swc-node/register": "^1.8.0",
+    "@swc/core": "^1.15.0",
    "@types/jasmine": "4.3.0",
-    "@types/node": "18.17.18",
+    "@types/node": "18.19.99",
    "@typescript-eslint/eslint-plugin": "^6.10.0",
    "@typescript-eslint/parser": "^6.10.0",
-    "angular-cli-ghpages": "1.0.7",
+    "angular-cli-ghpages": "3.0.2",


The PR title/description describe a dialog close-guard feature, but this PR also includes a major Angular/Nx/tooling upgrade (e.g., Angular 17 -> 21, Nx 17 -> 22) and migration-related changes. This is a significant scope increase; please either split the dependency/tooling upgrade into a separate PR or update the PR description to explicitly call out the upgrade and its impact.

Copilot · 2026-02-04T15:43:29Z

migrations.json

 {
  "migrations": [
    {
-      "version": "16.0.0",
-      "factory": "./update-16/remove-default-project-option",
-      "description": "Remove 'defaultProject' option from workspace configuration. The project to use will be determined from the current working directory.",
-      "package": "@angular/cli",
-      "name": "remove-default-project-option"
+      "cli": "nx",
+      "version": "19.2.0-beta.2",
+      "description": "Updates the default workspace data directory to .nx/workspace-data",
+      "implementation": "./src/migrations/update-19-2-0/move-workspace-data-directory",
+      "package": "nx",
+      "name": "19-2-0-move-graph-cache-directory"
    },


This file was replaced with a large set of Nx migration entries, which appears unrelated to the stated feature in the PR title/description. Please confirm whether the dependency/tooling upgrade is intentional for this PR; otherwise, consider moving these migration changes to a dedicated upgrade PR to reduce review/merge risk.

Copilot · 2026-02-04T15:43:29Z

libs/dialog/src/lib/types.ts

 type Sizes = 'sm' | 'md' | 'lg' | 'fullScreen' | string;
 export type DragConstraint = 'none' | 'bounce' | 'constrain';
 export type CloseStrategy = boolean | 'onlyLastStrategy';
+export type CloseGuardAction = 'escape' | 'backdrop' | 'closeButton' | 'all';


CloseGuardAction is used both as a config value (which includes the special value 'all') and as a concrete close-origin passed to close(). That conflates two concepts and allows passing 'all' as an actual close action, which doesn't represent a real trigger. Consider introducing two separate types (e.g., CloseAction = 'escape' | 'backdrop' | 'closeButton' and CloseGuardAction = CloseAction | 'all') and use CloseAction for the close(..., action) parameter.

Suggested change

export type CloseGuardAction = 'escape' | 'backdrop' | 'closeButton' | 'all';

export type CloseAction = 'escape' | 'backdrop' | 'closeButton';

export type CloseGuardAction = CloseAction | 'all';

yonathan-utila added 3 commits February 3, 2026 15:57

chore: update libraries

8473151

chore: added option to prevent closing on esc

090681b

chore: updated project to angular 21

ba6c43d

yonathan-utila commented Feb 4, 2026

View reviewed changes

.prettierignore Outdated

.next

out

# Logs

Copy link
Copy Markdown

Collaborator Author

yonathan-utila Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove that

yonathan-utila commented Feb 4, 2026

View reviewed changes

chore: fixed ERESOLVE resolvfe when building the lib

97bf70e

yonathan-utila changed the title ~~Close protection~~ Feat: prevent closing with option for confirmation dialog Feb 4, 2026

yonathan-utila requested a review from Copilot February 4, 2026 15:38

Copilot started reviewing on behalf of yonathan-utila February 4, 2026 15:38 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

yonathan-utila added 7 commits February 4, 2026 16:53

chore: better naming

93b773f

chore: typos and others

9302d65

chore: updated action.yml

03ecb17

chore: updated linter

792b9e9

chore: reverted migration

ca00ebb

chore: reverted update

0eefa11

chore: removed non used dps

a125a94

yonathan-utila merged commit ecddd8a into master Feb 5, 2026
4 checks passed

+              .next
+              out
+              # Logs

	// wr now track what truggers the actual closing, esc or backdrop click so we can act accordingly with the closingGuards
	// Track whether the dialog is closed via the Escape key or a backdrop click so close guards can react accordingly.

	// guard will fire on esdc keypress
	// guard will fire on esc keypress

	<p>Are you sure you want to close this dialog ?</p>
	<p>Are you sure you want to close this dialog?</p>

	export type CloseGuardAction = 'escape' \| 'backdrop' \| 'closeButton' \| 'all';
	export type CloseAction = 'escape' \| 'backdrop' \| 'closeButton';
	export type CloseGuardAction = CloseAction \| 'all';

Uh oh!

Conversation

yonathan-utila commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

bolt-new-by-stackblitz bot commented Feb 4, 2026

Uh oh!

yonathan-utila Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

yonathan-utila Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

yonathan-utila Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

yonathan-utila Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

yonathan-utila Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

yonathan-utila Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

yonathan-utila Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yonathan-utila commented Feb 4, 2026 •

edited

Loading