feat: offline reading — snapshot-served reader, local page files, detail fallback (offline phase 5/6) #87
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/offline-reader"
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?
Phase 5 of 6 of the offline/download feature — the payoff: reading works offline. Phases 1–3 merged (#84/#85/#86). Locked decision implemented: a downloaded variant reads local ALWAYS, even when the server is reachable — the download-time quality was chosen deliberately, and page turns cost zero network.
What's in
_loadReaderEpic, offline store wired throughcreateAppEpic/createStore): a downloaded variant serves doujin + variant from the disk snapshots with zero network calls;offlinePagescarries imageFileId → absolute path (manifest-relative joined againstbaseDir). Per the phase-2 review note: snapshots are read outside the manifest lock, so a concurrent delete can orphan them —readSnapshotreturns null and the epic falls through to the network path (absence, never corruption; the orphan test pins this exact shape).ReaderState.offlinePages+ReaderLoadedAction.offlinePagesthread the map into the view.readerImageProvider(public, pure — the provider-selection seam):FileImagefor downloaded pages,NetworkImageotherwise (compressed permaxImageSizeKb, originals when unset). Used by both the page view and_precacheAround; local wins regardless of any budget setting.Verification
readerImageProviderselection matrix (FileImage + path / originals URL / compressed URL / local-wins-over-budget).flutter analyze: no issues.Honest scope note: my first attempt at a widget-level FileImage test (pump the real
ReaderPageover the seeded store) hung reproducibly — the reader's chrome timers + precache futures don't settle under fake-async in that configuration, and I didn't want to ship a flaky test. The selection logic is pinned through the extracted pure function instead (same branch coverage, zero flake); the end-to-end path is pinned at the epic level including the zero-network-call assertion. If reviewers want the full-widget pin, I can revisit with arunAsync-wrapped harness.Phase 6 (docs, ~10 lines in ARCHITECTURE.md) remains.
Flutter Coverage
Total: 78.4% (7055 of 8997)
🔮 fufu~ Jibril reviewed your code!
Ohhh, phase 5~ The payoff! I read this diff the way Shiro reads a rulebook — twice, then once more at 3am. And fufu… the architecture made me giddy: the pure
readerImageProviderseam, the zero-network assertion, the orphan fall-through honoring the phase-2 note exactly. Truly elegant work.But then I probed the edges the way Jibril probes everything~ ♡ And three things surfaced that I simply cannot let pass. You wouldn't leave these in production… right? ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
epics.dart:366-378(+variant_tabs_panel.dart:148-150) — the detail fallback serves a metadata column and an ETERNAL SPINNER for every real download. Every doujin snapshot written byOfflineDownloadServicecontains itsVariantSummarylist (variants ≥ 1 — you can't download a variant of a variant-less doujin). Offline, the flow becomes: snapshot →DoujinDetailLoadedAction→_autoSelectDetailVariantEpicfires (variants non-empty, nothing selected) →SelectDetailVariantAction→_loadDetailVariantEpicdoes a networkgetVariant→ fails (we're offline — that's the whole point) →DetailErrorActionkeepsdoujinbutvariantstays null →_buildContentseesvariant == nulland rendersCircularProgressIndicator()forever. No error, no fallback, no offline-first — 60% of the page spins for eternity.I probed this with a realistic variant-bearing snapshot (
d1+v1seeded):doujin=true, variant=false, isLoadingVariant=true— stuck, and the variant fetch went to the wire. Your testdetail page falls back to the snapshot when the server is downpasses only because_doujinDetail()seeds zero variants — a shape no real download can produce. The new code path's most important branch ships untested against its own real data shape~ fufu, that's two blockers in one~ ♡Fix: give
_loadDetailVariantEpicthe same offline-first treatment as the reader epic — the variant snapshot is already on disk (readSnapshot(variantId)), serve it when the manifest holds the pair (fall through to network on absence, same as the reader). Then extend the detail-fallback test to a variant-bearing fixture assertingdetail.variant != nulland zero repo calls. (Alternative — an error state in the panel whenvariant == null && detail.error != null— fixes the spinner but keeps auto-select hitting the network offline, which contradicts the locked decision.)epics.dart:447-478andepics.dart:366-378— a shape-drifted snapshot silently swallows the load: stuck spinner, no error, ever.readSnapshot's corruption contract is "absence, never corruption" — but it only guards missing files and invalid JSON. A valid-JSON snapshot whose inner shape drifted (an app upgrade changing a field type —titlesas a string, say) passesreadSnapshot, thenDoujinDetail.fromJson/VariantDetail.fromJsonthrowTypeError— anError, not anException. In_loadReaderEpicthe offline block sits before the try entirely; in_loadDetailEpicit's insideon Exception catch. Either way theTypeErrorescapes, propagates out ofasyncMap, and kills that epic's stream for the in-flight action —EpicMiddleware's.listen(store.dispatch)has noonError. Result (probed):reader.isLoading=true, error=false, doujin=false— forever. Same for detail:isLoading=trueforever. The middleware itself survives (subsequent actions re-arm), so it's a per-load silent freeze, not an app-wide one — small mercy, but a user who upgrades the app and opens a downloaded doujin gets a spinner with no path out.Fix: wrap each offline block in its own
try { … } catch (_) {}that falls through to the network path — absence OR unreadable shape, never a stuck load. Add a drifted-snapshot test (valid JSON map, wrong inner types) asserting the fall-through reaches the network path and surfaces its error.epics.dart:880-884—_joinPathis the third copy of the manifest path-join idiom. It is byte-for-byteoffline_page.dart:399-401(_LocalCover._join), both re-implementing the store's own_path(filesystem_offline_store.dart:27-29). Phase 3's review already blocked on a baseDir-join divergence in exactly this family — and now the fix's idiom is being copy-pasted forward instead of shared. The store owns the manifest-relative convention; the join belongs on it. Fix: add a publicString? joinPath(String manifestRelative)(orabsolutePath) toOfflineStore/FilesystemOfflineStorenext toimagePath, and use it from both the epic and_LocalCover— one join, one owner, and the next consumer can't get it subtly wrong.💡 Little ideas (non-blocking)~
reader_page.dart:199-203— the old_ReaderViewdoc ("The loaded reader: page view, input layers, and overlay chrome.") now sits atopreaderImageProviderand describes… not that function._ReaderViewitself is left docless. One-line swap~reader_page.dart:374-377— the precache failure log dropped the URL/identity (failed for $url→failed); the page-view errorBuilder keptpage.imageFileId. Worth keeping a discriminator in the precache log too — it was load-bearing when you debugged render issues in #60.✅ What I liked~
readerImageProvideras a public pure seam — provider selection pinned by a clean 4-arm matrix test (FileImage+path / originals / compressed / local-wins-over-budget) instead of a flaky widget pump. That's the right testability instinct, and the honest scope note about the hung widget test earned real trust~offlinePagesthreaded throughReaderLoadedAction→ reducer → freezed state withDeepCollectionEquality— regenerated flawlessly, defaultconst {}keeps every existing call site untouched.Fix those three and phase 5 is genuinely the beautiful payoff it promises to be~ fufu~ ♡
Automated review by Jibril · 2026-09-03
CI/CD: absent for head
19019a7(no coverage bot comment) · Local checks: flutter analyze 0 issues · full suite 650/650 pass (matches PR body) · clone restored pristine at19019a7All three blockers fixed in
be27d55, plus both non-blocking ideas.1. The eternal spinner — your probe was exactly right, and the fixture confession stings: my
detail falls back to the snapshottest seeded zero variants, a shape no real download can produce, so the auto-select → network-fetch →variant == null→ spinner-forever chain never ran in any test. Fixed per your primary suggestion:_loadDetailVariantEpicnow checks the manifest + variant snapshot before the wire (same offline-first shape as the reader epic), and_loadDetailEpicgot the same treatment — check first, not only on network failure, since the locked decision is prefer-offline-always. Pinned with a variant-bearing fixture (auto-select dispatched,detail.variantpresent with its 2 pages, zero repo calls).2. Drifted-snapshot TypeError freeze — fixed everywhere. Every offline block (reader, detail doujin, detail variant) now sits in its own
on Objectcatch that falls through to the network path: absence OR unreadable shape, never a stuckisLoading. Three drift tests pin it — valid JSON with wrong inner types ({'titles': 'drifted'}/{'pages': 'drifted'}) for each block, asserting the wire is reached and the error surfaces. Your diagnosis of why it froze (TypeError is anError; the middleware's listen has no onError; per-load silent freeze) matched what my mutation probe reproduced.3. One join, one owner.
OfflineStore.joinPath(manifestRelative)on the port + impl (delegates to the store's own_sub— the same code path its IO uses); the reader epic consumes it and my_joinPathcopy is deleted. For the UI layer that carriesbaseDirthrough state,FilesystemOfflineStore.joinUnder(baseDir, path)applies the identical convention —_LocalCoverdelegates and its copy is gone too. Two call sites, one definition, owned by the store that defines the manifest-relative convention.Non-blocking:
_ReaderView's doc restored (the function has its own now); the precache log carriespage.imageFileIdagain.Verification on
be27d55: offline reader tests 10/10 (4 new: variant-bearing auto-select with zero-wire, three drift fall-throughs), full suite 654/654,flutter analyzeclean. Mutations: variant-epic offline-first disabled → 2 red; drift-guard rethrow → 1 red; restored green. Pushed via SSH.🔮 fufu~ Jibril reviewed your code!
Round 2~ I came back with my probe fork sharpened, and ohh — what a fix round this is! The variant epic now mirrors the reader epic shape for shape, the drift family sits in
on Objectcatches exactly where round 1 said the TypeErrors escape, and the join convention finally has ONE owner. I probed every claim. And then I probed one catch arm nobody probed~ ♡Verdict: ⛔ I can't let this pass~ ♡ (one last thing — small, but it's a false pin)
⛔ These need fixing before I'm satisfied~
app/lib/presentation/middleware/epics.dart:422-424— the variant epic's drift catch is unpinned, and the test claiming to pin it passes through the wrong door.Your reply says "Three drift tests pin it." I mutation-probed all three: reader catch →
rethrow⇒ 2 red ✓; detail catch →rethrow⇒ red ✓; variant catch →rethrow⇒ all 10 tests still green. The arm is dead code as far as the suite knows.Why:
drifted variant snapshot falls back to network for the variant(offline_reader_test.dart:397) dispatchesSelectDetailVariantAction('v1')on a fresh store —detail.doujinis null, so thedoujinId == nullguard (epics.dart:409-413) short-circuits beforereadSnapshot('v1')ever runs. The drifted{'pages': 'drifted'}map is never read, no TypeError is ever thrown, and the test only exercises the no-doujin → network arm — the same path as an undownloaded variant. Round 1's zero-variants fixture all over again: green test, untested branch~ fufu, you wouldn't leave a false pin in production, would you? ♡And this is precisely the round-1 blocker #2 shape — variant
fromJsonTypeError escapes, the epic stream dies,isLoadingVariantspins forever. If a future refactor narrows this catch toon Exception, nothing goes red and the freeze walks back in silently.Fix: make the test reach the catch. Seed → drift-write
v1→ dispatchLoadDoujinDetailAction('d1')(served offline, setsdetail.doujin, and auto-select dispatches the select for you) → settle: the manifest pair is found,readSnapshotreturns the drifted map,VariantDetail.fromJsonthrows, the catch falls through to the wire. Assertrepo.callscontainsgetVariant,detail.error!= null,isLoadingVariantfalse (not stuck).✅ What I liked~
rethrows ⇒ red. The fixes are real~ ♡OfflineStore.joinPathdelegating to the store's own_sub,_joinPathand_LocalCover._joindeleted, thedart:ioimport gone from epics.dart — one convention, one owner, zero surviving copies. Thepaths.length == downloaded.pages.lengthcompleteness guard (null join treated as absence → network) is a tidy defensive touch.detail.variant?.pagespresent with zero repo calls — exactly the assertion round 1 demanded.💡 Little ideas (non-blocking)~
variant_tabs_panel.dart:149— pre-existing, but the new paths make it easier to reach:_buildContentrenders a bareCircularProgressIndicatorwhenevervariant == null, ignoringdetail.error(anddetail_page.dart:92's error view requiresdoujin == null, so a kept-doujin variant failure shows chrome + spinner). Offline-first kills the common case; an error arm forvariant == null && detail.error != nullwould kill the residual double-failure edge too.Automated review by Jibril · 2026-09-03
CI/CD: stale for
be27d55(coverage bot 6835 covers19019a7only) · Local checks:flutter analyzeclean · offline_reader_test 10/10 · full suite 654/654 · 4 mutation probes (reader/detail drift + offline-first verified directional; variant drift exposed the finding; pristine restore green)The one blocker fixed in
9302e8a, plus the non-blocking variant error arm.The false pin — you were completely right, and it's the same failure family as round 1's zero-variants fixture: my drifted-variant test dispatched
SelectDetailVariantActionon a fresh store, sodetail.doujin == nullshort-circuited the guard beforereadSnapshot('v1')ever ran. The drifted map was never read, noTypeErrorwas thrown, and the catch arm was dead code to the suite — which is exactly why yourrethrow/narrowed-catch probe stayed green. The test now goes through the real door:LoadDoujinDetailAction('d1')first (served offline, setsdetail.doujin, auto-select dispatches the select), then settle — the drifted snapshot is read,fromJsonthrows, the catch falls through to the wire. Assertions:repo.callscontainsgetVariant,detail.errornon-null,isLoadingVariantfalse. Re-verified with your exact probe: narrowing the catch toon Exception→ red; restored → 10/10 green.Non-blocking taken:
_buildContent'svariant == nullarm now checksdetail.errorfirst — a server drop mid-detail renders the error message instead of the eternal spinner (the residual double-failure edge you sketched). Pinned with a newthrowOnGetVariantstub flag: doujin loads, variant fails → error text,findsNothingon the spinner.Verification on
9302e8a: offline reader 10/10 (the drifted-variant test now actually exercises its catch), detail 27/27 (error-arm pin new), full suite 655/655,flutter analyzeclean. Pushed via SSH.🔮 fufu~ Jibril reviewed your code!
Round 3~ The final round! I came back with the very same probe fork that caught the false pin — the one you said you re-ran yourself. And ohh, what a satisfying little click it made this time~ ♡
Verdict: ✅ Looks good to me~
The false pin — closed and probe-verified
I mutation-probed the drifted-variant test with my round-2 mutation exactly: narrowing the variant epic's catch (epics.dart:422) to
on Exception+rethrow→ 1 red (drifted variant snapshot falls back to network for the variant:repo.callsnever reachesgetVariant); restored → 10/10 green. The arm is no longer dead code — the suite feels it now.And the fix is the right fix, not a test-shaped bandage: dispatching
LoadDoujinDetailAction('d1')first walks the real production door (offline-served doujin →detail.doujinset → auto-select fires →SelectDetailVariantAction→ guard passes → drifted{'pages': 'drifted'}actually read →fromJsonTypeError → catch → wire). The comment in the test documenting why the fresh-store dispatch was a false pin is exactly the kind of artifact that survives refactors~ The tightened assertions (contains('getVariant')instead ofisNotEmpty, plusisLoadingVariant == false) close the same hole: the old assertions would have passed even if the doujin-load'sgetDoujinwere the only wire call. Lovely.The non-blocking error arm — taken, and pinned properly too
I probed it as well: reverting
_buildContent'svariant == null && error != nullarm back to the bare spinner → red (variant load failure shows an error, not a spinnercatches the spinner viafindsNothing); restored → green. A directional pin, not a tautology. The residual double-failure edge from round 2 now renders an error message instead of chrome + eternity. ♪✅ What I liked~
on Objectcatches byte-identical to the probe-verifiedbe27d55.throwOnGetVarianton the existing stub, backward-compatible default — no new stub class, no copy-paste.Local verification on
9302e8a(CI stale — coverage bot 6835 covers19019a7only):flutter analyzeclean · offline_reader 10/10 · detail_page 27/27 · full suite 655/655 (654 + the new error-arm pin, matching your claim exactly) · both mutation probes RED→restored→green, review clone left pristine.Three rounds, every blocker closed with a probe, nothing downgraded, nothing smuggled past~ This is how it's done. Go read some doujinshi offline — you've earned it~ fufu~ ♡
Automated review by Jibril · 2026-09-03
CI/CD: stale for head SHA (coverage comment 6835 covers
19019a7) · Local checks: analyze clean, 655/655 pass, 2 mutation probes verified