fix(651): make the stated invariant true on Playlists; pin the predicate's endpoints

The review's headline finding was in my prose, not my code: spa-conventions and the round-8
commit both claimed an unbindable id "surfaces as 'no selection' with Save disabled" and that
regressions "assert zero writes are reachable". True on RerunCollections and FillerPresets.
False on PlaylistsScreen in all three respects — `draftFromItem` nulled the id but KEPT
`selectedName`, so the row read "Cool Movie" over a null draft; Save had no selection check;
and clicking it did issue the PUT with `mediaItemId: null`. Only the server's
`ReplacePlaylistItemsHandler` 422 stood there, and the DB would have persisted it
(`PlaylistItemConfiguration` marks all four FKs `IsRequired(false)`).

Rather than weaken the claim, made it true: a dropped id now clears its label, and Save is
gated on every item having a selection, with a visible count as the reason.

`playlistGroupId` was the same class on the same screen — seeded from the wire into
`AddPlaylistDialog`, re-parsed with a bare `Number()`, and POSTed as an entity reference — so
"every path by which an id from the wire becomes editor state" was not literally true. Now
filtered from the group options and normalized on submit.

Added `selectionId.test.ts`. The predicate had become the single point of failure for eleven
call sites across three screens while being exercised only indirectly; nothing pinned the
inclusive endpoints, so a `>` for `>=` slip passed the entire suite. Verified by mutating
each comparison. Also documented why `0` and negatives are accepted — the contract is
bindability, not existence — because every other id check in this repo uses `id > 0` and the
next reader would otherwise "fix" the inconsistency.

Two of my assertions were vacuous, the eighth of that shape on this branch: one clicked a
button it had just asserted disabled (a restatement of `toBeDisabled()`), and one asserted a
POST count on a path that never attempted a save. The first is deleted; the second now
actually attempts the write, which makes it fail against the unguarded parent.

Corrected claim: all five round-8 regressions do fail against their parent, but on their
load-bearing assertions (`getByText('A selection is required')`,
`queryByText('Bogus Collection')`) — not on the write-count ones, which were passengers.

Follow-up filed as #677 (ScheduleItemInspector's unguarded ingresses; list-backed pickers
dropping malformed options silently).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

docs/spa-conventions.md

@@ -302,10 +302,18 @@ appear once the widget is asynchronous:
   and the selection restored from a detail read — so a check added to whichever one surfaced the
   defect leaves the others open (this is how #651 produced the same finding in two consecutive
   rounds). Share one predicate (`isSelectionId` / `selectionIdOrNull` in `web/src/api/selectionId.ts`)
-  and apply it on every path. **Treat an unbindable id as ABSENT, never coerce it** — rounding
-  `1.5` to `1` would submit a *different* record — so it surfaces as "no selection" with Save
-  disabled, and drop rather than render an option that cannot be selected safely. Prove it per
-  ingress by asserting **zero writes are reachable**, not just that the field looks empty.
+  and apply it on every path — including the ones that don't look like pickers, such as a
+  `playlistGroupId` seeded from the wire into a create dialog. **Treat an unbindable id as ABSENT,
+  never coerce it** — rounding `1.5` to `1` would submit a *different* record — and **clear its
+  label with it**: a row still reading "Blade Runner" over a null id makes two contradictory
+  statements about the same item. Drop rather than render an option that cannot be selected safely.
+  "Surfaces as no selection" is only true if that screen's Save gate actually checks for one — on
+  `PlaylistsScreen` it did not, so this claim was false there for a full round after being written
+  here. **Verify an invariant on every screen it names before writing it down.** Prove it per
+  ingress by asserting zero writes are reachable *after attempting the write*: a write-count
+  assertion on a path that never attempts one is trivially true. And unit-test the predicate's
+  INCLUSIVE endpoints directly — once it is the single point of failure for every ingress, a `>`
+  for `>=` slip passes an entire screen suite.
 - **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. Validate the **elements, not just the container**: `Array.isArray` accepts

web/src/api/selectionId.test.ts

