timothy/ersatztv
1
0
Fork 0
Code Issues 51 Pull Requests 5 Actions Packages Projects Releases 9 Wiki Activity

fix(650): rewrite the pageSize guard on the TS compiler API; fix two append-ownership races

Browse Source 
Second cold cross-family (Codex, BLOCKED) re-review of b90f8a3b found the
regex/bracket-tracking guard scanner still defeated in five ways, and two new
High-severity races introduced by the F3/F4 fixes. Addressed as a further
follow-up (b90f8a3b left untouched).

GUARD REWRITE (per the review's explicit direction — stop patching the regex,
use the compiler):

- New `web/src/api/pageSizeScan.ts`: `scanPageSizeSites` parses each file with
  `ts.createSourceFile` and walks the real AST for `pageSize`
  PropertyAssignment/ShorthandPropertyAssignment nodes inside an
  ObjectLiteralExpression. This eliminates categorically (not case-by-case):
    - M-3: comments and string/template CONTENTS are never revisited as code,
      so a `'https://...'` string can't be misread as an unterminated string
      that swallows the rest of the file.
    - M-4: an object literal nested in a ternary, `??`, or JSX expression
      container is still found — the walk visits every descendant node
      regardless of the syntactic context above the ObjectLiteralExpression.
    - M-5: template-literal interpolations are real AST children, not opaque
      text.
    - L-7: a type literal (`type P = { pageSize: 100 }`), an interface
      PropertySignature, and a destructuring ObjectBindingPattern (parameter
      or nested) are structurally different node kinds from
      ObjectLiteralExpression — excluded by kind, not by a
      preceding-character heuristic a stray `{`/`(`/`,` could fool.
  `getLineAndCharacterOfPosition` gives exact line+column (fixes M-6 identity
  granularity) instead of the prior line-only identity.
- `pageSizeCallSites.guard.test.ts` now imports the shared scanner; identity
  is `file:line:column:kind:value`, compared as a MULTISET (count, not
  membership) in both directions.
- Both directions (unregistered / stale) are computed and folded into ONE
  thrown Error so a failure always shows the complete picture in one run,
  addressing the line-churn "second direction never renders" concern.
- New `pageSizeScan.test.ts`: a FIXTURE test (inline source strings, no repo
  scan) pinning the exact discovered set for every case the review named —
  comment-in-string, string containing the literal text `pageSize: 100`,
  template interpolation, ternary, `??`, JSX container, same-line duplicates,
  parameter/nested destructuring, a type literal, an interface property, a
  forwarded call expression, a React dependency array. This is what actually
  protects the scanner going forward — the guard test alone only ever proved
  today's snapshot of real call sites, never the scanner's handling of input
  classes it hadn't happened to encounter yet.
- Re-verified both original plants (a duplicate at-cap call in an
  already-registered file, and a new file with both a literal and a
  shorthand site) against the rewritten scanner; both still fail with the
  new combined-direction message. Also verified a run with BOTH directions
  simultaneously non-empty renders both in one report.

