Pin cursor-pagination behaviors left untested in #1131#1136
Merged
alistair3149 merged 10 commits intoJul 24, 2026
Conversation
buildPage skips a list row whose summary fails to load without spending page space, so a full page stays full and a one-at-a-time walk never yields an empty page mid-listing. Pin this on the Mapping endpoint with an XML-imported Mapping missing its schemas key, and correct the trait doc: the cursor is the last served item's page ID, so a malformed row after it is re-scanned and skipped by the next page. Review-fix tests for PR #1131. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Schema, Layout and Mapping name lookups page their namespace in 100-row keyset batches, but no test exceeded a single batch. Drive each past 100 rows via a new bulk bare-page insert helper and assert every row is yielded once in page-ID order. The Schema case also denies the row sitting on the batch boundary, showing the keyset anchor advances past unreadable rows so later batches still arrive. Review-fix tests for PR #1131. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Schema, Layout and Mapping list pages leave totalRows undefined on a full page carrying a non-null cursor, keeping CdxTable indeterminate so Next stays enabled; only the determinate null-cursor flip was covered. Pin the indeterminate case for all three pages, adding a first spec for LayoutsPage (which also covers its determinate flip). Review-fix tests for PR #1131. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JeroenDeDauw
marked this pull request as ready for review
July 23, 2026 14:34
The denied row at the boundary does not distinguish anchor-advance variants (an anchor that only advances on yields re-scans one row and produces identical output); what it uniquely pins is that the drain's continue decision counts fetched rows, not yielded ones. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both testUnloadableMappingDoesNotConsumePageSpace and testWalkingPastAnUnloadableMappingNeverYieldsAnEmptyPage assert result sets that are byte-identical to the listing you get when the imported Beta page does not exist at all. So a silently no-op import — a future import-time validation, an export-format change that makes the str_replace miss, a slot/model change — would leave only Alpha and Gamma, keep both tests green, and never run the loader's skip branch they exist to pin. Guard the fixture's premise: assert the two substitutions took effect on the dump, then assert the import created a distinct Beta page sitting between Alpha and Gamma in page-ID order. Also drop the no-op markPageTableAsUsed() call — the class has no setUp truncation and MW 1.43 resets changed tables via ChangedTablesTracker, so it did nothing and no sibling test in the class uses it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
IDatabase::insert() is marked @internal for callers outside the rdbms library, which puts it on the same removal track as the other raw write methods (MW 1.43 turns such deprecations into test failures). Use the query builder, matching the newSelectQueryBuilder() read three lines below in the same helper and the only other raw-row insert in the suite (DatabaseDeletedSubjectPageIdsLookupTest). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The drain and batch-boundary tests hardcoded the private READABLE_NAMES_BATCH_SIZE of 100 in three ways (120 bulk rows, the boundary index [99], prose comments). Raising the constant above the row count would turn every one of them into a single-batch test that still passes, letting the exact while(false) truncation mutation they were written to kill survive again. Expose the constant and derive the bulk row count (BATCH_SIZE + 20) and the boundary index (BATCH_SIZE - 1) from it, so a changed batch size fails the tests instead of hollowing them out. Give the Layout and Mapping drains the Schema test's full ordered [pageId => name] assertion and matching name (DrainsEveryBatchInPageIdOrder) — the previous count + first-key + last-key shape passed a mid-run wrong-order or wrong-pairing regression that the Schema variant caught. Drop the stale "4 setUp Schemas" count from the doc-block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Codex-internal pager selector and its English "Next page" label were copy-pasted across five sites in three specs. A missed find() returns an empty DOMWrapper whose attributes( 'disabled' ) is undefined, so an enabled-state toBeUndefined() assertion would silently pass against a button that is not in the DOM. Extract findNextPageButton(), which owns the selector and asserts the button exists, and call it from all five sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The PR's premise that "only the null-cursor determinate flip was untested" held for SchemasPage, but MappingsPage had no pager test at all and gained only the indeterminate half. A totalRows.value = undefined mutation (drop the determinate branch) survived the whole MappingsPage suite, shipping a listing that ends on a page boundary with Next enabled forever and a click serving an empty page. Add the determinate test mirroring SchemasPage: a full page with a null cursor disables Next and shows "of 10". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The content guard added in 544a979 asserted assertStringNotContainsString( '"schemas"', $xml ) immediately after str_replace( '"schemas"', '"schemaX"', $xml ) — a post-condition str_replace makes true unconditionally, so it could never fail and guarded nothing (an independent review caught this). Assert the replacement token instead, assertStringContainsString( '"schemaX"', $xml ), which fails precisely when the substitution matched nothing and Beta would stay loadable. This mirrors the title guard, which already checks its replacement target. Verified by sabotage: a non-matching content needle now fails the guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alistair3149
merged commit Jul 24, 2026
17c40dc
into
fix/1062-totalrows-count-oracle
12 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows-up to #1131 — review-fix tests for the three coverage gaps found in its AI review, plus one doc-block correction. No production logic changes.
totalRowsundefined while a response carries a non-null cursor; only the null-cursor flip was tested. An "unconditional flip" mutation survived the whole vitest suite while disabling Next on every full page mid-listing. Adds the indeterminate test to all three pages, including a firstLayoutsPagespec (which also pins its determinate flip).buildPageback-fill over unloadable rows. A readable-but-unparseable page (reachable through XML import, which bypassesvalidateSave— the Imported revisions do not update the graph projection #1022 flow) must not consume page space or surface in the walk. The branch had zero executions; a mutation counting skips against the limit survived the suite. Pinned on the Mapping endpoint with an imported Mapping missing itsschemaskey.while(false)(truncate every listing at 100 rows) survived. Adds >100-row drains via a shared bare-page insert helper; the Schema case also denies the row sitting exactly on the batch boundary.buildPage's cursor is the last served item's page ID, so a trailing skipped row is re-scanned by the next page; the old sentence claimed skips are never re-consumed.Every test was sabotage-checked: the target mutation applied → test fails on the pinned behavior → mutation reverted → green. Full local
make phpunit(1976 tests),make cs, vitest (1107 tests, single-worker) andmake ts-lintpass.Merge note: master has since gained its own
LayoutsPage.spec.ts(delete-dialog tests,totalRows-shaped mocks). When #1131 takes a master merge, resolve the add/add by folding this PR's two pager tests into master's scaffolding and converting its mocks to thenextCursorshape.Production notes
Review orchestration and finding verification by
Fable 5 (max); test implementation by anOpus 4.8 (max)subagent against a fully specified brief. Findings confirmed by independent refute-first verifier agents before any test was written.