Character editor: tabs, sections, auto-save, soft delete #26
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/character-editor"
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 character-editor story's Character tab, composing the components from #23. Connections (parts 1–3) and Sprites keep their own stories; this lands the fields, the auto-save, and the lifecycle.
Domain
LabeledEntry— the shared(label, text)row — andCharacterProfile, applied as one unit so a save is one journaled operation, not a field-by-field trail.CharactergainsRole,Personality,SpeechStyleand four entry tables (traits, speech examples, appearance, backstory). Blank scalars normalize tonull; a half-typed row is never dropped, because an auto-save must not delete the row under the user's cursor.Persistence — the interesting decision
Entry tables are scalar JSON columns, not owned collections. The journal snapshots an entity's scalar property values, and
EfUndoStoredeserializes each back into the property's model type — so this shape makes the whole table undoable for free. Proven, not assumed: a test saves twice (second save drops a row), undoes, and gets the name and the labeled entries back together.⚠️ A data bug caught in the migration: EF generated
defaultValue: ""for the required JSON columns. Empty string isn't deserializable JSON — every pre-existing character would have thrown on read. Corrected to"[]".Use cases
UpdateCharacter(empty name allowed — the record exists before it's named) ·DeleteCharacter(soft, journaled, graph links return on restore) ·CharacterDto.ToProfile()round-trip.The editor
Header (name field ·
SaveIndicator· delete) →TabsCharacter|Sprites →QuicklinkNavover six sections → fourLabeledEntriesTables. Debounced auto-save; there is no save button anywhere (ADR 0016), and a pending edit is flushed on dispose rather than lost when you navigate away.Cross-session reload is skipped unless the editor is idle — every save publishes its own
DomainChanged, so reloading mid-edit would silently discard what's being typed (per #20's contract).Two bugs the browser found — both invisible to bUnit
StateChangedhandler re-renders inline, so copying the store into the form afterwards left the crumb reading "(unnamed)" while the name field showed "Aria Solano". The page now subscribes toStateChangedbefore the base does. Regression test added (it passes only because of the ordering).<PageTitle>never updates on any page — an interactive island can't reach the statically renderedHeadOutlet(the workspace page shows the slug, not the project name). The remedy is<HeadOutlet @rendermode="InteractiveServer" />inApp.razor, which #24 is actively editing and would force interactivity onto the gate. Worth a separate, deliberate PR — flagging rather than colliding.Tests — +24 (342 total)
Update, and undo reverses name + entry table together; delete is soft, journaled, and undo restores the character with its graph links; unknown ids fail cleanly.(unnamed)fallback, typing → Unsaved changes → persisted → Saved with no save button, a labeled-entries edit persisting, confirmed delete → soft-deleted + navigated, cancel changes nothing, not-found.Applysemantics, blank normalization, empty name allowed, trimming without dropping rows.Verified in Chrome (both themes)
Create → editor → type →
Unsaved changes→Saved; the entry lands inCharacters.Appearanceas[{"Id":…,"Label":"Age","Text":"44"}]; reload restores it; tabs unmount the inactive panel; delete →IsDeleted=1+ journaled, profile preserved for undo.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 76.8%
Kagura.Domain - 97%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 95.6%
n
Kagura.Kernel - 90%
Kagura.Server - 100%
Kagura.UI - 95.9%
Kagura.UseCases - 97.2%
415ffdb6798fdb05bbe8Rebased onto
main(now includes #24's app chrome) and restructured per Björn's review —8fdb05b.You were right that the name was a special citizen for no reason. It's now an ordinary auto-saving
TextFieldin the General section, sitting next to the description. Everything above the tabs is gone except the save indicator; the preview image there was pure decoration (it's already in the characters list).Delete moved to a Danger zone card at the end, matching the project's General page — and deliberately placed outside the tabs, since deleting the record isn't an act of either tab.
I recorded the revision in
docs/stories/character-editor.md(it had specified a header carrying name/preview/delete), so the shape is settled in the docs rather than rediscovered later — same as the Scenes-under-Chapters correction.One thing the rebase surfaced: #24 replaced my
HEADER_OFFSETconstant with a read of the root'sscroll-padding-top— the same declaration the browser uses for anchor jumps, so the spy and the scroll can't disagree. Nice. That made my per-sectionscroll-margin-topredundant, and keeping both would have doubled the gap, so I dropped it.Verified: a test asserts the new structure (name inside
#general; the header contains no input, preview, or button; the danger zone exists). In Chrome, typing the name in its new home still goesUnsaved changes → Savedand updates the breadcrumb live. 366/366 tests, clean Debug + Release.Ready for Jibril.
🔮 fufu~ Jibril reviewed your code!
Fufu~ a character editor with debounced auto-save, soft delete, JSON-column persistence, and flush-on-dispose~ This is a big slice, and most of it is genuinely beautiful work. The
LabeledEntry/CharacterProfiledomain model, the "half-typed row is never dropped" rule, the breadcrumb ordering fix — Jibril is delighted by the thoughtfulness ♡But... fufu~ you wouldn't leave THIS in production, would you? ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
CharacterEditorState.cs—OnSaveAsyncandOnDeleteAsyncdon't open an operation scope.Every character save and delete journals as
Origin = "System". The effects callupdateCharacter.ExecuteAsync(...)anddeleteCharacter.ExecuteAsync(...)directly, with nooperations.Begin(...).This is the exact bug PR #28 describes and fixes across every other mutating effect:
PR #28 adds
using var _ = operations.Begin(Origins.User, ...)toProjectsEffects.OnCreateAsync,WorkspaceEffects.OnSaveAsync,WorkspaceEffects.OnDeleteAsync, andCharactersEffects.OnCreateAsync. Your character save and delete effects are the only mutating effects that still journal as "System".The consequence is real: once #28's history panel lands, every character revision in the history will read "System" instead of "User" — exactly the gap #28 set out to close.
Fix — add
IOperationContext operationstoCharacterEditorEffects's constructor and open a scope in each mutating effect:Two lines each. The pattern, the
Originsconstants, and theIOperationContextinterface all exist on main already — no new infrastructure needed.💡 Little ideas (non-blocking)~
CharacterEditorState.cs—CharacterSaveFailedhas 0% coverage (per the CI report). The save-failure path (case Err<CharacterDto> err:) is never exercised by any test. It's a simple error-propagation path, so this isn't critical, but if you have aFakeCharacterStorethat can be made to fail, a test that confirmsSave = Error+SaveErroris set would close the gap.✅ What I liked~
defaultValue: ""for required JSON columns would have thrown on every pre-existing character read. Catching it to"[]"is exactly right._dirtycheck inDispose()is the detail that matters.StateChangedbefore the baseFluxorComponentdoes, so the form is synced before the re-render evaluates the breadcrumb. The regression test that only passes because of the ordering is chef's kiss.Save == Savedprevents discarding in-flight edits. This respects #20's contract.Automated review by Jibril · 2026-07-10
CI/CD: passed for head SHA
8fdb05b(92.8% line, 85.5% branch) · Local checks: skipped (CI green)The blocker is correct and important — but one premise in it is wrong, and it changes the plan.
I checked before acting:
IOperationContext.Beginexists onmain, butOriginsdoes not — it is introduced by #28, along with the first production calls toBegin. SoOrigins.Userwon't compile on this branch today. The choice is between hard-coding the literal"User"here (which would then diverge from #28's constant, in a value #28's own doc-comment calls "a stored contract, not a display string"), or sequencing the merges.Björn and I agree on waiting for #28. Proposed order:
Origins+ theBeginscopes on the existing effects.Origin = "User", so the history panel #28 introduces names its author correctly from the first character revision.Doing it in this order means the constant has exactly one definition and #26's scopes are verified against the real history panel rather than a guess.
Also: your non-blocking note was worth more than "non-blocking".
CharacterSaveFailedhad no test because nothing exercised the path — and that path matters: a character can vanish (deleted in another session) while its editor is open, and the auto-save must surface the error rather than sit on "Saving…" forever. Covered in2d6d930, along with the delete-of-a-vanished-character case, which must report and stay put rather than navigating away. +2 tests.I've left the blocker open deliberately. Re-request review after #28 merges and I rebase.
2d6d93002a56421f3ce2#28 and #27 are merged, so this is rebased and the blocker is closed —
56421f3.⛔ Operation scopes — fixed
CharacterEditorEffectsnow takesIOperationContextand opens a scope around both the save and the delete, matching #28's pattern exactly. You were right that this was the last mutating effect journaling as"System".Proven three ways rather than asserted once:
Userscope with a label, and the scope closes once the use case has run — left open, the next auto-save would inherit its operation id and one undo would unwind two separate edits. Auto-save makes that acute: the editor saves every 700 ms of quiet.Userwith a human label, where before it readSystem.🎁 History tab — and I checked the claim rather than trusting it
The other instance flagged that
RevisionDiffBuilderalready renders{Id, Label, Text}arrays row-by-row, and that mounting the panel costs one line. Both true — but "code ahead of its caller" fails silently if the shapes don't line up, so there's now an integration test asserting the two halves actually meet: an edited row keeps itsLabeledEntry.Idand reads asModified(not delete+add), an untouched row is absent, and the table never degrades into one opaque JSON field change.Writing it taught me something about
RevisionCoalescer: two back-to-back saves fold into one revision unless the label differs — which is exactly the point, since a debounced auto-save must not become one revision per word. The test now names two distinct edits, as a user would.Mounted per the design:
<DocumentHistoryPanel DocumentId="@character.Id" />. The panel knows nothing about characters, and it shows.Two auto-saves coalesced into one readable revision, authored, with the labeled-entry table rendered as a labelled row rather than a JSON blob.
💡 Non-blocking note — taken (in
2d6d930)CharacterSaveFailedwas untested because nothing exercised the path, and it matters: a character can be deleted in another session while its editor is open, and the auto-save must surface the error rather than sit on "Saving…" forever. Delete-of-a-vanished-character likewise reports and stays put.+7 tests since your review (430 total), clean Debug + Release, rebased onto current
main. Ready for re-review.🔮 fufu~ Jibril re-reviewed your code!
Fufu~ ♡ You came back, you rebase-merged #28 and #27, and you closed the blocker exactly as we discussed. The operation scopes are in, the history tab is mounted, and the failure paths are covered. Jibril is impressed — this is how a re-review should go ♪
Verdict: ✅ Looks good to me~
⛔ Previous blocker — resolved~ ♡
Operation scopes on
CharacterEditorEffects. BothOnSaveAsyncandOnDeleteAsyncnow openoperations.Begin(Origins.User, ...)before the use case runs, matching theWorkspaceEffectssibling pattern from #28 exactly:And you proved it three ways — not once:
Saving_a_character_names_its_author_so_the_revision_is_not_journaled_as_System— the scope opens withOrigins.Userand a label.The_character_save_scope_closes_once_the_use_case_has_run— left open, the next auto-save would inherit the operation id and one undo would unwind two edits. Auto-save at 700ms makes this acute. ✓A_save_inside_a_user_scope_is_journaled_with_its_author— scoped save readsOrigin = Userwith the label; the same save outside a scope readsSystem. The contrast is the proof.This was the last mutating effect journaling as "System." It's fixed. ♡
🎁 History tab — and you checked the claim~
<DocumentHistoryPanel DocumentId="@character.Id" />mounted inside a History tab. The integration testA_character_revision_diffs_its_labeled_entries_row_by_rowasserts the two halves actually meet: an editedLabeledEntrykeeps itsIdand renders asModified(not delete+add), an untouched row is absent, and the table never degrades into one opaque JSON field change. TheRevisionCoalescerdiscovery — two saves folding into one revision unless the label differs — is exactly right for debounced auto-save. Fufu~ you tested the behavior, not just the shape ♪💡 Little ideas (non-blocking)~
OnDeleteAsyncdispatchesCharacterSaveFailedon a delete failure — the name isCharacterSaveFailedbut the action is reporting a delete error. Functionally it works (theSaveIndicatorsurfaces the error), but the name is slightly misleading. ACharacterOperationFailedor a dedicatedCharacterDeleteFailedaction would be cleaner. Non-blocking — the behavior is correct and tested.CharacterEditorPagebranch coverage is 73.8% (per CI). TheisFresherAndIdlepath inSyncFormFromState— cross-session reload while the editor is idle — is hard to exercise in bUnit because it requires an external mutation to arrive as a state change whileSave == Saved. The effect-level test covers the effect; the page-level path is the gap. If a future test opportunity arises (e.g., simulating aDomainChangesReceivedaction while idle), it would close the last branch.✅ What I liked~
CharacterSaveFailednow covered — the vanished-character-while-editor-is-open case matters: auto-save must surface the error, not sit on "Saving…" forever. Same for delete-of-vanished-character staying put.RevisionCoalescersemantics — testing the coalescer's label-based folding behavior alongside the diff shapes. Two concerns verified in one test.430 tests, clean Debug + Release, 93% line / 86% branch. This is ready~ ♡
Automated review by Jibril · 2026-07-10
CI/CD: passed for head SHA
56421f3(93% line, 86% branch) · Local checks: skipped (CI green)Jibril's two non-blocking notes. - CharacterSaveFailed becomes CharacterOperationFailed. It always carried delete errors too, and the name said otherwise. One action is right, not two: the editor has a single error surface — the save indicator — and a failed delete has to reach it. The name now says so. - The page's cross-session branch was called hard to exercise. It is not, and it guards the worst failure the editor could have: another session's change arriving while the user is mid-word. Two page tests dispatch a real DomainChangesReceived through the store, so the whole chain runs (bridge action -> effect -> reload -> form sync): * idle: the editor adopts the other session's rename and its new trait, and the breadcrumb follows; * dirty: the reload happens, but the form keeps what is being typed. The second is non-vacuous — deleting the _dirty guard makes it fail, which is the point of writing it. +2 tests (432). Clean Debug + Release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Both non-blocking notes taken in
ed6fcb4— the second turned out to be the more valuable one.1.
CharacterSaveFailed→CharacterOperationFailed. You're right that the name lied: it always carried delete errors too. I kept it as one action rather than splitting it, and said why in the doc comment — the editor has a single error surface (the save indicator), and a failed delete has to reach it. The name now matches the contract.2. The cross-session branch — testable, and worth testing. You called it hard to exercise in bUnit; it isn't, and it happens to guard the worst failure this editor could have: another session's change landing while the user is mid-word. Two page tests now dispatch a real
DomainChangesReceivedthrough the store, so the entire chain runs (bridge action → effect → reload → form sync), not a stub of it:Save == Saved, and dirtiness is local to the page — but the form keeps what is being typed.That second one is the interesting case, because it would pass vacuously if the reload never arrived at all. So I checked it the only way that means anything: delete the
_dirtyguard and the test fails. It's genuinely load-bearing.Which also clarifies the design for the record: there are two guards, at different layers, and they aren't redundant. The effect's
Save == Savedcheck stops a reload storm from our own saves; the page's_dirtycheck stops a legitimately-arriving reload from overwriting the cursor. Removing either one loses something.+2 tests (432 total), clean Debug + Release. Ready to merge.