From dd01417b2c020e6c042b32feeecc836b5bbb4afc Mon Sep 17 00:00:00 2001 From: sporalla Date: Wed, 21 Oct 2026 14:19:29 +0000 Subject: [PATCH] Fix(UI): Block testrun and show persistent error when MQTT is unavailable(b/555143155) --- .../escapable-dialog.component.spec.ts | 13 +++ .../escapable-dialog.component.ts | 5 +- .../simple-dialog.component.html | 42 +++++---- .../simple-dialog.component.spec.ts | 50 ++++++++++ .../simple-dialog/simple-dialog.component.ts | 2 + .../interceptors/error.interceptor.spec.ts | 60 ++++++++++++ .../src/app/interceptors/error.interceptor.ts | 26 +++++- .../interceptors/loading.interceptor.spec.ts | 18 ++++ .../app/interceptors/loading.interceptor.ts | 4 + .../testrun-initiate-form.component.spec.ts | 83 ++++++++++++++++- .../testrun-initiate-form.component.ts | 40 +++++++- .../mqtt-error-dialog.service.spec.ts | 93 +++++++++++++++++++ .../app/services/mqtt-error-dialog.service.ts | 72 ++++++++++++++ .../services/test-run-mqtt.service.spec.ts | 81 +++++++++++++++- .../src/app/services/test-run-mqtt.service.ts | 66 ++++++++++++- .../ui/src/app/services/test-run.service.ts | 3 +- .../services/testrun-dialog.service.spec.ts | 29 +++++- .../app/services/testrun-dialog.service.ts | 5 + modules/ui/src/app/store/effects.spec.ts | 53 ++++++++++- modules/ui/src/app/store/effects.ts | 47 ++++++++++ 20 files changed, 758 insertions(+), 34 deletions(-) create mode 100644 modules/ui/src/app/services/mqtt-error-dialog.service.spec.ts create mode 100644 modules/ui/src/app/services/mqtt-error-dialog.service.ts diff --git a/modules/ui/src/app/components/escapable-dialog/escapable-dialog.component.spec.ts b/modules/ui/src/app/components/escapable-dialog/escapable-dialog.component.spec.ts index 5d18f9b3a..2a45cd148 100644 --- a/modules/ui/src/app/components/escapable-dialog/escapable-dialog.component.spec.ts +++ b/modules/ui/src/app/components/escapable-dialog/escapable-dialog.component.spec.ts @@ -69,4 +69,17 @@ describe('EscapableDialogComponent', () => { closeSpy.calls.reset(); }); + + it('should not close dialog on escape when escape is disabled', () => { + const closeSpy = spyOn(component.dialogRef, 'close'); + component.disableEscape = true; + + keyboardEvent.next(new KeyboardEvent('keydown', { code: 'Escape' })); + fixture.detectChanges(); + + expect(closeSpy).not.toHaveBeenCalled(); + + closeSpy.calls.reset(); + component.disableEscape = false; + }); }); diff --git a/modules/ui/src/app/components/escapable-dialog/escapable-dialog.component.ts b/modules/ui/src/app/components/escapable-dialog/escapable-dialog.component.ts index 2a982f758..c515ad3a0 100644 --- a/modules/ui/src/app/components/escapable-dialog/escapable-dialog.component.ts +++ b/modules/ui/src/app/components/escapable-dialog/escapable-dialog.component.ts @@ -26,6 +26,7 @@ import { MatDialogRef } from '@angular/material/dialog'; }) export class EscapableDialogComponent { dialogRef = inject>(MatDialogRef); + disableEscape = false; constructor() { const dialogRef = this.dialogRef; @@ -33,7 +34,9 @@ export class EscapableDialogComponent { this.dialogRef .keydownEvents() .pipe( - filter((e: KeyboardEvent) => e.code === 'Escape'), + filter( + (e: KeyboardEvent) => e.code === 'Escape' && !this.disableEscape + ), take(1) ) .subscribe(() => { diff --git a/modules/ui/src/app/components/simple-dialog/simple-dialog.component.html b/modules/ui/src/app/components/simple-dialog/simple-dialog.component.html index c3f37888b..f0b19fa66 100644 --- a/modules/ui/src/app/components/simple-dialog/simple-dialog.component.html +++ b/modules/ui/src/app/components/simple-dialog/simple-dialog.component.html @@ -25,23 +25,25 @@ {{ data.content }}

- - - - +@if (!data.isPersistent) { + + + + +} diff --git a/modules/ui/src/app/components/simple-dialog/simple-dialog.component.spec.ts b/modules/ui/src/app/components/simple-dialog/simple-dialog.component.spec.ts index e001f776d..95eaa3669 100644 --- a/modules/ui/src/app/components/simple-dialog/simple-dialog.component.spec.ts +++ b/modules/ui/src/app/components/simple-dialog/simple-dialog.component.spec.ts @@ -105,4 +105,54 @@ describe('DeleteFormComponent', () => { closeSpy.calls.reset(); }); + + describe('persistent mode', () => { + beforeEach(() => { + component.data = { + icon: 'error', + title: 'Connection Error', + content: 'content', + isPersistent: true, + }; + component.disableEscape = true; + fixture.detectChanges(); + }); + + it('should not render cancel and confirm buttons', () => { + expect(compiled.querySelector('.cancel-button')).toBeNull(); + expect(compiled.querySelector('.confirm-button')).toBeNull(); + expect(compiled.querySelector('mat-dialog-actions')).toBeNull(); + }); + + it('should still render title and content', () => { + expect( + compiled.querySelector('.simple-dialog-title')?.textContent?.trim() + ).toBe('Connection Error'); + expect( + compiled.querySelector('.simple-dialog-content')?.textContent?.trim() + ).toBe('content'); + }); + }); + + it('should set disableEscape from persistent dialog data', () => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + imports: [SimpleDialogComponent, MatDialogModule, MatButtonModule], + providers: [ + { + provide: MatDialogRef, + useValue: { + keydownEvents: () => of(new KeyboardEvent('keydown', { code: '' })), + close: () => ({}), + afterOpened: () => of(void 0), + beforeClosed: () => of(void 0), + }, + }, + { provide: MAT_DIALOG_DATA, useValue: { isPersistent: true } }, + ], + }); + const persistentFixture = TestBed.createComponent(SimpleDialogComponent); + + expect(persistentFixture.componentInstance.disableEscape).toBeTrue(); + }); }); diff --git a/modules/ui/src/app/components/simple-dialog/simple-dialog.component.ts b/modules/ui/src/app/components/simple-dialog/simple-dialog.component.ts index b2a9b102a..77df48728 100644 --- a/modules/ui/src/app/components/simple-dialog/simple-dialog.component.ts +++ b/modules/ui/src/app/components/simple-dialog/simple-dialog.component.ts @@ -31,6 +31,7 @@ interface DialogData { title?: string; content?: string; confirmName?: string; + isPersistent?: boolean; } @Component({ @@ -58,6 +59,7 @@ export class SimpleDialogComponent extends ComponentWithAnnouncement( super(dialogRef, data.title, liveAnnouncer, focusService); this.dialogRef = dialogRef; this.data = data; + this.disableEscape = !!data?.isPersistent; this.liveAnnouncer = liveAnnouncer; this.focusService = focusService; } diff --git a/modules/ui/src/app/interceptors/error.interceptor.spec.ts b/modules/ui/src/app/interceptors/error.interceptor.spec.ts index 7271223a2..5f9a61d98 100644 --- a/modules/ui/src/app/interceptors/error.interceptor.spec.ts +++ b/modules/ui/src/app/interceptors/error.interceptor.spec.ts @@ -17,6 +17,7 @@ import { TestBed } from '@angular/core/testing'; import { ErrorInterceptor } from './error.interceptor'; import { NotificationService } from '../services/notification.service'; +import { SYSTEM_START } from '../services/test-run.service'; import { HttpErrorResponse, HttpHandler, @@ -129,4 +130,63 @@ describe('ErrorInterceptor', () => { } ); }); + + describe('SYSTEM_START request', () => { + const requestMock = new HttpRequest('POST', SYSTEM_START, {}); + + it('should not notify via snackbar when Testrun is unreachable', done => { + const next: HttpHandler = { + handle: () => { + return throwError(new HttpErrorResponse({ status: 0 })); + }, + }; + + interceptor.intercept(requestMock, next).subscribe( + () => ({}), + () => { + expect(notificationServiceMock.notify).not.toHaveBeenCalled(); + done(); + } + ); + }); + + it('should not notify via snackbar when the request times out', done => { + const next: HttpHandler = { + handle: () => { + return of(new HttpResponse()).pipe(delay(2000)); + }, + }; + + interceptor.intercept(requestMock, next, 1000).subscribe( + () => ({}), + () => { + expect(notificationServiceMock.notify).not.toHaveBeenCalled(); + done(); + } + ); + }); + + it('should still notify about backend validation errors', done => { + const next: HttpHandler = { + handle: () => { + return throwError( + new HttpErrorResponse({ + error: { error: 'Device configuration is not complete' }, + status: 400, + }) + ); + }, + }; + + interceptor.intercept(requestMock, next).subscribe( + () => ({}), + () => { + expect(notificationServiceMock.notify).toHaveBeenCalledWith( + 'Device configuration is not complete' + ); + done(); + } + ); + }); + }); }); diff --git a/modules/ui/src/app/interceptors/error.interceptor.ts b/modules/ui/src/app/interceptors/error.interceptor.ts index 536728331..78773f10c 100644 --- a/modules/ui/src/app/interceptors/error.interceptor.ts +++ b/modules/ui/src/app/interceptors/error.interceptor.ts @@ -30,13 +30,31 @@ import { } from 'rxjs'; import { NotificationService } from '../services/notification.service'; -import { SYSTEM_STOP, EXPORT } from '../services/test-run.service'; +import { + SYSTEM_START, + SYSTEM_STOP, + EXPORT, +} from '../services/test-run.service'; import { finalize } from 'rxjs/operators'; const DEFAULT_TIMEOUT_MS = 5000; + const SYSTEM_STOP_TIMEOUT_MS = 60 * 1000; const EXPORT_ZIP_TIMEOUT_MS = 60 * 1000; +/** + * True when the request never got a response from Testrun: either the request + * timed out or the backend was unreachable (HTTP status 0). + */ +export function isConnectionFailure( + error: HttpErrorResponse | TimeoutError | unknown +): boolean { + return ( + error instanceof TimeoutError || + (error instanceof HttpErrorResponse && error.status === 0) + ); +} + @Injectable() export class ErrorInterceptor implements HttpInterceptor { private notificationService = inject(NotificationService); @@ -60,6 +78,12 @@ export class ErrorInterceptor implements HttpInterceptor { return next.handle(request).pipe( timeout(timeoutValue), catchError((error: HttpErrorResponse | TimeoutError) => { + // Connection-level failures of the start command (timeout / backend + // unreachable) are surfaced by the testrun initiate form as a blocking + // error dialog, so no snackbar must be shown for them. + if (request.url.includes(SYSTEM_START) && isConnectionFailure(error)) { + return throwError(error); + } if (error instanceof TimeoutError) { this.notificationService.notify( 'Testrun is not responding. Please try again in a moment.' diff --git a/modules/ui/src/app/interceptors/loading.interceptor.spec.ts b/modules/ui/src/app/interceptors/loading.interceptor.spec.ts index 1ecb6a714..eda9bdbd2 100644 --- a/modules/ui/src/app/interceptors/loading.interceptor.spec.ts +++ b/modules/ui/src/app/interceptors/loading.interceptor.spec.ts @@ -19,6 +19,7 @@ import { LoadingInterceptor } from './loading.interceptor'; import { of } from 'rxjs/internal/observable/of'; import { HttpHandler, HttpRequest, HttpResponse } from '@angular/common/http'; import { LoaderService } from '../services/loader.service'; +import { SYSTEM_START } from '../services/test-run.service'; describe('LoadingInterceptor', () => { const loaderServiceMock: jasmine.SpyObj = jasmine.createSpyObj( @@ -87,4 +88,21 @@ describe('LoadingInterceptor', () => { expect(loaderServiceMock.setLoading).toHaveBeenCalledWith(false); }); + + it('should bypass loader for SYSTEM_START request', done => { + const interceptor: LoadingInterceptor = TestBed.inject(LoadingInterceptor); + + const next: HttpHandler = { + handle: () => { + return of(new HttpResponse()); + }, + }; + + const requestMock = new HttpRequest('POST', SYSTEM_START, {}); + + interceptor.intercept(requestMock, next).subscribe(() => { + expect(loaderServiceMock.setLoading).not.toHaveBeenCalled(); + done(); + }); + }); }); diff --git a/modules/ui/src/app/interceptors/loading.interceptor.ts b/modules/ui/src/app/interceptors/loading.interceptor.ts index 3129e0d4d..e0d72e24c 100644 --- a/modules/ui/src/app/interceptors/loading.interceptor.ts +++ b/modules/ui/src/app/interceptors/loading.interceptor.ts @@ -23,6 +23,7 @@ import { import { Observable } from 'rxjs'; import { finalize } from 'rxjs/operators'; import { LoaderService } from '../services/loader.service'; +import { SYSTEM_START } from '../services/test-run.service'; @Injectable() export class LoadingInterceptor implements HttpInterceptor { @@ -35,6 +36,9 @@ export class LoadingInterceptor implements HttpInterceptor { request: HttpRequest, next: HttpHandler ): Observable> { + if (request.url.includes(SYSTEM_START)) { + return next.handle(request); + } this.totalRequests++; this.zone.run(() => { this.loadingService.setLoading(true); diff --git a/modules/ui/src/app/pages/testrun/components/testrun-initiate-form/testrun-initiate-form.component.spec.ts b/modules/ui/src/app/pages/testrun/components/testrun-initiate-form/testrun-initiate-form.component.spec.ts index 0226884de..14fb1f076 100644 --- a/modules/ui/src/app/pages/testrun/components/testrun-initiate-form/testrun-initiate-form.component.spec.ts +++ b/modules/ui/src/app/pages/testrun/components/testrun-initiate-form/testrun-initiate-form.component.spec.ts @@ -27,6 +27,8 @@ import { MatDialogRef, } from '@angular/material/dialog'; import { TestRunService } from '../../../../services/test-run.service'; +import { TestRunMqttService } from '../../../../services/test-run-mqtt.service'; +import { MqttErrorDialogService } from '../../../../services/mqtt-error-dialog.service'; import { BehaviorSubject } from 'rxjs/internal/BehaviorSubject'; import { Device, DeviceStatus } from '../../../../model/device'; import { DeviceItemComponent } from '../../../../components/device-item/device-item.component'; @@ -35,7 +37,8 @@ import { MatInputModule } from '@angular/material/input'; import { BrowserAnimationsModule } from '@angular/platform-browser/animations'; import { DeviceTestsComponent } from '../../../../components/device-tests/device-tests.component'; import { device, MOCK_TEST_MODULES } from '../../../../mocks/device.mock'; -import { of } from 'rxjs'; +import { of, throwError, TimeoutError } from 'rxjs'; +import { HttpErrorResponse } from '@angular/common/http'; import { MOCK_PROGRESS_DATA_WAITING_FOR_DEVICE } from '../../../../mocks/testrun.mock'; import { SpinnerComponent } from '../../../../components/spinner/spinner.component'; import { provideMockStore } from '@ngrx/store/testing'; @@ -46,6 +49,8 @@ describe('ProgressInitiateFormComponent', () => { let fixture: ComponentFixture; let compiled: HTMLElement; + const testRunMqttServiceMock = jasmine.createSpyObj(['isConnected']); + const mqttErrorDialogServiceMock = jasmine.createSpyObj(['open']); const testRunServiceMock = jasmine.createSpyObj([ 'getDevices', 'fetchDevices', @@ -57,6 +62,7 @@ describe('ProgressInitiateFormComponent', () => { testRunServiceMock.getDevices.and.returnValue( new BehaviorSubject([device, device]) ); + testRunMqttServiceMock.isConnected.and.returnValue(true); testRunServiceMock.startTestrun.and.returnValue(of(true)); testRunServiceMock.systemStatus$ = of(MOCK_PROGRESS_DATA_WAITING_FOR_DEVICE); @@ -64,6 +70,11 @@ describe('ProgressInitiateFormComponent', () => { TestBed.configureTestingModule({ providers: [ { provide: TestRunService, useValue: testRunServiceMock }, + { provide: TestRunMqttService, useValue: testRunMqttServiceMock }, + { + provide: MqttErrorDialogService, + useValue: mqttErrorDialogServiceMock, + }, { provide: MatDialogRef, useValue: { @@ -97,6 +108,11 @@ describe('ProgressInitiateFormComponent', () => { afterEach(() => { testRunServiceMock.getSystemStatus.calls.reset(); + testRunServiceMock.startTestrun.calls.reset(); + testRunServiceMock.fetchVersion.calls.reset(); + mqttErrorDialogServiceMock.open.calls.reset(); + testRunMqttServiceMock.isConnected.and.returnValue(true); + testRunServiceMock.startTestrun.and.returnValue(of(true)); }); describe('Class tests', () => { @@ -223,6 +239,71 @@ describe('ProgressInitiateFormComponent', () => { expect(testRunServiceMock.fetchVersion).toHaveBeenCalled(); }); + + it('should not start testrun and should open MQTT error dialog when MQTT is disconnected', () => { + testRunMqttServiceMock.isConnected.and.returnValue(false); + const cancelSpy = spyOn(component, 'cancel'); + + component.startTestRun(); + + expect(testRunServiceMock.startTestrun).not.toHaveBeenCalled(); + expect(testRunServiceMock.fetchVersion).not.toHaveBeenCalled(); + expect(cancelSpy).toHaveBeenCalledWith(null); + expect(mqttErrorDialogServiceMock.open).toHaveBeenCalled(); + }); + + it('should open MQTT error dialog when startTestrun request times out', () => { + testRunServiceMock.startTestrun.and.returnValue( + throwError(() => new TimeoutError()) + ); + const cancelSpy = spyOn(component, 'cancel'); + + component.startTestRun(); + + expect(cancelSpy).toHaveBeenCalledWith(null); + expect(mqttErrorDialogServiceMock.open).toHaveBeenCalled(); + }); + + it('should open MQTT error dialog when Testrun is unreachable', () => { + testRunServiceMock.startTestrun.and.returnValue( + throwError(() => new HttpErrorResponse({ status: 0 })) + ); + const cancelSpy = spyOn(component, 'cancel'); + + component.startTestRun(); + + expect(cancelSpy).toHaveBeenCalledWith(null); + expect(mqttErrorDialogServiceMock.open).toHaveBeenCalled(); + }); + + it('should open MQTT error dialog when MQTT drops while the start request is in flight', () => { + testRunMqttServiceMock.isConnected.and.returnValues(true, false); + const cancelSpy = spyOn(component, 'cancel'); + + component.startTestRun(); + + expect(testRunServiceMock.startTestrun).toHaveBeenCalled(); + expect(cancelSpy).toHaveBeenCalledWith(null); + expect(mqttErrorDialogServiceMock.open).toHaveBeenCalled(); + }); + + it('should keep the dialog open and not show MQTT error dialog on backend validation error', () => { + testRunServiceMock.startTestrun.and.returnValue( + throwError( + () => + new HttpErrorResponse({ + status: 409, + error: { error: 'Testrun cannot be started' }, + }) + ) + ); + const cancelSpy = spyOn(component, 'cancel'); + + component.startTestRun(); + + expect(cancelSpy).not.toHaveBeenCalled(); + expect(mqttErrorDialogServiceMock.open).not.toHaveBeenCalled(); + }); }); }); diff --git a/modules/ui/src/app/pages/testrun/components/testrun-initiate-form/testrun-initiate-form.component.ts b/modules/ui/src/app/pages/testrun/components/testrun-initiate-form/testrun-initiate-form.component.ts index 4c66007b0..fe3976e5d 100644 --- a/modules/ui/src/app/pages/testrun/components/testrun-initiate-form/testrun-initiate-form.component.ts +++ b/modules/ui/src/app/pages/testrun/components/testrun-initiate-form/testrun-initiate-form.component.ts @@ -28,6 +28,9 @@ import { MatDialogRef, } from '@angular/material/dialog'; import { TestRunService } from '../../../../services/test-run.service'; +import { TestRunMqttService } from '../../../../services/test-run-mqtt.service'; +import { MqttErrorDialogService } from '../../../../services/mqtt-error-dialog.service'; +import { isConnectionFailure } from '../../../../interceptors/error.interceptor'; import { Device, TestModule, @@ -97,6 +100,8 @@ export class TestrunInitiateFormComponent override dialogRef: MatDialogRef; data = inject(MAT_DIALOG_DATA); private readonly testRunService = inject(TestRunService); + private readonly testRunMqttService = inject(TestRunMqttService); + private readonly mqttErrorDialogService = inject(MqttErrorDialogService); private fb = inject(FormBuilder); private deviceValidators = inject(DeviceValidators); private readonly changeDetectorRef = inject(ChangeDetectorRef); @@ -206,8 +211,14 @@ export class TestrunInitiateFormComponent ); if (this.selectedDevice && !this.startRequestSent.value) { + // Scenario 1: MQTT is offline/disconnected - do not start the testrun + // and do not show any progress; surface the blocking error dialog. + if (!this.testRunMqttService.isConnected()) { + this.handleConnectionFailure(); + return; + } + this.startRequestSent.next(true); - this.testRunService.fetchVersion(); this.testRunService .startTestrun({ ...this.selectedDevice, @@ -217,15 +228,40 @@ export class TestrunInitiateFormComponent .pipe(take(1)) .subscribe({ next: status => { + // MQTT dropped while the start request was in flight: status + // updates can no longer be tracked. + if (!this.testRunMqttService.isConnected()) { + this.handleConnectionFailure(); + return; + } + // Refresh the version only after a successful start so that a + // hanging backend does not trigger a spinner or snackbar from this + // secondary request while the start command itself is failing. + this.testRunService.fetchVersion(); this.cancel(status); }, - error: () => { + error: error => { this.startRequestSent.next(false); + // Testrun did not acknowledge the start command (timeout or + // unreachable) or MQTT dropped meanwhile. Backend validation + // errors (4xx/5xx) are reported via the ErrorInterceptor snackbar + // and keep the dialog open so the user can correct and retry. + if ( + isConnectionFailure(error) || + !this.testRunMqttService.isConnected() + ) { + this.handleConnectionFailure(); + } }, }); } } + private handleConnectionFailure(): void { + this.cancel(null); + this.mqttErrorDialogService.open(); + } + private createInitiateForm() { this.initiateForm = this.fb.group({ firmware: ['', [this.deviceValidators.firmwareStringFormat()]], diff --git a/modules/ui/src/app/services/mqtt-error-dialog.service.spec.ts b/modules/ui/src/app/services/mqtt-error-dialog.service.spec.ts new file mode 100644 index 000000000..386b0c4be --- /dev/null +++ b/modules/ui/src/app/services/mqtt-error-dialog.service.spec.ts @@ -0,0 +1,93 @@ +/** + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { TestBed } from '@angular/core/testing'; +import { MatDialog, MatDialogRef } from '@angular/material/dialog'; +import { Subject } from 'rxjs'; + +import { + MQTT_ERROR_MESSAGE, + MqttErrorDialogService, +} from './mqtt-error-dialog.service'; +import { SimpleDialogComponent } from '../components/simple-dialog/simple-dialog.component'; + +describe('MqttErrorDialogService', () => { + let service: MqttErrorDialogService; + let dialog: MatDialog; + let afterClosed$: Subject; + let openSpy: jasmine.Spy; + let closeAllSpy: jasmine.Spy; + + beforeEach(() => { + TestBed.configureTestingModule({}); + service = TestBed.inject(MqttErrorDialogService); + dialog = TestBed.inject(MatDialog); + afterClosed$ = new Subject(); + closeAllSpy = spyOn(dialog, 'closeAll'); + openSpy = spyOn(dialog, 'open').and.returnValue({ + afterClosed: () => afterClosed$, + } as unknown as MatDialogRef); + }); + + it('should be created', () => { + expect(service).toBeTruthy(); + }); + + it('should expose the exact required error message', () => { + expect(MQTT_ERROR_MESSAGE).toBe( + 'Unable to connect and update test status. Please restart the application to try again.' + ); + }); + + it('should close other dialogs and open a persistent, non-dismissible dialog', () => { + service.open(); + + expect(closeAllSpy).toHaveBeenCalledTimes(1); + expect(openSpy).toHaveBeenCalledOnceWith(SimpleDialogComponent, { + ariaLabel: MQTT_ERROR_MESSAGE, + role: 'alertdialog', + data: { + icon: 'error', + title: 'Connection Error', + content: MQTT_ERROR_MESSAGE, + isPersistent: true, + }, + autoFocus: true, + hasBackdrop: true, + disableClose: true, + panelClass: ['simple-dialog', 'mqtt-error-dialog'], + }); + expect(service.isOpen()).toBeTrue(); + }); + + it('should open the dialog only once while it is displayed', () => { + service.open(); + service.open(); + + expect(openSpy).toHaveBeenCalledTimes(1); + expect(closeAllSpy).toHaveBeenCalledTimes(1); + }); + + it('should allow the dialog to be re-opened after it was closed', () => { + service.open(); + afterClosed$.next(); + + expect(service.isOpen()).toBeFalse(); + + service.open(); + + expect(openSpy).toHaveBeenCalledTimes(2); + }); +}); diff --git a/modules/ui/src/app/services/mqtt-error-dialog.service.ts b/modules/ui/src/app/services/mqtt-error-dialog.service.ts new file mode 100644 index 000000000..5a98724cb --- /dev/null +++ b/modules/ui/src/app/services/mqtt-error-dialog.service.ts @@ -0,0 +1,72 @@ +/** + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { inject, Injectable } from '@angular/core'; +import { MatDialog } from '@angular/material/dialog'; +import { SimpleDialogComponent } from '../components/simple-dialog/simple-dialog.component'; + +export const MQTT_ERROR_MESSAGE = + 'Unable to connect and update test status. Please restart the application to try again.'; + +/** + * Surfaces the blocking "MQTT connection lost" dialog. + * + * Kept separate from TestrunDialogService so that the testrun initiate form + * (which TestrunDialogService opens) can depend on it without creating a + * circular import. + */ +@Injectable({ providedIn: 'root' }) +export class MqttErrorDialogService { + private dialog = inject(MatDialog); + private isDialogOpen = false; + + /** Whether the blocking MQTT error dialog is currently displayed. */ + isOpen(): boolean { + return this.isDialogOpen; + } + + /** + * Opens a persistent, non-dismissible dialog informing the user that the + * connection to the MQTT service was lost. It can only be cleared by + * restarting the application. Any other open dialog is closed first, and + * repeated calls while it is displayed are ignored. + */ + open(): void { + if (this.isDialogOpen) { + return; + } + this.isDialogOpen = true; + this.dialog.closeAll(); + this.dialog + .open(SimpleDialogComponent, { + ariaLabel: MQTT_ERROR_MESSAGE, + role: 'alertdialog', + data: { + icon: 'error', + title: 'Connection Error', + content: MQTT_ERROR_MESSAGE, + isPersistent: true, + }, + autoFocus: true, + hasBackdrop: true, + disableClose: true, + panelClass: ['simple-dialog', 'mqtt-error-dialog'], + }) + .afterClosed() + .subscribe(() => { + this.isDialogOpen = false; + }); + } +} diff --git a/modules/ui/src/app/services/test-run-mqtt.service.spec.ts b/modules/ui/src/app/services/test-run-mqtt.service.spec.ts index 3198430da..e9253ec7a 100644 --- a/modules/ui/src/app/services/test-run-mqtt.service.spec.ts +++ b/modules/ui/src/app/services/test-run-mqtt.service.spec.ts @@ -1,9 +1,9 @@ import { TestBed } from '@angular/core/testing'; import { TestRunMqttService } from './test-run-mqtt.service'; -import { IMqttMessage, MqttService } from 'ngx-mqtt'; +import { IMqttMessage, MqttConnectionState, MqttService } from 'ngx-mqtt'; import SpyObj = jasmine.SpyObj; -import { of } from 'rxjs'; +import { BehaviorSubject, of, Subject, throwError } from 'rxjs'; import { MOCK_ADAPTERS } from '../mocks/settings.mock'; import { Topic } from '../model/topic'; import { MOCK_INFO, MOCK_INTERNET } from '../mocks/topic.mock'; @@ -14,9 +14,21 @@ describe('TestRunMqttService', () => { let service: TestRunMqttService; let mockService: SpyObj; let testRunServiceMock: SpyObj; + let stateSubject: BehaviorSubject; + let onOffline: Subject; + let onError: Subject; beforeEach(() => { - mockService = jasmine.createSpyObj(['observe']); + stateSubject = new BehaviorSubject( + MqttConnectionState.CONNECTED + ); + onOffline = new Subject(); + onError = new Subject(); + mockService = jasmine.createSpyObj(['observe'], { + state: stateSubject, + onOffline, + onError, + }); testRunServiceMock = jasmine.createSpyObj(['changeReportURL']); TestBed.configureTestingModule({ @@ -32,6 +44,69 @@ describe('TestRunMqttService', () => { expect(service).toBeTruthy(); }); + describe('connection state tracking', () => { + let connectionStates: boolean[]; + + beforeEach(() => { + connectionStates = []; + service + .getConnectionState() + .subscribe(isConnected => connectionStates.push(isConnected)); + }); + + it('should report connected when MqttConnectionState is CONNECTED', () => { + stateSubject.next(MqttConnectionState.CONNECTED); + + expect(service.isConnected()).toBeTrue(); + expect(connectionStates).toEqual([true]); + }); + + it('should report connection loss when MqttConnectionState is CLOSED', () => { + stateSubject.next(MqttConnectionState.CLOSED); + + expect(service.isConnected()).toBeFalse(); + expect(connectionStates).toEqual([true, false]); + }); + + it('should not report connection loss while CONNECTING (initial handshake / reconnect)', () => { + stateSubject.next(MqttConnectionState.CONNECTING); + + expect(service.isConnected()).toBeFalse(); + expect(connectionStates).toEqual([true]); + }); + + it('should report connection loss on offline event', () => { + onOffline.next(); + + expect(connectionStates).toEqual([true, false]); + }); + + it('should report connection loss on error event', () => { + onError.next(new Error('broker error')); + + expect(connectionStates).toEqual([true, false]); + }); + + it('should report connection loss when a topic subscription fails', () => { + mockService.observe.and.returnValue( + throwError(() => new Error('subscription failed')) + ); + + service.getStatus().subscribe(); + + expect(connectionStates).toEqual([true, false]); + }); + + it('should report connected again once the broker reconnects', () => { + stateSubject.next(MqttConnectionState.CLOSED); + stateSubject.next(MqttConnectionState.CONNECTING); + stateSubject.next(MqttConnectionState.CONNECTED); + + expect(service.isConnected()).toBeTrue(); + expect(connectionStates).toEqual([true, false, true]); + }); + }); + describe('getNetworkAdapters', () => { beforeEach(() => { mockService.observe.and.returnValue(of(getResponse(MOCK_ADAPTERS))); diff --git a/modules/ui/src/app/services/test-run-mqtt.service.ts b/modules/ui/src/app/services/test-run-mqtt.service.ts index 276c1c6b6..7dcd98620 100644 --- a/modules/ui/src/app/services/test-run-mqtt.service.ts +++ b/modules/ui/src/app/services/test-run-mqtt.service.ts @@ -1,6 +1,16 @@ -import { Injectable, inject } from '@angular/core'; -import { IMqttMessage, MqttService } from 'ngx-mqtt'; -import { catchError, Observable, of } from 'rxjs'; +import { DestroyRef, Injectable, inject } from '@angular/core'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; +import { IMqttMessage, MqttConnectionState, MqttService } from 'ngx-mqtt'; +import { + BehaviorSubject, + catchError, + distinctUntilChanged, + filter, + merge, + Observable, + of, + tap, +} from 'rxjs'; import { map } from 'rxjs/operators'; import { Adapters } from '../model/setting'; import { TestrunStatus } from '../model/testrun-status'; @@ -13,6 +23,55 @@ import { TestRunService } from './test-run.service'; export class TestRunMqttService { private mqttService = inject(MqttService); private testrunService = inject(TestRunService); + private destroyRef = inject(DestroyRef); + + /** + * Confirmed health of the MQTT connection. + * + * Starts optimistic (`true`) and is only flipped to `false` once the broker + * connection is confirmed lost (CLOSED / offline / error). The transient + * CONNECTING state is intentionally ignored so that the initial handshake on + * app start-up (e.g. a page reload during an active test) is not reported as + * a connection loss. + */ + private connectionState$ = new BehaviorSubject(true); + /** Latest raw client state (ngx-mqtt exposes `state` as a plain Subject). */ + private currentState = MqttConnectionState.CLOSED; + + constructor() { + this.initConnectionTracking(); + } + + private initConnectionTracking(): void { + this.mqttService.state + .pipe( + tap(state => (this.currentState = state)), + filter(state => state !== MqttConnectionState.CONNECTING), + map(state => state === MqttConnectionState.CONNECTED), + takeUntilDestroyed(this.destroyRef) + ) + .subscribe(isConnected => this.connectionState$.next(isConnected)); + + merge(this.mqttService.onOffline, this.mqttService.onError) + .pipe(takeUntilDestroyed(this.destroyRef)) + .subscribe(() => this.connectionState$.next(false)); + } + + /** + * Whether the MQTT client is connected to the broker right now. + * Used as a pre-flight check before starting a testrun. + */ + isConnected(): boolean { + return this.currentState === MqttConnectionState.CONNECTED; + } + + /** + * Emits `false` when the MQTT connection is confirmed lost and `true` once + * it is (re-)established. Used to detect a drop during an active testrun. + */ + getConnectionState(): Observable { + return this.connectionState$.pipe(distinctUntilChanged()); + } getNetworkAdapters(): Observable { return this.topic(Topic.NetworkAdapters); @@ -43,6 +102,7 @@ export class TestRunMqttService { JSON.parse(new TextDecoder().decode(res.payload)) as Type ), catchError(() => { + this.connectionState$.next(false); return of({} as Type); }) ); diff --git a/modules/ui/src/app/services/test-run.service.ts b/modules/ui/src/app/services/test-run.service.ts index adf081032..6909fd9d2 100644 --- a/modules/ui/src/app/services/test-run.service.ts +++ b/modules/ui/src/app/services/test-run.service.ts @@ -38,6 +38,7 @@ import { import { QuestionFormat } from '../model/question'; const API_URL = `http://${window.location.hostname}:8000`; +export const SYSTEM_START = '/system/start'; export const SYSTEM_STOP = '/system/stop'; export const EXPORT = '/export'; @@ -218,7 +219,7 @@ export class TestRunService { startTestrun(device: Device): Observable { return this.http.post( - `${API_URL}/system/start`, + `${API_URL}${SYSTEM_START}`, JSON.stringify({ device }) ); } diff --git a/modules/ui/src/app/services/testrun-dialog.service.spec.ts b/modules/ui/src/app/services/testrun-dialog.service.spec.ts index 39a17b3ea..87cf11f73 100644 --- a/modules/ui/src/app/services/testrun-dialog.service.spec.ts +++ b/modules/ui/src/app/services/testrun-dialog.service.spec.ts @@ -1,4 +1,4 @@ -import { TestBed, fakeAsync } from '@angular/core/testing'; +import { TestBed, fakeAsync, tick } from '@angular/core/testing'; import { TestrunDialogService } from './testrun-dialog.service'; import { MatDialogRef } from '@angular/material/dialog'; @@ -30,6 +30,7 @@ describe('TestrunDialogService', () => { }); service = TestBed.inject(TestrunDialogService); store = TestBed.inject(MockStore); + stateServiceMock.focusFirstElementInContainer.calls.reset(); spyOn(store, 'dispatch').and.callThrough(); }); @@ -73,4 +74,30 @@ describe('TestrunDialogService', () => { openSpy.calls.reset(); })); }); + + describe('#handleFocus', () => { + it('should focus the first element when no dialog is open', fakeAsync(() => { + spyOnProperty(service.dialog, 'openDialogs', 'get').and.returnValue([]); + + service.handleFocus(0); + tick(1); + + expect( + stateServiceMock.focusFirstElementInContainer + ).toHaveBeenCalledTimes(1); + })); + + it('should not steal focus while a dialog is open', fakeAsync(() => { + spyOnProperty(service.dialog, 'openDialogs', 'get').and.returnValue([ + {} as MatDialogRef, + ]); + + service.handleFocus(0); + tick(1); + + expect( + stateServiceMock.focusFirstElementInContainer + ).not.toHaveBeenCalled(); + })); + }); }); diff --git a/modules/ui/src/app/services/testrun-dialog.service.ts b/modules/ui/src/app/services/testrun-dialog.service.ts index 0977caab5..a7cb6aa05 100644 --- a/modules/ui/src/app/services/testrun-dialog.service.ts +++ b/modules/ui/src/app/services/testrun-dialog.service.ts @@ -47,6 +47,11 @@ export class TestrunDialogService { handleFocus(delay = 100): void { timer(delay).subscribe(() => { + // Never steal focus from a dialog that is still open (e.g. the blocking + // MQTT error dialog). + if (this.dialog.openDialogs.length > 0) { + return; + } this.focusManagerService.focusFirstElementInContainer(); }); } diff --git a/modules/ui/src/app/store/effects.spec.ts b/modules/ui/src/app/store/effects.spec.ts index 2b23ae542..c14e1fce8 100644 --- a/modules/ui/src/app/store/effects.spec.ts +++ b/modules/ui/src/app/store/effects.spec.ts @@ -16,6 +16,7 @@ import { discardPeriodicTasks, fakeAsync, + flushMicrotasks, TestBed, tick, } from '@angular/core/testing'; @@ -46,9 +47,11 @@ import { NotificationService } from '../services/notification.service'; import { PROFILE_MOCK } from '../mocks/profile.mock'; import { throwError } from 'rxjs/internal/observable/throwError'; import { HttpErrorResponse } from '@angular/common/http'; -import { IDLE_STATUS } from '../model/testrun-status'; +import { IDLE_STATUS, StatusOfTestrun } from '../model/testrun-status'; import { HISTORY } from '../mocks/reports.mock'; import { TestRunMqttService } from '../services/test-run-mqtt.service'; +import { MqttErrorDialogService } from '../services/mqtt-error-dialog.service'; +import { LoaderService } from '../services/loader.service'; describe('Effects', () => { let actions$ = new Observable(); @@ -60,11 +63,20 @@ describe('Effects', () => { jasmine.createSpyObj('notificationServiceMock', [ 'dismissWithTimout', 'openSnackBar', + 'dismissSnackBar', ]); + const mqttErrorDialogServiceMock: jasmine.SpyObj = + jasmine.createSpyObj('mqttErrorDialogServiceMock', ['open', 'isOpen']); + const loaderServiceMock: jasmine.SpyObj = jasmine.createSpyObj( + 'loaderServiceMock', + ['setLoading'] + ); const mockMqttService: jasmine.SpyObj = jasmine.createSpyObj('mockMqttService', [ 'getStatus', 'getInternetConnection', + 'getConnectionState', + 'isConnected', ]); beforeEach(() => { @@ -93,6 +105,11 @@ describe('Effects', () => { mockMqttService.getStatus.and.returnValue( of(MOCK_PROGRESS_DATA_IN_PROGRESS) ); + mockMqttService.getConnectionState.and.returnValue(of(true)); + mockMqttService.isConnected.and.returnValue(true); + mqttErrorDialogServiceMock.open.calls.reset(); + loaderServiceMock.setLoading.calls.reset(); + notificationServiceMock.dismissSnackBar.calls.reset(); TestBed.configureTestingModule({ providers: [ @@ -100,6 +117,11 @@ describe('Effects', () => { { provide: TestRunService, useValue: testRunServiceMock }, { provide: NotificationService, useValue: notificationServiceMock }, { provide: TestRunMqttService, useValue: mockMqttService }, + { + provide: MqttErrorDialogService, + useValue: mqttErrorDialogServiceMock, + }, + { provide: LoaderService, useValue: loaderServiceMock }, provideMockActions(() => actions$), provideMockStore({}), ], @@ -291,6 +313,35 @@ describe('Effects', () => { expect(mockMqttService.getInternetConnection).toHaveBeenCalled(); }); }); + + it('should not open MQTT error dialog while the connection is up', fakeAsync(() => { + effects.onFetchSystemStatusSuccess$.subscribe(); + flushMicrotasks(); + + expect(mqttErrorDialogServiceMock.open).not.toHaveBeenCalled(); + expect(dispatchSpy).not.toHaveBeenCalledWith(actions.stopInterval()); + })); + + it('should stop tracking and open MQTT error dialog when MQTT connection drops mid-test', fakeAsync(() => { + mockMqttService.getConnectionState.and.returnValue(of(false)); + + effects.onFetchSystemStatusSuccess$.subscribe(); + flushMicrotasks(); + + expect(dispatchSpy).toHaveBeenCalledWith(actions.stopInterval()); + expect(notificationServiceMock.dismissSnackBar).toHaveBeenCalled(); + expect(loaderServiceMock.setLoading).toHaveBeenCalledWith(false); + expect(dispatchSpy).toHaveBeenCalledWith( + actions.setIsTestingComplete({ isTestingComplete: false }) + ); + expect(dispatchSpy).toHaveBeenCalledWith( + setStatus({ status: StatusOfTestrun.Idle }) + ); + expect(dispatchSpy).toHaveBeenCalledWith( + actions.setTestrunStatus({ systemStatus: IDLE_STATUS }) + ); + expect(mqttErrorDialogServiceMock.open).toHaveBeenCalledTimes(1); + })); }); describe('with status "waiting for device"', () => { diff --git a/modules/ui/src/app/store/effects.ts b/modules/ui/src/app/store/effects.ts index 233e01f4a..54ebbdbff 100644 --- a/modules/ui/src/app/store/effects.ts +++ b/modules/ui/src/app/store/effects.ts @@ -23,7 +23,9 @@ import * as AppActions from './actions'; import { AppState } from './state'; import { TestRunService } from '../services/test-run.service'; import { + asapScheduler, filter, + observeOn, Subject, timer, take, @@ -51,6 +53,8 @@ import { NotificationService } from '../services/notification.service'; import { Profile } from '../model/profile'; import { DeviceStatus } from '../model/device'; import { TestRunMqttService } from '../services/test-run-mqtt.service'; +import { MqttErrorDialogService } from '../services/mqtt-error-dialog.service'; +import { LoaderService } from '../services/loader.service'; import { InternetConnection } from '../model/topic'; import { SystemConfig, SystemInterfaces } from '../model/setting'; @@ -61,12 +65,15 @@ export class AppEffects { private actions$ = inject(Actions); private testrunService = inject(TestRunService); private testrunMqttService = inject(TestRunMqttService); + private mqttErrorDialogService = inject(MqttErrorDialogService); + private loaderService = inject(LoaderService); private store = inject>(Store); private notificationService = inject(NotificationService); private isSinglePortMode: boolean | undefined = false; private statusSubscription: Subscription | undefined; private internetSubscription: Subscription | undefined; + private connectionSubscription: Subscription | undefined; private destroyWaitDeviceInterval$: Subject = new Subject(); onFetchSystemConfigSuccess$ = createEffect(() => { @@ -177,6 +184,7 @@ export class AppEffects { tap(() => { this.statusSubscription?.unsubscribe(); this.internetSubscription?.unsubscribe(); + this.connectionSubscription?.unsubscribe(); }) ); }, @@ -352,6 +360,45 @@ export class AppEffects { this.store.dispatch(fetchSystemStatusSuccess({ systemStatus })); }); } + if ( + this.connectionSubscription === undefined || + this.connectionSubscription?.closed + ) { + // Scenario 2: MQTT drops while a testrun is active. One-shot: the + // application has to be restarted to recover. Handled asynchronously so + // that an already-lost connection is processed after the current status + // update has been fully applied (and can then be cleaned up). + this.connectionSubscription = this.testrunMqttService + .getConnectionState() + .pipe( + filter(isConnected => !isConnected), + take(1), + observeOn(asapScheduler) + ) + .subscribe(() => { + this.handleMqttConnectionLoss(); + }); + } + } + + /** + * Stops tracking the testrun immediately (no orphaned subscriptions, + * timers, snackbars or spinners), resets the status to idle and surfaces the + * blocking MQTT error dialog. + */ + private handleMqttConnectionLoss(): void { + this.store.dispatch(stopInterval()); + this.destroyWaitDeviceInterval$.next(true); + this.notificationService.dismissSnackBar(); + this.loaderService.setLoading(false); + this.store.dispatch( + setIsTestingComplete({ + isTestingComplete: false, + }) + ); + this.store.dispatch(setStatus({ status: StatusOfTestrun.Idle })); + this.store.dispatch(setTestrunStatus({ systemStatus: IDLE_STATUS })); + this.mqttErrorDialogService.open(); } private fetchInternetConnection() {