fix(684): key the pageSize guard registry on identity, not source position
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 15s
PR Gates / Docs update reminder (pull_request) Successful in 21s
review-verdict/h10 Awaiting review verdict for fc8353c
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 25s
PR Gates / decisions lifecycle (pull_request) Successful in 34s
Review verdict / Set review-verdict status (pull_request) Successful in 31s
PR Gates / Script tests (pytest) (pull_request) Successful in 44s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m42s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m38s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 21m16s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m38s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 15s
PR Gates / Docs update reminder (pull_request) Successful in 21s
review-verdict/h10 Awaiting review verdict for fc8353c
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 25s
PR Gates / decisions lifecycle (pull_request) Successful in 34s
Review verdict / Set review-verdict status (pull_request) Successful in 31s
PR Gates / Script tests (pytest) (pull_request) Successful in 44s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m42s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m38s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 21m16s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m38s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
The #650 guard pinned every pageSize call site by absolute line:column, making the registry a function of every OTHER file in the repo. The guard was BORN RED. #651 moved AutoTuneScreen.tsx up ten lines and FillerPresetsScreen.tsx down seventy-two, and merged to main BEFORE the guard's own PR (#675) did -- so the registry, authored against a pre-#651 base, was stale the instant it landed. Its own merge run was CANCELLED, so nothing reported it; the red first surfaced on the next push (#676's merge, which touches no web/src file and is not the cause). One ordering accident, not a recurring pattern -- but the exposure is general, because every PR is green against its own base and the breakage exists only in the merge result. Identity is now (file, kind, value). New/removed/changed sites all still fail. The MULTISET comparison is preserved, so a shared identity must be discovered exactly as many times as it is registered. The scanner's positional pageSizeSiteId is untouched: pageSizeScan.test.ts asserts real AST positions against fixed fixtures, the opposite case, with no churn to remove. The one case this costs is stated rather than implied: a same-identity substitution within one file (delete a registered site, add a different unreviewed one with the same kind and value token) now passes. Narrow, and caught only incidentally before. Named in the guard and the record because "costs no coverage" is a claim that outlives whoever made it. Failure reports still print the discovered line:column -- identity and diagnostics need not share a format, and a bare id was useless in a file holding two such sites. Registry reconciled with #651: Playlists/RerunCollections lost their bounded windows to the shared searchLibraryPickerOptions, now registered in its place. Adds a 'search-bounded' class for that shape. Adds a 'deviation' class rather than laundering a live defect into a compliant-looking label. Reconciling the registry surfaced a §3b violation (#685, filed): CollectionsScreen's AddItemsDialog degrades to an unfiltered whole-type window on an empty query and surfaces nothing. Both existing labels would have been false, and either would have made the guard vouch for behaviour that does not exist. Deviation entries must name a tracking issue, enforced by a structural field -- a #\d+ scrape of the note passed with the reference deleted, because notes legitimately cite historical issues. Corrects SmartCollectionDialog to class-b: it does render a totalCount badge, which is class-b's defining evidence. fixes #684 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -179,6 +179,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `startup.parallel-orientation` | A fresh session runs two concurrent tracks at startup — Orientation (`AGENTS.md`/`CLAUDE.md` → `docs/README.md` task-signal map → the active decisions catalog `docs/decisions/README.md`) and, only when no issue is named, Selection (`scripts/select-queue.sh N`, deterministic live-Gitea ranking). A named issue skips Selection entirely. ersatztv#237, the closed pickup tracker this replaces, is reduced to a single archival breadcrumb and MUST NOT be read for live state. | 2026-07-21 | [link](records/startup/parallel-orientation.md) |
|
||||
| `testing.e2e-cleanup-scope-by-pid` | An E2E harness or agent may only kill processes whose PIDs it captured at launch — capture the PID; whoever owns the lifecycle releases it from a `trap ... EXIT INT TERM`. Never `pkill -f "dotnet ErsatzTV.dll"` (or any pattern that can match a process this run did not start). A foreign listener is reported, not reaped. | 2026-07-25 | [link](records/testing/e2e-cleanup-scope-by-pid.md) |
|
||||
| `testing.e2e-local-fresh-config-dir` | Always point `scripts/e2e-local.sh` at a fresh config dir — leftover channels/schedules/DB rows bleed state between runs and corrupt assertions. (The *readiness-probe hang* this record was originally written about was fixed in #533; the fresh-dir rule stands on state-bleed grounds alone.) | 2026-07-21 | [link](records/testing/e2e-local-fresh-config-dir.md) |
|
||||
| `testing.enumerating-guard-identity-not-position` | A guard that cross-checks a hand-reviewed registry against call sites discovered across the whole repo must key each entry on properties INTRINSIC to the site — file, kind, and the value source text — and never on its absolute line or column. A registry keyed on position is a function of every other file in the repo, so a branch that never touches the guard can invalidate it; and because each PR is green against its own base, that failure is structurally invisible pre-merge and lands on `main` after review and after the merge gate. Dropping the position keeps every mutation the guard exists for — a NEW site, a REMOVED site and a CHANGED value each still fail, since each changes the identity multiset — and costs exactly ONE case, which must be stated rather than implied: a SAME-IDENTITY SUBSTITUTION within one file (delete a registered site, add a different unreviewed one with the same kind and value token, net-zero count) now passes. A REPORTED failure still prints the discovered line:column, because identity and diagnostics need not share a format. Comparison stays a MULTISET count rather than set membership, so two sites in one file sharing an identity must be discovered exactly that many times and a third occurrence still fails. A SCANNER test that asserts real AST positions against FIXED inline fixtures is the opposite case and keeps its line/column identity — it has no churn, because its input does not move. | 2026-07-27 | [link](records/testing/enumerating-guard-identity-not-position.md) |
|
||||
| `testing.live-e2e-prepush-timing` | Run live-E2E via `scripts/e2e-local.sh` before pushing a write-path or UI change, and exercise download endpoints with curl, never a browser tab. | 2026-07-21 | [link](records/testing/live-e2e-prepush-timing.md) |
|
||||
| `testing.playwright-mcp-download-and-recovery` | In Playwright-MCP E2E, fetch file-download endpoints with curl — never a browser tab or `window.open` — and if browser tools stall repeatedly, `pkill -f ms-playwright-mcp` and drive a fresh session. | 2026-07-21 | [link](records/testing/playwright-mcp-download-and-recovery.md) |
|
||||
| `testing.scripted-playout-golden-deferred` | The `PlayoutBuildGoldenTests` in-memory golden net covers Sequential (YAML) as of #381. Scripted's *end-to-end pipeline* is excluded — `ScriptedPlayoutBuilder` runs a user-authored external program that drives the engine over HTTP loopback, which the in-memory harness can't pin — so that full-pipeline (integration) harness is deferred to #563. But the scheduling *behavior* those scripts drive lives entirely in the in-process `SchedulingEngine` (the `ScriptedScheduleController` is a 1:1 pass-through to it), which IS directly unit/golden-testable; the earlier "Scripted is un-golden-able by construction" framing overstated the constraint by conflating transport with engine. #395 extracts that shared switch to `ContentEnumeratorBuilder` and adds a direct regression net (`ContentEnumeratorBuilderTests`) over it. | 2026-07-22 | [link](records/testing/scripted-playout-golden-deferred.md) |
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
---
|
||||
key: testing.enumerating-guard-identity-not-position
|
||||
title: '2026-07-27 — an enumerating allow-list guard keys its registry on IDENTITY, never on a source position (#650, #651)'
|
||||
status: active
|
||||
since: '2026-07-27'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'A guard that cross-checks a hand-reviewed registry against call sites discovered across the whole repo must key each entry on properties INTRINSIC to the site — file, kind, and the value source text — and never on its absolute line or column. A registry keyed on position is a function of every other file in the repo, so a branch that never touches the guard can invalidate it; and because each PR is green against its own base, that failure is structurally invisible pre-merge and lands on `main` after review and after the merge gate. Dropping the position keeps every mutation the guard exists for — a NEW site, a REMOVED site and a CHANGED value each still fail, since each changes the identity multiset — and costs exactly ONE case, which must be stated rather than implied: a SAME-IDENTITY SUBSTITUTION within one file (delete a registered site, add a different unreviewed one with the same kind and value token, net-zero count) now passes. A REPORTED failure still prints the discovered line:column, because identity and diagnostics need not share a format. Comparison stays a MULTISET count rather than set membership, so two sites in one file sharing an identity must be discovered exactly that many times and a third occurrence still fails. A SCANNER test that asserts real AST positions against FIXED inline fixtures is the opposite case and keeps its line/column identity — it has no churn, because its input does not move.'
|
||||
signals: 'pageSize call-site guard · enumerating allow-list · registry went stale · UNREGISTERED and STALE report · line churn · line drift · semantic merge conflict · guard born red · registry stale on arrival · cancelled run hid a red · registry keyed on line:column · same-identity substitution residual gap · deviation classification · a registry must not launder a defect into a compliant label · multiset count not set membership · `pageSizeSiteId` vs `registryId` · scanner positions vs registry identity · paths: `web/src/api/pageSizeCallSites.guard.test.ts`, `web/src/api/pageSizeScan.ts`, `web/src/api/pageSizeScan.test.ts` · issues: #684, #650, #651, #676, #644'
|
||||
mechanics: 'Registry identity is `${file}:${kind}:${value}` (`registryId` in the guard); the scanner keeps `pageSizeSiteId` (`${line}:${column}:${kind}:${value}`) for `pageSizeScan.test.ts`.'
|
||||
---
|
||||
|
||||
The #650 guard enumerates every `pageSize` call site in the SPA and cross-checks it against a
|
||||
hand-reviewed registry in both directions. That part worked. Its follow-up review (F5/M-6) then made
|
||||
each entry's identity the site's absolute `line:column`, to distinguish two `pageSize` properties on
|
||||
one line. Sound about disambiguation, wrong about the cost.
|
||||
|
||||
**The guard was born red, and the sequence is the argument.** #651 moved `AutoTuneScreen.tsx` up ten
|
||||
lines and `FillerPresetsScreen.tsx` down seventy-two, and merged *before* the guard's own PR (#675).
|
||||
The registry, authored against a pre-#651 base, was stale the instant it landed. **A cancelled CI run
|
||||
is what let it through**: the guard's own merge run was cancelled, so nothing reported the red, which
|
||||
first surfaced on the next push (#676's merge — which touches no `web/src` file and is not the
|
||||
cause). `ci.cancelled-is-not-a-verdict`, paying out.
|
||||
|
||||
That is ONE ordering accident, not a recurring pattern; the honest count, because the argument needs
|
||||
no inflation. **The exposure is general anyway, because CI cannot see it coming.** Each PR is green
|
||||
against its own base, so the breakage exists only in the merge result and surfaces on `main` after
|
||||
review and after the merge gate.
|
||||
|
||||
**Identity should be what makes the site the thing being guarded.** A site's file, `kind` and value
|
||||
source text determine whether it is reviewed; where it sits in the file does not. The multiset
|
||||
comparison is what preserves what F5/M-6 was actually protecting and must not relax to set
|
||||
membership: `TrashScreen.tsx`'s two `PAGE_SIZE` requests must be discovered exactly twice, so a third
|
||||
occurrence still fails. The converse case is `pageSizeScan.test.ts`, which correctly KEEPS positions
|
||||
— verifying real AST positions is its subject and its fixtures cannot drift — which is why this
|
||||
introduced a separate `registryId` rather than changing `pageSizeSiteId` underneath it.
|
||||
|
||||
**What it costs, stated rather than implied.** A same-identity substitution inside one file now
|
||||
passes: delete a registered site, add a different unreviewed one with the same kind and value token,
|
||||
net-zero count. Narrow, and the old identity caught it only incidentally — it fired on every position
|
||||
change, so a reviewer conditioned to re-pin line numbers would likely have waved it through. Accepted
|
||||
knowingly and named in both places, because "costs no coverage" is the kind of claim that outlives
|
||||
whoever made it, and a guard described as exhaustive stops being re-examined.
|
||||
|
||||
**Diagnostics are not the identity.** Dropping position from the comparison key is the fix; dropping
|
||||
it from the failure *message* was collateral damage. The discovered direction prints `line:column`
|
||||
alongside each unregistered id — no churn, since positions appear only in an already-failing message.
|
||||
|
||||
**A registry must not launder a defect into a compliant-looking label.** Reconciling it surfaced a
|
||||
live §3b violation (#685): a picker degrading to an unfiltered whole-type window on an empty query,
|
||||
surfacing nothing. Both labels would have been false — `search-bounded` asserts a required query,
|
||||
`class-b` a rendered `totalCount` — and either would make the guard vouch for behaviour that does not
|
||||
exist. Hence a `deviation` class whose entries must name a tracking issue, enforced by a STRUCTURAL
|
||||
field rather than a `#\d+` scrape of the note: the first version of that test passed with the
|
||||
tracking reference deleted, because the note legitimately cited two historical issues.
|
||||
|
||||
The generalisation: **a guard whose input is the whole repository must not encode anything the whole
|
||||
repository can change without meaning to.** Position is the common instance; a line count, a file
|
||||
ordering or a byte offset would all fail the same way.
|
||||
@@ -1,5 +1,5 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { pageSizeSiteId, scanPageSizeSites } from './pageSizeScan';
|
||||
import { scanPageSizeSites } from './pageSizeScan';
|
||||
|
||||
/**
|
||||
* #650 guard: an ENUMERATING allow-list over every `pageSize` call site in the SPA.
|
||||
@@ -25,12 +25,43 @@ import { pageSizeSiteId, scanPageSizeSites } from './pageSizeScan';
|
||||
* sequential `expect` calls) — an early throw would otherwise hide the second direction's result
|
||||
* in the same run, understating what actually needs fixing.
|
||||
*
|
||||
* **Identity is per-OCCURRENCE** (line + column from the real AST position, #650 follow-up F5/M-6):
|
||||
* two `pageSize` properties on the same line (e.g. both branches of a ternary) get distinct
|
||||
* identities because each has its own token position. Comparison is by MULTISET count, not just
|
||||
* set membership, so an accidental duplicate registry entry — or, in principle, two structurally
|
||||
* identical sites colliding on identity — is still caught rather than one occurrence silently
|
||||
* covering both.
|
||||
* **Identity is `(file, kind, value)` — deliberately NOT line/column.** The original guard keyed
|
||||
* each site on its absolute `line:column` (#650 follow-up F5/M-6). That made the registry a
|
||||
* function of every OTHER file's line count, so a branch that never touches this guard can still
|
||||
* invalidate it. This guard was BORN RED, and the sequence is the whole argument (#684): #651
|
||||
* moved `AutoTuneScreen.tsx` up ten lines and `FillerPresetsScreen.tsx` down seventy-two, and
|
||||
* merged to `main` BEFORE this guard's own PR (#675) did — so the registry, authored against a
|
||||
* pre-#651 base, was stale the instant it landed. Its own merge run was CANCELLED, so nothing
|
||||
* reported it, and the red first surfaced on the NEXT push (#676's merge, which touches no
|
||||
* `web/src` file at all and is in no way the cause).
|
||||
*
|
||||
* That is one ordering accident, not a recurring two-merge pattern — but the exposure is the
|
||||
* general case, because it is structurally invisible pre-merge: every PR is green against its own
|
||||
* base, so the breakage exists only in the merge result and lands after review and after the merge
|
||||
* gate.
|
||||
*
|
||||
* A NEW call site, a REMOVED one, and a CHANGED `pageSize` value each still fail, because each
|
||||
* changes the `(file, kind, value)` multiset. What no longer fails is MOVING an unchanged site
|
||||
* within its own file — no truncation risk, and exactly the churn being removed.
|
||||
*
|
||||
* **The one real coverage case this costs, stated rather than implied** (#684 review M2): a
|
||||
* SAME-IDENTITY SUBSTITUTION inside one file — delete a registered site and add a different,
|
||||
* unreviewed one with the same `kind` and the same value TOKEN, keeping the count equal. Verified
|
||||
* to pass: deleting `TrashScreen.tsx`'s load-more `pageSize: PAGE_SIZE` and adding a whole-library
|
||||
* `getLibraryBrowseItems({ mediaType: 'Movie', pageSize: PAGE_SIZE })` is green. It is narrow (same
|
||||
* file, same kind, same token, net-zero count), and the old identity caught it only incidentally —
|
||||
* it fired on every position change, so a reviewer conditioned to re-pin line numbers would likely
|
||||
* have waved it through anyway. Accepted knowingly; do not describe this guard as exhaustive.
|
||||
*
|
||||
* **Comparison stays a MULTISET count, not set membership** (#650 follow-up M-6, preserved): two
|
||||
* sites in one file sharing an identifier (`TrashScreen.tsx`'s two `PAGE_SIZE` requests,
|
||||
* `paging.ts`'s two `loadAllPages` fetches) register as two entries and must be discovered twice.
|
||||
* So adding a third occurrence, or an accidental duplicate registry entry, is still caught rather
|
||||
* than one occurrence silently covering the others.
|
||||
*
|
||||
* The SCANNER's own positional identity (`pageSizeSiteId`, line:column) is unchanged and still
|
||||
* asserted by `pageSizeScan.test.ts` — verifying the compiler-API scan reports real AST positions
|
||||
* is that test's actual subject, and it runs against fixed inline fixtures, so it has no churn.
|
||||
*
|
||||
* `scanPageSizeSites` itself is verified against inline fixture source strings covering every
|
||||
* input class a text-level scanner previously got wrong (comment-in-string, template
|
||||
@@ -40,40 +71,91 @@ import { pageSizeSiteId, scanPageSizeSites } from './pageSizeScan';
|
||||
* itself, not just today's snapshot of call sites.
|
||||
*
|
||||
* Each registry entry classifies the site per `docs/spa-conventions.md` §3b /
|
||||
* `docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md`:
|
||||
* - 'class-a' — bounded-by-construction list, paged to completeness via `loadAllPages` (or,
|
||||
* for the two sites INSIDE `loadAllPages` itself, its implementation), with a
|
||||
* `complete`/`incomplete` flag surfaced (never silently partial).
|
||||
* - 'class-b' — media-library-scoped picker: one bounded page at (or under) the cap, with the
|
||||
* real truncation (`totalCount` vs items shown) surfaced to the user.
|
||||
* - 'paged-ui' — real paging UI (a page/"load more" control, or a user-adjustable page-size
|
||||
* selector, keyed to a genuine `totalCount`), so a `pageSize` at or below the
|
||||
* cap is correct as-is.
|
||||
* `spa.library-pickers-resolve-by-search` (#651). NOTE the record path: that key SUPERSEDED
|
||||
* `spa.list-completeness-vs-bounded-pickers`, whose record has since moved to
|
||||
* `docs/decisions/archive/spa/` — resolve it through `docs/decisions/README.md` by key, never by
|
||||
* the path a comment happens to name (the breadcrumb rule).
|
||||
* - 'class-a' — bounded-by-construction list, paged to completeness via `loadAllPages`
|
||||
* (or, for the two sites INSIDE `loadAllPages` itself, its
|
||||
* implementation), with a `complete`/`incomplete` flag surfaced (never
|
||||
* silently partial).
|
||||
* - 'search-bounded' — resolves by SEARCH and windows nothing: the typed query is the narrowing
|
||||
* mechanism, and the row bound is a property of the CODE rather than of a
|
||||
* caller's discipline (a clamp inside the shared helper for #651's
|
||||
* library picker; a fixed small constant at the inline preview sites).
|
||||
* Nothing is list-loaded, so there is no truncation to surface and the
|
||||
* absence of a truncation hint is correct — which is why such a site
|
||||
* cannot be filed under 'class-b', whose defining evidence IS a surfaced
|
||||
* `totalCount`. Applies only where the query is genuinely required: a
|
||||
* site that degrades to an unfiltered browse when the query is empty is
|
||||
* NOT search-bounded (see 'deviation').
|
||||
* - 'class-b' — one bounded page at (or under) the cap, with the real truncation
|
||||
* (`totalCount` vs items shown) surfaced to the user. Post-#651 this no
|
||||
* longer covers media-library pickers (those are 'search-bounded'). It
|
||||
* remains correct for exactly three shapes, which is the whole list: a
|
||||
* list bounded by its PARENT (`ChannelBuilder`, seasons of one show); the
|
||||
* collection-family types §3b explicitly excludes from search
|
||||
* (`FillerPresetsScreen`), which keep the bounded page and its hint; and
|
||||
* a preview over an already-bounded set (`AutoTuneScreen`'s channel
|
||||
* members). Each of the three renders a real `totalCount`-backed hint —
|
||||
* that render is the entry requirement, not the intent.
|
||||
* - 'paged-ui' — real paging UI (a page/"load more" control, or a user-adjustable
|
||||
* page-size selector, keyed to a genuine `totalCount`), so a `pageSize`
|
||||
* at or below the cap is correct as-is.
|
||||
* - 'deviation' — a KNOWN, TRACKED violation of §3b that this registry refuses to launder
|
||||
* into a compliant-looking label. A registry exists to state what is
|
||||
* true; recording a defect as 'class-b' or 'search-bounded' would make
|
||||
* the guard assert a hint or a query gate that demonstrably does not
|
||||
* exist, and the next reader would trust it. Every such entry MUST name
|
||||
* its tracking issue in the note, and flips to a real class only when the
|
||||
* behaviour is fixed.
|
||||
*
|
||||
* **Known residual gap:** object SPREAD (`getFoo({ ...opts })` where `opts` was built elsewhere
|
||||
* with an at-cap `pageSize`) and a `pageSize` passed as a bare POSITIONAL argument rather than an
|
||||
* object-literal property (`api/search.ts`'s `getAllSearchItemIds(query, pageNum, pageSize)`, the
|
||||
* api.search-allitems-paging precedent) are NOT resolvable by this scan — there is no `pageSize`
|
||||
* token inside an object-literal expression to find. Written down here, not silently absent: a
|
||||
* call site introduced through either path needs a human re-grep if that shape becomes common.
|
||||
* token inside an object-literal expression to find. A third, pre-existing gap (#684 review L2): a
|
||||
* `pageSize` whose value is a FORWARDED EXPRESSION rather than a literal or shorthand — e.g.
|
||||
* `api/collections.ts`'s `pageSize: String(pageSize)` — is a real object-literal property that
|
||||
* `scanPageSizeSites` still drops. Written down here, not silently absent: a call site introduced
|
||||
* through any of the three paths needs a human re-grep if that shape becomes common.
|
||||
*/
|
||||
|
||||
interface RegistryEntry {
|
||||
/** Path relative to `src/`, e.g. `api/paging.ts`. */
|
||||
file: string;
|
||||
line: number;
|
||||
column: number;
|
||||
kind: 'literal' | 'shorthand';
|
||||
/** The `pageSize` value's source text — an identifier (`PAGE_SIZE`) or a numeric literal. */
|
||||
value: string;
|
||||
classification: 'class-a' | 'class-b' | 'paged-ui';
|
||||
classification: 'class-a' | 'search-bounded' | 'class-b' | 'paged-ui' | 'deviation';
|
||||
/**
|
||||
* The Gitea issue tracking a 'deviation' — REQUIRED for that class and meaningless otherwise.
|
||||
* A dedicated field rather than a `#\d+` scrape of `note` (#684): notes legitimately cite
|
||||
* historical issues, so the regex passed even with the tracking reference deleted — a test
|
||||
* satisfiable by text that has nothing to do with what it claims to check.
|
||||
*/
|
||||
issue?: number;
|
||||
note: string;
|
||||
}
|
||||
|
||||
// Keep in file order, then position order, so a diff against the discovered set is easy to read.
|
||||
// Keep in file order, then in the order the sites appear within the file, so a diff against the
|
||||
// discovered set is easy to read. Two entries sharing a `(file, kind, value)` identity are
|
||||
// deliberate and load-bearing: the multiset comparison requires that site to be discovered exactly
|
||||
// twice (see the identity note above).
|
||||
const REGISTRY: RegistryEntry[] = [
|
||||
{
|
||||
file: 'api/libraryBrowse.ts',
|
||||
kind: 'literal',
|
||||
value: 'boundedPageSize',
|
||||
classification: 'search-bounded',
|
||||
note:
|
||||
"searchLibraryPickerOptions — the #651 shared media-library picker that REPLACED the bounded " +
|
||||
'windows previously registered for PlaylistsScreen and RerunCollectionsScreen (both now ' +
|
||||
'correctly absent). The bound is a clamp, not a default: Math.min(pageSize, ' +
|
||||
'LIBRARY_PICKER_RESULTS) inside the helper, so a caller cannot widen it.'
|
||||
},
|
||||
{
|
||||
file: 'api/paging.ts',
|
||||
line: 83,
|
||||
column: 62,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'class-a',
|
||||
@@ -83,30 +165,24 @@ const REGISTRY: RegistryEntry[] = [
|
||||
},
|
||||
{
|
||||
file: 'api/paging.ts',
|
||||
line: 93,
|
||||
column: 60,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'class-a',
|
||||
note: "loadAllPages's subsequent-page fetch inside the completeness loop; same helper as line 83."
|
||||
note: "loadAllPages's subsequent-page fetch inside the completeness loop; same helper as the entry above."
|
||||
},
|
||||
{
|
||||
file: 'builder/ChannelBuilder.tsx',
|
||||
line: 427,
|
||||
column: 79,
|
||||
kind: 'literal',
|
||||
value: '100',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
'SeasonsDialog: TelevisionSeason browse scoped to one show (parentId). No real show has ' +
|
||||
"100+ seasons, so this stays a single bounded page rather than paging to completeness — " +
|
||||
"but #650 found the response's totalCount went unread; it is now surfaced as a " +
|
||||
"'Showing the first N of M seasons' hint if a show somehow exceeds the cap."
|
||||
'SeasonsDialog: TelevisionSeason browse scoped to one show (parentId), so it is bounded by ' +
|
||||
'its PARENT rather than being a picker over the whole type — which is why #651 left it as a ' +
|
||||
"single bounded page. #650 found the response's totalCount went unread; it is now surfaced " +
|
||||
"as a 'Showing the first N of M seasons' hint if a show somehow exceeds the cap."
|
||||
},
|
||||
{
|
||||
file: 'builder/libraryBrowse.ts',
|
||||
line: 46,
|
||||
column: 98,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'paged-ui',
|
||||
@@ -117,8 +193,6 @@ const REGISTRY: RegistryEntry[] = [
|
||||
},
|
||||
{
|
||||
file: 'builder/libraryBrowse.ts',
|
||||
line: 73,
|
||||
column: 9,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'paged-ui',
|
||||
@@ -126,37 +200,36 @@ const REGISTRY: RegistryEntry[] = [
|
||||
},
|
||||
{
|
||||
file: 'builder/SmartCollectionDialog.tsx',
|
||||
line: 92,
|
||||
column: 52,
|
||||
kind: 'literal',
|
||||
value: '24',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
'Inline smart-query preview: a small bounded sample (well under the 100 cap) shown while ' +
|
||||
'authoring a query; the query itself is the narrowing mechanism, not paging.'
|
||||
'Inline smart-query preview while authoring a query. It DOES surface the real truncation — ' +
|
||||
"the response's totalCount is rendered as a `{count} matches` badge above a 12-row slice of " +
|
||||
'the 24 fetched — which is precisely what class-b requires, so it is not search-bounded ' +
|
||||
'despite being query-driven (#684 review M1: it was the counter-example to a claim that no ' +
|
||||
'such site renders a hint).'
|
||||
},
|
||||
{
|
||||
file: 'screens/AutoTuneScreen.tsx',
|
||||
line: 1527,
|
||||
column: 105,
|
||||
kind: 'literal',
|
||||
value: 'MEMBER_PREVIEW_SIZE',
|
||||
classification: 'class-b',
|
||||
note: "Channel-member preview; renders 'showing first N' once totalCount exceeds the preview size."
|
||||
note:
|
||||
"Channel-member preview: a real bounded window over the members, which is why it DOES render " +
|
||||
"'showing first N' once totalCount exceeds the preview size."
|
||||
},
|
||||
{
|
||||
file: 'screens/AutoTuneScreen.tsx',
|
||||
line: 1556,
|
||||
column: 67,
|
||||
kind: 'literal',
|
||||
value: 'ADD_SOURCE_RESULTS',
|
||||
classification: 'class-b',
|
||||
note: 'Tiny (8-row) debounced add-source search preview, far under the cap; query narrows results.'
|
||||
classification: 'search-bounded',
|
||||
note:
|
||||
'Tiny (8-row) debounced add-source search typeahead — the #440 picker whose compile-the-typed-' +
|
||||
'text rule #651 generalised. Nothing is windowed: a query narrows, and no hint is owed.'
|
||||
},
|
||||
{
|
||||
file: 'screens/BlockPlayoutTroubleshootingScreen.tsx',
|
||||
line: 161,
|
||||
column: 76,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'paged-ui',
|
||||
@@ -166,28 +239,34 @@ const REGISTRY: RegistryEntry[] = [
|
||||
},
|
||||
{
|
||||
file: 'screens/CollectionsScreen.tsx',
|
||||
line: 234,
|
||||
column: 58,
|
||||
kind: 'literal',
|
||||
value: '50',
|
||||
classification: 'class-b',
|
||||
classification: 'deviation',
|
||||
issue: 685,
|
||||
note:
|
||||
'AddItemsDialog per-kind library search preview (well under the 100 cap): a typed query ' +
|
||||
'narrows results; capped to a slice(0, 50) preview by design, not a truncated "whole list".'
|
||||
'TRACKED §3b VIOLATION — #685. AddItemsDialog.runSearch is reachable with an EMPTY query ' +
|
||||
'(blank form submit, and a kind-chip click, which calls it immediately), and ' +
|
||||
'getLibraryBrowseItems omits a falsy query — so it degrades to an unfiltered 50-row window ' +
|
||||
'over the whole media-library type, per kind. Nothing surfaces it: totalCount is never read ' +
|
||||
'here and no hint renders, and merged.slice(0, 50) drops up to 100 of 150 fetched rows even ' +
|
||||
'for a real query. Under the cap, so #644 and #650 both missed it. NOT search-bounded (the ' +
|
||||
'query is not required) and NOT class-b (no hint) — labelling it either would make this ' +
|
||||
'registry vouch for behaviour that does not exist.'
|
||||
},
|
||||
{
|
||||
file: 'screens/FillerPresetsScreen.tsx',
|
||||
line: 501,
|
||||
column: 67,
|
||||
kind: 'literal',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note: 'Media-library picker pattern shared with RerunCollectionsScreen, for filler preset content.'
|
||||
note:
|
||||
'The COLLECTION-FAMILY fallback (Collection / SmartCollection / MultiCollection / ' +
|
||||
'RerunCollection / Playlist), which spa-conventions §3b explicitly excludes from search ' +
|
||||
'because GetLibraryBrowseItemsHandler LIKE-matches `query` for those types and would match a ' +
|
||||
"compiled `title:*x*` literally. Keeps the bounded page AND its truncation hint; this " +
|
||||
"screen's media-item types went to searchLibraryPickerOptions in #651."
|
||||
},
|
||||
{
|
||||
file: 'screens/LogsScreen.tsx',
|
||||
line: 106,
|
||||
column: 32,
|
||||
kind: 'shorthand',
|
||||
value: 'pageSize',
|
||||
classification: 'paged-ui',
|
||||
@@ -197,8 +276,6 @@ const REGISTRY: RegistryEntry[] = [
|
||||
},
|
||||
{
|
||||
file: 'screens/MediaBrowseScreen.tsx',
|
||||
line: 119,
|
||||
column: 92,
|
||||
kind: 'literal',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
@@ -206,37 +283,13 @@ const REGISTRY: RegistryEntry[] = [
|
||||
},
|
||||
{
|
||||
file: 'screens/MediaDetailScreen.tsx',
|
||||
line: 267,
|
||||
column: 49,
|
||||
kind: 'literal',
|
||||
value: 'CHILD_PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Season/episode child list has a real page-number pager driven off the real totalCount.'
|
||||
},
|
||||
{
|
||||
file: 'screens/PlaylistsScreen.tsx',
|
||||
line: 193,
|
||||
column: 76,
|
||||
kind: 'literal',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note: 'Media-library picker pattern shared with RerunCollectionsScreen, for playlist entries.'
|
||||
},
|
||||
{
|
||||
file: 'screens/RerunCollectionsScreen.tsx',
|
||||
line: 146,
|
||||
column: 76,
|
||||
kind: 'literal',
|
||||
value: 'LIBRARY_BROWSE_PAGE_CAP',
|
||||
classification: 'class-b',
|
||||
note:
|
||||
'Media-library picker backing a rerun-collection <select>; single bounded page with a ' +
|
||||
"'Showing the first N of M — use search to narrow' hint (list-completeness-vs-bounded-pickers)."
|
||||
},
|
||||
{
|
||||
file: 'screens/SearchScreen.tsx',
|
||||
line: 118,
|
||||
column: 40,
|
||||
kind: 'literal',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
@@ -244,8 +297,6 @@ const REGISTRY: RegistryEntry[] = [
|
||||
},
|
||||
{
|
||||
file: 'screens/TrashScreen.tsx',
|
||||
line: 82,
|
||||
column: 44,
|
||||
kind: 'literal',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
@@ -253,12 +304,13 @@ const REGISTRY: RegistryEntry[] = [
|
||||
},
|
||||
{
|
||||
file: 'screens/TrashScreen.tsx',
|
||||
line: 113,
|
||||
column: 79,
|
||||
kind: 'literal',
|
||||
value: 'PAGE_SIZE',
|
||||
classification: 'paged-ui',
|
||||
note: 'Same per-group trash listing as line 82, the load-more request handler.'
|
||||
note:
|
||||
'The load-more request handler for the SAME per-group trash listing as the entry above — a ' +
|
||||
'deliberate second occurrence of one identity, which the multiset comparison requires to be ' +
|
||||
'discovered exactly twice.'
|
||||
}
|
||||
];
|
||||
|
||||
@@ -343,8 +395,32 @@ function discoverPageSizeCallSites(): DiscoveredSite[] {
|
||||
return sites;
|
||||
}
|
||||
|
||||
function fullId(site: { file: string; line: number; column: number; kind: string; value: string }): string {
|
||||
return `${site.file}:${pageSizeSiteId(site)}`;
|
||||
// The REGISTRY's identity: `(file, kind, value)`, with no source position — see the identity note
|
||||
// in this file's header for why the line/column were dropped. This is deliberately NOT
|
||||
// `pageSizeSiteId` (which keys on line:column and remains the SCANNER's identity, asserted over
|
||||
// fixed fixtures in `pageSizeScan.test.ts`); the two answer different questions, so they are
|
||||
// allowed to differ, and a registry entry has no position to supply anyway.
|
||||
function registryId(site: { file: string; kind: string; value: string }): string {
|
||||
return `${site.file}:${site.kind}:${site.value}`;
|
||||
}
|
||||
|
||||
// Identity and REPORT deliberately have different formats (#684 review M3). The comparison key
|
||||
// carries no position — that is the whole fix — but a bare `TrashScreen.tsx:literal:PAGE_SIZE` is
|
||||
// useless to whoever has to go find it in a file holding two such sites. So the UNREGISTERED
|
||||
// direction, which describes DISCOVERED sites and therefore does have real positions, prints them.
|
||||
// This reintroduces no churn: positions appear only in a failure message, never in a comparison.
|
||||
function describeDiscovered(sites: DiscoveredSite[], ids: string[]): string[] {
|
||||
const positions = new Map<string, string[]>();
|
||||
for (const site of sites) {
|
||||
const id = registryId(site);
|
||||
const at = positions.get(id) ?? [];
|
||||
at.push(`${site.line}:${site.column}`);
|
||||
positions.set(id, at);
|
||||
}
|
||||
return ids.map((id) => {
|
||||
const at = positions.get(id);
|
||||
return at && at.length > 0 ? `${id} (at ${at.join(', ')})` : id;
|
||||
});
|
||||
}
|
||||
|
||||
// Multiset (count per identity) comparison, not plain array `.includes` membership (#650
|
||||
@@ -419,8 +495,9 @@ describe('pageSize call-site guard (#650)', () => {
|
||||
});
|
||||
|
||||
it('matches the discovered pageSize call sites EXACTLY against the reviewed registry (not a non-empty check)', () => {
|
||||
const discoveredCounts = toCounts(discoverPageSizeCallSites().map(fullId));
|
||||
const registeredCounts = toCounts(REGISTRY.map(fullId));
|
||||
const discovered = discoverPageSizeCallSites();
|
||||
const discoveredCounts = toCounts(discovered.map(registryId));
|
||||
const registeredCounts = toCounts(REGISTRY.map(registryId));
|
||||
|
||||
const unregistered = multisetExcess(discoveredCounts, registeredCounts);
|
||||
const stale = multisetExcess(registeredCounts, discoveredCounts);
|
||||
@@ -431,7 +508,7 @@ describe('pageSize call-site guard (#650)', () => {
|
||||
if (unregistered.length > 0 || stale.length > 0) {
|
||||
const report = [
|
||||
`UNREGISTERED (${unregistered.length}) — discovered pageSize call site(s) missing from the REGISTRY above:`,
|
||||
...unregistered.map((id) => ` + ${id}`),
|
||||
...describeDiscovered(discovered, unregistered).map((line) => ` + ${line}`),
|
||||
`STALE (${stale.length}) — REGISTRY entries no longer found as a real pageSize call site:`,
|
||||
...stale.map((id) => ` - ${id}`)
|
||||
].join('\n');
|
||||
@@ -439,9 +516,48 @@ describe('pageSize call-site guard (#650)', () => {
|
||||
}
|
||||
});
|
||||
|
||||
// A 'deviation' entry is the registry admitting a live defect rather than laundering it into a
|
||||
// compliant-looking label (#684 review H2). That is only honest if the defect is TRACKED — an
|
||||
// untracked deviation is just a defect with better manners — so the issue reference is enforced
|
||||
// here rather than left to a reviewer noticing its absence.
|
||||
it("every 'deviation' entry names the issue tracking it", () => {
|
||||
const deviations = REGISTRY.filter((entry) => entry.classification === 'deviation');
|
||||
|
||||
// Anti-vacuity: if the deviations are ever all fixed, this must be deleted deliberately, not
|
||||
// silently pass over an empty list while claiming to enforce something.
|
||||
expect(deviations.length).toBeGreaterThan(0);
|
||||
|
||||
for (const entry of deviations) {
|
||||
expect(entry.issue, `${entry.file}:${entry.value} is a deviation but names no tracking issue`).toEqual(
|
||||
expect.any(Number)
|
||||
);
|
||||
expect(entry.issue!).toBeGreaterThan(0);
|
||||
}
|
||||
|
||||
// The converse, so the field cannot drift into decoration: only a deviation carries one.
|
||||
for (const entry of REGISTRY.filter((e) => e.classification !== 'deviation')) {
|
||||
expect(entry.issue, `${entry.file}:${entry.value} is not a deviation but carries an issue`).toBeUndefined();
|
||||
}
|
||||
});
|
||||
|
||||
// Pins the report format, not the comparison key (#684 review M3): dropping the position from
|
||||
// IDENTITY is the fix, dropping it from the failure MESSAGE was collateral damage — it left
|
||||
// `TrashScreen.tsx:literal:PAGE_SIZE` pointing at a file with two such sites.
|
||||
it('reports the discovered line:column for an unregistered site, while comparing without it', () => {
|
||||
const sites = discoverPageSizeCallSites();
|
||||
const target = sites.find((site) => site.file === 'screens/TrashScreen.tsx');
|
||||
expect(target).toBeDefined();
|
||||
|
||||
const described = describeDiscovered(sites, [registryId(target!)]);
|
||||
|
||||
expect(described[0]).toContain(`${target!.line}:${target!.column}`);
|
||||
// ...and the key it was looked up by still carries no position.
|
||||
expect(registryId(target!)).not.toContain(String(target!.line));
|
||||
});
|
||||
|
||||
it('every registry entry documents its class per docs/spa-conventions.md §3b', () => {
|
||||
for (const entry of REGISTRY) {
|
||||
expect(['class-a', 'class-b', 'paged-ui']).toContain(entry.classification);
|
||||
expect(['class-a', 'search-bounded', 'class-b', 'paged-ui', 'deviation']).toContain(entry.classification);
|
||||
expect(entry.note.length).toBeGreaterThan(20);
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user