From 1609e0ff11948b0611a52bdc02ac5cd1d8a5d13b Mon Sep 17 00:00:00 2001 From: 4gray Date: Mon, 27 Jul 2026 22:34:39 +0200 Subject: [PATCH] fix(electron-backend): lint the whole project, not one file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `nx lint electron-backend` ran `eslint apps/electron-backend/**/*.ts` unquoted. Nx executes commands through /bin/sh, which has no `globstar`, so `**` collapsed to `*` and the shell expanded the pattern itself — matching a single file out of 247. Quoting hands the pattern to eslint, which expands it recursively. Two sibling patterns were already quoted, so the breakage had been worked around per-file rather than fixed; they are now redundant (verified: the quoted glob alone resolves to the identical 247-file set, including all 24 frame-copy-runtime files) and collapse into one. That exposed five real errors the broken glob had been hiding: - `no-unsafe-finally` x2 — `closeSession` and `disposeSession` both did `try { ... } finally { return ...; }`. A `return` inside `finally` discards an in-flight exception, so a failing close/dispose was indistinguishable from a clean one. Both intentionally always complete teardown (`shutdown()` disposes every session in turn and must not stop at the first failure), so the fix keeps that guarantee via an explicit `catch` and logs the reason instead of dropping it. - `prefer-const` x2 — a write-once `timeoutId` and module-level `Database`. The other `timeoutId` in the same file is genuinely reassigned and stays `let`. - `no-control-regex` — the filename sanitizer strips control characters on purpose; disabled inline with that rationale. The three `max-lines` violations this would also have surfaced are already gone: #1288 split those specs. Regression coverage for both teardown paths, each verified to fail against the old code. Co-Authored-By: Claude Opus 5 --- .changes/electron-backend-lint-coverage.md | 9 +++ apps/electron-backend/project.json | 2 +- .../src/app/events/epg-worker.service.ts | 3 +- .../external-player-session-registry.spec.ts | 26 ++++++++ .../external-player-session-registry.ts | 12 +++- .../embedded-mpv-native.service.spec.ts | 35 ++++++++++- .../services/embedded-mpv-native.service.ts | 62 +++++++++++-------- .../app/workers/database.worker-connection.ts | 16 ++--- 8 files changed, 124 insertions(+), 41 deletions(-) create mode 100644 .changes/electron-backend-lint-coverage.md diff --git a/.changes/electron-backend-lint-coverage.md b/.changes/electron-backend-lint-coverage.md new file mode 100644 index 000000000..fa7fda0e7 --- /dev/null +++ b/.changes/electron-backend-lint-coverage.md @@ -0,0 +1,9 @@ +--- +type: internal +area: electron-backend +--- + +Lint now covers the whole Electron backend instead of a single file, and two +teardown paths that silently discarded their failure reason — closing an +external player session and disposing an embedded mpv session — now report it. +Both still finish the teardown exactly as before. diff --git a/apps/electron-backend/project.json b/apps/electron-backend/project.json index 6e4107999..f75803937 100644 --- a/apps/electron-backend/project.json +++ b/apps/electron-backend/project.json @@ -241,7 +241,7 @@ "{workspaceRoot}/tools/embedded-mpv/runtime-probe-contract.cjs", "{workspaceRoot}/tools/embedded-mpv/runtime-probe-contract.d.cts" ], - "command": "eslint apps/electron-backend/**/*.ts \"apps/electron-backend/src/app/services/embedded-mpv-frame-copy-runtime.ts\" \"apps/electron-backend/src/app/services/embedded-mpv-frame-copy-runtime/**/*.ts\"" + "command": "eslint \"apps/electron-backend/**/*.ts\"" }, "test": { "executor": "@nx/jest:jest", diff --git a/apps/electron-backend/src/app/events/epg-worker.service.ts b/apps/electron-backend/src/app/events/epg-worker.service.ts index f671ab999..1b278ed97 100644 --- a/apps/electron-backend/src/app/events/epg-worker.service.ts +++ b/apps/electron-backend/src/app/events/epg-worker.service.ts @@ -422,7 +422,6 @@ export class EpgWorkerService { } let settled = false; - let timeoutId: ReturnType; const settle = (fn: () => void) => { if (settled) return; settled = true; @@ -430,7 +429,7 @@ export class EpgWorkerService { fn(); }; - timeoutId = setTimeout(() => { + const timeoutId = setTimeout(() => { const errorMessage = `${options.timeoutLabel} timed out after ${ this.fetchTimeoutMs / 1000 }s`; diff --git a/apps/electron-backend/src/app/events/external-player-session-registry.spec.ts b/apps/electron-backend/src/app/events/external-player-session-registry.spec.ts index 18e573f11..ab58d44db 100644 --- a/apps/electron-backend/src/app/events/external-player-session-registry.spec.ts +++ b/apps/electron-backend/src/app/events/external-player-session-registry.spec.ts @@ -47,6 +47,32 @@ describe('ExternalPlayerSessionRegistry', () => { expect(registry.getActiveSessionId()).toBeNull(); }); + it('still closes the session when the player fails to close', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(); + const failure = new Error('player already exited'); + const close = jest.fn().mockRejectedValue(failure); + const session = registry.beginSession({ + player: 'mpv', + title: 'Example', + streamUrl: 'https://example.com/video.m3u8', + }); + + registry.attachCloser(session.id, close); + + const closed = await registry.closeSession(session.id); + + expect(closed?.status).toBe('closed'); + expect(closed?.canClose).toBe(false); + expect(registry.getActiveSessionId()).toBeNull(); + // The reason must survive: the old `return` inside `finally` dropped it. + expect(warn).toHaveBeenCalledWith( + `Failed to close external player session ${session.id}:`, + failure + ); + + warn.mockRestore(); + }); + it('marks runtime failures as errors without clearing the active id', () => { const session = registry.beginSession({ player: 'mpv', diff --git a/apps/electron-backend/src/app/events/external-player-session-registry.ts b/apps/electron-backend/src/app/events/external-player-session-registry.ts index f18e1fb39..953b831bb 100644 --- a/apps/electron-backend/src/app/events/external-player-session-registry.ts +++ b/apps/electron-backend/src/app/events/external-player-session-registry.ts @@ -128,10 +128,18 @@ export class ExternalPlayerSessionRegistry { return null; } + // The session must end up closed even if the player is already gone, + // but a `return` inside `finally` would also swallow the reason — so + // catch it explicitly and surface it. try { await runtime.close?.(); - } finally { - return this.markClosed(id); + } catch (error) { + console.warn( + `Failed to close external player session ${id}:`, + error + ); } + + return this.markClosed(id); } } diff --git a/apps/electron-backend/src/app/services/embedded-mpv-native.service.spec.ts b/apps/electron-backend/src/app/services/embedded-mpv-native.service.spec.ts index 0568a9f0c..ede4802d0 100644 --- a/apps/electron-backend/src/app/services/embedded-mpv-native.service.spec.ts +++ b/apps/electron-backend/src/app/services/embedded-mpv-native.service.spec.ts @@ -493,7 +493,11 @@ describe('EmbeddedMpvNativeService power blocker', () => { addon.createSession.mockReturnValueOnce('s-zoom'); addon.getSessionSnapshot.mockReturnValue(snapshot('loading')); - service.createSession({ x: 0, y: 0, width: 100, height: 100 }, '', 1); + service.createSession( + { x: 0, y: 0, width: 100, height: 100 }, + '', + 1 + ); expect(addon.createSession).toHaveBeenCalledWith( expect.any(Buffer), @@ -1112,4 +1116,33 @@ describe('EmbeddedMpvNativeService power blocker', () => { }) ); }); + + it('completes teardown when the native dispose throws', () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(); + const failure = new Error('native session already gone'); + startSession('s-throws', snapshot('playing')); + addon.disposeSession.mockImplementationOnce(() => { + throw failure; + }); + + // A `return` inside `finally` used to swallow this, so `shutdown()` + // could not tell a failed dispose from a clean one. + const disposed = service.disposeSession('s-throws'); + + expect(disposed?.id).toBe('s-throws'); + expect(disposed?.status).toBe('closed'); + expect(mainWindowSendMock).toHaveBeenLastCalledWith( + 'EMBEDDED_MPV_SESSION_UPDATE', + expect.objectContaining({ id: 's-throws', status: 'closed' }) + ); + expect(warn).toHaveBeenCalledWith( + 'Failed to dispose embedded mpv session s-throws:', + failure + ); + + // The session is gone, so a second dispose finds nothing. + expect(service.disposeSession('s-throws')).toBeNull(); + + warn.mockRestore(); + }); }); diff --git a/apps/electron-backend/src/app/services/embedded-mpv-native.service.ts b/apps/electron-backend/src/app/services/embedded-mpv-native.service.ts index 8142fb66b..7f17f50e1 100644 --- a/apps/electron-backend/src/app/services/embedded-mpv-native.service.ts +++ b/apps/electron-backend/src/app/services/embedded-mpv-native.service.ts @@ -663,32 +663,41 @@ export class EmbeddedMpvNativeService { lastRecording = undefined; } addon.disposeSession(sessionId); - } finally { - this.sessions.delete(sessionId); - this.pollFailuresLogged.delete(sessionId); - const payload: EmbeddedMpvSession = { - id: session.id, - title: session.title, - streamUrl: session.streamUrl, - status: 'closed', - positionSeconds: 0, - durationSeconds: null, - volume: 1, - audioTracks: [], - selectedAudioTrackId: null, - subtitleTracks: [], - selectedSubtitleTrackId: null, - playbackSpeed: 1, - aspectOverride: 'no', - recording: this.createClosedRecordingState(lastRecording), - startedAt: session.startedAt, - updatedAt: new Date().toISOString(), - }; - this.sendSessionUpdate(payload); - this.stopPollingIfIdle(); - this.updatePowerBlocker(); - return payload; + } catch (error) { + // Teardown below must run even when the native session is already + // gone — `shutdown()` disposes every session in turn and must not + // stop at the first failure. A `return` inside `finally` achieved + // that but also swallowed the reason, so log it instead. + console.warn( + `Failed to dispose embedded mpv session ${sessionId}:`, + error + ); } + + this.sessions.delete(sessionId); + this.pollFailuresLogged.delete(sessionId); + const payload: EmbeddedMpvSession = { + id: session.id, + title: session.title, + streamUrl: session.streamUrl, + status: 'closed', + positionSeconds: 0, + durationSeconds: null, + volume: 1, + audioTracks: [], + selectedAudioTrackId: null, + subtitleTracks: [], + selectedSubtitleTrackId: null, + playbackSpeed: 1, + aspectOverride: 'no', + recording: this.createClosedRecordingState(lastRecording), + startedAt: session.startedAt, + updatedAt: new Date().toISOString(), + }; + this.sendSessionUpdate(payload); + this.stopPollingIfIdle(); + this.updatePowerBlocker(); + return payload; } shutdown(): void { @@ -979,6 +988,9 @@ export class EmbeddedMpvNativeService { private sanitizeRecordingFileName(title: string): string { const normalized = title + // Stripping control characters is precisely what a filename + // sanitizer must do, so the rule does not apply here. + // eslint-disable-next-line no-control-regex .replace(/[<>:"/\\|?*\u0000-\u001f]/g, '_') .replace(/\s+/g, ' ') .trim(); diff --git a/apps/electron-backend/src/app/workers/database.worker-connection.ts b/apps/electron-backend/src/app/workers/database.worker-connection.ts index 6a2b29784..e19fa7753 100644 --- a/apps/electron-backend/src/app/workers/database.worker-connection.ts +++ b/apps/electron-backend/src/app/workers/database.worker-connection.ts @@ -15,17 +15,14 @@ import { trace, } from '../services/debug-trace'; -let Database: typeof BetterSqlite3; let drizzleFactory: - | (typeof import('drizzle-orm/better-sqlite3'))['drizzle'] - | undefined; + (typeof import('drizzle-orm/better-sqlite3'))['drizzle'] | undefined; const nativeModuleSearchPaths = [ ...getWorkerDataNativeModuleSearchPaths(workerData), ...getNativeModuleSearchPaths({ - resourcesPath: ( - process as NodeJS.Process & { resourcesPath?: string } - ).resourcesPath, + resourcesPath: (process as NodeJS.Process & { resourcesPath?: string }) + .resourcesPath, }), ]; @@ -50,14 +47,13 @@ function getDrizzleFactory(): (typeof import('drizzle-orm/better-sqlite3'))['dri // Require drizzle only after native lookup paths have been registered. // Its better-sqlite3 driver resolves the native package at module load time. // eslint-disable-next-line @typescript-eslint/no-require-imports - drizzleFactory = require('drizzle-orm/better-sqlite3').drizzle as ( - typeof import('drizzle-orm/better-sqlite3') - )['drizzle']; + drizzleFactory = require('drizzle-orm/better-sqlite3') + .drizzle as (typeof import('drizzle-orm/better-sqlite3'))['drizzle']; return drizzleFactory; } -Database = loadBetterSqlite3(); +const Database: typeof BetterSqlite3 = loadBetterSqlite3(); let db: AppDatabase | null = null; let sqlite: BetterSqlite3.Database | null = null;