feat: library refresh, created-date ordering, search-mode pagination #63
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/library-refresh-order-pagination"
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?
Three library-view improvements:
1. Auto-refresh + refresh button
LibraryState.lastLoadedAtrecords when the current result set finished loading. The timestamp rides onDoujinsLoadedAction(defaulting to construction time, injectable in tests) so the reducer stays pure.kLibraryAutoRefreshAfter(5 min,constants.dart) automatically re-runs the current view's query. App resume is detected viaWidgetsBindingObserver.2. Order by created date desc
DoujinService.ListAsyncandSearchServiceordered byUpdatedAtdesc, so editing metadata bumped a doujin to the front of the library. Both now order byCreatedAtdesc (newest additions first) with anIdtiebreak. Side benefit: infinite-scroll pages no longer reshuffle when something is edited mid-scroll.3. Search-mode pagination
Infinite scroll paged correctly in browse mode, but in search mode the load-more epic called
listDoujins— silently dropping every active filter for pages 2+ — and search itself never sent a page number. The load-more epic is now search-aware: it re-runs the current filter query with the next page number and the result set's page size (buildSearchQuerygainedpage/pageSize).Test changes
The library epic tests previously asserted against hand-maintained mirror copies of the production epics — changes to the real epics were invisible to them. They now exercise the actual production code via
...ForTestwrappers (matching the existingbuildSearchQueryForTestconvention), and the mirrors are deleted. New coverage: search-mode paging preserves filters, paging failure surfaces the error banner, stale-open refresh, fresh-open no-op, app-resume refresh, refresh button in both modes,lastLoadedAtreduction.Verification
flutter analyze: cleanflutter test: 464/464 pass (was 457)dotnet test: full backend suite passes (368 tests)🤖 Generated with Claude Code
Flutter Coverage
Total: 73.5% (5757 of 7829)
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 85.7%
DoujinManager.Infrastructure - 94.5%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 85.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! leans forward, eyes sparkling A three-for-one special~ Library refresh, CreatedAt ordering, AND the search-pagination bug? The load-more epic was calling
listDoujinsin search mode this whole time, silently dropping every filter for pages 2+? Fufu~ that's a delicious bug to squash, and the fix is chef's kiss — branching onisSearchModeand piping through_buildSearchQuerywith the right page number. I'm genuinely impressed~ ♡And deleting those hand-maintained mirror epics in favor of
...ForTestwrappers? fans self Now the tests actually exercise production code instead of a parallel universe that drifts on every refactor. That's a maintainability win that made my knowledge-loving heart sing~Verdict: ⛔ I can't let this pass~ ♡
⛔ This needs fixing before I'm satisfied~
app/lib/presentation/pages/library/library_page.dart:169— The refresh button's disabled-state is inconsistent with the auto-refresh path, and the gap can corrupt the list.The button guards on
isLoadingonly:But the auto-refresh path (
_refreshIfStale, line 80) correctly guards on both:This isn't just a style nit — it's a real race. If infinite scroll triggers a page-2 fetch (
isLoadingMore: true) and the user taps refresh before it completes,_refresh()dispatchesLoadDoujinsAction/SearchDoujinsAction(page 1). Now two epics race. If the page-1DoujinsLoadedActionlands first (replacing the list), the in-flightDoujinsAppendedActionfrom the old page-2 fetch lands second — appending stale page-2 items from the old result set to the new page-1 list. The reducer forDoujinsLoadedActiondoesn't resetisLoadingMore, so the stale append persists until the next load.Fix: make the button match the auto-refresh guard —
💡 Little ideas (non-blocking)~
backend/src/.../SearchService.cs:258+DoujinService.cs:42— TheUpdatedAt → CreatedAtordering swap is the right call (editing metadata should NOT reshuffle the library), but no test actually pins it.SearchIntegrationTests.AssertIdsis explicitly order-independent (line 254 comment), andDoujinService.ListAsynchas no integration test at all. A future refactor could swap it back toUpdatedAtand every test would stay green. Consider adding a directional test: seed two doujins with distinctCreatedAt, edit the older one'sUpdatedAtto be newer, assert the older-by-creation one still sorts first.✅ What I liked~
_buildSearchQueryparameterization is perfect — defaults (1/20) matchSearchQuery's own, so the initial-search path is zero-drift, and the load-more path just pipes throughnextPage/pageInfo.pageSize. Clean~ ♪lastLoadedAtriding onDoujinsLoadedActionwith an injectableloadedAtdefault keeps the reducer pure while making the timestamp testable. That's the kind of architectural care that makes me happy~WidgetsBindingObserverlifecycle is flawless:addObserverininitState,removeObserverindispose,mountedcheck in_refreshIfStale. Every box ticked.pages the search query in search modeassertslistDoujinsCalls == 0ANDsearchCalls == 1AND the query carries the right page/filters. If someone deletes the search branch, this goes red. Fufu~ that's how you write a regression test~ ♡DoujinService.ListAsyncgaining the.ThenBy(d => d.Id)tiebreak to matchSearchService— stable pagination, no reshuffling on equal timestamps. Important detail, well done.Automated review by Jibril · 2026-08-13
CI/CD: absent for head
451a186(PR just opened, 0 comments) · Local checks: flutter analyze 0 issues, flutter test 464/464 pass, dotnet build 0 errors, dotnet test 368/368 pass (60+1+307)@jibril Good catch on the race — fixed in
acb04ca, and I went one level deeper than the suggested guard:Blocking finding: The button now uses the same
(isLoading || isLoadingMore)guard as the auto-refresh path, as you suggested. But the button was only one trigger — the debounced filter epic dispatchesSearchDoujinsActionwithout checkingisLoadingMoreeither, so committing a filter token mid-scroll had the exact same stale-append race. So the reducer is hardened too:DoujinsLoadedActionnow clearsisLoadingMore(the point you flagged in passing), andDoujinsAppendedActionarriving with no pending load-more is dropped — it can only be a stray from a superseded result set.That closes the race for every reload path regardless of ordering: if the stale append lands before the reload it gets replaced wholesale; if it lands after, it's dropped. New reducer tests cover both, plus a widget test asserting the button is disabled while a page fetch is in flight.
Non-blocking finding: Also taken —
DoujinOrderingTests(Infrastructure.Tests, in-memory SQLite) seeds two doujins whoseCreatedAtandUpdatedAtorder disagree (older-created one freshly edited) and asserts bothListAsyncandSearchAsyncput the newer-created one first. A revert toUpdatedAtordering goes red in both tests. Done at the service level rather than inSearchIntegrationTestsbecause the API-seeded fixtures can't controlCreatedAt.Verification on
acb04ca: flutter analyze clean, flutter test 467/467, dotnet test 386/386 (16+1+62+307).🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
tilts head, eyes narrowing with delight Oh? Oh my~ You went deeper than I asked. I flagged one button guard and you rewired the entire race surface? Fufu~ that's the kind of thoroughness that makes my heart flutter~ ♡
Let me trace through the fix carefully...
Verdict: ✅ Looks good to me~
⛔→✅ Round-1 blocker — CLOSED
The refresh button now guards on
(isLoading || isLoadingMore), matching_refreshIfStaleexactly. Good~But you didn't stop there. You realized the button was only one trigger — the debounced
_filterTokensChangedEpicdispatchesSearchDoujinsActionwithout checkingisLoadingMoreeither, so committing a filter mid-scroll had the exact same stale-append race. sharp smile That's the kind of root-cause thinking I live for~The reducer hardening is elegant:
DoujinsLoadedActionnow clearsisLoadingMore— so any in-flight page fetch that started before the full reload has itsDoujinsAppendedActionrecognized as a stray.DoujinsAppendedActiondropped when!isLoadingMore— the stray can't pollute the fresh page-1 list.This closes the race for every reload path regardless of interleaving order. If the stale append lands before the reload, it gets replaced wholesale. If it lands after, it's dropped. Fufu~ defense in depth done right~ ♪
⛔→✅ Round-1 non-blocker — CLOSED
DoujinOrderingTestsis exactly what I asked for, and then some:CreatedAtandUpdatedAtorder disagree (older-by-creation one freshly edited — UpdatedAt 2026 vs CreatedAt 2024)ListAsyncANDSearchAsyncput the newer-created one firstUpdatedAtordering goes red in both tests — genuinely directionalEnsureCreated(), properIDisposable— no shared state leakage between testsSearchIntegrationTestsbecause the API fixtures can't controlCreatedAt— correct call, well reasoned✅ What I liked~
// Same guard as the stale auto-refresh...) names the why, not just the what. Future maintainers won't accidentally simplify it back~refresh button is disabled while a page fetch is in flight) dispatchesLoadMoreDoujinsActionwith only 1 total page, so the epic no-ops but the reducer flagsisLoadingMore— then asserts the button'sonPressedisnullAND that tapping it makes zero repo calls. Two-pump timing comment is honest about the async stream delivery~DoujinsAppendedAction is dropped when no load-more is pendingseeds a realDoujinSummaryand a stale one with a distinct id, then asserts the list is unchanged ANDpageInfois stillnull. If someone removes theif (!state.library.isLoadingMore) return state;guard, the stale item appears and this goes red~DoujinOrderingTestscorrectly usesNullLogger<DoujinService>.Instancematching the primary constructor signature, andSearchServicetakes onlydb— both constructors verified against sourceAutomated review by Jibril · 2026-08-14
CI/CD: stale for head
acb04ca(coverage bot 6193/6194 covers prior451a186only) · Local checks: flutter analyze 0 issues, flutter test 467/467 pass, dotnet test 386/386 pass (16+62+1+307)