diff --git a/src/angular/src/app/pages/files/bulk-action-bar.component.spec.ts b/src/angular/src/app/pages/files/bulk-action-bar.component.spec.ts index f04b068f..1e9a55e3 100644 --- a/src/angular/src/app/pages/files/bulk-action-bar.component.spec.ts +++ b/src/angular/src/app/pages/files/bulk-action-bar.component.spec.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { TestBed, ComponentFixture } from '@angular/core/testing'; import { BulkActionBarComponent } from './bulk-action-bar.component'; @@ -65,24 +65,32 @@ describe('BulkActionBarComponent', () => { expect(emitted).toBe(true); }); - it('should emit deleteLocalEvent when Delete Local button is clicked', () => { - let emitted = false; - component.deleteLocalEvent.subscribe(() => (emitted = true)); + it('should emit deleteLocalEvent only on the second Delete Local click', () => { + let emitCount = 0; + component.deleteLocalEvent.subscribe(() => (emitCount += 1)); - const btn = findButtonByText('Delete Local'); - btn.click(); + // First click arms the confirm, does NOT emit + findButtonByText('Delete Local').click(); + expect(emitCount).toBe(0); - expect(emitted).toBe(true); + // Second click emits + fixture.detectChanges(); + findButtonByText('Confirm?').click(); + expect(emitCount).toBe(1); }); - it('should emit deleteRemoteEvent when Delete Remote button is clicked', () => { - let emitted = false; - component.deleteRemoteEvent.subscribe(() => (emitted = true)); + it('should emit deleteRemoteEvent only on the second Delete Remote click', () => { + let emitCount = 0; + component.deleteRemoteEvent.subscribe(() => (emitCount += 1)); - const btn = findButtonByText('Delete Remote'); - btn.click(); + // First click arms the confirm, does NOT emit + findButtonByText('Delete Remote').click(); + expect(emitCount).toBe(0); - expect(emitted).toBe(true); + // Second click emits + fixture.detectChanges(); + findButtonByText('Confirm?').click(); + expect(emitCount).toBe(1); }); it('should emit clearEvent when Clear button is clicked', () => { @@ -100,3 +108,111 @@ describe('BulkActionBarComponent', () => { expect(buttons.length).toBe(5); }); }); + +describe('BulkActionBarComponent inline bulk delete confirmation', () => { + let fixture: ComponentFixture; + let component: BulkActionBarComponent; + + beforeEach(async () => { + vi.useFakeTimers(); + + await TestBed.configureTestingModule({ + imports: [BulkActionBarComponent], + }).compileComponents(); + + fixture = TestBed.createComponent(BulkActionBarComponent); + fixture.componentRef.setInput('count', 3); + fixture.detectChanges(); + component = fixture.componentInstance; + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + function findButtonByText(text: string): HTMLButtonElement { + const buttons = Array.from( + fixture.nativeElement.querySelectorAll('button'), + ) as HTMLButtonElement[]; + const btn = buttons.find((el) => el.textContent?.includes(text)); + expect(btn).toBeTruthy(); + return btn!; + } + + it('first click on Delete Local sets confirming state and does not emit', () => { + const spy = vi.spyOn(component.deleteLocalEvent, 'emit'); + + component.onDeleteLocal(); + + expect(component.confirmingDelete).toBe('local'); + expect(spy).not.toHaveBeenCalled(); + }); + + it('second click on Delete Local emits event and clears state', () => { + const spy = vi.spyOn(component.deleteLocalEvent, 'emit'); + + component.onDeleteLocal(); + expect(component.confirmingDelete).toBe('local'); + + component.onDeleteLocal(); + expect(component.confirmingDelete).toBeNull(); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('first click on Delete Remote sets confirming state and does not emit', () => { + const spy = vi.spyOn(component.deleteRemoteEvent, 'emit'); + + component.onDeleteRemote(); + + expect(component.confirmingDelete).toBe('remote'); + expect(spy).not.toHaveBeenCalled(); + }); + + it('second click on Delete Remote emits event and clears state', () => { + const spy = vi.spyOn(component.deleteRemoteEvent, 'emit'); + + component.onDeleteRemote(); + expect(component.confirmingDelete).toBe('remote'); + + component.onDeleteRemote(); + expect(component.confirmingDelete).toBeNull(); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('confirming state auto-resets after 3 seconds', () => { + component.onDeleteLocal(); + expect(component.confirmingDelete).toBe('local'); + + vi.advanceTimersByTime(3000); + expect(component.confirmingDelete).toBeNull(); + }); + + it('clicking Delete Local while confirming remote switches to local', () => { + component.onDeleteRemote(); + expect(component.confirmingDelete).toBe('remote'); + + component.onDeleteLocal(); + expect(component.confirmingDelete).toBe('local'); + }); + + it("button label switches to 'Confirm?' after first Delete Local click", () => { + // Drive through a real DOM click so OnPush marks the view dirty. + findButtonByText('Delete Local').click(); + fixture.detectChanges(); + + expect(component.confirmingDelete).toBe('local'); + const btn = findButtonByText('Confirm?'); + expect(btn.textContent).toContain('Confirm?'); + }); + + it('ngOnDestroy clears the confirm timer so no reset fires', () => { + component.onDeleteLocal(); + expect(component.confirmingDelete).toBe('local'); + + component.ngOnDestroy(); + vi.advanceTimersByTime(5000); + + // State stays as-is (timer was cleared, no reset happened) + expect(component.confirmingDelete).toBe('local'); + }); +}); diff --git a/src/angular/src/app/pages/files/bulk-action-bar.component.ts b/src/angular/src/app/pages/files/bulk-action-bar.component.ts index 266488be..57b76cfa 100644 --- a/src/angular/src/app/pages/files/bulk-action-bar.component.ts +++ b/src/angular/src/app/pages/files/bulk-action-bar.component.ts @@ -1,4 +1,6 @@ -import { Component, ChangeDetectionStrategy, input, output } from '@angular/core'; +import { + Component, ChangeDetectionStrategy, ChangeDetectorRef, OnDestroy, input, output, inject +} from '@angular/core'; @Component({ selector: 'app-bulk-action-bar', @@ -8,8 +10,16 @@ import { Component, ChangeDetectionStrategy, input, output } from '@angular/core {{ count() }} selected - - + + `, @@ -26,10 +36,16 @@ import { Component, ChangeDetectionStrategy, input, output } from '@angular/core font-weight: bold; margin-right: 8px; } + .btn.confirming { + background-color: var(--bs-danger); + color: #fff; + } `], changeDetection: ChangeDetectionStrategy.OnPush }) -export class BulkActionBarComponent { +export class BulkActionBarComponent implements OnDestroy { + private readonly cdr = inject(ChangeDetectorRef); + count = input.required(); queueEvent = output(); @@ -37,4 +53,49 @@ export class BulkActionBarComponent { deleteLocalEvent = output(); deleteRemoteEvent = output(); clearEvent = output(); + + // Inline double-click delete confirmation state + confirmingDelete: 'local' | 'remote' | null = null; + private confirmResetTimer: ReturnType | null = null; + + ngOnDestroy(): void { + this.clearConfirmTimer(); + } + + onDeleteLocal(): void { + if (this.confirmingDelete === 'local') { + this.clearConfirmTimer(); + this.confirmingDelete = null; + this.deleteLocalEvent.emit(); + } else { + this.setConfirming('local'); + } + } + + onDeleteRemote(): void { + if (this.confirmingDelete === 'remote') { + this.clearConfirmTimer(); + this.confirmingDelete = null; + this.deleteRemoteEvent.emit(); + } else { + this.setConfirming('remote'); + } + } + + private setConfirming(type: 'local' | 'remote'): void { + this.clearConfirmTimer(); + this.confirmingDelete = type; + this.confirmResetTimer = setTimeout(() => { + this.confirmingDelete = null; + this.confirmResetTimer = null; + this.cdr.markForCheck(); + }, 3000); + } + + private clearConfirmTimer(): void { + if (this.confirmResetTimer !== null) { + clearTimeout(this.confirmResetTimer); + this.confirmResetTimer = null; + } + } } diff --git a/src/angular/src/app/pages/files/file-list.component.spec.ts b/src/angular/src/app/pages/files/file-list.component.spec.ts index a358606c..f13d9ba4 100644 --- a/src/angular/src/app/pages/files/file-list.component.spec.ts +++ b/src/angular/src/app/pages/files/file-list.component.spec.ts @@ -1,15 +1,18 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { TestBed, ComponentFixture } from '@angular/core/testing'; -import { BehaviorSubject, Observable, of, EMPTY } from 'rxjs'; +import { BehaviorSubject, Observable, of, EMPTY, throwError } from 'rxjs'; import { ScrollingModule } from '@angular/cdk/scrolling'; import { FileListComponent } from './file-list.component'; import { ViewFileService } from '../../services/files/view-file.service'; import { ViewFileOptionsService } from '../../services/files/view-file-options.service'; import { LoggerService } from '../../services/utils/logger.service'; +import { NotificationService } from '../../services/utils/notification.service'; +import { NotificationLevel } from '../../models/notification'; import { ViewFile, ViewFileStatus } from '../../models/view-file'; import { ViewFileOptions, SortMethod } from '../../models/view-file-options'; import { fileKey } from '../../services/files/file-key'; +import { FileActionEvent } from './file.component'; interface MockViewFileService { filteredFiles$: Observable; @@ -32,6 +35,15 @@ interface MockViewFileService { bulkDeleteRemote: ReturnType; } +interface MockNotificationService { + show: ReturnType; + hide: ReturnType; +} + +function makeActionEvent(file: ViewFile): FileActionEvent { + return { file, clearActiveAction: vi.fn() }; +} + function makeViewFile(overrides: Partial = {}): ViewFile { return { name: 'test.txt', @@ -71,6 +83,7 @@ describe('FileListComponent', () => { let checkedSubject: BehaviorSubject>; let optionsSubject: BehaviorSubject; let mockViewFileService: MockViewFileService; + let mockNotifService: MockNotificationService; beforeEach(async () => { filteredFilesSubject = new BehaviorSubject([]); @@ -104,12 +117,15 @@ describe('FileListComponent', () => { bulkDeleteRemote: vi.fn().mockReturnValue(EMPTY), }; + mockNotifService = { show: vi.fn(), hide: vi.fn() }; + await TestBed.configureTestingModule({ imports: [FileListComponent, ScrollingModule], providers: [ { provide: ViewFileService, useValue: mockViewFileService }, { provide: ViewFileOptionsService, useValue: { options$: optionsSubject.asObservable() } }, { provide: LoggerService, useValue: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() } }, + { provide: NotificationService, useValue: mockNotifService }, ], }).compileComponents(); @@ -269,7 +285,7 @@ describe('FileListComponent', () => { const file = makeViewFile({ name: 'queue-me.txt' }); mockViewFileService.queue.mockReturnValue(of({ success: true, data: 'ok', errorMessage: null })); - component.onQueue(file); + component.onQueue(makeActionEvent(file)); expect(mockViewFileService.queue).toHaveBeenCalledWith(file); }); @@ -278,7 +294,7 @@ describe('FileListComponent', () => { const file = makeViewFile({ name: 'stop-me.txt' }); mockViewFileService.stop.mockReturnValue(of({ success: true, data: 'ok', errorMessage: null })); - component.onStop(file); + component.onStop(makeActionEvent(file)); expect(mockViewFileService.stop).toHaveBeenCalledWith(file); }); @@ -287,7 +303,7 @@ describe('FileListComponent', () => { const file = makeViewFile({ name: 'extract-me.txt' }); mockViewFileService.extract.mockReturnValue(of({ success: true, data: 'ok', errorMessage: null })); - component.onExtract(file); + component.onExtract(makeActionEvent(file)); expect(mockViewFileService.extract).toHaveBeenCalledWith(file); }); @@ -296,7 +312,7 @@ describe('FileListComponent', () => { const file = makeViewFile({ name: 'validate-me.txt' }); mockViewFileService.validate.mockReturnValue(of({ success: true, data: 'ok', errorMessage: null })); - component.onValidate(file); + component.onValidate(makeActionEvent(file)); expect(mockViewFileService.validate).toHaveBeenCalledWith(file); }); @@ -305,7 +321,7 @@ describe('FileListComponent', () => { const file = makeViewFile({ name: 'del-local.txt' }); mockViewFileService.deleteLocal.mockReturnValue(of({ success: true, data: 'ok', errorMessage: null })); - component.onDeleteLocal(file); + component.onDeleteLocal(makeActionEvent(file)); expect(mockViewFileService.deleteLocal).toHaveBeenCalledWith(file); }); @@ -314,11 +330,69 @@ describe('FileListComponent', () => { const file = makeViewFile({ name: 'del-remote.txt' }); mockViewFileService.deleteRemote.mockReturnValue(of({ success: true, data: 'ok', errorMessage: null })); - component.onDeleteRemote(file); + component.onDeleteRemote(makeActionEvent(file)); expect(mockViewFileService.deleteRemote).toHaveBeenCalledWith(file); }); + // --- Single-file action error surfacing (#513) --- + + it('shows a DANGER banner and clears activeAction when an action fails', () => { + const file = makeViewFile({ name: 'fail-me.txt' }); + const event = makeActionEvent(file); + mockViewFileService.queue.mockReturnValue( + of({ success: false, data: null, errorMessage: 'Boom' }), + ); + + component.onQueue(event); + + expect(mockNotifService.show).toHaveBeenCalledTimes(1); + const notif = mockNotifService.show.mock.calls[0][0]; + expect(notif.level).toBe(NotificationLevel.DANGER); + expect(notif.text).toContain('Boom'); + expect(event.clearActiveAction).toHaveBeenCalledTimes(1); + }); + + it('falls back to a generic message when a failed reaction has no errorMessage', () => { + const event = makeActionEvent(makeViewFile()); + mockViewFileService.stop.mockReturnValue( + of({ success: false, data: null, errorMessage: null }), + ); + + component.onStop(event); + + expect(mockNotifService.show).toHaveBeenCalledTimes(1); + expect(mockNotifService.show.mock.calls[0][0].text).toBe('Action failed'); + expect(event.clearActiveAction).toHaveBeenCalledTimes(1); + }); + + it('does not show a banner or clear activeAction on a successful action', () => { + const logger = TestBed.inject(LoggerService); + const event = makeActionEvent(makeViewFile()); + mockViewFileService.validate.mockReturnValue( + of({ success: true, data: 'ok', errorMessage: null }), + ); + + component.onValidate(event); + + expect(mockNotifService.show).not.toHaveBeenCalled(); + expect(event.clearActiveAction).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledWith('ok'); + }); + + it('shows a DANGER banner and clears activeAction when the action stream errors', () => { + const event = makeActionEvent(makeViewFile()); + mockViewFileService.deleteRemote.mockReturnValue( + throwError(() => new Error('net')), + ); + + component.onDeleteRemote(event); + + expect(mockNotifService.show).toHaveBeenCalledTimes(1); + expect(mockNotifService.show.mock.calls[0][0].level).toBe(NotificationLevel.DANGER); + expect(event.clearActiveAction).toHaveBeenCalledTimes(1); + }); + // --- Bulk actions --- it('should call viewFileService.bulkQueue on onBulkQueue', () => { @@ -367,7 +441,7 @@ describe('FileListComponent', () => { // --- Bulk response handling --- - it('should log failures from bulk action responses', () => { + it('should log failures and show a DANGER banner from bulk action responses', () => { const logger = TestBed.inject(LoggerService); mockViewFileService.bulkQueue.mockReturnValue(of([ { success: true, data: 'ok', errorMessage: null }, @@ -378,6 +452,21 @@ describe('FileListComponent', () => { expect(logger.error).toHaveBeenCalled(); expect(logger.warn).toHaveBeenCalled(); + expect(mockNotifService.show).toHaveBeenCalledTimes(1); + const notif = mockNotifService.show.mock.calls[0][0]; + expect(notif.level).toBe(NotificationLevel.DANGER); + expect(notif.text).toBe('1 of 2 items failed'); + }); + + it('should not show a banner when all bulk items succeed', () => { + mockViewFileService.bulkDeleteLocal.mockReturnValue(of([ + { success: true, data: 'ok', errorMessage: null }, + { success: true, data: 'ok2', errorMessage: null }, + ])); + + component.onBulkDeleteLocal(); + + expect(mockNotifService.show).not.toHaveBeenCalled(); }); it('should log info for successful bulk items', () => { diff --git a/src/angular/src/app/pages/files/file-list.component.ts b/src/angular/src/app/pages/files/file-list.component.ts index 75951c5e..6e38f588 100644 --- a/src/angular/src/app/pages/files/file-list.component.ts +++ b/src/angular/src/app/pages/files/file-list.component.ts @@ -20,8 +20,10 @@ import { ViewFile } from '../../models/view-file'; import { ViewFileOptions } from '../../models/view-file-options'; import { ViewFileOptionsService } from '../../services/files/view-file-options.service'; import { LoggerService } from '../../services/utils/logger.service'; +import { NotificationService } from '../../services/utils/notification.service'; +import { NotificationLevel, createNotification } from '../../models/notification'; import { fileKey } from '../../services/files/file-key'; -import { FileComponent } from './file.component'; +import { FileComponent, FileActionEvent } from './file.component'; import { BulkActionBarComponent } from './bulk-action-bar.component'; @Component({ @@ -38,6 +40,7 @@ export class FileListComponent implements AfterViewInit, OnDestroy { private readonly logger = inject(LoggerService); private readonly viewFileService = inject(ViewFileService); private readonly viewFileOptionsService = inject(ViewFileOptionsService); + private readonly notifService = inject(NotificationService); private readonly destroyRef = inject(DestroyRef); private readonly elRef = inject>(ElementRef); private readonly zone = inject(NgZone); @@ -121,54 +124,82 @@ export class FileListComponent implements AfterViewInit, OnDestroy { } } - onQueue(file: ViewFile): void { - this.viewFileService.queue(file).pipe( + onQueue(event: FileActionEvent): void { + this.viewFileService.queue(event.file).pipe( takeUntilDestroyed(this.destroyRef), - ).subscribe(data => { - this.logger.info(data); + ).subscribe({ + next: (data) => this.handleActionResponse(data, event), + error: (err) => this.handleActionError(err, event), }); } - onStop(file: ViewFile): void { - this.viewFileService.stop(file).pipe( + onStop(event: FileActionEvent): void { + this.viewFileService.stop(event.file).pipe( takeUntilDestroyed(this.destroyRef), - ).subscribe(data => { - this.logger.info(data); + ).subscribe({ + next: (data) => this.handleActionResponse(data, event), + error: (err) => this.handleActionError(err, event), }); } - onExtract(file: ViewFile): void { - this.viewFileService.extract(file).pipe( + onExtract(event: FileActionEvent): void { + this.viewFileService.extract(event.file).pipe( takeUntilDestroyed(this.destroyRef), - ).subscribe(data => { - this.logger.info(data); + ).subscribe({ + next: (data) => this.handleActionResponse(data, event), + error: (err) => this.handleActionError(err, event), }); } - onValidate(file: ViewFile): void { - this.viewFileService.validate(file).pipe( + onValidate(event: FileActionEvent): void { + this.viewFileService.validate(event.file).pipe( takeUntilDestroyed(this.destroyRef), - ).subscribe(data => { - this.logger.info(data); + ).subscribe({ + next: (data) => this.handleActionResponse(data, event), + error: (err) => this.handleActionError(err, event), }); } - onDeleteLocal(file: ViewFile): void { - this.viewFileService.deleteLocal(file).pipe( + onDeleteLocal(event: FileActionEvent): void { + this.viewFileService.deleteLocal(event.file).pipe( takeUntilDestroyed(this.destroyRef), - ).subscribe(data => { - this.logger.info(data); + ).subscribe({ + next: (data) => this.handleActionResponse(data, event), + error: (err) => this.handleActionError(err, event), }); } - onDeleteRemote(file: ViewFile): void { - this.viewFileService.deleteRemote(file).pipe( + onDeleteRemote(event: FileActionEvent): void { + this.viewFileService.deleteRemote(event.file).pipe( takeUntilDestroyed(this.destroyRef), - ).subscribe(data => { - this.logger.info(data); + ).subscribe({ + next: (data) => this.handleActionResponse(data, event), + error: (err) => this.handleActionError(err, event), }); } + private handleActionResponse(reaction: WebReaction, event: FileActionEvent): void { + if (reaction.success) { + this.logger.info(reaction.data); + } else { + this.failAction(reaction.errorMessage ?? 'Action failed', event); + } + } + + private handleActionError(err: unknown, event: FileActionEvent): void { + this.failAction('Action failed', event); + this.logger.error('Action failed:', err); + } + + // A backend rejection leaves the file model unchanged, so the child's + // ngOnChanges can't recover the row. Surface the error and clear the child's + // activeAction so the buttons re-enable and the spinner stops without a reload. + private failAction(text: string, event: FileActionEvent): void { + this.notifService.show(createNotification(NotificationLevel.DANGER, text, true)); + event.clearActiveAction(); + this.logger.error(text); + } + onCheck(event: {file: ViewFile, shiftKey: boolean}): void { if (event.shiftKey) { this.viewFileService.shiftCheck(event.file); @@ -206,6 +237,11 @@ export class FileListComponent implements AfterViewInit, OnDestroy { }); if (failures > 0) { this.logger.warn(`Bulk action: ${failures} of ${reactions.length} items failed`); + this.notifService.show(createNotification( + NotificationLevel.DANGER, + `${failures} of ${reactions.length} items failed`, + true, + )); } }, error: (err) => this.logger.error('Bulk action failed:', err), diff --git a/src/angular/src/app/pages/files/file.component.spec.ts b/src/angular/src/app/pages/files/file.component.spec.ts index abbb184f..07aa270a 100644 --- a/src/angular/src/app/pages/files/file.component.spec.ts +++ b/src/angular/src/app/pages/files/file.component.spec.ts @@ -175,7 +175,7 @@ describe('FileComponent inline delete confirmation', () => { component.onDeleteLocal(file); expect(component.confirmingDelete).toBeNull(); expect(component.activeAction).toBe(FileAction.DELETE_LOCAL); - expect(spy).toHaveBeenCalledWith(file); + expect(spy).toHaveBeenCalledWith(expect.objectContaining({ file })); }); it('first click on delete remote sets confirming state', () => { @@ -193,7 +193,7 @@ describe('FileComponent inline delete confirmation', () => { expect(component.confirmingDelete).toBeNull(); expect(component.activeAction).toBe(FileAction.DELETE_REMOTE); - expect(spy).toHaveBeenCalledWith(file); + expect(spy).toHaveBeenCalledWith(expect.objectContaining({ file })); }); it('confirming state auto-resets after 3 seconds', () => { @@ -256,3 +256,66 @@ describe('FileComponent inline delete confirmation', () => { expect(component.confirmingDelete).toBe('local'); }); }); + +describe('FileComponent action events', () => { + let fixture: ComponentFixture; + let component: FileComponent; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [FileComponent], + }).compileComponents(); + + fixture = TestBed.createComponent(FileComponent); + fixture.componentRef.setInput('file', makeViewFile()); + fixture.componentRef.setInput('options', of({ nameFilter: '', statusFilter: '' })); + fixture.detectChanges(); + component = fixture.componentInstance; + }); + + it('clearActiveAction resets activeAction to null', () => { + component.activeAction = FileAction.QUEUE; + component.clearActiveAction(); + expect(component.activeAction).toBeNull(); + }); + + it.each([ + ['onQueue', 'queueEvent'], + ['onStop', 'stopEvent'], + ['onExtract', 'extractEvent'], + ['onValidate', 'validateEvent'], + ] as const)( + '%s emits an event carrying the file and a clearActiveAction callback', + (method, eventName) => { + const file = makeViewFile(); + const spy = vi.spyOn(component[eventName], 'emit'); + + component[method](file); + + expect(spy).toHaveBeenCalledTimes(1); + const payload = spy.mock.calls[0][0]; + expect(payload.file).toBe(file); + expect(typeof payload.clearActiveAction).toBe('function'); + + // Action is active until the callback fires. + expect(component.activeAction).not.toBeNull(); + payload.clearActiveAction(); + expect(component.activeAction).toBeNull(); + }, + ); + + it('delete emits a clearActiveAction callback that resets activeAction', () => { + const file = makeViewFile(); + const spy = vi.spyOn(component.deleteLocalEvent, 'emit'); + + // Double-click to confirm and emit. + component.onDeleteLocal(file); + component.onDeleteLocal(file); + + expect(component.activeAction).toBe(FileAction.DELETE_LOCAL); + const payload = spy.mock.calls[0][0]; + expect(payload.file).toBe(file); + payload.clearActiveAction(); + expect(component.activeAction).toBeNull(); + }); +}); diff --git a/src/angular/src/app/pages/files/file.component.ts b/src/angular/src/app/pages/files/file.component.ts index 6ddd9da8..ebf2004e 100644 --- a/src/angular/src/app/pages/files/file.component.ts +++ b/src/angular/src/app/pages/files/file.component.ts @@ -21,6 +21,17 @@ export enum FileAction { DELETE_REMOTE } +// Payload emitted for each single-file action. Carries the target file plus a +// callback the parent invokes to clear this child's activeAction when the +// backend rejects the request — the file model never changes on failure, so +// ngOnChanges cannot recover the row on its own. The callback is necessary +// because is rendered inside *cdkVirtualFor, which recycles +// instances and prevents the parent from keying a @ViewChildren to a file. +export interface FileActionEvent { + file: ViewFile; + clearActiveAction: () => void; +} + @Component({ selector: 'app-file', standalone: true, @@ -48,12 +59,12 @@ export class FileComponent implements OnChanges, OnDestroy { options = input.required>(); checkEvent = output<{file: ViewFile, shiftKey: boolean}>(); - queueEvent = output(); - stopEvent = output(); - extractEvent = output(); - deleteLocalEvent = output(); - validateEvent = output(); - deleteRemoteEvent = output(); + queueEvent = output(); + stopEvent = output(); + extractEvent = output(); + deleteLocalEvent = output(); + validateEvent = output(); + deleteRemoteEvent = output(); activeAction: FileAction | null = null; @@ -128,24 +139,36 @@ export class FileComponent implements OnChanges, OnDestroy { return this.activeAction == null && this.file().isRemotelyDeletable; } + // Cleared by the parent when an action's backend request fails or errors. + // Runs inside the parent's async subscribe callback, so OnPush needs an + // explicit markForCheck to re-enable the buttons and stop the spinner. + clearActiveAction(): void { + this.activeAction = null; + this.cdr.markForCheck(); + } + + private actionEvent(file: ViewFile): FileActionEvent { + return { file, clearActiveAction: () => this.clearActiveAction() }; + } + onQueue(file: ViewFile): void { this.activeAction = FileAction.QUEUE; - this.queueEvent.emit(file); + this.queueEvent.emit(this.actionEvent(file)); } onStop(file: ViewFile): void { this.activeAction = FileAction.STOP; - this.stopEvent.emit(file); + this.stopEvent.emit(this.actionEvent(file)); } onExtract(file: ViewFile): void { this.activeAction = FileAction.EXTRACT; - this.extractEvent.emit(file); + this.extractEvent.emit(this.actionEvent(file)); } onValidate(file: ViewFile): void { this.activeAction = FileAction.VALIDATE; - this.validateEvent.emit(file); + this.validateEvent.emit(this.actionEvent(file)); } onDeleteLocal(file: ViewFile): void { @@ -153,7 +176,7 @@ export class FileComponent implements OnChanges, OnDestroy { this.clearConfirmTimer(); this.confirmingDelete = null; this.activeAction = FileAction.DELETE_LOCAL; - this.deleteLocalEvent.emit(file); + this.deleteLocalEvent.emit(this.actionEvent(file)); } else { this.setConfirming('local'); } @@ -164,7 +187,7 @@ export class FileComponent implements OnChanges, OnDestroy { this.clearConfirmTimer(); this.confirmingDelete = null; this.activeAction = FileAction.DELETE_REMOTE; - this.deleteRemoteEvent.emit(file); + this.deleteRemoteEvent.emit(this.actionEvent(file)); } else { this.setConfirming('remote'); } diff --git a/src/angular/src/app/pages/logs/logs-page.component.html b/src/angular/src/app/pages/logs/logs-page.component.html index e002f3ee..f6397869 100644 --- a/src/angular/src/app/pages/logs/logs-page.component.html +++ b/src/angular/src/app/pages/logs/logs-page.component.html @@ -23,7 +23,13 @@ - @if (historyLoaded && historyRecords.length > 0) { + @if (historyLoaded && historyLoadFailed) { +
+
Log History
+ +
+
+ } @else if (historyLoaded && historyRecords.length > 0) {
Log History
@for (entry of historyRecords; track $index) { diff --git a/src/angular/src/app/pages/logs/logs-page.component.scss b/src/angular/src/app/pages/logs/logs-page.component.scss index 51b55e90..353079d1 100644 --- a/src/angular/src/app/pages/logs/logs-page.component.scss +++ b/src/angular/src/app/pages/logs/logs-page.component.scss @@ -35,6 +35,13 @@ color: var(--ss-text-muted); margin-bottom: 5px; } + + // Distinct from p.record so log-record queries never match this banner. + .history-load-error { + color: var(--ss-log-error-text); + font-size: 90%; + margin: 0; + } } .log-divider { diff --git a/src/angular/src/app/pages/logs/logs-page.component.spec.ts b/src/angular/src/app/pages/logs/logs-page.component.spec.ts new file mode 100644 index 00000000..d109dc42 --- /dev/null +++ b/src/angular/src/app/pages/logs/logs-page.component.spec.ts @@ -0,0 +1,109 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { TestBed, ComponentFixture } from '@angular/core/testing'; +import { Subject, of, throwError } from 'rxjs'; + +import { LogsPageComponent } from './logs-page.component'; +import { LogService } from '../../services/logs/log.service'; +import { DomService } from '../../services/utils/dom.service'; +import { LoggerService } from '../../services/utils/logger.service'; + +function loggerMock() { + return { error: vi.fn(), debug: vi.fn(), info: vi.fn(), warn: vi.fn() }; +} + +/** + * Covers #516: a log-history fetch failure must render a distinct "failed to + * load" state, not be indistinguishable from an empty (successful) result. + * The debounced search pipe is not exercised here (the failure flag is set by + * its catchError); these tests verify the template renders each state. + */ +describe('LogsPageComponent history failure state (#516)', () => { + let fixture: ComponentFixture; + let component: LogsPageComponent; + + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [LogsPageComponent], + providers: [ + { + provide: LogService, + useValue: { logs$: new Subject(), fetchHistory: vi.fn().mockReturnValue(of([])) }, + }, + { provide: DomService, useValue: { headerHeight$: of(0) } }, + { provide: LoggerService, useValue: loggerMock() }, + ], + }); + fixture = TestBed.createComponent(LogsPageComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + afterEach(() => { + fixture.destroy(); + vi.restoreAllMocks(); + }); + + function renderedText(): string { + return (fixture.nativeElement as HTMLElement).textContent ?? ''; + } + + it('renders a distinct failure message when the history fetch failed', () => { + component.historyLoaded = true; + component.historyLoadFailed = true; + component.historyRecords = []; + fixture.detectChanges(); + + expect(renderedText()).toContain('Failed to load log history'); + }); + + it('shows no failure message for an empty (successful) result', () => { + component.historyLoaded = true; + component.historyLoadFailed = false; + component.historyRecords = []; + fixture.detectChanges(); + + expect(renderedText()).not.toContain('Failed to load log history'); + }); +}); + +/** + * Exercises the real debounced search pipe so the test fails if catchError stops + * setting the failure flag (not just the rendered template). Uses fake timers to + * flush the 300ms debounce deterministically. + */ +describe('LogsPageComponent history fetch failure (real pipe, #516)', () => { + let fixture: ComponentFixture; + let component: LogsPageComponent; + + beforeEach(() => { + vi.useFakeTimers(); + TestBed.configureTestingModule({ + imports: [LogsPageComponent], + providers: [ + { + provide: LogService, + useValue: { logs$: new Subject(), fetchHistory: vi.fn().mockReturnValue(throwError(() => new Error('boom'))) }, + }, + { provide: DomService, useValue: { headerHeight$: of(0) } }, + { provide: LoggerService, useValue: loggerMock() }, + ], + }); + fixture = TestBed.createComponent(LogsPageComponent); + component = fixture.componentInstance; + fixture.detectChanges(); // ngOnInit -> initial searchChange$.next() schedules the debounce + }); + + afterEach(() => { + fixture.destroy(); + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it('flips into the failure state when fetchHistory errors', () => { + vi.advanceTimersByTime(300); // flush debounce -> fetchHistory errors -> catchError + fixture.detectChanges(); + + expect(component.historyLoadFailed).toBe(true); + expect((fixture.nativeElement as HTMLElement).textContent).toContain('Failed to load log history'); + }); +}); diff --git a/src/angular/src/app/pages/logs/logs-page.component.ts b/src/angular/src/app/pages/logs/logs-page.component.ts index 56f83e11..c0929fad 100644 --- a/src/angular/src/app/pages/logs/logs-page.component.ts +++ b/src/angular/src/app/pages/logs/logs-page.component.ts @@ -21,6 +21,7 @@ import { of } from 'rxjs'; import { LogService, LogHistoryEntry } from '../../services/logs/log.service'; import { LogRecord, LogLevel } from '../../models/log-record'; import { DomService } from '../../services/utils/dom.service'; +import { LoggerService } from '../../services/utils/logger.service'; @Component({ selector: 'app-logs-page', @@ -38,6 +39,7 @@ export class LogsPageComponent implements OnInit, OnDestroy, AfterViewChecked { private readonly logService = inject(LogService); private readonly domService = inject(DomService); private readonly destroyRef = inject(DestroyRef); + private readonly logger = inject(LoggerService); readonly headerHeight$ = this.domService.headerHeight$; @@ -46,6 +48,7 @@ export class LogsPageComponent implements OnInit, OnDestroy, AfterViewChecked { searchQuery = ''; levelFilter = ''; historyLoaded = false; + historyLoadFailed = false; showScrollToTopButton = false; showScrollToBottomButton = false; @@ -78,13 +81,23 @@ export class LogsPageComponent implements OnInit, OnDestroy, AfterViewChecked { this.searchChange$.pipe( debounceTime(300), - switchMap(() => this.logService.fetchHistory({ - search: this.searchQuery || undefined, - level: this.levelFilter || undefined, - limit: 500, - }).pipe( - catchError(() => of([] as LogHistoryEntry[])) - )), + switchMap(() => { + // Reset the failure flag per search; catchError sets it on a real fetch + // failure so the template can distinguish "failed to load" from "no + // matching history" (both otherwise yield an empty list). + this.historyLoadFailed = false; + return this.logService.fetchHistory({ + search: this.searchQuery || undefined, + level: this.levelFilter || undefined, + limit: 500, + }).pipe( + catchError((err) => { + this.logger.error('Failed to load log history: %O', err); + this.historyLoadFailed = true; + return of([] as LogHistoryEntry[]); + }) + ); + }), takeUntilDestroyed(this.destroyRef), ).subscribe((entries) => { this.historyRecords = entries; diff --git a/src/angular/src/app/pages/settings/path-pairs.component.spec.ts b/src/angular/src/app/pages/settings/path-pairs.component.spec.ts index 7c4ca331..7b8daea0 100644 --- a/src/angular/src/app/pages/settings/path-pairs.component.spec.ts +++ b/src/angular/src/app/pages/settings/path-pairs.component.spec.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; -import { Observable, of } from 'rxjs'; +import { Observable, of, throwError } from 'rxjs'; import { PathPair } from '../../models/path-pair'; import { ArrInstance } from '../../models/arr-instance'; @@ -8,6 +8,7 @@ interface PathPairsServiceLike { create(pair: Omit): Observable; update(pair: PathPair): Observable; remove(pairId: string): Observable; + refresh(): void; } function makePair(overrides: Partial = {}): PathPair { @@ -47,6 +48,7 @@ class PathPairsLogic { addForm: Omit = this.emptyForm(); confirmingDeleteId: string | null = null; arrPickerPairId: string | null = null; + errorMessage: string | null = null; private confirmResetTimer: ReturnType | null = null; constructor(private service: PathPairsServiceLike) {} @@ -110,11 +112,27 @@ class PathPairsLogic { } onToggleEnabled(pair: PathPair, enabled: boolean): void { - this.service.update({ ...pair, enabled }).subscribe(); + this.applyToggle({ ...pair, enabled }); } onToggleAutoQueue(pair: PathPair, autoQueue: boolean): void { - this.service.update({ ...pair, auto_queue: autoQueue }).subscribe(); + this.applyToggle({ ...pair, auto_queue: autoQueue }); + } + + private applyToggle(updated: PathPair): void { + this.errorMessage = null; + this.service.update(updated).subscribe({ + next: (result) => { + if (!result) { + this.errorMessage = 'Failed to update path pair. Please try again.'; + this.service.refresh(); + } + }, + error: () => { + this.errorMessage = 'Failed to update path pair. Please try again.'; + this.service.refresh(); + }, + }); } // --- Arr targets --- @@ -200,6 +218,7 @@ describe('PathPairsComponent logic', () => { create: ReturnType; update: ReturnType; remove: ReturnType; + refresh: ReturnType; }; beforeEach(() => { @@ -207,6 +226,7 @@ describe('PathPairsComponent logic', () => { create: vi.fn().mockReturnValue(of(makePair({ id: '2', name: 'New' }))), update: vi.fn().mockReturnValue(of(makePair({ enabled: false }))), remove: vi.fn().mockReturnValue(of(true)), + refresh: vi.fn(), }; component = new PathPairsLogic(mockService as unknown as PathPairsServiceLike); }); @@ -288,6 +308,36 @@ describe('PathPairsComponent logic', () => { expect(component.editingId).toBe('p1'); }); + describe('toggle enable / auto-queue (#516)', () => { + it('clears the error and does not refresh on a successful toggle', () => { + mockService.update.mockReturnValue(of(makePair({ enabled: false }))); + + component.onToggleEnabled(makePair(), false); + + expect(mockService.update).toHaveBeenCalled(); + expect(component.errorMessage).toBeNull(); + expect(mockService.refresh).not.toHaveBeenCalled(); + }); + + it('sets an error and refreshes to reconcile when a toggle returns null', () => { + mockService.update.mockReturnValue(of(null)); + + component.onToggleAutoQueue(makePair(), true); + + expect(component.errorMessage).toBe('Failed to update path pair. Please try again.'); + expect(mockService.refresh).toHaveBeenCalledTimes(1); + }); + + it('sets an error and refreshes when a toggle errors (e.g. network failure)', () => { + mockService.update.mockReturnValue(throwError(() => new Error('network'))); + + component.onToggleEnabled(makePair(), true); + + expect(component.errorMessage).toBe('Failed to update path pair. Please try again.'); + expect(mockService.refresh).toHaveBeenCalledTimes(1); + }); + }); + describe('arr target attachment', () => { it('attachedInstances returns instances in the order of arr_target_ids', () => { const a = makeInstance({ id: 'a', name: 'A' }); diff --git a/src/angular/src/app/pages/settings/path-pairs.component.ts b/src/angular/src/app/pages/settings/path-pairs.component.ts index 6a0c37dc..b9c8e0ec 100644 --- a/src/angular/src/app/pages/settings/path-pairs.component.ts +++ b/src/angular/src/app/pages/settings/path-pairs.component.ts @@ -154,15 +154,34 @@ export class PathPairsComponent implements OnDestroy { // --- Toggle fields --- onToggleEnabled(pair: PathPair, enabled: boolean): void { - this.pathPairsService.update({ ...pair, enabled }).pipe( - takeUntilDestroyed(this.destroyRef), - ).subscribe(); + this.applyToggle({ ...pair, enabled }); } onToggleAutoQueue(pair: PathPair, autoQueue: boolean): void { - this.pathPairsService.update({ ...pair, auto_queue: autoQueue }).pipe( + this.applyToggle({ ...pair, auto_queue: autoQueue }); + } + + private applyToggle(updated: PathPair): void { + // The checkbox is bound one-way ([checked]="pair.enabled") with a (change) + // handler, so a failed update leaves it visually flipped vs the server. + // Surface an error and refresh() to reconcile the UI on any failure. + this.errorMessage = null; + this.pathPairsService.update(updated).pipe( takeUntilDestroyed(this.destroyRef), - ).subscribe(); + ).subscribe({ + next: (result) => { + if (!result) { + this.errorMessage = 'Failed to update path pair. Please try again.'; + this.pathPairsService.refresh(); + } + this.cdr.markForCheck(); + }, + error: () => { + this.errorMessage = 'Failed to update path pair. Please try again.'; + this.pathPairsService.refresh(); + this.cdr.markForCheck(); + }, + }); } // --- Arr targets --- diff --git a/src/angular/src/app/services/autoqueue/autoqueue.service.spec.ts b/src/angular/src/app/services/autoqueue/autoqueue.service.spec.ts index 7b1eb48e..354464a0 100644 --- a/src/angular/src/app/services/autoqueue/autoqueue.service.spec.ts +++ b/src/angular/src/app/services/autoqueue/autoqueue.service.spec.ts @@ -53,6 +53,18 @@ describe('AutoQueueService', () => { expect(snapshot()).toEqual([{ pattern: '*.mkv' }]); }); + it('should fall back to an empty list (and not throw) on malformed pattern JSON', () => { + mockRestService.sendRequest.mockReturnValue( + of(makeReaction({ success: true, data: 'not-json{' })), + ); + const logger = TestBed.inject(LoggerService); + + expect(() => connectedSubject.next(true)).not.toThrow(); + + expect(snapshot()).toEqual([]); + expect(vi.mocked(logger.error)).toHaveBeenCalled(); + }); + it('should clear patterns when disconnected', () => { mockRestService.sendRequest.mockReturnValue( of(makeReaction({ success: true, data: JSON.stringify([{ pattern: '*.mkv' }]) })), diff --git a/src/angular/src/app/services/autoqueue/autoqueue.service.ts b/src/angular/src/app/services/autoqueue/autoqueue.service.ts index b6402856..bd9484cb 100644 --- a/src/angular/src/app/services/autoqueue/autoqueue.service.ts +++ b/src/angular/src/app/services/autoqueue/autoqueue.service.ts @@ -105,11 +105,18 @@ export class AutoQueueService { this.logger.debug('Getting autoqueue patterns...'); this.restService.sendRequest(this.AUTOQUEUE_GET_URL).subscribe({ next: (reaction) => { - if (reaction.success) { + if (!reaction.success) { + this.patternsSubject.next([]); + return; + } + try { const parsed: AutoQueuePatternJson[] = JSON.parse(reaction.data!); const newPatterns: AutoQueuePattern[] = parsed.map((p) => ({ pattern: p.pattern })); this.patternsSubject.next(newPatterns); - } else { + } catch (e) { + // Malformed response must not throw inside the subscribe callback; + // log and fall back to an empty list (mirrors the SSE-handler guards). + this.logger.error('Failed to parse autoqueue patterns: %O', e); this.patternsSubject.next([]); } }, diff --git a/src/angular/src/app/services/base/stream-dispatch.service.spec.ts b/src/angular/src/app/services/base/stream-dispatch.service.spec.ts index ba459154..e478a245 100644 --- a/src/angular/src/app/services/base/stream-dispatch.service.spec.ts +++ b/src/angular/src/app/services/base/stream-dispatch.service.spec.ts @@ -82,6 +82,7 @@ describe("StreamDispatchService", () => { afterEach(() => { vi.useRealTimers(); + vi.restoreAllMocks(); (globalThis as unknown as { EventSource: typeof EventSource }).EventSource = originalEventSource; }); @@ -186,27 +187,100 @@ describe("StreamDispatchService", () => { // --- Reconnection --- - it("should reconnect after error with retry delay", () => { + it("should reconnect after error with the base backoff delay", () => { vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0); // no jitter service.start(); expect(MockEventSource.instances.length).toBe(1); latestEventSource().simulateError(); expect(MockEventSource.instances.length).toBe(1); - vi.advanceTimersByTime(3000); + vi.advanceTimersByTime(1000); expect(MockEventSource.instances.length).toBe(2); - }); - it("should not reconnect before retry interval", () => { + it("should not reconnect before the base backoff interval", () => { vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0); // no jitter service.start(); latestEventSource().simulateError(); - vi.advanceTimersByTime(2999); + vi.advanceTimersByTime(999); expect(MockEventSource.instances.length).toBe(1); + }); + + it("should use exponential backoff across successive errors", () => { + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0); // no jitter + service.start(); + + // First error -> reconnect after ~base (1000ms) + latestEventSource().simulateError(); + vi.advanceTimersByTime(1000); + expect(MockEventSource.instances.length).toBe(2); + + // Second error -> backoff doubles to ~2*base (2000ms) + latestEventSource().simulateError(); + vi.advanceTimersByTime(1000); + // base alone is not enough to reconnect now + expect(MockEventSource.instances.length).toBe(2); + vi.advanceTimersByTime(1000); + expect(MockEventSource.instances.length).toBe(3); + }); + + it("should cap the backoff at the maximum interval", () => { + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0); // no jitter + service.start(); + + // Fail many times so the exponential term would far exceed the cap. + for (let i = 0; i < 10; i++) { + latestEventSource().simulateError(); + vi.advanceTimersByTime(30000); // advance by the max each time + } + const countAfterCapping = MockEventSource.instances.length; + // One more error: even advancing only by the max must reconnect (capped). + latestEventSource().simulateError(); + vi.advanceTimersByTime(30000); + expect(MockEventSource.instances.length).toBe(countAfterCapping + 1); + }); + + it("should reset backoff to base after a successful open", () => { + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0); // no jitter + service.start(); + + // Fail a few times to grow the backoff. + latestEventSource().simulateError(); + vi.advanceTimersByTime(1000); // attempt 1: base + latestEventSource().simulateError(); + vi.advanceTimersByTime(2000); // attempt 2: 2*base + expect(MockEventSource.instances.length).toBe(3); + + // A successful open resets the backoff counter. + latestEventSource().simulateOpen(); + + // A fresh error should now reconnect after ~base again. + latestEventSource().simulateError(); + vi.advanceTimersByTime(1000); + expect(MockEventSource.instances.length).toBe(4); + }); + + it("should add jitter to the backoff delay", () => { + vi.useFakeTimers(); + // Math.random() returns [0, 1); use a value just below 1 (impossible: exactly 1). + vi.spyOn(Math, "random").mockReturnValue(0.999); // jitter ~999ms + service.start(); + + latestEventSource().simulateError(); + + // base (1000) + jitter (~999) = ~1999ms; base alone is not enough. + vi.advanceTimersByTime(1998); + expect(MockEventSource.instances.length).toBe(1); + vi.advanceTimersByTime(1); + expect(MockEventSource.instances.length).toBe(2); }); // --- API key --- diff --git a/src/angular/src/app/services/base/stream-dispatch.service.ts b/src/angular/src/app/services/base/stream-dispatch.service.ts index 81e7168c..9f50c79d 100644 --- a/src/angular/src/app/services/base/stream-dispatch.service.ts +++ b/src/angular/src/app/services/base/stream-dispatch.service.ts @@ -23,7 +23,12 @@ export interface StreamEventHandler { @Injectable({ providedIn: 'root' }) export class StreamDispatchService { private readonly STREAM_URL = '/server/stream'; - private readonly RETRY_INTERVAL_MS = 3000; + /** Base delay for the first reconnect attempt. */ + private readonly RETRY_BASE_MS = 1000; + /** Upper bound on the exponential backoff delay. */ + private readonly RETRY_MAX_MS = 30000; + /** Maximum random jitter added to each backoff delay. */ + private readonly RETRY_JITTER_MS = 1000; private readonly logger = inject(LoggerService); private readonly zone = inject(NgZone); @@ -45,6 +50,7 @@ export class StreamDispatchService { private apiKey: string | null = null; private eventSource: EventSource | null = null; private retryTimeout: ReturnType | null = null; + private retryAttempt = 0; setApiKey(key: string | null): void { if (key === this.apiKey) { @@ -53,6 +59,8 @@ export class StreamDispatchService { this.apiKey = key; // Reconnect with the new key if we have an active connection or pending retry if (this.eventSource || this.retryTimeout) { + // A key change should retry promptly, not wait out the current backoff. + this.retryAttempt = 0; this.closeAndCancelRetry(); this.connectStream(); } @@ -86,6 +94,8 @@ export class StreamDispatchService { eventSource.onopen = () => { this.logger.info('Connected to server stream'); + // A successful connection resets the backoff so the next drop retries fast. + this.retryAttempt = 0; for (const handler of this.handlers) { this.zone.run(() => handler.onConnected()); } @@ -105,10 +115,23 @@ export class StreamDispatchService { this.zone.run(() => handler.onDisconnected()); } + const delay = this.nextRetryDelayMs(); + this.retryAttempt += 1; this.retryTimeout = setTimeout(() => { this.retryTimeout = null; this.connectStream(); - }, this.RETRY_INTERVAL_MS); + }, delay); }; } + + /** + * Capped exponential backoff with jitter. Prevents a tight reconnect loop + * (e.g. when the server keeps returning 401 for a bad/missing API key) from + * hammering the backend on a fixed cadence. + */ + private nextRetryDelayMs(): number { + const exponential = Math.min(this.RETRY_MAX_MS, this.RETRY_BASE_MS * 2 ** this.retryAttempt); + const jitter = Math.random() * this.RETRY_JITTER_MS; + return exponential + jitter; + } } diff --git a/src/angular/src/app/services/files/model-file.service.spec.ts b/src/angular/src/app/services/files/model-file.service.spec.ts index ed16c25b..1cb767d7 100644 --- a/src/angular/src/app/services/files/model-file.service.spec.ts +++ b/src/angular/src/app/services/files/model-file.service.spec.ts @@ -30,23 +30,27 @@ describe("ModelFileService", () => { let service: ModelFileService; let mockStreamDispatch: { registerHandler: ReturnType }; let mockRestService: { sendRequest: ReturnType }; + let mockLogger: { + debug: ReturnType; + error: ReturnType; + info: ReturnType; + warn: ReturnType; + }; beforeEach(() => { mockStreamDispatch = { registerHandler: vi.fn() }; mockRestService = { sendRequest: vi.fn() }; + mockLogger = { + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + }; TestBed.configureTestingModule({ providers: [ ModelFileService, { provide: StreamDispatchService, useValue: mockStreamDispatch }, - { - provide: LoggerService, - useValue: { - debug: vi.fn(), - error: vi.fn(), - info: vi.fn(), - warn: vi.fn(), - }, - }, + { provide: LoggerService, useValue: mockLogger }, { provide: RestService, useValue: mockRestService }, ], }); @@ -225,4 +229,99 @@ describe("ModelFileService", () => { "/server/command/delete_remote/" + encoded, ); }); + + describe("malformed SSE payloads (issue #516)", () => { + const malformed = "{not valid json"; + + it("should not throw and should log on a malformed model-init event", () => { + expect(() => service.onEvent("model-init", malformed)).not.toThrow(); + expect(mockLogger.error).toHaveBeenCalled(); + + // Existing (empty) state is preserved — no silent clear/poison. + let result: Map | undefined; + service.files$.subscribe((f) => (result = f)); + expect(result!.size).toBe(0); + }); + + it("should not throw and should preserve the map on a malformed model-added event", () => { + service.onEvent("model-init", JSON.stringify([makeFileJson("file1")])); + + expect(() => service.onEvent("model-added", malformed)).not.toThrow(); + expect(mockLogger.error).toHaveBeenCalled(); + + let result: Map | undefined; + service.files$.subscribe((f) => (result = f)); + expect(result!.size).toBe(1); + expect(result!.has("file1")).toBe(true); + }); + + it("should not throw and should preserve the map on a malformed model-updated event", () => { + service.onEvent( + "model-init", + JSON.stringify([makeFileJson("file1", "DEFAULT")]), + ); + + expect(() => service.onEvent("model-updated", malformed)).not.toThrow(); + expect(mockLogger.error).toHaveBeenCalled(); + + let result: Map | undefined; + service.files$.subscribe((f) => (result = f)); + expect(result!.size).toBe(1); + expect(result!.get("file1")!.state).toBe("default"); + }); + + it("should not throw and should preserve the map on a malformed model-removed event", () => { + service.onEvent( + "model-init", + JSON.stringify([makeFileJson("file1"), makeFileJson("file2")]), + ); + + expect(() => service.onEvent("model-removed", malformed)).not.toThrow(); + expect(mockLogger.error).toHaveBeenCalled(); + + let result: Map | undefined; + service.files$.subscribe((f) => (result = f)); + expect(result!.size).toBe(2); + expect(result!.has("file1")).toBe(true); + expect(result!.has("file2")).toBe(true); + }); + + it("should still parse a valid event after a malformed one (no poisoned state)", () => { + service.onEvent("model-init", malformed); + // A subsequent valid init must replace state normally. + service.onEvent( + "model-init", + JSON.stringify([makeFileJson("file1"), makeFileJson("file2")]), + ); + + let result: Map | undefined; + service.files$.subscribe((f) => (result = f)); + expect(result!.size).toBe(2); + expect(result!.has("file1")).toBe(true); + expect(result!.has("file2")).toBe(true); + }); + + // Valid JSON, wrong shape: parses fine but the mapping below would throw. + // The dispatch (not just JSON.parse) must be guarded — log and skip. + it("should not throw on a valid-JSON-but-wrong-shape model-init (not an array)", () => { + expect(() => service.onEvent("model-init", "{}")).not.toThrow(); + expect(mockLogger.error).toHaveBeenCalled(); + + let result: Map | undefined; + service.files$.subscribe((f) => (result = f)); + expect(result!.size).toBe(0); + }); + + it("should not throw on a wrong-shape model-added (missing new_file)", () => { + service.onEvent("model-init", JSON.stringify([makeFileJson("file1")])); + + expect(() => service.onEvent("model-added", "{}")).not.toThrow(); + expect(mockLogger.error).toHaveBeenCalled(); + + let result: Map | undefined; + service.files$.subscribe((f) => (result = f)); + expect(result!.size).toBe(1); + expect(result!.has("file1")).toBe(true); + }); + }); }); diff --git a/src/angular/src/app/services/files/model-file.service.ts b/src/angular/src/app/services/files/model-file.service.ts index ab66a83a..44c9c74b 100644 --- a/src/angular/src/app/services/files/model-file.service.ts +++ b/src/angular/src/app/services/files/model-file.service.ts @@ -84,63 +84,75 @@ export class ModelFileService implements StreamEventHandler { private parseEvent(name: string, data: string): void { const currentFiles = this.filesSubject.getValue(); - if (name === this.EVENT_INIT) { - let t0: number; - let t1: number; - - t0 = performance.now(); - const parsed: ModelFileJson[] = JSON.parse(data); - t1 = performance.now(); - this.logger.debug('Parsing took', (t1 - t0).toFixed(0), 'ms'); - - t0 = performance.now(); - const newMap = new Map(); - for (const file of parsed) { - const modelFile = modelFileFromJson(file); - newMap.set(fileKey(modelFile.pair_id, modelFile.name), modelFile); - } - t1 = performance.now(); - this.logger.debug('ModelFile map creation took', (t1 - t0).toFixed(0), 'ms'); - - this.filesSubject.next(newMap); - } else if (name === this.EVENT_ADDED) { - const parsed: { new_file: ModelFileJson } = JSON.parse(data); - const file = modelFileFromJson(parsed.new_file); - const key = fileKey(file.pair_id, file.name); - if (currentFiles.has(key)) { - this.logger.error('ModelFile named ' + key + ' already exists'); - } else { - const updated = new Map(currentFiles); - updated.set(key, file); - this.filesSubject.next(updated); - this.logger.debug('Added file: %O', file); - } - } else if (name === this.EVENT_REMOVED) { - const parsed: { old_file: ModelFileJson } = JSON.parse(data); - const file = modelFileFromJson(parsed.old_file); - const key = fileKey(file.pair_id, file.name); - if (currentFiles.has(key)) { - const updated = new Map(currentFiles); - updated.delete(key); - this.filesSubject.next(updated); - this.logger.debug('Removed file: %O', file); - } else { - this.logger.error('Failed to find ModelFile named ' + key); - } - } else if (name === this.EVENT_UPDATED) { - const parsed: { new_file: ModelFileJson } = JSON.parse(data); - const file = modelFileFromJson(parsed.new_file); - const key = fileKey(file.pair_id, file.name); - if (currentFiles.has(key)) { - const updated = new Map(currentFiles); - updated.set(key, file); - this.filesSubject.next(updated); - this.logger.debug('Updated file: %O', file); + // Guard the parse of server-controlled SSE payloads: a malformed/truncated + // event must not throw inside the listener callback. Log and skip the bad + // event, leaving existing state untouched (mirrors ConfigService.getConfig). + let parsed: unknown; + try { + parsed = JSON.parse(data); + } catch (e) { + this.logger.error('Failed to parse %s event: %O', name, e); + return; + } + + // The payload parsed, but a valid-JSON-but-wrong-shape event (e.g. model-init + // not an array, or model-added missing new_file) would throw in the mapping + // below; guard the dispatch too so one bad event is logged and skipped, not + // fatal to the listener. + try { + if (name === this.EVENT_INIT) { + const init = parsed as ModelFileJson[]; + const t0 = performance.now(); + const newMap = new Map(); + for (const file of init) { + const modelFile = modelFileFromJson(file); + newMap.set(fileKey(modelFile.pair_id, modelFile.name), modelFile); + } + const t1 = performance.now(); + this.logger.debug('ModelFile map creation took', (t1 - t0).toFixed(0), 'ms'); + + this.filesSubject.next(newMap); + } else if (name === this.EVENT_ADDED) { + const added = parsed as { new_file: ModelFileJson }; + const file = modelFileFromJson(added.new_file); + const key = fileKey(file.pair_id, file.name); + if (currentFiles.has(key)) { + this.logger.error('ModelFile named ' + key + ' already exists'); + } else { + const updated = new Map(currentFiles); + updated.set(key, file); + this.filesSubject.next(updated); + this.logger.debug('Added file: %O', file); + } + } else if (name === this.EVENT_REMOVED) { + const removed = parsed as { old_file: ModelFileJson }; + const file = modelFileFromJson(removed.old_file); + const key = fileKey(file.pair_id, file.name); + if (currentFiles.has(key)) { + const updated = new Map(currentFiles); + updated.delete(key); + this.filesSubject.next(updated); + this.logger.debug('Removed file: %O', file); + } else { + this.logger.error('Failed to find ModelFile named ' + key); + } + } else if (name === this.EVENT_UPDATED) { + const updatedFile = parsed as { new_file: ModelFileJson }; + const file = modelFileFromJson(updatedFile.new_file); + const key = fileKey(file.pair_id, file.name); + if (currentFiles.has(key)) { + const updated = new Map(currentFiles); + updated.set(key, file); + this.filesSubject.next(updated); + this.logger.debug('Updated file: %O', file); + } else { + this.logger.error('Failed to find ModelFile named ' + key); + } } else { - this.logger.error('Failed to find ModelFile named ' + key); + this.logger.error('Unrecognized event:', name); } - } else { - this.logger.error('Unrecognized event:', name); + } catch (e) { + this.logger.error('Failed to handle %s event (unexpected shape): %O', name, e); } } } diff --git a/src/angular/src/app/services/logs/log.service.spec.ts b/src/angular/src/app/services/logs/log.service.spec.ts index 39138deb..b8fae150 100644 --- a/src/angular/src/app/services/logs/log.service.spec.ts +++ b/src/angular/src/app/services/logs/log.service.spec.ts @@ -2,18 +2,32 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { TestBed } from "@angular/core/testing"; import { LogService } from "./log.service"; import { StreamDispatchService } from "../base/stream-dispatch.service"; +import { LoggerService } from "../utils/logger.service"; import { LogRecord, LogRecordJson } from "../../models/log-record"; describe("LogService", () => { let service: LogService; let mockStreamDispatch: { registerHandler: ReturnType }; + let mockLogger: { + debug: ReturnType; + error: ReturnType; + info: ReturnType; + warn: ReturnType; + }; beforeEach(() => { mockStreamDispatch = { registerHandler: vi.fn() }; + mockLogger = { + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + }; TestBed.configureTestingModule({ providers: [ LogService, { provide: StreamDispatchService, useValue: mockStreamDispatch }, + { provide: LoggerService, useValue: mockLogger }, ], }); service = TestBed.inject(LogService); @@ -70,4 +84,31 @@ describe("LogService", () => { expect(results[0].message).toBe("First"); expect(results[1].message).toBe("Second"); }); + + it("should not throw and should emit nothing on a malformed log-record event (issue #516)", () => { + const results: LogRecord[] = []; + service.logs$.subscribe((r) => results.push(r)); + + expect(() => service.onEvent("log-record", "not json")).not.toThrow(); + expect(mockLogger.error).toHaveBeenCalled(); + expect(results.length).toBe(0); + }); + + it("should still emit a valid log-record after a malformed one (issue #516)", () => { + const results: LogRecord[] = []; + service.logs$.subscribe((r) => results.push(r)); + + service.onEvent("log-record", "not json"); + const logJson: LogRecordJson = { + time: 1700000000, + level_name: "INFO", + logger_name: "test.logger", + message: "After malformed", + exc_tb: "", + }; + service.onEvent("log-record", JSON.stringify(logJson)); + + expect(results.length).toBe(1); + expect(results[0].message).toBe("After malformed"); + }); }); diff --git a/src/angular/src/app/services/logs/log.service.ts b/src/angular/src/app/services/logs/log.service.ts index 838a3d21..ae1ed314 100644 --- a/src/angular/src/app/services/logs/log.service.ts +++ b/src/angular/src/app/services/logs/log.service.ts @@ -3,6 +3,7 @@ import { HttpClient, HttpParams } from '@angular/common/http'; import { Observable, Subject } from 'rxjs'; import { StreamEventHandler, StreamDispatchService } from '../base/stream-dispatch.service'; +import { LoggerService } from '../utils/logger.service'; import { LogRecord, logRecordFromJson } from '../../models/log-record'; export interface LogHistoryParams { @@ -25,6 +26,7 @@ export interface LogHistoryEntry { export class LogService implements StreamEventHandler { private readonly streamDispatch = inject(StreamDispatchService); private readonly http = inject(HttpClient); + private readonly logger = inject(LoggerService); private readonly logsSubject = new Subject(); @@ -39,7 +41,14 @@ export class LogService implements StreamEventHandler { } onEvent(_eventName: string, data: string): void { - this.logsSubject.next(logRecordFromJson(JSON.parse(data))); + // Guard the parse of server-controlled SSE payloads: a malformed/truncated + // event must not throw inside the listener callback. Log and skip the bad + // record, emitting nothing (mirrors ConfigService.getConfig). + try { + this.logsSubject.next(logRecordFromJson(JSON.parse(data))); + } catch (e) { + this.logger.error('Failed to parse log-record event: %O', e); + } } onConnected(): void { diff --git a/src/angular/src/app/services/server/server-status.service.spec.ts b/src/angular/src/app/services/server/server-status.service.spec.ts index 06f5d846..798b2138 100644 --- a/src/angular/src/app/services/server/server-status.service.spec.ts +++ b/src/angular/src/app/services/server/server-status.service.spec.ts @@ -2,19 +2,33 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { TestBed } from "@angular/core/testing"; import { ServerStatusService } from "./server-status.service"; import { StreamDispatchService } from "../base/stream-dispatch.service"; +import { LoggerService } from "../utils/logger.service"; import { ServerStatus, ServerStatusJson } from "../../models/server-status"; import { Localization } from "../../models/localization"; describe("ServerStatusService", () => { let service: ServerStatusService; let mockStreamDispatch: { registerHandler: ReturnType }; + let mockLogger: { + debug: ReturnType; + error: ReturnType; + info: ReturnType; + warn: ReturnType; + }; beforeEach(() => { mockStreamDispatch = { registerHandler: vi.fn() }; + mockLogger = { + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + }; TestBed.configureTestingModule({ providers: [ ServerStatusService, { provide: StreamDispatchService, useValue: mockStreamDispatch }, + { provide: LoggerService, useValue: mockLogger }, ], }); service = TestBed.inject(ServerStatusService); @@ -89,4 +103,47 @@ describe("ServerStatusService", () => { expect(result!.controller.latestRemoteScanFailed).toBe(false); expect(result!.controller.latestRemoteScanError).toBeNull(); }); + + it("should not throw and should keep last-good status on a malformed status event (issue #516)", () => { + // Seed a valid connected status. + const statusJson: ServerStatusJson = { + server: { up: true, error_msg: "" }, + controller: { + latest_local_scan_time: "1700000000", + latest_remote_scan_time: null, + latest_remote_scan_failed: false, + latest_remote_scan_error: null, + no_enabled_pairs: false, + }, + }; + service.onEvent("status", JSON.stringify(statusJson)); + + // A malformed payload must not throw and must not degrade the status. + expect(() => service.onEvent("status", "garbage")).not.toThrow(); + expect(mockLogger.error).toHaveBeenCalled(); + + let result: ServerStatus | undefined; + service.status$.subscribe((s) => (result = s)); + expect(result!.server.up).toBe(true); + }); + + it("should still parse a valid status event after a malformed one (issue #516)", () => { + service.onEvent("status", "garbage"); + + const statusJson: ServerStatusJson = { + server: { up: true, error_msg: "" }, + controller: { + latest_local_scan_time: null, + latest_remote_scan_time: null, + latest_remote_scan_failed: false, + latest_remote_scan_error: null, + no_enabled_pairs: false, + }, + }; + service.onEvent("status", JSON.stringify(statusJson)); + + let result: ServerStatus | undefined; + service.status$.subscribe((s) => (result = s)); + expect(result!.server.up).toBe(true); + }); }); diff --git a/src/angular/src/app/services/server/server-status.service.ts b/src/angular/src/app/services/server/server-status.service.ts index 62a503fa..3c9e4839 100644 --- a/src/angular/src/app/services/server/server-status.service.ts +++ b/src/angular/src/app/services/server/server-status.service.ts @@ -2,12 +2,14 @@ import { Injectable, inject } from '@angular/core'; import { BehaviorSubject, Observable } from 'rxjs'; import { StreamEventHandler, StreamDispatchService } from '../base/stream-dispatch.service'; +import { LoggerService } from '../utils/logger.service'; import { Localization } from '../../models/localization'; import { ServerStatus, ServerStatusJson, serverStatusFromJson } from '../../models/server-status'; @Injectable({ providedIn: 'root' }) export class ServerStatusService implements StreamEventHandler { private readonly streamDispatch = inject(StreamDispatchService); + private readonly logger = inject(LoggerService); private readonly statusSubject = new BehaviorSubject({ server: { @@ -34,8 +36,15 @@ export class ServerStatusService implements StreamEventHandler { } onEvent(_eventName: string, data: string): void { - const statusJson: ServerStatusJson = JSON.parse(data); - this.statusSubject.next(serverStatusFromJson(statusJson)); + // Guard the parse of server-controlled SSE payloads: a malformed/truncated + // event must not throw inside the listener callback. Log and skip the bad + // event, leaving the last-good status in place (mirrors ConfigService). + try { + const statusJson: ServerStatusJson = JSON.parse(data); + this.statusSubject.next(serverStatusFromJson(statusJson)); + } catch (e) { + this.logger.error('Failed to parse status event: %O', e); + } } onConnected(): void { diff --git a/src/angular/src/app/services/settings/config.service.spec.ts b/src/angular/src/app/services/settings/config.service.spec.ts index 98cd7dd9..3edfa474 100644 --- a/src/angular/src/app/services/settings/config.service.spec.ts +++ b/src/angular/src/app/services/settings/config.service.spec.ts @@ -8,7 +8,7 @@ import { ConnectedService } from "../utils/connected.service"; import { LoggerService } from "../utils/logger.service"; import { RestService, WebReaction } from "../utils/rest.service"; import { StreamDispatchService } from "../base/stream-dispatch.service"; -import { Config } from "../../models/config"; +import { Config, REDACTED_SENTINEL } from "../../models/config"; function makeConfig(overrides: Partial = {}): Config { return { @@ -81,9 +81,23 @@ describe("ConfigService", () => { let mockRestService: { sendRequest: ReturnType }; let mockStreamDispatch: { setApiKey: ReturnType }; + /** + * Lazily construct the service. The constructor fetches config, so each test + * configures its mocks (sendRequest return values) BEFORE calling this. + */ + function createService(): ConfigService { + service = TestBed.inject(ConfigService); + return service; + } + beforeEach(() => { connectedSubject = new BehaviorSubject(false); - mockRestService = { sendRequest: vi.fn() }; + // Safe default so the constructor's init fetch never crashes on undefined. + mockRestService = { + sendRequest: vi.fn().mockReturnValue( + of({ success: false, data: null, errorMessage: "not configured" }), + ), + }; mockStreamDispatch = { setApiKey: vi.fn() }; TestBed.configureTestingModule({ @@ -109,18 +123,20 @@ describe("ConfigService", () => { }, ], }); - service = TestBed.inject(ConfigService); }); // --- Initial state --- it("should emit null as the initial config", () => { + // Default mock returns a failure reaction, so the init fetch leaves config null. + createService(); let result: Config | null | undefined; service.config$.subscribe((c) => (result = c)); expect(result).toBeNull(); }); it("should return null from configSnapshot initially", () => { + createService(); expect(service.configSnapshot).toBeNull(); }); @@ -132,6 +148,7 @@ describe("ConfigService", () => { of({ success: true, data: JSON.stringify(config), errorMessage: null }), ); + createService(); connectedSubject.next(true); let result: Config | null | undefined; @@ -148,6 +165,7 @@ describe("ConfigService", () => { of({ success: true, data: JSON.stringify(config), errorMessage: null }), ); + createService(); connectedSubject.next(true); expect(service.configSnapshot).toEqual(config); }); @@ -158,36 +176,97 @@ describe("ConfigService", () => { of({ success: true, data: JSON.stringify(config), errorMessage: null }), ); + createService(); connectedSubject.next(true); expect(mockStreamDispatch.setApiKey).toHaveBeenCalledWith("my-key"); }); + // --- Init-ordering deadlock fix (#514) --- + + it("should fetch config at init independent of connected$", () => { + const config = makeConfig(); + mockRestService.sendRequest.mockReturnValue( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ); + + // connected$ is still false (never emits true) + createService(); + + let result: Config | null | undefined; + service.config$.subscribe((c) => (result = c)); + expect(result).toEqual(config); + expect(mockRestService.sendRequest).toHaveBeenCalledWith( + "/server/config/get", + ); + }); + + it("should NOT push the redacted sentinel from /server/config/get to the stream", () => { + const config = makeConfig({ web: { port: 8080, api_key: REDACTED_SENTINEL } }); + mockRestService.sendRequest.mockReturnValue( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ); + + createService(); + + // The redacted sentinel returned by the auth-exempt config endpoint must + // never be pushed to the stream as a credential. + expect(mockStreamDispatch.setApiKey).not.toHaveBeenCalledWith(REDACTED_SENTINEL); + }); + + it("preserves the real api_key in the snapshot when a refresh returns the redacted sentinel", () => { + // Init with a web section so set() has an existing option to update. + mockRestService.sendRequest.mockReturnValue( + of({ success: true, data: JSON.stringify(makeConfig({ web: { port: 8080, api_key: "" } })), errorMessage: null }), + ); + createService(); + + // The user enters the real key in Settings. + mockRestService.sendRequest.mockReturnValueOnce(of({ success: true, data: null, errorMessage: null })); + service.set("web", "api_key", "new-key"); + expect(service.configSnapshot?.web?.api_key).toBe("new-key"); + + // A later config refresh returns the redacted sentinel. It must NOT clobber + // the real key the apiKeyInterceptor relies on for REST auth this session. + mockRestService.sendRequest.mockReturnValue( + of({ success: true, data: JSON.stringify(makeConfig({ web: { port: 8080, api_key: REDACTED_SENTINEL } })), errorMessage: null }), + ); + connectedSubject.next(true); + + expect(service.configSnapshot?.web?.api_key).toBe("new-key"); + }); + // --- Disconnect --- - it("should emit null config when disconnected", () => { + it("should retain config on disconnect so the UI keeps working", () => { const config = makeConfig(); mockRestService.sendRequest.mockReturnValue( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ); + createService(); connectedSubject.next(true); connectedSubject.next(false); + // Config is fetched at init independent of the stream, so a transient + // disconnect must not wipe it out (and trigger a deadlock-prone refetch loop). let result: Config | null | undefined; service.config$.subscribe((c) => (result = c)); - expect(result).toBeNull(); + expect(result).toEqual(config); }); - it("should sync null API key when disconnected", () => { - const config = makeConfig(); + it("should NOT clear the api key on disconnect so the next reconnect can authenticate", () => { + const config = makeConfig({ web: { port: 8080, api_key: "my-key" } }); mockRestService.sendRequest.mockReturnValue( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ); + createService(); connectedSubject.next(true); mockStreamDispatch.setApiKey.mockClear(); connectedSubject.next(false); - expect(mockStreamDispatch.setApiKey).toHaveBeenCalledWith(null); + + // The key must stay on the stream so the backoff-driven reconnect carries it. + expect(mockStreamDispatch.setApiKey).not.toHaveBeenCalledWith(null); }); // --- Error handling --- @@ -201,6 +280,7 @@ describe("ConfigService", () => { }), ); + createService(); connectedSubject.next(true); let result: Config | null | undefined; @@ -213,6 +293,7 @@ describe("ConfigService", () => { of({ success: true, data: "not valid json {{{", errorMessage: null }), ); + createService(); connectedSubject.next(true); let result: Config | null | undefined; @@ -227,6 +308,7 @@ describe("ConfigService", () => { mockRestService.sendRequest.mockReturnValue( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ); + createService(); connectedSubject.next(true); let result: WebReaction | undefined; @@ -241,6 +323,7 @@ describe("ConfigService", () => { mockRestService.sendRequest.mockReturnValue( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ); + createService(); connectedSubject.next(true); let result: WebReaction | undefined; @@ -251,7 +334,8 @@ describe("ConfigService", () => { }); it("should return error when config is null", () => { - // Config is null by default (not connected) + // Default mock returns failure, so config stays null after init. + createService(); let result: WebReaction | undefined; service.set("web", "port", "9090").subscribe((r: WebReaction) => (result = r)); @@ -260,13 +344,18 @@ describe("ConfigService", () => { it("should call REST with double-encoded value", () => { const config = makeConfig(); + // init fetch -> connect fetch -> set response mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: true, data: null, errorMessage: null }), ); + createService(); connectedSubject.next(true); service.set("web", "api_key", "my/key"); @@ -280,12 +369,16 @@ describe("ConfigService", () => { it("should use __empty__ sentinel for empty string value", () => { const config = makeConfig(); mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: true, data: null, errorMessage: null }), ); + createService(); connectedSubject.next(true); service.set("web", "api_key", ""); @@ -297,15 +390,41 @@ describe("ConfigService", () => { expect(mockStreamDispatch.setApiKey).toHaveBeenCalledWith(null); }); + it("should push the real api_key to the stream on successful set", () => { + const config = makeConfig({ web: { port: 8080, api_key: "old" } }); + mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) + .mockReturnValueOnce( + of({ success: true, data: null, errorMessage: null }), + ); + createService(); + connectedSubject.next(true); + + service.set("web", "api_key", "new-key"); + + // set() is the only place the real key is available client-side; it is + // pushed to the stream (but not persisted anywhere). + expect(mockStreamDispatch.setApiKey).toHaveBeenCalledWith("new-key"); + }); + it("should update BehaviorSubject on successful set", () => { const config = makeConfig({ web: { port: 8080, api_key: "old" } }); mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: true, data: null, errorMessage: null }), ); + createService(); connectedSubject.next(true); service.set("web", "api_key", "new-key"); @@ -316,12 +435,16 @@ describe("ConfigService", () => { it("should not update BehaviorSubject when set request fails", () => { const config = makeConfig({ web: { port: 8080, api_key: "old" } }); mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: false, data: null, errorMessage: "fail" }), ); + createService(); connectedSubject.next(true); service.set("web", "api_key", "new-key"); @@ -332,12 +455,16 @@ describe("ConfigService", () => { it("should sync API key to StreamDispatchService when web.api_key is set", () => { const config = makeConfig({ web: { port: 8080, api_key: "old" } }); mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: true, data: null, errorMessage: null }), ); + createService(); connectedSubject.next(true); mockStreamDispatch.setApiKey.mockClear(); @@ -349,12 +476,16 @@ describe("ConfigService", () => { it("should encode boolean true as 'true' in the URL", () => { const config = makeConfig(); mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: true, data: null, errorMessage: null }), ); + createService(); connectedSubject.next(true); service.set("autoqueue", "enabled", true); @@ -369,12 +500,16 @@ describe("ConfigService", () => { it("should encode boolean false as 'false' in the URL", () => { const config = makeConfig(); mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: true, data: null, errorMessage: null }), ); + createService(); connectedSubject.next(true); service.set("autoqueue", "enabled", false); @@ -389,12 +524,16 @@ describe("ConfigService", () => { it("should use __empty__ sentinel for null value", () => { const config = makeConfig(); mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: true, data: null, errorMessage: null }), ); + createService(); connectedSubject.next(true); service.set("lftp", "net_limit_rate", null); @@ -407,12 +546,16 @@ describe("ConfigService", () => { it("should not sync API key when setting a non-api_key option", () => { const config = makeConfig({ web: { port: 8080, api_key: "old" } }); mockRestService.sendRequest + .mockReturnValueOnce( + of({ success: true, data: JSON.stringify(config), errorMessage: null }), + ) .mockReturnValueOnce( of({ success: true, data: JSON.stringify(config), errorMessage: null }), ) .mockReturnValueOnce( of({ success: true, data: null, errorMessage: null }), ); + createService(); connectedSubject.next(true); mockStreamDispatch.setApiKey.mockClear(); diff --git a/src/angular/src/app/services/settings/config.service.ts b/src/angular/src/app/services/settings/config.service.ts index 4a28920e..0bc06d1a 100644 --- a/src/angular/src/app/services/settings/config.service.ts +++ b/src/angular/src/app/services/settings/config.service.ts @@ -5,7 +5,7 @@ import { ConnectedService } from '../utils/connected.service'; import { LoggerService } from '../utils/logger.service'; import { RestService, WebReaction } from '../utils/rest.service'; import { StreamDispatchService } from '../base/stream-dispatch.service'; -import { Config } from '../../models/config'; +import { Config, REDACTED_SENTINEL } from '../../models/config'; /** Value a config field may take (matches OptionComponent's value shape). */ export type ConfigValue = string | number | boolean | null; @@ -16,6 +16,11 @@ type ConfigRecord = Record>; /** Sentinel value sent to the backend when the user clears a text field. */ export const EMPTY_VALUE_SENTINEL = '__empty__'; +// REDACTED_SENTINEL (imported from models/config) is the value the backend +// substitutes for web.api_key in /server/config/get responses. It must never be +// treated as a real key — pushing it to the stream would fail auth and could +// clobber a good persisted key. + @Injectable({ providedIn: 'root' }) export class ConfigService { private readonly CONFIG_GET_URL = '/server/config/get'; @@ -36,12 +41,18 @@ export class ConfigService { } constructor() { + // Fetch config at init independent of the stream connection. + // /server/config/get is auth-exempt, so config$ can populate for the UI + // even before (and without) an authenticated stream connection. This + // breaks the init-ordering deadlock where the config fetch was gated + // behind a stream that could never connect without the key. The key itself + // is NOT persisted client-side; after a reload on an auth-enabled instance + // the user re-enters it in Settings (which re-pushes it to the stream). + this.getConfig(); + this.connectedService.connected$.subscribe((connected) => { if (connected) { this.getConfig(); - } else { - this.configSubject.next(null); - this.syncStreamApiKey(null); } }); } @@ -71,9 +82,11 @@ export class ConfigService { const configRecord = config as unknown as ConfigRecord; const newConfig = { ...config, [section]: { ...configRecord[section], [option]: value } }; this.configSubject.next(newConfig); - // Propagate API key changes to the SSE stream immediately + // Propagate API key changes to the SSE stream immediately. set() is + // the only place the real (un-redacted) key is available client-side. if (section === 'web' && option === 'api_key') { - this.syncStreamApiKey(newConfig); + const realKey = String(value ?? '') || null; + this.setStreamApiKey(realKey); } } } @@ -88,25 +101,55 @@ export class ConfigService { next: (reaction) => { if (reaction.success) { try { - const configJson: Config = JSON.parse(reaction.data!); + const configJson: Config = this.preserveRealApiKey(JSON.parse(reaction.data!)); this.configSubject.next(configJson); this.syncStreamApiKey(configJson); } catch (e) { this.logger.error('Failed to parse config: %O', e); this.configSubject.next(null); - this.syncStreamApiKey(null); } } else { this.logger.error('Failed to get config: %s', reaction.errorMessage); this.configSubject.next(null); - this.syncStreamApiKey(null); } }, }); } + /** + * Push an API key from a fetched config to the stream, guarding against the + * redacted sentinel. /server/config/get redacts web.api_key to '********', + * which would fail auth and clobber a good persisted key, so a redacted + * value is treated as "no change" and never overwrites the stream's key. + */ + /** + * /server/config/get redacts web.api_key to the sentinel. A refresh must not + * clobber a real key the user entered this session — the apiKeyInterceptor + * reads configSnapshot.web.api_key for REST auth, so overwriting it with the + * sentinel would break authenticated REST calls until the next set(). Keep the + * existing real key when the incoming value is redacted. + */ + private preserveRealApiKey(incoming: Config): Config { + const currentKey = this.configSubject.getValue()?.web?.api_key; + if ( + incoming.web?.api_key === REDACTED_SENTINEL && + currentKey && + currentKey !== REDACTED_SENTINEL + ) { + return { ...incoming, web: { ...incoming.web, api_key: currentKey } }; + } + return incoming; + } + private syncStreamApiKey(config: Config | null): void { const apiKey = config?.web?.api_key || null; + if (apiKey === REDACTED_SENTINEL) { + return; + } + this.setStreamApiKey(apiKey); + } + + private setStreamApiKey(apiKey: string | null): void { // Use injector.get() to break circular dependency: // StreamDispatchService -> ConnectedService -> ConfigService const streamDispatch = this.injector.get(StreamDispatchService); diff --git a/src/angular/src/app/services/utils/api-key.interceptor.spec.ts b/src/angular/src/app/services/utils/api-key.interceptor.spec.ts index 8341472c..449d826e 100644 --- a/src/angular/src/app/services/utils/api-key.interceptor.spec.ts +++ b/src/angular/src/app/services/utils/api-key.interceptor.spec.ts @@ -148,4 +148,28 @@ describe("apiKeyInterceptor", () => { expect(req.request.headers.has("X-Api-Key")).toBe(false); req.flush(""); }); + + // --- Redacted sentinel guard (#514) --- + + it("should never send the redacted sentinel as a credential", () => { + // /server/config/get redacts the key to '********'; that must never be + // sent as a credential. + mockConfigService.configSnapshot = { web: { api_key: "********" } }; + + http.get("/server/config/get", { responseType: "text" }).subscribe(); + + const req = httpTesting.expectOne("/server/config/get"); + expect(req.request.headers.has("X-Api-Key")).toBe(false); + req.flush(""); + }); + + it("should send the real snapshot key (present during the session it was set)", () => { + mockConfigService.configSnapshot = { web: { api_key: "snapshot-key" } }; + + http.get("/server/config/get", { responseType: "text" }).subscribe(); + + const req = httpTesting.expectOne("/server/config/get"); + expect(req.request.headers.get("X-Api-Key")).toBe("snapshot-key"); + req.flush(""); + }); }); diff --git a/src/angular/src/app/services/utils/api-key.interceptor.ts b/src/angular/src/app/services/utils/api-key.interceptor.ts index 3e7464f8..482d5ea4 100644 --- a/src/angular/src/app/services/utils/api-key.interceptor.ts +++ b/src/angular/src/app/services/utils/api-key.interceptor.ts @@ -2,6 +2,7 @@ import { HttpInterceptorFn } from '@angular/common/http'; import { inject } from '@angular/core'; import { ConfigService } from '../settings/config.service'; +import { REDACTED_SENTINEL } from '../../models/config'; export const apiKeyInterceptor: HttpInterceptorFn = (req, next) => { // Only attach the API key to internal backend /server/* requests @@ -11,8 +12,12 @@ export const apiKeyInterceptor: HttpInterceptorFn = (req, next) => { } const configService = inject(ConfigService); - const config = configService.configSnapshot; - const apiKey = config?.web?.api_key; + const snapshotKey = configService.configSnapshot?.web?.api_key; + + // /server/config/get redacts web.api_key to '********'; never send the + // redacted sentinel as a credential. The real key is only present in the + // snapshot during the session in which the user entered it in Settings. + const apiKey = snapshotKey === REDACTED_SENTINEL ? null : snapshotKey; if (apiKey) { const authReq = req.clone({