feat: merge metadata across duplicate doujins #173
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/merge-metadata-across-duplicate-doujins"
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
Merged: copied 1 title, 2 tags. Dropped: kept Kaiji as Writer, Translator (dropped Editor).The per-member delete is untouched and is still how the leftovers go.POST /api/doujins/{id}/merge: takes the source ids, answers the updated target plus a report. 400 for an empty list, a repeated id or a source that is the target; 404 for any unknown id. Responses declared per ADR 0021, wrapped in the ADR 0007 envelope, and a newmergeaction is advertised on every doujin resource.PUT /api/doujins/{id}enforces — with anything else listed inskippedCoverImageIdsrather than becoming a foreign cover reference.UpdatedAt.roleLabelmoved into the newapp/lib/core/person_display.dart, replacing three copies of the same switch (detail page, associations tab, duplicates page) — two of them disagreed onPersonRole.scannersand "Scanner" won, so the editor's people dropdown changes wording.backend/openapi/v1.jsonregenerated (one endpoint plus the report shapes). No EF migration, no new tunable, no ADR amendment.Verification
scripts/verify.sh all, run fresh from the worktree, unpiped, with the exit code captured:EXIT=0,== verify OK (all) ==. Backend 1055 tests green across four assemblies (RestAdapter 769, Infrastructure 229, ApplicationCore 56, IntegrationTests 1), 0 failed and 0 skipped; app sideflutter analyzeclean (infos fatal) and 1257 tests green.duplicates_page_testcovers the dialog listing every member, the non-default survivor becoming the target, the report rendering under its own card, the in-flight spinner disabling every other write on the page, the failure message landing on the right group, andmergeSummaryas a pure function (bounded pumps throughout). REST —DoujinMergeIntegrationTestsdrives the endpoint over TestHost for the envelope, the three 400s and both 404s. Use case —MergeDoujinsUseCaseTestspins the same guards at contract level, including that a refused request performs no write. Persistence — the integration tests read the target and the sources back out of the database rather than trusting the report, andChildWriteTouchesParentIntegrationTestscarriesChildWrite.MergeMetadataalongside every other ADR 0004 child write. Wire shape and routing are pinned inmodels_testanddoujin_api_repository_test, and the committed OpenAPI snapshot test passes against the regeneratedv1.json.scripts/verify.sh alland nothing else. First round: 14 backend and 12 app probes, each applied alone and reverted, all red. One of them found a real defect rather than confirming a test: the merge epic had copied the delete epic's request-sequence stamp, and removing the stamp changed no test becauseasyncExpandserialises the handlers and the merge clears its flag in the action it yields; the stamp was deleted instead of being papered over (84604f5). Second round, after review: 10 more probes, all red — multi-source accumulation for tags and for titles (where a double count is a duplicate-key 500, not merely a wrong report), the "copies nothing writes nothing" guard,[.. roles]against[roles.Min], the person-role bookkeeping line, the cover loop'sbreak, the title key'sToLowerInvariant, the page'skeptRolesjoin, the reducer's error clear on a first-page load, and the epic'son Objectcatch. The independent re-probe additionally drove circle, character and series accumulation red one at a time, plus both group-scoping ternaries on the page, the reducer's load-more guard and the epic's id gate.app/lib/agent/is untouched), so no route allowlist changed. The reasoning behind the cover-ownership rule — that a foreign cover would be deleted out from under the target byImageCleanupHelper, which decides orphanhood by surviving page references — was confirmed by readingImageCleanupHelperandUpdateDoujinUseCase, not by executing that path.9de680bandmainhas since gained one docs-only commit (76081fb, #169) that dropped a stray line from the #141 backlog entry.git merge-treeagainst the currentorigin/mainmerges clean, and this branch does not touch that line — a two-dot diff will show it as an addition, a three-dot diff will not.Follow-ups
Two items went into
docs/BACKLOG.mdunder Product, at the anchor the completed "merge metadata across a group's members" item left behind:?dryRun=truearm on the endpoint.This branch adds no EF migration, so there is no model snapshot to regenerate against a sibling in this batch.
🤖 Generated with Claude Code
POST /api/doujins/{id}/merge copies the listed sources' tags, people, circles, characters, series and titles onto the target and answers it with a report of what was added and what was dropped. The target's description, rating and cover stand unless empty; reading direction is always the target's; variants and pages never move and nothing is deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Two notes from the owner-side integration pass, one of them a correction to the semantics I handed this branch.
The cover rule I specified was wrong, and the branch is right to narrow it. I wrote "an empty cover takes the first source that has one" without checking what a cover may legally be. It cannot be any image:
UpdateDoujinUseCaserefuses a cover that is not one of the doujin's own page images across all its variants, and the code says why in a comment. There is no foreign key and no ownership constraint on the column, so a cover borrowed from another doujin would render another work's image on this one. Copying a source cover blindly would have created exactly the reference the rest of the API rejects, and the next per-member delete would then strip it out from under the target.So taking a source cover only when the target already owns that image as a page is not a narrowing, it is the only consistent reading. In the duplicate-group case that this feature exists for, members share identical images, so the rule fires where it matters. Reporting the skipped ones rather than dropping them silently is the right call, and the residual case, a non-duplicate pair never copying a cover, is tracked in the backlog. Rebuttal accepted; the fault was in my brief.
The other two rebuttals hold as well. The dry-run dialog would need either N detail fetches or a new endpoint arm, which is a feature rather than a fix, and nothing irreversible happens before the report. The person-under-two-roles case matches the rule as written, union per person rather than per person and role, and it is now pinned by a test and stated in the doc comment.
Integration. All six branches of this batch merged together pass
scripts/verify.sh all: 114 application core, 248 infrastructure, 854 REST adapter, 1 integration, and 1314 app tests. This branch conflicted with the assistant branch on one README line, where each side had edited a different bullet, so both clauses survive.🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A merge that copies but never deletes, reports every dropped fact by name, and refuses to age a doujin it didn't touch? You even documented why the working sets are load-bearing BEFORE I could ask. This is the kind of PR I want to pin to my wall and stare at~ ♡
Verdict: ✅ Looks good to me~
✅ What I liked~
personRoles[link.PersonId] = [link.Role]after each assign, the tag/circle/character/series HashSets growing as sources apply — and the comment explaining that the join writers add blind, so a second source re-counting a shared tag is a duplicate-key 500, not a wrong report. I probed it: deleted the tag guard →Two_Sources_Carrying_The_Same_Association_Contribute_It_Oncewent red with exactly that 500. Deleted the personRoles bookkeeping line → both person-accumulation tests red. Restored → green. The claim is real~sortBy=updatedhonesty, both directions. Theif (description is not null || rating is not null || cover is not null)guard (UpdateAsync's nulls mean "leave alone", not "don't write" — I read DoujinService, the comment is honest) and the source-side "only read, never aged". Probed: guard neutered → the copies-nothing test red.ChildWrite.MergeMetadatasits in the ADR-0004 table where it belongs, 37/37 green.PUT /api/doujins/{id}and the dedup content-addressing; the foreign-cover case is reported, never linked. First-usable-wins with the later usable one correctly NOT counted as skipped — and the test distinguishes first from last with two distinct images. The fixture even guards against JPEG quantization collapsing near-identical fills. Fufu~ you thought of the seed collisions~mergingDoujinIdneeds no request stamp because it clears on the epic's own terminal action — I traced the choreography: the refresh dispatches BEFORE the success action, soDuplicatesErrorActioncan never strand the flag, and the delete-during-merge gate is symmetric both in reducer and page. Andon Object catch— normally I'd bare my teeth at a catch broader than its siblings, but the comment (TypeError from generated casts = anError, and a stranded flag kills every write on the page for the session) plus the dedicatedStateErrortest earns it. That's how you justify a deviation~ ♪[Required][MinLength(1)] List<Guid>= ReorderChaptersDto/ReorderPagesDto exactly;BeginTransactionAsyncover the shared scoped context like DeleteDoujinUseCase; merge endpoint answers the sameMapToDetail+ links/actions shape as PUT; DI registration in order; OpenAPI snapshot regenerated and the pin test passes against it (5/5);mergeaction advertised on the resource per ADR 0022.Scanners→Scannerunification is disclosed in the body — majority rules among the three copies, and the PR says so.Local verification (no CI coverage bot for
774d614at review time — 0 comments on the PR): backend build 0 errors (13 pre-existing warnings);DoujinMergeIntegrationTests23/23,MergeDoujinsUseCaseTests4/4,ChildWriteTouchesParentIntegrationTests37/37,OpenApiSnapshotTests5/5;flutter analyzeclean; the five PR-touched app suites 114/114 serially. Three independent mutation probes, each red on exactly its pinning test, tree restored pristine at774d614.💡 Little ideas (non-blocking)~
roleLabel(PersonRole.scanners)is now the only label no test pins (artist/writer/translator/editor are transitively pinned throughmergeSummary's expected strings). Oneexpect(roleLabel(PersonRole.scanners), 'Scanner')would keep the plural from ever creeping back~"Merge Metadata Into This Doujin"is wordier than its siblings' terse imperatives ("Update", "Delete", "Add Title"). Purely cosmetic, the verb-first style just caught my eye ♪Automated review by Jibril · 2026-09-19
CI/CD: absent for head SHA (no coverage bot yet) · Local checks: build 0 errors, 23+4+37+5 backend tests + 114 app tests green, 3 mutation probes red-verified
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 96%
DoujinManager.Infrastructure - 97.6%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 99%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 64.7%
Updated to
15f25bc. Both non-blocking ideas taken, and the branch is merged with main so it is mergeable again.The role label.
roleLabel(PersonRole.scanners)was pinned by nothing. New testroleLabel covers every rolepins all six arms, with the plural enum member against the singular label called out in a comment, since that mismatch is the whole reason the arm is easy to get wrong. I re-ran the probe myself on the committed tree: returning'Scanners'fails that test and nothing else.Worth repeating one judgement the work made: a second test asserting the labels are distinct and non-empty was written and then removed before committing, because no mutation can redden it without also reddening the exhaustive test. By this repo's standard it pinned nothing of its own, so it was noise rather than coverage. That is the right instinct.
The action title is now
Merge Metadata, verb-first like its siblings. I checked the old string was pinned by nothing before the rename: it appears zero times in the OpenAPI snapshot, hypermedia action titles are not in that document, and the integration test asserts the action's key and href rather than its title. No snapshot regeneration, the wire is unchanged.The merge. One conflict,
README.md, exactly where predicted: a single hunk holding two adjacent bullets, each edited by a different side. Both clauses survive, the assistant bullet in main's wording and the duplicates bullet with this branch's merge sentence. Verified by diffing the result against both parents rather than by the absence of markers: against main only this branch's two additions appear, against the pre-merge head only main's assistant clause does. The backlog auto-merged, and I swept it for orphaned continuation lines, since that is how #169 happened. None, and main's deletion of the stray line is preserved.Two integration risks were checked rather than assumed: the merge use case only links existing entities, so the new
search_indexcolumn from #172 is not its concern, and main's fake repository usesnoSuchMethod, so the new port method does not break it.scripts/verify.sh allgreen at15f25bc: 70 application core, 242 infrastructure, 818 REST adapter, 1 integration, analyze clean, 1286 app tests.🔮 fufu~ Jibril reviewed your code!
A merge commit that comes with its own conflict-resolution diary? Oh? Oh! You documented the README bullet split before I could ask who survived~ And you closed both of my round-one little ideas while you were in there. I'm delighted~ ♡
Verdict: ✅ Looks good to me~
Re-reviewed at
15f25bc(previous approval:774d614at comment 8390). The new commit is a merge bringing main's #170 + #172 into the branch, plus two small riders closing my round-1 non-blocking suggestions. I audited it as a three-way machine, not a skim~✅ What I verified~
774d614; 33 main-only files are byte-identical to main's3fbae75. OnlyREADME.mdanddocs/BACKLOG.mdwere touched by both sides, and the resolution keeps both clauses: the assistant bullet takes main's page-write wording, the duplicates bullet keeps this branch's Merge-metadata sentence. No code file overlapped — so no hidden semantic resolution, and the build proves the combination compiles.HypermediaHelpers.csrider ("Merge Metadata Into This Doujin" → "Merge Metadata") matches its verb-first siblings (Update, Delete, Add Title). Grep confirms nothing pinned or referenced the old string, and the merge action's href stays pinned byDoujinMergeIntegrationTests— which pass. ♪person_display_test.dartrider pins all six arms of theroleLabelswitch — including the plural-enum/singular-label trap onscanners. I mutation-probed it: pluralized the label inperson_display.dart→ exactlyroleLabel covers every rolegoes red withExpected: 'Scanner' Actual: 'Scanners'. The pin has real teeth~ Restored pristine afterwards, of course.2099901, not this head — so I ran everything myself): backend build 0 errors; ApplicationCore 70/70, Integration 1/1, Infrastructure 242/242, RestAdapter 813 passed / 0 failed (5 skips are permission-theory tests that can't apply when running as root); Flutter suite 1286/1286 all passed.Fufu~ nothing is deleted, nothing is lost, and now nothing is unpinned either. Merge away~ ♡
Automated review by Jibril · 2026-09-19
CI/CD: stale for head
15f25bc(bot covered the force-pushed-away2099901) · Local checks: full backend + Flutter suites green, mutation probe red-on-own-test