@@ -0,0 +1,55 @@
+import { describe, expect, it } from 'vitest';
+import { INT32_MAX, INT32_MIN, isSelectionId, selectionIdOrNull } from './selectionId';
+
+// This predicate is the single point of failure for every id that reaches editor state across three
+// screens (#651 round 8), and until now it was only exercised indirectly through screen tests. The
+// endpoints matter most: swapping either `>=`/`<=` for a strict comparison is the classic mutation
+// on exactly this code, and nothing else in the suite would notice.
+describe('isSelectionId', () => {
+  it('accepts the INCLUSIVE int32 endpoints', () => {
+    expect(INT32_MAX).toBe(2_147_483_647);
+    expect(INT32_MIN).toBe(-2_147_483_648);
+    // A `>` for `>=` slip in either comparison fails here and nowhere else.
+    expect(isSelectionId(INT32_MAX)).toBe(true);
+    expect(isSelectionId(INT32_MIN)).toBe(true);
+  });
+
+  it('rejects values one step outside the range', () => {
+    expect(isSelectionId(INT32_MAX + 1)).toBe(false);
+    expect(isSelectionId(INT32_MIN - 1)).toBe(false);
+  });
+
+  it('accepts ordinary ids, including 0 and negatives', () => {
+    // Bindability, not existence — see the note in selectionId.ts. `id > 0` would break this.
+    expect(isSelectionId(1)).toBe(true);
+    expect(isSelectionId(0)).toBe(true);
+    expect(isSelectionId(-1)).toBe(true);
+  });
+
+  it('rejects non-integers and non-numbers', () => {
+    for (const value of [1.5, -0.5, Number.NaN, Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY]) {
+      expect(isSelectionId(value)).toBe(false);
+    }
+
+    for (const value of ['1', null, undefined, {}, [], true, 1n]) {
+      expect(isSelectionId(value)).toBe(false);
+    }
+  });
+});
+
+describe('selectionIdOrNull', () => {
+  it('passes a bindable id through unchanged', () => {
+    expect(selectionIdOrNull(42)).toBe(42);
+    expect(selectionIdOrNull(INT32_MAX)).toBe(INT32_MAX);
+    expect(selectionIdOrNull(0)).toBe(0);
+  });
+
+  it('maps anything unbindable to null — never a coerced value', () => {
+    // Rounding 1.5 to 1 would submit a DIFFERENT record; absence is the only safe answer.
+    expect(selectionIdOrNull(1.5)).toBeNull();
+    expect(selectionIdOrNull(INT32_MAX + 1)).toBeNull();
+    expect(selectionIdOrNull('7')).toBeNull();
+    expect(selectionIdOrNull(null)).toBeNull();
+    expect(selectionIdOrNull(undefined)).toBeNull();
+  });
+});

web/src/api/selectionId.ts

@@ -8,6 +8,12 @@
 // selection restored from a detail read unguarded, so the identical malformed value entered editor
 // state through a different door (round 8). The predicate belongs at the BOUNDARY the class
 // crosses: every path by which an id from the wire becomes editor state.
+// DELIBERATELY not `id > 0`. Most id checks in this codebase (`routing.ts` and eight screens) use
+// `Number.isInteger(id) && id > 0` because they are asking "could this id EXIST?". This predicate
+// asks a different question — "can the API BIND this value as its `int` parameter?" — so `0` and
+// negatives are accepted: they are perfectly bindable, and rejecting them here would silently
+// convert a server-side 404/422 (a clear answer) into a client-side "no selection" (a confusing
+// one). Do not "fix" the inconsistency; the two predicates are answering different questions.
 export const INT32_MIN = -2_147_483_648;
 export const INT32_MAX = 2_147_483_647;
 

web/src/screens/PlaylistsScreen.test.tsx

@@ -357,4 +357,83 @@ describe('PlaylistsScreen', () => {
     expect(await within(picker()).findByRole('option', { name: 'Smart Pick' })).toBeInTheDocument();
     expect(within(picker()).queryByRole('option', { name: 'Favorites' })).not.toBeInTheDocument();
   });
