Round 11 code review: sync/state-sync fixes + coverage#75
Merged
Conversation
Full-codebase review after 78 commits (1.2.15 → 1.3.9): delta-sync overhaul, Today's-5 pin LWW + tombstones, manual Today's-5 rewrite, shared AddTaskFlow, create-task-from-search picker unification. 6 Important (all hand-verified): - I-48 Today's-5 LWW pushes push-time now() vs persisted-at stamp (LWW inversion) - I-49 Task hard-deletes never reconciled on pull (deleted tasks resurrect) - I-50 Bulk sync ops run outside the _syncing mutex (race with push/pull) - I-51 Uncompleting an external "Done today" task doesn't revert DB worked-on flag - I-52 getEffectiveDeadlines N+1 recursive CTE on the refresh hot path - I-53 Starred grandchildren miss the priority/blocked styling (DRY-rule violation) Plus M-39..M-52 minor + R-10..R-12 refactors. I-38/R-9 closed (obsolete). flutter analyze clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Durability, correctness, and hot-path fixes across the sync layer, Today's 5, starred view, and pickers so multi-device state stays consistent and mutations refresh reliably. Important: - I-49: task deletions now propagate via Firestore-only tombstones + delta/full-pull reconciliation (were hard-deleted, so deleted tasks resurrected on other devices). - I-48: push the persisted edit-time stamp as Today's-5 updated_at so cross-device LWW compares like clocks (was push-time now()). - I-50: bulk sync ops run under the _syncing guard (_runExclusive) so debounced push / periodic pull can't interleave with a half-wiped DB. - I-51: uncompleting an externally "Done today" task now reverts the DB worked-on flag (detect from task.isWorkedOnToday, not just session set). - I-52: batch getEffectiveDeadlines into one recursive CTE (was N+1 on every refresh). - I-53: route starred grandchild styling through childTextStyle + cover grandchildren in the blocked-info fetch (DRY rule). Minor: M-40 (persist pull-cycle counter only on success), M-42 (log tombstone-purge failures), M-43 (clear parallel session sets on remove), M-44 (refreshSnapshots reentrancy guard), M-45 (addRelationship ConflictAlgorithm.ignore), M-46 (refresh on empty-stack branch), M-47 (cache triage search), M-48 (Create uses live field text), M-49 (matched removeListener), M-50 (drop dead highlight field), M-51 (await starred unstar/undo), M-52 (CLAUDE.md v23). M-39/M-41 documented. Refactor: R-10 atomic insertTaskWithParents. R-11/R-12 deferred (see docs/CODE_REVIEW.md). Adds regression tests for I-49, I-51, I-52, I-53, M-48. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Independent verify pass over fix commit d499ee5: - All 6 Important (I-48..I-53) fixed at root cause, verified by hand - I-49 Firestore-only tombstone deviation checked end-to-end and sound (delta + full-pull reconciliation, pending-add guards, cleanupTombstones) - All 14 Minor + R-10 confirmed; M-39 no-op by design, M-44 partial accepted - R-11/R-12 deferred (reasonable); M-32/M-34 genuinely still open (low impact) - flutter analyze clean; flutter test green (+1457, 1 skipped, 0 failures) Merge verdict: ready to merge into main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fills the auto-test gaps left after the Round 11 fix commit so each fixed behavior is guarded: R-10 insertTaskWithParents atomicity (4), M-45 addRelationship conflict-ignore (2), M-46 empty-nav-stack refresh fallback (3), I-48 push edit-time stamp, I-50 bulk-op mutex deferral, and the I-53 blocked- grandchild card half. Full suite green (+1469). Also corrects two UI_VIEWS.md gaps surfaced during manual testing: inbox row tap opens "File under…" (long-press drills in), and the pre-existing DAG "Also show under…" root-vanish limitation (root = parentless). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ge nit Non-blocking follow-ups from the approving review of PR #75: - _runExclusive now debugLogs when the 5s bounded wait times out and it proceeds while a push/pull still holds _syncing (the accepted TOCTOU is no longer silent). - Pin the load-bearing I-49 invariant with a comment at the full-pull reconcile site (partial/failed remote fetch must abort before the delete loop) plus two sync tests: a control proving the reconcile deletes on a complete empty remote, and the invariant proving a failed remote fetch deletes nothing. - triage_dialog _onSearchChanged keeps _searchFilter mutations inside setState on every path (was assigned once up front). Full suite green (+1471). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full Round 11 code-review cycle on the
code-reviewbranch — findings → fixes → verification → test-suite. All items hand-verified;flutter analyzeclean; full suite green (+1469 tests).Fixes (6 Important, 14 Minor, R-10)
Important
updated_at(was push-timenow()), fixing cross-device last-write-wins inversion.initialMigration/replaceLocalWithCloud/replaceCloudWithLocal/mergeBoth) run under the_syncingmutex, so a debounced push / periodic pull can no longer interleave with them.isWorkedOnToday), so it no longer bounces back to done on reload.getEffectiveDeadlinesbatched into one recursive CTE per chunk (was N+1 per deadline-less task on every refresh).childTextStyleand includes grandchildren in the blocked-info fetch, matching the expanded dialog (DRY rule).Minor / Refactor — M-40…M-52 (counter-on-success, tombstone-cleanup logging, session-set cleanup, refreshSnapshots coalescing guard, addRelationship conflict-ignore, empty-nav-stack refresh fallback, triage search caching, live-text Create, listener cleanup, dead-field removal, unstar await). R-10 — atomic
insertTaskWithParents. M-39 no-op by design; R-11/R-12 deferred as focused follow-ups.Verification & tests
docs/CODE_REVIEW.md→ Round 11 Fix Verification).docs/UI_VIEWS.mdcorrections from manual testing (inbox row tap = "File under…"; "Also show under…" root-vanish limitation).Not covered here
🤖 Generated with Claude Code