diff --git a/docs/decisions/README.md b/docs/decisions/README.md index fa4e3c95a..67b76b6d3 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -164,7 +164,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `spa.deco-templates-table` | The deco-templates editor also renders its day/deco assignment as a table, extending (not replacing) the templates-editor-table convention. | 2026-07-09 | [link](records/spa/deco-templates-table.md) | | `spa.download-sample-gate` | The SPA disables both Download Media Sample and Download Results while a troubleshooting session is starting/running (Blazor only gated Download Results). | 2026-07-09 | [link](records/spa/download-sample-gate.md) | | `spa.legacy-redirect-matcher` | `LegacyUiRedirects.TryGetRedirect` is a two-tier matcher — an exact `OrdinalIgnoreCase` `Map` (Tier 1) then an ordered segment-template pattern list (Tier 2, first-match-wins) — collision-free by construction, with a guard invariant that no rule may prefix-match `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`, or `/media/sources`. | 2026-07-11 | [link](records/spa/legacy-redirect-matcher.md) | -| `spa.library-pickers-resolve-by-search` | A picker over a media-library table (Episode/Song/Image/Movie/MusicVideo/TelevisionShow/TelevisionSeason/Artist/OtherVideo/RemoteStream) resolves its options by SEARCH — a debounced `SearchPicker` calling `searchLibraryPickerOptions`, which issues at most ONE `getLibraryBrowseItems` request per settled query, bounded to `LIBRARY_PICKER_RESULTS` (25) rows — CLAMPED inside the helper, not merely defaulted — and gated on `LIBRARY_PICKER_MIN_QUERY` (2) characters. It list-loads NOTHING on mount or on a type switch, so there is no truncation to surface and no truncation hint. The typed text is COMPILED (`titleContainsQuery` → `title:**`), never forwarded raw. The current selection renders from the OWNING RECORD, not from the result set (`selectedName` on a rerun collection / playlist item; a single by-id detail read — `getShow`/`getSeason`/`getArtist` — for a filler preset, which stores only the id), and an edit-load refresh HYDRATES ONLY USER-UNTOUCHED FIELDS — resolving `collectionType`+id+name as ONE domain value, never field-by-field — so a response that fails to NAME a selection can never CLEAR its id, a type change can never strand an id belonging to the previous type, and a late refresh can never overwrite a live edit. An id NEVER travels without its namespace: search results are cached against `(source, query)` and list-backed options carry the type they were loaded for, so no id from one type can be offered under another. A conflict RELOAD is a separate `replaceDraft` path that adopts the server record wholesale and renders the editor inert while pending, and a touched identity contradicting the server type is surfaced as a CONFLICT rather than reconciled — both because installing a save-authorizing ETag over a local edit the server contradicts is a cross-user lost update; an asynchronously-resolved name is keyed to the id it was resolved for and never overwrites a label naming a different id. The typeahead implements the full ARIA combobox keyboard contract, because it replaces a natively keyboard-operable ``. The other half of the superseded record is UNCHANGED: bounded-by-construction admin lists (collections, multi-collections, smart collections, playlists) still page to completeness via `loadAllPages` and still report `complete`/`hint: incomplete`. Server-side caps are not raised — this is a web-only change. | 2026-07-26 | [link](records/spa/library-pickers-resolve-by-search.md) | | `spa.logs-page-size-local` | The Logs page rows-per-page preference is stored in `window.localStorage` (`ctv-logs-page-size`), not a server `ConfigElement`. | 2026-07-11 | [link](records/spa/logs-page-size-local.md) | | `spa.playback-troubleshoot-poll` | The playback-troubleshooting screen reports FFmpeg completion by polling `GET /api/troubleshoot/playback/status` (~2s) rather than a server push channel. | 2026-07-09 | [link](records/spa/playback-troubleshoot-poll.md) | | `spa.playout-reset-button` | The SPA keeps a single Reset action (server picks the default build mode) and drops Blazor's separate "Schedule reset" button since its capability already exists via the playout's Edit-details flow. | 2026-07-09 | [link](records/spa/playout-reset-button.md) | diff --git a/docs/decisions/records/spa/library-pickers-resolve-by-search.md b/docs/decisions/records/spa/library-pickers-resolve-by-search.md index f96faea8b..dbbdf43f5 100644 --- a/docs/decisions/records/spa/library-pickers-resolve-by-search.md +++ b/docs/decisions/records/spa/library-pickers-resolve-by-search.md @@ -5,7 +5,7 @@ status: active since: '2026-07-26' supersedes: spa.list-completeness-vs-bounded-pickers@2026-07-26 superseded-by: none -rule: 'A picker over a media-library table (Episode/Song/Image/Movie/MusicVideo/TelevisionShow/TelevisionSeason/Artist/OtherVideo/RemoteStream) resolves its options by SEARCH — a debounced `SearchPicker` calling `searchLibraryPickerOptions`, which issues at most ONE `getLibraryBrowseItems` request per settled query, bounded to `LIBRARY_PICKER_RESULTS` (25) rows — CLAMPED inside the helper, not merely defaulted — and gated on `LIBRARY_PICKER_MIN_QUERY` (2) characters. It list-loads NOTHING on mount or on a type switch, so there is no truncation to surface and no truncation hint. The typed text is COMPILED (`titleContainsQuery` → `title:**`), never forwarded raw. The current selection renders from the OWNING RECORD, not from the result set (`selectedName` on a rerun collection / playlist item; a single by-id detail read — `getShow`/`getSeason`/`getArtist` — for a filler preset, which stores only the id), and an edit-load refresh HYDRATES ONLY USER-UNTOUCHED FIELDS — resolving `collectionType`+id+name as ONE domain value, never field-by-field — so a response that fails to NAME a selection can never CLEAR its id, a type change can never strand an id belonging to the previous type, and a late refresh can never overwrite a live edit. An id NEVER travels without its namespace: search results are cached against `(source, query)` and list-backed options carry the type they were loaded for, so no id from one type can be offered under another. A conflict RELOAD is a separate `replaceDraft` path that adopts the server record wholesale and renders the editor inert while pending, and a touched identity contradicting the server type is surfaced as a CONFLICT rather than reconciled — both because installing a save-authorizing ETag over a local edit the server contradicts is a cross-user lost update; an asynchronously-resolved name is keyed to the id it was resolved for and never overwrites a label naming a different id. The typeahead implements the full ARIA combobox keyboard contract, because it replaces a natively keyboard-operable ``. The other half of the superseded record is UNCHANGED: bounded-by-construction admin lists (collections, multi-collections, smart collections, playlists) still page to completeness via `loadAllPages` and still report `complete`/`hint: incomplete`. Server-side caps are not raised — this is a web-only change.' signals: 'library picker typeahead, SearchPicker, searchLibraryPickerOptions, titleContainsQuery, LIBRARY_PICKER_RESULTS, LIBRARY_PICKER_MIN_QUERY, LIBRARY_PICKER_LUCENE_SPECIALS, compile typed text not raw Lucene, Lucene && || escaping, picker truncation hint removed, loadAllPages Class A, LuceneSearchIndex.Search hitsLimit, useIsMountedRef, aria-activedescendant combobox keyboard, refresh must not clear an unnamed id, hydrate-untouched-fields not field-wise merge, initialize-once draft not hydrate-merge, no list-row seeding, cross-type id, id never travels without its namespace, results keyed on (source query), failed search not cached as empty, npm run typecheck not tsc --noEmit, stale result set not committable by keyboard OR pointer · paths: `web/src/api/libraryBrowse.ts`, `web/src/schedules/pickers.tsx`, `web/src/hooks.ts`, `web/src/screens/RerunCollectionsScreen.tsx`, `web/src/screens/PlaylistsScreen.tsx`, `web/src/screens/FillerPresetsScreen.tsx`, `web/src/api/paging.ts`, `docs/spa-conventions.md` §3b · issues: #651, #644, #578, #440' mechanics: '`docs/spa-conventions.md` §3b' --- @@ -112,9 +112,8 @@ found a *cross-user lost update*, the worst defect in the series: the conflict " exists to discard local edits — ran through the refresh path with a touched-set reset. Because the reloaded record reports `selectedId: null` under the #671 gap, the keep-ours fallback restored the user's **dirty** selection, the fresh ETag was installed, and the next Save silently overwrote the -collaborator's change with edits the user had explicitly asked to throw away. `replaceDraft` is now -a separate function and the mode travels with the load, so the two cannot be confused at the call -site. Every interleaving is tested by holding the detail response open, acting as the user, then +collaborator's change with edits the user had explicitly asked to throw away. `replaceDraft` was made a separate function with the mode carried on the load — machinery that +round 5 then deleted outright along with the rest of the reconciliation layer. Every interleaving is tested by holding the detail response open, acting as the user, then releasing it. Symmetrically, a name resolved asynchronously is **keyed to the id it was resolved for** and refuses diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 1af5538c0..8d035375c 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -237,8 +237,15 @@ the #644 follow-up got Class A right and Class B only half right): through the same mapper, so the list response is a strict SUBSET of the detail one (#671). A seed can only add a race, never information. Verify that claim for your endpoint before relying on it. - - **Write the concurrency ETag in the same callback that sets the draft**, so `draft != null` - implies an ETag and a PUT with no `If-Match` (a silent force-write) is unreachable. + - **Fail CLOSED on a missing concurrency token.** Writing the ETag in the same callback that + sets the draft is *not* the same as "a draft implies an ETag" — the response can simply omit + the header, and then the PUT carries no `If-Match` and silently force-writes. No token ⇒ no + editable draft (error + Retry/Back). Note this makes your test mocks load-bearing: a detail + mock that omits `ETag` was previously exercising the force-write path without saying so, so + give every single-record GET mock a real ETag and test the absent case explicitly. + - **Bound the load and always offer a way out.** A caller-supplied fetch with no abort signal can + hang forever; race it against a deadline, and give the loading view a Back control so a hung + request is never a dead end. - **Detect conflicts at save time** via the existing `If-Match` → 412 → Reload path. Reload sets the draft back to `null` and re-runs the same initialize-once load, so "replace" needs no separate policy and the form is unmounted while the replacement is in flight. @@ -289,9 +296,12 @@ appear once the widget is asynchronous: suppresses automatic retries until an explicit user action — reopen, focus, or edit — re-arms it. - **A caller-supplied promise needs a deadline, and a 2xx body is not a contract.** `client.ts` turns malformed JSON into `undefined` rather than rejecting, so `setResults(undefined)` throws on - the next render; treat a non-array as a failed attempt, not an empty answer. And a `search` prop - carries no abort signal, so race it against a timeout — otherwise a never-settling request leaves - the picker spinning with no way back. + the next render. Validate the **elements, not just the container**: `Array.isArray` accepts + `[null]`, which then throws on `option.id` during render, and an element with a wrong-typed `id` + commits an invalid value through `onSelect`. Treat any malformed payload as a failed attempt (so + it stays retryable), not as an empty answer. And a `search` prop carries no abort signal, so race + it against a timeout — otherwise a never-settling request leaves the picker spinning with no way + back. - **A stale result set must not be committable — by ANY modality.** Between a keystroke and its response, `results` still describe the *previous* query, so highlighting an option, retyping, and pressing Enter commits the old option while the box reads the new text. Drop the highlight on diff --git a/web/src/schedules/pickers.test.tsx b/web/src/schedules/pickers.test.tsx index f2e9f8310..79b503699 100644 --- a/web/src/schedules/pickers.test.tsx +++ b/web/src/schedules/pickers.test.tsx @@ -495,6 +495,41 @@ describe('SearchPicker', () => { expect(await screen.findByRole('option', { name: 'Alpha' })).toBeInTheDocument(); }); + it.each([ + { label: 'a null element', payload: [null] }, + { label: 'an element with no id', payload: [{ name: 'Nameless' }] }, + { label: 'an element with a string id', payload: [{ id: '7', name: 'Stringly' }] }, + { label: 'an element with no name', payload: [{ id: 7 }] }, + { label: 'an element with a non-string name', payload: [{ id: 7, name: 42 }] }, + { label: 'a NaN id', payload: [{ id: Number.NaN, name: 'Not a number' }] }, + { label: 'one bad element among good ones', payload: [{ id: 1, name: 'Alpha' }, null] } + ])('MEDIUM-3 (round 6): rejects $label rather than rendering or committing it', async ({ payload }) => { + // `Array.isArray` checks the CONTAINER, not the CONTENTS: `[null]` passes it, reaches + // `setResults`, and throws on `option.id` during render. A wrong-typed id would commit an + // invalid value through `onSelect`. + const onSelect = vi.fn(); + const search = vi + .fn<(query: string) => Promise>() + .mockResolvedValueOnce(payload as unknown as SearchPickerOption[]) + .mockResolvedValue(OPTIONS); + + renderPicker({ onSelect, search }); + const input = screen.getByLabelText('Movie search') as HTMLInputElement; + fireEvent.focus(input); + fireEvent.change(input, { target: { value: 'Alpha' } }); + + // Renders (does not throw) and offers nothing — a malformed list is not a partial answer. + expect(await screen.findByText(/No matches/)).toBeInTheDocument(); + expect(screen.queryAllByRole('option')).toHaveLength(0); + expect(onSelect).not.toHaveBeenCalled(); + + // ...and it is a FAILED attempt, so it stays retryable rather than being cached. + fireEvent.keyDown(input, { key: 'Escape' }); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + await waitFor(() => expect(search).toHaveBeenCalledTimes(2)); + expect(await screen.findByRole('option', { name: 'Alpha' })).toBeInTheDocument(); + }); + it('MEDIUM-3: a never-settling search stops loading instead of spinning forever', async () => { vi.useFakeTimers(); try { diff --git a/web/src/schedules/pickers.tsx b/web/src/schedules/pickers.tsx index f28e88781..be750b7db 100644 --- a/web/src/schedules/pickers.tsx +++ b/web/src/schedules/pickers.tsx @@ -40,6 +40,19 @@ export interface SearchPickerProps { // Upper bound on how long a picker will wait for a caller-supplied search promise. const SEARCH_TIMEOUT_MS = 10_000; +// `Array.isArray` checks the CONTAINER, not the CONTENTS: `[null]` passes it, reaches `setResults`, +// and throws on `option.id` during render; an element with a missing or wrong-typed `id`/`name` +// yields a broken option or commits an invalid value (#651 round 6). Nothing between the network +// and `onSelect` re-checks these, so validate every element before storing it. +function isSearchPickerOption(value: unknown): value is SearchPickerOption { + if (typeof value !== 'object' || value === null) { + return false; + } + + const candidate = value as { id?: unknown; name?: unknown }; + return typeof candidate.id === 'number' && Number.isFinite(candidate.id) && typeof candidate.name === 'string'; +} + export function SearchPicker({ label, selectedId, @@ -136,9 +149,10 @@ export function SearchPicker({ if (mountedRef.current && seqRef.current === seq) { // A malformed 2xx body resolves as `undefined` rather than rejecting (`client.ts` // `readJsonResponse` swallows a SyntaxError), and `setResults(undefined)` then throws - // on the next render reading `results.length`. Treat a non-array as a failed attempt: - // it is not an authoritative empty answer (#651 round 5 MEDIUM-3). - if (!Array.isArray(items)) { + // on the next render reading `results.length`. Treat anything that is not a well-formed + // list of options as a FAILED attempt — not an authoritative empty answer — so it is + // retryable rather than cached (#651 round 5 MEDIUM-3, round 6). + if (!Array.isArray(items) || !items.every(isSearchPickerOption)) { setResults([]); setResultsFor({ ok: false, query: trimmed, source }); setActiveIndex(-1); diff --git a/web/src/screens/RerunCollectionsScreen.test.tsx b/web/src/screens/RerunCollectionsScreen.test.tsx index bd3c44e97..2fbf3936b 100644 --- a/web/src/screens/RerunCollectionsScreen.test.tsx +++ b/web/src/screens/RerunCollectionsScreen.test.tsx @@ -1,7 +1,17 @@ -import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { act, cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { RerunCollectionsScreen } from './RerunCollectionsScreen'; +// A single-record GET response. It ALWAYS carries an ETag, because a real server does and because +// the editor now refuses to open without one (#651 round 6): mocks that omitted the header were +// silently exercising a force-write path that must not exist. Use this for every detail GET. +function detailResponse(body: unknown, etag = '"v1"'): Response { + return new Response(JSON.stringify(body), { + headers: { 'Content-Type': 'application/json', ETag: etag }, + status: 200 + }); +} + function jsonResponse(body: unknown, status = 200): Response { return new Response(JSON.stringify(body), { headers: { 'Content-Type': 'application/json' }, @@ -66,7 +76,7 @@ function mockApi(options: MockOptions = {}) { const rerunById = url.match(/^\/api\/v1\/rerun-collections\/(\d+)$/); if (rerunById && method === 'GET') { const found = (list as Array<{ id: number }>).find((r) => String(r.id) === rerunById[1]) ?? list[0]; - return Promise.resolve(jsonResponse(found)); + return Promise.resolve(detailResponse(found)); } if (url.startsWith('/api/v1/rerun-collections') && method === 'GET') { @@ -477,7 +487,7 @@ describe('RerunCollectionsScreen', () => { onRequest: (url, method) => { if (url === '/api/v1/rerun-collections/9' && method === 'GET') { detailCalls += 1; - return jsonResponse({ ...storedSelection, collectionType: 'RemoteStream', selectedId: null, selectedName: null }); + return detailResponse({ ...storedSelection, collectionType: 'RemoteStream', selectedId: null, selectedName: null }); } return null; } @@ -557,7 +567,7 @@ describe('RerunCollectionsScreen', () => { const url = input.toString(); if (url === '/api/v1/rerun-collections/9' && (init?.method ?? 'GET').toUpperCase() === 'GET') { return new Promise((resolve) => { - detailRelease.resolve = () => resolve(jsonResponse(storedSelection)); + detailRelease.resolve = () => resolve(detailResponse(storedSelection)); }); } return inner(input, init); @@ -582,7 +592,7 @@ describe('RerunCollectionsScreen', () => { list: [{ ...storedSelection, collectionType: 'Collection', name: 'Stale List Name', selectedId: 5, selectedName: 'Stale List Selection' }], onRequest: (url, method) => url === '/api/v1/rerun-collections/9' && method === 'GET' - ? jsonResponse({ ...storedSelection, collectionType: 'Collection', name: 'Server Name', selectedId: 5, selectedName: 'Favorites' }) + ? detailResponse({ ...storedSelection, collectionType: 'Collection', name: 'Server Name', selectedId: 5, selectedName: 'Favorites' }) : null }); @@ -642,37 +652,77 @@ describe('RerunCollectionsScreen', () => { }); }); - it('412 -> Reload discards local edits by unmounting the form, then re-initializes from the server', async () => { + it('412 -> Reload: the form is ABSENT while pending, and a dirty selection is discarded even when the server sends none', async () => { + // Replaces the deleted "conflict Reload with a null server selection" regression. Both halves + // matter and the previous version had neither: its second GET resolved immediately (so it never + // observed a pending Reload) and returned a non-null selection (so removing `setDraft(null)` + // could leave it green). Here the reload is HELD OPEN and returns `selectedId: null` — the #671 + // shape that round 3 showed could resurrect the user's dirty id over a collaborator's change. + const reloadRelease: { resolve: (() => void) | null } = { resolve: null }; let detailCalls = 0; + const fetchMock = mockApi({ - list: [storedSelection], - onRequest: (url, method) => { - if (url === '/api/v1/rerun-collections/9' && method === 'GET') { - detailCalls += 1; - return jsonResponse({ ...storedSelection, name: detailCalls === 1 ? 'First Load' : 'Collaborator Name' }); + browsePage: { page: [{ id: 99, mediaItemId: 99, mediaType: 'RemoteStream', title: 'New Stream' }], totalCount: 1 }, + list: [{ ...storedSelection, collectionType: 'RemoteStream' }], + onRequest: (url, method) => + url === '/api/v1/rerun-collections/9' && method === 'PUT' ? new Response(null, { status: 412 }) : null + }); + + const inner = fetchMock.getMockImplementation() as (i: RequestInfo | URL, r?: RequestInit) => Promise; + fetchMock.mockImplementation((input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url === '/api/v1/rerun-collections/9' && (init?.method ?? 'GET').toUpperCase() === 'GET') { + detailCalls += 1; + if (detailCalls === 1) { + return Promise.resolve( + detailResponse({ ...storedSelection, collectionType: 'RemoteStream', selectedId: 42, selectedName: 'Stored Stream' }) + ); } - return url === '/api/v1/rerun-collections/9' && method === 'PUT' ? new Response(null, { status: 412 }) : null; + return new Promise((resolve) => { + reloadRelease.resolve = () => + resolve( + detailResponse( + { ...storedSelection, collectionType: 'RemoteStream', selectedId: null, selectedName: null }, + '"v2"' + ) + ); + }); } + return inner(input, init); }); render(); fireEvent.click(await screen.findByText('Stored Rerun')); - const nameInput = (await screen.findByPlaceholderText('Rerun collection name')) as HTMLInputElement; - expect(nameInput.value).toBe('First Load'); + await screen.findByPlaceholderText('Rerun collection name'); + + // Dirty the selection, then hit the conflict and choose Reload. + fireEvent.click(await screen.findByRole('button', { name: 'Change' })); + const input = await screen.findByLabelText('Remote Stream search'); + fireEvent.focus(input); + fireEvent.change(input, { target: { value: 'New Stream' } }); + fireEvent.click(await screen.findByRole('option', { name: 'New Stream' })); + expect(screen.getByLabelText(/^Clear /)).toBeInTheDocument(); - fireEvent.change(nameInput, { target: { value: 'My Local Edit' } }); fireEvent.click(screen.getByRole('button', { name: 'Save rerun collection' })); - const dialog = await screen.findByRole('dialog'); fireEvent.click(within(dialog).getByRole('button', { name: 'Reload' })); - - // The form is gone while the replacement is in flight — there is no window in which an edit can - // be typed into a draft that is about to be discarded (round-4 MEDIUM-4, now structural). await waitFor(() => expect(detailCalls).toBe(2)); - const reloaded = (await screen.findByPlaceholderText('Rerun collection name')) as HTMLInputElement; - expect(reloaded.value).toBe('Collaborator Name'); - expect(reloaded.value).not.toBe('My Local Edit'); - expect(fetchMock.mock.calls.filter(([u, i]) => u === '/api/v1/rerun-collections/9' && (i?.method ?? '').toUpperCase() === 'PUT')).toHaveLength(1); + + // (1) While the reload is PENDING the form does not exist — nothing to edit, nothing to preserve. + expect(await screen.findByText('Loading rerun collection…')).toBeInTheDocument(); + expect(screen.queryByPlaceholderText('Rerun collection name')).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'Save rerun collection' })).not.toBeInTheDocument(); + + reloadRelease.resolve?.(); + await screen.findByPlaceholderText('Rerun collection name'); + + // (2) The server sent NO selection, and the user's dirty id 99 is gone rather than resurrected. + expect(screen.queryByText('New Stream')).not.toBeInTheDocument(); + expect(screen.queryByLabelText(/^Clear /)).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Save rerun collection' })).toBeDisabled(); + expect( + fetchMock.mock.calls.filter(([u, i]) => u === '/api/v1/rerun-collections/9' && (i?.method ?? '').toUpperCase() === 'PUT') + ).toHaveLength(1); }); it('a #671 unnamed selection disables Save with a visible reason — it never silently saves a guess', async () => { @@ -683,7 +733,7 @@ describe('RerunCollectionsScreen', () => { list: [{ ...storedSelection, collectionType: 'RemoteStream' }], onRequest: (url, method) => url === '/api/v1/rerun-collections/9' && method === 'GET' - ? jsonResponse({ ...storedSelection, collectionType: 'RemoteStream', selectedId: null, selectedName: null }) + ? detailResponse({ ...storedSelection, collectionType: 'RemoteStream', selectedId: null, selectedName: null }) : null }); @@ -694,4 +744,73 @@ describe('RerunCollectionsScreen', () => { expect(screen.getByText('A selection is required')).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Save rerun collection' })).toBeDisabled(); }); + + it('HIGH: a detail response with NO ETag yields an error, never an editable draft', async () => { + // "The draft is only created alongside the ETag" is not the same as "a draft implies an ETag": + // the header can simply be absent, and then the PUT carries no `If-Match` — a silent + // force-write (#651 round 6). Fail closed: no token, no editor. + const fetchMock = mockApi({ + list: [storedSelection], + onRequest: (url, method) => + url === '/api/v1/rerun-collections/9' && method === 'GET' + ? jsonResponse(storedSelection) // deliberately WITHOUT an ETag header + : null + }); + + render(); + fireEvent.click(await screen.findByText('Stored Rerun')); + + expect(await screen.findByRole('alert')).toBeInTheDocument(); + expect(screen.getByText(/without a version tag/i)).toBeInTheDocument(); + expect(screen.queryByPlaceholderText('Rerun collection name')).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'Save rerun collection' })).not.toBeInTheDocument(); + + // The decisive assertion: no unconditional write can be issued at all. + expect( + fetchMock.mock.calls.filter(([u, i]) => u === '/api/v1/rerun-collections/9' && (i?.method ?? '').toUpperCase() === 'PUT') + ).toHaveLength(0); + }); + + it('a never-settling detail GET times out into an error with a way back', async () => { + vi.useFakeTimers(); + try { + mockApi({ + list: [storedSelection], + onRequest: (url, method) => + url === '/api/v1/rerun-collections/9' && method === 'GET' + ? (undefined as unknown as Response) // fall through to the never-settling stub below + : null + }); + vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL) => { + const url = input.toString(); + if (url === '/api/v1/rerun-collections/9') { + return new Promise(() => {}); + } + if (url.startsWith('/api/v1/rerun-collections')) { + return Promise.resolve(jsonResponse({ page: [storedSelection], totalCount: 1 })); + } + return Promise.resolve(new Response(null, { status: 204 })); + }); + + render(); + await act(async () => { + await vi.advanceTimersByTimeAsync(10); + }); + fireEvent.click(screen.getByText('Stored Rerun')); + + // While waiting there is already a route out — a hung request is not a dead end. + await act(async () => { + await vi.advanceTimersByTimeAsync(10); + }); + expect(screen.getByRole('button', { name: 'All rerun collections' })).toBeInTheDocument(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(16_000); + }); + expect(screen.getByRole('alert')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Retry' })).toBeInTheDocument(); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/web/src/screens/RerunCollectionsScreen.tsx b/web/src/screens/RerunCollectionsScreen.tsx index e677854cc..3554bfc06 100644 --- a/web/src/screens/RerunCollectionsScreen.tsx +++ b/web/src/screens/RerunCollectionsScreen.tsx @@ -218,6 +218,9 @@ interface Draft { selectedName: string; } +// Upper bound on the detail read. A never-settling request must not strand the editor. +const LOAD_TIMEOUT_MS = 15_000; + function draftFromRerun(rerun: RerunCollection): Draft { return { collectionType: rerun.collectionType, @@ -304,13 +307,34 @@ function RerunCollectionEditor({ let active = true; - getRerunCollectionWithMeta(initial.id) + // A caller-supplied fetch can hang indefinitely; without a deadline the editor sits on a bare + // spinner with no way out (#651 round 6). The request is not cancelled — `active` already makes + // a late resolution inert — but the UI stops waiting and offers a route back. + Promise.race([ + getRerunCollectionWithMeta(initial.id), + new Promise((_resolve, reject) => + window.setTimeout(() => reject(new Error('Timed out loading rerun collection')), LOAD_TIMEOUT_MS) + ) + ]) .then((meta) => { - if (active) { - // ETag and draft are written together and only here, so `draft != null` implies an ETag. - etagRef.current = meta.etag; - setDraft(draftFromRerun(meta.data)); + if (!active) { + return; } + + // FAIL CLOSED on a missing ETag. "The draft is only created here, alongside the ETag" is not + // the same as "a draft implies an ETag": the response can simply omit the header, and then + // `updateRerunCollection` sends no `If-Match` at all — a silent force-write, which is the + // round-4 hole in a new costume (#651 round 6 HIGH). An editor with no concurrency token + // cannot save safely, so it must not exist. + if (meta.etag == null) { + setLoadError( + 'This rerun collection was served without a version tag, so it cannot be edited safely. Reload the page or try again.' + ); + return; + } + + etagRef.current = meta.etag; + setDraft(draftFromRerun(meta.data)); }) .catch((error: unknown) => { if (active) { @@ -374,7 +398,17 @@ function RerunCollectionEditor({
@@ -383,9 +417,17 @@ function RerunCollectionEditor({ if (!draft) { return ( -
- - Loading rerun collection… +
+
+ {/* A way out while loading: a hung request must never be a dead end (#651 round 6). */} + +
+
+ + Loading rerun collection… +
); }