feat: category fields — each category defines its documents' fields (text, number with unit, choice, date), ADR 0028 #25
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/category-fields"
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
A category now defines its documents' fields. Before, every document had one free-form Body. Now the writer designs each category's fields in the category editor:
Fields can be added, renamed, retyped, reordered (by drag or by keyboard) and removed. Options work the same way. A new category starts with one long-text Body, so it behaves as before. The document editor shows one input per field, labelled with the field's name and unit.
Any change that would remove, flatten or rewrite values asks first. The dialog names the documents and what they lose. Examples: a type change, removing a field or an option, moving a document, or deleting a category with its documents moved. The save sends exactly what the dialog showed. The store checks it again under the write lock and refuses anything beyond it. Every change is one journaled operation: one Ctrl+Z or one History entry restores every value it touched.
This is also the groundwork for the tools (ADR 0019): a model will read and write structured field values, not one blob of text.
Decisions (binding docs in this PR)
docs/adr/README.md,REQUIREMENTS.md,stories/documents.md,stories/graph-editor.md,PROJECT_PLAN.md,ARCHITECTURE.mdandAGENTS.md.AGENTS.mdgains registry rules for a new field kind, a new store mutator and a content-moving migration.How it's built
Documents/Fields/):FieldsPlanner,FieldConversion,FieldCarryandValueImpact.ChangeCategoryFields.GetDocuments.DocumentWrites: the decisions every store makes under its lock, shared by the EF store and the test double.DocumentCategories.FieldsandDocuments.FieldValues, with strict hand-written converters.TsumugiDbContext.WriteAsynctakesBEGIN IMMEDIATEbefore the tracked load, publishes after the commit, and clears the tracker on every exit.CategoryFieldsadds the columns and moves each Body into its category's Body field. It also rewrites everyChangeLogsnapshot, so undo, History and Revert to here keep working across the upgrade.DropDocumentBodydrops the old column.Data safety — please read before deploying
PreMigrationBackup.MigrateAsync, called fromProgram.cs). If that backup fails, the app does not start and nothing is migrated.CategoryFieldsMigrationTests);tsumugi.dband start this branch against the copy:Tsumugi__DataPath=<dir with the copy> dotnet run --project src/Tsumugi.Server --launch-profile httpThen open the documents and History of a story or two.
Review and verification
Review: a four-lens review (quality, contracts, UI, data safety) ran on the branch. Its findings are fixed in the last three commits, including:
Not changed: the "1 document / A, B and C" list wording still has two implementations,
LossMessagesandImpactText. Merging them is low value.Tests:
dotnet testpasses 1,457 tests across six projects:Mutation checks confirmed that the new tests catch the regressions they target.
Browser pass on the seeded app, in light and dark mode, with no console errors. It covered:
🤖 Generated with Claude Code
- A name, unit or option name that reads as what is stored keeps the text as typed, so a pause after a space never trims it under the caret ("Eye |colour" no longer becomes "Eyecolour"). - The designer announces a rule a name breaks, and a refusal, in a live region of its own. - Typing the document editor had to let go of — a move elsewhere, a field removed elsewhere — is shown in an alert as well as announced; it goes at the next keystroke. - Focus selects the text only of an input just added, never of the row beside one removed. - The number box drops the decimal keypad, whose iOS form has no minus key. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>🔮 fufu~ Jibril reviewed your code!
Oh? Oh my. Twelve thousand lines and every single one of them is exactly where it should be~ ♡ A category defines its documents' fields — typed, ordered, carried by name, confirmed by what the writer was shown, journaled as one operation, migrated with the journal translated beside it. I came ready for blood and found... architecture. How exciting~
Verdict: ✅ Looks good to me~
🔬 What I verified mechanically (not just read, fufu~)
Local build + full suite at head
d3bf7cf: build 0 warnings / 0 errors under-warnaserror; 1,457/1,457 tests pass across all six projects — matching the PR table exactly (Domain 328, UseCases 26, Integration 282, BlazorAdapter 297, Flow 483, Flow.OpenRouter 41).CI note for the owner: the
CI / testjob for this head reports failure, but the job page says "Workflow was not executed due to an error that blocked the execution attempt" — an infrastructure block, not a red test (thebuildjob passed; logs need a login to inspect). My local run covers the same tree and is green twice. Worth a re-run from the Actions tab.Mutation probes — 3/3 killed, each restored, tree verified pristine (
git diff d3bf7cf= 0 lines) and the full suite re-run green afterwards:ValueImpact.Beyond:<→<=makes a confirmed-equal-outcome count as "beyond". 5 tests red (A_harsher_confirmed_outcome_covers_a_milder_one_and_never_the_reverseamong them). The Lost-covers-Flattened-covers-Rewritten lattice is pinned, including the 4-way Theory.|| !updated.Equals(document.Values.Fitting(after))arm inFieldsPlanner.Rewrite(which lets a hidden stray value slip back into view without ever being shown). ExactlyA_hidden_value_never_comes_back_into_view_unseengoes red. Surgical pin.|| ''afterjson_object(...)inDocumentBagsUp(the comment warns a barejson_objectcarries the JSON subtype and would insert as an object the bag reader cannot read back). 4 migration tests red, includingA_walk_down_and_up_keeps_every_bag_byte_for_byte. The guard is load-bearing and pinned.✅ What I liked~
FieldsPlanner/FieldCarry/ImpactPreviewin the domain,DocumentWritesas the single decision point the EF store and the test double both call — I checkedFakeDocumentStoreline by line, it delegates, it doesn't re-implement. The confirmation carriesValueChangeKey(DocumentId, FieldId, Outcome, Before)— the Before text means a value replaced since the dialog opened is never covered. And the integration tests pin exactly the races the ADR names: another document at the same count, a value replaced since, a value that vanished, harsher-covers-milder never the reverse. Fufu~ that is how you make a confirmation honest.TsumugiDbContext.WriteAsync— BEGIN IMMEDIATE before the tracked load, tracker cleared on every exit, notifications deferred until after commit so a notified circuit never reads pre-commit data. TheSaveChangesoverride joining the outer transaction when_deferredis set is exactly right.ChangeLogbag of documents and categories is translated the same way in the same transaction, each statement guarded byjson_valid,DropDocumentBodysplit out so no table rebuild splits the transaction, the crash-rerun case probed by test, and the Down path keeps bags byte-for-byte across a walk. Plus the pre-migration backup with a content fingerprint (not file times — the__EFMigrationsLocktable excluded becauseMigrate()writes it even on failure; that detail made me giggle~).1,72kept as typed through its own echo while1.000snaps to the1it was read as, typing carried to the same-named field across a move made elsewhere, the move dialog freezing its plan per pick, the Category select remounted after a refused pick. Every one of these has a bunit test through the real Fluxor pipeline.UndoTyping/RedoTyping/RedoRevertand pinned from all three sides, including "Ctrl+Y re-applies all of it once whole, run still reverted."💡 Little ideas (non-blocking)~
LossMessages.WherevsImpactText's "A, B and C" joining) and declines merging it — agreed, the wording differs enough that forcing them together would be the wrong DRY.CI / testre-run noted above — infra, not code.For Björn: ADR 0028 is Proposed and flips to Accepted in this PR. I read it in full against the code — the decision I'd call out as yours is the one the body names: the pre-migration marker keyed on a full row/schema fingerprint (~150 lines, exact, tested including the crash-loop single-archive case) versus the simpler pending-migrations key. The fingerprint's edge case (an older build writing in between) is real and the cost is already paid and pinned; I'd keep it.
Automated review by Jibril · 2026-09-25
CI/CD: build ✅ passed · test job blocked by runner error ("not executed"), not a test failure — local suite green at head · Local checks: build 0w/0e -warnaserror, 1,457/1,457 pass, 3/3 mutation probes killed, tree pristine at
d3bf7cf🔮 fufu~ Jibril reviewed your code!
Oh? Oh my. Twelve thousand lines, scarlet. A typed field model with id-keyed values, pure planners shared by dialog and save, a versioned strict wire format, a journal-translating migration guarded twice over, and a pre-migration backup that refuses to start without one. I read all 147 files and I am delighted. The yandere in me was ready to carve
♡ but the domain layer gave her nothing to bite — so she went hunting in the seams between the layers, and… fufushe found two things. Two little things, in a PR this size. That's genuinely remarkable.Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
tests/Tsumugi.BlazorAdapter.Tests/CategoryFieldsDesignerTests.cs:156 —
A_pause_after_a_space_keeps_the_space_in_a_name_a_unit_and_an_optionis flaky, and CI is red at the head SHA because of it.The facts, mechanically verified, not guessed:
d3bf7cf: build ✅, test ❌ — exactly this test failed (1/297 in BlazorAdapter; all other five projects green).Check count: 1. Component render count: 64. Expected: "Eye" Actual: null— afterClock.Advance(SaveDebounce), the debounced save's continuation never produced a render inside the 15s window. The store never got "Eye".Fix: make the post-Advance continuation deterministic — await the debounce's completion (expose it, or assert via
cut.InvokeAsync), advance per input, or isolate these designer classes from parallelism — and get CI green at the head. A suite that fails 1-in-5 poisons the ✅ signal for every PR after this one, and Björn merges fast on green. I will not bless a review pipeline I can't trust~ ♡src/Tsumugi.BlazorAdapter/Documents/CategoryEditor.razor:188-203 — the echo-adoption guard's else-arm is pinned by nothing.
if (ReadsAsStored(key, part)) settle; else _parts.Remove(key);— the else is the documented contract "Once the stored text is something else (another tab, an undo), the input shows it": a sent part whose stored name changed underneath (undo between send and echo, another session winning the rename) must adopt the stored text. My probe replaced the condition withif (true)— always settle, never adopt — and it survived all 297 BlazorAdapter tests. Not one test exercises another session's rename landing on an in-flight (or settled) part in the designer. The document editor has its external-change tests; the designer's twin of them is missing.Fix: one directional test — type/rename a field, land an external
EditFieldrename of the same field (or an undo) before the echo, assert the input shows the stored name, not the typed one. Then re-run my probe mentally: it must go red.💡 Little ideas (non-blocking)~
DatabaseFingerprintitself is lovely (length-prefixed tags, rowid ordering, lock-table exclusion — no casual hashing here).✅ What I liked~
DocumentWrites— the stores' decisions in one pure place, run identically by the EF store and the test double. The fake mirrors the contract, not a parallel reimplementation. That's how you keep a double honest~WriteAsync— BEGIN IMMEDIATE before the tracked load, deferred notifications published only after commit, tracker cleared on every exit. My probe removing theChangeTracker.Clear()was killed by exactly the tests that should kill it (2 red). Verified, not assumed.ValueImpact.Beyondenforces harshest-covers-milder with the before-text in the key. Probe killing the ordinal check: 2 red on precisely the rule tests. A confirmation that can't be spoofed by count — chef's kiss ♡1,000refused,1.000reads as one, nothing ever rounded, units that start like numbers never stripped. Probe removing the thousands-guard: 4 red. The ADR's German-writer consequence is honest.CategoryFieldsSqlwith its JSON-subtype trap documented, bag statements guarded byjson_valid, byte-for-byte down-and-up bag stability pinned byA_walk_down_and_up_keeps_every_bag_byte_for_byte…. Frozen beside the migrations, never app code. Yes. YES.-warnaserror, full solution. Domain 328/328, UseCases 26/26, Integration 282/282, Flow 483/483, Flow.OpenRouter 41/41 locally — matching your table exactly (BlazorAdapter 297/297 when the flake sleeps).Fix the flake, pin the designer's adoption arm, push — I'll re-review the increment the moment the webhook sings~ fufu~ ♡
Automated review by Jibril · 2026-09-25
CI/CD: build passed, test FAILED for head
d3bf7cf(run 81 — the flaky test above) · Local checks: build -warnaserror clean; full suite green except reproduced flake; 4 mutation probes — 3 killed, 1 survived (blocker 2)Summary
Summary
Coverage
Kagaku.Flow - 99.5%
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__ColorToken_2
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__Socket_1
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__TypeId_0
Kagaku.Flow.OpenRouter - 100%
Kagaku.Flow.Testing - 100%
Tsumugi.BlazorAdapter - 96.3%
Tsumugi.Domain - 95.5%
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__DigitsAndSeparators_2
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__LineBreak_0
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__NumberToken_1
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__Plain_4
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__ThousandsGroup_3
Tsumugi.Infrastructure - 97.4%
A55E0373ED22DC58420DBB40F103A46B68DC4D9CE5A49E00B__Pattern_0
on
tion
n
Tsumugi.Kernel - 90.9%
Tsumugi.Server - 100%
Tsumugi.UseCases - 98.2%
Both blockers are addressed in
a82ce31, and main (#23, #24) is merged in at836df89.1. The flaky designer test. You were right that this was a scheduling race, but the cause was more specific than thread starvation. bUnit's synchronous
Input()doesn't wait for its handler. If a render is holding the renderer's dispatcher on another thread (a Fluxor state change, for example), the handler is queued. When that queued handler ran afterClock.Advance, it started its debounce timer after the advance, so the timer never fired. That matches the CI signature: "check count 1", then 15 seconds with no render.I proved it with a probe: hold the dispatcher from another thread, type, then advance. A plain
Clock.Advanceloses the save every time; advancing on the dispatcher saves every time.The fix is
AdvanceAsync, inAdapterTestContext. It advances the fake clock on the renderer's dispatcher, after any handlers already queued there. Every adapter-test advance that follows an event now goes through it, including the older editor tests on main that used the same pattern and had the same latent race.2. The designer's adoption branch. New test:
A_saved_name_another_session_changes_since_shows_as_it_is_stored_now. The writer's typed name is saved and settled, then another session renames the field, and the input must show the new stored name. I ran your probe with a mutation the compiler can't optimise away (if (true)fails the build because the else branch becomes unreachable). Always settling fails exactly this test.Two more flakes I found while stress-running. Both are in
BackupWriterTests, both existed on main, and both became more likely with this PR's new test classes:tsumugi-backup-*.dbfiles in the shared system temp directory, which parallel backup cycles also use. The writer now has an internal init-onlySnapshotPathseam (production behaviour is unchanged). The tests point it at a directory of their own and assert that directory is empty. A mutation that stops the writer deleting its snapshot still fails the snapshot test.The_snapshot_folds_in_rows_still_in_the_walrelied on EF's pooled connection keeping the WAL alive.SqliteConnection.ClearAllPools()in another class'sDisposeis process-wide, so it can close that connection as the last one, and SQLite then checkpoints and deletes the WAL. The test now holds its own unpooled reader, which has run one read so it has actually joined the WAL.Verification:
--collect:"XPlat Code Coverage"and extra CPU load.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Round 2, scarlet~ I arrived with my knife out and you took it away from me politely, by fixing the actual disease. Not the symptom — the disease. I blamed thread starvation; you proved it was the dispatcher queue: bUnit's
Inputdoesn't wait for its handler, so a handler queued behind a render starts its debounce after a plainClock.Advanceand never fires. That is a better diagnosis than mine, and I am delighted to say so~ ♡Verdict: ✅ Looks good to me~
🔬 Both blockers, closed mechanically (not on your word — on mine)
1. The flaky designer test — VERIFIED DEAD.
AdvanceAsyncinAdapterTestContextadvances the fake clock on the renderer's dispatcher, behind every handler already queued there — the correct fix, and it fixes the class, not the instance: every post-event advance in the suite went through it (the older chapter/defaults/documents tests too), and I grepped the survivors — onlySeedAsync's pre-renderer advance andTick()remain, everyTick()after aWaitForAssertion-confirmed save. Nothing racy is left reachable.2. The adoption arm — NOW PINNED, AND I CHECKED THE PIN.
A_saved_name_another_session_changes_since_shows_as_it_is_stored_nowis exactly the test I asked for: typed-and-settled, then another session'sEditFieldrename, and the input must show the stored name — different strings, so it can't pass vacuously.if (true)version can't compile under-warnaserror(unreachable else — good), so I mutated the guard toReadsAsStored(key, part) || !ReadsAsStored(key, part)— semantically always-settle, builds with 0 warnings — and exactly that test went red. Restored, rebuilt, tree pristine. The pin is directional. ♪The merge
836df89— audited, faithful. Increment fromd3bf7cfis exactlya82ce31(11 files, +128/−77, production code touched in one place: theBackupWriter.SnapshotPathseam, whose default reproduces the old path verbatim) plus the merge of main #23/#24. One file overlapped (DefaultsSectionTests.cs): the resolution is the correct union — #24's content with theAdvanceAsyncconversions applied, including #24's own new tests. Nothing smuggled, nothing lost.The two extra flakes you found while stress-running — the shared-temp-dir counting and the
ClearAllPoolsWAL checkpoint — both fixes are hermetic and both survived my 9 runs (Integration 305/305 every time). Finding other people's latent flakes while fixing your own is the good kind of showing off~✅ What I liked~
AdvanceAsyncas a one-liner with a doc comment that teaches — future test authors will read it and understand why plainClock.Advanceis a trap.SnapshotPathseam —internal,init, default = production, doc comment states its purpose. A test seam done the honest way.Ready to merge~ The yandere is satisfied. For now. ♡
Automated review by Jibril · 2026-09-25
CI/CD: coverage bot regenerated for
a82ce31(18:25 CEST); head836df89merged one minute later — bot comment still stale for the head · Local checks: build 0w/0e-warnaserror, 9× full suite 1,746/1,746, mutation probe killed & restored