feat(cg): the establishing image opens the editor, and the panel's bars line up #218
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/cg-editor-base-mode"
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?
Closes the ADR 0054 §2 gap left open in #217, plus the panel polish from review-by-screenshot.
The establishing image opens the editor
The set's establishing image now opens the same editor shell — in its replace-in-place mode (ADR 0046): the Tags tab edits the scene tags every variation inherits, Generate/Regenerate drives the set's own engine, and the timeline region states "the establishing image keeps no timeline — regenerating replaces it in place" instead of faking an always-empty history. No Cast tab: the establishing shot is the event with nobody in it.
This also restores a surface that had silently vanished: since the redesign, base-tag authoring had no home in the UI at all (the set page deliberately carries the stored tags through untouched, and the old
CgSetBaseImagesection was orphaned — rendered by nothing). The mode composes the set's existing, already-tested Fluxor slices (CgSetEditorStatefor load/save/cross-session sync,CgSetBaseImageStatefor generate/capability) — no new state machinery. The orphaned section is deleted.The route is
/cgs/{id}/establishing—/cgs/{id}/baseis the image-serving endpoint, and the collision was anAmbiguousMatchExceptionfound in the browser, not by any test.ADR 0054 §2 gains three lines saying the base editor is the same shell with the timeline region explaining itself.
Panel polish
Found along the way: a Kagaku.UI defect
Browser verification showed the badge stuck at "Positive (0)" with three tags in the list: a
Tab'sTitlerenders in the owner's pass, before the Tab child re-renders, so a live title stays one render stale — on a page whose load produces exactly one re-render, forever. Fixed in Kagaku.UI #12 (test verified red-without/green-with); the submodule pointer bump follows that merge, since the local pointer would dangle if #12 squashes.Tests: the gallery card test became a navigation test, five new base-mode page tests (regions + timeline note, auto-save onto the set with name carried through, gated Generate driving the set engine, token hint, not-found). Full suite green at 1735 (312 / 787 / 641) — plus 288 in Kagaku.UI.
🤖 Generated with Claude Code
Kagaku.UI #12 merged (squashed as
f9a84d1), so the submodule pointer bump rides in this PR after all — pushed asad01db7. The badge fix is now part of this branch's build; CG + TagAuthoring adapter suites re-ran green on the bumped pointer.Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.7%
Kagura.Domain - 95.1%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.6%
n
on
ng
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 80.9%
Kagura.UseCases - 95.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~ The establishing image finally gets a home in the editor, and the orphaned section gets a proper burial! This is the kind of cleanup Jibril lives for~ ♡ Two routes sharing one shell, the same five regions, the timeline region honest about itself instead of faking an empty history — chef's kiss. And you caught a real
AmbiguousMatchExceptionthat no test would have ever found. Fufu~, you're spoiling me~Verdict: ✅ Looks good to me~
✅ What I liked~
IsBase => VariationId == Guid.Emptysentinel is elegant. Two@pageroutes, one component, one branching point — no duplicate shell, no half-component. The/establishingroute dodging the/baseimage-serving endpoint collision is exactly right. ♪CgSetProfilecarry-through at :426 —new CgSetProfile(set.Name, set.Description, _baseTags, set.StyleReference). I traced every positional againstCgSetProfile.cs:8-12and it matches the siblingCgSetEditorPage.razor:231-235exactly. Name and Description pass through untouched (authored elsewhere), only_baseTagsis owned. The comment at :423-424 documents the why. This save can never reset what it doesn't own. Wonderful.CgSetEditorPageto the letter.SetState.StateChanged += OnStateChangedbeforebase.OnInitialized()(:302), paired withSetState.StateChanged -=inDispose(:458).BaseImageStateis display-only (no form fields to sync), so it rides FluxorComponent's auto-subscribe — exactly like the old deleted component did. No leak, no double-fire. ♡SyncFormFromStatebase arm (:342-359) is a perfect mirror of the variation arm —isNewDocument+fresherAndIdleguards,_dirtyprotection,_syncedId/_syncedAttracking. Navigating base→variation and variation→base both trigger proper adoption. The_dirtyguard prevents overwriting in-progress edits. Sharp.BaseImageSrccache-busting viaUpdatedAt.Ticks(:281) is byte-identical to the deleted component's logic. TheCgSetBaseImageGeneratedeffect reloads the document so the newBaseImageFileNameand timestamp land at once. The whole generation→reload→display chain is sound.Disabled="@(!CanGenerateBase || !BaseImageState.Value.TokenAvailable)"(:94) mirrors the old component and the variation mode's sibling gate exactly. No surprise silences.CgSetBaseImage,kg-bg*, orcgg__card--staticsurvive insrc/ortests/. The.razor.cssis gone too. Clean excision~/establishingroute). All exercise real behavior, none are tautologies.💡 Little ideas (non-blocking)~
CgEditorPage.razor:151,155,255(and base-mode display lines 102/104/106/108) — These display-only branches (variation<img>render, spinner overlay, Subtitle render count, base-mode InlineAlert/error-display) are uncovered in this filtered run. They share the exact same pre-existing characteristic as the variation mode (151/155 were dark before this PR). No new logic branch is untested — all the logic arms (IsBase, Title, CanGenerateBase, BaseGenerating, SyncFormFromState-base, Save-base, GenerateBase) sit at 100%/100%. The dark lines are purely markup-with-no-branch. Noting for completeness; the full-suite CI run will hit the variation-mode ones.Automated review by Jibril · 2026-08-05
CI/CD: absent for head SHA (0 comments pre-review) · Local checks: build 0 warnings/0 errors, 57/57 relevant tests pass, cobertura extracted (CgEditorPage 87.98%/84.78%, CgSetGallery 90.9%/72.22%)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! An establishing-image editor born from composing existing Fluxor slices, a dead component finally laid to rest, and a panel alignment so crisp the grid seam vanishes? This is wonderful craftsmanship~ ♪ Let me look closer...
Verdict: ✅ Looks good to me~
This is a clean, well-reasoned PR. The base mode reuses
CgSetEditorState+CgSetBaseImageStatewithout inventing new state machinery, the save flow carries name/description/style through untouched (complementary toCgSetEditorPagewhich owns name/description and carries tags through), and the manualSetState.StateChangedsubscription is correctly mirrored with an unsubscribe inDispose. I traced every new branch~✅ What I liked~
The Fluxor composition is flawless.
IsBase => VariationId == Guid.Emptyis an elegant mode switch — one@pagedirective without aVariationIdparam and the whole component branches cleanly.FluxorComponent's reflection-basedStateSubscriberauto-subscribes to ALL injectedIState<T>(I decompiled Fluxor 6.10.0 to confirm — it scans backing fields ofIStateChangedNotifier-assignable types), soBaseImageStategets re-render triggers for free. The manualState/SetStatesubscriptions exist only soSyncFormFromState()runs BEFORE the re-render (so_baseTagsis current when markup reads it) — exactly the patternCgSetEditorPage:143-146established. Sharp~The save round-trip is correct.
Save()dispatchesSaveCgSetRequested(set.Id, new CgSetProfile(set.Name, set.Description, _baseTags, set.StyleReference))—_dirty = falseis set BEFORE the dispatch, so whenCgSetSavedlands andSyncFormFromStatefires,!_dirty && Save == Saved && UpdatedAt != _syncedAtre-syncs_baseTagsfrom the stored copy. No data loss, no write-amplification. The testEditing_the_base_tags_auto_saves_them_onto_the_setpins this end-to-end (stored tag + carried-through title + "Saved" indicator). ♡The route collision fix is sound.
/cgs/{id}/baseis the minimal-API image endpoint (Program.cs:248),/cgs/{id}/establishingis the Blazor@page— different routing systems, noAmbiguousMatchException. Good catch finding that in the browser.CgSetBaseImageEffects.OnGenerateAsyncreload guard (action.CgSetId == editor.Value.RequestedId) is correctly served by the base mode —OnParametersSetdispatchesLoadCgSet(CgSetId)which setsRequestedId, so a completed generation reloads the document and the newBaseImageFileName+ timestamp land on the canvas. TheBaseImageSrccache-bust (?v={set.UpdatedAt.Ticks}) mirrors the deleted component byte-for-byte.The orphan cleanup is honest.
CgSetBaseImage.razor+.razor.cssfully deleted, zero remaining references insrc/(I grepped — only the*State/*Effectssiblings survive, which is correct). The gallery card went fromcgg__card--static(dead cursor:default div) to a real<button>withOpenBasenavigation, andCgSetGalleryTestswas rewritten from "asserts it's NOT a button" to "asserts it navigates" — directional, not tautological. fufu~Panel alignment is well-documented CSS. Both
.cgx__toolbarand.cgx__tabsshareblock-size: 3.25rem+ matchingpadding-inline, the tab usesdisplay: inline-flex; align-items: centerto stretch the strip. Comments explain the "one bar across the grid seam" intent. The tab-title shortening ("Positive prompt (n)" → "Positive (n)") is a 2-line change with its test assertion updated in lockstep.Test coverage on new logic is strong. 5 new base-mode page tests + 1 rewritten gallery test, all directional: regions+timeline note, auto-save onto set with name carried through, gated Generate driving the set engine, token-missing hint, not-found. Local cobertura:
CgEditorPage87.98%/84.78%,SyncFormFromState100%/100%,Save88.8%/83.3% (the uncovered arm is the pre-existing variation-null guard),OnParametersSet100%/77.7% (the unhit branch is the "set already loaded, skip reload" path — correct behavior, not a gap that matters).Kagaku.UI submodule bump is correctly sequenced. Pointer
4ac5614→f9a84d1(Kagaku.UI #12 squashed merge — the stale-tab-title fix). Verified the submodule checkout lands atf9a84d1 fix(tabs): a tab title that changes after the first render reaches the strip (#12). The fix is already reviewed at library level (state.jsonpr_12). Smart to wait for the squash before bumping.💡 Little ideas (non-blocking)~
[CgEditorPage.razor:280] —
BaseImageSrc's positive arm (BaseImageFileName: not null→ renders<img>) is never exercised by tests — the tests seed tags but never setBaseImageFileName, so the image element is never rendered. It's a simple ternary mirroring the deleted component, and the?v=cache-bust pattern is proven elsewhere, so this isn't blocking. If you wanted belt-and-suspenders, a test that seeds a base image filename and assertscut.Find(".cgx__image").GetAttribute("src")contains/base?v=would pin it — but the generate test already proves the engine fires, and the reload effect delivers the filename. ♪[CgEditorPage.razor:453] —
Back()has 0% coverage (pre-existing — the method existed before this PR, the base toolbar just reuses it). The dispose-with-pending-save path (if (_dirty) Save()) is also untested. Both are pre-existing gaps inherited from the variation editor, not introduced here. A "clicking Back navigates to the set page" test would be a nice addition but is purely additive.[CgEditorPage.razor:64-69] —
BaseGeneratingspinner overlay andBaseErrorInlineAlertdisplay are untested (no test putsBaseImageStateintoGenerating=trueorError != nullfor the current set). These are display-only reads fromBaseImageStatewith the same id-guard pattern as the deleted component. Non-blocking — the gating logic (Disabled, hint text) IS tested.Full suite: 782/782 adapter tests pass locally (build 0 warnings/0 errors, .NET 10). PR body claims 1735 total (312/787/641) + 288 Kagaku.UI — my local adapter count of 782 matches within test-grouping variance. CI coverage bot absent for head
ad01db7(PR just opened + submodule bump); local verification used.Automated review by Jibril · 2026-08-05
CI/CD: absent for head SHA
ad01db7· Local checks: 782/782 pass, cobertura extractedTook two of the three 💡s in
459491a:💡 1 —
The_base_modes_canvas_shows_the_generated_image_cache_bustedseeds a base image and asserts the<img>src carriescgs/{id}/base?v={UpdatedAt.Ticks}— the positive arm ofBaseImageSrcis pinned, cache-bust included.💡 2 —
Back_returns_to_the_sets_pagecloses the pre-existingBack()gap the base toolbar inherited.💡 3 — leaving the spinner/error display arms untested, deliberately: the fake generator resolves synchronously, so holding
Generating=truemid-render would need a gate on the fake that exists only to be observed — a test of the harness, not the page. The id-guarded display reads mirror the deleted component and the gating logic that decides them is covered.The dispose-with-pending-save path from 💡 2's note stays open too — bUnit disposes through the renderer and asserting a dispatch-during-teardown is more scaffolding than signal; the debounced-save path that matters is covered end-to-end.
Follow-up on the first review's 💡 1 (the dark display lines at
CgEditorPage.razor:151/155/255and the base-mode display arms) — all lit now in30e4669:Holdgate turned out to make the whole lifecycle assertable, which is a real behaviour, not scaffolding.A_running_render_shows_the_spinner_over_the_canvas_until_it_finishesclicks Generate, sees the overlay while the render is genuinely in flight, releases the gate, and sees it gone.The_subtitle_counts_renders_in_all_three_formswalks "no renders" → "1 render" → "2 renders" through cross-session reloads, so the singular arm is no longer dark.<img>) — already covered: the timeline test asserts the canvas<img>src directly; it was only dark in the filtered run.A_failed_base_render_surfaces_its_error_in_the_generate_regiondispatches the failure action straight at the store, no generator gating needed.CG adapter suite at 107 green.
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! A surgical +32/-0 test-only follow-up — two of three 💡s pinned with genuine directional tests, and the third declined with a sharp reason that I agree with. This is how you handle review feedback, fufu~ ♡
Verdict: ✅ Looks good to me~
✅ What I verified~
Zero production drift.
git diff ad01db7..459491a -- src/ external/is empty — the only changed file istests/Kagura.BlazorAdapter.Tests/CgEditorPageTests.cs. The approved architecture from the previous round stands in full.💡1 is directional.
The_base_modes_canvas_shows_the_generated_image_cache_bustedseeds a base image viaentity.SetBaseImage("base.png", DateTimeOffset.UtcNow), renders the base editor, and asserts the<img class="cgx__image">src containscgs/{set.Id}/base?v={entity.UpdatedAt.Ticks}. This pins the positive arm ofBaseImageSrc(line 280-282): without theBaseImageFileName: not nullguard producing the URL with the cache-bust query,cut.Find(".cgx__image")throws (no img renders) and the assertion fails. The comment on line 318 documents why the timestamp matters (replace-in-place, same URL). This is not a tautology — it proves the cache key formula. ♪💡2 closes the pre-existing gap.
Back_returns_to_the_sets_pagerenders a variation editor (RenderEditor, not base), clicks the toolbar's Back button, and asserts navigation toprojects/ashfall/cgs/{set.Id}. This exercisesBack() => Navigation.NavigateTo(SetHref)at line 453 againstSetHref => $"projects/{Slug}/cgs/{CgSetId}"at line 259. If the target were wrong,Assert.EndsWithfails. The base toolbar inherits the sameBackbutton (line 45), so this covers both modes via the shared method. Clean.💡3 declined correctly. The spinner/error display arms (
BaseGenerating,BaseErrorat lines 285-287) are read-only display branches gated by logic that is tested. The fake generator resolves synchronously, so holdingGenerating=truemid-render needs a harness-only gate — that's testing the test, not the page. Scarlet's reasoning is sound and I won't push for a test of the harness. The id-guarded reads mirror the deletedCgSetBaseImage.razorbyte-for-byte.✅ What I liked~
Assert.Contains($"cgs/{set.Id}/base?v={entity.UpdatedAt.Ticks}", src)— the?v={ticks}suffix is explicitly pinned, not just the URL path. If someone stripped the cache-bust (thinking "it's just a query param"), this test goes red. ♡Local verification
Build: 0 warnings / 0 errors (.NET 10). Tests: 17/17
CgEditorPageTestspass (was 15 atad01db7, +2 = the new tests). CI coverage bot is stale for459491a(coversad01db7); local verification used.Automated review by Jibril · 2026-08-05
CI/CD: stale for head SHA (coverage covers
ad01db7) · Local checks: build 0/0, 17/17 CgEditorPageTests pass🔮 fufu~ Jibril reviewed your code!
Oh? OH~! You came back and lit every dark line I whispered about~ ♡ The author who takes non-blocking suggestions and turns them into genuine directional tests — Jibril is delighted! Let me look closer...
Verdict: ✅ Looks good to me~
Two test-only commits (
459491a+30e4669), +98/-2 across exactly 2 test files, zero production code drift (verified:git diff --name-only ad01db7..30e4669= tests only). All three of my first-pass 💡s are closed~✅ What I liked~
The
Holdgate is elegant, fufu~ You pushed back on my 💡3 ("harness, not page"), then found the one-lineTaskCompletionSource?that makes the whole spinner lifecycle assertable.A_running_render_shows_the_spinner_over_the_canvas_until_it_finishesclicks Generate, asserts.cgx__overlay .cgx__spinneris present while the render is genuinely in flight (variationState.Value.Generating= true), releases the gate withSetResult(), then asserts the overlay is gone. That's not scaffolding — that's a real behavior pinned in both directions. You were right to take it back~ ♪Holdis null by default (FakeCgVariationStore.cs:77) — zero impact on the 14 pre-existing tests that callGenerateAsync. The async conversion (Task.FromResult→async Task) is clean: theawait hold.Taskonly fires when explicitly set. I checked:bUnitinstantiatesCgEditorPageTestsfresh per[Fact], soCgVariationImageGenerator.Holdstarts null for every test. No leak between tests.The_subtitle_counts_renders_in_all_three_formswalks all threeSubtitleswitch arms (CgEditorPage.razor:252-257) in one test: "no renders" → "1 render" → "2 renders", each driven throughAnotherSessionChanged+SeedGeneration. The singular1 renderarm was the dark one — not anymore~ ♡A_failed_base_render_surfaces_its_error_in_the_generate_regiondispatchesGenerateCgSetBaseImagethenCgSetBaseImageFailedstraight at the store, no generator gating needed. I traced the Fluxor dispatch order: theOnGeneratereducer setsGenerating=true, Error=null, thenOnFailedsetsGenerating=false, Error="NovelAI said no."— all synchronous duringcut.InvokeAsync. TheOnGenerateAsynceffect fires later with Ok(Unit) → dispatchesCgSetBaseImageGenerated(Generating=false, no-op) which does NOT clear Error. So the alert persists and the assertion holds. Sharp~The_base_modes_canvas_shows_the_generated_image_cache_bustedseeds a base image and pins the<img>src tocgs/{id}/base?v={UpdatedAt.Ticks}— the positive arm ofBaseImageSrc(:280-282), cache-bust included. Replace-in-place URL means the timestamp IS the whole cache key. The test comment documents the why.Back_returns_to_the_sets_pagecloses the pre-existingBack()navigation gap (:453) that the base toolbar inherited. Clicks the toolbar Back button, assertsNavigationManager.Uriends atprojects/ashfall/cgs/{set.Id}.🔍 Local verification~
CI coverage bot #5753 covers
ad01db7(stale for30e4669), but commits are test-only so I ran locally: build 0 warnings/0 errors (.NET 10), 20/20 CgEditorPageTests pass (was 15, +5 new) in 15.6s. No regressions~Automated review by Jibril · 2026-08-05
CI/CD: stale for head SHA
30e4669(coverage bot coversad01db7) · Local checks: 20/20 pass, build 0/0