diff --git a/design-system/design_handoff_auto_tune/AutoTune.jsx b/design-system/design_handoff_auto_tune/AutoTune.jsx index 1e120c825..eec1f35fe 100644 --- a/design-system/design_handoff_auto_tune/AutoTune.jsx +++ b/design-system/design_handoff_auto_tune/AutoTune.jsx @@ -816,7 +816,7 @@
toggle(r.name)} /> - {r.number} + {r.number} - {label && {label}} + {label && !hideLabel && {label}} ); } diff --git a/web/src/screens/AutoTuneScreen.test.tsx b/web/src/screens/AutoTuneScreen.test.tsx index d968ee61f..3e73eb23e 100644 --- a/web/src/screens/AutoTuneScreen.test.tsx +++ b/web/src/screens/AutoTuneScreen.test.tsx @@ -149,6 +149,51 @@ describe('AutoTuneScreen', () => { expect(screen.getByText('Exists')).toBeInTheDocument(); }); + it('#721: renders each proposal name once and lays the row out as a fixed-track grid', async () => { + mockAutoTuneApi(); + render(); + + fireEvent.click(screen.getByRole('button', { name: /Preview channels/ })); + await screen.findByText('TV Shows'); + + // The name used to render TWICE per row — once as the Checkbox's visible label, once in the + // detail column. The checkbox keeps it as its accessible name (asserted below), so this + // reddens on a re-added visible `label` without weakening the a11y guarantee. + expect(screen.getAllByText('The Office')).toHaveLength(1); + expect(screen.getAllByText('Friends')).toHaveLength(1); + + const box = screen.getByRole('checkbox', { name: 'The Office' }); + expect(box).toHaveAttribute('aria-label', 'The Office'); + + // The group header carried the same duplicate one line up: a visible "Select all " label + // immediately followed by the axis title itself. The accessible name survives, the text does not. + expect(screen.queryByText('Select all TV Shows')).not.toBeInTheDocument(); + expect(screen.getByRole('checkbox', { name: 'Select all TV Shows' })).toBeInTheDocument(); + + // Alignment is a property of the TRACK LIST, not of any rendered text: jsdom computes no + // layout, so the only thing that can redden on a revert to the old flex row is the declared + // grid template itself. Matched as a pattern rather than a literal: the invariant is that the + // NUMBER track is a fixed `ch` width — a content-sized track resolves per row (each row is + // its own grid container) and would not align — not the particular n, which may be retuned. + const row = box.closest('.ctv-checkbox')?.parentElement; + expect(row).not.toBeNull(); + expect(row).toHaveStyle({ display: 'grid' }); + expect(row?.style.gridTemplateColumns).toMatch(/^auto [1-9]\d*ch minmax\(0, 1fr\) auto$/); + + // Right-alignment is what makes numbers of differing digit-length agree on their last digit. + // Without it the grid track aligns the column's left edge and the digits still ragged-right. + const numberCell = row?.children[1] as HTMLElement | undefined; + expect(numberCell?.textContent).toBe('500'); + expect(numberCell).toHaveStyle({ textAlign: 'right' }); + // No fixed track bounds a channel number (`Channel.NumberValidator` allows an unbounded integer + // part), so an over-long value must clip rather than overflow into the checkbox beside it. + expect(numberCell).toHaveStyle({ overflow: 'hidden' }); + // `text-overflow: ellipsis` only applies to non-wrapping inline overflow, and the clipped value + // must stay recoverable — the ellipsis is the whole reason a fixed track is acceptable here. + expect(numberCell).toHaveStyle({ whiteSpace: 'nowrap' }); + expect(numberCell).toHaveAttribute('title', '500'); + }); + it('supports select-all / clear over the selectable proposals', async () => { mockAutoTuneApi(); render(); diff --git a/web/src/screens/AutoTuneScreen.tsx b/web/src/screens/AutoTuneScreen.tsx index a4373acf0..1b41e38fc 100644 --- a/web/src/screens/AutoTuneScreen.tsx +++ b/web/src/screens/AutoTuneScreen.tsx @@ -118,6 +118,40 @@ const eyebrowStyle: CSSProperties = { const monoStyle: CSSProperties = { fontFamily: 'var(--font-mono)', fontVariantNumeric: 'tabular-nums' }; +// Proposal rows are a grid so the channel-number column starts at the same x on every row (#721). +// Each row is its OWN grid container, so the number track has to be a FIXED width — a content-sized +// track (`auto`/`max-content`) is resolved per container and would go back to varying per row. +// +// No fixed width is a true upper bound on the value, and this track does not pretend to be one: +// `AutoTuneNumberAllocator.Allocate` emits `int.ToString()` from a `StartingNumber` that +// `PreviewAutoTuneChannelsHandler` only floors at 1, and a per-proposal override is free text +// checked against `Channel.NumberValidator` (`^[0-9]+(\.[0-9]{1,2})?$`) — unbounded integer part +// plus up to two decimals. So the track is sized for the range seen in practice and the cell CLIPS +// beyond it: an extreme value degrades to an ellipsis with the full value on hover, rather than +// overflowing a fixed track and overlapping the checkbox to its left. +// +// What actually stopped the column drifting with the name length is the checkbox track being +// constant — a label-less Checkbox renders one fixed-size control — not the number track's width. +const NUMBER_TRACK = '7ch'; +const proposalRowStyle: CSSProperties = { + display: 'grid', + gridTemplateColumns: `auto ${NUMBER_TRACK} minmax(0, 1fr) auto`, + alignItems: 'center', + columnGap: 12, + padding: '9px 14px' +}; + +// Right-aligned + tabular so 2-, 3- and 4-digit numbers agree on their last digit (#721). +const proposalNumberStyle: CSSProperties = { + ...monoStyle, + font: 'var(--text-sm) var(--font-mono)', + color: 'var(--text-secondary)', + textAlign: 'right', + overflow: 'hidden', + textOverflow: 'ellipsis', + whiteSpace: 'nowrap' +}; + // Selection identity — axis+value uniquely identifies a proposal. Keying on the display // name would couple two same-named proposals (e.g. a TV show titled "Comedy" and the // "Comedy" genre), flipping/creating both. An axis id never contains a space, so a single @@ -1019,6 +1053,7 @@ function PreviewStep({ checked={allOn} indeterminate={someOn && !allOn} disabled={groupSelectable.length === 0} + hideLabel label={`Select all ${axis.title}`} onChange={() => setAll(rows, !allOn)} /> @@ -1047,17 +1082,18 @@ function PreviewStep({ opacity: row.alreadyExists ? 0.55 : 1 }} > -
+
toggle(key)} /> - + {overrides[key]?.number?.trim() || row.number} -
+
- {edited && ( - - )} - - {row.itemCount} items - - {row.alreadyExists ? ( - Exists - ) : ( - - )} + {/* One grid cell, not three: the trailing controls are conditional, and a + conditional child would shift every track after it from row to row. */} +
+ {edited && ( + + )} + + {row.itemCount} items + + {row.alreadyExists ? ( + Exists + ) : ( + + )} +
); diff --git a/web/src/screens/CollectionsScreen.guards.test.tsx b/web/src/screens/CollectionsScreen.guards.test.tsx new file mode 100644 index 000000000..67a9e649d --- /dev/null +++ b/web/src/screens/CollectionsScreen.guards.test.tsx @@ -0,0 +1,310 @@ +import { act, cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { CollectionsScreen } from './CollectionsScreen'; + +// #740: AddItemsDialog's stale-response and is-mounted guards. +// +// These live in their own file rather than alongside the rest of CollectionsScreen's tests because +// both need a fetch mock whose /library/browse responses settle on demand — the defect only exists +// in the window between two overlapping searches — and one needs a module mock of `../hooks` that +// would otherwise record reads from every other component in the tree. + +// Every value AddItemsDialog read out of the shared is-mounted guard, in order. This is what makes +// the unmount test non-vacuous (the same reason spelled out in schedules/pickers.test.tsx): an +// unmounted tree renders nothing either way, so no DOM assertion can distinguish "the guard stopped +// the update" from "React discarded it". Recording the reads proves the component actually CONSULTS +// the guard, and specifically that it consults it once the late response lands, seeing `false`. +const mountedReads: boolean[] = []; + +vi.mock('../hooks', async (importOriginal) => { + const actual = await importOriginal(); + const { useRef } = await import('react'); + return { + ...actual, + useIsMountedRef: () => { + const inner = actual.useIsMountedRef(); + const proxyRef = useRef<{ readonly current: boolean } | null>(null); + proxyRef.current ??= { + get current() { + mountedReads.push(inner.current); + return inner.current; + } + }; + return proxyRef.current; + } + }; +}); + +vi.mock('../builder/rules/fieldCatalog', () => ({ + useSearchFields: () => ({ byGroup: [], fields: [], fieldTypes: {}, status: 'success' }) +})); + +function jsonResponse(body: unknown): Response { + return new Response(JSON.stringify(body), { + headers: { 'Content-Type': 'application/json' }, + status: 200 + }); +} + +const manualCollections = [ + { collectionType: 'Collection', id: 1, name: 'Favorites', state: 'Normal', useCustomPlaybackOrder: false } +]; + +const browseRows: Record = { + Artist: { id: 3, mediaType: 'Artist', title: 'Zapp Band' }, + Movie: { id: 1, mediaType: 'Movie', title: 'Zathura' }, + TelevisionShow: { id: 2, mediaType: 'TelevisionShow', title: 'Adventure Time' } +}; + +/** A `/library/browse` request parked until the test settles or fails it. */ +interface Parked { + fail: () => void; + mediaType: string; + settle: () => void; +} + +/** + * fetch mock whose add-items browse requests — and, when `parkAdd` is set, the add-items POST — + * PARK instead of resolving. Everything else answers immediately, so the screen and the dialog + * reach their normal state. + */ +function mockDeferredApi(options: { parkAdd?: boolean } = {}): Parked[] { + const parked: Parked[] = []; + + vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? 'GET').toUpperCase(); + + if (url === '/api/v1/collections' && method === 'GET') { + return Promise.resolve(jsonResponse(manualCollections)); + } + + if (url === '/api/v1/smart-collections' && method === 'GET') { + return Promise.resolve(jsonResponse([])); + } + + if (url.startsWith('/api/v1/library/browse')) { + const params = new URL(url, 'http://localhost').searchParams; + const query = params.get('query') ?? ''; + const mediaType = params.get('mediaType') ?? ''; + + // The manual-collection preview query is not an add-items search — never park it. + if (query.startsWith('collection:') || mediaType === '') { + return Promise.resolve(jsonResponse({ page: [], totalCount: 0 })); + } + + return new Promise((resolve, reject) => { + const row = browseRows[mediaType]; + parked.push({ + fail: () => reject(new Error(`browse failed for ${mediaType}`)), + mediaType, + settle: () => resolve(jsonResponse(row ? { page: [row], totalCount: 1 } : { page: [], totalCount: 0 })) + }); + }); + } + + if (/^\/api\/v1\/collections\/\d+\/items/.test(url) && method === 'GET') { + return Promise.resolve(jsonResponse({ page: [], totalCount: 0 })); + } + + if (options.parkAdd === true && /^\/api\/v1\/collections\/\d+\/items$/.test(url) && method === 'POST') { + return new Promise((resolve, reject) => { + parked.push({ + fail: () => reject(new Error('add failed')), + mediaType: 'ADD', + settle: () => resolve(new Response(null, { status: 204 })) + }); + }); + } + + return Promise.resolve(new Response(null, { status: 204 })); + }); + + return parked; +} + +async function openAddItemsDialog(): Promise { + render(); + await screen.findByText('Favorites'); + + fireEvent.click(screen.getByText('Favorites')); + await screen.findByRole('button', { name: 'Add items' }); + + fireEvent.click(screen.getByRole('button', { name: 'Add items' })); + return screen.getByRole('dialog'); +} + +function typeAndSearch(dialog: HTMLElement, text: string): void { + fireEvent.change(within(dialog).getByPlaceholderText('Search movies, shows, seasons, artists…'), { + target: { value: text } + }); + fireEvent.click(within(dialog).getByRole('button', { name: 'Search' })); +} + +/** + * Flush the settled fetch promises and the React updates they schedule. Deliberately NOT keyed on + * anything a guard produces — a barrier that waits on the guard's own side effect would make every + * test in this file redden when any one guard is removed, which is the opposite of pinning them + * separately. + */ +async function flush(): Promise { + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); +} + +beforeEach(() => { + mountedReads.length = 0; +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe('AddItemsDialog async guards (#740)', () => { + it('a superseded fan-out settling LAST does not repaint the newer kind-filtered results', async () => { + const parked = mockDeferredApi(); + const dialog = await openAddItemsDialog(); + + // Search A: the "All" fan-out over Movie / TelevisionShow / Artist. + typeAndSearch(dialog, 'za'); + await waitFor(() => expect(parked).toHaveLength(3)); + const fanOut = parked.splice(0, 3); + + // Search B, issued while A is still in flight: the Movies chip, one request. + fireEvent.click(within(dialog).getByRole('button', { name: 'Movies' })); + await waitFor(() => expect(parked).toHaveLength(1)); + const chip = parked[0]; + expect(chip.mediaType).toBe('Movie'); + + // B settles first and owns the list. + chip.settle(); + expect(await within(dialog).findByText('Zathura')).toBeInTheDocument(); + + // A settles second. Without the seq guard its 3 kinds overwrite B's results, leaving a list of + // shows and artists under an active "Movies" chip. + for (const request of fanOut) { + request.settle(); + } + await flush(); + + expect(within(dialog).getByText('Zathura')).toBeInTheDocument(); + expect(within(dialog).queryByText('Adventure Time')).not.toBeInTheDocument(); + expect(within(dialog).queryByText('Zapp Band')).not.toBeInTheDocument(); + }); + + it('a SUPERSEDED search settling first does not clear the spinner the newer one still owes', async () => { + const parked = mockDeferredApi(); + const dialog = await openAddItemsDialog(); + + // Search A (fan-out), then search B (Movies chip) while A is still in flight. + typeAndSearch(dialog, 'za'); + await waitFor(() => expect(parked).toHaveLength(3)); + const fanOut = parked.splice(0, 3); + + fireEvent.click(within(dialog).getByRole('button', { name: 'Movies' })); + await waitFor(() => expect(parked).toHaveLength(1)); + const chip = parked[0]; + + // A — now superseded — settles FIRST. `Promise.all` resolves, so its `finally` runs while B is + // still outstanding. Without the guard on `setSearching(false)` the spinner clears here and the + // dialog claims to be idle with a request in flight. + for (const request of fanOut) { + request.settle(); + } + await flush(); + + // `loading` is what disables the Search button — there is no other disabled condition on it. + const searchButton = within(dialog).getByRole('button', { name: 'Search' }); + expect(searchButton).toBeDisabled(); + + chip.settle(); + await waitFor(() => expect(searchButton).toBeEnabled()); + expect(within(dialog).getByText('Zathura')).toBeInTheDocument(); + }); + + it('a SUPERSEDED search that FAILS does not wipe the newer results with an error', async () => { + const parked = mockDeferredApi(); + const dialog = await openAddItemsDialog(); + + typeAndSearch(dialog, 'za'); + await waitFor(() => expect(parked).toHaveLength(3)); + const fanOut = parked.splice(0, 3); + + fireEvent.click(within(dialog).getByRole('button', { name: 'Movies' })); + await waitFor(() => expect(parked).toHaveLength(1)); + + // B succeeds and paints. + parked[0].settle(); + expect(await within(dialog).findByText('Zathura')).toBeInTheDocument(); + + // A — superseded — now REJECTS. Its catch clears results/totalMatches/submittedQuery and raises + // an error banner. Unguarded, a failure belonging to a search the user has already moved on + // from destroys the results they are currently looking at. + fanOut[0].fail(); + fanOut[1].settle(); + fanOut[2].settle(); + await flush(); + + expect(within(dialog).getByText('Zathura')).toBeInTheDocument(); + // Matched by ROLE, not by the fallback message text: `messageFromCollectionError` returns + // `error.message` for an `Error` throw and only reaches its fallback string for a non-Error, + // so asserting the fallback text here would be null-vs-null and pass with the guard removed. + expect(within(dialog).queryByRole('alert')).not.toBeInTheDocument(); + }); + + it('a search resolving AFTER the dialog unmounts consults the is-mounted guard and is dropped', async () => { + const parked = mockDeferredApi(); + const dialog = await openAddItemsDialog(); + + typeAndSearch(dialog, 'za'); + await waitFor(() => expect(parked).toHaveLength(3)); + + // Cancel unmounts the dialog for real — the caller renders it under `key={add-${pickerOpen}}`. + fireEvent.click(within(dialog).getByRole('button', { name: 'Cancel' })); + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + + // Only reads that happen from here on can prove the LATE response consulted the guard. + mountedReads.length = 0; + for (const request of parked) { + request.settle(); + } + + await waitFor(() => expect(mountedReads.length).toBeGreaterThan(0)); + expect(mountedReads).toContain(false); + expect(mountedReads.every((read) => read === false)).toBe(true); + }); + + it('a FAILED add resolving after an Escape-close consults the is-mounted guard', async () => { + const parked = mockDeferredApi({ parkAdd: true }); + const dialog = await openAddItemsDialog(); + + typeAndSearch(dialog, 'za'); + await waitFor(() => expect(parked).toHaveLength(3)); + for (const request of parked.splice(0, 3)) { + request.settle(); + } + + // Select a result, then submit. + fireEvent.click(await within(dialog).findByRole('button', { name: /Zathura/ })); + fireEvent.click(within(dialog).getByRole('button', { name: /Add 1 item/ })); + await waitFor(() => expect(parked).toHaveLength(1)); + + // The Add button is disabled while adding, but Escape is not gated on `adding` — this is the + // unmount-while-in-flight path a disabled submit button does NOT close. + fireEvent.keyDown(document, { key: 'Escape' }); + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + + mountedReads.length = 0; + parked[0].fail(); + + // `submit` has exactly TWO post-await writes to this component's own state — `setError` in the + // catch and `setAdding` in the finally — and the invariant is that BOTH consult the guard. + // Counting them is what keeps the two from masking each other: asserting only that SOME read + // saw `false` stays green with either one removed, because the survivor still reads the ref. + await waitFor(() => expect(mountedReads).toHaveLength(2)); + expect(mountedReads.every((read) => read === false)).toBe(true); + }); +}); diff --git a/web/src/screens/CollectionsScreen.tsx b/web/src/screens/CollectionsScreen.tsx index d73b3f7d1..def0eb4c0 100644 --- a/web/src/screens/CollectionsScreen.tsx +++ b/web/src/screens/CollectionsScreen.tsx @@ -48,6 +48,7 @@ import { type MediaCollection, type SmartCollection } from '../api'; +import { useIsMountedRef } from '../hooks'; import { TYPE_LABEL } from '../media/mediaKinds'; import { SmartCollectionDialog } from '../builder/SmartCollectionDialog'; @@ -245,6 +246,8 @@ function AddItemsDialog({ const [error, setError] = useState(null); const [adding, setAdding] = useState(false); const [kindFilter, setKindFilter] = useState('all'); + const seqRef = useRef(0); + const mountedRef = useIsMountedRef(); // `searchLibraryBrowseItems` owns the min-query gate, the pageSize clamp, and the // raw-Lucene-vs-compiled-query choice (§3b: "the bound belongs to the helper, not the caller"). @@ -252,8 +255,21 @@ function AddItemsDialog({ // the two masked each other (deleting either one left the whole suite green, so the boundary // test pinned nothing) — so there is now exactly one gate, in the helper, and every caller // (blank form submit, kind-chip click) routes through it unconditionally. + // + // #740: `runSearch` is reachable twice concurrently — a form submit fans out over + // DEFAULT_SEARCH_KINDS while a kind-chip click issues a single-kind search — so the two guards + // §3b mandates for asynchronous pickers both apply. `seq` drops a SUPERSEDED response (the + // 3-kind batch settling after the 1-kind chip would otherwise repaint the list with all kinds + // while the active chip reads Movies), and `mountedRef` drops a response that lands after the + // dialog is unmounted (it genuinely is — `key={`add-${pickerOpen}`}` on the caller). Neither + // subsumes the other: a late response still matches the latest seq if no newer search started. + // + // `searching` is written under the same guard, which is what stops the spinner lying: without + // it, whichever search settles FIRST clears the flag while the other is still in flight. const runSearch = async (filter: MediaKindFilter = kindFilter) => { const trimmed = query.trim(); + const seq = ++seqRef.current; + const isCurrent = () => mountedRef.current && seqRef.current === seq; setSearching(true); setError(null); @@ -261,6 +277,9 @@ function AddItemsDialog({ try { const kinds = filter === 'all' ? DEFAULT_SEARCH_KINDS : [filter]; const perKind = await Promise.all(kinds.map((mediaType) => searchLibraryBrowseItems(mediaType, trimmed))); + if (!isCurrent()) { + return; + } const merged = perKind .flatMap((result) => result.items) .filter((item) => ADDABLE_TYPES.has(item.mediaType)) @@ -277,6 +296,9 @@ function AddItemsDialog({ // searched" rather than name the sub-gate text. setSubmittedQuery(trimmed.length < LIBRARY_PICKER_MIN_QUERY ? '' : trimmed); } catch (searchError) { + if (!isCurrent()) { + return; + } setError(messageFromCollectionError(searchError, 'Unable to search library')); // A failed search must not leave a stale results/totalMatches pair rendering a confident // "Showing N of M matches" hint beside the error banner (#685 review). @@ -284,7 +306,10 @@ function AddItemsDialog({ setTotalMatches(0); setSubmittedQuery(''); } finally { - setSearching(false); + // A superseded search must not clear the spinner a NEWER one is still owed. + if (isCurrent()) { + setSearching(false); + } } }; @@ -322,12 +347,25 @@ function AddItemsDialog({ try { await addItemsToCollection(collection.id, toAddItemsRequest([...selected.values()])); + // NOT guarded: both belong to the still-mounted PARENT. `onAdded` reloads the collection's + // items, and dropping it because this dialog went away would leave the list stale after a + // write that succeeded. onAdded(); onClose(); } catch (addError) { - setError(messageFromCollectionError(addError, 'Unable to add items')); + // Guarded: these two are this component's own state. The dialog is unmount-reachable while a + // request is in flight even though the Add button is disabled — Escape, a backdrop click and + // the header close button all reach `onClose` without consulting `adding` + // (`components/overlay.tsx`), and the caller remounts on `key={`add-${pickerOpen}`}`. This is + // the is-mounted guard's own trigger, unmount-while-in-flight, and it needs no `seq`: the + // disabled Add button means there is never a second submission to be superseded by. + if (mountedRef.current) { + setError(messageFromCollectionError(addError, 'Unable to add items')); + } } finally { - setAdding(false); + if (mountedRef.current) { + setAdding(false); + } } };