HIGH-1 (ChannelBuilder.tsx useLibraryBrowse, now web/src/builder/libraryBrowse.ts):
`reqId` identifies a query GENERATION, not an individual fetch — a page-0
refresh and a "Load more" append can be outstanding simultaneously under the
same reqId (query changes while an append is in flight for the new
generation). Whichever settled first used to clear `loadingMore`, letting a
second click fire an out-of-order/duplicate page fetch. Fixed with a
per-fetch `fetchId` plus a `loadingFetchIdRef`/`loadingFetchReqIdRef` pair:
only the fetch that OWNS the currently-displayed spinner can clear it; a
same-generation page-0 refresh leaves a same-generation append's spinner
alone, while a page-0 refresh for a NEW generation still retires an
abandoned OLDER-generation append's spinner (preserving the original #650 F3
fix). Reproduced the exact interleaving from the review in a new test
(gate B's page-0 and page-1 fetches independently, click "Load more" while
B's page-0 is still in flight) and confirmed it fails without the fix
(button re-enables while the append is still pending).

HIGH-2 (same file): the append-failure rollback mutated whatever
`pageRef.current` currently held, rather than the specific page THIS fetch
requested — under an overlapping-append race, a later page's success
followed by an earlier page's failure could roll the cursor back past
already-appended progress, corrupting a retry into refetching a duplicate.
Fixed with a compare-and-set guard (`if (pageRef.current === pageNum)`) so
the rollback only fires when nothing has advanced the cursor since. Since
this overlap is UI-unreachable once HIGH-1's single-flight disabling is
wired up (verified empirically: two synchronous fireEvent.click calls in RTL
only produce one request, since act() flushes the disabling render between
them), the regression test drives `useLibraryBrowse` directly via
`renderHook` (now exported) to force the exact interleaving and confirms it
fails without the fix (page 2 gets duplicated, page 3 never requested).

Extracted `useLibraryBrowse` (plus `loadCollections`/`loadLibraryItems`/
`BrowseState`/the media-type const arrays) into a new non-JSX module
`web/src/builder/libraryBrowse.ts` — exporting a hook from a .tsx file
tripped `react-refresh/only-export-components`; this also makes the hook
importable by `renderHook` without pulling in the whole screen component.

No server-side/C# change. Full local gate: lint clean, tsc clean, full
vitest run 110 files / 1051 tests passed (one LibrariesScreen.test.tsx
flake reproduced under full-suite parallel load, confirmed pre-existing and
unrelated — passes in isolation, never touched that file).
This commit is contained in:
timothy
2026-07-27 01:29:15 +02:00
parent 2d049e9a28
commit ca99bedb1a
6 changed files with 754 additions and 359 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web/src/api/pageSizeCallSites.guard.test.ts
+100 -189
View File
@@ -2,6 +2,7 @@ import { readFileSync, readdirSync } from 'node:fs';
import { dirname, join, relative } from 'node:path';
import { fileURLToPath } from 'node:url';
import { describe, expect, it } from 'vitest';
import { pageSizeSiteId, scanPageSizeSites } from './pageSizeScan';
/**
* #650 guard: an ENUMERATING allow-list over every `pageSize` call site in the SPA.
@@ -14,36 +15,32 @@ import { describe, expect, it } from 'vitest';
* just don't match a "pageSize above the cap" pattern.
*
* So this guard does NOT pattern-match on the pageSize VALUE (that repeats the #644 mistake for
* the next magic number). It enumerates every call site that passes a `pageSize` property into an
* object literal — either `pageSize: <literal-or-const>` or the ES6 shorthand `{ ..., pageSize }`
* — and cross-checks the discovered set against a hand-reviewed registry below, in BOTH
* directions:
* the next magic number). It enumerates every `pageSize` property inside a real object-literal
* expression via `scanPageSizeSites` (the TypeScript compiler API — see `pageSizeScan.ts`'s doc
* comment for why a hand-rolled text/regex scan was replaced) and cross-checks the discovered set
* against a hand-reviewed registry below, in BOTH directions:
* - a NEWLY discovered, unregistered site fails (a new call site was added without a documented
* classification — the exact way #650 could recur invisibly);
* - a REGISTERED site no longer discovered fails (the registry has gone stale — e.g. a site was
* removed or refactored to no longer pass a `pageSize` property, and the registry should
* shrink to match, not silently claim coverage of code that no longer exists).
* Both directions are computed and reported in a SINGLE combined failure message (not two
* 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, not per-file** (#650 follow-up F5): a second at-cap call added to
* an already-registered file must not hide behind that file's existing entry. Each discovered/
* registered site is keyed by `file:line:kind:value`, not `file:value` — two matches in the same
* file on different lines are two distinct identities, and a duplicate literal on a NEW line in an
* already-registered file is therefore an unregistered site like any other.
* **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.
*
* **Known residual gap (#650 follow-up F6):** this scan is text/bracket-based, not a real parser.
* It resolves the ES6 shorthand form (`{ ..., pageSize }`, distinguishing an object-literal `{`
* from a block-statement `{` and an array `[` by looking at the token immediately before the
* opening brace), but it CANNOT resolve:
* - a `pageSize` value carried through object SPREAD, e.g. `getFoo(makeAtCapParams())` where
* `makeAtCapParams` returns `{ pageSize: 100 }` from another module or `getFoo({ ...opts })`
* where `opts` was built elsewhere with an at-cap `pageSize` — the literal/shorthand is not
* textually present at the call site;
* - a `pageSize` passed as a bare POSITIONAL argument (`getSearchAllItems(query, pageNum,
* pageSize)`) rather than an object-literal property — `api/search.ts`'s
* `getAllSearchItemIds` (the api.search-allitems-paging precedent) does this and is
* deliberately out of this guard's reach.
* These are written down here, not silently absent: a call site introduced through either path
* would not be caught by this guard and needs a human re-grep if that shape becomes common.
* `scanPageSizeSites` itself is verified against inline fixture source strings covering every
* input class a text-level scanner previously got wrong (comment-in-string, template
* interpolation, ternary, `??`, JSX container, same-line duplicates, parameter/nested
* destructuring, a type literal, a string containing the text `pageSize: 100`) in
* `pageSizeScan.test.ts` — that test does not depend on the real repo, so it protects the SCANNER
* 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`:
@@ -55,22 +52,31 @@ import { describe, expect, it } from 'vitest';
* - '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.
*
* **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.
*/
interface RegistryEntry {
file: string;
line: number;
column: number;
kind: 'literal' | 'shorthand';
value: string;
classification: 'class-a' | 'class-b' | 'paged-ui';
note: string;
}
// Keep in file order, then line order, so a diff against the discovered set is easy to read.
// Keep in file order, then position order, so a diff against the discovered set is easy to read.
const REGISTRY: RegistryEntry[] = [
{
file: 'api/paging.ts',
line: 83,
column: 62,
kind: 'shorthand',
value: 'pageSize',
classification: 'class-a',
@@ -81,6 +87,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'api/paging.ts',
line: 93,
column: 60,
kind: 'shorthand',
value: 'pageSize',
classification: 'class-a',
@@ -88,7 +95,8 @@ const REGISTRY: RegistryEntry[] = [
},
{
file: 'builder/ChannelBuilder.tsx',
line: 447,
line: 427,
column: 79,
kind: 'literal',
value: '100',
classification: 'class-b',
@@ -99,8 +107,9 @@ const REGISTRY: RegistryEntry[] = [
"'Showing the first N of M seasons' hint if a show somehow exceeds the cap."
},
{
file: 'builder/ChannelBuilder.tsx',
line: 539,
file: 'builder/libraryBrowse.ts',
line: 50,
column: 98,
kind: 'shorthand',
value: 'pageSize',
classification: 'paged-ui',
@@ -110,8 +119,9 @@ const REGISTRY: RegistryEntry[] = [
'is meaningful — this is the paged-ui replacement for the original truncating implementation.'
},
{
file: 'builder/ChannelBuilder.tsx',
line: 566,
file: 'builder/libraryBrowse.ts',
line: 77,
column: 9,
kind: 'shorthand',
value: 'pageSize',
classification: 'paged-ui',
@@ -120,6 +130,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'builder/SmartCollectionDialog.tsx',
line: 92,
column: 52,
kind: 'literal',
value: '24',
classification: 'class-b',
@@ -130,6 +141,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/AutoTuneScreen.tsx',
line: 1527,
column: 105,
kind: 'literal',
value: 'MEMBER_PREVIEW_SIZE',
classification: 'class-b',
@@ -138,6 +150,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/AutoTuneScreen.tsx',
line: 1556,
column: 67,
kind: 'literal',
value: 'ADD_SOURCE_RESULTS',
classification: 'class-b',
@@ -146,6 +159,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/BlockPlayoutTroubleshootingScreen.tsx',
line: 161,
column: 76,
kind: 'shorthand',
value: 'pageSize',
classification: 'paged-ui',
@@ -156,6 +170,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/CollectionsScreen.tsx',
line: 234,
column: 58,
kind: 'literal',
value: '50',
classification: 'class-b',
@@ -166,6 +181,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/FillerPresetsScreen.tsx',
line: 501,
column: 67,
kind: 'literal',
value: 'LIBRARY_BROWSE_PAGE_CAP',
classification: 'class-b',
@@ -174,6 +190,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/LogsScreen.tsx',
line: 106,
column: 32,
kind: 'shorthand',
value: 'pageSize',
classification: 'paged-ui',
@@ -184,6 +201,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/MediaBrowseScreen.tsx',
line: 119,
column: 92,
kind: 'literal',
value: 'PAGE_SIZE',
classification: 'paged-ui',
@@ -192,6 +210,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/MediaDetailScreen.tsx',
line: 267,
column: 49,
kind: 'literal',
value: 'CHILD_PAGE_SIZE',
classification: 'paged-ui',
@@ -200,6 +219,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/PlaylistsScreen.tsx',
line: 193,
column: 76,
kind: 'literal',
value: 'LIBRARY_BROWSE_PAGE_CAP',
classification: 'class-b',
@@ -208,6 +228,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/RerunCollectionsScreen.tsx',
line: 146,
column: 76,
kind: 'literal',
value: 'LIBRARY_BROWSE_PAGE_CAP',
classification: 'class-b',
@@ -218,6 +239,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/SearchScreen.tsx',
line: 118,
column: 40,
kind: 'literal',
value: 'PAGE_SIZE',
classification: 'paged-ui',
@@ -226,6 +248,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/TrashScreen.tsx',
line: 82,
column: 44,
kind: 'literal',
value: 'PAGE_SIZE',
classification: 'paged-ui',
@@ -234,6 +257,7 @@ const REGISTRY: RegistryEntry[] = [
{
file: 'screens/TrashScreen.tsx',
line: 113,
column: 79,
kind: 'literal',
value: 'PAGE_SIZE',
classification: 'paged-ui',
@@ -244,15 +268,6 @@ const REGISTRY: RegistryEntry[] = [
// This file lives in `src/api/`; the scan root is `src/`.
const SRC_DIR = join(dirname(fileURLToPath(import.meta.url)), '..');
// Matches an object-literal `pageSize:` key followed by either a decimal literal or an
// identifier — NOT a type annotation (`pageSize: number`) and NOT a forwarded call expression
// (`pageSize: String(pageSize)`, a dynamic passthrough of a caller-supplied value, not a literal).
const PAGE_SIZE_LITERAL = /\bpageSize:\s*(\d+|[A-Za-z_][A-Za-z0-9_]*)\b/g;
const TYPE_ANNOTATION_VALUES = new Set(['number', 'string', 'boolean', 'undefined', 'null', 'any', 'unknown']);
// A `{` is treated as opening an object LITERAL (vs a block statement) only when the token
// immediately before it puts us in expression position.
const OBJECT_CONTEXT_PRECEDERS = new Set(['(', ',', '[', ':', '=']);
function listSourceFiles(dir: string): string[] {
const out: string[] = [];
for (const entry of readdirSync(dir, { withFileTypes: true })) {
@@ -271,129 +286,10 @@ function listSourceFiles(dir: string): string[] {
return out;
}
// Strips block comments (preserving newlines, so line numbers stay accurate) and line comments.
// Block comments must go first: a JSDoc block routinely contains an ODD mid-comment backtick (an
// inline-code marker like `` `pageSize` ``) that would otherwise desync the quote-tracking bracket
// scan below across the rest of the file.
function stripComments(text: string): string {
const noBlockComments = text.replace(/\/\*[\s\S]*?\*\//g, (block) => block.replace(/[^\n]/g, ' '));
return noBlockComments
.split('\n')
.map((line) => {
const idx = line.indexOf('//');
return idx >= 0 ? line.slice(0, idx) : line;
})
.join('\n');
}
function lineNumberAt(text: string, index: number): number {
let line = 1;
for (let i = 0; i < index; i++) {
if (text[i] === '\n') {
line++;
}
}
return line;
}
// Finds every ES6 shorthand `pageSize` property (bare `pageSize`, no `:`) that sits directly
// inside an object-literal `{ ... }` — not a block statement, not an array, not a destructuring
// default array (`const [pageSize = 100] = ...`), not a string/template literal. Bracket- and
// quote-tracked rather than a single regex, because a naive "preceded by `{` or `,`" check cannot
// tell `getFoo({ pageNum, pageSize })` (an object literal — a real call-site risk) apart from
// `useCallback(fn, [pageNum, pageSize])` (a React dependency array — not a call site at all); both
// have `pageSize` immediately preceded by a comma.
function findShorthandIndices(codeText: string): number[] {
const stack: Array<{ ch: '{' | '[' | '('; isObjectContext?: boolean }> = [];
const results: number[] = [];
const len = codeText.length;
let i = 0;
while (i < len) {
const ch = codeText[i];
if (ch === '{') {
let p = i - 1;
while (p >= 0 && /\s/.test(codeText[p])) {
p--;
}
let isObjectContext = OBJECT_CONTEXT_PRECEDERS.has(codeText[p]);
if (!isObjectContext && p >= 0) {
const wordEnd = p + 1;
let wordStart = wordEnd;
while (wordStart > 0 && /[A-Za-z0-9_]/.test(codeText[wordStart - 1])) {
wordStart--;
}
isObjectContext = codeText.slice(wordStart, wordEnd) === 'return';
}
stack.push({ ch, isObjectContext });
i++;
continue;
}
if (ch === '[' || ch === '(') {
stack.push({ ch: ch as '[' | '(' });
i++;
continue;
}
if (ch === '}' || ch === ']' || ch === ')') {
stack.pop();
i++;
continue;
}
if (ch === '"' || ch === "'" || ch === '`') {
const quote = ch;
i++;
while (i < len && codeText[i] !== quote) {
if (codeText[i] === '\\') {
i++;
}
i++;
}
i++;
continue;
}
if (/[A-Za-z_]/.test(ch)) {
let j = i;
while (j < len && /[A-Za-z0-9_]/.test(codeText[j])) {
j++;
}
const word = codeText.slice(i, j);
if (word === 'pageSize') {
let k = j;
while (k < len && /\s/.test(codeText[k])) {
k++;
}
// Skip an optional `?` (a TS optional-property declaration, e.g. `pageSize?: number`) so
// the colon check below still correctly excludes it.
if (codeText[k] === '?') {
k++;
while (k < len && /\s/.test(codeText[k])) {
k++;
}
}
const isColonForm = codeText[k] === ':';
const enclosing = stack[stack.length - 1];
if (!isColonForm && enclosing && enclosing.ch === '{' && enclosing.isObjectContext) {
results.push(i);
}
}
i = j;
continue;
}
i++;
}
return results;
}
interface DiscoveredSite {
file: string;
line: number;
column: number;
kind: 'literal' | 'shorthand';
value: string;
}
@@ -404,32 +300,43 @@ function discoverPageSizeCallSites(): DiscoveredSite[] {
for (const absPath of files) {
const relPath = relative(SRC_DIR, absPath);
const codeText = stripComments(readFileSync(absPath, 'utf8'));
const text = readFileSync(absPath, 'utf8');
for (const match of codeText.matchAll(PAGE_SIZE_LITERAL)) {
const value = match[1];
if (TYPE_ANNOTATION_VALUES.has(value)) {
continue;
}
// A forwarded call expression, e.g. `pageSize: String(pageSize)` — the character right
// after the matched identifier is `(`, i.e. this is a function call, not a literal/const.
const afterIndex = match.index! + match[0].length;
if (codeText[afterIndex] === '(') {
continue;
}
sites.push({ file: relPath, line: lineNumberAt(codeText, match.index!), kind: 'literal', value });
}
for (const idx of findShorthandIndices(codeText)) {
sites.push({ file: relPath, line: lineNumberAt(codeText, idx), kind: 'shorthand', value: 'pageSize' });
for (const site of scanPageSizeSites(text, absPath)) {
sites.push({ file: relPath, ...site });
}
}
return sites;
}
function siteId(site: { file: string; line: number; kind: string; value: string }): string {
return `${site.file}:${site.line}:${site.kind}:${site.value}`;
function fullId(site: { file: string; line: number; column: number; kind: string; value: string }): string {
return `${site.file}:${pageSizeSiteId(site)}`;
}
// Multiset (count per identity) comparison, not plain array `.includes` membership (#650
// follow-up M-6) — so a registry that accidentally lists the same identity twice, or a future
// scanner change that could (in principle) emit a duplicate, is still caught rather than one
// occurrence silently covering both.
function toCounts(ids: string[]): Map<string, number> {
const counts = new Map<string, number>();
for (const id of ids) {
counts.set(id, (counts.get(id) ?? 0) + 1);
}
return counts;
}
// Returns entries present in `left` more times than in `right`, expanded per the excess count —
// e.g. a `left` id appearing 3 times against 1 in `right` yields that id listed twice.
function multisetExcess(left: Map<string, number>, right: Map<string, number>): string[] {
const excess: string[] = [];
for (const [id, count] of left) {
const remaining = count - (right.get(id) ?? 0);
for (let i = 0; i < remaining; i++) {
excess.push(id);
}
}
return excess.sort();
}
describe('pageSize call-site guard (#650)', () => {
@@ -444,20 +351,24 @@ describe('pageSize call-site guard (#650)', () => {
});
it('matches the discovered pageSize call sites EXACTLY against the reviewed registry (not a non-empty check)', () => {
const discovered = discoverPageSizeCallSites().map(siteId).sort();
const registered = REGISTRY.map(siteId).sort();
const discoveredCounts = toCounts(discoverPageSizeCallSites().map(fullId));
const registeredCounts = toCounts(REGISTRY.map(fullId));
// Two separate assertions rather than one set-equality check, so a failure message names
// exactly which side is wrong: an unregistered NEW site (a defect risk) vs a stale registry
// entry that no longer matches anything in the source (a maintenance smell), never both
// collapsed into an opaque "sets differ" message. Identity is per-occurrence (file:line:kind:
// value, #650 follow-up F5) — a second at-cap call added to an already-registered FILE is a
// distinct, unregistered identity, not silently absorbed by that file's existing entry.
const unregistered = discovered.filter((site) => !registered.includes(site));
const stale = registered.filter((site) => !discovered.includes(site));
const unregistered = multisetExcess(discoveredCounts, registeredCounts);
const stale = multisetExcess(registeredCounts, discoveredCounts);
expect(unregistered, 'discovered pageSize call site(s) missing from the REGISTRY above').toEqual([]);
expect(stale, 'REGISTRY entries no longer found as a real pageSize call site').toEqual([]);
// Both directions are folded into ONE assertion so a failure always shows the complete
// picture in a single run (#650 follow-up, line-churn concern) — two sequential `expect`
// calls would throw on the first failing direction and never evaluate/report the second.
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}`),
`STALE (${stale.length}) — REGISTRY entries no longer found as a real pageSize call site:`,
...stale.map((id) => ` - ${id}`)
].join('\n');
throw new Error(report);
}
});
it('every registry entry documents its class per docs/spa-conventions.md §3b', () => {
web/src/api/pageSizeScan.test.ts
+160
View File
@@ -0,0 +1,160 @@
import { describe, expect, it } from 'vitest';
import { pageSizeSiteId, scanPageSizeSites, type PageSizeSite } from './pageSizeScan';
/**
* Fixture test for `scanPageSizeSites` itself — NOT a scan of the real repo (that's
* `pageSizeCallSites.guard.test.ts`). This is what actually protects the SCANNER going forward:
* a prior hand-rolled regex/bracket-tracking version passed the guard test against unmodified
* source at both #650 commits while still being defeated by every case below, because the guard
* only ever exercised today's snapshot of real call sites — it never proved the scanner handles
* the INPUT CLASSES that expose a text-level scanner's blind spots. Pinning the exact discovered
* set against synthetic source strings closes that gap.
*/
function ids(sites: PageSizeSite[]): string[] {
return sites.map(pageSizeSiteId);
}
describe('scanPageSizeSites', () => {
it('finds a literal pageSize: property in a plain object-literal call argument', () => {
const source = `getFoo({ pageSize: 100, query });\n`;
const sites = scanPageSizeSites(source, 'fixture.ts');
expect(ids(sites)).toEqual(['1:10:literal:100']);
});
it('finds the ES6 shorthand pageSize property in a plain object-literal call argument', () => {
const source = `getFoo({ pageSize, query });\n`;
const sites = scanPageSizeSites(source, 'fixture.ts');
expect(ids(sites)).toEqual(['1:10:shorthand:pageSize']);
});
it('is NOT fooled by a "//" inside a string literal (M-3)', () => {
// A naive text scanner that treats every `//` as a comment start turns this into an
// unterminated string, then swallows the REAL call site below it.
const source = [`const endpoint = 'https://example.test';`, `getFoo({ pageSize: 100 });`, ''].join('\n');
const sites = scanPageSizeSites(source, 'fixture.ts');
expect(ids(sites)).toEqual(['2:10:literal:100']);
});
it('does NOT match a string literal that merely CONTAINS the text "pageSize: 100" (L-7)', () => {
const source = `const label = "pageSize: 100";\n`;
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('finds an object literal passed inside a template-literal interpolation (M-5)', () => {
const source = 'const url = `${await getFoo({ pageSize })}`;\n';
const sites = scanPageSizeSites(source, 'fixture.ts');
expect(ids(sites)).toEqual(['1:31:shorthand:pageSize']);
});
it('finds an object literal in each branch of a ternary, even on the SAME line (M-4, M-6)', () => {
const source = `return ok ? getA({ pageSize }) : getB({ pageSize });\n`;
const sites = scanPageSizeSites(source, 'fixture.ts');
// Two distinct occurrences on one line get distinct identities (different columns) — a
// single registry entry cannot silently cover both.
expect(ids(sites)).toEqual(['1:20:shorthand:pageSize', '1:41:shorthand:pageSize']);
expect(sites[0].column).not.toBe(sites[1].column);
});
it('finds an object literal on the right-hand side of ?? (M-4)', () => {
const source = `getFoo(options ?? { pageSize: 50 });\n`;
const sites = scanPageSizeSites(source, 'fixture.ts');
expect(ids(sites)).toEqual(['1:21:literal:50']);
});
it('finds an object literal inside a JSX expression container attribute (M-4)', () => {
const source = `const el = <Component options={{ pageSize }} />;\n`;
const sites = scanPageSizeSites(source, 'fixture.tsx');
expect(ids(sites)).toEqual(['1:34:shorthand:pageSize']);
});
it('does NOT match a parameter destructuring pattern (L-7)', () => {
const source = `function f({ pageSize }: { pageSize: number }) {}\n`;
// The destructured PARAMETER `{ pageSize }` is an ObjectBindingPattern, not an
// ObjectLiteralExpression — excluded by node kind. Its TYPE annotation `{ pageSize: number }`
// is a TypeLiteral (PropertySignature), also excluded by node kind — never an object literal.
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('does NOT match a nested destructuring pattern (L-7)', () => {
const source = `const { nested: { pageSize } } = input;\n`;
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('does NOT match a type-literal declaration (L-7)', () => {
const source = `type P = { pageSize: 100 };\n`;
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('does NOT match an interface property declaration', () => {
const source = `interface Params {\n pageSize?: number;\n}\n`;
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('does NOT match a forwarded call expression (a dynamic passthrough, not a fixed value)', () => {
const source = `getFoo({ pageSize: String(pageSize) });\n`;
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('is not confused by a pageSize reference inside a comment', () => {
const source = [`// pageSize: 999 — this is just prose, not code`, `getFoo({ query });`, ''].join('\n');
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('is not confused by a pageSize reference inside a block/JSDoc comment', () => {
const source = ['/**', ' * Uses `pageSize` under the hood — see also `{ pageSize: 100 }`.', ' */', 'getFoo({ query });', ''].join(
'\n'
);
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('does NOT match a React dependency array containing pageSize', () => {
const source = `useCallback(load, [pageNum, pageSize, sortField]);\n`;
expect(scanPageSizeSites(source, 'fixture.ts')).toEqual([]);
});
it('finds a const-identifier literal value (not just a numeric literal)', () => {
const source = `getFoo({ pageSize: LIBRARY_BROWSE_PAGE_CAP });\n`;
const sites = scanPageSizeSites(source, 'fixture.ts');
expect(ids(sites)).toEqual(['1:10:literal:LIBRARY_BROWSE_PAGE_CAP']);
});
it('covers every case above together in one multi-line fixture and pins the exact discovered set', () => {
const source = [
`const endpoint = 'https://example.test';`, // M-3: not a comment
`const label = "pageSize: 100";`, // L-7: string contents, not code
`// pageSize: 999 in a line comment`, // not code
`/** block comment mentioning \`pageSize\` */`, // not code
`type P = { pageSize: 100 };`, // L-7: type literal, not a value
`interface Q { pageSize?: number; }`, // not a value
`function f({ pageSize }: { pageSize: number }) {}`, // L-7: destructuring + its type
`const { nested: { pageSize } } = input;`, // L-7: nested destructuring
`useCallback(load, [pageNum, pageSize]);`, // dependency array, not an object literal
`getFoo({ pageSize: String(pageSize) });`, // forwarded call, not a fixed value
`getFoo({ pageSize: 100 });`, // REAL: literal
`getBar({ pageSize });`, // REAL: shorthand
`getBaz(options ?? { pageSize: 50 });`, // REAL: ?? context (M-4)
`const url = \`\${await getQux({ pageSize })}\`;`, // REAL: template interpolation (M-5)
`return ok ? getA({ pageSize }) : getB({ pageSize });` // REAL x2: ternary, same line (M-4/M-6)
].join('\n');
const sites = scanPageSizeSites(source, 'fixture.ts');
expect(ids(sites)).toEqual([
'11:10:literal:100',
'12:10:shorthand:pageSize',
'13:21:literal:50',
'14:31:shorthand:pageSize',
'15:20:shorthand:pageSize',
'15:41:shorthand:pageSize'
]);
// Anti-vacuity: the fixture packs in 10 non-matching traps ahead of the 6 real sites — a
// scanner that matched everything (or nothing) would fail this count, not just the ids above.
expect(sites.length).toBe(6);
});
it('scans .tsx source using the TSX script kind (JSX does not parse under plain .ts rules)', () => {
const source = `export function C() {\n return <div data={{ pageSize: 10 }} />;\n}\n`;
const sites = scanPageSizeSites(source, 'fixture.tsx');
expect(ids(sites)).toEqual(['2:23:literal:10']);
});
});
web/src/api/pageSizeScan.ts
+83
View File
@@ -0,0 +1,83 @@
import * as ts from 'typescript';
/**
* #650 follow-up: an AST-based scanner for every `pageSize` property that appears inside a real
* object LITERAL expression. Extracted into its own module so both the enumerating guard
* (`pageSizeCallSites.guard.test.ts`, which scans the real repo) and a fixture test
* (`pageSizeScan.test.ts`, which scans synthetic source strings and does NOT touch the repo) can
* exercise the exact same scanning logic.
*
* A prior hand-rolled regex/bracket-tracking version of this scan was replaced after a review
* found it defeated by comments-in-strings, template-literal interpolations, ternary/`??`
* contexts, JSX containers, and same-line duplicates — each a DIFFERENT input class a text-level
* lexer has to special-case one at a time. The TypeScript compiler API sidesteps the whole
* category: comments and string/template CONTENTS are trivia/literal text the parser never
* revisits as code, and a real object-literal expression (`ObjectLiteralExpression`) is a
* structurally different AST node from a type literal (`type X = { pageSize: number }`,
* `PropertySignature` inside a `TypeLiteralNode`/`InterfaceDeclaration`) or a destructuring
* pattern (`ObjectBindingPattern`, e.g. `function f({ pageSize }) {}` or
* `const { pageSize } = x`) — so those are excluded by NODE KIND, not by a preceding-character
* heuristic that can be fooled by an unrelated `{`/`(`/`,`.
*/
export interface PageSizeSite {
/** 1-based source line of the `pageSize` property (name), matching editor line numbers. */
line: number;
/** 1-based source column of the `pageSize` property (name). */
column: number;
kind: 'literal' | 'shorthand';
/**
* For `kind: 'literal'`: the numeric-literal text or the referenced const identifier's name.
* For `kind: 'shorthand'`: always the literal string `'pageSize'` (the shorthand form only ever
* forwards whatever `pageSize` binding is in scope — there is no separate "value" to name).
*/
value: string;
}
function scriptKindFor(fileName: string): ts.ScriptKind {
return fileName.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS;
}
export function scanPageSizeSites(sourceText: string, fileName: string): PageSizeSite[] {
const sourceFile = ts.createSourceFile(fileName, sourceText, ts.ScriptTarget.Latest, true, scriptKindFor(fileName));
const sites: PageSizeSite[] = [];
function positionOf(node: ts.Node): { line: number; column: number } {
const { line, character } = sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile));
return { line: line + 1, column: character + 1 };
}
function visit(node: ts.Node): void {
if (ts.isObjectLiteralExpression(node)) {
for (const property of node.properties) {
if (ts.isPropertyAssignment(property) && ts.isIdentifier(property.name) && property.name.text === 'pageSize') {
const initializer = property.initializer;
// Only a numeric literal or a bare identifier (a const/variable reference) counts as a
// fixed value baked into THIS call site. A forwarded expression — `String(pageSize)`, a
// ternary, a template, a function call — is a dynamic passthrough of whatever the
// caller supplied, not a literal this site chose; it is deliberately not recorded here
// (see the module doc comment on `loadAllPages`/positional-argument residual gaps).
if (ts.isNumericLiteral(initializer)) {
const { line, column } = positionOf(property.name);
sites.push({ line, column, kind: 'literal', value: initializer.text });
} else if (ts.isIdentifier(initializer)) {
const { line, column } = positionOf(property.name);
sites.push({ line, column, kind: 'literal', value: initializer.text });
}
} else if (ts.isShorthandPropertyAssignment(property) && property.name.text === 'pageSize') {
const { line, column } = positionOf(property.name);
sites.push({ line, column, kind: 'shorthand', value: 'pageSize' });
}
}
}
ts.forEachChild(node, visit);
}
visit(sourceFile);
return sites.sort((a, b) => (a.line === b.line ? a.column - b.column : a.line - b.line));
}
export function pageSizeSiteId(site: { line: number; column: number; kind: string; value: string }): string {
return `${site.line}:${site.column}:${site.kind}:${site.value}`;
}