From 76804ee1bc00830d84598e5452a73caadcb0f03a Mon Sep 17 00:00:00 2001 From: 4gray Date: Wed, 29 Jul 2026 21:10:53 +0200 Subject: [PATCH] fix(portals): a failed pin read must not export as "no pins" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all in code from this session. Backup called the lenient `listForPlaylist`, which turns a failed read into `[]`. Since `e0ebbeaf` made restore treat `sourcePins` as authoritative — clearing the playlist's pins before applying it — an export whose read failed produced a file that looks complete and wipes every pin on restore. Losing them is bad; losing them through the one feature meant to protect them is worse. Backup now uses a strict listing that throws, so the export fails instead. The diacritic map stopped at U+024F, which is tidy and leaves Vietnamese out: `ố` is U+1ED1, the normalizer folds it to `o`, and the scan filtered those rows out before confirmation. Latin Extended Additional is included now; the filter decides what belongs, so the range only has to be wide. And two panels spelling one language differently (`eng` vs `en`, or `en-US`) raised a dub warning between identical tracks. Tags are canonicalized before comparison — 639-2 collapses to 639-1, both German forms meet at `de`, regions drop, and `und` becomes nothing. Anything that survives longer than three characters is not a language code, so the comparison is declined rather than guessed. Co-Authored-By: Claude Opus 5 --- .../operations/title-token-glob.spec.ts | 13 ++++ .../database/operations/title-token-glob.ts | 40 +++++++---- .../vod-source-metadata.util.spec.ts | 41 +++++++++++ .../multi-source/vod-source-metadata.util.ts | 53 ++++++++++++-- .../vod-multi-source-current-row.spec.ts | 7 +- .../lib/playlist-backup.service.pins.spec.ts | 35 +++++++-- .../playlist-backup.service.test-helpers.ts | 4 +- .../src/lib/playlist-backup.service.ts | 6 +- .../src/lib/vod-source-pin.service.spec.ts | 71 +++++++++++++++++++ .../src/lib/vod-source-pin.service.ts | 35 ++++++--- 10 files changed, 267 insertions(+), 38 deletions(-) create mode 100644 libs/services/src/lib/vod-source-pin.service.spec.ts diff --git a/apps/electron-backend/src/app/database/operations/title-token-glob.spec.ts b/apps/electron-backend/src/app/database/operations/title-token-glob.spec.ts index ebc6e11b8..de9a80922 100644 --- a/apps/electron-backend/src/app/database/operations/title-token-glob.spec.ts +++ b/apps/electron-backend/src/app/database/operations/title-token-glob.spec.ts @@ -135,6 +135,19 @@ describeWithSqlite('caseInsensitiveGlobPattern against SQLite', () => { expect(sqliteGlob('Ca', pattern)).toBe(true); }); + it('reaches beyond Latin Extended-B for the fold', () => { + // "Bố" (U+1ED1) is Vietnamese and normalizes to "bo" like any other + // accented o. A range that stops at Latin Extended-B looks tidy and + // leaves a whole language filtered out before confirmation. + const pattern = caseInsensitiveGlobPattern('bo', { + foldDiacritics: true, + }) as string; + + expect(sqliteGlob('Bố', pattern)).toBe(true); + expect(sqliteGlob('bồ', pattern)).toBe(true); + expect(sqliteGlob('Bo', pattern)).toBe(true); + }); + it('does not fold diacritics unless asked', () => { const pattern = caseInsensitiveGlobPattern('ca') as string; diff --git a/apps/electron-backend/src/app/database/operations/title-token-glob.ts b/apps/electron-backend/src/app/database/operations/title-token-glob.ts index f9f612c97..299743ab6 100644 --- a/apps/electron-backend/src/app/database/operations/title-token-glob.ts +++ b/apps/electron-backend/src/app/database/operations/title-token-glob.ts @@ -25,23 +25,39 @@ const GLOB_METACHARACTERS = /[*?[\]^-]/; */ const ACCENTED_BY_BASE = ((): Map => { const byBase = new Map(); - // Latin-1 Supplement through Latin Extended-B, which is where the accented - // forms of ASCII letters live. - for (let codePoint = 0xc0; codePoint <= 0x24f; codePoint += 1) { - const character = String.fromCodePoint(codePoint); - const base = character.normalize('NFD').replace(/\p{M}/gu, ''); - if (base.length !== 1 || !/[a-z]/i.test(base)) { - continue; + // Latin-1 Supplement through Latin Extended-B, PLUS Latin Extended + // Additional — which is where Vietnamese lives (ố is U+1ED1). Stopping at + // Latin Extended-B looks tidy and silently leaves a whole language out, + // while the normalizer folds it perfectly well. The filter below decides + // what actually belongs, so a range only has to be wide enough. + const ranges: ReadonlyArray<[number, number]> = [ + [0x00c0, 0x024f], + [0x1e00, 0x1eff], + ]; + for (const [first, last] of ranges) { + for (let codePoint = first; codePoint <= last; codePoint += 1) { + addAccentedForm(byBase, String.fromCodePoint(codePoint)); } - - const key = base.toLowerCase(); - const forms = byBase.get(key) ?? []; - forms.push(character); - byBase.set(key, forms); } return byBase; })(); +/** Files one character under the plain ASCII letter it decomposes to. */ +function addAccentedForm( + byBase: Map, + character: string +): void { + const base = character.normalize('NFD').replace(/\p{M}/gu, ''); + if (base.length !== 1 || !/[a-z]/i.test(base)) { + return; + } + + const key = base.toLowerCase(); + const forms = byBase.get(key) ?? []; + forms.push(character); + byBase.set(key, forms); +} + /** * A `*…*` containment pattern matching `token` in any case, or `null` when the * token cannot be expressed safely. diff --git a/libs/portal/shared/data-access/src/lib/multi-source/vod-source-metadata.util.spec.ts b/libs/portal/shared/data-access/src/lib/multi-source/vod-source-metadata.util.spec.ts index de85c0903..c6d3a6d86 100644 --- a/libs/portal/shared/data-access/src/lib/multi-source/vod-source-metadata.util.spec.ts +++ b/libs/portal/shared/data-access/src/lib/multi-source/vod-source-metadata.util.spec.ts @@ -164,6 +164,33 @@ describe('applyApiMetadata', () => { ).toEqual({ value: '576p', provenance: 'api' }); }); + it.each([ + ['eng', 'en'], + ['en', 'en'], + ['en-US', 'en'], + // Both ISO 639-2 spellings of German have to land together. + ['ger', 'de'], + ['deu', 'de'], + // No two-letter code exists for these, so three is canonical. + ['fil', 'fil'], + ])('canonicalizes the language tag %s', (raw, expected) => { + expect( + applyApiMetadata(candidate(), { audioLanguage: raw }).audioLanguage + ).toEqual({ value: expected, provenance: 'api' }); + }); + + it.each([ + // ffprobe's marker for "we do not know". + ['und'], + ['Russian'], + ['en_US'], + [''], + ])('states no language for %s', (raw) => { + expect( + applyApiMetadata(candidate(), { audioLanguage: raw }).audioLanguage + ).toBeUndefined(); + }); + it('refuses a wide frame taller than the format its width names', () => { // 1440x1080 is anamorphic 1080 and 1600x900 is 900p. Both fall in the // 1200-1699 band, so both were published as "720p" — with `api` @@ -319,6 +346,20 @@ describe('audioDiffersFactually', () => { expect(audioDiffersFactually(null, known)).toBe(false); }); + it('stays silent when two panels spell one language differently', () => { + // ffprobe says `eng`, a panel hoists `en`, and both mean English. A + // literal comparison turns that into a dub change on every switch + // between those two portals. + const from = candidate( + applyApiMetadata(candidate(), { audioLanguage: 'eng' }) + ); + const to = candidate( + applyApiMetadata(candidate(), { audioLanguage: 'en-US' }) + ); + + expect(audioDiffersFactually(from, to)).toBe(false); + }); + it('stays silent when both sides state the same track', () => { const from = candidate({ audioLanguage: { value: 'rus', provenance: 'api' }, diff --git a/libs/portal/shared/data-access/src/lib/multi-source/vod-source-metadata.util.ts b/libs/portal/shared/data-access/src/lib/multi-source/vod-source-metadata.util.ts index 824f269ae..f3ec6ed5e 100644 --- a/libs/portal/shared/data-access/src/lib/multi-source/vod-source-metadata.util.ts +++ b/libs/portal/shared/data-access/src/lib/multi-source/vod-source-metadata.util.ts @@ -144,12 +144,9 @@ export function applyApiMetadata( next.audio = { value: audio, provenance: 'api' }; } - const language = cleanString(api.audioLanguage); + const language = canonicalLanguage(api.audioLanguage); if (language) { - next.audioLanguage = { - value: language.toLowerCase(), - provenance: 'api', - }; + next.audioLanguage = { value: language, provenance: 'api' }; } const quality = qualityFromDimensions(api.width, api.height); @@ -210,6 +207,52 @@ function keepFactual(field?: VodSourceField): VodSourceField | undefined { return field && field.provenance !== 'parsed' ? field : undefined; } +/** + * A language tag reduced to the one form two panels can be compared on. + * + * The same spoken language is written several ways in the wild — ffprobe emits + * ISO 639-2 (`eng`, and `ger` or `deu` for German), panels hoist 639-1 (`en`), + * and either may carry a region (`en-US`). Comparing those literally makes a + * dub "change" out of two spellings of English. + * + * `Intl.Locale` does the canonicalizing: 639-2 collapses to 639-1 where one + * exists, both German forms land on `de`, and regions drop away. `und` — + * ffprobe's marker for undetermined — canonicalizes to nothing, which is + * exactly right: it means the provider does not know either. + * + * Anything that survives with more than three characters is not a language + * code (`Russian` parses as the subtag `russian`), so the comparison is + * declined rather than guessed at. + */ +function canonicalLanguage(raw: string | null | undefined): string | null { + const value = cleanString(raw); + if (!value || !LocaleCtor) { + return null; + } + + try { + const language = new LocaleCtor(value).language; + return language && language.length <= 3 ? language : null; + } catch { + // Not a well-formed tag at all; saying nothing beats comparing junk. + return null; + } +} + +/** + * `Intl.Locale` is ES2020 and this workspace compiles against the es2018 lib, + * so it is reached through a narrow shim rather than by moving the target. + * + * Absent — which no supported runtime actually is — the comparison is declined + * rather than half-canonicalized, since `eng` measured against `en` by a + * partial fold is exactly the false warning this function exists to prevent. + */ +const LocaleCtor = ( + Intl as unknown as { + Locale?: new (tag: string) => { language?: string }; + } +).Locale; + function cleanString(raw: string | null | undefined): string | null { const trimmed = typeof raw === 'string' ? raw.trim() : ''; return trimmed === '' ? null : trimmed; diff --git a/libs/portal/xtream/feature/src/lib/vod-details/vod-multi-source-current-row.spec.ts b/libs/portal/xtream/feature/src/lib/vod-details/vod-multi-source-current-row.spec.ts index 0a1772db0..9bd08742b 100644 --- a/libs/portal/xtream/feature/src/lib/vod-details/vod-multi-source-current-row.spec.ts +++ b/libs/portal/xtream/feature/src/lib/vod-details/vod-multi-source-current-row.spec.ts @@ -28,6 +28,7 @@ function alternative(language: string): VodSourceCandidate { rawTitle: 'Dune', matchConfidence: 'exact', year: null, + // Already canonical, as `applyApiMetadata` would have produced. audioLanguage: { value: language, provenance: 'api' }, } as VodSourceCandidate; } @@ -80,8 +81,8 @@ describe('currentSourceRow', () => { // route row made the warning structurally unreachable on the // commonest switch there is — route to alternative. It only ever // fired between two alternatives that had both been resolved. - expect(audioDiffersFactually(row, alternative('eng'))).toBe(true); - expect(audioDiffersFactually(row, alternative('rus'))).toBe(false); + expect(audioDiffersFactually(row, alternative('en'))).toBe(true); + expect(audioDiffersFactually(row, alternative('ru'))).toBe(false); }); it('does not call a codec change a dub change', () => { @@ -97,6 +98,6 @@ describe('currentSourceRow', () => { // and warning anyway trains people to ignore the one that matters. expect(row.audio?.value).toBe('ac3'); expect(row.audioLanguage).toBeUndefined(); - expect(audioDiffersFactually(row, alternative('eng'))).toBe(false); + expect(audioDiffersFactually(row, alternative('en'))).toBe(false); }); }); diff --git a/libs/services/src/lib/playlist-backup.service.pins.spec.ts b/libs/services/src/lib/playlist-backup.service.pins.spec.ts index a4b77fbd6..30178f71b 100644 --- a/libs/services/src/lib/playlist-backup.service.pins.spec.ts +++ b/libs/services/src/lib/playlist-backup.service.pins.spec.ts @@ -25,13 +25,34 @@ describe('PlaylistBackupService Xtream source pins', () => { localStorage.clear(); }); + it('refuses to export a backup whose pin read failed', async () => { + const collaborators = createRestoreCollaborators(); + const service = createPlaylistBackupService({ + playlistsService: collaborators.playlistsService, + databaseService: collaborators.databaseService, + vodSourcePinService: { + listForPlaylistOrThrow: jest + .fn() + .mockRejectedValue(new Error('database is locked')), + set: jest.fn().mockResolvedValue(true), + }, + }); + + // `sourcePins` is authoritative and restore CLEARS the playlist's pins + // before applying it, so a read that failed and reported "none" would + // produce a file that looks complete and wipes every pin the user had + // — through the one feature meant to protect them. Failing the export + // is the only honest outcome. + await expect(service.exportBackup()).rejects.toThrow(); + }); + it('exports the pins that point at this playlist', async () => { const collaborators = createRestoreCollaborators(); const service = createPlaylistBackupService({ playlistsService: collaborators.playlistsService, databaseService: collaborators.databaseService, vodSourcePinService: { - listForPlaylist: jest.fn().mockResolvedValue([ + listForPlaylistOrThrow: jest.fn().mockResolvedValue([ { matchKey: 'tmdb:603', playlistId: 'xtream-1', @@ -65,7 +86,7 @@ describe('PlaylistBackupService Xtream source pins', () => { const service = createPlaylistBackupService({ ...collaborators, vodSourcePinService: { - listForPlaylist: jest.fn().mockResolvedValue([]), + listForPlaylistOrThrow: jest.fn().mockResolvedValue([]), set: setPin, clear: jest.fn().mockResolvedValue(true), clearForPlaylist: jest.fn().mockResolvedValue(true), @@ -94,7 +115,7 @@ describe('PlaylistBackupService Xtream source pins', () => { const service = createPlaylistBackupService({ ...collaborators, vodSourcePinService: { - listForPlaylist: jest.fn().mockResolvedValue([]), + listForPlaylistOrThrow: jest.fn().mockResolvedValue([]), // `set` reports failure rather than throwing, so ignoring the // result would drop the preference while the summary claims // the import succeeded. @@ -120,7 +141,7 @@ describe('PlaylistBackupService Xtream source pins', () => { const service = createPlaylistBackupService({ ...collaborators, vodSourcePinService: { - listForPlaylist: jest.fn().mockResolvedValue([ + listForPlaylistOrThrow: jest.fn().mockResolvedValue([ { matchKey: 'tmdb:999', playlistId: 'xtream-1', @@ -149,7 +170,7 @@ describe('PlaylistBackupService Xtream source pins', () => { const service = createPlaylistBackupService({ ...collaborators, vodSourcePinService: { - listForPlaylist: jest.fn().mockResolvedValue([]), + listForPlaylistOrThrow: jest.fn().mockResolvedValue([]), set: jest.fn().mockResolvedValue(true), clear: jest.fn().mockResolvedValue(true), clearForPlaylist: jest.fn().mockResolvedValue(false), @@ -175,7 +196,7 @@ describe('PlaylistBackupService Xtream source pins', () => { vodSourcePinService: { // A pin EXISTS, or an empty list would make this pass whether // or not the clear was skipped. - listForPlaylist: jest.fn().mockResolvedValue([ + listForPlaylistOrThrow: jest.fn().mockResolvedValue([ { matchKey: 'tmdb:999', playlistId: 'xtream-1', @@ -202,7 +223,7 @@ describe('PlaylistBackupService Xtream source pins', () => { const service = createPlaylistBackupService({ ...collaborators, vodSourcePinService: { - listForPlaylist: jest.fn().mockResolvedValue([]), + listForPlaylistOrThrow: jest.fn().mockResolvedValue([]), set: setPin, clear: jest.fn().mockResolvedValue(true), clearForPlaylist: jest.fn().mockResolvedValue(true), diff --git a/libs/services/src/lib/playlist-backup.service.test-helpers.ts b/libs/services/src/lib/playlist-backup.service.test-helpers.ts index 6676e8131..a14f9f3af 100644 --- a/libs/services/src/lib/playlist-backup.service.test-helpers.ts +++ b/libs/services/src/lib/playlist-backup.service.test-helpers.ts @@ -63,7 +63,7 @@ export function createPlaylistBackupService( savePlaybackPosition: jest.fn().mockResolvedValue(undefined), }, vodSourcePinService: { - listForPlaylist: jest.fn().mockResolvedValue([]), + listForPlaylistOrThrow: jest.fn().mockResolvedValue([]), set: jest.fn().mockResolvedValue(true), clear: jest.fn().mockResolvedValue(true), clearForPlaylist: jest.fn().mockResolvedValue(true), @@ -240,7 +240,7 @@ export function createStatefulBackupCollaborators( }, }, vodSourcePinService: { - listForPlaylist: async (playlistId: string) => + listForPlaylistOrThrow: async (playlistId: string) => state.sourcePins .filter((pin) => pin.playlistId === playlistId) .map((pin) => ({ ...pin })), diff --git a/libs/services/src/lib/playlist-backup.service.ts b/libs/services/src/lib/playlist-backup.service.ts index 52adf7c18..32a1663f2 100644 --- a/libs/services/src/lib/playlist-backup.service.ts +++ b/libs/services/src/lib/playlist-backup.service.ts @@ -274,7 +274,11 @@ export class PlaylistBackupService { this.databaseService.getFavorites(playlist._id), this.databaseService.getRecentItems(playlist._id), this.playbackPositionService.getAllPlaybackPositions(playlist._id), - this.vodSourcePinService.listForPlaylist(playlist._id), + // Throws rather than reading a failure as "no pins": restore + // treats this collection as authoritative and clears the + // playlist's pins before applying it, so an empty list born of a + // failed read would wipe them. + this.vodSourcePinService.listForPlaylistOrThrow(playlist._id), ]); return { diff --git a/libs/services/src/lib/vod-source-pin.service.spec.ts b/libs/services/src/lib/vod-source-pin.service.spec.ts new file mode 100644 index 000000000..d7b8a8c25 --- /dev/null +++ b/libs/services/src/lib/vod-source-pin.service.spec.ts @@ -0,0 +1,71 @@ +import { VodSourcePinService } from './vod-source-pin.service'; + +/** + * The two listing methods differ in ONE way that matters: what a failed read + * means. Backup depends on that difference — its `sourcePins` collection is + * authoritative and restore clears the playlist's pins before applying it, so + * a failure reported as "no pins" writes a backup that silently wipes them. + */ + +const electronWindow = window as unknown as { + electron?: Record; +}; + +function withBridge(dbListVodSourcePins: unknown) { + electronWindow.electron = { + // `isAvailable` probes for this one. + dbGetVodSourcePin: jest.fn(), + dbListVodSourcePins, + }; +} + +describe('VodSourcePinService listing', () => { + let service: VodSourcePinService; + let warn: jest.SpyInstance; + + beforeEach(() => { + service = new VodSourcePinService(); + warn = jest.spyOn(console, 'warn').mockImplementation(() => undefined); + }); + + afterEach(() => { + delete electronWindow.electron; + warn.mockRestore(); + }); + + it('returns the pins the bridge reports', async () => { + const pin = { + matchKey: 'tmdb:603', + playlistId: 'p1', + contentId: 501, + portalType: 'xtream' as const, + }; + withBridge(jest.fn().mockResolvedValue([pin])); + + await expect(service.listForPlaylist('p1')).resolves.toEqual([pin]); + await expect(service.listForPlaylistOrThrow('p1')).resolves.toEqual([ + pin, + ]); + }); + + it('reads a failure as "no pins" only for the lenient caller', async () => { + withBridge( + jest.fn().mockRejectedValue(new Error('database is locked')) + ); + + await expect(service.listForPlaylist('p1')).resolves.toEqual([]); + // The whole point of the second method: backup must be able to tell a + // failed read from an empty one, or it exports a wipe. + await expect(service.listForPlaylistOrThrow('p1')).rejects.toThrow( + 'database is locked' + ); + }); + + it('reports no pins, without throwing, outside Electron', async () => { + delete electronWindow.electron; + + // Not a failure — there is no pin store in the PWA at all, so an empty + // list is the true answer and a backup built there is complete. + await expect(service.listForPlaylistOrThrow('p1')).resolves.toEqual([]); + }); +}); diff --git a/libs/services/src/lib/vod-source-pin.service.ts b/libs/services/src/lib/vod-source-pin.service.ts index a0c58d279..8de5b054a 100644 --- a/libs/services/src/lib/vod-source-pin.service.ts +++ b/libs/services/src/lib/vod-source-pin.service.ts @@ -67,16 +67,13 @@ export class VodSourcePinService { } } - /** Every pin pointing at this playlist. Used by playlist backup. */ + /** + * Every pin pointing at this playlist, empty when there are none OR when + * the read failed. Only for callers that treat both the same way. + */ async listForPlaylist(playlistId: string): Promise { - if (!this.isAvailable || !playlistId) { - return []; - } - try { - return ( - (await window.electron.dbListVodSourcePins(playlistId)) ?? [] - ); + return await this.readForPlaylist(playlistId); } catch (error) { console.warn( 'Listing pinned VOD sources failed:', @@ -86,6 +83,28 @@ export class VodSourcePinService { } } + /** + * As above, but a failed read THROWS instead of reading as "none". + * + * Backup needs this distinction and nothing else does. An export writes + * `sourcePins` as the authoritative set for the playlist, and restore + * clears whatever is there before applying it — so a read that failed and + * returned `[]` produces a backup that looks complete and silently wipes + * every pin the user had. Losing the preferences is bad; losing them via a + * file created to protect them is worse. + */ + async listForPlaylistOrThrow(playlistId: string): Promise { + return this.readForPlaylist(playlistId); + } + + private async readForPlaylist(playlistId: string): Promise { + if (!this.isAvailable || !playlistId) { + return []; + } + + return (await window.electron.dbListVodSourcePins(playlistId)) ?? []; + } + /** * Store the pin, also under `aliasKeys`, retiring `retireKeys` — all in * the same transaction, because a half-applied change has no honest