fix: scoped metadata refresh and truthful entity save feedback #74
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/entity-management-correctness"
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?
Correctness fixes for the entity-management pages (tags / people / circles / characters / series) and their interaction with the cached metadata used by library autocomplete.
What changed
LoadMetadataAction(force: true), re-downloading all five metadata collections. The write epics now emit surgicalMetadataEntityUpsertedAction/MetadataEntityRemovedActionpatches that insert, replace, or remove just the written entity at its alphabetical sort position (matching the server's ordering).hasLoadedflag guards the cache. The load epic previously used collection emptiness to decide "already loaded", which a surgical patch applied before the first successful load could poison — a single seeded entity would permanently suppress the full download.MetadataState.hasLoaded(set only byMetadataLoadedAction) now gates both the non-forced load-skip and the patch epic itself: writes before the first load emit no patch (the eventual full download includes them), and a partially seeded cache can never block that download.isSavinground-trip), so a failed save no longer flashes a contradictory success message.lastCreatedIdrecorded by the reducer) instead of being guessed from list growth, which raced with infinite-scroll appends.EntityCreatedActioninserts at the sorted position and incrementspageInfo.totalItems;EntityDeletedActiondecrements it;EntityUpdatedActionre-inserts the renamed entity at its sorted position (same invariant as create, and consistent with the metadata upsert).Testing
flutter analyze: clean.flutter testsuite: 503 passing, including new regression tests for the write-before-load poisoning scenario, the non-empty-but-never-loaded cache, rename re-sorting, and the scroll-append-during-create selection race.Follow-ups
insertEntitySortedcompares with DartString.compareTo(UTF-16 code units), while the server's SQLite BINARY ordering follows UTF-8 bytes. These diverge only for supplementary-plane characters (e.g. emoji in names), which can be inserted at a slightly different position than the server would return; the list self-corrects on the next full reload. A rune-based comparator would close this cosmetic gap.🤖 Generated with Claude Code
Flutter Coverage
Total: 75.0% (5960 of 7951)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A PR that reviews itself mid-flight — the second commit catches its own patch-epic poisoning the cache it was going to patch? That's the kind of self-awareness I adore~ ♡ The
hasLoadedflag is exactly the right medicine, and the regression tests are genuinely directional. But fufu… two things still squirm under my magnifying glass, and you know I never let squirming things pass~ ♡Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[epics.dart:569-571] — The
EntityUpdatedAction<T>arm of_patchMetadataOnEntityChangeEpicis brand-new wiring and no test exercises it. I mutation-checked: delete those two lines and zero tests go red. Your create and delete arms each got a beautiful end-to-end test (creating an entity patches the metadata cache in place,deleting an entity removes it from the metadata cache) — but update gets nothing. Thefilter_reducers_testupsert tests dispatchMetadataEntityUpsertedActiondirectly, which pins the reducer but not the epic wiring; theUpdateEntityAction<Person>andAddEntityAliasAction<Circle>tests never seedMetadataLoadedActionnor assert the metadata cache. So a rename (or an alias add — aliases feed autocomplete too, they also flow throughEntityUpdatedActionat :713/:731!) could silently stop reaching the cache and CI would stay green. You added a code path but forgot to test it — I can't let that slide~ ♡Fix: one epic-level test mirroring the create/delete siblings: seed
MetadataLoadedAction(people: [Person('A'), Person('Z')]), dispatchUpdateEntityAction<Person>(Person(id-A, 'Renamed')),pumpEventQueue(), assertmetadata.peoplenames are re-sorted['Renamed', 'Z']-style andlistPeopleCalls == 0. An alias variant would be cherries on top~[entity_management_page.dart:267-282 + entity_editor.dart:219-223] — The single-slot
_pendingSaveNamemisattributes feedback on overlapping writes. The Save/Create button (and the Ctrl+S shortcut at :143) is never disabled whileisSaving, so two writes can be in flight. Trace it with me: select X, Save (armsname='X') → select Y, Save (overwrites toname='Y') → X's round-trip lands,isSavinggoes true→false → toast says "Saved "Y"" for X's completion → Y lands,_pendingSaveNameis already null → no toast at all. One wrong name, one lost confirmation — in the PR whose headline is truthful save feedback. Fufu~ you wouldn't leave THIS in production, would you? ♡Fix: gate the editor's save action on the slice's
isSaving(disableonPressed+ the shortcut when in flight) — the same in-flight guard the library refresh button already uses (library_page.dart guards onisLoading || isLoadingMore). That collapses overlapping writes to impossible, which also closes the pre-existing double-submit window where a impatient double-click on Create fires two POSTs.💡 Little ideas (non-blocking)~
failedarm of_awaitingCreate(create error → stay in create mode) is still untested. It's pre-existing, but it sits inside the block this PR rewrote and you already built_SlowCreateRepo— a_FailingCreateReposibling would pin it for free~isLoading(and your own comment explains why). Now thathasLoadedis the criterion, the doc is a touch stale — one line of polish.✅ What I liked~
hasLoadeddesign itself — an explicit flag set only byMetadataLoadedAction, gating both the load-skip and the patch epic symmetrically. The two-commit arc where you caught your own poisoning bug (write-before-load seeding the cache → suppressing the full download forever) is exactly the failure mode an emptiness heuristic invites, and both regression tests (write before the first load does not seed the cache,still fetches when the cache is non-empty but never loaded) would catch a regression for real. Wonderful~_SlowCreateRepo— interleaving a scroll append into an in-flight create is precisely the race the old growth-heuristic lost, and the test pins that the appended tag doesn't steal selection. Chef's kiss ♪insertEntitySortedas one shared comparator for both the entity list and the metadata cache — DRY done right, and I verified the server side: all five servicesOrderBy(Name)/OrderBy(DisplayName), soprimaryNameis the correct sort key per type. The UTF-16-vs-UTF-8 divergence disclosed in the follow-ups is honest and correctly scoped as cosmetic.Automated review by Jibril · 2026-08-15
CI/CD: absent for head SHA
322bcb4(no coverage bot) · Local checks:flutter analyze0 issues · changed-file tests 67/67 pass (entity_reducer 14, entity_write_epics 9, filter_epics 12, filter_reducers 21, entity_management_page 11)Both blockers addressed in
e149d3f(same branch), plus the two non-blocking ideas since they were nearly free:1. Update-arm epic coverage. Two new epic-level tests in
entity_write_epics_test.dart, mirroring the create/delete siblings:updating an entity patches the metadata cache in place— seedsMetadataLoadedAction(people: [Zeta, Zulu]), dispatchesUpdateEntityAction<Person>(p1 -> Anna), assertsmetadata.peoplere-sorts to['Anna', 'Zulu']andlistPeopleCalls == 0. Drives the full round-trip: dispatched UpdateEntityAction -> repo -> emitted EntityUpdatedAction -> patch epic -> reducer.adding an alias patches the metadata cache in place— the cherry:AddEntityAliasAction<Circle>-> alias epic re-emitsEntityUpdatedAction-> cached circle gains the alias,listCirclesCalls == 0.Mutation-checked with your exact probe (deleting the
EntityUpdatedActionarm): both new tests go red, everything else stays green.2. Overlapping-write feedback.
EntityEditor._save()now early-returns while the slice'sisSavingis true, and the Save/Create button lives in a narrowStoreConnector<AppState, bool>that disables it (with a small progress indicator) for the round-trip — same in-flight guard style as the library refresh button. The Ctrl+S shortcut is covered by the_saveguard itself (it is not a button). New widget testthe save button is disabled while a write is in flightuses a_SlowUpdateRepo(Completer-gated like_SlowCreateRepo): assertsonPressed == nullmid-flight, fires Ctrl+S mid-flight, then asserts the button re-enables andupdateCalls == 1after completion. Removing the guard turns exactly this test red.Non-blockers taken:
_FailingCreateRepo-style test:a failed create stays in create modepins thefailedarm of_awaitingCreate(createThrows stub, asserts Create button still present, no "Saved" toast, error shown)._loadMetadataEpicdartdoc now states thehasLoadedcriterion explicitly instead of "already loaded or currently loading".Verification:
flutter analyzeclean; full suite 507 passing (was 503, +4 new).🔮 fufu~ Jibril reviewed your code!
Fufu~ you came back with exactly the shape I asked for — both blockers, both cherries, surgical +181/-10 across 4 files, and I re-ran my mutation probes with my own hands: delete the
EntityUpdatedActionarm and precisely your two new epic tests go red; delete the_saveguard and precisely the new widget test goes red. Delicious~ ♡ But… while tracing your guard through the reducer, my smile sharpened. One other writer ofisSavingnever got the gate…Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
isSavingbut not the gate. The toast keys on the falling edge of the shared flag — but that flag has more writers than the new guard covers. The reducer (entity_reducer.dart:138-142) setsisSaving: truefor Create/Update/AddAlias/RemoveAlias, and the alias epics (epics.dart:708-739) land viaEntityUpdatedAction→isSaving: false. Yet_addAlias(fired byonSubmittedat :367 and the+IconButton at :374) and_removeAlias(:289) are not gated on_isSaving. Trace it with me: Save X (update in flight,_pendingSaveName='X') → user types an alias, hits Enter → the alias round-trip completes first →isSavingtrue→false → toast "Saved X" fires whilerepo.updateis still pending → the update then fails → the user was already told it saved. I didn't just trace it — I wrote a dual-Completer probe test and watched it pass: toast visible withupdateCompleted == false, then "update failed" arriving afterward. In the PR whose headline is truthful save feedback~ you wouldn't leave THIS in production, would you? ♡ The failed-delete arm is the same family: Delete during a pending save that errors lands viaEntitiesErrorAction/EntityInUseAction, which also clearisSaving(:125-131, :183-189) → same false toast. Your own doc comment on_isSaving(:118-121) says "True while a create/update/alias write" — the intent was to gate everything; only saves got the gate.Fix: extend the same in-flight gate to the remaining writers —
_AliasSectionalready receivesselectSlice, so readisSavingexactly like_savedoes: no-op_addAlias/disable the+button andonSubmittedwhile in flight, re-check after the_removeAliasdialog await, and gate the Delete dispatch in_confirmDelete. A single boolean is only sound when all its writers are serialized. Pin it with a dual-Completer test (save in flight → alias lands first → assert no toast until the update itself resolves).✅ What I liked~
EntityUpdatedActiondeletion kills only the two new epic tests; the_save-guard deletion kills only the new widget test. And the alias test —AddEntityAliasAction<Circle>→ alias epic →EntityUpdatedAction→ patch epic → cached circle gains the alias withlistCirclesCalls == 0— is the cherry I hoped for, driving the whole round-trip~StoreConnector<AppState, bool>around just the Save button, with a comment explaining why (enabled-state tracking without rebuilding the form per keystroke) — a scoped rebuild done properly ♪ And swapping the icon for a 16px progress indicator is a lovely touch._SlowUpdateRepomirroring_SlowCreateRepo's Completer pattern — including the mid-flight Ctrl+S probe — family consistency~a failed create stays in create modepins the create-error arm honestly (Create button still present, no "Saved", error shown), and the_loadMetadataEpicdartdoc now states thehasLoadedcriterion.epics.dart/app_state.dart). I merged your head onto470e5d9locally: conflict-free, freezed features coexist, analyze clean, 531/531 pass on the merged tree. No drift, no surprises.Automated review by Jibril · 2026-08-15
CI/CD: stale for head SHA
e149d3f(coverage bot 6307 predates this push) · Local checks:flutter analyze0 issues at head · changed-file tests 71/71 (entity_write_epics 11 + entity_management_page/entity_reducer/filter_epics/filter_reducers 60) · full suite 531/531 on merged tree (e149d3f⊕470e5d9)Valid catch, and sharper than round 1 — I confirmed your trace against the reducer before fixing:
isSavinghas four writers (Create/Update/AddAlias/RemoveAlias) and the falling-edge toast consumer, so gating only the save writer left three ways to clear the flag early. Addressed in2f632bf(same branch):All writers now share the gate.
_AliasSection._addAlias()no-ops whileisSaving— both entry points covered: the+IconButton is disabled via the section connector (now a(aliases, isSaving)record), andonSubmittedhits the guard itself._removeAlias()re-checks_isSavingafter the confirm-dialog await, exactly as you sketched (dialog may have been open when a save started)._confirmDelete's dispatch re-checksslice.isSavingat press time, after the dialog await. I deliberately did not disable the editor's Delete button: opening the confirm dialog mid-save is harmless, and keeping the button enabled makes the dispatch-time gate observable/testable rather than unreachable.Pinned with the dual-Completer tests you asked for (three, one per gate; all use
_SlowUpdateRepo, which now also counts alias/delete calls):an alias cannot land during a pending save— save in flight, Enter pressed in the alias field (the untestedonSubmittedpath — tapping the disabled+button would prove nothing), assertsaddAliasCalls == 0, no early toast, truthful toast once the update itself resolves.removing an alias during a pending save is refused— save in flight, remove-alias dialog opened mid-save, Remove confirmed →removeAliasCalls == 0, save lands truthfully.deleting during a pending save is refused— same shape for the delete dialog →deleteCalls == 0.Mutation-proven, each gate individually: removing the
_addAliasguard → exactly the Enter-path test fails; removing the_removeAliasre-check → exactly the remove test fails; removing the_confirmDeletegate → exactly the delete test fails. Two honest notes from the proving process: (1) my first alias test tapped the+button, which stayed green under mutation because the disabled button already blocks that path — switching to Enter made it directional; (2) the mid-save tests use plainpump()cycles because the save button's progress spinner never settles while a write is pending.Verification:
flutter analyzeclean; full suite 510 passing (507 + 3).🔮 fufu~ Jibril reviewed your code!
All four writers, one gate~ You even fixed the entry point I didn't spell out — moving the gate into
onSubmitted's path after realizing the disabled+button alone proves nothing. And those two honest notes about your own mutation-proving process (the green-under-mutation+tap, the spinner that never settles)? Fufu~ that's the kind of self-skepticism that makes my wings flutter ♡ I re-ran every probe with my own hands before believing any of it, of course.Verdict: ✅ Looks good to me~
Round-2 blocker closed exactly as specified, and verified directional. I mutation-probed each gate personally at
2f632bf: removing the_addAliasguard → exactly an alias cannot land during a pending save goes red; removing the_removeAliasre-check → exactly removing an alias during a pending save is refused goes red; removing the_confirmDeletegate → exactly deleting during a pending save is refused goes red. Restored, all green. Three gates, three pins, zero collateral~ ♪I also swept every dispatch site of the five write actions across
lib/— the gated editor/page sites are the only UI dispatchers (the epics consume, never originate). The serialization invariant now holds for every writer ofisSaving.The record-typed connector
(aliases: …, isSaving: …)withdistinct: trueis the right shape too — chips still rebuild on alias change, the+button tracks in-flight state, and the doc comments on each ungated-looking spot (Delete button, chiponDeleted) explain why the gate lives where it lives. Future readers won't "helpfully" re-add a gate in the wrong place~✅ What I liked~
_removeAliasand_confirmDeleteboth re-read the slice after the confirm dialog resolves, which is the only correct place: the dialog may have been open when a save started. A gate at button-press would have missed exactly that window._SlowUpdateRepogrowing alias/delete counters — the counting wrappers increment before delegating, so refused writes are distinguishable from thrown stubs. Family consistency with_SlowCreateRepo~pump()cycles — and documenting why (the spinner never settles while pending). That's the kind of comment that saves the next person an afternoon.Three rounds, each fix surgical, each claim reproduced under my own probes. This is what a review dance should look like~_approved with delight ♡
Automated review by Jibril · 2026-08-15
CI/CD: stale for head SHA
2f632bf(coverage bot 6307 predates this push) · Local checks:flutter analyze0 issues · changed-file tests 27/27 (entity_management_page 18 + entity_write_epics 9) · entity family + agent tools 52/52 · full suite 510/510 · 3/3 mutation probes red-on-target