+
+  // ---- #651 round 9: making the stated invariant actually true on this screen ----
+
+  it.each([
+    { id: 1.5, label: 'a fractional mediaItemId' },
+    { id: 2_147_483_648, label: 'a mediaItemId above int32' }
+  ])('round 9: $label drops its NAME too, blocks Save, and cannot reach a PUT', async ({ id }) => {
+    // The invariant claimed in spa-conventions — "surfaces as no selection, Save disabled, zero
+    // writes reachable" — was false here in all three respects: the name survived the dropped id
+    // (so the row read "Cool Movie" while the draft held null), Save had no selection check, and
+    // clicking it issued the PUT with `mediaItemId: null`.
+    const fetchMock = mockApi({
+      onRequest: (url, method) => {
+        if (url === '/api/v1/playlists/10/items' && method === 'GET') {
+          return jsonResponse([
+            {
+              ...(playlistItems[10] as Array<Record<string, unknown>>)[1],
+              mediaItemId: id,
+              mediaItemName: 'Cool Movie'
+            }
+          ]);
+        }
+        return null;
+      }
+    });
+
+    render(<PlaylistsScreen />);
+    fireEvent.click(await screen.findByText('Bumps'));
+
+    // LOAD-BEARING 1: the label goes with the id — no row claiming a selection the draft lacks.
+    expect(await screen.findByText(/no movie selected/i)).toBeInTheDocument();
+    expect(screen.queryByText('Cool Movie')).not.toBeInTheDocument();
+
+    // LOAD-BEARING 2: Save is gated on it, with a visible reason.
+    expect(screen.getByText(/need a selection/i)).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Save playlist' })).toBeDisabled();
+
+    // LOAD-BEARING 3: and the write is genuinely unreachable, not merely discouraged.
+    fireEvent.click(screen.getByRole('button', { name: 'Save playlist' }));
+    await new Promise((resolve) => setTimeout(resolve, 0));
+    expect(
+      fetchMock.mock.calls.filter(([u, i]) => u === '/api/v1/playlists/10/items' && (i?.method ?? '').toUpperCase() === 'PUT')
+    ).toHaveLength(0);
+  });
+
+  it('round 9: an unbindable playlist GROUP id is not offered and cannot be posted', async () => {
+    // `playlistGroupId` is seeded from the wire and submitted as an entity reference — the same
+    // class, on the same screen, which the round-8 sweep missed.
+    const fetchMock = mockApi({
+      onRequest: (url, method) =>
+        url === '/api/v1/playlists/groups' && method === 'GET'
+          ? jsonResponse([
+              { id: 2_147_483_648, isSystem: false, name: 'Bogus Group', playlistCount: 0 },
+              { id: 1, isSystem: false, name: 'Idents', playlistCount: 1 }
+            ])
+          : null
+    });
+
+    render(<PlaylistsScreen />);
+    fireEvent.click(await screen.findByRole('button', { name: 'Add playlist' }));
+
+    const dialog = await screen.findByRole('dialog');
+    const groupSelect = within(dialog).getByRole('combobox');
+    // LOAD-BEARING: the unbindable group is not offered, so it cannot be chosen or submitted.
+    expect(within(groupSelect).getByRole('option', { name: 'Idents' })).toBeInTheDocument();
+    expect(within(groupSelect).queryByRole('option', { name: 'Bogus Group' })).not.toBeInTheDocument();
+
+    fireEvent.change(groupSelect, { target: { value: '2147483648' } });
+    fireEvent.change(within(dialog).getByPlaceholderText('Playlist name'), { target: { value: 'New' } });
+    fireEvent.click(within(dialog).getByRole('button', { name: 'Create' }));
+    await new Promise((resolve) => setTimeout(resolve, 0));
+
+    const posts = fetchMock.mock.calls.filter(
+      ([u, i]) => u === '/api/v1/playlists' && (i?.method ?? '').toUpperCase() === 'POST'
+    );
+    for (const post of posts) {
+      expect(JSON.parse(String(post[1]?.body)).playlistGroupId).not.toBe(2_147_483_648);
+    }
+  });
 });

web/src/screens/PlaylistsScreen.tsx

