From ea3dacb0528d235edd3e87fd8550a333149e1514 Mon Sep 17 00:00:00 2001 From: 4gray Date: Wed, 14 Oct 2026 01:45:55 +0200 Subject: [PATCH] fix(sources): hand over only to an idle source; hide names that are a short secret - Removing the active source could hand over to a neighbour a refresh was still clearing, opening it and starting its import early. `SourceRemovalService.canTakeOver` (not leaving, not deleting, not held in SourceActivityService) now picks the replacement in both lists and gates it in the service. - Secrets under three characters were dropped from the check entirely, so a name that was just a two-letter password showed it. Short secrets now count as the whole text (trimmed, any case); longer ones still match inside a text. Co-Authored-By: Claude Opus 5.5 --- .../playlist-switcher-panel.component.spec.ts | 13 +++++++ .../playlist-switcher-panel.component.ts | 2 +- .../recent-playlists.component.spec.ts | 17 +++++++++ .../recent-playlists.component.ts | 2 +- .../source-removal.service.spec.ts | 9 +++++ .../source-removal/source-removal.service.ts | 21 +++++++---- .../lib/source-display/source-display.spec.ts | 21 +++++++++++ .../src/lib/source-display/source-display.ts | 37 ++++++++++--------- 8 files changed, 95 insertions(+), 27 deletions(-) diff --git a/libs/playlist/shared/ui/src/lib/playlist-switcher/playlist-switcher-panel.component.spec.ts b/libs/playlist/shared/ui/src/lib/playlist-switcher/playlist-switcher-panel.component.spec.ts index ad3793886..17f33aade 100644 --- a/libs/playlist/shared/ui/src/lib/playlist-switcher/playlist-switcher-panel.component.spec.ts +++ b/libs/playlist/shared/ui/src/lib/playlist-switcher/playlist-switcher-panel.component.spec.ts @@ -133,6 +133,9 @@ describe('PlaylistSwitcherPanelComponent', () => { progress: { isDeleting: (id: string) => deleting().has(id), }, + canTakeOver: (id: string) => + !deleting().has(id) && + !TestBed.inject(SourceActivityService).isBusy(id), }, }, { @@ -348,6 +351,16 @@ describe('PlaylistSwitcherPanelComponent', () => { expect(selectPlaylist).toHaveBeenCalledWith(ALPHA); }); + it('hands over past a source a refresh holds when removing the current one', () => { + // Bravo is current; Charlie would follow it but is refreshing. + const release = TestBed.inject(SourceActivityService).begin([ + 'charlie', + ]); + component.remove(BRAVO); + expect(remove).toHaveBeenCalledWith(BRAVO, ALPHA, expect.anything()); + release(); + }); + it('leaves out a source whose delete is pending or running', () => { pending.set(new Set(['bravo'])); fixture.detectChanges(); diff --git a/libs/playlist/shared/ui/src/lib/playlist-switcher/playlist-switcher-panel.component.ts b/libs/playlist/shared/ui/src/lib/playlist-switcher/playlist-switcher-panel.component.ts index 812fe2f8a..8306de236 100644 --- a/libs/playlist/shared/ui/src/lib/playlist-switcher/playlist-switcher-panel.component.ts +++ b/libs/playlist/shared/ui/src/lib/playlist-switcher/playlist-switcher-panel.component.ts @@ -334,7 +334,7 @@ export class PlaylistSwitcherPanelComponent { const next = replacementAfterRemoval( this.groups().all.map((option) => option.playlist), playlist._id, - (row) => this.removal.progress.isDeleting(row._id) + (row) => !this.removal.canTakeOver(row._id) ); this.close(); // Closing put the focus back on the trigger. diff --git a/libs/playlist/shared/ui/src/lib/recent-playlists/recent-playlists.component.spec.ts b/libs/playlist/shared/ui/src/lib/recent-playlists/recent-playlists.component.spec.ts index 2c885a471..06ddd76b5 100644 --- a/libs/playlist/shared/ui/src/lib/recent-playlists/recent-playlists.component.spec.ts +++ b/libs/playlist/shared/ui/src/lib/recent-playlists/recent-playlists.component.spec.ts @@ -63,6 +63,7 @@ describe('RecentPlaylistsComponent', () => { pendingIds: typeof pending; remove: jest.Mock; isPending: jest.Mock; + canTakeOver: jest.Mock; progress: object; }; let viewState: SourcesViewStateService; @@ -84,6 +85,15 @@ describe('RecentPlaylistsComponent', () => { pendingIds: pending, remove: jest.fn(), isPending: jest.fn((id: string) => pending().has(id)), + canTakeOver: jest.fn( + (id: string) => + !( + removal.progress as { + isDeleting: (id: string) => boolean; + } + ).isDeleting(id) && + !TestBed.inject(SourceActivityService).isBusy(id) + ), progress: { isDeleting: () => false, message: () => '', @@ -227,6 +237,13 @@ describe('RecentPlaylistsComponent', () => { expect(removal.remove).toHaveBeenCalledWith(M3U, FILE); }); + it('hands over past a source a refresh holds', () => { + const release = TestBed.inject(SourceActivityService).begin(['xtream']); + component.remove(M3U); + expect(removal.remove).toHaveBeenCalledWith(M3U, FILE); + release(); + }); + it('keeps a running delete on screen whatever the filters', () => { ( removal.progress as { isDeleting: (id: string) => boolean } diff --git a/libs/playlist/shared/ui/src/lib/recent-playlists/recent-playlists.component.ts b/libs/playlist/shared/ui/src/lib/recent-playlists/recent-playlists.component.ts index e126b3467..2e434fbef 100644 --- a/libs/playlist/shared/ui/src/lib/recent-playlists/recent-playlists.component.ts +++ b/libs/playlist/shared/ui/src/lib/recent-playlists/recent-playlists.component.ts @@ -217,7 +217,7 @@ export class RecentPlaylistsComponent { const next = replacementAfterRemoval( this.visiblePlaylists(), playlist._id, - (row) => this.removal.progress.isDeleting(row._id) + (row) => !this.removal.canTakeOver(row._id) ); this.removal.remove(playlist, next); } diff --git a/libs/playlist/shared/ui/src/lib/source-removal/source-removal.service.spec.ts b/libs/playlist/shared/ui/src/lib/source-removal/source-removal.service.spec.ts index 3193c6af8..0ce7323d6 100644 --- a/libs/playlist/shared/ui/src/lib/source-removal/source-removal.service.spec.ts +++ b/libs/playlist/shared/ui/src/lib/source-removal/source-removal.service.spec.ts @@ -233,6 +233,15 @@ describe('SourceRemovalService', () => { expect(toasts[0].message).toBe('WORKSPACE.SOURCES.REMOVED'); }); + it('does not hand over to a source a refresh or import holds', () => { + resolvedPlaylistId.set('a'); + const release = TestBed.inject(SourceActivityService).begin(['b']); + service.remove(playlist('a'), playlist('b')); + expect(context.selectPlaylist).not.toHaveBeenCalled(); + expect(toasts[0].message).toBe('WORKSPACE.SOURCES.REMOVED'); + release(); + }); + it('leaves a switch the user made after the delete alone on Undo', () => { resolvedPlaylistId.set('a'); service.remove(playlist('a'), playlist('b')); diff --git a/libs/playlist/shared/ui/src/lib/source-removal/source-removal.service.ts b/libs/playlist/shared/ui/src/lib/source-removal/source-removal.service.ts index d1028742c..9e267df9a 100644 --- a/libs/playlist/shared/ui/src/lib/source-removal/source-removal.service.ts +++ b/libs/playlist/shared/ui/src/lib/source-removal/source-removal.service.ts @@ -85,6 +85,19 @@ export class SourceRemovalService { return this.pendingState().has(id); } + /** + * Whether a source can become active in place of a removed one: not + * leaving itself, and not held by a refresh, import or delete, since + * opening it would start an import against a catalog being rewritten. + */ + canTakeOver(id: string): boolean { + return ( + !this.isPending(id) && + !this.progress.isDeleting(id) && + !this.activity.isBusy(id) + ); + } + /** * @param next The source to switch to when `playlist` is the active one: * the caller's next row, so the switch follows the list on screen. @@ -112,14 +125,8 @@ export class SourceRemovalService { const wasOnItsRoute = this.playlistContext.routePlaylistId() === playlist._id; const previousUrl = this.router.url; - // A source that is leaving too cannot take over. const replacement = - wasActive && - next && - !this.isPending(next._id) && - !this.progress.isDeleting(next._id) - ? next - : null; + wasActive && next && this.canTakeOver(next._id) ? next : null; this.setPending(playlist._id, true); if (replacement) { diff --git a/libs/playlist/shared/util/src/lib/source-display/source-display.spec.ts b/libs/playlist/shared/util/src/lib/source-display/source-display.spec.ts index 9087243df..2367cbb87 100644 --- a/libs/playlist/shared/util/src/lib/source-display/source-display.spec.ts +++ b/libs/playlist/shared/util/src/lib/source-display/source-display.spec.ts @@ -139,6 +139,27 @@ describe('sourceDisplayName', () => { ); }); + it('drops a name that is nothing but a short password', () => { + const xtream = { + serverUrl: 'http://xtream.tv:8080', + username: 'joe', + password: 'pw', + }; + expect(sourceDisplayName({ ...xtream, title: 'pw' })).toBe( + 'xtream.tv:8080' + ); + expect(sourceDisplayName({ ...xtream, title: ' PW ' })).toBe( + 'xtream.tv:8080' + ); + // Inside a longer name a short password is just letters. + expect(sourceDisplayName({ ...xtream, title: 'Pwnova' })).toBe( + 'Pwnova' + ); + expect(sourceDisplayUrl({ ...xtream, username: 'pw' })).toBe( + 'http://xtream.tv:8080' + ); + }); + it('keeps a name that only shares a letter with a decoded password', () => { // `%2542` is the password as written; decoded it is just `B`. const url = 'https://host.test/list?password=%2542'; diff --git a/libs/playlist/shared/util/src/lib/source-display/source-display.ts b/libs/playlist/shared/util/src/lib/source-display/source-display.ts index c10a71c45..bb4e48e93 100644 --- a/libs/playlist/shared/util/src/lib/source-display/source-display.ts +++ b/libs/playlist/shared/util/src/lib/source-display/source-display.ts @@ -27,7 +27,10 @@ export type SourceDisplayFields = Partial< /** The account name stays on screen by design; every other secret goes. */ const SHOWN_ACCOUNT_KEYS = new Set(['username', 'login', 'user', 'usr']); -/** Shorter values match ordinary words, not secrets. */ +/** + * Shorter secrets match ordinary words inside a text, so they count only + * as the whole text: a name that is just a two-letter password is hidden. + */ const MIN_SECRET_LENGTH = 3; const ABSOLUTE_URL = /^[a-z][a-z0-9+.-]*:\/\//i; @@ -185,24 +188,14 @@ function sourceSecrets(playlist: SourceDisplayFields): string[] { if (url) { collectUrlSecrets(url, secrets, 0); } - const shown: string[] = []; + const forms: string[] = []; for (const secret of secrets) { - if (!secret) { - continue; - } - // A value encoded more than once reads as itself in a name. Each - // form must be long enough on its own: `%2542` decodes to `B`. - for (const form of [ - secret, - encodeURIComponent(secret), - decodeFully(secret), - ]) { - if (form.length >= MIN_SECRET_LENGTH) { - shown.push(form); - } + if (secret) { + // A value encoded more than once reads as itself in a name. + forms.push(secret, encodeURIComponent(secret), decodeFully(secret)); } } - return shown; + return forms; } /** @@ -215,10 +208,18 @@ export function containsSourceSecret( text: string, playlist: SourceDisplayFields ): boolean { - const forms = [text, decodeFully(text)].map((form) => form.toLowerCase()); + const forms = [text, decodeFully(text)].map((form) => + form.trim().toLowerCase() + ); return sourceSecrets(playlist).some((secret) => { const needle = secret.toLowerCase(); - return forms.some((form) => form.includes(needle)); + // Each form on its own: `%2542` decodes to `B`, which may only hide + // a text that is nothing but `b`. + return forms.some( + (form) => + form === needle || + (needle.length >= MIN_SECRET_LENGTH && form.includes(needle)) + ); }); }