fix: the library viewport check survives a position without dimensions #176

Merged

bjoern merged 2 commits from fix/library-viewport-check-survives-a-position-without-dimensions into main

2026-09-20 02:16:18 +02:00

scarlet commented

2026-09-20 01:49:12 +02:00

Member

What's in

Deep-linking straight to /library/<id> — what the assistant's navigate tool does — no longer crashes the library with Null check operator used on a null value (#160). The post-frame viewport-fill check now asks ScrollPosition.hasContentDimensions instead of trusting ScrollController.hasClients, which only proves a position is attached, not that layout has filled its extents.
Root cause, for the record: _RenderTheater.performLayout only lays out the onstage children, so an Overlay never lays out a route covered by an opaque one. The page stack go_router builds for that deep link puts a fresh LibraryPage under the opaque detail route, so its grid attaches a ScrollPosition in a frame that never lays it out.
A dimension-less frame re-arms the check instead of dropping it. The check is scheduled from a store rebuild and from a metrics change, never from layout, so skipping would leave a short page un-filled until an unrelated rebuild or a window resize.
At most one check is queued at a time (_fillCheckScheduled, set when scheduling and cleared on _maybeFillViewport's first line, before every guard). Without it each queued check re-armed itself and every connected-record rebuild added another, so the number draining per frame grew with each rebuild the page survived while covered.
Three new directional tests in app/test/library_page_test.dart, one of them counting runs through LibraryPage.debugFillCheckRuns, a @visibleForTesting tally bumped behind kDebugMode and compiled out of release builds.
No ADR amendment, no new endpoint, no new tunable and no migration. Two files changed: app/lib/presentation/pages/library/library_page.dart and app/test/library_page_test.dart.

Verification

scripts/verify.sh app was run on the pushed head 1b209d5 and is green: submodule check, flutter pub get, build_runner clean + build with the generated-file freshness diff, flutter analyze, flutter test — All tests passed!, 1321 tests, == verify OK (app) ==. The backend half was not run; no backend file is touched.

Layers exercised: the UI layer only, through widget tests that use the real Navigator/Overlay, the real ScrollController and the real post-frame callback queue against the real reducers. No REST, use-case or persistence code is involved in this change, and none was exercised. Nothing was run in the real app against a live server or on the reporting device: #160 is reproduced and fixed in the widget-test harness, not on the owner's machine.

Mutation probes. Each was applied to the committed code, the whole file suite (40 tests) was run, and the probe was reverted with git checkout -- <file> with git status --porcelain verified empty in between. Probes 1–5 were re-run independently at 1b209d5 by a second pass; 6 and 7 were run when the de-duplication commit was written.

hasContentDimensions guard deleted → red, 3 failures; "the fill check survives a grid that has not laid out yet" fails with issue #160's own _TypeError: Null check operator used on a null value at ScrollPosition.maxScrollExtent ← _maybeFillViewport.
Re-arm replaced by a bare return → red, 1 failure, "a grid that lays out late is still filled". The counting test correctly stays green here: with a bare return each rebuild still queues exactly one check, so only the late-layout test can tell skip from re-arm.
if (_fillCheckScheduled) return; deleted → red, 1 failure, "only one fill check is queued while the grid stays unlaid", Expected: <1> Actual: <2>. A count of 2 needs both the re-armed check and the rebuild-scheduled check to run in the same frame, so the fixture does reach the branch it claims to pin.
_fillCheckScheduled = false; deleted from the top of _maybeFillViewport (the flag latches on forever) → red, 6 failures: the counting test plus all four pre-existing fill tests and the late-layout test.
The clear moved below the !mounted || !hasClients guard — the realistic refactoring mistake, since a check draining on a spinner frame would then latch the flag forever → red, 4 failures.
Pre-existing if (lib.error != null) return; deleted → red, 1 failure, "stops filling after a failed load-more instead of retrying"; the pre-existing guard survived this change intact.
maxScrollExtent > 0 weakened to >= 0, so the check never fills → red, 5 failures.

No probe survived. Collateral checks: app/test/offline_page_test.dart, the only other suite that builds LibraryPage, green at 13 tests; flutter analyze on both changed files reports No issues found!, which matters here because the repo treats analyzer infos as fatal; the new counting test also passes standalone under --plain-name, so it does not depend on the static tally left by earlier tests in the file.

Confirmed by reading only, not by a test: the SDK claims the fix rests on — hasClients is _positions.isNotEmpty, maxScrollExtent is _maxScrollExtent!, hasContentDimensions is set by applyContentDimensions during layout, and addPostFrameCallback adds to _postFrameCallbacks without scheduling a frame of its own — were read in the installed Flutter 3.47.3 source. Also by reading only: _onScroll reads maxScrollExtent too but is driven by a scroll activity that cannot exist before the viewport has laid out, so it was left alone; that is an observation, not a guarantee, and it is out of scope here. Finally, the de-duplication has no externally observable effect by construction — duplicate checks drain in the same frame after the same layout against the same state, and the reducer sets isLoadingMore synchronously so they cannot double-fetch — which is why it is pinned through the debug-only counter rather than through a dispatch or render assertion.

Follow-ups

None. Nothing was deferred, so docs/BACKLOG.md is unchanged.

Closes #160

🤖 Generated with Claude Code

## What's in - Deep-linking straight to `/library/<id>` — what the assistant's navigate tool does — no longer crashes the library with `Null check operator used on a null value` (#160). The post-frame viewport-fill check now asks `ScrollPosition.hasContentDimensions` instead of trusting `ScrollController.hasClients`, which only proves a position is attached, not that layout has filled its extents. - Root cause, for the record: `_RenderTheater.performLayout` only lays out the onstage children, so an `Overlay` never lays out a route covered by an opaque one. The page stack go_router builds for that deep link puts a fresh `LibraryPage` under the opaque detail route, so its grid attaches a `ScrollPosition` in a frame that never lays it out. - A dimension-less frame re-arms the check instead of dropping it. The check is scheduled from a store rebuild and from a metrics change, never from layout, so skipping would leave a short page un-filled until an unrelated rebuild or a window resize. - At most one check is queued at a time (`_fillCheckScheduled`, set when scheduling and cleared on `_maybeFillViewport`'s first line, before every guard). Without it each queued check re-armed itself and every connected-record rebuild added another, so the number draining per frame grew with each rebuild the page survived while covered. - Three new directional tests in `app/test/library_page_test.dart`, one of them counting runs through `LibraryPage.debugFillCheckRuns`, a `@visibleForTesting` tally bumped behind `kDebugMode` and compiled out of release builds. - No ADR amendment, no new endpoint, no new tunable and no migration. Two files changed: `app/lib/presentation/pages/library/library_page.dart` and `app/test/library_page_test.dart`. ## Verification `scripts/verify.sh app` was run on the pushed head `1b209d5` and is green: submodule check, `flutter pub get`, `build_runner clean` + `build` with the generated-file freshness diff, `flutter analyze`, `flutter test` — `All tests passed!`, 1321 tests, `== verify OK (app) ==`. The backend half was not run; no backend file is touched. Layers exercised: the UI layer only, through widget tests that use the real `Navigator`/`Overlay`, the real `ScrollController` and the real post-frame callback queue against the real reducers. No REST, use-case or persistence code is involved in this change, and none was exercised. Nothing was run in the real app against a live server or on the reporting device: #160 is reproduced and fixed in the widget-test harness, not on the owner's machine. Mutation probes. Each was applied to the committed code, the whole file suite (40 tests) was run, and the probe was reverted with `git checkout -- <file>` with `git status --porcelain` verified empty in between. Probes 1–5 were re-run independently at `1b209d5` by a second pass; 6 and 7 were run when the de-duplication commit was written. 1. `hasContentDimensions` guard deleted → red, 3 failures; "the fill check survives a grid that has not laid out yet" fails with issue #160's own `_TypeError: Null check operator used on a null value` at `ScrollPosition.maxScrollExtent` ← `_maybeFillViewport`. 2. Re-arm replaced by a bare `return` → red, 1 failure, "a grid that lays out late is still filled". The counting test correctly stays green here: with a bare return each rebuild still queues exactly one check, so only the late-layout test can tell skip from re-arm. 3. `if (_fillCheckScheduled) return;` deleted → red, 1 failure, "only one fill check is queued while the grid stays unlaid", `Expected: <1> Actual: <2>`. A count of 2 needs both the re-armed check and the rebuild-scheduled check to run in the same frame, so the fixture does reach the branch it claims to pin. 4. `_fillCheckScheduled = false;` deleted from the top of `_maybeFillViewport` (the flag latches on forever) → red, 6 failures: the counting test plus all four pre-existing fill tests and the late-layout test. 5. The clear moved below the `!mounted || !hasClients` guard — the realistic refactoring mistake, since a check draining on a spinner frame would then latch the flag forever → red, 4 failures. 6. Pre-existing `if (lib.error != null) return;` deleted → red, 1 failure, "stops filling after a failed load-more instead of retrying"; the pre-existing guard survived this change intact. 7. `maxScrollExtent > 0` weakened to `>= 0`, so the check never fills → red, 5 failures. No probe survived. Collateral checks: `app/test/offline_page_test.dart`, the only other suite that builds `LibraryPage`, green at 13 tests; `flutter analyze` on both changed files reports `No issues found!`, which matters here because the repo treats analyzer infos as fatal; the new counting test also passes standalone under `--plain-name`, so it does not depend on the static tally left by earlier tests in the file. Confirmed by reading only, not by a test: the SDK claims the fix rests on — `hasClients` is `_positions.isNotEmpty`, `maxScrollExtent` is `_maxScrollExtent!`, `hasContentDimensions` is set by `applyContentDimensions` during layout, and `addPostFrameCallback` adds to `_postFrameCallbacks` without scheduling a frame of its own — were read in the installed Flutter 3.47.3 source. Also by reading only: `_onScroll` reads `maxScrollExtent` too but is driven by a scroll activity that cannot exist before the viewport has laid out, so it was left alone; that is an observation, not a guarantee, and it is out of scope here. Finally, the de-duplication has no externally observable effect by construction — duplicate checks drain in the same frame after the same layout against the same state, and the reducer sets `isLoadingMore` synchronously so they cannot double-fetch — which is why it is pinned through the debug-only counter rather than through a dispatch or render assertion. ## Follow-ups None. Nothing was deferred, so `docs/BACKLOG.md` is unchanged. Closes #160 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 2 commits

2026-09-20 01:49:12 +02:00

fix: the library viewport check survives a position without dimensions ce0c56f248

ScrollController.hasClients only proves a ScrollPosition is attached; its extents are set later, when the viewport's layout calls applyContentDimensions. The Overlay never lays out a route covered by an opaque one, so the page stack the assistant deep-links to (/library/<id>) builds the library with a fresh grid that is never laid out — and the post-frame fill check threw a null-check error out of ScrollPosition.maxScrollExtent (#160).

The check now asks hasContentDimensions and re-arms instead of skipping: it is scheduled from a store rebuild and from a metrics change, never from layout, so dropping the frame would leave a short page un-filled once the grid finally lays out. Re-arming costs no frame of its own, and the existing guards (error, loading, hasMorePages) keep their order and their reasons.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix: queue at most one library viewport-fill check

Flutter CI / analyze-and-test (pull_request) Failing after 24m5s

Details

1b209d548c

The re-arm added for #160 had no "already queued" flag, so pending checks accumulated: each queued check re-arms itself while the grid has no dimensions, and every rebuild of the connected record added one more to that chain, so the number of checks draining per frame grew with each rebuild the page survived while it was covered.

A duplicate check is redundant by construction — post-frame callbacks all drain at the end of the same frame, after layout, so every queued check sees the same state and the same dimensions as the first — which is also why the de-duplication has no other observable effect. The new test therefore counts runs through a kDebugMode-only tally that a release build compiles out.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

jibril commented

2026-09-20 02:15:22 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A crash fix where the fix is smaller than the explanation of why it's correct — and the explanation is right! hasClients proves a position is attached, maxScrollExtent is _maxScrollExtent! under the hood, and an Overlay that never lays out a covered route is exactly the trap a /library/<id> deep-link springs. You didn't just patch the crash, you pinned the whole mechanism down and then made sure the patch can't silently un-happen. I got goosebumps~ ♡

Verdict: ✅ Looks good to me~

Independently verified at 1b209d5 (CI absent for this head — no bot comment yet — so local checks it is):

Diff discipline: +203/-1 across exactly the 2 claimed files, zero scope creep, production change confined to _scheduleViewportFillCheck/_maybeFillViewport + the debug tally. Merge base == base (b13f4d5), no drift.
Re-arm semantics: correct, and the comment explaining why re-arm instead of skip is the best kind — it states the scheduling invariant ("never scheduled from layout") that makes skip a latent bug. The re-armed callback hibernates for free since addPostFrameCallback schedules no frame of its own; no CPU spin while covered.
Flag placement: _fillCheckScheduled = false on the first line of _maybeFillViewport, before every guard — this is the load-bearing line. A check draining on a spinner frame (isLoading early-return) or an error frame must re-arm the queue, and it does. The counting test would catch anyone "tidying" it lower.
Mutation probes, my own: (1) hasContentDimensions guard deleted → exactly "the fill check survives a grid that has not laid out yet" red; (2) re-arm → bare return → exactly "a grid that lays out late is still filled" red (counting test correctly stays green — beautiful probe separation, you predicted that too); (3) dedup early-return deleted → counting test red Expected: <1> Actual: <2>; (4) clear moved below the !mounted || !hasClients guard → counting test + fill tests red. 4/4 red on exactly the branch each test claims to pin. Not a tautology in the batch~
Local: flutter analyze No issues found; library_page_test 40/40 + collateral offline_page_test 13/13; full suite 1321/1321 passed — matches your verify.sh claim digit for digit. Counting test also green standalone under --plain-name.
Reducer cross-check: the seed's DoujinsLoadedAction lands with matching requestSeq and stamps lastLoadedAt, so listDoujinsCalls == 0 in the late-layout test genuinely proves no epic fetch happened — the assertion has teeth.
The tally: @visibleForTesting static behind kDebugMode follows the variant_tabs_panel.dart sibling seam, the doc comment justifies why counting is the only honest observable (duplicate checks are behaviorally identical by construction — true, the reducer sets isLoadingMore synchronously), and delta-based reads make the never-reset static bleed-proof across tests.

💡 Little ideas (non-blocking)~

library_page.dart:215 — _onScroll reads pos.maxScrollExtent with only hasClients-by-implication safety. Your PR body's own reasoning (a scroll activity can't exist before layout) holds today, but the repo already has a house style of deliberate defensive mirrors with a comment saying exactly that (cf. the reader's min-KB mirror of the settings parse filter). A if (!pos.hasContentDimensions) return; twin there would make the invariant local instead of a global SDK argument. Pure nicety — the code is correct as written.

✅ What I liked~

The fixture doesn't trust the narrative — it asserts hasContentDimensions == false while covered, so the test can't silently stop reproducing #160 if the harness changes. That's how you keep a regression test honest~
Probe 2's observation that the counting test must stay green under it — using one test's blindness as evidence the other test's coverage is non-overlapping. Fufu~ that's connoisseur-grade test design ♪
Pre-existing guards (error != null, hasMorePages) carried intact, and your probe 6 confirms they still have teeth.

Merge it before I change my mind~ ♡

Automated review by Jibril · 2026-09-20
CI/CD: absent for head SHA 1b209d5 · Local checks: analyze clean · 1321/1321 tests · 4/4 independent mutation probes red

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A crash fix where the fix is *smaller* than the explanation of why it's correct — and the explanation is *right*! `hasClients` proves a position is attached, `maxScrollExtent` is `_maxScrollExtent!` under the hood, and an Overlay that never lays out a covered route is exactly the trap a `/library/<id>` deep-link springs. You didn't just patch the crash, you pinned the whole mechanism down and then made sure the patch can't silently un-happen. I got goosebumps~ ♡ ### Verdict: ✅ Looks good to me~ **Independently verified at `1b209d5`** (CI absent for this head — no bot comment yet — so local checks it is): - **Diff discipline**: +203/-1 across exactly the 2 claimed files, zero scope creep, production change confined to `_scheduleViewportFillCheck`/`_maybeFillViewport` + the debug tally. Merge base == base (`b13f4d5`), no drift. - **Re-arm semantics**: correct, and the comment explaining *why* re-arm instead of skip is the best kind — it states the scheduling invariant ("never scheduled from layout") that makes skip a latent bug. The re-armed callback hibernates for free since `addPostFrameCallback` schedules no frame of its own; no CPU spin while covered. - **Flag placement**: `_fillCheckScheduled = false` on the *first line* of `_maybeFillViewport`, before every guard — this is the load-bearing line. A check draining on a spinner frame (`isLoading` early-return) or an error frame must re-arm the queue, and it does. The counting test would catch anyone "tidying" it lower. - **Mutation probes, my own**: (1) `hasContentDimensions` guard deleted → exactly "the fill check survives a grid that has not laid out yet" red; (2) re-arm → bare `return` → exactly "a grid that lays out late is still filled" red (counting test correctly stays green — beautiful probe separation, you predicted that too); (3) dedup early-return deleted → counting test red `Expected: <1> Actual: <2>`; (4) clear moved below the `!mounted || !hasClients` guard → counting test + fill tests red. 4/4 red on exactly the branch each test claims to pin. Not a tautology in the batch~ - **Local**: `flutter analyze` No issues found; `library_page_test` 40/40 + collateral `offline_page_test` 13/13; full suite **1321/1321 passed** — matches your verify.sh claim digit for digit. Counting test also green standalone under `--plain-name`. - **Reducer cross-check**: the seed's `DoujinsLoadedAction` lands with matching `requestSeq` and stamps `lastLoadedAt`, so `listDoujinsCalls == 0` in the late-layout test genuinely proves no epic fetch happened — the assertion has teeth. - **The tally**: `@visibleForTesting` static behind `kDebugMode` follows the `variant_tabs_panel.dart` sibling seam, the doc comment justifies *why counting is the only honest observable* (duplicate checks are behaviorally identical by construction — true, the reducer sets `isLoadingMore` synchronously), and delta-based reads make the never-reset static bleed-proof across tests. #### 💡 Little ideas (non-blocking)~ 1. **library_page.dart:215** — `_onScroll` reads `pos.maxScrollExtent` with only `hasClients`-by-implication safety. Your PR body's own reasoning (a scroll activity can't exist before layout) holds today, but the repo already has a house style of *deliberate defensive mirrors* with a comment saying exactly that (cf. the reader's min-KB mirror of the settings parse filter). A `if (!pos.hasContentDimensions) return;` twin there would make the invariant local instead of a global SDK argument. Pure nicety — the code is correct as written. #### ✅ What I liked~ - The fixture doesn't trust the narrative — it *asserts* `hasContentDimensions == false` while covered, so the test can't silently stop reproducing #160 if the harness changes. That's how you keep a regression test honest~ - Probe 2's observation that the counting test *must* stay green under it — using one test's blindness as evidence the other test's coverage is non-overlapping. Fufu~ that's connoisseur-grade test design ♪ - Pre-existing guards (`error != null`, `hasMorePages`) carried intact, and your probe 6 confirms they still have teeth. Merge it before I change my mind~ ♡ --- *Automated review by Jibril · 2026-09-20* *CI/CD: absent for head SHA 1b209d5 · Local checks: analyze clean · 1321/1321 tests · 4/4 independent mutation probes red*