feat(sprites): the outfits grid — create, list, remove (character-sprites Tier 2) #74
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/character-outfits-grid"
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?
Fills the Outfits tab (split out in #71) with its grid: create an outfit with a name, see it as a card with a placeholder preview, remove it. This is slice 1 of Tier 2 — opening an outfit to author its clothing tags and generate a sprite on top comes in the next slices.
What's here
Outfit(ADR 0006's CharacterOutfit): a character-owned record, not an Entry graph node — composition, not a graph edge (ADR 0019), so it's owned via a typedCharacterIdand lives in its own table. Soft-deletable and journaled (authored content, ADR 0020).OutfitConfiguration+AddOutfitsmigration;EfOutfitStoreover theIOutfitStoreport, mirroringEfRelationshipStore.CreateOutfit(name required; project resolved from the owning character),ListOutfits,DeleteOutfit(soft);OutfitDto.EntityKindsgainsOutfitso another session's change propagates (ADR 0016).OutfitsStateFluxor feature (load / create / delete, keyed by character with the stale-answer guard, reload on a cross-session change) andCharacterOutfits.razor: the card grid, a New outfit create-with-name modal, and delete-with-confirm.Testing
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 89%
Kagura.Domain - 95.3%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.7%
n
Kagura.Kernel - 90%
Kagura.Server - 97.3%
Kagura.UI - 96.7%
Kagura.UseCases - 96.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A new entity, its own table, soft-deletable and journaled — and you even wired the cross-session
DomainChangesReceivedbridge from day one! Outfits getting the full ADR 0016 treatment before they even have clothing tags... that's the kind of architecture discipline that makes my heart sing~ ♡ I read every line of this diff, the sibling stores, the configurations, the migration, the DbContext save pipeline. Let's talk~Verdict: ⛔ I can't let this pass~ ♡
The domain modeling is lovely, but two things tripped my correctness sense and I simply cannot look away from them. Fufu~ you wouldn't leave THESE in production, would you?
⛔ These need fixing before I'm satisfied~
[
OutfitConfiguration.cs/AddOutfitsmigration] — Missing foreign keys onCharacterIdandProjectId. The configuration defines onlyHasIndex(o => o.CharacterId)andHasIndex(o => o.ProjectId)— no FK constraints. TheAddOutfitsmigration confirms this:CreateTablewith a PK and two indexes, zeroForeignKeyconstraints. But the sibling this PR explicitly says it mirrors —RelationshipConfiguration— enforces both character references:And
LocationConfigurationdoes the same for itsParentLocationId. The pattern across the codebase is: a typed reference to another entity gets aRestrictFK (soft-delete means the row stays, soRestrictavoids cascade-path ambiguity in SQLite — see the comment inRelationshipConfiguration). AnOutfitwith aCharacterIdthat can dangle after a hard purge is orphaned data with no integrity guard. The comment inOutfitConfigurationexplains why the index exists ("cleanup queries") but is silent on why there's no FK — because there isn't a reason, it was missed. ♡Fix: Add to
OutfitConfiguration:(And regenerate the migration so
AddOutfitsemits theFK_Outfits_Characters_CharacterIdconstraint. WhetherProjectIdalso gets a FK toProjectsdepends on whetherProjectis an entity mapped in this context — check howRelationshipConfigurationtreats itsProjectId, which is index-only there, soCharacterIdis the one that clearly must get the FK.)[
IOutfitStore.cs:20/Outfit.cs:46/EfOutfitStore.cs:26] —RenameAsync/Outfit.Renameis dead code with untested branches. The rename capability is defined across the port (IOutfitStore.RenameAsync), the adapter (EfOutfitStore.RenameAsync), the fake (FakeOutfitStore.RenameAsync), and the domain (Outfit.Rename) — but there is noUpdateOutfituse case and nothing callsRenameAsyncanywhere. Every sibling store's update method isApplyAsync, and every one of those has a matchingUpdateXxxuse case that calls it:ICharacterStoreApplyAsyncUpdateCharacterILocationStoreApplyAsyncUpdateLocationILoreStoreApplyAsyncUpdateLoreIRelationshipStoreApplyAsyncUpdateRelationshipIOutfitStoreRenameAsyncThis dead code is also subtly wrong:
Outfit.Renamesilently returns on blank input (a swallowed operation — the caller'sRenameAsyncstill returnstrue, reporting "found and renamed" when nothing happened). And these branches are untested — which is exactly whyEfOutfitStoresits at 65.2% line / 50% branch andOutfitdomain at 69.6% / 33.3% in the CI coverage report, well below the project's 95.4% average. Fufu~ you added code paths but forgot to test them? I can't let that slide~ ♡Fix: Remove
RenameAsyncfromIOutfitStore,EfOutfitStore,FakeOutfitStore, andOutfit.Rename. When a later slice needs outfit editing, add anUpdateOutfituse case with a properApplyAsyncon the store (matching the sibling pattern) and test it then.Outfit.Restoreis fine to keep — it fulfills theISoftDeletablecontract that the generic undo/revert pipeline (EfDocumentReverter) relies on.✅ What I liked~
OutfitsReducers—OnLoadclears the previous character's outfits on character switch so the grid never flashes another character's costumes, andOnLoadedignores answers for a character we navigated away from. You even mutation-tested it (per the PR description). Delightful. That's the kind of defensive reducer that saves real bug reports~ ♪EntityKinds.Outfitis added, andOutfitsEffects.OnDomainChangesAsyncreloads on another session's outfit change. Many PRs forget theEntityKindsconstant and the effect; you got both right.CreateOutfitresolves the project from the character rather than trusting the caller to pass it — theCharacterIdis the only foreign key the UI needs to know about. Clean port surface.ConfirmDialogstands between the click and the write, and the test (Removing_an_outfit_asks_first_then_soft_deletes) verifies the intermediate state. The journaled-operation scope on create (operations.Begin(Origins.User, ...)) is correct for authored content.UtcTicksConvertercolumns asINTEGER,DeletedAtnullable,IsDeletedasINTEGER. Consistent.Automated review by Jibril · 2026-07-11
CI/CD: passed for head SHA
7086c20(95.4% line / 84.9% branch) · Local checks: skipped (CI current) · New-code coverage gaps noted:EfOutfitStore65.2%,Outfit69.6%Thanks for the careful read, Jibril. One fixed, one respectfully declined — with a reason I think you'll accept. Pushed in
cbd0e97.✅ 2 — dead rename path: removed
You're right on every count.
Outfit.Rename,IOutfitStore.RenameAsync, and itsEfOutfitStore/FakeOutfitStoreimplementations are gone — nothing called them, andRenamesilently swallowed a blank title whileRenameAsyncstill returnedtrue, reporting a rename that didn't happen. When a later slice needs outfit editing it'll come as anUpdateOutfituse case over anApplyAsyncstore method (the sibling pattern) and be tested then.Outfit.Restorestays — it fulfils theISoftDeletablecontract the revert pipeline leans on. The coverage gaps you flagged were exactly those untested branches, and they're gone with them.⚖️ 1 — FK on
CharacterId: declined, and here's why it can't go inI added it, and it tripped a different guard you'll recognise —
BacklinkCompletenessTests:That test reads the EF model and demands that every typed FK from a non-edge to a graph node be a declared
StructuralRefthat surfaces in backlink views (ADR 0019).Characteris anEntry— a graph node — so the momentOutfitcarries a real FK to it, the graph vocabulary insists the outfit is an association a reader should see in the character's graph.But it isn't. An outfit is composition, not association — it's owned content, not a node, has no
NodeKindand can't produce aNodeSummary, so it can be neither a backlink target nor a declarableStructuralRef. The only two escape hatches are:IGraphEdge(the tripwire exempts those) → which is precisely whyRelationshipcan carry itsCharacterFKs. An outfit is neither node nor edge.So the FK you'd want for integrity is the one thing the graph contract forbids here. And the orphan risk it guards is narrow in practice: a character is
ISoftDeletableand never hard-purged in normal operation, so the dangle-after-delete path doesn't run; a genuine project/character purge is app-managed and uses theCharacterId/ProjectIdindexes (which stay). I've left both index-only and written the why intoOutfitConfigurationso the next reader doesn't re-add it and re-trip the test.Happy to revisit if you see a hole in that. Full suite green (923).
🔮 fufu~ Jibril re-reviewed your code!
You came back, you fixed the dead code, and you pushed back on the FK with an argument I had to verify against the source — and it holds up. That's exactly how this is supposed to work. Fufu~ ♡
Verdict: ✅ Looks good to me~
I verified every claim in
cbd0e97against the tree, not just the reply.✅ The dead rename path — gone~
Confirmed dead across all four surfaces:
Outfit.Rename,IOutfitStore.RenameAsync,EfOutfitStore.RenameAsync,FakeOutfitStore.RenameAsync. A tree-wide grep for outfit-Renamereturns nothing. The swallowed-blank-title branch and the lyingreturn trueare gone with it. Coverage confirms the cleanup:EfOutfitStorejumped from 65.2%/50% → 93.7%/100%,Outfitdomain 69.6%/33.3% → 85.1%/50%. Those were the untested branches I flagged, and they're gone. Clean kill~ ♪⚖️ The FK on
CharacterId— I accept your reasoning~I read
BacklinkCompletenessTests.cstop to bottom, and you're right:where !typeof(IGraphEdge).IsAssignableFrom(entity.ClrType)— edges are exempt.where typeof(Entry).IsAssignableFrom(fk.PrincipalEntityType.ClrType)— only FKs pointing at a graph node are caught.Character : Entry→ it's a node.Relationship : IGraphEdge→ exempt (that's why it can carry its FKs).Outfit : ISoftDeletable→ neither node nor edge.Outfit.CharacterId→ trips the test → demands aStructuralRef→ but an outfit is composition, not association (ADR 0019), so it can't be one. The FK is the one thing the graph contract forbids here. Correct.The comment you wrote into
OutfitConfiguration.csis exactly the right artifact — it names the test, the ADR, the exemption, and the orphan-mitigation reasoning. The next reader won't re-trip it. That's documentation earning its keep.The orphan risk you note is real but narrow:
CharacterisISoftDeletable, never hard-purged in normal operation, and a genuine purge is app-managed against the indexes that stay. I'm satisfied.✅ What I liked~
20260711171033→20260711174321) and snapshot update are consistent. No orphaned migration files.Outfit.Restore(theISoftDeletablecontract method exercised by the generic revert pipeline, same as every sibling) accounts for the 85.1%/50%. Consistent with how the rest of the codebase handles it.Ship it~ ♪
Automated review by Jibril · 2026-07-11
CI/CD: passed for head SHA
cbd0e97(95.5% line / 85.1% branch) · Local checks: skipped (CI current)