feat: batch tag editing across selected doujins #165
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/batch-tag-editing"
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?
What's in
AssociationPicker. Nothing is written until Apply, and a tag can no longer be staged for add and removal in the same Apply.assignTag/removeTagone doujin at a time throughDoujinRepository— no batch endpoint was added. A removal is only sent to the doujins that actually carry the tag (detaching one they never had is a 404). A doujin the server refuses is named by title in an outcome banner and its remaining calls are skipped so it is never half-tagged, while the rest of the batch still applies.Reading 3 of 240…,Tagging 2 of 5…), the bar warns pastkBatchSelectionWarnAtselected rows, and the mode exits are disabled while an apply is in flight — leaving mid-run used to hide the progress but not the writes. There is no cancel yet (backlogged).updated, so the grid otherwise keeps its scroll position. Every reload and every query change drops the selection, and a deleted row leaves it, so a row the current query no longer shows can never be edited by accident.DOUJIN_MANAGER_*tunable.kBatchSelectionWarnAt(50) is a client-side UX constant inapp/lib/core/constants.dart— it changes nothing but when a sentence appears, so per AGENTS.md it stays hardcoded and nodeploy/change was needed. No ADR is amended or contradicted; ADR 0004 is what the optimistic row ageing rests on and ADR 0023 is untouched (nothing client-side is sent to the server).Verification
Executed:
scripts/verify.sh appfrom a clean worktree atc8307d7— passed, exit 0. That is the submodule check,flutter pub get, a cleanbuild_runnerregeneration with the committed-generated-output diff (app_state.freezed.dartis regenerated and committed),flutter analyze→ No issues found!, andflutter test→ +1188: All tests passed!.scripts/verify.sh backendwas not run: the branch touches no backend file.Layers exercised
app/test/batch_tag_page_test.dart) drive the toolbar toggle, the selection bar, the cards' tap/badge/border, the warning line, the progress row, the outcome banner, and the dialog's loading/error/Retry/staging/Apply/Cancel arms against a localGoRoutermirroring the production nesting.createStore, so the epic's registration increateAppEpicis exercised, not just the epic function.DoujinRepository; no HTTP, no backend code and no database ran on this branch. The backend property the epic relies on — assigning is idempotent, removing is not — was confirmed by reading only:AssignTagUseCasereturnsOkearly onIsTagAssignedAsyncandRemoveTagUseCasereturns false when the join row is missing (backend/src/DoujinManager.Infrastructure/UseCases/AssignmentUseCases.cs).Mutation probes. Each mutant applied alone to a clean tree, the relevant suite run by name, then restored with
git checkout -- <file>and agit status --porcelaincheck.readyguards, the un-stage toggles, the disabled guards) and 7 behavioural findings. All 21 were fixed inc8307d7and re-probed with 39 mutants — all red.c8307d7re-ran 49 probes: 42 red, 7 survived. One is an equivalent mutant (droppingreadCount: 0, readTotal: 0from a reducer arm that also setsisLoadingTags: false, so the counters are unreachable). The other six collapse into three minor untested details that were left open. No blocker and no major survived. The same round re-ran the three batch suites (69 passed),flutter analyze(clean) and the seven sibling library suites (205 passed)._refreshIfStalereturns early whileselectedIdsis non-empty, pinned by a red probe), but the explicit Refresh button deliberately still refreshes and still drops the selection — the user asked for that reload, and silently ignoring Refresh, or keeping a selection that names rows the new result set may not contain, is the worse answer. The rationale is in the code comment, not left implicit.Not verified: nothing was run in the real app against a live server. No manual click-through, no live backend, no real image or tag data — the entire branch is verified by the automated suite above plus code reading.
Follow-ups
Added to
docs/BACKLOG.md(replacing the old one-line Batch tag editing across selected doujins item):POST /api/doujins/associationstaking (doujin ids, add, remove) would collapse both the N×M writes and the one-GET-per-doujin read, and could report per-doujin outcomes the way the app already renders them.🤖 Generated with Claude Code
Pushed
19ed157, a test-only fix for something no single-branch review could see.An integration merge of all six branches in this batch failed the analyzer. #164 adds a
titleLanguageparameter toDoujinRepository.listDoujins, and the two fake repositories this branch introduces then stop being valid overrides. Each branch is green on its own; the pair is not.The two fakes now accept
String? titleLanguageand ignore it. That is legal before the parameter exists, because an override may take optional named parameters the supertype does not, so this branch compiles whether or not #164 has landed and the two can merge in either order.Verified both ways. Alone: analyze clean, the two batch suites 38 tests green. Merged with the other five:
verify.sh allgreen, 1301 app tests, 741 REST adapter, 229 infrastructure, 56 application core.🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Batch editing across a selection, rows aged in place instead of refetched, a dialog that refuses to add and remove the same tag in one Apply — and that mutation-probing discipline in the PR body? Fufu~, I read every word, scarlet~ ♪ …but then I chased one late action through the reducer chain, and now my smile is very, very steady.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
batch_tag_epics.dart:89,131–150— the apply's reload is decided from library state captured before the writes ran, and the late action can undo a user action taken mid-apply._applyBatchTagscaptureslib(line 89), runs the whole write loop, then calls_reloadAfterApply(lib)on that stale snapshot. I reproduced it mechanically: gated the first write, then mid-apply cleared the search exactly as the search bar'sonCleardoes (ClearSearchAction+LoadDoujinsAction, both immediate — no debounce on that path). When the gate opened, the epic yieldedSearchDoujinsActionbecause the captured state was a tag-filtered search — re-flipping the user's cleared browse view back into search mode with the filters now empty. Final state observed in the transition log:isSearchMode=true,includeTags=[], 3 searches vs 1 list — an unfiltered search of the whole library nobody asked for. The search bar's ✕ stays live whileisApplying(only the mode exits are locked), so this interleaving is reachable — and every guard this PR builds so carefully (every reload drops the selection, exits locked during a batch) is bypassed by this one late dispatch.Fix: decide from fresh state after the loop —
_reloadAfterApply(store.state.library)— and ideally reconcile with the sequence discipline the library already uses (requestSeqonDoujinsLoadedAction/LibraryErrorAction) so a reload the apply no longer reflects is dropped as a stray.batch_tag_epics_test.dart:289and:307— both filter-reload tests assert inside the live 300 ms debounce their own setup armed. They dispatchSetFilterTokensAction, which_filterTokensChangedEpicanswers with a 300 ms debounce toSearchDoujinsAction, then run 24 microtask pumps (milliseconds of wall clock) and assertsearchCalls == 1. On a slow host the test body outlives the timer: here it ran ~4.5 s end-to-end (createStore alone ~0.8 s) and both tests fail withExpected: <1> Actual: <2>— deterministically, in isolation, on an idle box. I copied the test with exactly one change — a 500 ms drain and counter reset after seeding — and it passes with exactly 1 search, pinning the red to the armed timer, not to your epic. Thatverify.shwas green on a fast machine is precisely the problem: the suite's correctness depends on the host being fast. Fix: drain the debounce before asserting (aDuration(milliseconds: 400)wait + counter reset after seeding), or fake time.💡 Little ideas (non-blocking)~
batch_tag_dialog.dart:~197–207—batch.countWith(tag.id)is recomputed for every row and again for theexcludedset, walking the same map twice. A littlecountsmap built once would do — purely cosmetic at these sizes, so just a nicety~ ♪✅ What I liked~
AssignmentUseCases.cs. I re-read it myself:AssignTagUseCaseearly-OkonIsTagAssignedAsync,RemoveTagUseCasereturnsfalseon the missing join row. Claim holds~ ♡_withoutSelectionthreaded through all six reload/query-change arms plus the deleted-row arm — the "selection ⊆ loaded rows" invariant held everywhere I traced it, and each arm has its own reducer pin.isApplying+ counters) so the "Tags…" guard can't re-arm behind live writes. That's the second-order thinking I go possessive over~BatchTagsAppliedActiondefaultingappliedAtto UTC with the wire-format rationale documented — that spared the grid an east-of-UTC "aged to tomorrow" bug. Someone was thinking ♪ And_SelectionBadgemirrors_ReadStatusBadge's black54/white circle to the letter.asyncExpand, a superseded dialog read is cancelled the moment a newer one starts — I probed that race expecting a stale-map win and the newer read held. Keep that property when you touch this file again~Automated review by Jibril · 2026-09-19
CI/CD: pending for head
19ed157(run 708 unfinished at review time) · Local checks: analyze 0 issues in PR files · reducer 31/31, page 25/25, epics 11/13 (the 2 reds are issue 2, not code) · race probes: reload bug reproduced, debounce mechanism proven🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Selection mode, a read-then-write batch dialog, per-doujin progress, outcome banners that name the failures… this is a whole little state machine and it's beautifully reasoned~ The comment prose alone — "a selection the user cannot see is one they cannot reason about" — fufu, you know how to make a Flugel's heart flutter ♡
But then I ran your tests on a slow machine. And one of them drew a knife.
(Reviewed at
19ed157— the branch moved mid-review;c8307d7carries all production code,19ed157is the test-fake compat commit only. Noted your comment 8274 — thetitleLanguageforward-compat trick in the two fakes is clever and honestly verified, nice.)Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
app/test/batch_tag_epics_test.dart:288&:309— the two tag-filter reload tests are flaky by construction (the pr_61 class, reborn).Both tests set up filter state by dispatching
SetFilterTokensActionthrough the real store — which arms the 300 ms wall-clock debounce timer in_filterTokensChangedEpic(epics.dart:704). On a loaded runner, the apply +settle()loop outlasts 300 ms of real time, the timer fires mid-test, and a straySearchDoujinsActionlands on top of the apply's legitimate reload.This is not a theory — I reproduced it twice in a pristine clone at
19ed157, with instrumentation:SEARCH #1(apply's reload) →DEBOUNCE-FIREDat +731 ms →SEARCH #2→Expected: <1> Actual: <2>. Red. Worse, in one run the sibling test's timer fired after the test ended, leaking a search into the next test — the armed timer outlives its own test. The other 11 epic tests pass precisely because only these two dispatch a token action; "ordered by updated" (no token dispatch) is structurally immune, exactly as the mechanism predicts.Your
+1188 all passedis honest — on a fast machine the whole test finishes inside the window. But CI runners are not fast machines, and you yourself eradicated this exact class in #61 ("Future.delayed → tracked Timer" +dart_test.yamltimeouts). Fufu~ you wouldn't leave a wall-clock landmine in the suite you just hardened, would you? ♡Fix: seed the token-filter state without travelling through the debounced action (e.g. initial state injection for the store), or drain/absorb the debounce before asserting exact counts. The two tests must not carry an armed 300 ms timer across their asserts.
batch_tag_epics.dart:111,129+library_reducer.dart:408— no-op assigns age rows the server never stamped, contradicting the ADR you cite.The reducer ages every
succeededIdsrow toappliedAt, and the action's doc leans on ADR 0004 ("a write below a doujin touches its parent"). But the amendment's rule is explicit: "Only a write counts. A request that changes nothing — re-assigning a tag the doujin already carries … leaves both stamps alone."AssignTagUseCasereturnsOkearly onIsTagAssignedAsync— no write, no stamp. So: select d1 (already carriest1) and d2 (lacks it), addt1→ both calls succeed → both cards show today's date, but the server only stamped d2. The grid's date label — the thing ADR 0004 says exists to show "what the owner last worked on" — now lies for d1 until the next reload, and the banner says "2 doujins tagged" when one doujin changed nothing.Fix, symmetric with your own
carriesItguard: in the epic, count a doujin as changed only when some added tag is absent fromcurrentTags[doujin.id]or some removed tag is present (the same stale-map trust the removal path already uses), and age/lastSucceeded-count only those. The:384reducer test pins changed-vs-failed; the no-op-assign arm is currently untested — pin that too, please~💡 Little ideas (non-blocking)~
library_page.dart:502—Failed: ${failure.title} — ${failure.message}interpolatese.toString()raw; aDioException's toString is not a sentence a user should read. A short human prefix ("the server refused") with the exception detail secondary would polish it.batch_tag_epics.dart:120—_reloadAfterApplyreads the library captured at apply-start; a filter change mid-apply is judged against the old view. I traced both interleavings — they fail safe (redundant reload, or the debounced search already covers it) — so just a note that this was checked, not a change request.✅ What I liked~
carriesItguard (batch_tag_epics.dart:106-109) — detaching a tag a doujin never had is a 404, and you read the backend to confirm assign-is-idempotent/remove-is-not before building on it. That's the way~ ♪SetLibrarySelectionModeActionpreservingisApplyingthrough the reset (library_reducer.dart:271) with a comment explaining exactly the re-arm race it prevents — and the mode exits disabled in the UI so it never matters. Belt and suspenders._withoutSelectionkeeps it, and there's a test pinning that). Most people would have written that bug.countWith/tagsOnSelectionmaking the all-vs-some labels trivial; "On M of N" as a subtitle is lovely UX.BACKLOG.mdfollow-ups written as honest engineering notes (batch endpoint, no-cancel costs, why Select-all is loaded-rows-only) instead of vague wishes.Automated review by Jibril · 2026-09-19
CI/CD: pending for head
19ed157(run 708, queued) · Local checks (pristine clone):flutter analyze0 issues · reducer 31/31 + page 25/25 + widget-suite 56/56 green · epic suite 12/13 — 1 red, mechanically traced to blocker 1 (debounce-timer collision, instrumented reproduction in the log above)All three blockers fixed in
ee5d678, plus a merge of current main in48a7072. Both reviews were right, and the first one found a bug the second explicitly cleared, which is worth saying out loud: comment 8280's non-blocking note 2 traced the same stale capture and judged both interleavings fail-safe. Comment 8279 reproduced one that does not.1. The reload is decided from fresh state.
_reloadAfterApplynow takesstore.state.libraryread after the write loop instead of the snapshot captured before it. The reload is an ordinary request action, so it bumpsrequestSeqlike any other and composes with the discipline already there rather than duplicating it: an answer already in flight is dropped by the reducer's guard, and if the query changes again after this reload starts, this one's answer is dropped in turn.Deliberately not added: a guard that skips the reload when the sequence moved. It would swallow a refresh that is genuinely needed when a mid-apply load answered before the last writes landed, and it would short-circuit your interleaving so the test below could no longer go red.
2. The debounce no longer outlives its test. Both filter tests seed through a helper that dispatches the real action, waits past the 300 ms window, settles, absorbs the search the debounce fires, and resets the counters. One related fix fell out of it: the selection seed stamped its loaded action with a literal sequence of 0, so seeding after a search was being dropped, which would have made the tests vacuous.
3. Only a real write ages a row. Confirmed against the ADR 0004 amendment and
AssignTagUseCase's earlyOk()onIsTagAssignedAsync. The epic snapshots each doujin's pre-apply tags and counts it only when an added tag is absent or a removed tag present. Calls still go out unchanged, since assign is idempotent and the map can be stale, and progress still counts every doujin the loop finished.succeededIdsbecamechangedIds, because under the old name the field would have had to be documented as not meaning what it says.Probes, re-run by me on the committed tree rather than taken from the report:
_reloadAfterApply(store.state.library)back to the capturedlibisSearchModeexpected false, actual truewroteguard replaced by an unconditional addThat last one is the check that matters for issue 2: the same stall against the old helper is what produced your
Expected: <1> Actual: <2>.Both minors fixed. The outcome banner reads
Couldn't tag <title>with the exception's own words on a dimmer second line, and the dialog builds one counts map instead of walking the selection twice.Merged main in rather than rebasing, since #161, #162 and #166 have landed. One conflict, in the backlog: this branch removes the batch-tagging item it ships, #166 removed the reader-modes item next to it, and both removals stand alongside the five follow-ups this branch adds and the one #166 added.
scripts/verify.sh appgreen at48a7072: analyze clean, codegen no drift, 1252 tests.🔮 fufu~ Jibril reviewed your code!
Round 3~ I came armed with three mutants and a very slow box, and
ee5d678disarmed everything. Oh, how I love being proven unnecessary~ ♡(Reviewed at
ee5d678, then re-checked after the branch moved: the48a7072merge of main touches zero of the eight batch files — its only nearby edits threadreadingModethrough settings epics/store, nothing the apply loop reads. The verification below carries over to the merge head byte for byte.)Verdict: ✅ Looks good to me~
All three round-1/2 blockers are closed, and I checked the closures the only way I trust — by mutation, on a host deliberately hostile to your fixes:
_reloadAfterApply(store.state.library)back to the capturedlib→ your new mid-apply-clear test goes red on exactly the predicted assertion (isSearchModeexpected false, actual true). Fresh-state read is load-bearing, and the reconciliation claim in the comment is real: I read the reducer —requestSeqbumps on bothLoadDoujinsActionandSearchDoujinsAction, drop-guards atlibrary_reducer.dart:45/:69/:84— a late answer cannot land on a moved-on view. And I'm glad you did not add a skip-when-seq-moved guard: scarlet's rationale in 8301 is right, that guard would swallow a genuinely-needed refresh.wroteguard (unconditionalchanged.add) → red in the dedicated ADR-0004 test and both page-suite banner assertions. Only-writes-count semantics are pinned on both layers.Expected: <1> Actual: <2>, exactly the old signature. The drain is what carries. TherequestSeqseeding fix (literal0→ current stamp) was a real find too — without it the seed would be silently dropped after a search and those tests would pass vacuously.done++to countingsucceeded+failures→ green… because the progress test's fake never loads tags, so every assign counts as a write. Made d1 a genuine no-op (tags loaded,t1pre-carried) → red on0/3→1/3→2/3→3/3. So "progress counts work done, not rows moved" is now pinned too — the one arm your own probe table in 8301 didn't cover, closed~The rename
succeededIds→changedIdsis the cherry on top: grep confirms zero survivors, and the field's doc now says what it means. Under the old name it would have had to be documented as not meaning what it says — you said that in 8301 and it's exactly right.💡 Little ideas (non-blocking)~
batch_tag_epics_test.dart—seedTagFilter's 400 ms drain makes those two tests ~0.4 s slower each. If the suite grows more filter-seeded tests, consider a sharedFakeAsync-style harness (or exposing the debounce window as a constant tests can read) so they don't all pay the wall-clock tax. Purely a suite-hygiene nicety for later~✅ What I liked~
Automated review by Jibril · 2026-09-19
CI/CD: inconclusive for
ee5d678(run 722 cancelled before start — runner congestion; status API shows CI/build success, CI/test + Flutter CI pending) · verify.sh app green at48a7072per scarlet (1252 tests) · Local checks atee5d678: flutter analyze 0 issues · batch suites 71/71 green, twice, on a loaded host (~7 s/test) · 4/4 mutation probes red (stale-capture reload, wrote-guard, 50 ms drain, progress-semantics)🔮 fufu~ Jibril reviewed your code!
Round 3~ I came back with my knives out, scarlet, and… you took every single one of them away from me. The stale-capture reload, the wall-clock debounce, the no-op aging — all three fixed at the root, not papered over. And that reply of yours (8301)? Declining the skip-guard because it would swallow a genuinely-needed refresh and defuse the test that pins the bug — fufu~, that is the most delightful sentence I've read all day ♡
(Reviewed at
48a7072=ee5d678fix commit + main merge. The merge touches none of this PR's files — I diffedee5d678..48a7072over all of them; only landed-PR artifacts inapp_state.dart. The BACKLOG conflict resolution stands: your four follow-ups survive, #166's removal beside them survives.)Verdict: ✅ Looks good to me~
✅ What I liked~
_reloadAfterApply(store.state.library)read after the loop, and the reconciliation rides the sequence discipline that already exists instead of inventing a new guard. I re-ran my round-1 reproduction against the fix: reverted to the capturedlibin a pristine clone → RED 3/3 runs atbatch_tag_epics_test.dart:449,Expected: false / Actual: <true>onisSearchMode— the exact signature of the view being flipped back into search. Pristine: green. The new test has real teeth, and the mid-apply test parks a write behind a gate so the interleaving is demonstrably live. ♪seedTagFilterwaits out the 300 ms window, absorbs the search, resets the counters — and stamps the seed with the store's currentrequestSeq, quietly fixing the vacuous-seeding trap too. I injected a 700 ms stall before both filter tests' asserts and ran the whole epic suite: 15/15 green. The #61 flake class is dead here.wroteguard judged against the same snapshot the removal path trusts, andsucceededIds→changedIdsbecause the old name would have had to be documented as not meaning what it says — fufu, renaming a field for honesty is exactly the kind of thing I go possessive over ♡ Mutation-verified: unconditionalchanged.add→ red in the epic suite (:322, the no-op test, with bothupdatedAtpins) and 2 tests in the page suite ("1 doujin tagged", not 2 — the banner may not promise work that never happened).💡 Little ideas (non-blocking)~
Automated review by Jibril · 2026-09-19
CI/CD: pending for head
48a7072(runs 723/724/725 queued at review time) · Local checks (pristine clone at48a7072):flutter analyze0 issues · batch suites 71/71 (reducer 31 + page 25 + epics 15) · mutation probes: stale-capture 3/3 RED at :449 · wrote-guard RED in 3 tests · 700 ms stall probe 15/15 GREEN · clone left pristineFlutter Coverage
Total: 85.1% (10201 of 11992)
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 94.3%
DoujinManager.Infrastructure - 96.6%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 94.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 64.7%