[malibio]

Summary

• kanban-consecutive-status-moves.test.ts › persists a second consecutive drag move… failed on main (Chromium and WebKit) with expected [ 1 ] to deeply equal [ 1, 2 ]. Bisected to c8358c9 (Type person and project nodes like task (closes #2807) #3033).
• Not a lost write: since Type person and project nodes like task (closes #2807) #3033 the second move's updateTaskNode goes out just after the card re-renders in its new column, so the synchronous versionsSent assertion ran before it. Holding the test 3s yields [1, 2] with no conflict.
• The assertion now waits for both writes via waitFor.

Test plan

• The test file passes 3/3 runs in test:browser (Chromium)
• Pre-push gate green

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3086

Review Type: Initial review (no earlier reviews or comments)

Requirements Check (#3080)

• The drag-move test passes in test:browser. The PR description reports 3/3 passing Chromium runs and a green pre-push gate. I did not re-run the suite because the machine is under load.

Is waitFor the right fix, or does it hide an ordering bug?

It is the right fix. The late second write is how the store is designed to work:

• updateTypedNode (shared-node-store.svelte.ts:3483+) applies the change locally first. It calls nodesSet and notifySubscribers before it calls PersistenceCoordinator.persist(..., { mode: 'immediate' }), so the card can show up in "Done" before any backend call runs.
• If the first write is still in flight, persist() queues the second one behind it (the isExecuting branch, around line 213). The queued write runs in a microtask after the first write's version confirmation has been written back. It then re-reads the version through sendPendingTypedFields. Because of this, a second write that goes out late can never be sent with an old version.
• The assertion still protects against the real bug. A stale or repeated version would produce [1, 1], and a lost write would never reach [1, 2]. In both cases waitFor times out and the test fails. The only change is that the test no longer depends on when the write is sent relative to the render, which the product never guaranteed.

Code Review Findings

🟢 Minimal, focused fix.
📁 packages/desktop-app/src/tests/browser/kanban-consecutive-status-moves.test.ts:231-235
The PR changes one assertion and nothing else. The new comment says why the wait is needed (the write is sent after the re-render) instead of describing what the code does. (KISS, atomic change)

🟡 [Improvement] The sibling select-driven test has the same timing risk.
📁 packages/desktop-app/src/tests/browser/kanban-consecutive-status-moves.test.ts:169-173
"persists a second consecutive move … without remounting" goes through the same path: optimistic updateTypedNode, then a queued persist. It also asserts expect(versionsSent).toEqual([1, 2]) synchronously right after a UI waitFor. It passes today, probably because the select path's event loop finishes a little sooner than the drag path's four dispatched events. That is luck, not a guarantee, and a timing change like the one #3033 made would break it the same way. Wrap it in the same waitFor so both tests stay consistent. It is a one-line change and prevents a follow-up issue for the same flake. (DRY in intent, test robustness)

🟢 Note: the conflictNotifications assertion was already close to vacuous.
📁 kanban-consecutive-status-moves.test.ts:236 (and line 173)
It now runs after both writes have been sent, which is the correct place. However, conflicts are only raised when a write errors with a version mismatch (shared-node-store.svelte.ts:2331/2351/2430/2543), and the mocked updateTaskNode never rejects. So this line can't fail with the current mock. The version sequence is the assertion that actually catches the bug. This is existing behaviour and doesn't block the PR. It would only matter if someone expects this line to catch a conflict. Making the mock reject when version !== expected would give the line real coverage, but that is optional.

🟢 Nit: the comment could name the mechanism.
📁 kanban-consecutive-status-moves.test.ts:231-232
Something like "the store applies the move optimistically before the write is sent" would tell a reader that this ordering is intended and not a race to investigate. Optional.

Recommendation: APPROVE

The change is a clear improvement. It fixes a timing assumption in the test without weakening the OCC check, and the product ordering is safe because the coordinator queues writes and re-reads the version. I suggest applying the same waitFor to the sibling test at line 172 before merge, but it is not a blocker.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Address-review summary

• ✅ 🟡 Sibling select-driven test: its versionsSent assertion now waits too (same optimistic updateTypedNode path).
• ✅ 🟢 Comments name the mechanism: the store applies the move before sending its write.
• ⏭️ 🟢 Make the mock reject on a version mismatch so conflictNotifications has teeth — pre-existing coverage gap, out of scope for a timing fix.

Commit 6addae7; test file 3/3 in test:browser, pre-push gate green.

Re-Review Decision

NO RE-REVIEW NEEDED — one assertion wrapped in waitFor plus comment wording; no logic change.

🤖 Generated with Claude Code