From 4112413ca5397bd91bbd075cc1c4d658478df3f5 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 14:34:05 +0200 Subject: [PATCH] fix(spa): #271 disable all family rows on click (family-global lock parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold-review nit fixes on PR #298: - useCollectionsScan.scan() now guards on the whole family being busy (active OR any pending key of that family), not just the exact key — a sibling source of a family with a scan in flight no longer fires a redundant (benign-409) POST. - LibrariesScreen ExternalCollectionsSection disables every row of a family that has a pending or active scan (derives pendingFamilies from pendingKeys), matching Blazor's instant all-rows-disabled behavior instead of waiting a poll RTT. - Rewrite the promote test to actually observe the optimistic-pending window via a deferred POST (was only asserting the promoted end state), and add a test proving a sibling-source click fires no second POST while the family is pending. Co-Authored-By: Claude Opus 4.8 (1M context) --- web/src/api/libraries.test.ts | 64 +++++++++++++++++++++++------ web/src/api/libraries.ts | 12 ++++-- web/src/screens/LibrariesScreen.tsx | 8 +++- 3 files changed, 66 insertions(+), 18 deletions(-) diff --git a/web/src/api/libraries.test.ts b/web/src/api/libraries.test.ts index fe23561be..9feed970b 100644 --- a/web/src/api/libraries.test.ts +++ b/web/src/api/libraries.test.ts @@ -1,4 +1,4 @@ -import { renderHook, waitFor } from '@testing-library/react'; +import { act, renderHook, waitFor } from '@testing-library/react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { getCollectionsScanStatus, @@ -24,31 +24,41 @@ function lastCall(fetchMock: ReturnType) { // Stateful collections backend mock: the scan-collections POST acquires the family-global lock // synchronously (as the real controller does before returning 202/409), so a subsequent status GET // reports that family active. `postStatus` overrides the POST response for error-path tests. -function mockCollectionsBackend(opts: { postStatus?: number; seededFamilies?: string[] } = {}) { +function mockCollectionsBackend(opts: { deferPost?: boolean; postStatus?: number; seededFamilies?: string[] } = {}) { const active = new Set(opts.seededFamilies ?? []); - const fetchMock = vi.spyOn(window, 'fetch').mockImplementation((input, init) => { + // When deferPost is set, the scan-collections POST does not resolve until releasePost() is called - + // this lets a test observe the optimistic-pending window before the POST completes. + let releasePost = () => {}; + const postGate = opts.deferPost ? new Promise((resolve) => (releasePost = resolve)) : Promise.resolve(); + + const fetchMock = vi.spyOn(window, 'fetch').mockImplementation(async (input, init) => { const url = String(input); const method = (init?.method ?? 'GET').toUpperCase(); if (url.includes('/api/media-sources/collections-scan-status')) { - return Promise.resolve(json([...active].map((family) => ({ family })))); + return json([...active].map((family) => ({ family }))); } const match = url.match(/\/api\/media-sources\/(\w+)\/\d+\/scan-collections/); if (match && method === 'POST') { + await postGate; const status = opts.postStatus ?? 202; if (status === 202 || status === 409) { // Both mean a collections scan is now running for this family -> the lock is held. active.add(match[1]); - return Promise.resolve(status === 202 ? noContent() : json({ detail: 'already scanning' }, 409)); + return status === 202 ? noContent() : json({ detail: 'already scanning' }, 409); } - return Promise.resolve(json({ detail: 'not found' }, status)); + return json({ detail: 'not found' }, status); } - return Promise.resolve(json({ detail: 'unexpected' }, 500)); + return json({ detail: 'unexpected' }, 500); }); - return { active, fetchMock }; + function scanPostCount(): number { + return fetchMock.mock.calls.filter((call) => String(call[0]).includes('scan-collections')).length; + } + + return { active, fetchMock, releasePost: () => releasePost(), scanPostCount }; } describe('libraries api client', () => { @@ -120,20 +130,50 @@ describe('useCollectionsScan', () => { expect(result.current.pendingKeys.size).toBe(0); }); - it('optimistically marks pending on scan, then promotes to active once the family is observed', async () => { - mockCollectionsBackend(); + it('bridges the click->observed-active gap with optimistic pending, then promotes', async () => { + const backend = mockCollectionsBackend({ deferPost: true }); const { result } = renderHook(() => useCollectionsScan()); await waitFor(() => expect(result.current.activeFamilies.size).toBe(0)); - await result.current.scan('jellyfin', 7, false); + let scanPromise: Promise = Promise.resolve(); + act(() => { + scanPromise = result.current.scan('jellyfin', 7, false); + }); - // The scan()'s 202 poll observes the (synchronously-locked) family active and reconciles. + // Optimistic pending is visible immediately, BEFORE the (deferred) POST resolves. + await waitFor(() => expect(result.current.pendingKeys.has('jellyfin:7')).toBe(true)); + expect(result.current.activeFamilies.has('jellyfin')).toBe(false); + + // Release the POST (lock now held) -> the follow-up poll promotes the family to active. + act(() => backend.releasePost()); + await scanPromise; await waitFor(() => expect(result.current.activeFamilies.has('jellyfin')).toBe(true)); expect(result.current.pendingKeys.has('jellyfin:7')).toBe(false); expect(result.current.error).toBeNull(); }); + it('does not fire a second scan for a sibling source while the family is already pending', async () => { + const backend = mockCollectionsBackend({ deferPost: true }); + const { result } = renderHook(() => useCollectionsScan()); + + await waitFor(() => expect(result.current.activeFamilies.size).toBe(0)); + + act(() => { + void result.current.scan('plex', 1, false); + }); + await waitFor(() => expect(result.current.pendingKeys.has('plex:1')).toBe(true)); + const postsAfterFirst = backend.scanPostCount(); + + // Sibling source of the same (family-global-locked) family - must be ignored, no second POST. + await act(async () => { + await result.current.scan('plex', 2, false); + }); + expect(backend.scanPostCount()).toBe(postsAfterFirst); + + backend.releasePost(); + }); + it('keeps the family disabled and surfaces no error on a 409 (already scanning)', async () => { mockCollectionsBackend({ postStatus: 409 }); const { result } = renderHook(() => useCollectionsScan()); diff --git a/web/src/api/libraries.ts b/web/src/api/libraries.ts index 2de95a261..6ca875e66 100644 --- a/web/src/api/libraries.ts +++ b/web/src/api/libraries.ts @@ -430,7 +430,7 @@ export function collectionsScanKey(source: CollectionsScanSource, sourceId: numb } // Recover the family from a `${family}:${sourceId}` key (the family segment has no colon). -function collectionsScanKeyFamily(key: string): CollectionsScanSource { +export function collectionsScanKeyFamily(key: string): CollectionsScanSource { return key.slice(0, key.indexOf(':')) as CollectionsScanSource; } @@ -522,9 +522,13 @@ export function useCollectionsScan(pollMs = 10000): CollectionsScanState { const scan = useCallback( (source: CollectionsScanSource, sourceId: number, deep: boolean): Promise => { const key = collectionsScanKey(source, sourceId); - if (pendingKeysRef.current.has(key) || activeFamiliesRef.current.has(source)) { - // Already pending, or the whole family is already scanning (family-global lock) - ignore the - // duplicate submission. Quick and deep scans share the same lock, so both gate on this. + const familyBusy = + activeFamiliesRef.current.has(source) || + [...pendingKeysRef.current].some((pendingKey) => collectionsScanKeyFamily(pendingKey) === source); + if (familyBusy) { + // The whole family is already scanning or has a pending scan - ignore the duplicate. The lock + // is family-global, so a sibling source of a busy family would only earn a benign 409; don't + // fire it. (Covers the same-key case too - a pending key implies its family is pending.) return Promise.resolve(); } diff --git a/web/src/screens/LibrariesScreen.tsx b/web/src/screens/LibrariesScreen.tsx index 5675ea39a..cba971637 100644 --- a/web/src/screens/LibrariesScreen.tsx +++ b/web/src/screens/LibrariesScreen.tsx @@ -28,6 +28,7 @@ import { } from '../components'; import { collectionsScanKey, + collectionsScanKeyFamily, useCollectionsScan, useLibrariesScreenQuery, type CollectionsScanSource, @@ -151,6 +152,10 @@ function ExternalCollectionsSection({ pendingKeys: Set; rows: Array<{ family: CollectionsScanSource; name: string; sourceId: number }>; }) { + // The collections lock is family-global, so a scan running (or just clicked) for any source of a + // family disables every row of that family - matching Blazor's all-rows-disabled behavior. + const pendingFamilies = new Set([...pendingKeys].map((key) => collectionsScanKeyFamily(key))); + return (
@@ -172,8 +177,7 @@ function ExternalCollectionsSection({
{rows.map((row) => { - const scanning = - activeFamilies.has(row.family) || pendingKeys.has(collectionsScanKey(row.family, row.sourceId)); + const scanning = activeFamilies.has(row.family) || pendingFamilies.has(row.family); return (