diff --git a/src/app/admin/api-keys/api-keys.service.ts b/src/app/admin/api-keys/api-keys.service.ts index e7dfe69da..6ed31c5b5 100644 --- a/src/app/admin/api-keys/api-keys.service.ts +++ b/src/app/admin/api-keys/api-keys.service.ts @@ -223,12 +223,15 @@ export class APIKeyService { ); if (details?.reason !== 'done') return; details.loading('Removing API key...'); - await remove({ - id: key.id, - query_params: {}, - path: 'api_keys', - }); - details.close(); + try { + await remove({ + id: key.id, + query_params: {}, + path: 'api_keys', + }); + } finally { + details.close(); + } notifySuccess('Successfully removed API key.'); this._change.set(Date.now()); } diff --git a/src/app/admin/brokers.component.ts b/src/app/admin/brokers.component.ts index 2f8bebdb5..234d0f34c 100644 --- a/src/app/admin/brokers.component.ts +++ b/src/app/admin/brokers.component.ts @@ -251,7 +251,7 @@ export class AdminBrokersComponent extends AsyncHandler implements OnInit { }, this._dialog, ); - if (!details) return; + if (details.reason !== 'done') return; details.loading('Deleting broker...'); const err = await removeBroker(item.id).catch((_) => _); details.close(); diff --git a/src/app/admin/build-list.component.ts b/src/app/admin/build-list.component.ts index 03f4e486c..da1942124 100644 --- a/src/app/admin/build-list.component.ts +++ b/src/app/admin/build-list.component.ts @@ -166,7 +166,7 @@ export class PlaceBuildListComponent implements OnInit { }, this._dialog, ); - if (!details) return; + if (details.reason !== 'done') return; details.loading(i18n('ADMIN.BUILD_LIST_REMOVE_LOADING')); const err = await cancelBuildJob(i.id).catch((_) => _); details.close(); diff --git a/src/app/admin/cluster-details/cluster-task-list.component.ts b/src/app/admin/cluster-details/cluster-task-list.component.ts index 43ed72be2..587848bf2 100644 --- a/src/app/admin/cluster-details/cluster-task-list.component.ts +++ b/src/app/admin/cluster-details/cluster-task-list.component.ts @@ -270,7 +270,7 @@ export class PlaceClusterTaskListComponent ref.close(); }, (err) => { - ref.componentInstance.loading.set(null); + ref.componentInstance?.loading.set(''); this.killing.set(null); notifyError( i18n('ADMIN.CLUSTER_PROCESS_KILL_ERROR', { diff --git a/src/app/admin/edge.component.ts b/src/app/admin/edge.component.ts index 8b5bb97fb..404b1f78a 100644 --- a/src/app/admin/edge.component.ts +++ b/src/app/admin/edge.component.ts @@ -233,7 +233,7 @@ export class PlaceEdgeComponent implements OnInit { }, this._dialog, ); - if (!details) return; + if (details.reason !== 'done') return; details.loading('Removing edge...'); const err = await removeEdge(i.id).catch((_) => _); details.close(); diff --git a/src/app/admin/extensions.component.ts b/src/app/admin/extensions.component.ts index 410b6e538..e4868ef7f 100644 --- a/src/app/admin/extensions.component.ts +++ b/src/app/admin/extensions.component.ts @@ -271,7 +271,7 @@ export class PlaceExtensionsComponent implements OnInit { await this.updateDomain(ext_list).catch((e) => notifyError(`Error removing extension: ${e}`), ); - ref.componentInstance.loading.set(''); + ref.componentInstance?.loading.set(''); ref.close(); }); } diff --git a/src/app/admin/resource-imports.component.ts b/src/app/admin/resource-imports.component.ts index d922a3188..45f7a8010 100644 --- a/src/app/admin/resource-imports.component.ts +++ b/src/app/admin/resource-imports.component.ts @@ -226,8 +226,13 @@ export class ResourceImportsComponent implements OnInit { if (resp?.reason !== 'done') return; resp.loading(i18n('ADMIN.RESOURCE_IMPORTS_ALL_LOADING')); - await Promise.all(missing.map((_) => this.importResource(_, false))); - resp.close(); + try { + await Promise.all( + missing.map((_) => this.importResource(_, false)), + ); + } finally { + resp.close(); + } notifySuccess( i18n('ADMIN.RESOURCE_IMPORTS_ALL_SUCCESS', { count: missing.length, diff --git a/src/app/admin/signage-plugins/signage-plugins.component.ts b/src/app/admin/signage-plugins/signage-plugins.component.ts index 159607ec4..1edcdd915 100644 --- a/src/app/admin/signage-plugins/signage-plugins.component.ts +++ b/src/app/admin/signage-plugins/signage-plugins.component.ts @@ -282,7 +282,7 @@ export class AdminSignagePluginsComponent }, this._dialog, ); - if (!details) return; + if (details.reason !== 'done') return; details.loading(i18n('ADMIN.SIGNAGE_PLUGINS_REMOVE_LOADING')); const err = await removeSignagePlugin(item.id).catch((_) => _); details.close(); diff --git a/src/app/admin/staff-api.component.ts b/src/app/admin/staff-api.component.ts index f39b68c95..544275828 100644 --- a/src/app/admin/staff-api.component.ts +++ b/src/app/admin/staff-api.component.ts @@ -260,7 +260,7 @@ export class PlaceStaffAPIComponent implements OnInit { }, this._dialog, ); - if (!details || !details.reason) return; + if (details.reason !== 'done') return; details.loading('Removing tenant from domain...'); const system = await del(`/api/staff/v1/tenants/${tenant.id}`).catch( (err) => { diff --git a/src/app/admin/storage/storage.component.ts b/src/app/admin/storage/storage.component.ts index 08be7e8c5..19efc24ec 100644 --- a/src/app/admin/storage/storage.component.ts +++ b/src/app/admin/storage/storage.component.ts @@ -229,8 +229,11 @@ export class StorageComponent implements OnInit { ); if (resp.reason !== 'done') return; resp.loading(i18n('ADMIN.STORAGE_REMOVE_LOADING')); - await removeStorage(item.id); - resp.close(); + try { + await removeStorage(item.id); + } finally { + resp.close(); + } this.loadStorage(); } diff --git a/src/app/admin/upload-library.component.ts b/src/app/admin/upload-library.component.ts index ad24f8f0e..43f6984fe 100644 --- a/src/app/admin/upload-library.component.ts +++ b/src/app/admin/upload-library.component.ts @@ -539,12 +539,15 @@ export class UploadLibraryComponent extends AsyncHandler implements OnInit { ); if (result?.reason !== 'done') return; result.loading(i18n('ADMIN.UPLOADS_LIB_REMOVE_LOADING')); - await remove({ - id: upload.id, - query_params: {}, - path: 'uploads', - }); - result.close(); + try { + await remove({ + id: upload.id, + query_params: {}, + path: 'uploads', + }); + } finally { + result.close(); + } this.refresh.update((value) => value + 1); } } diff --git a/src/app/domains/domain-state.service.ts b/src/app/domains/domain-state.service.ts index d594f52f8..6da59ddbf 100644 --- a/src/app/domains/domain-state.service.ts +++ b/src/app/domains/domain-state.service.ts @@ -246,7 +246,7 @@ export class DomainStateService { ), waitForEvent(ref.afterClosed()), ]); - if (!details) return; + if (details?.reason !== 'done') return; this._changed.set(new Date().valueOf()); } @@ -263,7 +263,7 @@ export class DomainStateService { }, this._dialog, ); - if (!details) return; + if (details.reason !== 'done') return; details.loading('Deleting domain application...'); const err = await removeApplication(item.id).catch((_) => _); details.close(); @@ -294,7 +294,7 @@ export class DomainStateService { ), waitForEvent(ref.afterClosed()), ]); - if (!details) return; + if (details?.reason !== 'done') return; this._changed.set(new Date().valueOf()); } diff --git a/src/app/drivers/driver-state.service.ts b/src/app/drivers/driver-state.service.ts index ec415b979..f352c50bf 100644 --- a/src/app/drivers/driver-state.service.ts +++ b/src/app/drivers/driver-state.service.ts @@ -111,7 +111,7 @@ export class DriverStateService { }, this._dialog, ); - if (!details?.reason) return details.close(); + if (details.reason !== 'done') return details.close(); details.loading('Updating driver...'); const success = await updateDriver(item.id, { ...item, @@ -133,7 +133,7 @@ export class DriverStateService { }, this._dialog, ); - if (!details?.reason) return details.close(); + if (details.reason !== 'done') return details.close(); details.loading('Recompiling driver... This may take a while.'); await recompileDriver(item.id).catch(async (e) => { console.log('Error:', e); @@ -160,7 +160,7 @@ export class DriverStateService { }, this._dialog, ); - if (!details?.reason) return details.close(); + if (details.reason !== 'done') return details.close(); details.loading('Reload driver... This may take a while.'); const success = await reloadDriver(item.id).catch(() => false); if (success === false) { @@ -186,7 +186,7 @@ export class DriverStateService { }, this._dialog, ); - if (!details?.reason) return; + if (details.reason !== 'done') return; const system = await removeSystemModule( this.active_item.id, device.id, diff --git a/src/app/overlays/confirm-modal.component.ts b/src/app/overlays/confirm-modal.component.ts index 66f169fd8..0b4f1387c 100644 --- a/src/app/overlays/confirm-modal.component.ts +++ b/src/app/overlays/confirm-modal.component.ts @@ -4,6 +4,7 @@ import { OnInit, Output, computed, + effect, inject, signal, } from '@angular/core'; @@ -104,11 +105,6 @@ export interface ConfirmModalData { close_delay?: number; } -/** - * Renders a receipt as tab separated rows, for pasting into a ticket or - * spreadsheet. Failed rows are marked so a partial run is not mistaken for a - * complete one. - */ /** * Readable text for whatever an option's `details()` rejected with. * @@ -133,6 +129,11 @@ export function describeError(error: unknown): string { return 'Unknown error'; } +/** + * Renders a receipt as tab separated rows, for pasting into a ticket or + * spreadsheet. Failed rows are marked so a partial run is not mistaken for a + * complete one. + */ export function receiptToTsv(result: ConfirmModalResult): string { const rows = [ ...result.items.map((item) => [item.type, item.name, item.id]), @@ -156,17 +157,26 @@ export const CONFIRM_METADATA = { height: 'auto', }; -export interface ConfirmRepsonse { - reason: 'done' | '' | null; +export interface ConfirmResponse { + /** + * `'done'` when the user confirmed. `undefined` when the modal was + * dismissed (Cancel, Escape or backdrop click). + */ + reason?: 'done'; metadata?: { options?: ConfirmModalSelection }; loading: (_: string) => void; close: () => void; } +/** + * Opens a confirm modal and resolves when the user confirms or dismisses it. + * Always resolves to an object, so callers must check + * `details.reason !== 'done'` before they run the action. + */ export async function openConfirmModal( data: ConfirmModalData, dialog: MatDialog, -): Promise { +): Promise { const ref = dialog.open( ConfirmModalComponent, { @@ -182,7 +192,7 @@ export async function openConfirmModal( ), lastValueFrom(ref.afterClosed()), ])), - loading: (s) => ref.componentInstance.loading.set(s), + loading: (s) => ref.componentInstance?.loading.set(s), close: () => ref.close(), }; } @@ -552,6 +562,17 @@ export class ConfirmModalComponent extends AsyncHandler implements OnInit { /** Allow the user to close the modal */ public readonly enableClose = () => (this._dialog_ref.disableClose = false); + constructor() { + super(); + // Block Escape and backdrop dismissal while the action runs. Material + // nulls `componentInstance` on close, so a dismissal mid-action makes + // the caller's later `loading.set()` throw. The receipt stays + // dismissable. + effect(() => { + this._dialog_ref.disableClose = !!this.loading() && !this.result(); + }); + } + public ngOnInit() { // An option that starts enabled has never been toggled, so nothing has // asked it for a breakdown. Without this it would count as selected diff --git a/src/app/systems/system-state.service.ts b/src/app/systems/system-state.service.ts index fd0a715d6..441156b57 100644 --- a/src/app/systems/system-state.service.ts +++ b/src/app/systems/system-state.service.ts @@ -279,7 +279,7 @@ export class SystemStateService extends AsyncHandler { content: `Are you sure you want to start this system?
All stopped modules within the system will boot up.`, icon: { type: 'icon', class: 'backoffice-controller-play' }, }); - if (!details?.reason) return; + if (details.reason !== 'done') return; details.loading('Starting system...'); const error = await startSystem(this.active_item.id) .then(() => null) @@ -307,7 +307,7 @@ export class SystemStateService extends AsyncHandler { content: `Are you sure you want to stop this system?
All modules will be immediately stopped regardless of any other systems they may be in.`, icon: { type: 'icon', class: 'backoffice-controller-stop' }, }); - if (!details?.reason) return; + if (details.reason !== 'done') return; details.loading('Stopping system...'); const error = await stopSystem(this.active_item.id) .then(() => null) @@ -383,7 +383,7 @@ export class SystemStateService extends AsyncHandler { ), waitForEvent(ref.afterClosed()), ]); - if (!details?.reason) return ref.close(); + if (details?.reason !== 'action') return ref.close(); const system = ref.componentInstance.item as PlaceSystem; if (!system) return ref.close(); await addSystemModule(system.id, device.id).catch((_e) => { @@ -424,7 +424,7 @@ export class SystemStateService extends AsyncHandler { ), waitForEvent(ref.afterClosed()), ]); - if (!details?.reason) return ref.close(); + if (details?.reason !== 'action') return ref.close(); const t = await this.addTrigger( ref.componentInstance.item as PlaceTrigger, ); @@ -463,7 +463,7 @@ export class SystemStateService extends AsyncHandler { ), waitForEvent(ref.afterClosed()), ]); - if (!details?.reason) return; + if (details?.reason !== 'action') return; instance.loading = 'Saving trigger settings...'; const url = `${apiEndpoint()}/systems/${ @@ -491,7 +491,8 @@ export class SystemStateService extends AsyncHandler { content: `

Are you sure you want remove trigger "${trigger.name}"?

Configuration will be updated immediately.

`, icon: { type: 'icon', content: 'delete' }, }); - if (!details?.reason) return; + if (details.reason !== 'done') return; + details.loading('Removing trigger...'); await removeSystemTrigger(this.active_item.id, trigger.id).catch( (err) => { details.close(); @@ -514,7 +515,7 @@ export class SystemStateService extends AsyncHandler { content: `Are you sure you want to change the module priority?
Settings will be updated immediately for the system.`, icon: { type: 'icon', content: 'layers' }, }); - if (!details?.reason) return; + if (details.reason !== 'done') return; details.loading('Updating module order...'); const list: string[] = [...this.active_item.modules]; moveItemInArray(list, fst, snd); @@ -545,7 +546,7 @@ export class SystemStateService extends AsyncHandler { content: `Are you sure you want to sort modules by class?
Modules with the same class name will be grouped together.`, icon: { type: 'icon', content: 'sort' }, }); - if (!details?.reason) return; + if (details.reason !== 'done') return; details.loading('Sorting modules by class...'); let sorted_modules = []; if (alphabetical) { @@ -616,7 +617,7 @@ export class SystemStateService extends AsyncHandler { content: `Are you sure you want to change the zone priority?
Settings will be updated immediately for the system.`, icon: { type: 'icon', content: 'layers' }, }); - if (!details?.reason) return; + if (details.reason !== 'done') return; details.loading('Updating zone order...'); const resp = await updateSystem(this.active_item.id, { ...this.active_item, @@ -666,7 +667,8 @@ export class SystemStateService extends AsyncHandler { content: `Remove ${device.driver_id} from this system?
If this is not used elsewhere the associated data will be removed immediately.`, icon: { type: 'icon', content: 'delete' }, }); - if (!details?.reason) return; + if (details.reason !== 'done') return; + details.loading('Removing module...'); const system = await removeSystemModule( this.active_item.id, device.id, @@ -715,7 +717,7 @@ export class SystemStateService extends AsyncHandler { content: `

Are you sure you want remove zone "${zone.name}" from the system?

Configuration will be updated immediately.`, icon: { type: 'icon', content: 'delete' }, }); - if (!details?.reason) return; + if (details.reason !== 'done') return; const zones = this.active_item.zones.filter((z) => z !== zone.id); const system = await updateSystem(this.active_item.id, { ...this.active_item, diff --git a/src/app/triggers/trigger-state.service.ts b/src/app/triggers/trigger-state.service.ts index bd7f2b69d..a9935a51a 100644 --- a/src/app/triggers/trigger-state.service.ts +++ b/src/app/triggers/trigger-state.service.ts @@ -144,11 +144,11 @@ export class TriggerStateService { { title: i18n('TRIGGERS.REORDER_CONFIRM_TITLE', { type }), content: i18n('TRIGGERS.REORDER_CONFIRM_MSG'), - icon: { type: 'icon', content: 'delete' }, + icon: { type: 'icon', content: 'reorder' }, }, this._dialog, ); - if (!details) return; + if (details.reason !== 'done') return; const list: Array = [ ...(type === 'function' ? this.active_item.actions.functions @@ -173,6 +173,7 @@ export class TriggerStateService { ...this.active_item.toJSON(), actions, }).catch((_) => _); + details.close(); if (!(resp instanceof PlaceTrigger)) { const error = resp as { response?: string; message?: string }; return notifyError( @@ -198,7 +199,7 @@ export class TriggerStateService { }, this._dialog, ); - if (!details?.reason) return; + if (details.reason !== 'done') return; details.loading(i18n('TRIGGERS.REMOVE_CONDITION_LOADING')); const item = this.active_item; const conditions = { @@ -240,7 +241,7 @@ export class TriggerStateService { }, this._dialog, ); - if (!details?.reason) return; + if (details.reason !== 'done') return; details.loading(i18n('TRIGGERS.REMOVE_ACTION_LOADING')); const item = this.active_item; const actions = { @@ -290,7 +291,7 @@ export class TriggerStateService { }, this._dialog, ); - if (!details?.reason) return; + if (details.reason !== 'done') return; details.loading(i18n('TRIGGERS.REMOVE_INSTANCE_LOADING', { type })); const method = type === 'zone' ? removeSystemTrigger : removeSystemTrigger; diff --git a/src/app/ui/metadata-display.component.ts b/src/app/ui/metadata-display.component.ts index 8c5dd0beb..78d603353 100644 --- a/src/app/ui/metadata-display.component.ts +++ b/src/app/ui/metadata-display.component.ts @@ -330,6 +330,7 @@ export class MetadataDisplayComponent ); if (result.reason !== 'done') return; await removeMetadata(this.item().id, { name: field }).catch((err) => { + result.close(); notifyError( `Error removing old "${field}" metadata. Error: ${ err.response || err.message || err @@ -337,6 +338,7 @@ export class MetadataDisplayComponent ); throw err; }); + result.close(); notifySuccess(`Successfully removed "${field}" metadata.`); this.metadata.set( this.metadata().filter((prop) => prop && prop.name !== field), diff --git a/src/app/zones/zones-state.service.ts b/src/app/zones/zones-state.service.ts index eb930b45c..7adf5f9d5 100644 --- a/src/app/zones/zones-state.service.ts +++ b/src/app/zones/zones-state.service.ts @@ -225,7 +225,7 @@ export class ZonesStateService { ), waitForEvent(ref.afterClosed()), ]); - if (!details?.reason) return ref.close(); + if (details?.reason !== 'action') return ref.close(); const zone = await this.addTrigger( ref.componentInstance.item as PlaceTrigger, ); @@ -255,7 +255,7 @@ export class ZonesStateService { }, this._dialog, ); - if (!details?.reason) return; + if (details.reason !== 'done') return; const zone = await updateZone(this.active_item.id, { ...this.active_item, triggers: this.active_item.triggers.filter((t) => t !== trigger.id), diff --git a/src/tests/groups/group-state.service.spec.ts b/src/tests/groups/group-state.service.spec.ts index 6b0e88bb7..bb3b41520 100644 --- a/src/tests/groups/group-state.service.spec.ts +++ b/src/tests/groups/group-state.service.spec.ts @@ -164,7 +164,11 @@ describe('group membership actions', () => { it.each(['user', 'zone'] as const)( 'does not remove a %s when confirmation is cancelled', async (kind) => { - mocks.confirm.mockResolvedValue({ reason: '', close, loading }); + mocks.confirm.mockResolvedValue({ + reason: undefined, + close, + loading, + }); if (kind === 'user') await service.removeUser(user); else await service.removeZone(zone); expect(mocks.removeGroupUser).not.toHaveBeenCalled(); diff --git a/src/tests/overlays/confirm-modal.component.spec.ts b/src/tests/overlays/confirm-modal.component.spec.ts index 292811502..14a18c84f 100644 --- a/src/tests/overlays/confirm-modal.component.spec.ts +++ b/src/tests/overlays/confirm-modal.component.spec.ts @@ -1,10 +1,13 @@ +import { EventEmitter, signal } from '@angular/core'; import { ComponentFixture, TestBed } from '@angular/core/testing'; import { MAT_DIALOG_DATA, + MatDialog, MatDialogModule, MatDialogRef, } from '@angular/material/dialog'; import { NoopAnimationsModule } from '@angular/platform-browser/animations'; +import { Subject } from 'rxjs'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; // Mock the ts-client - factory must be self-contained @@ -30,11 +33,13 @@ vi.mock('../../app/common/notifications', () => ({ })); import { MockComponent } from 'ng-mocks'; +import { DialogEvent } from '../../app/common/types'; import { CONFIRM_METADATA, ConfirmModalComponent, ConfirmModalData, describeError, + openConfirmModal, receiptToTsv, } from '../../app/overlays/confirm-modal.component'; import { IconComponent } from '../../app/ui/icon.component'; @@ -572,6 +577,18 @@ describe('ConfirmModalComponent', () => { component.enableClose(); expect(dialog_ref_mock.disableClose).toBe(false); }); + + it('should block dismissal only while loading', () => { + // A dismissal mid-action tears down the instance the caller is + // still writing to. + component.loading.set('Deleting...'); + fixture.detectChanges(); + expect(dialog_ref_mock.disableClose).toBe(true); + + component.loading.set(''); + fixture.detectChanges(); + expect(dialog_ref_mock.disableClose).toBe(false); + }); }); describe('ngOnInit with close_delay', () => { @@ -681,6 +698,35 @@ describe('ConfirmModalComponent', () => { }); }); +describe('openConfirmModal', () => { + it('does not report a dismissed modal as confirmed', async () => { + const closed = new Subject(); + const ref = { + componentInstance: { + event: new EventEmitter(), + loading: signal(''), + } as Pick | null, + afterClosed: () => closed.asObservable(), + close: vi.fn(), + }; + const dialog = { open: vi.fn(() => ref) } as unknown as MatDialog; + + const pending = openConfirmModal( + { title: '', content: '', icon: { content: 'delete' } }, + dialog, + ); + // Cancel, Escape and backdrop all close without a result, and + // Material drops the component instance on close. + ref.componentInstance = null; + closed.next(undefined); + closed.complete(); + const details = await pending; + + expect(details.reason).not.toBe('done'); + expect(() => details.loading('Deleting...')).not.toThrow(); + }); +}); + describe('receiptToTsv', () => { it('renders one tab separated row per resource', () => { expect( diff --git a/src/tests/users/user-lifecycle.spec.ts b/src/tests/users/user-lifecycle.spec.ts index 708d73364..461891274 100644 --- a/src/tests/users/user-lifecycle.spec.ts +++ b/src/tests/users/user-lifecycle.spec.ts @@ -163,7 +163,7 @@ describe('user restoration', () => { it('does not restore after cancellation', async () => { vi.mocked(openConfirmModal).mockResolvedValue({ - reason: '', + reason: undefined, close, loading, });