feat: offline page — downloads manager, offline library, download button (offline phase 3/6) #86
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/offline-page"
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 3 of 6 of the offline/download feature. Phases 1 (#84) and 2 (#85) merged; this adds the UI: the
/offlinepage (Downloads + Library tabs — the locked design decision), the rail destination, and the per-variant download button on the detail page. Phase 5 (offline reader fallback) and 6 (docs) follow.What's in
OfflineLibraryPage(/offline, rail destination "Offline" between Editor and Tags):TabBarDownloads/Library per decision A. NamedOfflineLibraryPage— the manifest entityOfflinePage(phase 1) owns the short name.doujinTitle — variantLabel, subtitle withpagesDone/totalPages+ bytes + linear progress (queued rows show a 0% determinate bar by design), failed rows show the error + retry/remove, queued rows remove. Empty-state hint teaches the detail-page button.ExpansionTiles — local cover viaFilewith the null-fallback Jibril prescribed for phase 3 (vanished/orphaned covers render the book icon, never a broken image: "absence, never corruption"); per-variant rows with pages / size (KB/MB) / date / compressed flag, Read →/read/:doujinId/:variantId, Delete with confirm dialog dispatching variant-scopedDeleteDownloadAction; client-side title filter; storage footer (Σ bytes)._OfflineDownloadButtonunder the variant info line — three states from the offline slice + settings:Download(enqueues;maxKbfrommaxImageSizeKb— the locked quality decision),Downloading…(disabled + spinner while the variant is queued/running),Available offline(check + label whenhasVariant). Title from the sametitles.firstconvention as the editor pane.Verification
maxKbpassthrough and the in-queue button state; downloaded →Available offline, no download button).flutter analyze: no issues.pumpAndSettletime out — the known mid-flight pattern), tab switches usehitTestable()finders (TabBarView keeps neighbors offstage where taps can't land), 500 ms of animation pumps per tab switch.tapping a page thumbnail opens the readernowensureVisibles its chip first — its assertion (?page=2) is unchanged and still passes.Not in scope (deliberately): phase 5's offline reading — the Read button routes to the normal reader; the snapshot fallback lands next phase.
Flutter Coverage
Total: 78.1% (6991 of 8954)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! The offline page is here! Phase 3 — the part users actually touch! Tabs, queues, covers, a little download button~ I read all 979 lines, chased every wire into phases 1–2 (reducer, epics, service, store), and ran your full suite plus two probe experiments of my own. The structure is lovely, scarlet… but one of my probes caught something red-handed ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
app/lib/presentation/pages/offline/offline_page.dart:357—_LocalCovercan never render a real cover. The manifest-relative path is fed raw toFile().The manifest contract (phase 1) stores
coverPathrelative:writeCoverreturns'covers/<id>.webp'(filesystem_offline_store.dart:326), and the store joins all its own IO against_baseDir(_sub(...), baseDir =<config>/offline, wired in main.dart:40). The presentation layer never receivesbaseDir— I grepped; nothing outsidemain.dartand the repo interface touches it. SoFile('covers/d1.webp').existsSync()resolves against the process CWD → always false → every single tile takes the book-icon fallback. Probe-verified, not theorized: I round-tripped a real cover throughFilesystemOfflineStore+mergeDoujin+readManifest, then did exactly what_LocalCoverdoes —coverPath='covers/d1.webp', UIFile().existsSync()→ false, base-dir-joined path → true. The entireImage.filebranch is dead code in production. Fufu~ you wouldn't ship a cover feature that never shows a cover, would you? ♡Fix: the store already exposes
baseDir(offline_repository.dart:63) — carry it into state (e.g. onOfflineLibraryLoadedAction→OfflineState.baseDir) and join in_LocalCover(File(p.join(baseDir, path))). Keep the manifest itself relative (it's the portable on-disk shape).**
app/lib/presentation/pages/detail/variant_tabs_panel.dart:282 —inQueuecounts *failed* items; one failed download bricks the button as “Downloading…” forever.**queue.any((i) => i.variantId == variant.id)matches every status, but the queue deliberately *retains failed items for retry* (state contract + your own retry/remove UI). After a failure:inQueuestays true → disabled spinner button,onPressed: null— the detail page offers no re-enqueue, and the PR body's own spec (“disabled + spinner while the variant is **queued/running**”) is not what the code does. Neither detail test seeds a failed queue item, so the wrong branch ships untested. Fix:.any((i) => i.variantId == variant.id && (i.status == DownloadStatus.queued || i.status == DownloadStatus.running)), plus a test: queue holds a failed item for v1 → button shows enabledDownload, notDownloading…`.**
app/lib/presentation/pages/detail/variant_tabs_panel.dart:344-348 — the title snapshot contradicts the AppBar directly above it.**_primaryTitlereturnstitles.first(citing the editor pane — I saw the PR body~). But the *host detail page* showsprimaryTitle(doujin)(detail_page.dart:542): kind-priority original → romaji → translated → alias. Your own test seed proves the divergence: titles =[translated "My Doujin Title", original "Original Japanese Title"]→ the AppBar above the Download button reads **“Original Japanese Title”**, while the button snapshots **“My Doujin Title”** into the manifest — and the offline Library then shows a different title for the same doujin than every other surface (library grid uses the server's kind-prioritizedprimaryTitletoo; the backend doesFirstOrDefault(Original) ?? First()). That's this-codebase's third title-selection variant, and the one it lives next to disagrees with it. Fix: hoist detail_page'sprimaryTitle` to a shared location (it can't be imported here — detail_page imports this file) and snapshot that, so what the button stores is what the user sees above it.app/test/offline_page_test.dart— the cover branches have zero coverage. The_doujinhelper acceptscoverPath, but no test ever passes one: of_LocalCover's three arms (null / orphaned-missing / existing-image) only the null arm is exercised. That gap is precisely how blocker #1 escaped — a test seeding a real file viaFilesystemOfflineStorein a temp dir (my probe is a working recipe) asserting an image renders, plus an orphan-path arm asserting the fallback, would have caught it. New branches must be pinned~ ♡**
app/lib/presentation/pages/offline/offline_page.dart:322 —context.gointo the reader breaks the sibling navigation contract.** Both existing reader entry points (_ContinueReadingButton, the page grid) usecontext.push; the reader's_close(reader_page.dart:165) relies on that:canPop → pop back to where you launched. Withgo, the stack is replaced by[reader]→canPop()false → the fallback fires and close lands on **/library/** — the user leaves the Offline page, reads, closes, and ends up on a detail page they never visited. Fix:unawaited(context.push(...))like the siblings (your router-based test stays green either way). Ifgo` was deliberate, say why in a comment — but the sibling pattern is unanimous.💡 Little ideas (non-blocking)~
OfflineLibraryLoadedActionlands;isLibraryLoadedexists in the slice exactly for that distinction (you even set it in one test). A tiny “loading” vs “empty” split would use it honestly.DownloadStatus.donearm is unreachable today (the reducer's done-path removes the item into the library) — fine for switch exhaustiveness, but worth a one-line comment so nobody hunts for the state that never comes.✅ What I liked~
hitTestable()harness discipline is exactly right — you understood whypumpAndSettlelies next to an infinite spinner, and the comment teaches the next reader. Wonderful~Automated review by Jibril · 2026-09-02
CI/CD: absent for head SHA
de2f016(no coverage bot comment) · Local checks: flutter analyze clean; full suite 640/640 pass; cover-resolution verified by probe experimentAll five blockers fixed in
b0c6f43, plus both non-blocking ideas.1. Cover resolution — you were probe-right, and it's the embarrassing kind:
_LocalCoverfed the manifest-relative path toFile()raw, resolving against CWD; theImage.filebranch was dead in production.baseDirnow flows store →OfflineLibraryLoadedAction→OfflineState, and_LocalCoverjoins beforeFile()(manifest stays relative — the portable on-disk shape, per your framing). The cover tests write a real 1×1 PNG (magick-generated, verified decodable after my hand-typed constant turned out CRC-corrupt and hung the decode) throughFilesystemOfflineStorein a temp dir and assertImagerenders; the orphan arm asserts the book-icon fallback. One harness discovery worth noting: real async file IO insidetestWidgetsneedstester.runAsync— fake-async never pumps the IO event loop, which is why my first version hung with no output at all.2.
inQueue— now counts only queued/running. Pinned: a failed item seeded through the real reducer → button shows enabledDownload, noDownloading….3. Title snapshot —
primaryTitlehoisted tocore/title_display.dart(detail_page imports it; the reader's oldshow primaryTitleimport from detail_page consolidated there too — that was a latent duplicate of the rule you flagged). The button snapshots the kind-priority title, same as the AppBar above it; the existing enqueue test now pinsOriginal Japanese Title(original-kind wins over the first-listed translated title — the exact divergence your probe described).4. Cover-branch coverage — the real-file + orphan tests above; all three
_LocalCoverarms now pinned.5.
context.push— the Read button pops back to the Offline page like every sibling entry point; comment says why.Non-blocking: loading-spinner vs empty-hint split using
isLibraryLoaded(both arms pinned with a new pre-load test); thedone-arm unreachable-comment added.Verification on
b0c6f43: page 13/13, detail 26/26 (failed-item pin new), full suite 644/644,flutter analyzeclean. Pushed via SSH.🔮 fufu~ Jibril reviewed your code!
Back so soon~? I like that. You pushed
b0c6f43and told me all five blockers were dead — so of course I didn't take your word for it. I read the full incremental diff, chased every fix into the store/epics/reducer, ran the whole suite, and then broke your code five times on purpose to see if your new tests would scream. Oh, they screamed~ ♡Verdict: ✅ Looks good to me~
All five blockers verified closed, both non-blocking ideas landed. One commit,
+269/−70, zero scope creep. This is how a review round should be answered.🔬 How I verified each fix (mutation probes, not vibes)
baseDirnow flowsstore→OfflineLibraryLoadedAction→OfflineState→_DoujinTile→_LocalCover, joined via_join(which mirrors the store's own_pathidiom at filesystem_offline_store.dart:29 — including the/→separator rewrite). The manifest stays relative; the UI layer joins. Exactly the shape I asked for. Probe: reverted to rawFile(path)→existing cover file renders an Imagewent RED. And the new test uses a realFilesystemOfflineStorewriting a real decodable 1×1 PNG into a temp dir — not a mock assuring me files exist. TherunAsyncdiscovery (fake-async never pumps real IO) is documented in your reply; that kind of harness knowledge is precious, fufu~inQueuestatus filter — nowqueued || runningonly, with the why-comment pointing at the Downloads-tab retry. Probe: reverted to the unfilteredany→failed queue item re-enables the download buttonwent RED. And that test seeds the failure through the real reducer (EnqueueDownloadAction→DownloadItemFailedAction), not a hand-built state — it pins the state machine, not a snapshot.primaryTitlehoisted intocore/title_display.dart, the detail-page copy deleted, the reader's oldshow primaryTitleimport consolidated onto the shared one (you even killed the latent duplicate I didn't explicitly flag — good instinct). The enqueue test now pinsOriginal Japanese Titleon a fixture whose translated title is listed first — the exact divergence shape from round 1. Probe: reverted totitles.first→ RED._LocalCoverarms pinned (real file →Image, orphan → book icon, null → book icon), plus the_fallbackIconextraction removing the triple copy-paste. The deadImage.filebranch is dead no more.context.push— the Read button now matches both sibling entry points (variant_tabs_panel.dart:251Continue-reading,:425page grid). Honest note: my revert probe (push→go) did not turn the test red — it only asserts arrival, like its siblings. I checked: no test in the repo pins push-vs-go back-stack semantics, so this is at exact sibling parity, not below it. The fix itself is verified by inspection; I won't demand a test the sibling family never had. (If you ever want one, a router-stub with an on-did-navigate recorder could pin the whole family at once — an idea, not a request~)Non-blocking round-1 ideas: the pre-load spinner / empty-hint split is pinned by the new
shows a loading spinner before the manifest loadstest (probe-verified RED when the arm is removed), and the unreachabledone-arm comment landed as requested.✅ What I liked~
baseDirrides the same action → reducer → slice path as everything else; no side-door path provider reaching into the widget tree from the epic. Architecture stays clean.app_state.freezed.dartis consistent throughcopyWith(sentinel-aware nullable), equality, hashCode, toString, when/maybeWhen/whenOrNull — no half-regen smell.644/644locally,flutter analyzeclean — and your comment's claims match my run exactly, which is the trust currency I care about ♪One trinket, absolutely non-blocking:
test/offline_page_test.dart:96has a stray double blank line before_tinyPng(the formatter also wants the byte-list one-per-line — I checked,26 files repo-wide are already formatter-divergent and CI doesn't enforce).dart format, so this is status quo, not a new sinPhase 5 next — the reader fallback. I'm already curious about it. Fufu~
Automated review by Jibril · 2026-09-02 22:20 CEST
CI/CD: stale for head SHA
b0c6f43(coverage bot 6820 predates the fix push; coversde2f016only) · Local checks: flutter analyze 0 issues · full suite 644/644 · 5 mutation probes (4 RED-verified, 1 sibling-parity)