Fix: solidify the download/caching/storage pipeline#70
Merged
Conversation
Behavior-preserving cleanup of the reader restore logic. - Extract the unified scroll-restore logic out of the ContentArea composable into two named functions: runScrollRestore (orchestrator) and awaitStableRestore (the watch-until-stable loop). The restore effect is now a one-line call. - Restructure the orchestrator's early returns into a single when/handled flow so each function stays within detekt's complexity limits; the loop's essential interrelated conditions carry a documented @Suppress("CyclomaticComplexMethod"). - Document every restore magic constant (poll cadence, stability window, percent tolerance, stability threshold, ghost-row percent, sentinel item size). - Add a doc block above ReaderProgressController's gating flags describing the transitions and the semi-independent axes. No runtime logic, thresholds, control flow, or ordering changed. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
ReaderViewModel aliased four ReaderProgressController fields as its own pass-through properties. Three (suppressAutoNavUntilUserInteraction, restoredScrollPercent, restoredProgressSnapshot) were never referenced; hasUserInteractedSinceLoad was read once internally and not by any UI code. Remove all four and route the single remaining read directly through the controller. The controller's own fields are unchanged (its tests use them), and the members still used by the reader UI (userHasDragged, restoreInProgress, currentLibraryItemId) stay. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
…nManager Move the resolved-image-dimension machinery out of ReaderViewModel into a new ImageDimensionManager: the fine-grained Compose state map, the prompt off-main cache flush, and the trailing-debounced content rebuild (with its jobs and pending/queued maps). ReaderViewModel keeps the ui-state-coupled content rewrite (updateCurrentContentImageDimensions / withResolvedImageDimensions) and passes it to the manager as a callback; it exposes the same public surface (resolvedImageDimensions, persistImageDimensions) by delegation, so callers and the reader UI are unchanged. resetState now calls manager.reset(), preserving the original behavior of leaving the db-flush job and pending map running. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
…entShaper WebContentLoader mixed ~130 lines of pure, stateless element shaping (long-strip detection, adjacent-image grouping, wide/double-page splitting) into its I/O and caching orchestration. Move those six functions and their two constants verbatim into a new stateless WebChapterElementShaper object; WebContentLoader now calls through it at the three existing sites. No behavior change. The moved code's pre-existing, already-baselined detekt findings (magic numbers, long lines, nesting/return-count on the grouping functions) are relocated in detekt-baseline.xml from WebContentLoader to WebChapterElementShaper — same findings, new home. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
…nager Move LibraryViewModel's selection state (_selectedItems, _selectionModeEnabled) and the toggle/select/deselect/group/all/enter/clear operations into a new LibrarySelectionManager. The ViewModel delegates and reads the manager's flows in its state combine; group- and select-all operations pass their ids in, keeping the manager decoupled from the library list and ui state. No behavior change. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
Move LibraryViewModel's pending-deletion state and the 5s undo-window logic (_pendingDeletion, the delete job, schedule/undo/commit/flush/removeImmediate) into a new LibraryDeletionCoordinator. The ViewModel delegates and reads the coordinator's pendingDeletion flow in its state combine; deletion errors surface back through a callback. No behavior change. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
Move LibraryViewModel's search/content-type/sort/status filter flows, their setters, and the pure filterAndSortItems function into a new LibraryFilters holder. The ViewModel exposes the flows by delegation and applies them via filters.apply() in its state combine. No behavior change. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
…tates Move LibraryViewModel's per-chapter download/cache badge state and its reconciliation — the queue observer, on-demand reconcile + auto-resume of incomplete downloads, and the semaphore-limited cache-state refresh — into a new LibraryDownloadStates. The ViewModel delegates the public reconcile/refresh methods, reads the manager's chapterCacheStates in its combine, and routes its three remaining setCacheState writes through it. No behavior change. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
With selection, deletion, filters, and download/cache-state extracted into their own collaborators, LibraryViewModel's class body is now under detekt's LargeClass threshold, so the suppression it carried is no longer needed. Verified: ./gradlew detekt (clean, no LargeClass).
Drop pre-Room content/session persistence that nothing reads anymore: saveParagraphs/loadParagraphs, saveLibraryItems, the currentUrl / currentTitle / scrollPosition properties, clearContent, and their now-orphaned keys. loadLibraryItems stays (LibraryRepository still uses it for one-time migration to Room). With the dead functions gone, PreferencesManager is under detekt's TooManyFunctions threshold, so its @Suppress is removed too. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
Delete confirmed-unused private members that were baselined rather than removed: nine dead regexes in TextUtils (one a duplicate of MULTIPLE_SPACES_REGEX), HtmlParser's unused WHITESPACE_REGEX, ReaderViewModel's unused MIN_SCROLL_OFFSET_DELTA_PX constant and isPlaceholderAtCurrentPosition (a duplicate of the live one in ReaderProgressController), and MangaBatSource's unused fetchChaptersFromApi fallback. Regenerated detekt-baseline.xml to drop the 13 now-stale entries. Verified: ./gradlew testStandardDebugUnitTest detekt (349 tests, 0 failures, detekt clean).
…ailures WebOfflineChapterStore refused zero-image chapters (clear + retryable), so text-only novel chapters could never finish downloading and WorkManager retried them forever; permanent 4xx image failures were never recorded on the download path, so dead-image chapters also retried forever. - Classify zero-image documents via a new ChapterDocumentClassifier (extracted detectMangaReaderHints + isRenderableTextChapter): renderable text chapters persist a complete empty-images manifest; JS shells return the existing inspect state instead of clearing a good download. - Record permanent image failures into PermanentFailureStore from downloadMissingImages, classifying via ImageFetchResult.isRetryable(). - inspect() now accounts fresh permanent failures toward isComplete (with hasPermanentFailures=true, isRetryable=false) so such chapters go terminal instead of retrying; cachedImages stays the on-disk count. - validateManifestFiles accepts empty image lists so loadContent serves text chapters offline. Manifest schema unchanged (v1). New WebOfflineChapterStoreTest covers text chapters, shells, refetch safety, permanent/retryable failures, inspect accounting, and legacy manifest compat.
The retryable branch of ChapterDownloadWorker returned Result.retry() unconditionally — MAX_RUN_ATTEMPTS only guarded the exception path — so a chapter that could never complete retried forever. The final reconcile also used the worker's in-memory result, so a remove-download that raced a finishing worker could re-promote isDownloaded for files just deleted. - Cap the retryable path at MAX_RUN_ATTEMPTS, then fail with terminal data. - Reconcile from a fresh inspectDownload() so the DB flag tracks on-disk reality; the returned WorkManager Result still maps from the in-memory result so a post-removal empty inspect can't trigger a re-download. Worker tests updated to consecutive inspectDownload stubbing; new tests for attempt-budget exhaustion and cleared-disk non-promotion.
Since the offline store took over user downloads, executePrefetch's USER_REQUESTED mode early-returns through offlineChapterStore; the 2-pass cacheImages(DOWNLOADS) loop, permanent-failure recording, and the persistentOnly inspect below it were unreachable, and the document fetch still wrote raw HTML into the persistent downloads dir on every download. - Fetch HTML with writeTier=CACHE for both prefetch modes; the manifest store is the sole persistent artifact for downloaded chapters. - Delete the dead members: cacheImages, ImageBatchDownloadReport, ensureResultForTier, the DOWNLOADS-promotion branch and writeTier param of downloadAndCacheImageInternal, tierForMode, recordPermanentFailures, loadPermanentFailures, migrateLegacySidecarIfPresent, and the pass/ concurrency constants. - inspectCacheInternal loses persistentOnly and the cache-side permanent- failure accounting; cache inspects now always report a non-persistent, failure-free view (downloads-tier truth lives in the offline store). detekt baseline regenerated: entries for deleted/shrunken members removed, none added. New test asserts user downloads leave the html downloads dir empty.
…kInfos The badge map merged every observeAll() emission last-writer-wins. WorkManager keeps finished records for days, so a stale SUCCEEDED record could resurrect a deleted chapter's Downloaded state and a data-less CANCELLED record (defaults isPersistentDownload=true) could wipe a valid one; both raced the disk-inspect refresh. Optimistic isInProgress writes had no rollback and auto-resume re-enqueued without limit from a stale DB snapshot. - Queue observer merges only in-progress states; an in-progress→terminal transition triggers a disk inspect for those urls — the inspect is the only writer of terminal states. Non-in-progress writes are dropped for urls with live queue work (both directions of the race). - enqueue() now reports failure; new markPendingAndEnqueue() rolls the optimistic state back when enqueueing fails. New removeCacheStates() for deletion cleanup. - Auto-resume: capped at 2 attempts per session, skips terminal permanent-failure results, and re-checks the item still exists with a fresh DB read before each enqueue. - Prune finished WorkManager records once at app startup so the first observeAll emission isn't a graveyard of stale terminal states. New LibraryDownloadStatesTest covers the arbitration, rollback, caps, and cleanup.
…queues Deleting chapters left their queued downloads running and their badge-map entries in place (stale state that resurfaced if the chapter was re-added), and removeDownload left the old downloaded badge until some unrelated refresh. The three optimistic enqueue sites could strand a permanent spinner when enqueueing failed. - LibraryDeletionCoordinator reports successfully removed urls; the ViewModel cancels their queued work and drops their cache states. - removeDownload refreshes the chapter's cache state after demoting the flag so the UI reflects not-downloaded immediately. - addChapters, prefetchLibrary, and retryDownload route through markPendingAndEnqueue (rollback on failure); user-initiated paths surface a queueing error.
Nothing writes downloads/html or downloads/media anymore — downloaded web chapters live entirely in the web_chapters_v2 manifest store. Bump the web offline pipeline version to 3: installs at v2 get a light sweep that deletes the orphaned legacy dirs (manifests, DB flags, and queue state untouched); pre-v2 installs keep the existing full reset. clearDownload drops the Jsoup re-parse used to delete legacy per-image files and keeps only the manifest clear, cheap legacy html/.failed deletes, memo eviction, and permanent-failure clearing.
Downloaded chapters serve images as file:// URIs rewritten by the offline manifest store, but ReaderImageTileFetcher.resolveFile only knew the URL-hash media-cache lookup and a network fallback — both dead ends for a file URI. Every tall strip (anything past the 2048px tiling threshold) rendered its slices black in airplane mode; only short untiled images, which go through Coil's built-in file fetcher, survived. Resolve file: URIs directly to the local file (when present and non-empty) before the cache/network path. New ReaderImageTileFetcherTest covers the file-URI fast path, the missing-file fallback, and the unchanged http path.
ImageBoundsParser handled only the VP8X and VP8L WebP chunk variants, so plain lossy WebP — the most common flavor on manga CDNs — stored 0x0 dimensions in offline manifest records. Zero dimensions make readerImageSliceCount fall back to a single slice, so tall strips skip the tiled renderer and decode as one giant bitmap, reintroducing the software-draw scroll stutter tiling exists to avoid. Add a 'VP8 ' branch that validates the keyframe start code and reads the 14-bit little-endian dimensions. New ImageBoundsParserTest covers lossy VP8 (incl. start-code rejection and dimension masking) plus regression cases for VP8X, VP8L, PNG, JPEG, and unknown payloads. Already-stored 0x0 records self-heal at the reader layer once an image decodes (the dimension manager persists real sizes), so no manifest migration.
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
1 participant
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.
Fixes downloads lying about downloaded chapters and general unreliability. Stacked on #69 (architecture cleanup) — merge that first; this PR's own commits start at the offline-store fix.
Root causes fixed
Changes
Verification
384 tests / 0 failures (35 new), detekt clean with baseline entries only removed, assembleStandardDebug + assembleAiDebug green. Manually confirmed offline manhwa reading renders all strips.