fix(651): review round 4 — an id never travels without its namespace
Round 4's through-line: rounds 2-3 made HYDRATION treat {collectionType, selectedId,
selectedName} as one value, but the fix was applied to the structure that was named rather
than to every structure that carries an id. Three more instances of the same class, plus
two independent findings.
HIGH-1 + HIGH-2 (one change, per the structural directive): every result, option and
cached result set now carries its SOURCE, and identity is compared as (type, id).
- `SearchPicker` takes a REQUIRED `source` prop and caches results against (source, query),
not query text. Keying on text let the round-3 re-query guard SUPPRESS the new source's
request and leave the previous namespace's hit clickable under the new label — a
Collection id stored as a SmartCollection id. Results from another source are now hidden
outright rather than dimmed: they are not stale, they are wrong. Required rather than
defaulted, because a default would silently opt every caller out.
- `pickerFor` tags list-backed options with the type they were loaded for, on both
RerunCollectionsScreen and PlaylistsScreen, so the previous type's rows stop being
selectable the instant the active type changes rather than lingering through the
replacement load.
HIGH-3: a touched identity contradicting the server's type is a CONFLICT. Pinning the
user's edit was right; adopting the response's newest ETag alongside it authorized a Save
that silently overwrote the collaborator's type change with no 412. The conflict dialog is
raised and the stale ETag kept, so even a forced Save 412s. With round 3's Reload defect
this is the second cross-user lost update, so it is recorded as a category: never install a
save-authorizing ETag over a local edit the server contradicts.
MEDIUM-4: the editor is inert while a Reload is pending. The dialog closes immediately, so
an edit typed before the replacement landed was silently erased along with the touched set
that protected it.
MEDIUM-5: cached search provenance records `ok`, so a transient 500 is retried instead of
being cached as an authoritative "No matches" that reopening can never clear.
Also: `npx tsc --noEmit` typechecks NOTHING in web/ — the root tsconfig is solution-style
("files": [] + references), so it resolves to zero inputs and exits 0. The real gate, and
what CI runs, is `npm run typecheck` (tsc -b). Verified by planting a deliberate type error:
--noEmit stayed green, -b caught it. Running the real gate surfaced four genuine errors in
tests written earlier this branch (a missing required prop and three `never has no call
signatures` from closure-assigned mock variables), fixed with the repo's existing
holder-object pattern rather than casts. Recorded in spa-conventions so the next session
doesn't repeat it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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:*<escaped>*`), 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. A conflict RELOAD is a separate `replaceDraft` path that adopts the server record wholesale, because routing it through the refresh policy 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 `<select>`. 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.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:*<escaped>*`), 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 `<select>`. 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) |
|
||||
|
||||
@@ -5,8 +5,8 @@ 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:*<escaped>*`), 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. A conflict RELOAD is a separate `replaceDraft` path that adopts the server record wholesale, because routing it through the refresh policy 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 `<select>`. 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, refresh vs replace policy, cross-type id, 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'
|
||||
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:*<escaped>*`), 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 `<select>`. 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, refresh vs replace policy, 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'
|
||||
---
|
||||
|
||||
@@ -110,6 +110,32 @@ collection-family types keep the bounded single-page load and the truncation hin
|
||||
`loadAllPages` paths in the other two screens are untouched. The `api.search-allitems-paging`
|
||||
precedent holds: the client bounds itself, the server cap is not raised.
|
||||
|
||||
**The generalisation that took four rounds: an id never travels without its namespace.** Rounds 2
|
||||
and 3 made *hydration* treat `{collectionType, selectedId, selectedName}` as one value. Round 4 found
|
||||
the same defect in three more places, because the fix had been applied to the one structure that was
|
||||
named rather than to every structure that carries an id. A typeahead cached its results against the
|
||||
query TEXT, so switching the search source with the same text made the re-query guard *suppress* the
|
||||
new request and leave the previous namespace's hit clickable under the new label. List-backed
|
||||
`<select>` options were normalised to `{id, name}`, dropping the type, so on a slow connection the
|
||||
previous type's rows stayed selectable while the replacement loaded — on both screens. The rule that
|
||||
covers all of them: **every result, option and cached result set carries its source, and identity is
|
||||
compared as `(type, id)`.** `SearchPicker` now takes a required `source` prop (required, not
|
||||
defaulted — a default would silently opt every caller out), and `pickerFor` tags list-backed options
|
||||
with the type that produced them.
|
||||
|
||||
**Two cross-user lost updates make a category, not two incidents.** Round 3's was conflict-Reload
|
||||
running through the refresh policy. Round 4's was subtler: a touched identity pinned against a
|
||||
server-side type change is *correct*, but adopting the response's newest ETag alongside it authorized
|
||||
a Save that silently overwrote the collaborator's change with no 412. The category is **never install
|
||||
a save-authorizing ETag over a local edit the server contradicts** — such a collision is a conflict to
|
||||
surface, not a state to reconcile. Relatedly, the Reload path now renders the editor inert while the
|
||||
replacement is in flight, since the dialog closes immediately and an edit typed in that window was
|
||||
silently erased.
|
||||
|
||||
**Cache provenance must distinguish failure from emptiness.** The round-3 re-query guard cached a
|
||||
failed search as an authoritative empty result, so a transient 500 became a permanent "No matches"
|
||||
that no amount of reopening could retry. The cached entry now records `ok`.
|
||||
|
||||
**Replacing a native control means owing its keyboard behaviour.** A `<select>` is fully
|
||||
keyboard-operable, so an input-plus-listbox that only responds to Tab and click is a regression
|
||||
introduced by this change rather than a pre-existing gap. `SearchPicker` implements the ARIA
|
||||
@@ -141,5 +167,5 @@ being done on its own branch.
|
||||
distinct findings — the search bound, the compile rule, selection preservation, the
|
||||
clear-what-you-cannot-name prohibition, the async-name keying, and the keyboard contract — most of
|
||||
which came from review rounds, and each of which names a specific way the obvious implementation is
|
||||
wrong (three times over for the hydration rule, whose first two fixes were each themselves the
|
||||
defect — the recurring error being to patch the named instance instead of covering its class). Summarising any of them back out would lose the counter-example that makes it actionable.)*
|
||||
wrong. The recurring error across four review rounds was always the same: patching the named
|
||||
instance instead of covering its class — which is why the record states the rules as classes. Summarising any of them back out would lose the counter-example that makes it actionable.)*
|
||||
|
||||
+33
-1
@@ -209,6 +209,19 @@ the #644 follow-up got Class A right and Class B only half right):
|
||||
**#671** — the client guard stays even once that lands.) Degrading the label to `#id` is
|
||||
acceptable; dropping the id strands the user on a record they cannot re-save, because Save is
|
||||
gated on it. Test this per affected type, not on the one type that happens to work.
|
||||
- **An id NEVER travels without its namespace — in results, in options, in cached result sets.**
|
||||
A media/collection id only means anything inside the type that produced it, so any structure
|
||||
holding ids must hold the type too, and identity is compared as `(type, id)`. Three places this
|
||||
bites, all the same bug:
|
||||
1. A typeahead's cached results must be keyed on `(source, query)`, not the query text. Same
|
||||
query, different source ⇒ the results are not *stale*, they are *wrong*: hide them and
|
||||
re-query. Keying on text alone lets a re-query guard **suppress** the new source's request
|
||||
and leave the old namespace's hit clickable under the new label.
|
||||
2. List-backed `<select>` options must carry the type they were loaded for and be dropped the
|
||||
moment the active type differs — otherwise the previous type's rows stay selectable during
|
||||
the replacement load on a slow connection.
|
||||
3. A local edit whose type contradicts the server's is a **conflict**, not something to
|
||||
reconcile (below).
|
||||
- **Hydrate the whole domain value; never merge field-by-field.** The obvious fix — coalescing
|
||||
each field independently (`refreshed.selectedId ?? current.selectedId`,
|
||||
`refreshed.selectedName || current.selectedName`) — is *worse than the bug it fixes*, and this
|
||||
@@ -233,7 +246,15 @@ the #644 follow-up got Class A right and Class B only half right):
|
||||
is a **cross-user lost update**: the keep-ours-when-the-id-is-missing fallback restores the
|
||||
user's dirty selection, the fresh ETag is installed, and the next Save silently overwrites
|
||||
the collaborator's change. A shared path with a clearing side effect is not enough — the
|
||||
mode must be explicit and carried with the load.
|
||||
mode must be explicit and carried with the load. A pending replace must also make the editor
|
||||
**inert**: the dialog closes immediately, so an edit typed while the replacement is in flight
|
||||
is silently erased when it lands.
|
||||
4. **Never install a save-authorizing ETag over a local edit the server contradicts.** This is
|
||||
the general category behind two separate lost updates on this branch. If the user has touched
|
||||
the identity and the refresh reports a *different type*, that is a conflict — surface it and
|
||||
keep the stale ETag, so even a forced Save gets a 412 instead of clobbering. Pinning the
|
||||
user's edit (correct) while adopting the response's newest ETag (incorrect) converts a
|
||||
detectable conflict into an undetected overwrite.
|
||||
|
||||
Test this by **interleaving**: issue the refresh, have the user act while it is in flight, then
|
||||
release it. A test that doesn't interleave proves nothing about a race.
|
||||
@@ -271,6 +292,11 @@ implements the ARIA combobox pattern — `role="combobox"` + `aria-expanded`/`ar
|
||||
matches in tests: count `<select>` elements when that is what you mean. Two failure modes that only
|
||||
appear once the widget is asynchronous:
|
||||
|
||||
- **Freshness is `(source, query)`, and cached failures are not answers.** A `SearchPicker`-style
|
||||
cache must record which source produced the results and whether the attempt *succeeded*. Caching a
|
||||
failed search as an authoritative empty result turns a transient 500 into a permanent "No matches"
|
||||
that reopening, blurring and refocusing can never retry — the re-query guard sees a cache hit
|
||||
forever. Distinguish success from failure in the cached provenance.
|
||||
- **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
|
||||
@@ -286,6 +312,12 @@ appear once the widget is asynchronous:
|
||||
has since moved, so Enter silently does nothing. Reopening also places the cursor (ARIA APG)
|
||||
rather than swallowing the keypress.
|
||||
|
||||
**The web typecheck gate is `npm run typecheck`, never `npx tsc --noEmit`.** `web/tsconfig.json` is
|
||||
solution-style (`"files": []` + `references`), so a bare `tsc --noEmit` resolves to zero input files
|
||||
and exits 0 **without checking anything** — a green that means "I looked at nothing". CI runs
|
||||
`npm run typecheck` (`tsc -b --pretty false`), which builds the referenced projects and includes the
|
||||
test files. Verified by planting a deliberate type error: `--noEmit` stayed green, `-b` caught it.
|
||||
|
||||
**Testing an is-mounted guard: React 19 does not warn on a setState-after-unmount, and an unmounted
|
||||
tree renders nothing either way** — so no DOM assertion can distinguish "the guard stopped it" from
|
||||
"React discarded it". Prove the *mechanism* (a `useIsMountedRef` unit test, with a StrictMode
|
||||
|
||||
@@ -435,6 +435,11 @@ function SourcePicker({
|
||||
selectedId={selectedId ?? null}
|
||||
selectedName={selectedName ?? null}
|
||||
search={search}
|
||||
// The picker is NOT remounted when the item's collection type changes, so results must be
|
||||
// bound to the type that produced them — otherwise a Collection hit stays clickable under a
|
||||
// SmartCollection label and stores a Collection id in the SmartCollection field (#651 round 4
|
||||
// HIGH-1). `type` names both the search function and the id namespace, so it IS the source.
|
||||
source={type}
|
||||
onSelect={(id, name) => setSelection(id, name)}
|
||||
onClear={() => setSelection(null, null)}
|
||||
/>
|
||||
|
||||
@@ -47,6 +47,7 @@ function renderPicker(overrides: Partial<Parameters<typeof SearchPicker>[0]> = {
|
||||
search: vi.fn<(query: string) => Promise<SearchPickerOption[]>>().mockResolvedValue([]),
|
||||
selectedId: null,
|
||||
selectedName: null,
|
||||
source: 'Movie',
|
||||
...overrides
|
||||
};
|
||||
return { props, ...render(<SearchPicker {...props} />) };
|
||||
@@ -54,11 +55,14 @@ function renderPicker(overrides: Partial<Parameters<typeof SearchPicker>[0]> = {
|
||||
|
||||
describe('SearchPicker', () => {
|
||||
it('#578: a search resolving AFTER unmount consults the is-mounted guard and is dropped', async () => {
|
||||
let release: ((items: SearchPickerOption[]) => void) | null = null;
|
||||
// Holder object, not a bare `let`: TS control-flow analysis cannot see that the Promise
|
||||
// executor already ran, so a plain variable narrows to `null` and `release?.()` fails to
|
||||
// typecheck as `never`. (The repo already uses this shape in RerunCollectionsScreen.test.)
|
||||
const release: { resolve: ((items: SearchPickerOption[]) => void) | null } = { resolve: null };
|
||||
const search = vi.fn(
|
||||
() =>
|
||||
new Promise<SearchPickerOption[]>((resolve) => {
|
||||
release = resolve;
|
||||
release.resolve = resolve;
|
||||
})
|
||||
);
|
||||
|
||||
@@ -73,7 +77,7 @@ describe('SearchPicker', () => {
|
||||
expect(mountedReads).not.toContain(false);
|
||||
|
||||
unmount();
|
||||
release?.([{ id: 1, name: 'Alpha Movie' }]);
|
||||
release.resolve?.([{ id: 1, name: 'Alpha Movie' }]);
|
||||
// Let the resolved promise's .then run against an unmounted tree.
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
|
||||
@@ -252,11 +256,11 @@ describe('SearchPicker', () => {
|
||||
|
||||
it('BLOCKER 2: Enter cannot commit a result from the PREVIOUS query', async () => {
|
||||
const onSelect = vi.fn();
|
||||
let release: ((items: SearchPickerOption[]) => void) | null = null;
|
||||
const release: { resolve: ((items: SearchPickerOption[]) => void) | null } = { resolve: null };
|
||||
const search = vi
|
||||
.fn<(query: string) => Promise<SearchPickerOption[]>>()
|
||||
.mockResolvedValueOnce(OPTIONS)
|
||||
.mockImplementation(() => new Promise((resolve) => { release = resolve; }));
|
||||
.mockImplementation(() => new Promise((resolve) => { release.resolve = resolve; }));
|
||||
|
||||
renderPicker({ onSelect, search });
|
||||
const input = screen.getByLabelText('Movie search') as HTMLInputElement;
|
||||
@@ -279,7 +283,7 @@ describe('SearchPicker', () => {
|
||||
|
||||
// Once the response for "Be" lands, the keyboard is live again.
|
||||
await waitFor(() => expect(search).toHaveBeenCalledTimes(2));
|
||||
release?.([{ id: 7, name: 'Beta Two' }]);
|
||||
release.resolve?.([{ id: 7, name: 'Beta Two' }]);
|
||||
await waitFor(() => expect(screen.getByRole('option', { name: 'Beta Two' })).toBeInTheDocument());
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
fireEvent.keyDown(input, { key: 'Enter' });
|
||||
@@ -336,11 +340,11 @@ describe('SearchPicker', () => {
|
||||
|
||||
it('round 3 HIGH: a stale result cannot be committed by POINTER either', async () => {
|
||||
const onSelect = vi.fn();
|
||||
let release: ((items: SearchPickerOption[]) => void) | null = null;
|
||||
const release: { resolve: ((items: SearchPickerOption[]) => void) | null } = { resolve: null };
|
||||
const search = vi
|
||||
.fn<(query: string) => Promise<SearchPickerOption[]>>()
|
||||
.mockResolvedValueOnce(OPTIONS)
|
||||
.mockImplementation(() => new Promise((resolve) => { release = resolve; }));
|
||||
.mockImplementation(() => new Promise((resolve) => { release.resolve = resolve; }));
|
||||
|
||||
renderPicker({ onSelect, search });
|
||||
const input = screen.getByLabelText('Movie search') as HTMLInputElement;
|
||||
@@ -361,11 +365,103 @@ describe('SearchPicker', () => {
|
||||
|
||||
// Once "Be" resolves, pointer commits work again.
|
||||
await waitFor(() => expect(search).toHaveBeenCalledTimes(2));
|
||||
release?.([{ id: 7, name: 'Beta Two' }]);
|
||||
release.resolve?.([{ id: 7, name: 'Beta Two' }]);
|
||||
const fresh = await screen.findByRole('option', { name: 'Beta Two' });
|
||||
expect(fresh).toHaveAttribute('aria-disabled', 'false');
|
||||
fireEvent.click(fresh);
|
||||
expect(onSelect).toHaveBeenCalledWith(7, 'Beta Two');
|
||||
});
|
||||
});
|
||||
|
||||
// ---- #651 review round 4: results carry their SOURCE, not just their query text ----
|
||||
describe('result provenance', () => {
|
||||
const COLLECTION_HITS: SearchPickerOption[] = [{ id: 5, name: 'News Collection' }];
|
||||
const SMART_HITS: SearchPickerOption[] = [{ id: 8, name: 'News Smart' }];
|
||||
const OPTIONS: SearchPickerOption[] = [
|
||||
{ id: 1, name: 'Alpha' },
|
||||
{ id: 2, name: 'Beta' }
|
||||
];
|
||||
|
||||
it('HIGH-1: changing the SOURCE re-queries and retires the old namespace\'s results', async () => {
|
||||
const onSelect = vi.fn();
|
||||
const search = vi
|
||||
.fn<(query: string) => Promise<SearchPickerOption[]>>()
|
||||
.mockResolvedValueOnce(COLLECTION_HITS)
|
||||
.mockResolvedValue(SMART_HITS);
|
||||
|
||||
const { rerender } = renderPicker({ onSelect, search, source: 'Collection' });
|
||||
|
||||
const input = screen.getByLabelText('Movie search') as HTMLInputElement;
|
||||
fireEvent.focus(input);
|
||||
fireEvent.change(input, { target: { value: 'news' } });
|
||||
expect(await screen.findByRole('option', { name: 'News Collection' })).toBeInTheDocument();
|
||||
expect(search).toHaveBeenCalledTimes(1);
|
||||
|
||||
// Same component instance (no `key`), same query text — only the id namespace changed. The
|
||||
// round-3 re-query guard keyed on query alone, which SUPPRESSED this request and left the
|
||||
// Collection hit clickable under the new source.
|
||||
rerender(
|
||||
<SearchPicker
|
||||
label="Movie"
|
||||
onClear={vi.fn()}
|
||||
onSelect={onSelect}
|
||||
search={search}
|
||||
selectedId={null}
|
||||
selectedName={null}
|
||||
source="SmartCollection"
|
||||
/>
|
||||
);
|
||||
|
||||
// The other namespace's result is gone immediately — not merely dimmed. It is not stale, it
|
||||
// is wrong: a Collection id offered under a SmartCollection label.
|
||||
expect(screen.queryByRole('option', { name: 'News Collection' })).not.toBeInTheDocument();
|
||||
|
||||
await waitFor(() => expect(search).toHaveBeenCalledTimes(2));
|
||||
expect(await screen.findByRole('option', { name: 'News Smart' })).toBeInTheDocument();
|
||||
fireEvent.click(screen.getByRole('option', { name: 'News Smart' }));
|
||||
expect(onSelect).toHaveBeenCalledWith(8, 'News Smart');
|
||||
expect(onSelect).not.toHaveBeenCalledWith(5, 'News Collection');
|
||||
});
|
||||
|
||||
it('MEDIUM-5: a FAILED search is retried on reopen, not cached as "no matches" forever', async () => {
|
||||
const search = vi
|
||||
.fn<(query: string) => Promise<SearchPickerOption[]>>()
|
||||
.mockRejectedValueOnce(new Error('boom'))
|
||||
.mockResolvedValue(OPTIONS);
|
||||
|
||||
renderPicker({ search });
|
||||
const input = screen.getByLabelText('Movie search') as HTMLInputElement;
|
||||
fireEvent.focus(input);
|
||||
fireEvent.change(input, { target: { value: 'Alien' } });
|
||||
|
||||
// The transient failure renders as an empty result set...
|
||||
expect(await screen.findByText(/No matches/)).toBeInTheDocument();
|
||||
expect(search).toHaveBeenCalledTimes(1);
|
||||
|
||||
// ...but must NOT be cached as an authoritative one: reopening the same query has to retry.
|
||||
fireEvent.keyDown(input, { key: 'Escape' });
|
||||
await waitFor(() => expect(screen.queryByText(/No matches/)).not.toBeInTheDocument());
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
|
||||
await waitFor(() => expect(search).toHaveBeenCalledTimes(2));
|
||||
expect(await screen.findByRole('option', { name: 'Alpha' })).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('a SUCCESSFUL empty result is still cached — the retry is for failures only', async () => {
|
||||
const search = vi.fn<(query: string) => Promise<SearchPickerOption[]>>().mockResolvedValue([]);
|
||||
|
||||
renderPicker({ search });
|
||||
const input = screen.getByLabelText('Movie search') as HTMLInputElement;
|
||||
fireEvent.focus(input);
|
||||
fireEvent.change(input, { target: { value: 'Nope' } });
|
||||
expect(await screen.findByText(/No matches/)).toBeInTheDocument();
|
||||
|
||||
fireEvent.keyDown(input, { key: 'Escape' });
|
||||
fireEvent.keyDown(input, { key: 'ArrowDown' });
|
||||
await new Promise((resolve) => setTimeout(resolve, 400));
|
||||
|
||||
// "There genuinely are no matches" is an answer; re-asking would be the round-3 defect.
|
||||
expect(search).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -20,6 +20,11 @@ export interface SearchPickerProps {
|
||||
selectedId: null | number;
|
||||
selectedName: null | string;
|
||||
search: (query: string) => Promise<SearchPickerOption[]>;
|
||||
// Identifies WHAT `search` queries — the id NAMESPACE its results live in. Results are cached and
|
||||
// validated against `(source, query)`, never the query alone: an id is only meaningful inside its
|
||||
// own namespace, so a source change invalidates a result set exactly as a query change does.
|
||||
// Required, deliberately: a defaulted source would silently opt every caller out of the check.
|
||||
source: string;
|
||||
onSelect: (id: number, name: string) => void;
|
||||
onClear: () => void;
|
||||
ariaDescribedBy?: string;
|
||||
@@ -37,6 +42,7 @@ export function SearchPicker({
|
||||
selectedId,
|
||||
selectedName,
|
||||
search,
|
||||
source,
|
||||
onSelect,
|
||||
onClear,
|
||||
ariaDescribedBy,
|
||||
@@ -48,9 +54,15 @@ export function SearchPicker({
|
||||
const [query, setQuery] = useState('');
|
||||
const [open, setOpen] = useState(false);
|
||||
const [results, setResults] = useState<SearchPickerOption[]>([]);
|
||||
// The query `results` was produced for, so an empty result set can be told apart from "hasn't
|
||||
// searched yet" — without it the no-matches copy flashes between a keystroke and its debounce.
|
||||
const [resultsQuery, setResultsQuery] = useState<null | string>(null);
|
||||
// The PROVENANCE of `results`: which source and query produced them, and whether that attempt
|
||||
// succeeded. All three matter.
|
||||
// - source: results from another namespace are not stale, they are wrong (#651 round 4 HIGH-1);
|
||||
// - query: distinguishes an empty result set from "hasn't searched yet", so the no-matches
|
||||
// copy doesn't flash between a keystroke and its debounce;
|
||||
// - ok: a FAILED search must not be cached as a successful empty result, or the re-query
|
||||
// guard below turns a transient 500 into a permanent "No matches" that reopening,
|
||||
// blurring and refocusing can never retry (#651 round 4 MEDIUM-5).
|
||||
const [resultsFor, setResultsFor] = useState<null | { ok: boolean; query: string; source: string }>(null);
|
||||
const [loading, setLoading] = useState(false);
|
||||
// -1 = no option highlighted (the ARIA combobox "virtual cursor" is parked).
|
||||
const [activeIndex, setActiveIndex] = useState(-1);
|
||||
@@ -67,10 +79,11 @@ export function SearchPicker({
|
||||
}
|
||||
const trimmed = query.trim();
|
||||
|
||||
// Already holding results for exactly this query — e.g. reopening after Escape. Re-running the
|
||||
// search would issue a duplicate request whose response resets the highlight the user has since
|
||||
// moved, leaving Enter doing nothing (#651 review round 3, MEDIUM).
|
||||
if (resultsQuery === trimmed) {
|
||||
// Already holding SUCCESSFUL results for exactly this source and query — e.g. reopening after
|
||||
// Escape. Re-running the search would issue a duplicate request whose response resets the
|
||||
// highlight the user has since moved, leaving Enter doing nothing (#651 round 3, MEDIUM). A
|
||||
// different source, or a cached failure, must still re-query.
|
||||
if (resultsFor !== null && resultsFor.ok && resultsFor.source === source && resultsFor.query === trimmed) {
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -80,7 +93,7 @@ export function SearchPicker({
|
||||
if (tooShort) {
|
||||
if (mountedRef.current && seqRef.current === seq) {
|
||||
setResults([]);
|
||||
setResultsQuery(null);
|
||||
setResultsFor(null);
|
||||
setActiveIndex(-1);
|
||||
setLoading(false);
|
||||
}
|
||||
@@ -91,7 +104,7 @@ export function SearchPicker({
|
||||
.then((items) => {
|
||||
if (mountedRef.current && seqRef.current === seq) {
|
||||
setResults(items);
|
||||
setResultsQuery(trimmed);
|
||||
setResultsFor({ ok: true, query: trimmed, source });
|
||||
// A new result set invalidates the highlight — index 2 of the old list is not index 2
|
||||
// of the new one.
|
||||
setActiveIndex(-1);
|
||||
@@ -101,7 +114,9 @@ export function SearchPicker({
|
||||
.catch(() => {
|
||||
if (mountedRef.current && seqRef.current === seq) {
|
||||
setResults([]);
|
||||
setResultsQuery(trimmed);
|
||||
// ok: false — a failure, not an authoritative empty result. Marked so the guard above
|
||||
// retries instead of caching the outage forever.
|
||||
setResultsFor({ ok: false, query: trimmed, source });
|
||||
setActiveIndex(-1);
|
||||
setLoading(false);
|
||||
}
|
||||
@@ -109,17 +124,18 @@ export function SearchPicker({
|
||||
}, tooShort ? 0 : 250);
|
||||
|
||||
return () => window.clearTimeout(handle);
|
||||
}, [query, open, search, minQueryLength, mountedRef, resultsQuery]);
|
||||
}, [query, open, search, source, minQueryLength, mountedRef, resultsFor]);
|
||||
|
||||
const trimmedQuery = query.trim();
|
||||
// Whether `results` actually describe what is currently typed. Between a keystroke and its
|
||||
// response they do not, and a stale list must not be committable: typing "Al", highlighting
|
||||
// Alpha, retyping to "Be" and pressing Enter would otherwise select Alpha while the box reads
|
||||
// "Be" (#651 review round 2, BLOCKER 2). The list stays VISIBLE while the next query loads —
|
||||
// hiding it would flicker on every keystroke — it just stops being actionable by keyboard.
|
||||
const resultsCurrent = resultsQuery !== null && resultsQuery === trimmedQuery;
|
||||
// Results from a DIFFERENT source aren't stale, they're wrong — a Collection id offered under a
|
||||
// SmartCollection label — so they are not shown at all. Results from the same source but an older
|
||||
// query stay VISIBLE while the next response loads (hiding them flickers on every keystroke) but
|
||||
// become inert: typing "Al", highlighting Alpha, retyping "Be" and pressing Enter must not select
|
||||
// Alpha (#651 round 2 BLOCKER 2, round 4 HIGH-1).
|
||||
const sameSource = resultsFor !== null && resultsFor.source === source;
|
||||
const resultsCurrent = sameSource && resultsFor.query === trimmedQuery;
|
||||
const noMatches = open && !loading && results.length === 0 && resultsCurrent;
|
||||
const listboxOpen = open && results.length > 0;
|
||||
const listboxOpen = open && sameSource && results.length > 0;
|
||||
const keyboardArmed = listboxOpen && resultsCurrent;
|
||||
|
||||
// Keyboard navigation (#651 F6). Replacing a native <select> with a custom widget removed
|
||||
@@ -138,7 +154,7 @@ export function SearchPicker({
|
||||
onSelect(option.id, option.name);
|
||||
setQuery('');
|
||||
setResults([]);
|
||||
setResultsQuery(null);
|
||||
setResultsFor(null);
|
||||
setActiveIndex(-1);
|
||||
setOpen(false);
|
||||
};
|
||||
|
||||
@@ -228,7 +228,8 @@ describe('FillerPresetsScreen', () => {
|
||||
// Hold the by-id read for the STORED show (9999) open, so it resolves only after the user has
|
||||
// already picked a different show. One bespoke mock — layering a second spy over mockApi's
|
||||
// would re-enter itself.
|
||||
let releaseStoredName: ((response: Response) => void) | null = null;
|
||||
// Holder object rather than a bare `let` — TS narrows a closure-assigned variable to `null`.
|
||||
const releaseStoredName: { resolve: ((response: Response) => void) | null } = { resolve: null };
|
||||
|
||||
vi.spyOn(window, 'fetch').mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const pathname = new URL(input.toString(), 'http://localhost').pathname;
|
||||
@@ -236,7 +237,7 @@ describe('FillerPresetsScreen', () => {
|
||||
|
||||
if (pathname === '/api/v1/shows/9999') {
|
||||
return new Promise<Response>((resolve) => {
|
||||
releaseStoredName = resolve;
|
||||
releaseStoredName.resolve = resolve;
|
||||
});
|
||||
}
|
||||
if (pathname === '/api/v1/filler-presets' && method === 'GET') {
|
||||
@@ -274,7 +275,7 @@ describe('FillerPresetsScreen', () => {
|
||||
|
||||
// NOW the stale read lands. It must not relabel the newer selection: the draft holds 123, so a
|
||||
// title resolved for 9999 is simply not about the current selection.
|
||||
releaseStoredName?.(
|
||||
releaseStoredName.resolve?.(
|
||||
new Response(JSON.stringify({ id: 9999, libraryId: 1, mediaSourceKind: 'Local', title: 'Stored Show' }), {
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
status: 200
|
||||
|
||||
@@ -839,6 +839,7 @@ function FillerEditor({ mode }: { mode: { id: number; kind: 'edit' } | { kind: '
|
||||
}}
|
||||
placeholder={`Search ${activeConfig.label.toLowerCase()}…`}
|
||||
search={searchLibrary}
|
||||
source={activeConfig.value}
|
||||
selectedId={pickerSelectedId}
|
||||
// No id comparison here: there is no reachable path TODAY on which `selectedLabel`
|
||||
// names an id other than the draft's while a selection is shown. (Not "every writer
|
||||
|
||||
@@ -320,4 +320,41 @@ describe('PlaylistsScreen', () => {
|
||||
expect(chip?.textContent).toContain('Another Movie');
|
||||
expect(browseCalls()).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('HIGH-2: options from the PREVIOUS item type are not selectable while the new type loads', async () => {
|
||||
// Same defect as RerunCollectionsScreen, second screen: switching a playlist item's type on a
|
||||
// slow connection must not leave the old namespace's options on offer under the new label
|
||||
// (#651 round 4 HIGH-2).
|
||||
const smartRelease: { resolve: (() => void) | null } = { resolve: null };
|
||||
|
||||
const fetchMock = mockApi();
|
||||
const inner = fetchMock.getMockImplementation() as (i: RequestInfo | URL, r?: RequestInit) => Promise<Response>;
|
||||
fetchMock.mockImplementation((input: RequestInfo | URL, init?: RequestInit) => {
|
||||
const url = input.toString();
|
||||
if (url === '/api/v1/smart-collections' && (init?.method ?? 'GET').toUpperCase() === 'GET') {
|
||||
return new Promise<Response>((resolve) => {
|
||||
smartRelease.resolve = () => resolve(jsonResponse([{ id: 77, name: 'Smart Pick' }]));
|
||||
});
|
||||
}
|
||||
return inner(input, init);
|
||||
});
|
||||
|
||||
render(<PlaylistsScreen />);
|
||||
fireEvent.click(await screen.findByText('Bumps'));
|
||||
fireEvent.click(await screen.findByText('Favorites'));
|
||||
|
||||
// The Collection item's picker is offering its own options.
|
||||
const picker = () => screen.getAllByRole('combobox')[1];
|
||||
await within(picker()).findByRole('option', { name: 'Favorites' });
|
||||
|
||||
const typeSelect = screen.getAllByRole('combobox')[0];
|
||||
fireEvent.change(typeSelect, { target: { value: 'SmartCollection' } });
|
||||
|
||||
// Mid-load: no options from the previous namespace.
|
||||
expect(within(picker()).queryByRole('option', { name: 'Favorites' })).not.toBeInTheDocument();
|
||||
|
||||
smartRelease.resolve?.();
|
||||
expect(await within(picker()).findByRole('option', { name: 'Smart Pick' })).toBeInTheDocument();
|
||||
expect(within(picker()).queryByRole('option', { name: 'Favorites' })).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -400,7 +400,9 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
const [name, setName] = useState('');
|
||||
const [items, setItems] = useState<DraftItem[]>([]);
|
||||
const [selectedKey, setSelectedKey] = useState<string | null>(null);
|
||||
const [pickerItems, setPickerItems] = useState<PickerOption[]>([]);
|
||||
// Options carry the TYPE that produced them — the previous type's options must not stay
|
||||
// selectable under the new type's label while its load is in flight (#651 round 4 HIGH-2).
|
||||
const [pickerFor, setPickerFor] = useState<{ items: PickerOption[]; type: CollectionType } | null>(null);
|
||||
const [pickerError, setPickerError] = useState<string | null>(null);
|
||||
const [pickerHint, setPickerHint] = useState<PickerLoadResult['hint']>('none');
|
||||
const pickerHelpId = useId();
|
||||
@@ -468,14 +470,14 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
loadPickerOptions(selectedType, controller.signal)
|
||||
.then(({ hint, items }) => {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems(items);
|
||||
setPickerFor({ items, type: selectedType });
|
||||
setPickerError(null);
|
||||
setPickerHint(hint);
|
||||
}
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems([]);
|
||||
setPickerFor({ items: [], type: selectedType });
|
||||
setPickerError(messageFromPlaylistError(error, 'Unable to load picker items'));
|
||||
setPickerHint('none');
|
||||
}
|
||||
@@ -486,6 +488,10 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
};
|
||||
}, [selectedType]);
|
||||
|
||||
// Only options loaded FOR the active type may be offered; anything else belongs to another id
|
||||
// namespace and must not be selectable, however briefly.
|
||||
const pickerItems = pickerFor !== null && pickerFor.type === selectedType ? pickerFor.items : [];
|
||||
|
||||
const browseMediaType = selectedType !== undefined ? configFor(selectedType)?.browse : undefined;
|
||||
|
||||
// Stable across renders: `SearchPicker`'s debounce effect lists `search` in its deps, so a fresh
|
||||
@@ -786,6 +792,7 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
onSelect={(id, name) => updateItem(selectedItem.key, { selectedId: id, selectedName: name })}
|
||||
placeholder={`Search ${(activeConfig?.label ?? 'items').toLowerCase()}…`}
|
||||
search={searchLibrary}
|
||||
source={selectedItem.collectionType}
|
||||
selectedId={selectedItem.selectedId}
|
||||
selectedName={selectedItem.selectedName || null}
|
||||
/>
|
||||
|
||||
@@ -810,4 +810,164 @@ describe('RerunCollectionsScreen', () => {
|
||||
expect(screen.queryByText('#5')).not.toBeInTheDocument();
|
||||
expect(screen.getByLabelText('Remote Stream search')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('HIGH-2: options from the PREVIOUS type are not selectable while the new type loads', async () => {
|
||||
// Slow connection: switching Collection -> SmartCollection clears the selection immediately but
|
||||
// the smart-collections request is still in flight. The old Collection options must not remain
|
||||
// on offer under the SmartCollection label — picking one would write a Collection id into the
|
||||
// SmartCollection namespace (#651 round 4 HIGH-2).
|
||||
const smartRelease: { resolve: (() => void) | null } = { resolve: null };
|
||||
|
||||
const fetchMock = mockApi({ list: [] });
|
||||
fetchMock.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/smart-collections' && method === 'GET') {
|
||||
return new Promise<Response>((resolve) => {
|
||||
smartRelease.resolve = () => resolve(jsonResponse([{ id: 77, name: 'Smart Pick' }]));
|
||||
});
|
||||
}
|
||||
if (url.pathname === '/api/v1/collections' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse(manualCollections));
|
||||
}
|
||||
if (url.pathname === '/api/v1/rerun-collections' && method === 'GET') {
|
||||
return Promise.resolve(jsonResponse({ page: [], totalCount: 0 }));
|
||||
}
|
||||
return Promise.resolve(new Response(null, { status: 204 }));
|
||||
});
|
||||
|
||||
render(<RerunCollectionsScreen />);
|
||||
fireEvent.click(await screen.findByRole('button', { name: 'New rerun collection' }));
|
||||
|
||||
const picker = () => (screen.getAllByRole('combobox'))[1];
|
||||
await within(picker()).findByText('Favorites');
|
||||
|
||||
const typeSelect = screen.getAllByRole('combobox')[0];
|
||||
fireEvent.change(typeSelect, { target: { value: 'SmartCollection' } });
|
||||
|
||||
// Mid-load: the picker offers nothing rather than the wrong namespace's rows.
|
||||
expect(within(picker()).queryByText('Favorites')).not.toBeInTheDocument();
|
||||
expect(within(picker()).queryByRole('option', { name: 'Favorites' })).not.toBeInTheDocument();
|
||||
|
||||
smartRelease.resolve?.();
|
||||
expect(await within(picker()).findByText('Smart Pick')).toBeInTheDocument();
|
||||
expect(within(picker()).queryByText('Favorites')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('HIGH-3: a touched selection contradicting the server type is a CONFLICT, not a hydrate', async () => {
|
||||
// The list row is stale: it says Collection/5, but a collaborator has already retyped the
|
||||
// record to SmartCollection/8. The user picks Collection/9 before the detail GET returns.
|
||||
// Pinning their old-type identity while installing the response's NEW ETag would authorize a
|
||||
// Save that silently overwrites the collaborator's type change with no 412 (#651 round 4
|
||||
// HIGH-3).
|
||||
const detailRelease: { resolve: (() => void) | null } = { resolve: null };
|
||||
let detailCalls = 0;
|
||||
|
||||
const fetchMock = mockApi({
|
||||
list: [{ ...storedSelection, collectionType: 'Collection', selectedId: 5, selectedName: 'Favorites' }]
|
||||
});
|
||||
|
||||
// Count in the OUTER implementation: it short-circuits the detail GET before mockApi's own
|
||||
// handlers (and its `onRequest` hook) ever see it.
|
||||
const inner = fetchMock.getMockImplementation() as (i: RequestInfo | URL, r?: RequestInit) => Promise<Response>;
|
||||
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;
|
||||
return new Promise<Response>((resolve) => {
|
||||
detailRelease.resolve = () =>
|
||||
resolve(
|
||||
new Response(
|
||||
JSON.stringify({
|
||||
...storedSelection,
|
||||
collectionType: 'SmartCollection',
|
||||
selectedId: 8,
|
||||
selectedName: 'Collaborator Smart'
|
||||
}),
|
||||
{ headers: { 'Content-Type': 'application/json', ETag: '"v9-newest"' }, status: 200 }
|
||||
)
|
||||
);
|
||||
});
|
||||
}
|
||||
return inner(input, init);
|
||||
});
|
||||
|
||||
render(<RerunCollectionsScreen />);
|
||||
fireEvent.click(await screen.findByText('Stored Rerun'));
|
||||
await waitFor(() => expect(detailCalls).toBe(1));
|
||||
|
||||
// The user retargets within the (stale) Collection type while the GET is in flight.
|
||||
const collectionPicker = (await screen.findAllByRole('combobox'))[1];
|
||||
fireEvent.change(collectionPicker, { target: { value: '5' } });
|
||||
|
||||
detailRelease.resolve?.();
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
|
||||
// The collision surfaces as the existing conflict dialog rather than a silent reconcile.
|
||||
const conflict = await screen.findByRole('dialog');
|
||||
expect(within(conflict).getByRole('button', { name: 'Reload' })).toBeInTheDocument();
|
||||
expect(within(conflict).getByText(/your unsaved changes will be discarded/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('MEDIUM-4: edits made while a Reload is pending are refused, not silently erased', async () => {
|
||||
const reloadRelease: { resolve: (() => void) | null } = { resolve: null };
|
||||
let detailCalls = 0;
|
||||
|
||||
const fetchMock = mockApi({
|
||||
list: [{ ...storedSelection, collectionType: 'Collection', selectedId: 5, selectedName: 'Favorites' }],
|
||||
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<Response>;
|
||||
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(
|
||||
jsonResponse({ ...storedSelection, collectionType: 'Collection', selectedId: 5, selectedName: 'Favorites' })
|
||||
);
|
||||
}
|
||||
// The RELOAD: held open so the user can try to type into a doomed draft.
|
||||
return new Promise<Response>((resolve) => {
|
||||
reloadRelease.resolve = () =>
|
||||
resolve(
|
||||
jsonResponse({
|
||||
...storedSelection,
|
||||
collectionType: 'Collection',
|
||||
name: 'Server Name',
|
||||
selectedId: 5,
|
||||
selectedName: 'Favorites'
|
||||
})
|
||||
);
|
||||
});
|
||||
}
|
||||
return inner(input, init);
|
||||
});
|
||||
|
||||
render(<RerunCollectionsScreen />);
|
||||
fireEvent.click(await screen.findByText('Stored Rerun'));
|
||||
await waitFor(() => expect(detailCalls).toBe(1));
|
||||
|
||||
fireEvent.click(screen.getByRole('button', { name: 'Save rerun collection' }));
|
||||
const dialog = await screen.findByRole('dialog');
|
||||
fireEvent.click(within(dialog).getByRole('button', { name: 'Reload' }));
|
||||
await waitFor(() => expect(detailCalls).toBe(2));
|
||||
|
||||
// The dialog is gone but the replacement has not landed: the editor must be inert and say so,
|
||||
// rather than accepting an edit it is about to throw away.
|
||||
expect(screen.getByText('Reloading…')).toBeInTheDocument();
|
||||
expect(screen.getByRole('button', { name: 'Save rerun collection' })).toBeDisabled();
|
||||
|
||||
const nameInput = screen.getByPlaceholderText('Rerun collection name') as HTMLInputElement;
|
||||
fireEvent.change(nameInput, { target: { value: 'Edited While Pending' } });
|
||||
expect(nameInput.value).not.toBe('Edited While Pending');
|
||||
|
||||
reloadRelease.resolve?.();
|
||||
await waitFor(() => expect(screen.queryByText('Reloading…')).not.toBeInTheDocument());
|
||||
// The server's record won, and the doomed edit never existed.
|
||||
expect((screen.getByPlaceholderText('Rerun collection name') as HTMLInputElement).value).toBe('Server Name');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -273,10 +273,24 @@ function hydrateSelection(current: Draft, refreshed: Draft): Pick<Draft, 'select
|
||||
return { selectedId: refreshed.selectedId, selectedName: refreshed.selectedName };
|
||||
}
|
||||
|
||||
// A touched local identity that contradicts the server's TYPE is not something to reconcile — it is
|
||||
// a conflict. Pinning the user's old-type identity while installing the response's newest ETag
|
||||
// authorizes a Save that silently overwrites the collaborator's type change with no 412 (#651 round
|
||||
// 4 HIGH-3 — the second cross-user lost update on this branch, so the category is "any path that
|
||||
// installs a save-authorizing ETag over a local edit the server contradicts").
|
||||
function identityConflicts(current: Draft, refreshed: Draft, touched: ReadonlySet<TouchedField>): boolean {
|
||||
return (
|
||||
(touched.has('collectionType') || touched.has('selection')) &&
|
||||
refreshed.collectionType !== current.collectionType
|
||||
);
|
||||
}
|
||||
|
||||
function hydrateIdentity(current: Draft, refreshed: Draft, touched: ReadonlySet<TouchedField>): Identity {
|
||||
// Touching EITHER half pins the whole unit. `setCollectionType` already touches both, but a user
|
||||
// who only changed the selection has implicitly pinned the type too — it is the type their
|
||||
// chosen id belongs to, so letting the type hydrate underneath it recreates the cross-type bug.
|
||||
// (When that pin CONTRADICTS the server's type the caller must treat it as a conflict rather than
|
||||
// hydrate at all — see `identityConflicts`.)
|
||||
if (touched.has('collectionType') || touched.has('selection')) {
|
||||
return identityOf(current);
|
||||
}
|
||||
@@ -340,7 +354,12 @@ function RerunCollectionEditor({
|
||||
selectedName: ''
|
||||
}
|
||||
);
|
||||
const [pickerItems, setPickerItems] = useState<PickerOption[]>([]);
|
||||
// Options carry the TYPE that produced them. Keeping a bare array meant the previous type's
|
||||
// options stayed on screen under the new type's label while its load was in flight, so a slow
|
||||
// connection let the user store e.g. a Collection id in the SmartCollection namespace (#651
|
||||
// round 4 HIGH-2). Same rule as the search picker's `source`: an id is only meaningful inside
|
||||
// the namespace it came from.
|
||||
const [pickerFor, setPickerFor] = useState<{ items: PickerOption[]; type: RerunCollectionType } | null>(null);
|
||||
const [pickerError, setPickerError] = useState<string | null>(null);
|
||||
const [pickerHint, setPickerHint] = useState<PickerLoadResult['hint']>('none');
|
||||
const pickerHelpId = useId();
|
||||
@@ -351,6 +370,12 @@ function RerunCollectionEditor({
|
||||
// the mount refresh (protect user edits, tolerate a #671 unnamed selection), 'replace' is the
|
||||
// conflict reload (adopt the server's record wholesale, discarding local edits as promised).
|
||||
const [load, setLoad] = useState<{ key: number; mode: 'hydrate' | 'replace' }>({ key: 0, mode: 'hydrate' });
|
||||
// True from the moment Reload is confirmed until the replacement lands. The dialog closes
|
||||
// immediately, so without this the user can keep editing a draft that is about to be thrown away
|
||||
// — the response would silently erase those edits AND reset the touched set that protected them
|
||||
// (#651 round 4 MEDIUM-4). Edits are refused for the duration rather than queued: the whole
|
||||
// point of Reload is that the local state is being discarded.
|
||||
const [replacePending, setReplacePending] = useState(false);
|
||||
// Concurrency ETag (issue #253): captured from the single-record GET when editing an existing
|
||||
// rerun collection, sent as If-Match on save. There's no rotation-on-save because the editor
|
||||
// always returns to the list after a successful save (see onSaved below).
|
||||
@@ -361,12 +386,19 @@ function RerunCollectionEditor({
|
||||
|
||||
// Every user edit goes through this, so "did the user touch it?" cannot drift from "did it
|
||||
// change?" — a field mutated anywhere else would silently become hydratable again.
|
||||
const edit = useCallback((fields: TouchedField[], patch: (current: Draft) => Partial<Draft>) => {
|
||||
for (const field of fields) {
|
||||
touchedRef.current.add(field);
|
||||
}
|
||||
setDraft((current) => ({ ...current, ...patch(current) }));
|
||||
}, []);
|
||||
const edit = useCallback(
|
||||
(fields: TouchedField[], patch: (current: Draft) => Partial<Draft>) => {
|
||||
if (replacePending) {
|
||||
return;
|
||||
}
|
||||
|
||||
for (const field of fields) {
|
||||
touchedRef.current.add(field);
|
||||
}
|
||||
setDraft((current) => ({ ...current, ...patch(current) }));
|
||||
},
|
||||
[replacePending]
|
||||
);
|
||||
|
||||
const { collectionType } = draft;
|
||||
|
||||
@@ -383,19 +415,35 @@ function RerunCollectionEditor({
|
||||
getRerunCollectionWithMeta(initial.id)
|
||||
.then((meta) => {
|
||||
if (active) {
|
||||
etagRef.current = meta.etag;
|
||||
if (mode === 'replace') {
|
||||
// Adopt the server's record and forget every local edit — including the touched set,
|
||||
// which is what made those edits sticky in the first place.
|
||||
etagRef.current = meta.etag;
|
||||
touchedRef.current = new Set();
|
||||
setDraft(replaceDraft(meta.data));
|
||||
} else {
|
||||
setDraft((current) => hydrateDraft(current, meta.data, touchedRef.current));
|
||||
setReplacePending(false);
|
||||
return;
|
||||
}
|
||||
|
||||
setDraft((current) => {
|
||||
if (identityConflicts(current, draftFromRerun(meta.data), touchedRef.current)) {
|
||||
// Do NOT install the new ETag: it would authorize a Save that overwrites the type
|
||||
// change we just learned about. Leaving the stale ETag means even a forced Save gets
|
||||
// a 412 rather than clobbering — belt and braces with the dialog below.
|
||||
setConflictOpen(true);
|
||||
return current;
|
||||
}
|
||||
|
||||
etagRef.current = meta.etag;
|
||||
return hydrateDraft(current, meta.data, touchedRef.current);
|
||||
});
|
||||
}
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (active) {
|
||||
// Unfreeze on failure as well — a stuck `replacePending` would leave the editor
|
||||
// permanently read-only with no way back.
|
||||
setReplacePending(false);
|
||||
setSaveError(messageFromRerunCollectionError(error, 'Unable to load rerun collection'));
|
||||
}
|
||||
});
|
||||
@@ -415,14 +463,14 @@ function RerunCollectionEditor({
|
||||
loadPickerOptions(collectionType, controller.signal)
|
||||
.then(({ hint, items }) => {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems(items);
|
||||
setPickerFor({ items, type: collectionType });
|
||||
setPickerError(null);
|
||||
setPickerHint(hint);
|
||||
}
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
if (!controller.signal.aborted) {
|
||||
setPickerItems([]);
|
||||
setPickerFor({ items: [], type: collectionType });
|
||||
setPickerError(messageFromRerunCollectionError(error, 'Unable to load picker items'));
|
||||
setPickerHint('none');
|
||||
}
|
||||
@@ -450,6 +498,10 @@ function RerunCollectionEditor({
|
||||
edit(['selection'], (current) => ({ selectedId: id, selectedName: match?.name ?? current.selectedName }));
|
||||
};
|
||||
|
||||
// Only options loaded FOR the active type may be offered; anything else belongs to another id
|
||||
// namespace and must not be selectable, however briefly.
|
||||
const pickerItems = pickerFor !== null && pickerFor.type === collectionType ? pickerFor.items : [];
|
||||
|
||||
const activeConfig = COLLECTION_TYPES.find((entry) => entry.value === collectionType);
|
||||
const browseMediaType = COLLECTION_TYPES.find(
|
||||
(entry) => entry.value === collectionType && entry.source === 'browse'
|
||||
@@ -466,7 +518,7 @@ function RerunCollectionEditor({
|
||||
trimmedName.length === 0 ? 'Name is required' : draft.selectedId == null ? 'A selection is required' : null;
|
||||
|
||||
const save = async () => {
|
||||
if (validationError || saving || draft.selectedId == null) {
|
||||
if (validationError || saving || replacePending || draft.selectedId == null) {
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -504,6 +556,7 @@ function RerunCollectionEditor({
|
||||
const reloadAfterConflict = () => {
|
||||
setConflictOpen(false);
|
||||
setSaveError(null);
|
||||
setReplacePending(true);
|
||||
setLoad((current) => ({ key: current.key + 1, mode: 'replace' }));
|
||||
};
|
||||
|
||||
@@ -532,10 +585,11 @@ function RerunCollectionEditor({
|
||||
</span>
|
||||
<span className="ctv-channels-spacer" />
|
||||
{saveError && <span className="ctv-settings-savebar-error">{saveError}</span>}
|
||||
{validationError && <Badge tone="neutral">{validationError}</Badge>}
|
||||
{replacePending && <Badge tone="neutral">Reloading…</Badge>}
|
||||
{!replacePending && validationError && <Badge tone="neutral">{validationError}</Badge>}
|
||||
<Button
|
||||
disabled={validationError !== null || saving}
|
||||
loading={saving}
|
||||
disabled={validationError !== null || saving || replacePending}
|
||||
loading={saving || replacePending}
|
||||
onClick={() => void save()}
|
||||
size="sm"
|
||||
startIcon={<Check aria-hidden="true" size={14} />}
|
||||
@@ -576,6 +630,7 @@ function RerunCollectionEditor({
|
||||
onSelect={(id, name) => edit(['selection'], () => ({ selectedId: id, selectedName: name }))}
|
||||
placeholder={`Search ${(activeConfig?.label ?? 'items').toLowerCase()}…`}
|
||||
search={searchLibrary}
|
||||
source={collectionType}
|
||||
selectedId={draft.selectedId}
|
||||
selectedName={draft.selectedName || null}
|
||||
/>
|
||||
|
||||
Reference in New Issue
Block a user