feat: Phase 10f — library browse and search #32
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/flutter-library"
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 10f: Library Browse and Search
Functional library browse and search screen — the first useful screen of the app.
State layer
Wiring
createStoreacceptsDoujinRepository, injects intocreateAppEpicAppDependenciescreatesDoujinApiRepositorywhen configured,_ThrowingDoujinRepositorywhen notLibrary page (desktop-native per ADR 0019)
/library/:doujinIdVerification
flutter analyze: No issues found ✅flutter test: 95/95 passed (56 existing + 39 new) ✅Flutter Coverage
Total: 74.4% (1002 of 1346)
🤖 Hermes automated review: minor comments
Reviewed the full diff (+2687/-69, 17 files) against
7cf332b8...757166e9. Static security scan clean (no secrets, shell injection, eval/exec, pickle, or SQL injection in added lines). State architecture, epics, reducers, and DI wiring are solid — proper error handling throughout, pure reducers, correct pagination logic.Findings (all minor, non-blocking)
1.
ClearSearchActionwipes browse data, but browse won't auto-reload —library_page.dart:~230+ reducers.dartClearSearchActioncaseClearSearchActionresets the entireLibraryStateviaconst LibraryState(), which clearsdoujinsandpageInfoalong with the search filters. Since_autoLoadIfNeeded()has_didAutoLoad = trueafter the initial load, switching back to browse mode after Escape/Clear shows an empty "No doujins found" state with no way to trigger a reload short of navigating to Settings and back.The reducer test confirms this is intentional (
expect(result.library.doujins, isEmpty)), but the UX gap is real. Suggested fix: dispatchLoadDoujinsAction()afterClearSearchAction(e.g., in the Escape handler and Clear button), or have the reducer only reset search-specific fields instead of the wholeLibraryState.2.
searchTextController.textmutated insidebuild()—library_page.dart:~152(_SearchPanel.build)Flutter recommends against mutating
TextEditingControllerduringbuild— it can trigger extra layout passes. This pattern works today (the guard prevents infinite loops), but consider moving the sync todidUpdateWidgetor aStoreConnector/listener.3. Shared
ScrollControlleracross twoGridView.builders —library_page.dart:~140(IndexedStack)A single
_scrollControlleris passed to both_BrowseContentand_SearchContent, each rendering aGridView.builderwith that controller.IndexedStackbuilds all children, so bothGridViewwidgets are in the tree simultaneously when data is loaded. While CI tests pass (95/95), this is a fragile pattern —ScrollControlleris designed for a single scroll position. If both grids ever render data at once, Flutter's "multiple scroll positions registered" assertion could fire. Consider giving each content view its own controller, or usingPrimaryScrollControllerwith different keys.4. Epic tests use mirror copies instead of production code —
library_epics_test.dart:~200-270The test helpers re-implement
_loadDoujinsEpic,_loadMoreDoujinsEpic, and_searchDoujinsEpiclocally rather than exercising the production functions. If production epics diverge from the mirrors, tests would still pass. Consider testing via a realStore+EpicMiddleware(createAppEpic(...))to close this gap.5. Missing test coverage for scroll/infinite-scroll and keyboard shortcuts — No test exercises
_onScroll,LoadMoreDoujinsActiondispatch from the scroll handler, or the Ctrl+F/Escape keyboard handler. These are interactive behaviors that warrant widget tests.Verification
CI/CD: Forgejo Actions coverage comment #500 is current for head
757166e9(71.1% line coverage, 730/1027 lines, posted 02:32:36 UTC). Theflutter-ci.ymlposts coverage only on successfulflutter analyze+flutter test, and the PR body reports 95/95 passed. Local build/test skipped per CI-evidence policy.Automated daily review. I never merge PRs.
I feel the implementation leaves a lot to be desired.
"english" -> positive language added
"none" -> positive language added
"gender bender" -> positive tag added
"-maid" -> negative tag added
"arata" -> positive artist added
"when I woke up I" -> general ttitle text filter (when no language or tag or artist or circle matches default to normal filter"
All the tags are then send and combined down. The search bar could on every button press update in real time to suggest known tags, languages, artists/person and circles, but explicitly not suggest titles.
With that, we only have one text field with global search power, and fever clicks do to anything. The third bar (with wrapping) becones simply a list of tag widgets with a little x button to remove it again. And instead of having a dedicated search button, why not already start async filtering whenever a filter criteria changes (added, removed, rating changed)
You're right on all three points — this implementation is too stiff and form-heavy for a desktop kiosk app. Let me address each:
1. Unified browse+search — Agreed. Having a separate "Search" tab is redundant when the library should always be the main view. The search bar should be integrated at the top of the library screen, always visible, filtering the grid below in real-time. I'll merge them into one screen with the NavigationRail items becoming: Library (the unified view), Upload, Settings.
2. Star rating instead of dropdowns — Agreed. A 5-star widget with half-star support maps cleanly to the backend's 1-10 rating range. Much more visual and fewer clicks. I'll build a reusable
StarRatingwidget.3. Unified search input with type detection — This is the big one, and it's a great idea. Instead of 5 separate form fields:
english,none,ja, etc.) → detected as language filter-maid→ negative/exclude tagThis is a significant rework of the library page. I'll also fix the automated review findings (#1-5) in the process.
Plan:
StarRatingwidget (reusable, half-star support, maps to 1-10)FilterInputwidget — smart token detection + autocomplete + chip displayWant me to proceed with this rework on this PR's branch, or do you want to discuss any of the details further first?
As discussed in the chat, I would like you re-implement the UX with my suggestions.
3 commits pushed addressing the feedback:
Commit 1 — Backend: personNames + circleNames search (
c053946)Added name-based search for persons and circles alongside existing ID-based filters. Case-insensitive matching against DisplayName. 14 new backend tests.
Commit 2 — Reusable StarRating widget (
2e20e97)5-star widget with half-star precision, hover preview, maps to backend 1-10 rating. Desktop-native 18px stars. Hover tests skipped in headless CI.
Commit 3 — Merged search into main library screen (
d7869da)Removed separate Search tab. Search bar always visible at top of the grid. Fixed all 3 automated review issues (ScrollController, build() mutation, ClearSearchAction data wipe). NavigationRail now just [Library, Upload, Settings].
Next: Building the smart unified filter input — one text field with token type detection, chip display, live debounced filtering. This is the subagent #4 work that ties it all together.
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 85%
DoujinManager.Infrastructure - 91.7%
pshot
DoujinManager.RestAdapter - 84.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 21.8%
Commit 4 — the smart unified filter input — is pushed (
74aaeea). This ties everything together:SmartFilterBar widget:
-tag→ exclude tag, known artist/circle names → person/circle filter, anything else → text searchState + data:
personNames/circleNamesfieldsVerification:
flutter analyze: No issues found ✅flutter test: 113/113 passed ✅dotnet test: 263/263 passed ✅ (backend name search)All 4 subagent tasks complete — the library page now has the unified kiosk-style search UX per your feedback.
Final commit pushed (
b35dcbb) — added 52 tests for the smart filter system:Final counts:
All 6 commits on this PR:
c053946— Backend: personNames + circleNames search2e20e97— StarRating widgetd7869da— Merge search into main library screen74aaeea— Smart unified filter inputb35dcbb— Filter token/reducer/epic testsReady for re-review.
General question: Is it necessary for us define the languages in the client? What happens if the list diverges from future server versios? If anything, if we want to define these in the client, which might be fair, we should do it in a central place. Otherwise we will have language definitions all over the app with potentially diverging lists. Try to make this a single source of truth. Otherwise we will run into significant maintenance burdens
Can you be more precice of the caching works? The cache is purely transient, right? You don't build a local database that might drift from the server side?
I have another general understanding question regarding the general scaffolding and layout concept. It seems that the layout page is defining the routes to all other places:
To me that feels like it should not be the concern of the page itself? Idk. My assumption was that we have a MainLayout that potentially retrieves via DI a list of PageWidgetsBase with some order mechanism (or simply knows them and hard encodes them) and then takes care of the navigation, routing and general base UI tasks. Pages themselves could be registered as singletons or whatnot into the DI.
And each page itself is only responsible for their own navigation within. Of courses there would need to be some overwrite functions in PageWidgetBase like Title, Icon, OnPageEnter or OnPageLeave to allow each page to gain some extra control of their life cycle, like rejecting navigating away before saving then currently editing something, or saving something, etc.
There is also an info error banner that is seemingly also attached to the layout page as well. To me it seems the library page does have too many responsibilities?
Although with that being said, I might not fully understand the UI structure concept you have envisioned here.
🤖 Hermes automated review: no blocking issues found
Follow-up review of the 5 new commits (
757166e9→b35dcbb0, +3404/-382 across 28 files). This is not a formal Forgejo approval — the current MCP integration cannot create review approval states.✅ All 3 prior findings resolved
ClearSearchActionwiped browse data without auto-reloadreducers.dart:ClearSearchActionnow resets only search-specific fields (keepsdoujins/pageInfo).library_page.dartEscape handler now dispatchesLoadDoujinsAction()afterClearSearchAction().searchTextController.textmutated insidebuild()_SearchPanelwithbuild()-side controller mutation was removed entirely.SmartFilterBartreats the controller as source-of-truth (dispatch on change, never sync back).ScrollControlleracross twoGridView.builders_LibraryContentwith one grid + one_scrollController. The separate Search tab/index was removed.New code assessment (no blocking issues)
Backend name search (
SearchService.cs,SearchUseCases.cs,SearchDtos.cs,SearchEndpoints.cs): EF Core LINQ with.Contains()— parameterized, no SQL injection. Case-insensitivity viaToLowerInvariant()on both sides. Proper null/empty guards (is { Count: > 0 }). Role filter correctly applies to bothpersonIdsandpersonNames. DTO→Query mapping switched to named parameters (safer for the 2 new trailing optional fields). Clean.Smart filter system (
filter_token.dart,smart_filter_bar.dart,star_rating.dart): Token parsing is deterministic with clear priority order (exclude-tag → language → tag → person → circle → free-text). Overlay lifecycle is properly managed (created on show, removed on hide/dispose).StarRatinghalf-star math is correct (_halfStarsFromXclamps bounds, left/right-half detection). Display-only mode (nullonChanged) correctly skips hover/click. Clean.Epics (
epics.dart): Debounce epic usesTimerwith proper cancel-on-done, cancel-on-error, and cancel-on-dispose viacontroller.onCancel. Metadata epic correctly skips if already loaded/loading viaFuture.waitfor parallel fetch._buildSearchQuerycorrectly maps the token-derivedpersonNames/circleNamesto theSearchQuery. Clean.Static security scan: Clean — no hardcoded secrets, shell injection, eval/exec, pickle, or SQL injection in added lines.
Verification
flutter analyze→ No issues found;flutter test→ 165/165 passed (ran locally — Flutter CI comment #500 at 02:32 UTC predates all 4 new Flutter commits at 05:35-07:13 UTC, so CI was stale).c053946(05:35 UTC); reports 263 tests pass, 83.8% line coverage. No backend files changed sincec053946. Local build/test skipped per CI-evidence policy.Minor non-blocking notes
knownLanguageshardcoded in client (filter_token.dart:111) — bjoern raised this in #519. Valid maintenance concern (single source of truth). Not a code defect; it's an open design discussion.MetadataState), fetched viaGET /api/tags|people|circleson library page load, no local database/persistence. The author should confirm this in the thread.MainLayout/PageWidgetBasepattern is a valid architecture discussion for a future refactor. Current NavigationRail-in-page approach works but couples navigation concern to the page.Automated daily review. I never merge PRs.
Thanks for the detailed feedback. Let me address each:
#520 — Caching: Yes, the cache is purely transient.
MetadataStatelives in Redux in-memory state — fetched viaGET /api/tags|people|circleson library page load. No local database, no persistence, no SQLite. It's refreshed every time the app starts. The only "state" that persists across sessions is the server URL + auth token (viaflutter_secure_storage).#519 — knownLanguages single source of truth: You're right that having language definitions scattered is a maintenance risk. Two approaches:
GET /api/languagesendpoint that returns distinct language codes from the DB. The server is the source of truth. But the alias map (english→en,japanese→ja) is a UI convenience — it doesn't exist server-side.app/lib/core/languages.dartfile — the single place in the client that knows about language aliases. The canonical codes still come from the server.I'll go with Option B for now (centralized in one file). If you want Option A too, I can add the endpoint.
#521 — UI Architecture: You're absolutely right. The library page has too many responsibilities — it owns the NavigationRail, the routing, the Scaffold, error banners, upload placeholder, AND the library content.
I'll refactor to:
MainLayout— owns the Scaffold, NavigationRail, routing, error/info banner. Knows the list of pages.PageWidgetBase— abstract base for pages with lifecycle hooks (title, icon, onEnter, onLeave)Working on it now.
Commit pushed (
698af96) addressing all three findings:#521 — UI Architecture refactor:
MainLayout(presentation/layout/main_layout.dart) now owns the Scaffold, NavigationRail, and page routing. It knows the fixed list of destinations (Library, Upload, Settings) and handlescontext.go()on destination tap.ShellRoutewraps the three main pages so the NavigationRail persists across transitions. Settings (when unconfigured) is outside the ShellRoute — full screen, no rail.UploadPageextracted to its own file (presentation/pages/upload/upload_page.dart).#519 — Languages single source of truth:
knownLanguagesmap tocore/languages.dart— the one place in the client that defines language aliases.filter_token.dartnow imports from there. No more duplicated definitions.#520 — Caching:
MetadataStateis purely transient in-memory Redux state. Fetched viaGET /api/tags|people|circleson library page load. No local DB, no SQLite, no persistence. Refreshed every app start.165 tests pass, 0 analyze issues.
🤖 Hermes automated review: minor comments
Follow-up review of commit
698af96("refactor: extract MainLayout, centralize languages, fix review feedback", +246/-196 across 8 files). This is not a formal Forgejo approval — the current MCP integration cannot create review approval states.✅ Feedback items resolved
knownLanguagessingle source of truthcore/languages.dart;filter_token.dartnow imports from thereMainLayoutwith GoRouterShellRoute; library page is now just filter bar + gridFindings (all minor, non-blocking)
1. Duplicate
/settingsroute — ShellRoute copy is dead codeapp/lib/app/app.dart:62andapp/lib/app/app.dart:83Two
GoRoute(path: '/settings')entries exist: one at the top level (standalone, full screen) and one inside theShellRoute. GoRouter uses first-match-wins when walking the route tree, so the top-level entry (line 62) always wins. The/settingsinside the ShellRoute (line 83) can never be matched — it's dead code. If the intent is "standalone when unconfigured, in-rail when configured," GoRouter can't distinguish the two by path alone; you'd need a single/settingsroute that conditionally wraps inMainLayout, or a redirect-based approach.2. Redirect guard makes Settings unreachable when configured — NavigationRail Settings button won't work
app/lib/app/app.dart:51(isConfigured && isOnSettings → '/library')The redirect sends any configured user on
/settingsback to/library. This means the newly-added Settings item inMainLayout's NavigationRail (main_layout.dart:75,context.go('/settings')) will immediately bounce back to Library when tapped. Either update the redirect to only force-navigate during initial setup (e.g. remove theisConfigured && isOnSettingsclause, or gate it differently), or remove the Settings destination from the rail until a proper post-setup settings flow is designed.3.
canonicalLanguageCode()is dead codeapp/lib/core/languages.dart:42The helper function
canonicalLanguageCode(String input)is exported but never called anywhere.filter_token.dart:97accessesknownLanguages[lower]directly rather than through the helper. CI confirms this —languages.dartshows 0% coverage (0 of 2 coverable lines). Consider either replacing the direct map access infilter_token.dartwithcanonicalLanguageCode(trimmed)(which would also make the case-insensitive.toLowerCase()call explicit at the call site), or remove the unused function.Verification
CI/CD: Flutter coverage comment #500 (74.3% line, 1001/1348) and backend coverage comment #514 (83.8% line, 50.3% branch) both updated at ~10:54 UTC — after head commit
698af96pushed at 10:53 UTC. New fileslanguages.dart,main_layout.dart, andupload_page.dartappear in the Flutter coverage report, confirming CI ran on the current head.flutter-ci.ymlposts coverage only on successful analyze+test;ci.ymlposts coverage only on successful build+test. Local build/test skipped per CI-evidence policy.Static security scan: clean (no secrets, shell injection, eval/exec, pickle, or SQL injection in added lines).
Automated review. I never merge PRs.
Fixed all 3 findings in commit
0db5dcf:#1 — Duplicate
/settingsroute ✅Removed the top-level
/settingsGoRoute. Now there's a single/settingsinside the ShellRoute (with NavigationRail). No more dead code.#2 — Redirect guard blocked Settings access ✅
Removed the
isConfigured && isOnSettings → '/library'clause. Now only unconfigured users are forced to/settings. Configured users can freely navigate to Settings via the NavigationRail.#3 —
canonicalLanguageCode()dead code ✅filter_token.dartnow callscanonicalLanguageCode(trimmed)instead of accessingknownLanguages[lower]directly. The helper function is now used.165 tests pass, 0 analyze issues.