@@ -233,6 +233,10 @@ function draftFromItem(item: PlaylistItem): DraftItem {
   const source = configFor(item.collectionType)?.source ?? 'browse';
   let selectedId: number | null;
   let selectedName: string;
+  // NOTE the name is cleared with the id below whenever `selectionIdOrNull` rejects one: keeping a
+  // label for an id we refused to store makes the row claim "Blade Runner" while the draft holds
+  // null — two contradictory statements about the same item, and no "no selection" signal
+  // (#651 round 9).
 
   switch (source) {
     case 'collection':
@@ -252,6 +256,11 @@ function draftFromItem(item: PlaylistItem): DraftItem {
       selectedName = item.mediaItemName ?? '';
   }
 
+  if (selectedId === null) {
+    // An id we declined to store cannot have a valid label.
+    selectedName = '';
+  }
+
   return {
     collectionType: item.collectionType,
     count: item.count != null ? String(item.count) : '',
@@ -352,10 +361,14 @@ function AddPlaylistDialog({
   onSubmit: (values: { name: string; playlistGroupId: number }) => void;
   open: boolean;
 }) {
-  const [groupId, setGroupId] = useState(() => (groups[0] ? String(groups[0].id) : ''));
+  // The group id is seeded from the wire and submitted as an entity reference, so it is a selection
+  // id by `selectionId.ts`'s own definition and gets the same boundary treatment — otherwise "every
+  // path by which an id from the wire becomes editor state" is not literally true (#651 round 9).
+  const bindableGroups = groups.filter((group) => isSelectionId(group.id));
+  const [groupId, setGroupId] = useState(() => (bindableGroups[0] ? String(bindableGroups[0].id) : ''));
   const [name, setName] = useState('');
   const trimmed = name.trim();
-  const numericGroupId = groupId === '' ? null : Number(groupId);
+  const numericGroupId = selectionIdOrNull(groupId === '' ? null : Number(groupId));
 
   return (
     <Dialog
@@ -382,7 +395,7 @@ function AddPlaylistDialog({
       <Select
         label="Playlist group"
         onChange={(event) => setGroupId(event.target.value)}
-        options={groups.map((group) => ({ label: group.name, value: String(group.id) }))}
+        options={bindableGroups.map((group) => ({ label: group.name, value: String(group.id) }))}
         value={groupId}
       />
       <div style={{ marginTop: 12 }}>
@@ -571,8 +584,20 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
 
   const buildRequest = () => ({ items: items.map(toItemRequest), name: name.trim() });
 
+  // Every item must carry a selection the API can bind. Without this the screen happily PUT an item
+  // with a null id — the server 422s it (`ReplacePlaylistItemsHandler.CollectionTypeMustBeValid`),
+  // but the DB would persist it (`PlaylistItemConfiguration` marks all four FKs `IsRequired(false)`),
+  // so that handler check is the only thing standing there. Fail closed on the client too.
+  const itemsMissingSelection = items.filter((item) => item.selectedId == null).length;
+  const validationError =
+    name.trim().length === 0
+      ? 'Name is required'
+      : itemsMissingSelection > 0
+        ? `${itemsMissingSelection} item${itemsMissingSelection === 1 ? '' : 's'} need a selection`
+        : null;
+
   const save = async () => {
-    if (isSystem || saving || name.trim().length === 0) {
+    if (isSystem || saving || validationError !== null) {
       return;
     }
 
@@ -684,8 +709,9 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
         >
           Add item
         </Button>
+        {!isSystem && validationError && <Badge tone="neutral">{validationError}</Badge>}
         <Button
-          disabled={isSystem || saving || name.trim().length === 0}
+          disabled={isSystem || saving || validationError !== null}
           loading={saving}
           onClick={() => void save()}
           size="sm"

web/src/screens/RerunCollectionsScreen.test.tsx

@@ -972,7 +972,7 @@ describe('RerunCollectionsScreen', () => {
     { id: 2_147_483_648, label: 'a selectedId above int32' },
     { id: -2_147_483_649, label: 'a selectedId below int32' }
   ])('round 8: $label in the DETAIL response cannot enable Save or reach a PUT', async ({ id }) => {
-    const fetchMock = mockApi({
+    mockApi({
       list: [storedSelection],
       onRequest: (url, method) =>
         url === '/api/v1/rerun-collections/9' && method === 'GET'
@@ -985,14 +985,10 @@ describe('RerunCollectionsScreen', () => {
     await screen.findByPlaceholderText('Rerun collection name');
 
     // Treated as ABSENT — visibly, with the reason — rather than as a selection that fails on write.
+    // These two ARE the load-bearing assertions; clicking a button already asserted disabled would
+    // only restate `toBeDisabled()`, so it is deliberately not done here.
     expect(screen.getByText('A selection is required')).toBeInTheDocument();
     expect(screen.getByRole('button', { name: 'Save rerun collection' })).toBeDisabled();
-
-    fireEvent.click(screen.getByRole('button', { name: 'Save rerun collection' }));
-    await new Promise((resolve) => setTimeout(resolve, 0));
-    expect(
-      fetchMock.mock.calls.filter(([u, i]) => u === '/api/v1/rerun-collections/9' && (i?.method ?? '').toUpperCase() === 'PUT')
-    ).toHaveLength(0);
   });
 
   it.each([
@@ -1014,12 +1010,18 @@ describe('RerunCollectionsScreen', () => {
     fireEvent.click(await screen.findByRole('button', { name: 'New rerun collection' }));
 
     const picker = (await screen.findAllByRole('combobox'))[1];
-    // The bindable option is offered; the malformed one is dropped rather than rendered.
+    // LOAD-BEARING: the bindable option is offered, the malformed one is not rendered at all.
     expect(await within(picker).findByText('Favorites')).toBeInTheDocument();
     expect(within(picker).queryByText('Bogus Collection')).not.toBeInTheDocument();
-    expect(within(picker).queryByRole('option', { name: 'Bogus Collection' })).not.toBeInTheDocument();
 
-    // ...and no POST can carry it.
+    // Now actually try to submit it. Selecting a value the <select> never offered leaves the draft
+    // without a selection, so the save is refused — asserting a POST count without attempting one
+    // would be trivially true and would pass against the unguarded parent too.
+    fireEvent.change(picker, { target: { value: String(id) } });
+    fireEvent.change(screen.getByPlaceholderText('Rerun collection name'), { target: { value: 'Attempted' } });
+    fireEvent.click(screen.getByRole('button', { name: 'Add rerun collection' }));
+    await new Promise((resolve) => setTimeout(resolve, 0));
+
     expect(
       fetchMock.mock.calls.filter(([u, i]) => u === '/api/v1/rerun-collections' && (i?.method ?? '').toUpperCase() === 'POST')
     ).toHaveLength(0);