feat: Phase 10g — read-only doujin detail page #33
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/flutter-detail"
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 10g: Doujin Detail (Read-Only)
Two-column desktop layout for viewing full doujin metadata. Read-only — no editing, no upload.
Layout
State
DetailState(isLoading, error, doujin) added to AppStateLoadDoujinDetailAction,DoujinDetailLoadedAction,ClearDetailAction,DetailErrorAction_loadDetailEpic—GET /api/doujins/{id}viaDoujinRepository.getDoujinVerification
flutter analyze: 0 issues ✅flutter test: 180/180 passed (15 new + 165 existing) ✅Two-column desktop layout for viewing doujin metadata: - Left (40%): titles, StarRating display, reading direction, description, tag chips, people grouped by role, circles - Right (60%): variant cards (language, kind, page/chapter counts, default badge) State: - DetailState (isLoading, error, doujin) in AppState - LoadDoujinDetailAction/DoujinDetailLoadedAction/ClearDetailAction /DetailErrorAction - _loadDetailEpic: GET /api/doujins/{id} via DoujinRepository.getDoujin - Reducers for all detail actions Navigation: - Dispatches LoadDoujinDetailAction on init, ClearDetailAction on dispose - Store reference cached in didChangeDependencies to avoid deactivated widget lookups in dispose 15 new tests (4 reducer, 9 widget, 2 epic), 180 total pass. flutter analyze: 0 issues.Flutter Coverage
Total: 78.0% (1235 of 1584)
🤖 Hermes automated review: minor comments
Reviewed the full diff (+1357/-22, 7 files, single commit
0f37227onfeat/flutter-detail→main). Read-only detail page wiring into the existing Redux store (actions → reducers → epic →DetailState), the two-column Flutter layout, and the 15 new tests.No blocking issues found. State management, error handling, and lifecycle (store-reference caching +
ClearDetailActionon dispose) are all correct. Three minor non-blocking notes:secondaryTitles()is value-based, not identity-based —app/lib/presentation/pages/detail/detail_page.dart(secondaryTitleshelper). It drops the first title whosevaluestring equals the primary title. If two titles of differentTitleKinds share identical text (e.g. an original and a translated title that happen to be the same string), only the first is dropped and the duplicate text would appear a second time in the secondary list. Narrow edge case; no data loss. Consider matching by kind+id, or deduplicating by value, if exact title dedup matters.No retry on the error view —
_DetailErrorView(detail_page.dart). For a read-only page a transient network error (the epic dispatchesDetailErrorActionand the store slice holds the message) leaves the user on a static error screen with no "retry" affordance — recovery requires navigating away and back sodidChangeDependenciesre-firesLoadDoujinDetailAction. Clearly intentional for this phase; worth a retry button or a pull-to-refresh when the page gains interactivity.Epic test mirrors production code instead of importing it —
app/test/detail_page_test.dart(loadDetailEpicForTest). The test re-declares a copy of_loadDetailEpic's body rather than exercising the registered production epic (contrast withbuildSearchQueryForTest, which is exported fromepics.dartfor exactly this purpose). If the production epic drifts from the mirror, the test would still pass. Minor; consider exporting_loadDetailEpicfor test reuse.Verification
0f372279(posted 14:38:14 +02:00, ~1 min after PR open;flutter-ci.ymlposts coverage only on successfulflutter analyze+flutter test). New file coverage:detail_page.dart92.7% (202/218),reducers.dart100%,star_rating.dart100%,epics.dart87.9%. Local build/test skipped per CI-evidence policy.eval/exec,pickle, or SQL injection in added lines. (Note:action.authTokeninepics.dartis a runtime value, not a hardcoded secret.)mergeable: true**,merge_base` equals base sha (clean, no drift).Automated daily review. I never merge PRs.
It feels to me that app/lib/presentation/state/actions.dart is becoming very large. Looking forward to the editing, it is very likely that this will only grow more and more, specifically once we go to the editor. Could you spread the action definitions out into individual files for each logical unit, e.g. general navigation and error, settings, library view, detail view, ...? I think that would make this a lot better handable
And also take a look at the reducer as well. I think the same concern is also true there as well.
Good call — I'm on it. Splitting both
actions.dartandreducers.dartinto per-domain files with barrel re-exports so existing imports keep working unchanged.Structure:
actions.dartandreducers.dartbecome barrel files that re-export everything. This prep for the editor phase cleanly.Done — commit
ac027e8:Split into per-domain files:
actions/: settings, library, metadata, filter_token, detailreducers/: settings, library, metadata, detailactions.dartandreducers.dartare now barrel files that re-export everything. All existing imports keep working unchanged — no test or page files modified.180 tests pass, 0 analyze issues.
Done — merged filter token actions into
library_actions.dart(ddf034b). Now action and reducer files match 1:1:settings_actions.dartsettings_reducer.dartlibrary_actions.dartlibrary_reducer.dartmetadata_actions.dartmetadata_reducer.dartdetail_actions.dartdetail_reducer.dart180 tests pass, 0 analyze issues.
🤖 Hermes automated review: follow-up (minor comments)
Reviewed the incremental diff since my last review (head
0f37227→ddf034b, 3 commits:554e671inject.py,ac027e8actions/reducers split,ddf034bfilter_token merge). This addresses @bjoern's request to split the largeactions.dart/reducers.dartinto per-domain files.Refactor verification — faithful, no regressions ✅
I verified the split moved every declaration with zero loss:
is XActioncases (new)Both totals match the pre-split monolithic files exactly (30 action classes, 30 reducer cases). The
appReducerbarrel keeps the same first-non-null chain (settingsReducer ?? libraryReducer ?? metadataReducer ?? detailReducer ?? state). Theddf034bmerge offilter_token_actionsintolibrary_actionswas clean — no orphanedfilter_token_actions.dartremains, and all 3 filter-token action classes (AddFilterTokenAction,RemoveFilterTokenAction,SetFilterTokensAction) are present inlibrary_actions.dart. Barrel re-exports preserve all existing imports.Findings (1 new minor, non-blocking)
scripts/inject.pyleaks file handles on upload failure —scripts/inject.py:87-94. The page-upload loop opens N image files withopen(img, "rb")(line 87) into a list, but the cleanup loop that closes them (line 100) only runs on the success path. Ifresp.raise_for_status()at line 94 throws (upload HTTP error, network failure, etc.), the exception propagates and theforloop at line 100 is skipped, leaking all N file descriptors. This is a dev-only utility script, so impact is low, but the fix is a one-liner: wrap the upload +raise_for_statusintry/finally, or use a context manager /ExitStack. Suggested:Status of prior findings (from comment #538)
All 3 prior minor notes were about the original detail-page feature, not the refactor. They remain valid but non-blocking and are unaffected by this refactor:
secondaryTitles()value-based dedup edge case — still applies._DetailErrorView— still applies (read-only phase).Verification
ddf034b(updated 15:20:42 +02:00, ~43s after head commit at 15:19:59;flutter-ci.ymlposts coverage only on successfulflutter analyze+flutter test; 180/180 tests). New per-domain files appear in the coverage report (e.g.detail_actions.dart100%,library_reducer.dart100%,settings_reducer.dart100%). Local build/test skipped per CI-evidence policy.eval/exec,pickle, or SQL injection. TheAUTH_TOKEN="your-token-here"ininject.pyis a literal placeholder, not a real credential (verified via raw byte inspection).mergeable: true;merge_baseequals base sha (no drift).Automated daily review. I never merge PRs.
Scans a folder: root-level images go into the variant directly, each subfolder with images becomes a chapter. Creates doujin + variant + chapters, uploads all images as pages. Note: pages are uploaded without chapter assignment (the current API endpoint POST /api/variants/{id}/pages passes null for chapterId).The variant creation was sending {"title": ...} but the DTO expects languageCode (mandatory), displayName, and isDefault.The upload endpoint returns UploadPagesResponseDto { pages: [...], count: N } but the exact JSON field might differ. Now tries 'pages' list length, then 'count' field, then falls back to the number of files sent.