From 80a9cd946a5343a80eb4fbf20823d70ed1b5e234 Mon Sep 17 00:00:00 2001 From: 4gray Date: Tue, 13 Oct 2026 18:37:48 +0200 Subject: [PATCH] fix(sources): never hand over to a leaving source; browser checks per connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local review round 4 (Codex + Greptile): - Removing the active source handed over to its neighbour even when that neighbour's own delete was still running ("Removing…"), activating a source about to disappear. replacementAfterRemoval (playlist shared util) picks the nearest row that is not leaving, on the Sources page and in the switcher; SourceRemovalService also refuses a leaving replacement. - In the browser build a "Check again" for an account the user then edited could finish last and overwrite the new account's status under the same playlist ID. Portal statuses are now keyed by playlist and connection fingerprint, so the old check lands under the old key. - The switcher scrolled the highlighted option into view through a selector built from the playlist ID, which a restored ID with "." or ":" broke (or made throw). It now finds the row by its exact DOM ID. Co-Authored-By: Claude Opus 5.5 --- .../playlist-switcher-panel.component.spec.ts | 10 ++++++ .../playlist-switcher-panel.component.ts | 34 +++++++++++-------- .../recent-playlists.component.spec.ts | 8 +++++ .../recent-playlists.component.ts | 9 +++-- .../source-removal.service.spec.ts | 11 ++++++ .../source-removal/source-removal.service.ts | 9 ++++- .../lib/source-display/source-list.spec.ts | 21 ++++++++++++ .../src/lib/source-display/source-list.ts | 27 +++++++++++++++ .../src/lib/source-status.service.spec.ts | 20 +++++++++++ .../src/lib/source-status.service.ts | 33 ++++++++++++------ 10 files changed, 153 insertions(+), 29 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 60a221c88..d001c9838 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 @@ -194,6 +194,16 @@ describe('PlaylistSwitcherPanelComponent', () => { expect(closed).toHaveBeenCalled(); }); + it('moves through options whose IDs a selector would misread', () => { + const restored = { ...ALPHA, _id: 'odd:id.one', title: 'Aardvark' }; + playlists.set([restored, BRAVO]); + fixture.detectChanges(); + expect(() => press('ArrowDown')).not.toThrow(); + expect(input().getAttribute('aria-activedescendant')).toBe( + 'playlist-switcher-all-odd:id.one' + ); + }); + it('filters by name or host as you type and highlights the first match', () => { type('charlie.example'); expect(optionIds()).toEqual(['playlist-switcher-all-charlie']); 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 0bb26d390..f67f620de 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 @@ -27,6 +27,7 @@ import { orderFailingFirst, PlaylistContextFacade, PlaylistRecentHistoryService, + replacementAfterRemoval, SourceBucketOf, sourceSortName, SourcesTypeFilter, @@ -280,22 +281,21 @@ export class PlaylistSwitcherPanelComponent { * from Undo, which then takes the focus and returns it to the trigger. */ remove(playlist: PlaylistMeta): void { - const all = this.groups().all.map((option) => option.playlist); - const index = all.findIndex((item) => item._id === playlist._id); + const next = replacementAfterRemoval( + this.groups().all.map((option) => option.playlist), + playlist._id, + (row) => this.removal.progress.isDeleting(row._id) + ); this.close(); // Closing put the focus back on the trigger. const returnFocusTo = this.document.activeElement; - this.removal.remove( - playlist, - all[index + 1] ?? all[index - 1] ?? null, - { - returnFocusTo: - returnFocusTo instanceof HTMLElement && - returnFocusTo !== this.document.body - ? returnFocusTo - : null, - } - ); + this.removal.remove(playlist, next, { + returnFocusTo: + returnFocusTo instanceof HTMLElement && + returnFocusTo !== this.document.body + ? returnFocusTo + : null, + }); } /** A row menu gives the focus back to the search, unless closing. */ @@ -337,7 +337,13 @@ export class PlaylistSwitcherPanelComponent { private scrollHighlightIntoView(): void { const id = this.activeOptionId(); const list = this.list().nativeElement; - const option = id ? list.querySelector(`#${id}`) : null; + // By exact ID, not a selector: a restored playlist ID may hold "." + // or ":", which a selector would misread. + const option = id + ? this.rowElements() + .map((row) => row.nativeElement as HTMLElement) + .find((element) => element.id === id) + : undefined; if (!option) { return; } 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 2753485de..c6afca2b1 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 @@ -206,6 +206,14 @@ describe('RecentPlaylistsComponent', () => { expect(ids()).toEqual(['file', 'm3u', 'xtream', 'url']); }); + it('hands over past a source whose delete is still running', () => { + ( + removal.progress as { isDeleting: (id: string) => boolean } + ).isDeleting = (id) => id === 'xtream'; + component.remove(M3U); + expect(removal.remove).toHaveBeenCalledWith(M3U, FILE); + }); + it('applies the type and status filters together', () => { viewState.setType('m3u'); expect(ids()).toEqual(['m3u', 'file']); 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 bc602026e..d83cb582b 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 @@ -31,6 +31,7 @@ import { filterSources, orderFailingFirst, PlaylistContextFacade, + replacementAfterRemoval, SourceBucketOf, sourceSortName, SourcesViewStateService, @@ -201,9 +202,11 @@ export class RecentPlaylistsComponent { if (this.refresh.isRefreshing(playlist._id)) { return; } - const rows = this.visiblePlaylists(); - const index = rows.findIndex((item) => item._id === playlist._id); - const next = rows[index + 1] ?? rows[index - 1] ?? null; + const next = replacementAfterRemoval( + this.visiblePlaylists(), + playlist._id, + (row) => this.removal.progress.isDeleting(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 2f5b82f00..0b59a40fd 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 @@ -209,6 +209,17 @@ describe('SourceRemovalService', () => { expect(context.activatePlaylist).toHaveBeenCalledWith('a'); }); + it('does not hand over to a source that is being deleted', () => { + resolvedPlaylistId.set('a'); + service.progress.track('b', { + operation: 'delete-playlist', + status: 'started', + }); + service.remove(playlist('a'), playlist('b')); + expect(context.selectPlaylist).not.toHaveBeenCalled(); + expect(toasts[0].message).toBe('WORKSPACE.SOURCES.REMOVED'); + }); + 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 54839e20f..b24ff937e 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 @@ -105,7 +105,14 @@ export class SourceRemovalService { const wasOnItsRoute = this.playlistContext.routePlaylistId() === playlist._id; const previousUrl = this.router.url; - const replacement = wasActive ? next : null; + // A source that is leaving too cannot take over. + const replacement = + wasActive && + next && + !this.isPending(next._id) && + !this.progress.isDeleting(next._id) + ? next + : null; this.setPending(playlist._id, true); if (replacement) { diff --git a/libs/playlist/shared/util/src/lib/source-display/source-list.spec.ts b/libs/playlist/shared/util/src/lib/source-display/source-list.spec.ts index 9e5212239..861c41792 100644 --- a/libs/playlist/shared/util/src/lib/source-display/source-list.spec.ts +++ b/libs/playlist/shared/util/src/lib/source-display/source-list.spec.ts @@ -6,6 +6,7 @@ import { filterSources, matchesSourceQuery, orderFailingFirst, + replacementAfterRemoval, SourceBucketOf, } from './source-list'; import { @@ -98,6 +99,26 @@ describe('source list filters', () => { }); }); +describe('replacementAfterRemoval', () => { + const rows = ['a', 'b', 'c', 'd'].map((id) => ({ _id: id })); + const none = () => false; + + it('hands over to the next row, or the previous one at the end', () => { + expect(replacementAfterRemoval(rows, 'b', none)?._id).toBe('c'); + expect(replacementAfterRemoval(rows, 'd', none)?._id).toBe('c'); + expect(replacementAfterRemoval([{ _id: 'a' }], 'a', none)).toBeNull(); + }); + + it('skips rows that are leaving too', () => { + const deleting = (row: { _id: string }) => + row._id === 'c' || row._id === 'd'; + expect(replacementAfterRemoval(rows, 'b', deleting)?._id).toBe('a'); + expect( + replacementAfterRemoval(rows, 'a', (row) => row._id !== 'a') + ).toBeNull(); + }); +}); + describe('SourcesViewStateService', () => { const create = () => { TestBed.configureTestingModule({ diff --git a/libs/playlist/shared/util/src/lib/source-display/source-list.ts b/libs/playlist/shared/util/src/lib/source-display/source-list.ts index 32b19765e..d77ecbcc5 100644 --- a/libs/playlist/shared/util/src/lib/source-display/source-list.ts +++ b/libs/playlist/shared/util/src/lib/source-display/source-list.ts @@ -122,3 +122,30 @@ export function orderFailingFirst( } return [...failing, ...rest]; } + +/** + * The source that takes over when `removedId` leaves a list: the nearest + * row after it, else before it, skipping rows that are leaving too (a + * delete still running). Null when no other source can take over. + */ +export function replacementAfterRemoval( + rows: readonly T[], + removedId: string, + leaving: (row: T) => boolean +): T | null { + const index = rows.findIndex((row) => row._id === removedId); + if (index === -1) { + return null; + } + for (let next = index + 1; next < rows.length; next += 1) { + if (!leaving(rows[next])) { + return rows[next]; + } + } + for (let previous = index - 1; previous >= 0; previous -= 1) { + if (!leaving(rows[previous])) { + return rows[previous]; + } + } + return null; +} diff --git a/libs/portal/shared/data-access/src/lib/source-status.service.spec.ts b/libs/portal/shared/data-access/src/lib/source-status.service.spec.ts index cb5400f23..085319db3 100644 --- a/libs/portal/shared/data-access/src/lib/source-status.service.spec.ts +++ b/libs/portal/shared/data-access/src/lib/source-status.service.spec.ts @@ -232,6 +232,26 @@ describe('SourceStatusService', () => { }); }); + it("keeps an edited account's status from the old account's slow check", async () => { + let finishOld!: (status: string) => void; + portal.checkPortalStatus + .mockReturnValueOnce(new Promise((r) => (finishOld = r))) + .mockResolvedValueOnce('active'); + const service = create(false); + service.recheck(xtream); + + const edited = { ...xtream, password: 'changed' } as PlaylistMeta; + service.ensureChecked(edited, new AbortController().signal); + await Promise.resolve(); + await Promise.resolve(); + expect(service.statusOf(edited)?.reachability).toBe('ok'); + + finishOld('inactive'); + await Promise.resolve(); + await Promise.resolve(); + expect(service.statusOf(edited)?.reachability).toBe('ok'); + }); + it('drops a result that lands after the list closed', async () => { const service = create(false); const controller = new AbortController(); diff --git a/libs/portal/shared/data-access/src/lib/source-status.service.ts b/libs/portal/shared/data-access/src/lib/source-status.service.ts index 481628879..b6039757d 100644 --- a/libs/portal/shared/data-access/src/lib/source-status.service.ts +++ b/libs/portal/shared/data-access/src/lib/source-status.service.ts @@ -57,6 +57,11 @@ export class SourceStatusService { private readonly injector = inject(Injector); private readonly history = inject(SourceHealthHistoryService); private readonly portalStatus = inject(PortalStatusService); + /** + * Browser-build portal statuses by playlist and connection + * (`portalKey`): a slow check of an account the user has since edited + * lands under the old key and cannot overwrite the new account's. + */ private readonly portalStatuses = signal>( new Map() ); @@ -77,7 +82,7 @@ export class SourceStatusService { return null; } if (!this.runtime.supportsSourceHealth) { - const status = this.portalStatuses().get(playlist._id); + const status = this.portalStatuses().get(portalKey(playlist)); return status ? { reachability: sourceReachabilityFromPortalStatus(status), @@ -147,16 +152,17 @@ export class SourceStatusService { fresh: boolean, signal?: AbortSignal ): Promise { - const { _id, serverUrl, username, password } = playlist; + const { serverUrl, username, password } = playlist; + const key = portalKey(playlist); const cached = fresh ? null : this.portalStatus.getCachedStatus(serverUrl, username, password); if (cached) { - this.setPortalStatus(_id, cached); + this.setPortalStatus(key, cached); return; } - if (fresh || !this.portalStatuses().has(_id)) { - this.setPortalStatus(_id, 'checking'); + if (fresh || !this.portalStatuses().has(key)) { + this.setPortalStatus(key, 'checking'); } let status: PortalStatus; try { @@ -172,23 +178,28 @@ export class SourceStatusService { if (signal?.aborted) { // A slow portal must not land after the list has closed; drop a // placeholder so the next opening checks again. - if (this.portalStatuses().get(_id) === 'checking') { - this.setPortalStatus(_id, null); + if (this.portalStatuses().get(key) === 'checking') { + this.setPortalStatus(key, null); } return; } - this.setPortalStatus(_id, status); + this.setPortalStatus(key, status); } - private setPortalStatus(id: string, status: PortalStatus | null): void { + private setPortalStatus(key: string, status: PortalStatus | null): void { this.portalStatuses.update((current) => { const next = new Map(current); if (status) { - next.set(id, status); + next.set(key, status); } else { - next.delete(id); + next.delete(key); } return next; }); } } + +/** A playlist's portal status key: its ID and its connection. */ +function portalKey(playlist: PlaylistMeta): string { + return `${playlist._id}\u0000${sourceHealthFingerprint(playlist)}`; +}