Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d72788fda8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Future<void> flush() { | ||
| _debounce?.cancel(); | ||
| return _flush ??= _sendPending().whenComplete(() => _flush = null); |
There was a problem hiding this comment.
Queue edits made during an active flush
When a layout save remains in flight for longer than the 500 ms debounce and the user makes a newer edit, that edit's timer reaches flush() while _flush is non-null, so this expression only returns the old future. If the old request then fails, _sendPending exits and whenComplete merely nulls _flush; the newer values remain pending with no timer and are not sent unless another edit or full-list read occurs, so a final reorder or indent can be lost when the process exits. Record a follow-up flush for work queued during the active request.
Useful? React with 👍 / 👎.
| if (_sortOrders[id] == value) _sortOrders.remove(id); | ||
| }); | ||
| indentLevels.forEach((id, value) { | ||
| if (_indentLevels[id] == value) _indentLevels.remove(id); |
There was a problem hiding this comment.
Fence reads that predate the layout acknowledgement
If a list request takes its server snapshot before the PATCH but completes after the PATCH succeeds, these removals clear the only overlay before that response is applied. Layout edits never advance _taskWriteRevision, so fetchActionItems still accepts the older response and _applyFetchedActionItems replaces the local row with stale order/indent values, visibly reverting an acknowledged edit until another fetch. Retain the acknowledged overlay until pre-write reads drain, or invalidate those reads when the layout write is acknowledged.
Useful? React with 👍 / 👎.
| _showCompletedView = true; | ||
| _completedViewWasAutoRevealed = true; | ||
| } | ||
| _layoutWrites.retryPending(); |
There was a problem hiding this comment.
Retry pending writes after every successful task read
When a failed layout write is pending and the next successful task request is pagination via loadMoreActionItems or a saved-task point read via receiveSavedTask, neither path reaches this sole retryPending() call even though both now apply overlay(). The edit therefore remains only local unless the user later makes another edit or performs a first-page fetch; invoke the retry hook from each successful read path so restored connectivity actually persists the pending layout.
Useful? React with 👍 / 👎.
|
Verdict: the production change is correct and nicely scoped — it fixes the real data-loss path (pending indent/sort edits were cleared before the PATCH and the result ignored), and the replacement write path is sound. No blocking issues; three small race windows worth a follow-up. What I verified:
Non-blocking follow-ups (also flagged by the bot review; I verified each against the code):
Performance: no new requests — same 500 ms debounce batching into one PATCH. Security: no new surfaces; data stays on the existing action-items API. Blast radius: confined to task list layout persistence in the app. USER IMPACT: when you indent a sub-task or reorder tasks, the change now stays put when the list refreshes and is retried instead of silently lost if the save fails. Nothing blocking from my side; the three races above are worth a small follow-up but don't hold this fix up. Leaving for human maintainer review before merge. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
|
Thanks for the review. I addressed all three follow-ups in
Tests: three new cases in |
|
Follow-up in |
Summary
Refs #5078. Fixes the "changes disappear" part only; parent links and moves are out of scope.
_flushIndentUpdatesand_flushSortUpdatescleared pending edits before the PATCH and ignored its result, so a failed save was lost.app/lib/providers/task_layout_writes.dart, soaction_items_provider.dartshrinks rather than growing past its line-count ratchet.Implementation and verification were AI-assisted.
Verification
flutter test --no-pub test/providers/action_items_layout_persistence_test.dart: 3 new tests pass. All 3 fail againstorigin/mainwith only the request seam added.flutter test --no-pub test/providers/action_item*_test.dart test/providers/deletion_consistency_test.dart test/providers/siri_authoritative_fetch_test.dart test/perf/scroll_task_cache_test.dart: 93 passed.flutter analyzeon the changed provider and test: only the existingmaterial.dartdeprecation info.UI-Evidence: none -- provider persistence fix; no layout change.
Product invariants affected
none
Failure-Class: none