Third cold cross-family review (BLOCKED) found the append/page-0-refresh races were being fixed one interleaving at a time — round 1 fixed page-0-settles-first, round 2's compare-and-set rollback fixed the duplicate-append case but introduced a permanently-skipped page, and the reviewer found the exact mirror of round 1's fix (page-1-settles-first, erasing page 1 with no cursor reset). Direction from the review: stop enumerating orderings, make the overlap structurally impossible. SINGLE-FLIGHT (web/src/builder/libraryBrowse.ts): a new `busyRef` guard is true from the moment ANY fetch (a page-0 refresh OR an append) for the current query generation is issued until it settles. `loadMore` checks it SYNCHRONOUSLY and returns immediately (ignored, not queued) if a fetch is already in flight — including a page-0 refresh, not just a prior append, so a "Load more" click that lands while a query change is still resolving is a no-op rather than starting a second, overlapping request. With overlapping fetches eliminated by construction, the append-failure rollback no longer needs the round-2 compare-and-set: single-flight guarantees nothing else could have moved `pageRef` since a given fetch started, so it now always rolls back and retries the exact page that failed, unconditionally. Visual feedback (the button showing loading/disabled during a page-0 refresh, not just an append) is set via `queueMicrotask(() => setLoadingMore (true))` rather than a bare synchronous call in the generation-change effect — `react-hooks/set-state-in-effect` flags the latter; a microtask-deferred call resolves before any human-perceptible input, satisfies the lint rule (the same reason `.then()` callbacks elsewhwere in this hook aren't flagged), and keeps the actual correctness guarantee (the ref check) perfectly synchronous regardless. TESTS REWRITTEN, not just added — the round-2 "HIGH-2" hook test explicitly asserted the NEXT request after a failed page 1 (following an overlapping page 2 success) should be page 3, i.e. it blessed page 1's permanent loss. Replaced with two hook-level tests: single-flight ignores a synchronous double `loadMore()` call (only one fetch issued), and a failed page is retried as the SAME page number. Replaced the round-2 component-level "HIGH-1" test (which drove the now-impossible overlap through the DOM) with one asserting the click during a pending page-0 refresh is ignored, and that once free, the correct page-1-then-page-2 sequence completes with both pages' rows present. Verified all three new/rewritten tests against the prior committed hook (7b1ae48b0): the two single-flight-specific tests fail as expected (`[0, 1]` requested when only `[0]` should have been); the retry-semantics test happens to pass against 7b1ae48b0 too (compare-and-set and unconditional rollback coincide in the non-overlapping case) but is kept because it is the correct "retry as page 1, not page 3" pin the review asked for, replacing the one that asserted the wrong thing. SCANNER (pageSizeScan.ts) — closed three documented false-negative classes: - Transparent TS wrappers around the initializer (`pageSize: 100 as const`, `100 satisfies number`, parenthesized) are now unwrapped before the NumericLiteral/Identifier check. - Non-Identifier property names: a quoted string key (`'pageSize': 100`) or a statically-resolvable computed key (`['pageSize']: 100`) are now accepted; a computed key that isn't a literal correctly stays unresolved. - `.mts`/`.cts` are no longer silently excluded from the guard's file discovery glob (tsconfig.app.json's `include` covers all of `src`; no such files exist in the repo today, but the glob shouldn't hide one if it ever does). 10 new fixture tests in pageSizeScan.test.ts pin each case (plus a rejection test confirming a forwarded call wrapped in `as` still doesn't match, and one confirming an unresolvable computed key stays unmatched). TEST LABELLING: relabeled the URL/M-3 and `??`/M-4 fixtures as CONTRACT fixtures rather than regression pins — a round-3 review found round 1's plain literal regex already handled those two exact inputs correctly on its own; only the combined multi-case fixture (and the string-contains-text, template-interpolation, same-line-identity, JSX, and destructuring fixtures) actually fail against round 1. Labeled the guard test's 4 tests as BASELINE assertions (they all pass on clean b90f8a3b) rather than implying they prove this round's specific fixes — pageSizeScan.test.ts's fixtures are what actually regression-pin the scanner. No server-side/C# change. Full local gate: lint clean, tsc clean, full vitest run 110 files / 1063 tests passed (re-run twice, stable).
132 lines
7.1 KiB
TypeScript
132 lines
7.1 KiB
TypeScript
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 `{`/`(`/`,`.
|
|
*
|
|
* A round-3 review found the AST version still had its own — smaller, but real — false
|
|
* negatives: an initializer wrapped in a transparent TS construct (`pageSize: 100 as const`,
|
|
* `pageSize: 100 satisfies number`, `pageSize: (100)`) was rejected outright because only a bare
|
|
* `NumericLiteral`/`Identifier` was checked; a property written as a quoted string key
|
|
* (`'pageSize': 100`) or a statically-resolvable computed key (`['pageSize']: 100`) was missed
|
|
* because only an `Identifier` name was checked. `unwrapTransparentExpression` and
|
|
* `isPageSizePropertyName` close both — see their doc comments below. Genuinely UNRESOLVABLE
|
|
* cases remain out of reach on purpose and are documented as a residual gap where this scanner is
|
|
* actually used (`pageSizeCallSites.guard.test.ts`'s module doc comment): object SPREAD
|
|
* (`getFoo({ ...opts })` built elsewhere) and a `pageSize` passed as a bare POSITIONAL argument
|
|
* rather than an object-literal property at all.
|
|
*/
|
|
|
|
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 {
|
|
// `.mts`/`.cts` parse as plain TS (no JSX support), same as `.ts` — only `.tsx` needs the JSX
|
|
// grammar. `tsconfig.app.json`'s `include` covers all of `src`, and `.mts`/`.cts` are legal
|
|
// TS extensions the guard's file-discovery glob must not silently skip even though none exist
|
|
// in this repo today (#650 follow-up round 3 MEDIUM finding).
|
|
return fileName.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS;
|
|
}
|
|
|
|
// Unwraps TS constructs that are transparent to the runtime VALUE but would otherwise hide a
|
|
// numeric literal / identifier from a naive node-kind check: `expr as T`, `expr satisfies T`,
|
|
// and `(expr)`. `pageSize: 100 as const` and `pageSize: 100 satisfies number` are both real
|
|
// fixed-100 call sites; only the TS type-checking wrapper differs (#650 follow-up round 3 MEDIUM).
|
|
function unwrapTransparentExpression(node: ts.Expression): ts.Expression {
|
|
let current = node;
|
|
for (;;) {
|
|
if (ts.isParenthesizedExpression(current)) {
|
|
current = current.expression;
|
|
} else if (ts.isAsExpression(current)) {
|
|
current = current.expression;
|
|
} else if (ts.isSatisfiesExpression(current)) {
|
|
current = current.expression;
|
|
} else {
|
|
return current;
|
|
}
|
|
}
|
|
}
|
|
|
|
// A property name is `pageSize` whether written as a plain identifier (`pageSize: 100`), a
|
|
// quoted string key (`'pageSize': 100`), or a computed key that's STATICALLY a `'pageSize'`
|
|
// string literal (`['pageSize']: 100`) — all three compile to the identical property, so all
|
|
// three are real call sites (#650 follow-up round 3 MEDIUM). A computed key that ISN'T a literal
|
|
// (e.g. `[dynamicKeyVar]: 100`) can't be resolved statically and is correctly left unmatched.
|
|
function isPageSizePropertyName(name: ts.PropertyName): boolean {
|
|
if (ts.isIdentifier(name) || ts.isStringLiteral(name)) {
|
|
return name.text === 'pageSize';
|
|
}
|
|
if (ts.isComputedPropertyName(name)) {
|
|
const expr = unwrapTransparentExpression(name.expression);
|
|
return ts.isStringLiteral(expr) && expr.text === 'pageSize';
|
|
}
|
|
return false;
|
|
}
|
|
|
|
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) && isPageSizePropertyName(property.name)) {
|
|
const initializer = unwrapTransparentExpression(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) || 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}`;
|
|
}
|