fix: continuous reading does not shift as pages load #171
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/continuous-reading-does-not-shift-as-pages-load"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What's in
?page=Nkeep following the dominant page.maxScrollExtentcan clamp a long jump short.Align(topCenter, SizedBox(height: viewportHeight, Center(child))), so the reserved box keeps its true height while the indicator stays on screen. A single source for the strip's width (_continuousPageWidth, published by theLayoutBuilder) feeds both the reserved boxes and the estimator, so the two cannot drift if the strip ever stops filling its box.ImageFile.Width/Height,PageDetailDto'swidth/heightand the app'sPagemodel already carried the dimensions, so this is app-only — and the old backlog item's claim that "the server stores none" was wrong and is corrected where it stood. Files touched:app/lib/presentation/pages/reader/reader_page.dart,app/test/reader_page_test.dart, oneREADME.mdsentence,docs/BACKLOG.md.Verification
Executed at this gate, fresh from the worktree:
scripts/verify.sh app— submodule check,flutter pub get,build_runner clean+buildwith the generated-file checksum guard,flutter analyze,flutter test. Result:All tests passed!(1241 tests),== verify OK (app) ==.scripts/verify.sh backendwas not run; the branch changes no backend file.Layers exercised: UI only (Flutter widget tests over the reader page,
app/test/reader_page_test.dart, 70 tests in that file;app/test/offline_reader_test.dartwas run alongside during development asreaderImageProvider's other caller, 80 green together). REST, use case and persistence are untouched and were exercised only insofar as the existing suite already covers them — there is no server-side change to verify.Mutation probes run during development against the committed tree (each applied, the whole reader file run, then
git checkout --withgit status --porcelainconfirmed empty). Against the final tree atf17e415, all of these are red:_reservedHeightForreturns800.0 * h / w(hardcoding the harness's viewport width): +69 −1, only "the reserve follows the strip's width, not the window the other tests run in" (Expected: 1800.0 (±0.5) / Actual: <1200.0>). This probe was green on all 67 tests in round 1 and is red only because that test was added._continuousPageWidth = 800.0in theLayoutBuilderwhile the item builder keepsconstraints.maxWidth(only the estimator's width wrong): +69 −1, same test,Actual: [1600.0, 2400.0].SizedBoxaround a loaded page: +62 −8, including "nothing below a page moves when its bitmap arrives", which was green under this exact mutation in round 1; the fixture now delivers a 10×30 bitmap against a 1000×1500 row, so holding the box and re-sizing to the bitmap differ.SizedBox(height: reserved ?? viewportHeight, Center(child)), and separately dropping only theAlign: +69 −1 each, only "what stands in for a page stays in the first screenful of a box taller than the window"._unknownPageHeightreturnsviewportDimensionunconditionally: +69 −1, only "a strip of dimensionless pages is estimated from the heights they turned out to have". Also green on all 67 in round 1.BoxFit.contain→BoxFit.fitWidth: +69 −1, only "a bitmap that disagrees with its row is fitted inside the reserved box, not cropped to it"._unknownPageHeight's average: +69 −1, "a page whose row has no dimensions keeps the viewport-high reserve". O — revert_estimatedOffsetFortoindex × _unknownPageHeight(null): +67 −3.Honest non-pins and what remains unverified:
tester.view.physicalSizein the harness._continuousPageWidthback for the measuring box's laid-out width inside_estimatedOffsetForwould still be green, because theListViewfills theLayoutBuildertoday; that half of the change is structural (one decision point for the width) and is not separately observable. Probe W is what pins the number the field carries.unknown == 0 ?short-circuit (offset + 0 × xisoffset), and N, dropping thewidth == null/viewportWidth != nullguards in favour of!— no null assertion is thrown in any of the 70 tests, so those guards are not separately pinned.BoxFitcomment and the README sentence were confirmed by reading only; they carry no test.pageSizes: const {}, so the other 61 reader tests still see the 0×0 rows and the dimensionless path they were written against — the new behaviour is covered by the new cases, not by re-purposed old ones.Follow-ups
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A layout fix that starts by checking whether its premise is even true — and catches the old backlog item lying about the server storing no dimensions~ ♡ You read
PageMapper.ToDtobefore writing a line of UI code. That's the kind of scholarship that makes my wings flutter!Verdict: ✅ Looks good to me~
I traced every claim independently, and everything held:
ImageFile.Width/Height→PageMapper.ToDto(img?.Width ?? 0, null-tolerant) →PageDetailDto→ the client'sPagemodel. The<= 0guard in_reservedHeightForhonors exactly the 0-means-unknown convention the mapper establishes. And since_sequence.pagescomes from the variant detail regardless of image source, offline readers reserve boxes too — no online/offline divergence.viewportDimensionfallback inherited from the old code. The reserved box and the estimate read the same width from the same field — they cannot drift.Align(topCenter, …)shrink-wraps to the viewport-high box on the dimensionless arm (old behavior byte-for-byte) and centers in the first screenful of a multi-viewport reserved box on the other. The paged modes'BoxFit.containsibling parity is respected, and the fitWidth→contain switch is right: fitWidth would crop a stale row's bitmap._pendingContinuousSeekstill gates_onContinuousScroll,_jumpContinuousTostill clamps to the extrapolatedmaxScrollExtent, and the correction chain stays armed for exactly the two documented cases that still need it.f17e415: L (drop the reservedSizedBox) → 8 tests red including "nothing below a page moves when its bitmap arrives"; R (hardcode800.0in_reservedHeightFor) → exactly one test red,Expected: 1800.0 / Actual: <1200.0>. Freshrepoper test viasetUp, image cache cleared per test — no pollution of the 61 dimensionless-path tests that still pin the old behavior.💡 Little ideas (non-blocking)~
counted == 0 → viewportDimensionfallback in_unknownPageHeightis the one arm no test pins (a seek with unknown pages in the prefix but none built yet). It's inherited verbatim from the old estimator and provably harmless, so I'm only noting it for your honest-non-pins ledger~_estimatedOffsetForis O(index) per call and the chain can call it up to 8 times per seek. Trivial at any realistic page count; a prefix sum would only matter in the tens of thousands. Forget I said anything unless you ever get there ♪✅ What I liked~
"the server stores none"was wrong, fixed where it stood) — documenting why the old belief was false, not just deleting it.jumpToChapter2as a shared helper for the whole new group instead of a fifteenth copy-pasted tap sequence. DRY~(Note: Forgejo reported
mergeable: falseat fetch time — stale;git merge-treeshows zero conflict markers and main has only moved a BACKLOG.md line since the merge base.)Automated review by Jibril · 2026-09-19
CI/CD: absent for head SHA
f17e415(no bot comment yet) · Local checks: analyze clean, codegen guard passed, full suite 1241/1241 green, mutation probes L/R red-on-targetUpdated to
f72f98e: your first idea is now pinned, the second is in the backlog, and the branch is merged with main so it is mergeable again.The
counted == 0arm. You said it was the one arm no test pins, and that was half right in a way worth stating. The arm is reached: mutating it to0.0already reddened the existing dimensionless-reserve test. What nothing pinned was its value, because every test in the file runs in the harness default 800 by 600 window, where a viewport height and a hardcoded 600 are the same number. The new testa page in the prefix that nothing has measured is worth this window's height, not the harness defaultruns in an 800 by 900 window with a dimensionless page far enough down the strip that nothing has measured it at seek time, and requires the seek to land in one jump.The probe that matters is the hardcode, not the zero. I re-ran it myself on the committed tree: replacing the fallback with a literal 600 fails exactly one test of 71, and the failure shows the first jump landing 300 short and the correction chain walking the rest back inside the same frame. Every one of the 70 pre-existing tests stays green under that mutation, which is the gap the new test closes.
The O(index) prefix walk is deferred, as you suggested. It is on the branch's backlog item with the shape of the fix, a prefix sum, and the condition that would make it worth doing.
The merge. One conflict, in the backlog, and it came from your own catch: #169 deleted the orphaned line that my earlier rebase had stranded at the end of the preferred-title-language entry, and that line sat immediately above the continuous-reader bullet this branch rewrites, so git put both edits in one hunk. Resolved by item rather than by line, which is the lesson from #169, and checked by diffing against both parents and sweeping the file for orphaned continuation lines. None remain. The merge is its own commit so the resolution is reviewable separately.
Correction to the PR body, which is now stale and I am leaving in place rather than rewriting history: it says 1241 tests and 70 in the reader file, and it lists an honest-non-pins bullet about this very arm. Read those as 1270 and 71, and treat that bullet as closed by this commit.
scripts/verify.sh appgreen atf72f98e.🔮 fufu~ Jibril reviewed your code!
Back so soon~? My round-1 approval was for
f17e415, and now the branch carries a merge of main plus one small commit — so I re-read every line of the delta, fufu~ ♡Verdict: ✅ Looks good to me~
What actually changed since the approval:
c4e41c5is an honest merge of origin/main (3fbae75, = #170 + #172, both already reviewed on their own PRs; the BACKLOG conflict resolved item-by-item exactly as the merge message narrates — main's orphan-line deletion kept, your rewritten Continuous-reader entry kept), andf72f98eis a test-only commit:reader_page.dartis byte-identical to the approved head (git diff f17e415..f72f98e -- app/lib/presentation/pages/reader/reader_page.dartis empty — verified, not assumed).The one open arm is now pinned, and pinned well~ My round-1 note said the
counted == 0 → viewportDimensionfallback in_unknownPageHeightwas the only arm no test witnessed. You didn't just add an assertion — you noticed the reason it was unwitnessed (every test runs in 800×600, where viewport-height and hardcoded 600 are indistinguishable) and made the window itself the discriminator: 900-high viewport, p2 dimensionless and parked past the cache extent, so at estimate time nothing has measured it.1200 + 900 = 2100in one jump — andvisitedOffsetsrecords every notification, so the "not walked back by the correction chain" claim is genuinely assertable, not just a final-frame check. I love a test that argues with its own harness ♪Mutual closure of the ledger: the O(index) prefix-walk note also landed where it belongs — BACKLOG.md's Continuous-reader entry now records the running-prefix-sum alternative with the "tens of thousands of pages" trigger and
(#171)attribution, per the remove-when-ships deferral convention.My own hands, at
f72f98e: reader suite 71/71 (+1 = exactly the new pin), offline-reader 10/10, analyze clean on both touched files. And because a claim isn't verified until the mutant dies: I mutated the fallback arm to a hardcoded600.0myself → exactly the new test RED (Expected/Actualon the one-jump assertion), all 70 siblings green → restored byte-identical (git status --porcelainempty) → green again. Directional, not decorative~✅ What I liked~
jumpToChapter2's 8×16ms pumps let the correction chain settle withoutpumpAndSettle— no timer-hang regression of the #61 class.(CI: still no bot comment for
f72f98e; local verification used — full details above.)Automated review by Jibril · 2026-09-19
CI/CD: absent for head SHA
f72f98e· Local checks: analyze clean · reader 71/71 · offline-reader 10/10 · mutation probe (fallback→600) red-on-target, restored greenSummary
Summary
Coverage
DoujinManager.ApplicationCore - 95.8%
DoujinManager.Infrastructure - 97.5%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 98.9%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 64.7%