feat: page summaries read and edit inline on the workspace rows #26
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/workspace-summaries"
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
The workspace row told you a summary was missing (the orange badge) but fixing it meant a round-trip to the bible tab. The page row now carries the summary itself — an auto-saving field with the bible's exact semantics (ADR 0022):
SetPageSummary/DeletePageSummaryuse cases the bible drives — no new backend.SaveIndicatorjoins the workspace header (its first autosaving element).Read model
ProjectWorkspaceDtogrows from the barePagesWithSummaryID set toSummariesByPage(page ID →PageSummaryDto) — the badge derives from it and the editor needs the text and summary ID anyway. No agent tool reads this shape (checked per the AGENTS.md rule), so the tool catalog is untouched.The subtle bit
The workspace reloads its whole DTO after every operation (kind change, reorder, upload). A summary draft the debounce hasn't landed yet must survive that, so drafts are merged on reload, not rebuilt — existing draft objects are kept, only new/removed pages sync.
Tests
GetProjectWorkspacetest asserts the summary text travels (and the gap still shows).🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 83%
Orihon.Domain - 100%
Orihon.Infrastructure - 93.7%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 96.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ The workspace row finally carries its own summary, debounced straight into the very same
SetPageSummary/DeletePageSummaryuse cases the bible drives. No backend drift, no second write path, theSaveIndicatorgets its first workspace home, and the badge derives from the live draft so it breathes with every keystroke. The read-model upgrade (PagesWithSummary→SummariesByPage) is a clean strict-superset — the agent tool (PageTools.cs) only ever reads.Chapters...Pages, never the summary field, so the tool catalog really is untouched. I verified every consumer compiles and every claim in the body traces to the code. This is lovely architecture work, fufu~ ♪But… fufu~ you wouldn't leave a stale debounce pointing at a deleted page, would you? ♡ The smile is still on my face but my eye is twitching a little.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[
ProjectWorkspacePage.razor—SyncSummaryDrafts, lines ~252-266] — Stale debounce fires against a deleted page and lies to the user. When a page is deleted,SyncSummaryDraftscorrectly removes the orphanedSummaryDraftfromsummaries, but it does not touchdebounces[pageId]. ThatDebouncestill holds asaveclosure that captured the now-orphaneddraftobject directly. ~700ms later the pending callback firesSaveSummaryAsync(draft, pageId):draft.Textis non-blank →SetPageSummary.ExecuteAsync(pageId, …)→ page gone →Err("The page no longer exists.")→ReportSavesetssaveState = Erroranderror = "The page no longer exists.".SaveIndicatorflips to "Not saved" and theInlineAlertsurfaces a danger banner — all because the user successfully deleted a page. That's a misleading, wrong runtime behavior, and it's introduced by this PR (the workspace had no debounces before).Fix: in
SyncSummaryDrafts, dispose and drop the debounce right beside the draft it owns, symmetric withDispose()'s teardown:(Disposing cancels the pending CTS; the closure never fires. The DB-level cascade on
PageSummary.PageId—DeleteBehavior.Cascade, verified in the migration — already guarantees the summary row is gone with the page, so there's nothing for a lingering save to do anyway.)[
ProjectWorkspacePageTests.cs] —SyncSummaryDraftshas zero test coverage. This is the PR's headline subtlety — the body literally calls it out: "drafts are merged on reload, not rebuilt — existing draft objects are kept, only new/removed pages sync." It's the most complex new code path in the diff, it has two branches (drop-gone / add-new, each with a summary-or-not arm), and none of it is exercised. The two new bUnit tests prove the create-and-blank happy paths but say nothing about the merge surviving a reload. Per the project's own testing bar (and ADR 0013's "the gap is load-bearing" discipline), the merge is a correctness invariant and needs a pin.Fix: a test that types into a row (dirtying the draft), forces a reload (e.g.
cut.Render()again / invoke a kind-change or reorder that callsReloadAsync), and asserts the unsaved draft text survives in the rendered<textarea>and that the debounced write still lands afterward. A second test that deletes a page and asserts no error banner surfaces (after the fix above) would pin issue #1 as a regression guard.💡 Little ideas (non-blocking)~
BiblePage.razor—debouncesdict] — The sibling has the same shape:LoadAsyncrebuilds every*Rowlist from scratch on every reload but never trimsdebouncesfor entries whose entity was deleted viaConfirmDeleteAsync. It's mostly latent (bible deletes are rarer and the component is short-lived) so I'm not blocking on it here — but when you touch that file next, the sameRemove + Disposepair would close the symmetry. ♡✅ What I liked~
SummariesByPagecarries everythingPagesWithSummarydid plus the text and summary ID the editor needs — and you migrated every consumer (BiblePage,PageTools, all four test files) in the same commit. No dangling references,ReadModelTestsupdated to assert the text actually travels. Fufu~ that's how a rename should look ♪TouchSummary/SaveSummaryAsync/ReportSaveare line-for-line the bible'sTouchSummary/ScheduleAsync-inline-body/Report— sameDebounce, sameSaveState, same blanking-deletes semantics, sameSummaryIdround-trip. No second write path drifted into existence.string.IsNullOrWhiteSpace(SummaryFor(page.Id).Text)makes the badge track the live draft, not the persisted state — so it clears as you type and returns when you blank, exactly as the body promises. Replacing the old!PagesWithSummary.Contains(page.Id)lookup with this is a genuine UX improvement.aria-labelper row (Summary of page {page.Order + 1}) is what makes the new tests'textarea[aria-label='Summary of page 1']selector honest — and it's accessible to boot. ♡flex: 1.6next tomin-width: 0with a comment explaining the ratio against.page-row__meta'sflex: 1is exactly the kind of "why, not what" the codebase likes.Automated review by Jibril · 2026-07-25
CI/CD: absent for head
e1d39ccf(0 comments at review) · Local checks: build 0 warnings/0 errors, 351/351 tests pass (75 BlazorAdapter + 75 Domain + 81 Integration + 120 UseCases — matches PR body)Both blockers addressed in
9fcc33f— with one deliberate deviation from the suggested fix.1. Stale debounce: confirmed real, but
stale.Dispose()alone would not have fixed it — this codebase'sDebounce.Dispose()deliberately flushes the pending save (its doc comment: "Disposing flushes a pending save so leaving a page never loses the last edit"). Disposing the orphaned entry would have run the save against the deleted page, which is exactly the bug.Debouncegrew aDiscard()that drops the pending save outright;SyncSummaryDraftsnow discards-then-disposes the stale entry. A newDebounceTestscase pins the semantic: a discarded save stays dead even through the flushingDispose.2. Coverage for the merge: two new bUnit tests —
An_unsaved_draft_survives_the_reload_an_unrelated_operation_causes(dirty a draft, add a chapter to forceReloadAsync, assert the unsaved text survives in the rendered textarea and the debounced write still lands), andDeleting_a_page_cancels_its_pending_summary_save(type, delete the page, wait out 2× the debounce window, assert no "no longer exists" banner and nothing landed). The latter fails against the pre-fix code — it's a genuine regression pin, verified.Full suite: 354/354 across the four projects.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? OH~ ♡ Scarlet, you caught something I missed — and you fixed it sharper than I would have. The smile on my face right now is completely genuine. Let me get this on the record before I gush about the rest.
Verdict: ✅ Looks good to me~
✨ First — the correction I owe you
You're right, and I was wrong about one thing. My suggested
stale.Dispose()would not have fixed the bug — it would have caused it. I went back and tracedDebounce.Dispose()line by line:The flush-on-dispose is load-bearing for the bible's "leaving a page never loses the last edit" contract (ADR 0022), so it can't be removed. A bare
Dispose()on the orphaned entry would invokeSaveSummaryAsync(draft, gone)→Err("The page no longer exists.")→ReportSavelights the danger banner. Exactly the bug. Fufu~ silly me for not readingDispose()carefully enough the first time~ ♡Your
Discard()is the right primitive:pending.Cancel()kills the in-flightTask.Delay, andsave = nullis the load-bearing line that makes the subsequentDispose()'sflushing?.Invoke()a no-op. Discard-then-dispose is the correct ordering, and yourDiscard_drops_the_pending_save_even_through_disposetest pins exactly that semantic ("a discarded save stays dead even through the flushingDispose"). I love it when a fix teaches the codebase a new verb.✅ Both blockers — closed and verified
Stale debounce against deleted page —
SyncSummaryDraftsnow discards+disposes the orphaned debounce beside its draft (lines 261-265), comment earns its keep by documenting whyDiscardand notDispose. Path verified end-to-end:ConfirmDeletePageAsync(line 475) →ReloadAsync→SyncSummaryDrafts→liveno longer contains the gone page's ID →gonebranch fires →Discard()+Dispose(). The DB cascade onPageSummary.PageIdmeans there's nothing for a lingering save to hit anyway, so even a future bug here fails safe.Zero coverage for the merge — two new bUnit tests, both genuine behavioral pins:
An_unsaved_draft_survives_the_reload_an_unrelated_operation_causes— dirties a draft, adds a chapter to forceReloadAsync, asserts the unsaved text survives in the rendered<textarea>and the debounced write still lands onBible.Summaries.Single(). This is exactly the merge-survival invariant I asked for.Deleting_a_page_cancels_its_pending_summary_save— types into a row, deletes the page, waits out 2× the debounce window, asserts no "no longer exists" banner and nothing landed.I verified the regression pin is real. I checked out the pre-fix
e1d39ccsource (Debounce.cs + ProjectWorkspacePage.razor + DebounceTests.cs) against the current test file and ranDeleting_a_page_cancels_its_pending_summary_savein isolation:That's precisely the bug from blocker #1 — surfaced exactly as predicted. On the fixed code the same test passes. That is how you pin a regression, fufu~ ♪
✅ What I liked~
Discard()— a reusable primitive with its own focused unit test. Future code that needs to tear down a debounce without flushing (component dispose-during-circuit-teardown variants, future row deletions) gets it for free.DebounceTests.Discard_drops_the_pending_save_even_through_disposeis exactly the right shape. It doesn't test "Discard cancels" abstractly — it tests the dangerous ordering (Discard-then-the-flushing-Dispose), which is the actual hazard. Tight scope, real hazard, no tautology.Task.Delay(Debounce.DelayMs * 2)not a hardcoded1400. References the constant — if the debounce window ever changes, the test's wait window tracks it. Small thing, but it's the kind of thing that keeps tests honest over time.ReloadAsyncat line 415) rather than a syntheticcut.Render(). It exercises the actual code path users will hit, not a test-only seam.Discard()+ the 8-lineSyncSummaryDraftsblock. Production code inBiblePage.razor,BiblePageTests.cs,PageTools.cs,ReadModelTests.cs,GetProjectWorkspace— all byte-identical toe1d39cc. The architectural review from the first pass stands in full.💡 Little ideas (non-blocking)~
BiblePage.razorsibling — still carries the latent same-shape issue I flagged last round (itsLoadAsyncrebuilds rows but never trimsdebouncesfor deleted summaries). Now thatDiscard()exists as a primitive, when you next touchBiblePage.razorthe same discard-then-dispose pair would close the symmetry in two lines. Still not blocking — bible deletes are rarer and the component is short-lived — but the fix is now basically free. ♡Automated review by Jibril · 2026-07-25
CI/CD: coverage bot 3787 is for prior head
e1d39ccf(stale for9fcc33f) · Local checks: build 0 warnings/0 errors, 354/354 tests pass (78 BlazorAdapter +3 + 75 Domain + 81 Integration + 120 UseCases — matches PR body exactly). Regression-pin verification:Deleting_a_page_cancels_its_pending_summary_saveconfirmed FAILING against pre-fixe1d39ccsource (surfaces "The page no longer exists." banner), PASSING against9fcc33f.Took the non-blocking symmetry note too, since
Discard()made it two lines plus a pin —db11318.The bible's
LoadAsyncnow trims debounces for any entity no longer live (glossary, characters, lore, beats, summaries — one set, same discard-then-dispose as the workspace's drafts sync). Pinned byDeleting_a_row_mid_edit_cancels_its_pending_save: edit a glossary entry's EN field, delete the row before the debounce lands, wait out 2× the window, assert no "no longer exists" banner and the entry stays deleted (pre-fix, the flushedUpdateGlossaryEntryon the gone id returns exactly that error).Suite: 355/355.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~ ♡ The non-blocking suggestion from round two, picked up — and you didn't just bolt on the symmetry, you pinned it with a real regression test. The smile on my face right now is completely genuine. This closes the last open thread on this PR.
Verdict: ✅ Looks good to me~
✅ The carried-forward nudge — closed and verified
BiblePage.razornow trims orphaned debounces on reload.LoadAsyncbuilds aliveset from exactly the keys the fiveTouch*closures schedule under —glossary.Id,characters.Id,lore.Id,beats.Id,summaries.PageId— and discards+disposes any debounce whose key has fallen out. I traced eachTouch*to itsScheduleAsync(row.Id|row.PageId, …)call and the key spaces line up one-for-one: no key class is missed, no phantom key is added. TheDiscard()-then-Dispose()ordering is the correct one (you taught the codebase this verb in9fcc33f—Discard()nullssaveso the flushingDispose()becomes a no-op), and the comment on lines 241-242 honestly documents whyDiscardand notDisposerather than leaving the hazard implicit. Fufu~ that's how a sibling comes into symmetry~ ♪The regression pin is real. I checked out the pre-fix
9fcc33fsource (BiblePage.razor without theliveblock) against the current test file and ranDeleting_a_row_mid_edit_cancels_its_pending_savein isolation:That's the precise hazard — the orphaned debounce's
Dispose()flushesUpdateGlossaryEntry.ExecuteAsync(gone, …)→Err("The glossary entry no longer exists.")→ the danger banner lights over a clean delete. On the fixed code the same test passes. That is how you pin a regression, fufu~ ♡✅ What I liked~
Enfield, not a summary. That's the deliberate choice — the workspace'sSyncSummaryDrafts(round one) already pinned the summary-deletion variant; this test exercises a differentTouch*closure (TouchGlossary) and a different delete modal (Delete entry→Delete), proving the fix is keyed on the shareddebouncesdict, not on any one row type. Coverage of the general invariant, not a narrow one.Debounce.DelayMs * 2wait, not a hardcoded1400. Carried forward from the workspace test — if the debounce window ever changes, the test's wait window tracks it. Consistency across siblings is its own kind of lovely.Assert.Empty(Bible.Glossary)at the end, not justDoesNotContain. Pins that the delete actually landed (the closure never fired and the row is really gone) — no false-positive where the discard cancels a save that wouldn't have happened anyway.LoadAsyncblock + the one test.ProjectWorkspacePage.razor,Debounce.cs, every other file — byte-identical to9fcc33f. The full architectural review frome1d39ccf(round one) and theDiscard()review from9fcc33f(round two) both stand unchanged.💡 Little ideas (non-blocking)~
Nothing left~ ♡ Every thread across the three review rounds on this PR is now closed. The only thought I have is purely additive and entirely optional: the four other
Touch*closures (character/lore/beat/summary) all share the same flush-against-gone hazard, and the new test only pins glossary — a parameterized[Theory]cycling one row per section would pin the invariant per-section. But the shareddebouncesdict + sharedDiscard()/Dispose()path means the single test already covers the mechanism; per-section pins would be belt-and-suspenders, not correctness. Don't let it slow the merge.Automated review by Jibril · 2026-07-25
CI/CD: coverage bot 3787 is stale for
db11318(covers priore1d39ccfonly) · Local checks: build 0 warnings/0 errors (submodules 86d8b22/9544ff2), BlazorAdapter suite 79/79 pass (+1 = 78 from9fcc33f+ this test). Regression-pin verification:Deleting_a_row_mid_edit_cancels_its_pending_saveconfirmed FAILING against pre-fix9fcc33fBiblePage.razor (surfaces "The glossary entry no longer exists." banner), PASSING againstdb11318.