From 689451161ef526ffbd2dd4ee4727bad62c3fdaa0 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 27 Jul 2026 19:17:07 +0200 Subject: [PATCH] fix(684): review round 2 -- drop a false exhaustiveness claim I introduced The L1 fix (name every class-b shape) collided with the M1 fix (move SmartCollectionDialog into class-b) in the same commit: the comment claimed "exactly three shapes, which is the whole list" while the registry 80 lines below already held four. That is the same false-exhaustiveness defect this PR exists to remove -- a reader adding a fifth class-b site would conclude theirs cannot be class-b despite rendering a real totalCount hint, and reach for search-bounded or deviation. The operative rule is now stated first and the shape list is explicitly illustrative: a site qualifies by RENDERING a totalCount-backed hint, not by resembling a listed shape. Also, both non-blocking review items: - the deviation prose said the tracking issue goes "in the note", while enforcement is on the structural `issue` field -- it now points at the mechanism that actually binds, and says why the note scrape was rejected; - the UNREGISTERED report prints every position sharing an identity, so it now says "identity seen at:" rather than implying all of them are unregistered. A positionless key cannot know which occurrence is excess; the candidate set is the honest answer. Co-Authored-By: Claude Opus 5 (1M context) --- web/src/api/pageSizeCallSites.guard.test.ts | 36 ++++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/web/src/api/pageSizeCallSites.guard.test.ts b/web/src/api/pageSizeCallSites.guard.test.ts index 6fef3ae59..a923d9126 100644 --- a/web/src/api/pageSizeCallSites.guard.test.ts +++ b/web/src/api/pageSizeCallSites.guard.test.ts @@ -91,14 +91,21 @@ import { scanPageSizeSites } from './pageSizeScan'; * 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. + * longer covers media-library pickers (those are 'search-bounded'). + * **The RENDER is the entry requirement, not the intent** — that is the + * operative rule, and the only one to apply to a new site. Today's + * entries happen to take four shapes: a list bounded by its PARENT + * (`ChannelBuilder`, seasons of one show); the collection-family types + * §3b excludes from search (`FillerPresetsScreen`), which keep the + * bounded page and its hint; a preview over an already-bounded set + * (`AutoTuneScreen`'s channel members); and a preview over an UNBOUNDED + * user-authored query that surfaces its match count + * (`SmartCollectionDialog`). That list is illustrative and NOT + * exhaustive: a site qualifies by rendering a real `totalCount`-backed + * hint, not by resembling one of these four. (#684 review: an earlier + * revision of this comment called it "the whole list" while the registry + * below already held a fourth — the same false-exhaustiveness defect this + * PR exists to remove.) * - '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. @@ -106,9 +113,11 @@ import { scanPageSizeSites } from './pageSizeScan'; * 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. + * exist, and the next reader would trust it. Every such entry MUST carry + * its tracking issue in the structural `issue` field — enforced below, + * and deliberately NOT a `#\d+` scrape of the note, which passed with the + * reference deleted because notes legitimately cite historical issues — + * 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 @@ -419,7 +428,10 @@ function describeDiscovered(sites: DiscoveredSite[], ids: string[]): string[] { } return ids.map((id) => { const at = positions.get(id); - return at && at.length > 0 ? `${id} (at ${at.join(', ')})` : id; + // Every position sharing this identity, not just the excess one: a positionless key genuinely + // cannot tell which occurrence is new, so the candidate set IS the honest answer. Labelled so a + // reader does not take all of them as unregistered (#684 review L-a). + return at && at.length > 0 ? `${id} (identity seen at: ${at.join(', ')})` : id; }); }