refactor(outfits): independent sheet & sprite generation (ADR 0067 revised) #265
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/outfit-sheet-split"
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?
Björn's follow-up to #264 after living with the merged feature: the fused one-button pipeline coupled the two artifacts — regenerating either burned both, and neither could be iterated alone. This splits them end to end.
What
IOutfitImageGenerator.GenerateSheetAsync(plain sheet job) besideGenerateAsync(sprite job, keeps the character-base-sheet lineage under the likeness flag). One sharedPlanAsyncresolves both jobs' inputs and the shared preconditions; the likeness/base-image precondition belongs to the sprite alone — the sheet generates plain whatever the flag says. Separate activities (character-outfit-sheet/character-outfit-sprite), separate failure surfaces.Why it's clean
The two generations were already independent by construction — the sprite's lineage is the character's base sheet, not the outfit's own sheet — so the split uncouples the queue without touching any lineage semantics.
Verification
Summary
Summary
Coverage
Kagura.BlazorAdapter - 89%
Kagura.Domain - 94.8%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.7%
n
n
on
ng
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 81.5%
Kagura.UseCases - 95.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Splitting the fused pipeline into two independent artifacts — sheet on the Outfit tab, sprite on the Expressions tab, one shared
PlanAsync? That's a lovely uncoupling~ The lineage insight (the sprite follows the character's base sheet, never the outfit's own) makes the split genuinely clean, and your integration pins are exactly the right shape. But fufu… you wouldn't believe how carefully I looked at every seam, would you? ♡Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Kagura.BlazorAdapter/KnowledgeBase/OutfitSheet.razor:12–50— the new card is invisible to its own styles. It reuseskg-outfit-sprite*classes, but those live only inOutfitSprite.razor.css, and Blazor CSS isolation compiles them with OutfitSprite's scope attribute. I checked the compiled bundle (scopedcss/bundle/Kagura.BlazorAdapter.styles.css:2575): every rule is.kg-outfit-sprite[b-euf1eejxvb] { … }, and the objscopedcss/folder has noOutfitSheet.razor.rz.scp.css— so OutfitSheet's markup never carries that attribute and zero of the card's styles apply. No flex column, no muted hint, no spinner animation — and worst,__previewlosesposition: relative, so the generating overlay'sposition: absolute; inset: 0escapes its anchor and scrims the wrong thing entirely. Every one of the ~34 styled siblings in this folder owns its.razor.css; this is a runtime-visible bug bUnit can't catch (it doesn't apply isolation attributes — the compiled bundle is the witness).Fix: add
OutfitSheet.razor.csswith its own scoped copy —kg-outfit-sheet*classes per the BEM-per-component convention (duplicating the tiny spinner block beats a cross-component coupling; seeCharacterBaseSprite.razor.cssfor the sibling spinner).docs/adr/0067-outfit-sheet.md:18–21+docs/stories/character-sprites.md:43— the PR title says “ADR 0067 revised,” butdocs/has zero changes. The accepted ADR still mandates the exact opposite of this PR: “An outfit generates two artifacts in one activity … A sheet failure stops the pipeline before the sprite,” and the story checklist says the same. In a docs-first repo (ADR 0001), code that contradicts an accepted ADR without revising it is a bug in the decision record, not just a docs nit — six months from now someone reading ADR 0067 would “restore" the fused pipeline thinking it was the design. ♡Fix: revise ADR 0067 (decision 1 + consequences — two activities, independent failure surfaces, the sprite card living on the Expressions tab) and the character-sprites story line.
OutfitSpriteState.cs:61–72, 109–116— three new reducers and a new effect, and their test files weren't touched.OutfitSpriteReducersTests.csandOutfitSpriteEffectsTests.csare byte-identical to base (verified), yet every sprite arm has its pin: stale-id guard, failure-clears-spinner, unexpected-throw. The sheet arms have nothing: no test thatOutfitSheetFailedclearsSheetGeneratingand surfacesSheetError(a rejected generation leaving the button spinning forever is precisely the bug the sibling tests exist to prevent), no stale-id guard test for the sheet fields, no throw-arm test forOnGenerateSheetAsync. The bUnit happy path clicks through, but the failure surface — the PR's own headline (“separate failure surfaces”) — is dark. Fufu~ you added failure branches but forgot to test them? I can't let that slide~ ♡Fix: mirror the sprite tests — sheet-flag set + prior error cleared, sheet completion/failure only applying to the named outfit, failure → spinner cleared + error surfaced + no reload, throw →
OutfitSheetFailed.OutfitSpriteTests.cs— the no-positive-tag hint test was deleted, not split. Base hadWith_a_token_but_no_positive_tag_the_button_is_disabled_with_a_hint; this PR drops it and adds no replacement for either card, though both still ship the branch (OutfitSheet.razor:49–51,OutfitSprite.razor:49–52).canGenerate: falsehas zero matches in the new test file, and no other test asserts the “Add at least one positive tag first” hint for these cards. A coverage regression on a live branch~Fix: one hint test per card (
RenderSheet(canGenerate: false)/RenderSprite(canGenerate: false)with a token set).💡 Little ideas (non-blocking)~
OutfitImageGenerationTests.cs:76— the helper comment still says “the pipeline's second generation (the sheet is the first).” There is no pipeline anymore, and in the sprite-only tests this is the only spec. One-line comment fix while you're in there.✅ What I liked~
PlanAsyncwithrequireBaseImage— one shared precondition core, and the likeness/base-image precondition correctly belongs to the sprite alone. The sheet generating plain whatever the flag says is exactly right per ADR 0066's composed-guard reasoning~SheetOutfitId/SheetGenerating/SheetError) mirror the sprite's stale-answer guard faithfully — one card's spinner never spins the other's, and a stale completion for a navigated-away outfit stays ignored. Wonderful~ImageFileNamestays null, sprite-only assertsSheetImageFileNamestays null, failure-in-one asserts the other survives, and the port-isolation pins (Assert.Empty(Requests)/Assert.Empty(SheetRequests)) catch a card wired to the wrong port. That's how you pin a split ♪RunAsynceffect helper — the DRY extraction keeps theLoadOutfitstale-id navigation guard intact for both artifacts.Automated review by Jibril · 2026-09-02
CI/CD: absent for head SHA
5243e54(no coverage bot yet) · Local checks: build 0/0, 335+764+858 all greenJibril — all four fixed in
3e89e02, and blocker 1 is the round's lesson: I reached for the sibling's class names and never thought about what the isolation compiler does with them. You checked the compiled bundle — the witness bUnit structurally cannot be.kg-outfit-sheet*classes per the BEM-per-component convention + its ownOutfitSheet.razor.css(spinner duplicated, matchingCharacterBaseSprite's sibling precedent). No cross-component CSS coupling.canGenerate: false+ token → disabled + "Add at least one positive tag"). And the stale "pipeline's second generation" comment in the integration helper now says what the split means.Full suites on the branch: UseCases 335/335, Integration 764/764, BlazorAdapter 867/867.
🔮 fufu~ Jibril reviewed your code!
Round 2~ You brought the styles home, revised the ADR properly, pinned every arm — and then wrote a cross-arm isolation test so thorough you corrected your own first draft's misunderstanding before I could even see it. That's the kind of author a reviewer falls a little in love with~ ♡
Verdict: ✅ Looks good to me~
All four blockers from round 1, verified closed at
3e89e02:OutfitSheet.razor.cssexists withkg-outfit-sheet*classes per the BEM-per-component convention. I checked the compiled scoped-CSS this time, same as before:obj/scopedcss/KnowledgeBase/OutfitSheet.razor.rz.scp.cssis present, and the bundle carries six.kg-outfit-sheet*[b-lrcxtti7io]rules on the sheet's own scope attribute, beside the sprite's six on[b-euf1eejxvb]— zero cross-component coupling. The duplicated spinner matchesCharacterBaseSprite's precedent. The.kg-viewreuse is fine — that one is a global Kagaku.UI class, not scoped.OnGeneratedto also clearSheetError(the "helpful cleanup" regression) in my review clone → exactlyThe_sheet_and_sprite_arms_never_touch_each_others_flagsgoes red; restored → green. Directional, not a tautology. ♪canGenerate: false+ token → disabled button + "Add at least one positive tag" — and the staleSpriteSpec()helper comment now describes the split.💡 Little ideas (non-blocking)~
A_completion_for_an_outfit_no_longer_open_does_not_reload_it); the sheet arm doesn't have its mirror. The guard lives in the sharedRunAsync, so the branch is genuinely exercised — this is pure family symmetry, nothing untested.✅ What I liked~
Planrecord: onePlanAsyncresolving both jobs' inputs,requireBaseImagescoped to exactly the one precondition that belongs to the sprite alone. The duplication didn't get split — it got dissolved. Oh, that's lovely~ImageFileNamenull and vice versa.Automated review by Jibril · 2026-09-02
CI/CD: stale for
3e89e02(coverage bot 6785 @ 12:42 covers5243e54— where it showedOutfitSheetFailed0% andOutfitSpriteReducers85.7%/50%, precisely the gaps these tests close) · Local checks: build 0/0, UseCases 335/335, Integration 764/764, BlazorAdapter 867/867 (858 + 9 new), mutation probe RED→restored→green