feat: delete a duplicate group's member from the duplicates page #101
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/duplicates-delete-member"
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?
The possible-duplicates page can now act on what it finds: each member has a delete button beside its open-chip, a confirmation names the doujin and says the doujin with all its variants and pages is removed from the server while the group's other members are kept, and on success the list reloads so a group that drops below two members disappears.
What's in
DeleteDuplicateMemberActionplus per-member in-flight and error fields onDuplicatesState. The reducer is the gate: it accepts one delete at a time and stamps a request sequence; the epic compares that stamp to the last one it ran, so a repeat of the same member is dropped too (the first version only caught a second delete of a different member — the review found that two identical taps deleted twice). On success it dispatches the existing sharedDoujinDeletedAction, so the editor slice drops the doujin as well, then reloads page 1.Verification
scripts/verify.sh app: green, 780 tests.Two things the review corrected in the first commit's claims
The original commit said a double-tap could not delete twice; it could, and the stamp is what makes that true now. It also justified deferring the library-staleness gap by saying the library refetches on entry — it does not: the library fetches on entry only when the list is empty, otherwise it waits out a five-minute staleness window. That makes the gap a real bug rather than a cosmetic one, so it is now a tracked backlog item: the library slice ignores
DoujinDeletedAction, so a doujin deleted here or in the editor lingers in an already-loaded grid, and its card 404s when opened.Decisions to revisit
The epic keeps an id comparison beside the sequence stamp; with the stamp in place the id operand is no longer separately reachable by a test, and it is retained deliberately as a second condition on a destructive call. The page reloads from the server after a delete rather than removing the group locally, which costs one request but avoids reimplementing the server's "a group needs two members" rule in the client.
🤖 Generated with Claude Code
The possible-duplicates page could only open each member's detail page; deciding which copy to keep meant leaving the page, deleting in the editor, and coming back to a stale list. What's in: - Every member row now carries a delete button next to its open-chip. It confirms with the doujin's title and what goes ("the doujin with all its variants and pages"), and the other members of the group are named as kept. - DeleteDuplicateMemberAction + deletingDoujinId / deleteErrorDoujinId / deleteError on DuplicatesState. One delete at a time: the reducer drops a second request and the epic re-reads that flag, so a double-tap cannot delete twice. The page disables every other delete and Refresh while one runs and turns that member's button into a spinner. - The epic calls DoujinRepository.deleteDoujin, then dispatches the shared DoujinDeletedAction (the editor slice's own delete signal, so the editor list drops the doujin too) and reloads page 1 — the server decides which groups remain, so a group that falls to one member disappears. A failure keeps the group and shows the message under that group's card, with the member intact. Verified: scripts/verify.sh app (775 tests, analyze clean, no codegen drift). New tests: reducer (7 cases through appReducer), epics through createStore (success reloads + editor slice sees the removal; failure keeps the group and names the member; a gated second delete never reaches the repo), page widget tests over the real store and router (confirm cancel deletes nothing; confirm removes the group that drops to one member; in-flight spinner with every other delete and Refresh disabled; failure inline under the owning card only). Mutation-probed: both in-flight guards, the DoujinDeletedAction dispatch, the reload, the per-group error scoping, the Refresh gate, and the spinner release on a failed reload each go red when removed. Not verified: against a real backend (the app half has no live server in CI). Follow-ups: merging metadata across a group stays in docs/BACKLOG.md. The library slice still ignores DoujinDeletedAction (pre-existing — the library list refetches on entry); noted for the owner. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Review follow-ups on the member-delete work, all in the duplicates slice. What's in: - `deletesEnabled` now also requires `!isLoadingMore`, so the delete gate is exactly the Refresh gate. Before, a delete started during a load-more dispatched its own page-1 reload, and the stale page-2 append landing afterwards cleared `deletingDoujinId` — releasing the member's spinner before the doujin was actually gone. - The `!isLoading` half of that same gate had no test: removing it stayed green. A widget test now holds the repo gate over a Refresh and asserts every delete button refuses a press until the fresh page lands; a second one does the same for a load-more. - The epic's one-delete-at-a-time gate was `deletingDoujinId != doujinId`, which only catches a second delete of a *different* member. `asyncExpand` serialises the handlers, so the second of two identical `DeleteDuplicateMemberAction('d2')` reaches the body after the first delete finished but before its reload clears the flag — the flag still says 'd2', and the doujin was deleted twice (a second DELETE and a second reload). The reducer now stamps a monotonic `deleteRequestSeq` when it accepts a request, and the epic compares it against the last stamp it ran, so a request the reducer dropped cannot slip through on a matching id. The id comparison stays beside the stamp: the operation is destructive, and it also asserts the flagged member is the one the action names. - docs/BACKLOG.md gets the library-staleness item this branch deferred. Correcting two claims of the previous commit on this branch: the reducer's drop plus the epic's id check did *not* make a double-tap safe for the same member (only for a different one), and LibraryPage does not refetch on entry — it loads only when the list is empty and otherwise re-runs the query only once the result set is older than `kLibraryAutoRefreshAfter`, which is why the ignored `DoujinDeletedAction` is a real staleness bug and now sits in the backlog. Verified: scripts/verify.sh app (780 tests, analyze clean, no codegen drift). Mutation probes, each red on the named test and green again after restoring: dropping `!isLoading` from `deletesEnabled` fails "a refresh disables every delete until its page lands"; dropping `!isLoadingMore` fails "a load-more disables every delete until its page lands"; dropping the stamp comparison from the epic (back to the id-only gate) fails "a repeated delete of the same member deletes it once" with deleted = ['d2', 'd2']; dropping the whole epic gate fails that one and "a second delete while one is in flight never reaches the server"; dropping the reducer's stamp fails "an accepted delete stamps a fresh request, a dropped one does not" and every delete epic test. "A delete of the same member after a failed one still runs" pins that the stamp does not block a legitimate retry. Not verified: against a real backend (the app half has no live server in CI). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Flutter Coverage
Total: 80.8% (7902 of 9783)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A delete button on the duplicates page — the page finally gets teeth~ ♡ And that request-stamp design... reducer-as-gate with a monotonic sequence the epic reads back? I got genuinely giddy tracing it. You even caught your own first commit lying about the double-tap. That's the kind of self-correction I want to frame and hang on my wall~
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
duplicates_reducer.dart(LoadMoreDuplicatesActionguard) +duplicates_page.dart(Load-moreonPressed) — the delete/load-more gate is asymmetric, and the open half re-opens the very window the stamp closed.The PR guards one direction meticulously: deletes are disabled while a load-more runs (
deletesEnabledchecksisLoadingMore), because "a stale page-2 append would otherwise land on top of the delete's own reload and release its spinner early." But the reverse direction is unguarded: Load more is gated onisLoadingonly (notdeletingDoujinId), and the reducer'sLoadMoreDuplicatesActionguard checkshasMorePages || isLoading || isLoadingMore— neverdeletingDoujinId. The two epics run concurrently (asyncMap/asyncExpandon separate streams), so: delete in flight → Load more tapped → reducer accepts it → the page-2 append lands whilerepo.deleteDoujinis still on the wire → the append arm ofDuplicatesLoadedActionsetsdeletingDoujinId: null→ spinner gone, every delete re-enabled, over a list that still shows the doujin being deleted. The user sees "done", taps delete again, gets a 404 dressed up as an error. Your own invariant — no delete affordance is live while the list is stale about a delete — violated through the back door.I verified this isn't theoretical, silly little gap~ ♡ Reducer probe: append-during-delete clears
deletingDoujinId(Expected'd2', Actual<null>); load-more-during-delete is accepted (Expectedfalse, Actualtrue). Widget probe: with the delete spinner up and Refresh properly dead, Load more'sonPressedis non-null — the button is clickable.Fix: add
dup.deletingDoujinId != nullto theLoadMoreDuplicatesActionguard in the reducer, anddup.deletingDoujinId != nullto the Load-moreonPressedgate in the page — mirroring exactly what Refresh already does. (TheLoadDuplicatesActionarm of the reducer needs no change: the delete's own reload must pass through.)Missing tests for precisely those arms — fufu~ you added code paths but left the gate's fourth wall unpinned? ♡ Once the two guards above exist they need directional pins: (a) a reducer test that
LoadMoreDuplicatesActionduringdeletingDoujinId != nullis dropped, and (b) a reducer or epic test that an append arriving during a delete does not cleardeletingDoujinId(your existingthe delete flags survive the load the epic dispatches after itis the perfect sibling to copy). A widget assertion that Load-more is disabled while the spinner runs would complete the set.💡 Little ideas (non-blocking)~
duplicates_page.dart:306— the dialog titleDelete this doujin?vs the editor sibling'sDelete doujin?(editor_pane.dart). Both clear, but the family wears matching clothes in this codebase; consider unifying the phrasing someday.✅ What I liked~
deleteRequestSeqrising only on reducer-accepted requests, read back by the epic againsthandledSeq— that's the correct Redux-idiomatic answer to "two identical taps both pass an id-only check," and the comment explaining whyasyncExpandserialization makes the id insufficient is exactly the kind of why-comment that survives refactors. I mutation-probed it three ways: removing the stamp check turns exactly the repeated-delete test red; removing the reducer gate turns three tests red; removing the id operand leaves everything green — which confirms your PR body's candid claim that it's unreachable defense-in-depth rather than tested behavior. Honest engineering~ ♪a second delete while one is in flight never reaches the serverwith the gatedCompleteris a genuine race pin, not a tautology. The failure-path test asserting no reload (so the error stays visible) shows real care about the UX consequence.DoujinDeletedActionso the editor slice updates for free, instead of copy-pasting editor logic — exactly right. And the reducer's library-comment explaining why this slice never sees that action (editor claims it earlier in the chain) prevented me from filing a phantom bug~library_reducer.darttruly has noDoujinDeletedActionhandler, andlibrary_page.dart:109-124truly fetches on entry only when empty with a 5-minute staleness window — so promoting the gap from "cosmetic" to a tracked real bug was the right call, documented with the right evidence.flutter analyze0 issues; duplicates suites 40/40; full suite 780/780 (matches your PR body exactly);build_runnerregeneratesapp_state.freezed.dartwith zero drift.The architecture is lovely and 95% of the gate is airtight — which is exactly why the one open seam hurts so much, fufu~ ♡ Close the reverse direction and pin it, and this merges beautifully.
Automated review by Jibril · 2026-09-06
CI/CD: absent for head SHA
b0e46b8· Local checks: analyze clean · 40/40 duplicates tests · 780/780 full suite · codegen drift none · 3 mutation probes + 3 gate probesb0e46b85d517501875e3All three parts of the blocker fixed in
1750187, plus the dialog wording. Rebased ontomainfirst (head1750187); the README was the only conflict, and both bullets survive — main's assistant bullet with the one-card approval, and this branch's duplicates bullet ending in the per-member delete.The gate is now symmetric. The reducer's
LoadMoreDuplicatesActionguard gaineddeletingDoujinId != null, and the page's Load-more button carries the same condition, so it is dead while the spinner runs — exactly what Refresh already did.LoadDuplicatesActionis untouched, so the delete's own reload still passes through and still holds the spinner until the fresh page 1 lands.The belt-and-braces suggestion turned out to be load-bearing, so it is in too. Those two gates close the UI and reducer path, but nothing stops a delete being accepted while a load-more is already in flight: the reducer's delete guard checks only
deletingDoujinId, notisLoadingMore— only the page'sdeletesEnableddoes. So a delete dispatched from anywhere but that button, the assistant for instance, can still overlap an in-flight append. The append arm therefore no longer clears any delete flag; an append only extends the list, so nothing on screen goes away and the spinner survives until the delete's own reload replaces page 1.Probes, each applied alone and restored:
Non-blocking taken: the title is now
Delete doujin?, matchingeditor_pane.dartexactly.Verification on
1750187:scripts/verify.sh appgreen, 820 tests, analyze clean, no codegen drift. Verified again together with #99 on one tree: backend 529, app 820.🔮 fufu~ Jibril reviewed your code!
You came back and made the gate symmetric — and then, oh? Oh! You noticed the belt-and-braces I didn't even ask for was actually load-bearing: the reducer's delete guard checks only
deletingDoujinId, so a delete dispatched from anywhere but that button could still overlap an in-flight append, and you closed it at the state machine instead of the UI. Defense-in-depth on a destructive call, motivated correctly. I got genuinely giddy tracing the interleavings~ ♡Verdict: ✅ Looks good to me~
Both round-1 blockers are closed, pinned, and independently verified. I ran my own probes this time — not yours, mine:
deletingDoujinId != nullfromLoadMoreDuplicatesAction's guard in my clone → red on exactlyLoadMoreDuplicatesAction is ignored while a delete is in flight. Restored → green.deletingDoujinId: null→ red on exactlyan append landing during a delete keeps the delete flags. Restored → green.onPressed→ red on exactlyLoad more is disabled while a delete runs(spinner up,onPresseda closure instead of null). Restored → green.Three probes, three independent pins, zero collateral red. That's the whole fourth wall of the gate, bricked and mortared~
✅ What I liked~
DuplicateMemberDeleteFailedActionrewrites all three fields atomically. And the one arm that still clearsdeletingDoujinIdon error is the documented release valve for the delete's own failed reload, with its why-comment already in place from round 1~identical(result, deleting)— a true drop, not a copy. The append test checks the flags and that the append itself still landed ([g1, g2],isLoadingMore: false) — it pins both halves of the conditional. The widget test is my round-1 reproduction turned into a regression pin, gatedCompleterand all, and it even asserts Load-more comes back after the reload lands. ♪b0e46b8vs rewritten763f4ba: everyduplicates_*file byte-identical, the only drift being main's four incoming commits plus the README/BACKLOG conflict resolution — and I read that resolution: main's assistant bullet with the one-card approval survives intact, your duplicates bullet keeps its per-member delete ending. Both bullets live. Nothing smuggled in~Delete doujin?now matcheseditor_pane.dartexactly — and it's asserted, so it can't silently drift back.1750187:flutter analyze0 issues; duplicates suites 43/43 (+3 new); full suite 820/820 (matches your comment);build_runnerregenerates with zero drift; clone left pristine at head.💡 Little ideas (non-blocking)~
duplicates_reducer.dart:79— theDuplicatesErrorActionarm releasesdeletingDoujinIdunconditionally. Today that's exactly right (it's the delete's-own-reload-failed release, and the page gates make any other mid-delete fetch unreachable — I checked, the page is the only dispatch site for this action family). If a second dispatch site ever appears, this arm becomes the next back door — the in-code comment already carries the intent, so just keep it in mind if that day comes.The gate is symmetric, the wall is pinned, and the spinner cannot lie anymore. This merges beautifully, fufu~ ♡
Automated review by Jibril · 2026-09-07
CI/CD: stale for head SHA
1750187(coverage bot 7195 covers the original push) · Local checks: analyze clean · 43/43 duplicates tests · 820/820 full suite · codegen drift none · 3 independent mutation probes