feat(editor): dialogue highlighting — quoted speech in a colour of its own (editor appearance A6) #77
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/editor-appearance-dialogue"
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?
This is slice A6, the last of the editor appearance track (ADR 0031, stories/editor-appearance.md). Once a dialogue colour is set in either theme, quoted speech shows in it while the writer types.
What's in it
dialogue.ts, the scan. It reads a paragraph's visible text, where a line break counts as one character, just as it is one position. The rules are the story's:“ „ « » "open speech only at the paragraph's start, or after whitespace, an opening bracket or a dash. So…sagte er“and5'11"stay narration.“…”,„…“,„…”,«…»(spaces inside allowed),»…«,"…". Spans don't nest.dialogue.ts, the plugin. It decorates each piece of speech withspan.tsm-dialogueand never touches the document.compositionend, on the samesetTimeout(0)that releases the session, the module dispatches a transaction carrying only a rescan meta, and the held paragraphs are scanned then.dialoguemount option installs the plugin. Without it, the plugin isn't installed and costs nothing.ChapterPagepasses it toManuscriptSurfacewhile the stored appearance gives dialogue a colour.surface.csscolours the span with--tsm-dialogue. The span sits innermost, inside the marks, so speech is one colour with its italic and bold included. An unset side of the colour falls back tocurrentColor.EditorPreviewSampleand the shared fixture fix:Tests
vitest
dialogue.test.ts:vitest
dialogue-plugin.test.ts, usingprosemirror-statewith no DOM:vitest: 241 passed.
Browser suite, Chromium and Firefox:
127 passed; the 5 skips are the 4 existing ones plus the input-method test in Firefox.
bUnit:
emandstrong;dialogueonly while either side of the dialogue colour is set.BlazorAdapter 724 passed, five full runs in a row. While making these runs I fixed a test race in A5's ordering test: its hand-dispatched answer took one off the pending count, so it now waits for both writes rather than the indicator.
Every other suite passes: Domain 517, UseCases 499, Integration 836, Kagaku.Flow 1103, OpenRouter 592 (+4 skips).
Mutation probes, each killed:
compositionend;Seeded app in Chromium:
"You swore,"shows in#8ecae6, and the narration after it doesn't;🤖 Generated with Claude Code
Review: the general approach
Verdict: the approach is sound, and it matches ADR 0031 point by point. Nothing below blocks the merge. I checked it against the ADR's Dialogue is a decoration bullet:
What works well
changedBlocksmaps each step's range through the steps after it (tr.mapping.slice(i + 1)). So a transaction with several steps, such as a paste, an adoption or a reshape, finds the right blocks in the final document. Removing byfind(pos, pos + nodeSize)doesn't reach into a neighbour: the previous paragraph's decorations end atpos - 1.RESCANdispatch runs in the samesetTimeout(0), aftersession.compositionEnded(). Any remote deltas the session held are therefore released and scanned first, and the meta only picks up what's left.session.applysends nothing for it and history ignores it.destroy(),session.applyreturns early, so a timeout that fires late does no harm.--tsm-emand--tsm-strong.@key="ChapterId"and the route change remount the surface anyway.Small points, none blocking
the word "home" meant…,she read „Faust“. The story's rules allow this and a decoration can't do better without parsing. Still, the story's What stays narration list is precise enough that a reader could take it as complete. One line saying that any quoted stretch counts as speech would set expectations. Optional.EditorPreviewBlockoverridesEquals/GetHashCodeonly because a tuple list breaks record equality. That's correct as written: the hash leavesDialogueout, which is consistent. The cost is a hand-written equality on a public record that the next field added to it has to remember. Keeping the ranges in an equatable wrapper, or comparing them in the test, would avoid that. It's a matter of taste.ManuscriptSurface.Dialoguesays it's "read at mount, as the rest of the appearance is". The rest of the appearance is really a livestylebound to the state. It only behaves like a mount-time read because of the ADR's No restyle once the text shows rule. Something like "read at mount; the ADR accepts a change landing on the next chapter page" would be more exact. Wording only.Overall this is a clean finish to the track: one plugin with a narrow job, one host option, the CSS hook and the settings pair, with tests at each layer.
Summary
Summary
Coverage
Kagaku.Flow - 99.7%
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__ColorToken_2
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__Socket_1
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__TypeId_0
Kagaku.Flow.OpenRouter - 100%
Kagaku.Flow.Testing - 100%
Tsumugi.BlazorAdapter - 96.9%
Tsumugi.Domain - 96.5%
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__DigitsAndSeparators_2
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__LineBreak_0
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__NumberToken_1
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__Plain_4
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__ThousandsGroup_3
Tsumugi.Infrastructure - 99%
A55E0373ED22DC58420DBB40F103A46B68DC4D9CE5A49E00B__Pattern_0
on
tion
n
n
Tsumugi.Kernel - 90.9%
Tsumugi.Server - 99.6%
Tsumugi.UseCases - 98.9%
663A4576626E1322FB9C6CE472CAC0A2D2F3DEFF8CB40D9F7__HexColour_1
663A4576626E1322FB9C6CE472CAC0A2D2F3DEFF8CB40D9F7__LineBreaks_0
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! The last slice of the appearance track~ A decoration that never touches the document, a scan that counts a line break as one character because it is one position, compositions held by segment id and released on the very
setTimeout(0)the session already owns… and you hookedprosemirror-view's owncompositiontransaction meta rather than inventing a parallel signal — I checked the installedprosemirror-view@1.42.6source and that meta is exactly what its DOM reader sets mid-IME. Clever, clever~ ♡But you know what I do to unguarded branches, don't you? ♪
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Tsumugi.BlazorAdapter/ManuscriptEditor/src/dialogue.ts—changedBlocks, the multi-step mapping branch is guarded by no test.The loop
tr.mapping.maps.forEach((map, i) => { const after = tr.mapping.slice(i + 1); … })exists precisely to attribute changes through every step of a transaction — and multi-step transactions are real here:insertBlocks(paste/drop) chainsinsert+deletes, andreconcilechains a step per changed segment. Yet I mutated it to the naive single-step version (maps.slice(-1), identityafter) and all 18 vitest tests still passed — the suite cannot distinguish the careful code from the naive code. That's a missing guard on the exact branch the care is for~I proved the gap real: this test passes on pristine code and fails under the mutant — Under the mutant, the pasted spoken paragraph stays uncoloured — a user-visible bug class nothing would catch. Fix: add that test (a multi-step transaction touching two different paragraphs, ideally one insert-step and one delete-step, asserting the exact spans). One test, and the branch finally has a guardian~
💡 Little ideas (non-blocking)~
dialogue.test.ts:58— the property test's LCG loses its low bits.seed * 1103515245 % 2147483648runs in float64 where products beyond 2^53 drop precision, so the generator's cycle is shorter than it looks. The test still explores fine, but aBigIntLCG or a mulberry32 would explore better. Nicety only~✅ What I liked~
{}spec as a probe surface: mapped keeps its spec, rescanned gets a new one — your own tests use spec identity to prove “mapped, not rescanned”. I'm genuinely delighted~setTimeout(0), andview.composingre-checked so back-to-back compositions don't dispatch early. Respect~AppendTextcut-walk reproducing ProseMirror's innermost-span placement exactly, held to the module by three suites (vitest ranges, bUnit replica, browser blocks) through one shared fixture. That's how you keep a static replica honest.„closing with either“or”— both branches tested, per the story's letter. And the scan's rules (5'11",…sagte er“,« Viens »with spaces) all have names beside their assertions~The code itself is correct — my repro passes on pristine
HEAD. It's the guard that's missing, and a yandere never trusts unguarded branches~ Add the one test and I'll be back to coo over it. Fufu~ ♡Automated review by Jibril · 2026-10-03
CI: green for head SHA
d5c22d07(build, test, editor-browsers) · Mutation probes: 3 run, 1 survived (multi-step mapping → missing test) · Local suite: skipped (CI green); baseline dialogue tests 18/18- The plugin test now walks a transaction that inserts a spoken paragraph, adds speech to another and deletes a third: every step's range is mapped through the steps after it. Mapping the last step alone fails it. (Jibril's ⛔ on dialogue highlighting) - The scanner's property test draws from mulberry32 rather than a float64 LCG that lost its low bits. - The story says the scan reads quotation marks, not meaning: scare quotes and titles count as speech. - EditorPreviewBlock is a plain record again; the fixture test compares the sample field by field. - ManuscriptSurface.Dialogue says exactly what is read at mount and when a change lands. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>Both reviews are addressed in the latest push.
Jibril
⛔ The mapping across several steps had no guard. A new plugin test walks one transaction that does three things:
«and more»to another paragraph;The test asserts exactly
['“New”', '«and more»']. I probed your mutant: last step only, with identity mapping after it. It fails the new test, and pristine code passes. vitest: 242 passed.💡 The random generator. The property test now draws from mulberry32, which uses integer arithmetic and has a full period, instead of the float64 LCG that lost its low bits.
The general-approach review
the word "home",„Faust“).EditorPreviewBlockis a plain record again. The fixture test compares the sample field by field, with the speech ranges compared as a sequence.ManuscriptSurface.Dialoguecomment now says exactly what happens: it's read at mount only, the plugin is installed then or not at all, and a change lands on the next chapter page, as the ADR accepts for every appearance change.The adapter suite passes, 724 tests.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Back so soon~ And you came prepared! I did promise I'd check, didn't I? ♪ I mutated
changedBlocksback to the naive last-step-only version — last map, identity mapping after it — and your new three-step test struck it down without mercy:spokencame back[]where['“New”', '«and more»']was owed. The branch finally has its guardian. And mulberry32 in place of that float64 LCG — integer arithmetic, full period — the little generator that could~ ♡But fufu… you wouldn't leave a red CI on your own head SHA, would you? ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
CI / testis red onb623949— and I reproduced it.EditorAppearanceSectionTests.Saves_run_in_order_and_a_later_change_is_not_taken_back_by_an_earlier_savefails at line 278:Expected: "22" Actual: "16"— the same failure CI'stestjob shows (run 318, job 1, failing after 2m2s). It passed when I ran your two touched classes alone (27/27 and 51/51) and failed in my full-suite run — which is the signature of the race the old prose was written to guard. The race lives in the test's own last line, the one this PR rewrote:writes.SetResult()releases both gated writes. The effects run under the semaphore, so write #2 chains synchronously inside write #1's continuation:AppSettings.Writesreaches 2 whileEditorAppearanceSaved(#2)is still only queued on the dispatcher.cut.WaitForAssertion(() => Assert.Equal(2, AppSettings.Writes.Count))returns true at exactly that moment — before the component has rendered either answer.Assert.Equal("22", Range(cut, "Size").GetAttribute("value"))then reads whatever the last processed render produced. Because the hand-dispatchedEditorAppearanceSavedearlier in the test already tookPendingto 0,SyncFromStateis entitled to adopt the font-only appearance on that intermediate render — slider 16 — untilSaved(#2)'s render lands a breath later. Whether the bare read catches the interim or the final state is pure scheduling luck, and CI's coverage-collected run makes the unlucky order likely enough to fail for real.Pending > 0) is sound — this is the test observing too early. Your sibling tests in this very file wait on the rendered outcome, not the write log; this one line should too.Fix — one line:
WaitForAssertionpumpsInvokeAsync, which processes the dispatcher's queued renders before reading, so the observation can no longer race the second answer's render. TheWritesassertion above it can stay bare — the log is ordered before any render.Push that, and this track is done — everything else you fixed earned only my delight~
✅ What I liked~
Math.imul— the property test explores the mark soup properly now, not a truncated float64 cycle.EditorPreviewBlockback to a plain record, the fixture compared field by field with the speech as a joined sequence — simpler than hand-written equality and it still compares by value. Clean.ManuscriptSurface.Dialoguesummary now states the mount-only contract exactly as the ADR accepts it — a comment that tells the truth is worth ten that flatter~Automated review by Jibril · 2026-10-03
CI: failed for head SHA (
CI / test, run 318 — the finding above, reproduced locally) · Mutation probes: 1 run, 0 survived (changedBlocks multi-step mutant now killed by the new test; closers probe not re-run — that code unchanged since last review) · Local suite: vitest 242/242, GraphEditor 14/14, typecheck green, Domain 517, UseCases 499, Integration 836, Flow 1103, BlazorAdapter 723/724 — the one failure is the finding above; OpenRouter.Tests could not run locally (sandbox OOM)⛔ Fixed in the latest push, with your one line.
The ordering test's last read is now
cut.WaitForAssertion(() => Assert.Equal("22", Range(cut, "Size").GetAttribute("value"))), with a comment explaining why. The answer dispatched by hand earlier lets the first save's answer adopt the font alone for one render, so the test has to wait for the second answer's render rather than its write.The
Writesassertion stays bare, as you said.I checked the other bare reads in that area: the one mid-test, and the ones in the "a later save fails" test. Both are safe. No render in between can adopt there, because a save is still out (
Pending > 0) or the last answer was a failure.The full BlazorAdapter suite has now passed 6 runs in a row locally, 724/724 each.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Third visit, and what do I find? My one line, placed exactly where I said it belonged, wrapped in a comment that tells the truth about why — "the answer dispatched by hand above lets the first save's answer adopt the font alone for one render in between." A comment that explains the race instead of apologising for it. You even went back and audited the other bare reads in that file, and your reasoning holds: line 270 can't race (a save is still out,
Pending > 0, nothing adopts), and the failure test's reads sit past aWaitForAssertionthat already drained both answers, with a failure as the last word — nothing left to adopt. I checked each one myself, naturally~ ♡Verdict: ✅ Looks good to me~
No blockers. Nothing to fix. The track is finished, and it's clean.
✅ What I liked~
WaitForAssertionpumpsInvokeAsync, which processes the dispatcher's queued renders before reading — the observation can no longer land between write #2 touching the log andEditorAppearanceSaved(#2)getting its render. TheWritesassertion above it stays bare, correctly: the log is ordered before any render.dialogue.tsand its suites) is untouched since my last pass, where the multi-step mapping mutant died on your three-step test and the closers probe was already dead. Nothing left unguarded, nothing left unproven~Merge it. The appearance track ends coloured, incremental, and correct — fufu, what a way to close an ADR~ ♪
Automated review by Jibril · 2026-10-03
CI: green for head SHA
9474f280(build, test, editor-browsers — the previously-redCI / testpasses on this head) · Mutation probes: 0 run, 0 survived — skipped, incremental diff touches only the C# test file;dialogue.tsand its tests unchanged since last review (multi-step mutant killed there, closers mutant killed in the first review) · Local suite: skipped (CI green)