fix(644): round-3 review — split truncated/incomplete picker hints, F2 out-of-list gaps, F3 abort warns, F4 aria wiring, F5 FillerPresetsScreen tests
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 14s
PR Gates / Docs update reminder (pull_request) Successful in 24s
PR Gates / decisions lifecycle (pull_request) Successful in 28s
Review verdict / Set review-verdict status (pull_request) Successful in 6s
PR Gates / Script tests (pytest) (pull_request) Successful in 33s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m17s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 9s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 20m19s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m24s
review-verdict/h10 Review-verdict: MERGEABLE @ daedf00
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 14s
PR Gates / Docs update reminder (pull_request) Successful in 24s
PR Gates / decisions lifecycle (pull_request) Successful in 28s
Review verdict / Set review-verdict status (pull_request) Successful in 6s
PR Gates / Script tests (pytest) (pull_request) Successful in 33s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m17s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 9s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 20m19s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m24s
review-verdict/h10 Review-verdict: MERGEABLE @ daedf00
Addresses the round-3 cold re-review's five low-severity findings on #644's client-side paging fix: - F1: `loadPickerOptions` (RerunCollectionsScreen, PlaylistsScreen) returned one `truncated: boolean` for two different conditions — a real Class B cap hit vs an unconverged Class A `loadAllPages` load — so an incomplete multi-collection load rendered the self-contradictory "Showing the first 47 of 47 — use search to narrow." Replaced with a `hint: 'incomplete' | 'none' | 'truncated'` discriminator and distinct copy per value; 'incomplete' matches the wording already used by the Class A list-load warn Badge. - F2: mirrored the out-of-list current-selection injection (RerunCollectionsScreen/PlaylistsScreen's `selectedInList` prepend) into FillerPresetsScreen and ScheduleItemInspector's rerun-collection picker, so an id outside the loaded page still renders as selected instead of misrepresenting the stored value as "(none)". - F3: gated the `console.warn` on an incomplete Class A load with `!signal?.aborted` in the `multi` branches (RerunCollectionsScreen, PlaylistsScreen) and SchedulesScreen.loadAllRerunCollections, so a superseded/aborted load (Retry, or a type switch mid-load) no longer logs a false warning. - F4: added `Select`'s `ariaDescribedBy` prop and wired the truncation/incomplete hint span to it via `useId()` in RerunCollectionsScreen and PlaylistsScreen, so screen readers announce the hint (FillerPresetsScreen already routed it through `Row help=`). - F5: added FillerPresetsScreen.test.tsx (previously untested) covering the Class B single-request guarantee, the truncation hint's totalCount>100/<=100 boundary, and the F2 injection; added the two assertions the re-review found missing anywhere in the suite — the Class A `incomplete` warn Badge actually rendering, and a screen-level seqRef stale-overwrite race — to RerunCollectionsScreen.test.tsx. Updates docs/spa-conventions.md §3b and the spa.list-completeness-vs-bounded-pickers decision record to describe the hint discriminator. Decisions-Edit: yes
This commit is contained in:
@@ -35,3 +35,19 @@ narrow.`) instead of either paging forever or truncating without saying so. A fu
|
||||
typeahead/search-driven picker over the media library is a materially larger feature (a `query`
|
||||
param already exists on `getLibraryBrowseItems` for it) and is deliberately out of scope here — a
|
||||
follow-up issue, not this fix.
|
||||
|
||||
**2026-07-26 addendum (round-3 review F1):** `loadPickerOptions`'s `multi` branch (a Class A
|
||||
source — `MultiCollection`) reused the same `truncated: boolean` field as the Class B media-library
|
||||
pickers, but the two conditions are not the same thing: Class B's `truncated` means "there are more
|
||||
rows than fit in one page — narrow via search," while a Class A picker's flag meant "the
|
||||
`loadAllPages` loop didn't converge" (`complete: false`) — a defensive/incomplete load, not a cap.
|
||||
Rendering both through the shared "Showing the first N of M — use search to narrow" copy produced a
|
||||
self-contradictory "Showing the first 47 of 47" on an incomplete Class A load, pointing at a search
|
||||
box that picker doesn't have. `RerunCollectionsScreen.tsx`/`PlaylistsScreen.tsx` now return a
|
||||
`hint: 'incomplete' | 'none' | 'truncated'` discriminator instead of a boolean, and render distinct
|
||||
copy per value — `'truncated'` keeps the existing search-narrowing text, `'incomplete'` renders
|
||||
"List may be incomplete — retry to reload" (matching the wording already used for the Class A
|
||||
list-load warn `Badge`). A picker's `console.warn` on an incomplete load — and the analogous one in
|
||||
`SchedulesScreen.loadAllRerunCollections` — is also gated on `!signal?.aborted`, so a superseded or
|
||||
user-aborted load (Retry, or a type switch mid-load) no longer logs a false "did not complete"
|
||||
warning.
|
||||
|
||||
@@ -169,6 +169,17 @@ below was itself the defect for one class of caller):
|
||||
for the pattern. A full typeahead/search-driven picker is a separate, larger feature — out of scope
|
||||
for this fix.
|
||||
|
||||
**A Class B truncation and a Class A `complete: false` are different conditions — don't collapse
|
||||
them into one boolean** (#644 follow-up round-3 review F1): a `loadPickerOptions` result that can
|
||||
come from either a Class A (`loadAllPages`) or Class B (single bounded page) source should carry a
|
||||
`hint: 'incomplete' | 'none' | 'truncated'` discriminator, not a `truncated: boolean` reused for
|
||||
both. `'truncated'` (Class B, an expected cap) keeps the "Showing the first N of M — use search to
|
||||
narrow" copy; `'incomplete'` (Class A, `loadAllPages`'s `complete: false`) renders different copy
|
||||
("List may be incomplete — retry to reload") — rendering both through the search-narrowing text
|
||||
produces a self-contradictory "Showing the first 47 of 47" when a Class A load doesn't converge.
|
||||
Also gate any `console.warn` on a Class A `complete: false` with `!signal?.aborted` — a superseded
|
||||
or user-aborted load returns `complete: false` too, and that's expected, not a defect.
|
||||
|
||||
**If a screen shows a bounded preview or has real paging UI** (a "load more" button, a page-size
|
||||
selector, a fixed-size typeahead result list), a `pageSize` at or below the cap is correct as-is —
|
||||
`loadAllPages` is only for "I need literally everything, and the list is small by construction"
|
||||
|
||||
@@ -187,6 +187,10 @@ export interface SelectProps {
|
||||
size?: ControlSize;
|
||||
fullWidth?: boolean;
|
||||
style?: CSSProperties;
|
||||
// Associates an out-of-band hint (e.g. a truncation message rendered as a sibling) with this
|
||||
// control for screen readers, matching how `error`/`help` text is otherwise co-located with a
|
||||
// field. Pass the id of the element carrying the hint text.
|
||||
ariaDescribedBy?: string;
|
||||
}
|
||||
|
||||
export function Select({
|
||||
@@ -197,7 +201,8 @@ export function Select({
|
||||
disabled = false,
|
||||
size = 'md',
|
||||
fullWidth = true,
|
||||
style
|
||||
style,
|
||||
ariaDescribedBy
|
||||
}: SelectProps) {
|
||||
const normalizedOptions = options.map((option) =>
|
||||
typeof option === 'string' ? { value: option, label: option } : option
|
||||
@@ -215,7 +220,13 @@ export function Select({
|
||||
)}
|
||||
style={style}
|
||||
>
|
||||
<select className="ctv-select" value={value} onChange={onChange} disabled={disabled}>
|
||||
<select
|
||||
aria-describedby={ariaDescribedBy}
|
||||
className="ctv-select"
|
||||
value={value}
|
||||
onChange={onChange}
|
||||
disabled={disabled}
|
||||
>
|
||||
{normalizedOptions.map((option) => (
|
||||
<option key={option.value} value={option.value}>
|
||||
{option.label}
|
||||
|
||||
@@ -368,6 +368,13 @@ function SourcePicker({
|
||||
}
|
||||
|
||||
if (type === 'RerunFirstRun' || type === 'RerunRerun') {
|
||||
// Out-of-list injection (round-3 review F2): the picker source is a Class A `loadAllPages`
|
||||
// loop (SchedulesScreen.loadAllRerunCollections) that can legitimately return an incomplete
|
||||
// page set, so the item's stored rerunCollectionId may not be among `pickers.rerunCollections`
|
||||
// — mirrors RerunCollectionsScreen's and PlaylistsScreen's `selectedInList` prepend.
|
||||
const rerunSelectedInList =
|
||||
item.rerunCollectionId != null && pickers.rerunCollections.some((c) => c.id === item.rerunCollectionId);
|
||||
|
||||
return (
|
||||
<Select
|
||||
label="Rerun collection"
|
||||
@@ -377,7 +384,13 @@ function SourcePicker({
|
||||
const name = pickers.rerunCollections.find((c) => c.id === id)?.name ?? null;
|
||||
onChange({ ...item, rerunCollectionId: id, rerunCollectionName: name });
|
||||
}}
|
||||
options={[{ value: '', label: '(none)' }, ...pickers.rerunCollections.map((c) => ({ value: `${c.id}`, label: c.name ?? `Rerun ${c.id}` }))]}
|
||||
options={[
|
||||
{ value: '', label: '(none)' },
|
||||
...(item.rerunCollectionId != null && !rerunSelectedInList
|
||||
? [{ value: `${item.rerunCollectionId}`, label: item.rerunCollectionName || `Rerun ${item.rerunCollectionId}` }]
|
||||
: []),
|
||||
...pickers.rerunCollections.map((c) => ({ value: `${c.id}`, label: c.name ?? `Rerun ${c.id}` }))
|
||||
]}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,159 @@
|
||||
import { cleanup, render, screen, waitFor, within } from '@testing-library/react';
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { FillerPresetsScreen } from './FillerPresetsScreen';
|
||||
|
||||
function jsonResponse(body: unknown, status = 200): Response {
|
||||
return new Response(JSON.stringify(body), {
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
status
|
||||
});
|
||||
}
|
||||
|
||||
const presetList = [{ fillerKind: 'PreRoll', id: 1, name: 'Bumper' }];
|
||||
|
||||
// Editing this preset: collectionType 'TelevisionShow', mediaItemId 9999 — deliberately NOT among
|
||||
// the library-browse page below, to exercise the F2 out-of-list injection. (FillerPresetsScreen's
|
||||
// COLLECTION_TYPES doesn't offer a raw 'Movie' entry — TelevisionShow/TelevisionSeason/Artist are
|
||||
// its media-item-backed types.)
|
||||
const editPreset = {
|
||||
allowWatermarks: false,
|
||||
collectionId: null,
|
||||
collectionType: 'TelevisionShow',
|
||||
count: 3,
|
||||
duration: null,
|
||||
expression: null,
|
||||
fillerKind: 'PreRoll',
|
||||
fillerMode: 'Count',
|
||||
id: 1,
|
||||
mediaItemId: 9999,
|
||||
multiCollectionId: null,
|
||||
name: 'Bumper',
|
||||
padToNearestMinute: null,
|
||||
playlistId: null,
|
||||
smartCollectionId: null,
|
||||
useChaptersAsMediaItems: false
|
||||
};
|
||||
|
||||
// One bounded page (cap 100) of browse items, none of which is id 9999. Every id-ish field is
|
||||
// populated (not just mediaItemId) so the fixture works regardless of which COLLECTION_TYPES
|
||||
// entry's `itemId` extractor is in play (Collection reads collectionId, Movie reads mediaItemId).
|
||||
function browsePage(count: number, totalCount: number) {
|
||||
return {
|
||||
page: Array.from({ length: count }, (_, i) => ({
|
||||
collectionId: i + 1,
|
||||
id: i + 1,
|
||||
mediaItemId: i + 1,
|
||||
mediaType: 'Movie' as const,
|
||||
multiCollectionId: i + 1,
|
||||
smartCollectionId: i + 1,
|
||||
title: `Movie ${i + 1}`
|
||||
})),
|
||||
totalCount
|
||||
};
|
||||
}
|
||||
|
||||
interface MockOptions {
|
||||
browseCount?: number;
|
||||
browseTotal?: number;
|
||||
onRequest?: (url: string, method: string, body: unknown) => Response | null;
|
||||
}
|
||||
|
||||
function mockApi(options: MockOptions = {}) {
|
||||
const browseCount = options.browseCount ?? 3;
|
||||
const browseTotal = options.browseTotal ?? 3;
|
||||
|
||||
return vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
const pathname = url.pathname;
|
||||
const method = (init?.method ?? 'GET').toUpperCase();
|
||||
const body = init?.body ? JSON.parse(String(init.body)) : undefined;
|
||||
|
||||
if (options.onRequest) {
|
||||
const override = options.onRequest(input.toString(), method, body);
|
||||
if (override) {
|
||||
return Promise.resolve(override);
|
||||
}
|
||||
}
|
||||
|
||||
if (pathname === '/api/v1/filler-presets' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse(presetList));
|
||||
}
|
||||
|
||||
if (pathname === '/api/v1/filler-presets/1' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse(editPreset));
|
||||
}
|
||||
|
||||
if (pathname === '/api/v1/library/browse' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse(browsePage(browseCount, browseTotal)));
|
||||
}
|
||||
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
}
|
||||
|
||||
describe('FillerPresetsScreen', () => {
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
vi.restoreAllMocks();
|
||||
window.history.pushState({}, '', '/app/filler-presets');
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
window.localStorage.clear();
|
||||
window.history.pushState({}, '', '/app/filler-presets');
|
||||
});
|
||||
|
||||
it('renders the filler preset list', async () => {
|
||||
mockApi();
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
expect(await screen.findByText('Bumper')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('Class B picker: the default Collection-type picker issues exactly ONE /library/browse request (#644 follow-up)', async () => {
|
||||
window.history.pushState({}, '', '/app/filler-presets/add');
|
||||
const fetchMock = mockApi();
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
// Default draft collectionType is 'Collection'; wait for its picker to finish loading.
|
||||
await screen.findByText('Movie 1');
|
||||
|
||||
const browseCalls = fetchMock.mock.calls.filter(
|
||||
([u]) => new URL(u.toString(), 'http://localhost').pathname === '/api/v1/library/browse'
|
||||
);
|
||||
expect(browseCalls).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('shows the truncation hint when totalCount exceeds the loaded page, and hides it when it does not', async () => {
|
||||
window.history.pushState({}, '', '/app/filler-presets/add');
|
||||
mockApi({ browseCount: 100, browseTotal: 250 });
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
expect(await screen.findByText('Showing the first 100 of 250 — use search to narrow.')).toBeInTheDocument();
|
||||
|
||||
cleanup();
|
||||
|
||||
window.history.pushState({}, '', '/app/filler-presets/add');
|
||||
mockApi({ browseCount: 50, browseTotal: 50 });
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
await screen.findByText('Movie 1');
|
||||
expect(screen.queryByText(/Showing the first/)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('F2: injects the out-of-list current selection so an id outside the loaded page still renders as selected', async () => {
|
||||
window.history.pushState({}, '', '/app/filler-presets/1');
|
||||
mockApi();
|
||||
render(<FillerPresetsScreen />);
|
||||
|
||||
await waitFor(() => expect(screen.getByDisplayValue('Bumper')).toBeInTheDocument());
|
||||
|
||||
// Select order: Kind, Mode, Pad to nearest minute, Collection type, then the activeConfig
|
||||
// ("Movie") picker last — it shows the out-of-list #9999 option, selected, never falling back
|
||||
// to "(none)" even though 9999 isn't in the 3-item loaded page.
|
||||
const comboboxes = await screen.findAllByRole('combobox');
|
||||
const pickerSelect = comboboxes[comboboxes.length - 1];
|
||||
expect((pickerSelect as HTMLSelectElement).value).toBe('9999');
|
||||
expect(within(pickerSelect).getByText('#9999')).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
@@ -577,8 +577,19 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: '
|
||||
};
|
||||
|
||||
const pickerValue = activeConfig ? String((draft[activeConfig.field] as null | number) ?? '') : '';
|
||||
const pickerSelectedId = activeConfig ? (draft[activeConfig.field] as null | number) : null;
|
||||
const pickerSelectedInList =
|
||||
pickerSelectedId != null &&
|
||||
pickerItems.some((item) => activeConfig?.itemId(item) === pickerSelectedId);
|
||||
// Out-of-list injection (round-3 review F2): the Class B picker loads only ONE bounded page
|
||||
// (LIBRARY_BROWSE_PAGE_CAP), so a preset whose stored id sits outside that page would otherwise
|
||||
// render as "(none)" while the draft still holds the id — mirrors RerunCollectionsScreen's and
|
||||
// PlaylistsScreen's `selectedInList` prepend.
|
||||
const pickerOptions = [
|
||||
{ label: '(none)', value: '' },
|
||||
...(pickerSelectedId != null && !pickerSelectedInList
|
||||
? [{ label: `#${pickerSelectedId}`, value: String(pickerSelectedId) }]
|
||||
: []),
|
||||
...pickerItems.flatMap((item) => {
|
||||
const id = activeConfig?.itemId(item);
|
||||
return id == null ? [] : [{ label: item.title ?? `#${id}`, value: String(id) }];
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import { useCallback, useEffect, useId, useRef, useState } from 'react';
|
||||
import {
|
||||
ArrowDown,
|
||||
ArrowLeft,
|
||||
@@ -53,14 +53,17 @@ interface PickerOption {
|
||||
}
|
||||
|
||||
// #644 follow-up: `browse` (media-library) picker sources are Class B — bounded to one page, with
|
||||
// `truncated`/`totalCount` telling the caller whether there's more than fits. `collection`/`multi`/
|
||||
// `smart` sources stay Class A (page to completeness) since they're inherently small,
|
||||
// admin-created lists; `truncated` there instead reflects `loadAllPages`'s `complete` flag, i.e. a
|
||||
// defensive/incomplete load rather than an expected cap.
|
||||
// `hint: 'truncated'`/`totalCount` telling the caller there's more than fits (a real, expected
|
||||
// cap — narrow via search). `collection`/`multi`/`smart` sources stay Class A (page to
|
||||
// completeness) since they're inherently small, admin-created lists; `hint: 'incomplete'` there
|
||||
// instead reflects `loadAllPages`'s `complete` flag, i.e. a defensive load that did not converge,
|
||||
// not a cap. These two are NOT the same condition and must render different copy (round-3 review
|
||||
// F1): "showing the first N of M" is arithmetically vacuous — and points at a search box that
|
||||
// doesn't exist for this picker — when N === M on an incomplete Class A load.
|
||||
interface PickerLoadResult {
|
||||
items: PickerOption[];
|
||||
totalCount: number | null;
|
||||
truncated: boolean;
|
||||
hint: 'incomplete' | 'none' | 'truncated';
|
||||
}
|
||||
|
||||
const LIBRARY_BROWSE_PAGE_CAP = 100;
|
||||
@@ -156,27 +159,29 @@ function orderOptionsWithCurrent(type: CollectionType, current: PlaybackOrder):
|
||||
function loadPickerOptions(type: CollectionType, signal?: AbortSignal): Promise<PickerLoadResult> {
|
||||
const config = configFor(type);
|
||||
if (!config) {
|
||||
return Promise.resolve({ items: [], totalCount: 0, truncated: false });
|
||||
return Promise.resolve({ hint: 'none', items: [], totalCount: 0 });
|
||||
}
|
||||
|
||||
switch (config.source) {
|
||||
case 'collection':
|
||||
return getCollections().then((list) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
return { items, totalCount: items.length, truncated: false };
|
||||
return { hint: 'none' as const, items, totalCount: items.length };
|
||||
});
|
||||
case 'multi':
|
||||
return loadAllPages(getMultiCollections, undefined, undefined, signal).then(({ complete, items: list }) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
if (!complete) {
|
||||
if (!complete && !signal?.aborted) {
|
||||
// #644 follow-up F3: a superseded/aborted load (a type switch mid-load) also returns
|
||||
// `complete: false` — that's expected, not a defect, so don't warn on it.
|
||||
console.warn('PlaylistsScreen: multi-collection picker load did not complete; some items may be missing');
|
||||
}
|
||||
return { items, totalCount: items.length, truncated: !complete };
|
||||
return { hint: complete ? ('none' as const) : ('incomplete' as const), items, totalCount: items.length };
|
||||
});
|
||||
case 'smart':
|
||||
return getSmartCollections().then((list) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
return { items, totalCount: items.length, truncated: false };
|
||||
return { hint: 'none' as const, items, totalCount: items.length };
|
||||
});
|
||||
default:
|
||||
// Class B (#644 follow-up): a media-library picker over the largest tables (Episode, Song,
|
||||
@@ -190,7 +195,7 @@ function loadPickerOptions(type: CollectionType, signal?: AbortSignal): Promise<
|
||||
const page = result.page ?? [];
|
||||
const items = page.map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` }));
|
||||
const totalCount = result.totalCount ?? items.length;
|
||||
return { items, totalCount, truncated: totalCount > items.length };
|
||||
return { hint: totalCount > items.length ? ('truncated' as const) : ('none' as const), items, totalCount };
|
||||
}
|
||||
);
|
||||
}
|
||||
@@ -410,8 +415,9 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
const [selectedKey, setSelectedKey] = useState<string | null>(null);
|
||||
const [pickerItems, setPickerItems] = useState<PickerOption[]>([]);
|
||||
const [pickerError, setPickerError] = useState<string | null>(null);
|
||||
const [pickerTruncated, setPickerTruncated] = useState(false);
|
||||
const [pickerHint, setPickerHint] = useState<PickerLoadResult['hint']>('none');
|
||||
const [pickerTotalCount, setPickerTotalCount] = useState<number | null>(null);
|
||||
const pickerHelpId = useId();
|
||||
const [saving, setSaving] = useState(false);
|
||||
const [saveError, setSaveError] = useState<string | null>(null);
|
||||
const [previewItems, setPreviewItems] = useState<PlaylistPreviewItem[] | null>(null);
|
||||
@@ -474,11 +480,11 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
|
||||
const controller = new AbortController();
|
||||
loadPickerOptions(selectedType, controller.signal)
|
||||
.then(({ items, totalCount, truncated }) => {
|
||||
.then(({ hint, items, totalCount }) => {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems(items);
|
||||
setPickerError(null);
|
||||
setPickerTruncated(truncated);
|
||||
setPickerHint(hint);
|
||||
setPickerTotalCount(totalCount);
|
||||
}
|
||||
})
|
||||
@@ -486,7 +492,7 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems([]);
|
||||
setPickerError(messageFromPlaylistError(error, 'Unable to load picker items'));
|
||||
setPickerTruncated(false);
|
||||
setPickerHint('none');
|
||||
setPickerTotalCount(null);
|
||||
}
|
||||
});
|
||||
@@ -774,21 +780,27 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
|
||||
<div style={{ marginTop: 12 }}>
|
||||
<Select
|
||||
ariaDescribedBy={pickerError || pickerHint !== 'none' ? pickerHelpId : undefined}
|
||||
label={activeConfig?.label ?? 'Selection'}
|
||||
onChange={(event) => setItemSelection(selectedItem.key, event.target.value)}
|
||||
options={pickerOptions}
|
||||
value={selectedItem.selectedId == null ? '' : String(selectedItem.selectedId)}
|
||||
/>
|
||||
{pickerError && (
|
||||
<span className="ctv-field-error" role="alert">
|
||||
<span className="ctv-field-error" id={pickerHelpId} role="alert">
|
||||
{pickerError}
|
||||
</span>
|
||||
)}
|
||||
{!pickerError && pickerTruncated && (
|
||||
<span className="ctv-field-help">
|
||||
{!pickerError && pickerHint === 'truncated' && (
|
||||
<span className="ctv-field-help" id={pickerHelpId}>
|
||||
Showing the first {pickerItems.length} of {pickerTotalCount} — use search to narrow.
|
||||
</span>
|
||||
)}
|
||||
{!pickerError && pickerHint === 'incomplete' && (
|
||||
<span className="ctv-field-help" id={pickerHelpId}>
|
||||
List may be incomplete — retry to reload.
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
|
||||
<div style={{ marginTop: 12 }}>
|
||||
|
||||
@@ -265,6 +265,100 @@ describe('RerunCollectionsScreen', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('renders the "List may be incomplete" badge when the rerun-collections load does not complete (round-3 review)', async () => {
|
||||
let callCount = 0;
|
||||
|
||||
vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
const method = (init?.method ?? 'GET').toUpperCase();
|
||||
|
||||
if (url.pathname === '/api/v1/rerun-collections' && method === 'GET') {
|
||||
callCount += 1;
|
||||
// First page reports totalCount 2 but only returns 1 item; the second page (pageNum=1)
|
||||
// then comes back EMPTY, which loadAllPages treats as a defensive break: `complete: false`.
|
||||
if (callCount === 1) {
|
||||
return Promise.resolve(jsonResponse({ page: [rerunCollections[0]], totalCount: 2 }));
|
||||
}
|
||||
return Promise.resolve(jsonResponse({ page: [], totalCount: 2 }));
|
||||
}
|
||||
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
|
||||
render(<RerunCollectionsScreen />);
|
||||
|
||||
expect(await screen.findByText('List may be incomplete — retry to reload')).toBeInTheDocument();
|
||||
expect(callCount).toBe(2);
|
||||
});
|
||||
|
||||
it('screen-level: an older overlapping refresh must NOT overwrite a newer one (seqRef guard, round-3 review)', async () => {
|
||||
const itemA = { ...rerunCollections[0], id: 101, name: 'Item A' };
|
||||
const itemB = { ...rerunCollections[0], id: 102, name: 'Item B' };
|
||||
const staleAfterA = { ...rerunCollections[0], id: 201, name: 'Stale After A' };
|
||||
const freshAfterB = { ...rerunCollections[0], id: 202, name: 'Fresh After B' };
|
||||
|
||||
let listCallCount = 0;
|
||||
const staleRelease: { resolve: (() => void) | null } = { resolve: null };
|
||||
|
||||
const fetchMock = vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = new URL(input.toString(), 'http://localhost');
|
||||
const method = (init?.method ?? 'GET').toUpperCase();
|
||||
|
||||
if (url.pathname === '/api/v1/rerun-collections' && method === 'GET') {
|
||||
listCallCount += 1;
|
||||
// Call 1: initial mount load. Call 2: refresh after deleting A — held pending (the OLDER
|
||||
// overlapping load). Call 3: refresh after deleting B — resolves immediately (the NEWER
|
||||
// load), before call 2 is ever released.
|
||||
if (listCallCount === 1) {
|
||||
return Promise.resolve(jsonResponse({ page: [itemA, itemB], totalCount: 2 }));
|
||||
}
|
||||
if (listCallCount === 2) {
|
||||
return new Promise<Response>((resolve) => {
|
||||
staleRelease.resolve = () => resolve(jsonResponse({ page: [staleAfterA], totalCount: 1 }));
|
||||
});
|
||||
}
|
||||
return Promise.resolve(jsonResponse({ page: [freshAfterB], totalCount: 1 }));
|
||||
}
|
||||
|
||||
if (/^\/api\/v1\/rerun-collections\/\d+$/.test(url.pathname) && method === 'DELETE') {
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
}
|
||||
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
|
||||
render(<RerunCollectionsScreen />);
|
||||
|
||||
expect(await screen.findByText('Item A')).toBeInTheDocument();
|
||||
expect(screen.getByText('Item B')).toBeInTheDocument();
|
||||
|
||||
// Delete A -> refresh() issues the OLDER overlapping load (call 2), held pending.
|
||||
const rowA = screen.getByText('Item A').closest('.ctv-settings-flush-row') as HTMLElement;
|
||||
fireEvent.click(within(rowA).getByRole('button', { name: 'Delete' }));
|
||||
fireEvent.click(within(await screen.findByRole('dialog')).getByRole('button', { name: 'Delete' }));
|
||||
|
||||
await waitFor(() => expect(listCallCount).toBe(2));
|
||||
// The pending call 2 hasn't resolved, so the list (and Item B) is still showing.
|
||||
expect(screen.getByText('Item B')).toBeInTheDocument();
|
||||
|
||||
// Delete B -> refresh() issues the NEWER load (call 3), which resolves right away.
|
||||
const rowB = screen.getByText('Item B').closest('.ctv-settings-flush-row') as HTMLElement;
|
||||
fireEvent.click(within(rowB).getByRole('button', { name: 'Delete' }));
|
||||
fireEvent.click(within(await screen.findByRole('dialog')).getByRole('button', { name: 'Delete' }));
|
||||
|
||||
expect(await screen.findByText('Fresh After B')).toBeInTheDocument();
|
||||
|
||||
// Now release the OLDER (call 2) load. Its seq no longer matches seqRef.current, so it must
|
||||
// NOT overwrite the already-rendered newer result.
|
||||
staleRelease.resolve?.();
|
||||
await waitFor(() => expect(fetchMock.mock.calls.length).toBeGreaterThan(0));
|
||||
// Let the resolved (but stale) promise's `.then` run.
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
|
||||
expect(screen.queryByText('Stale After A')).not.toBeInTheDocument();
|
||||
expect(screen.getByText('Fresh After B')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('confirms and DELETEs a rerun collection', async () => {
|
||||
const fetchMock = mockApi();
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import { useCallback, useEffect, useId, useRef, useState } from 'react';
|
||||
import { ArrowLeft, Check, Plus, Repeat, Trash2, TriangleAlert } from 'lucide-react';
|
||||
import { Badge, Button, Card, ConfirmDialog, IconButton, Input, Select, Spinner } from '../components';
|
||||
import type { SelectOption } from '../components';
|
||||
@@ -30,14 +30,17 @@ interface PickerOption {
|
||||
}
|
||||
|
||||
// #644 follow-up: `browse` (media-library) picker sources are Class B — bounded to one page, with
|
||||
// `truncated`/`totalCount` telling the caller whether there's more than fits. `collection`/`multi`/
|
||||
// `smart` sources stay Class A (page to completeness) since they're inherently small,
|
||||
// admin-created lists; `truncated` there instead reflects `loadAllPages`'s `complete` flag, i.e. a
|
||||
// defensive/incomplete load rather than an expected cap.
|
||||
// `hint: 'truncated'`/`totalCount` telling the caller there's more than fits (a real, expected
|
||||
// cap — narrow via search). `collection`/`multi`/`smart` sources stay Class A (page to
|
||||
// completeness) since they're inherently small, admin-created lists; `hint: 'incomplete'` there
|
||||
// instead reflects `loadAllPages`'s `complete` flag, i.e. a defensive load that did not converge,
|
||||
// not a cap. These two are NOT the same condition and must render different copy (round-3 review
|
||||
// F1): "showing the first N of M" is arithmetically vacuous — and points at a search box that
|
||||
// doesn't exist for this picker — when N === M on an incomplete Class A load.
|
||||
interface PickerLoadResult {
|
||||
items: PickerOption[];
|
||||
totalCount: number | null;
|
||||
truncated: boolean;
|
||||
hint: 'incomplete' | 'none' | 'truncated';
|
||||
}
|
||||
|
||||
const LIBRARY_BROWSE_PAGE_CAP = 100;
|
||||
@@ -109,27 +112,29 @@ function orderOptionsWithCurrent(type: RerunCollectionType, current: PlaybackOrd
|
||||
function loadPickerOptions(type: RerunCollectionType, signal?: AbortSignal): Promise<PickerLoadResult> {
|
||||
const config = COLLECTION_TYPES.find((entry) => entry.value === type);
|
||||
if (!config) {
|
||||
return Promise.resolve({ items: [], totalCount: 0, truncated: false });
|
||||
return Promise.resolve({ hint: 'none', items: [], totalCount: 0 });
|
||||
}
|
||||
|
||||
switch (config.source) {
|
||||
case 'collection':
|
||||
return getCollections().then((list) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
return { items, totalCount: items.length, truncated: false };
|
||||
return { hint: 'none' as const, items, totalCount: items.length };
|
||||
});
|
||||
case 'multi':
|
||||
return loadAllPages(getMultiCollections, undefined, undefined, signal).then(({ complete, items: list }) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
if (!complete) {
|
||||
if (!complete && !signal?.aborted) {
|
||||
// #644 follow-up F3: a superseded/aborted load (Retry, or a type switch mid-load) also
|
||||
// returns `complete: false` — that's expected, not a defect, so don't warn on it.
|
||||
console.warn('RerunCollectionsScreen: multi-collection picker load did not complete; some items may be missing');
|
||||
}
|
||||
return { items, totalCount: items.length, truncated: !complete };
|
||||
return { hint: complete ? ('none' as const) : ('incomplete' as const), items, totalCount: items.length };
|
||||
});
|
||||
case 'smart':
|
||||
return getSmartCollections().then((list) => {
|
||||
const items = list.map((entry) => ({ id: entry.id, name: entry.name ?? `#${entry.id}` }));
|
||||
return { items, totalCount: items.length, truncated: false };
|
||||
return { hint: 'none' as const, items, totalCount: items.length };
|
||||
});
|
||||
default:
|
||||
// Class B (#644 follow-up): a media-library picker over the largest tables (Episode, Song,
|
||||
@@ -143,7 +148,7 @@ function loadPickerOptions(type: RerunCollectionType, signal?: AbortSignal): Pro
|
||||
const page = result.page ?? [];
|
||||
const items = page.map((item) => ({ id: item.mediaItemId ?? item.id, name: item.title ?? `#${item.id}` }));
|
||||
const totalCount = result.totalCount ?? items.length;
|
||||
return { items, totalCount, truncated: totalCount > items.length };
|
||||
return { hint: totalCount > items.length ? ('truncated' as const) : ('none' as const), items, totalCount };
|
||||
}
|
||||
);
|
||||
}
|
||||
@@ -260,8 +265,9 @@ function RerunCollectionEditor({
|
||||
);
|
||||
const [pickerItems, setPickerItems] = useState<PickerOption[]>([]);
|
||||
const [pickerError, setPickerError] = useState<string | null>(null);
|
||||
const [pickerTruncated, setPickerTruncated] = useState(false);
|
||||
const [pickerHint, setPickerHint] = useState<PickerLoadResult['hint']>('none');
|
||||
const [pickerTotalCount, setPickerTotalCount] = useState<number | null>(null);
|
||||
const pickerHelpId = useId();
|
||||
const [saveError, setSaveError] = useState<string | null>(null);
|
||||
const [saving, setSaving] = useState(false);
|
||||
const [conflictOpen, setConflictOpen] = useState(false);
|
||||
@@ -308,11 +314,11 @@ function RerunCollectionEditor({
|
||||
const controller = new AbortController();
|
||||
|
||||
loadPickerOptions(collectionType, controller.signal)
|
||||
.then(({ items, totalCount, truncated }) => {
|
||||
.then(({ hint, items, totalCount }) => {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems(items);
|
||||
setPickerError(null);
|
||||
setPickerTruncated(truncated);
|
||||
setPickerHint(hint);
|
||||
setPickerTotalCount(totalCount);
|
||||
}
|
||||
})
|
||||
@@ -320,7 +326,7 @@ function RerunCollectionEditor({
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems([]);
|
||||
setPickerError(messageFromRerunCollectionError(error, 'Unable to load picker items'));
|
||||
setPickerTruncated(false);
|
||||
setPickerHint('none');
|
||||
setPickerTotalCount(null);
|
||||
}
|
||||
});
|
||||
@@ -450,21 +456,27 @@ function RerunCollectionEditor({
|
||||
|
||||
<div style={{ marginTop: 12 }}>
|
||||
<Select
|
||||
ariaDescribedBy={pickerError || pickerHint !== 'none' ? pickerHelpId : undefined}
|
||||
label={activeConfig?.label ?? 'Selection'}
|
||||
onChange={(event) => setSelected(event.target.value)}
|
||||
options={pickerOptions}
|
||||
value={draft.selectedId == null ? '' : String(draft.selectedId)}
|
||||
/>
|
||||
{pickerError && (
|
||||
<span className="ctv-field-error" role="alert">
|
||||
<span className="ctv-field-error" id={pickerHelpId} role="alert">
|
||||
{pickerError}
|
||||
</span>
|
||||
)}
|
||||
{!pickerError && pickerTruncated && (
|
||||
<span className="ctv-field-help">
|
||||
{!pickerError && pickerHint === 'truncated' && (
|
||||
<span className="ctv-field-help" id={pickerHelpId}>
|
||||
Showing the first {pickerItems.length} of {pickerTotalCount} — use search to narrow.
|
||||
</span>
|
||||
)}
|
||||
{!pickerError && pickerHint === 'incomplete' && (
|
||||
<span className="ctv-field-help" id={pickerHelpId}>
|
||||
List may be incomplete — retry to reload.
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
|
||||
<div style={{ marginTop: 12 }}>
|
||||
|
||||
@@ -54,9 +54,12 @@ const DIRTY_PROMPT = 'You have unsaved schedule changes. Discard them?';
|
||||
// all for the ordinary case of <=100 rerun collections — the same call site, one defect earlier.
|
||||
function loadAllRerunCollections(signal?: AbortSignal): Promise<RerunCollection[]> {
|
||||
return loadAllPages(getRerunCollections, undefined, undefined, signal).then(({ complete, items }) => {
|
||||
if (!complete) {
|
||||
if (!complete && !signal?.aborted) {
|
||||
// #644 follow-up F4: a partial result is otherwise indistinguishable from a complete one —
|
||||
// log it so a stalled/incomplete rerun-collections load doesn't silently render as whole.
|
||||
// A superseded/aborted load (retry, or a newer boot supersedes this one — see the abort
|
||||
// below) also returns `complete: false`; that's expected, not a defect, so don't warn on it
|
||||
// (round-3 review F3).
|
||||
console.warn('SchedulesScreen: rerun-collections picker load did not complete; some items may be missing');
|
||||
}
|
||||
return items;
|
||||
|
||||
Reference in New Issue
Block a user