fix(ai): prevent stale agent edit overwrites - #309
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Limit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughChangesAgent document conflict handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LeftPanel
participant chatRun
participant applyAgentDocumentIfCurrent
participant useProjectStore
LeftPanel->>useProjectStore: Capture document and revision
LeftPanel->>chatRun: Start agent run
chatRun-->>LeftPanel: Return agent document
LeftPanel->>applyAgentDocumentIfCurrent: Apply with expected revision
applyAgentDocumentIfCurrent->>useProjectStore: Check current revision
alt Revision unchanged
applyAgentDocumentIfCurrent->>useProjectStore: Set and save document
applyAgentDocumentIfCurrent-->>LeftPanel: Return applied
else Revision changed
applyAgentDocumentIfCurrent-->>LeftPanel: Return conflict
LeftPanel->>LeftPanel: Show warning toast
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Reviewed at the PR head (3a2255db). Verdict: ship with nits, but please read §1 — the failure mode is more common than it looks.
The guard itself is correct for the window it targets, and I checked the things that usually go wrong here and did not find them: documentRevision is read from the same getState() snapshot as documentSnapshot (LeftPanel.tsx:930-932) and before the await chatRun, so there is no read-after-await bug. The comparison expectedRevision !== undefined && store.revision !== expectedRevision correctly treats revision 0 as a real value, and !== rather than < also catches a counter moving backwards. No stale closure — applyAgentDocument is a []-dep wrapper over an imported module function reading live state via getState() at call time.
Blast radius is genuinely small: only two call sites apply an agent document, and both go through the new module. I confirmed no chat event carries a document (AiEditionChatEvent in src/native/contracts.ts:779-784 is text/thinking/toolStart/toolEnd/error only), and that runTimelineOperation (chat-service.ts:526) has no IPC handler and no renderer caller, so it is not a live bypass.
i18n is clean — npm run i18n:check passes, all 13 locales have the key at the same position after chat.applyEditsFailed, every value is genuinely translated (zh-TW is correctly distinct from zh-CN, not a copy), and terminology for "agent" matches the neighbouring keys in each language. Both typecheck configs, biome and the new tests all pass.
1. A conflict discards the whole turn, and background writers will trigger it routinely
This is the finding I'd most like addressed before merge. The conflict path drops result.document on the floor with no recovery, and the thing that moves revision is usually not the user.
transcriptionStore.ts transcribes every asset that lands in the document automatically in the background and finishes with saveDocument at :398 → revision + 1. Same for the load-time dimension probe (useTimeline.ts:140-157) and the camera auto-link (projectStore.ts:172).
Concretely: import a fresh 5-minute recording, ask "cut the silences". Whisper finishes 20s into the agent turn. applyResult === "conflict". The user's 30 seconds and their tokens are gone, result.document is unrecoverable except by re-asking (which races again) — and the chat still renders the assistant's "done, I removed 14 silences" text plus its green tool-call summary chips, because LeftPanel.tsx:953-966 appends the message unconditionally. The only feedback is a toast blaming "the project changed" for something the user never did.
Minimal improvement — keep the document and make the toast actionable:
const pending = result.document;
toast.warning(t("chat.agentEditConflict"), {
action: { label: t("chat.applyAnyway"), onClick: () => void applyAgentDocumentIfCurrent(pending) },
});Better long-term would be re-running the turn against the fresh snapshot.
2. The guard closes the read window but not the write window
agentDocumentApply.ts:23-24 — the check happens before the write, but setDocument + saveDocument is not atomic.
projectStore.saveDocument (projectStore.ts:214-226) awaits nativeBridgeClient.aiEdition.save(document) and only then calls set({ document: parsed, revision: +1 }). So: guard passes → setDocument(agentDoc) repaints the timeline with the agent's cuts → the user immediately drags a clip (the moment they are most likely to react) → useTimeline calls its own saveDocument → both IPC calls are in flight independently → whichever set() resolves last wins in memory and on disk. The user's drag is silently reverted.
The PR narrows the window from "the whole agent turn" (seconds) to "one IPC + disk write" (tens of ms), which is a real improvement — but it is the same bug the PR is named after, just smaller. Cheapest fix inside the module is to re-check after the await. The structurally better version is to move the compare-and-swap into projectStore.saveDocument itself (optional expectedRevision arg), so useTimeline/transcriptionStore/captions all inherit it — or to route agent applies through the existing serialising queue in useSequentialTimelineOps.ts, whose file header documents precisely this "two concurrent calls both read the pre-edit doc and the second clobbers the first" race.
3. On a save failure, the UI shows the edits while the toast says they were rejected
agentDocumentApply.ts:23-24 — setDocument(parsed) mutates the store (and sets dirty: true) before saveDocument can throw.
Project file locked by another process, or disk full → saveDocument throws → LeftPanel.tsx:947-951 shows "Could not apply the agent's edits" → but the timeline is displaying the agent's edits anyway, dirty is true, and the next unrelated edit's saveDocument persists the agent's document to disk. The user was told it was rejected.
const prev = store.document;
store.setDocument(parsed);
try {
await store.saveDocument(parsed);
} catch (e) {
if (prev) store.setDocument(prev);
throw e;
}(Pre-existing ordering, but the new module is where it now lives.)
4. The rewind button undoes the protection the conflict just gave
LeftPanel.tsx:1026 — confirmRewind calls applyAgentDocument(doc) with no expectedRevision, deliberately. But it sits right next to the message that just conflicted: conflict toast appears → user clicks ↩ to retry the turn → chatRewind returns the checkpoint recorded before the turn (chat-service.ts:355-361) → the manual edit is overwritten, five seconds after the app told the user it was preserving it. The rewind popover copy doesn't warn that current work will be replaced.
Either pass the revision on that path too with a second confirmation, or amend the confirmation copy to say the current document will be replaced by the checkpoint.
Nits
LeftPanel.tsx:930-943—expectedRevisionis passed even whendocumentSnapshotisundefined. In that caserunChatruns the agent againstemptyDocumentForTextOnly(projectId)(chat-service.ts:401) carrying the real project id, so if the agent mutates it and the revision happens to match, a near-empty document gets written over a real project. I could not construct this today (loadProjectsetsprojectIdanddocumentin the sameset(),clear()nulls both), so purely defensive:if (result.document && documentSnapshot).LeftPanel.tsx:876-879—applyAgentDocumentis auseCallbackidentity wrapper around an already-stable module-level function. Five lines of indirection plus a pointless entry inconfirmRewind's dep array. CallingapplyAgentDocumentIfCurrent(...)directly at both sites would be clearer.agentDocumentApply.ts— the comment explaining why bothsetDocumentandsaveDocumentare called did not survive the move.setDocumentnow looks redundant next tosaveDocument(which also setsdocument), so the obvious "simplification" is to delete it — which silently breaks Ctrl+Z after an agent edit, sincesetDocumentis the only thing that pushes the previous document onto the undo stack (projectStore.ts:228-236). Worth carrying that sentence over.- Extracting to a module was the right call despite the guard being ~3 lines —
LeftPanel.tsxis 1976 lines with no test file, so this is what makes the tests possible at all.
One coverage gap worth knowing: the three tests exercise the module, but nothing pins the thing that would actually break. Move the documentRevision read below the await chatRun and all three still pass with the bug fully restored. A render test on ChatStripPanel, or extracting the send-and-apply sequence, would cover it.
3a2255d to
17fa988
Compare
|
Picking this up — @arhxam hasn't come back on the review. Pushed to the same branch, rebased onto §1 — a conflict discarded the whole turn. This was the one you most wanted addressed, and it's fixed. The document is kept and the toast carries an Apply anyway action, with no auto-dismiss since it's the only way back to it. Your point about who moves §2 — the write window. Deliberately not done here, and here's the coordination note: your §3 on #308 asks for the compare-and-swap's natural home, That also means these two PRs must merge in order. #308 makes §3 — edits on screen while the toast said rejected. Fixed. The previous document is restored on a failed save, through §4 — rewind undid the protection. The confirmation copy now says edits made since are replaced too, in all 13 locales. It sits one click from the conflict toast, so it had to stop saying the opposite. Nits. The The coverage gap. You noted that moving the Verification. 1810 passed / 5 skipped; both typecheck configs, biome, i18n and docs clean. Removing the rollback fails 1 test. |
Three things the conflict guard left open. **The turn was thrown away.** On conflict the document was dropped on the floor while the chat went on rendering the assistant's "done, I removed 14 silences" and its green tool-call chips, and the only feedback was a toast blaming "the project changed". The thing that usually moves `revision` mid-turn is not the user: `transcriptionStore` transcribes every asset that lands in a document in the background and finishes with a save. So importing a five-minute recording and asking for silences to be cut costs a minute of waiting and the tokens, for something the user never did. The document is kept now and the toast carries "Apply anyway", with no auto-dismiss, because it is the only way back to it. **A failed save left the edits on screen.** `setDocument` runs before `saveDocument` can throw, so a locked project file showed the agent's edits under a toast saying they had been rejected -- with `dirty` set, so the next unrelated save persisted them. The previous document is restored now, through `setState` so the rejected one does not enter the undo stack. **Rewind quietly did the opposite of what the conflict just promised.** The confirmation sits one click away from the conflict toast and replaces the live document with the checkpoint, manual edit and all. Its copy said only that the agent's turns would be rolled back. It now says the edits made since are replaced too. The send-and-apply pair moves into `runAgentTurn`, which is what makes the guard testable: the revision has to be read from the same store snapshot as the document and before the turn is awaited, and none of that was visible at the call site, where the two are twenty lines apart in a 2,000-line component. Moving the read below the `await` restored the bug in full with all three tests green. It now fails two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
17fa988 to
25f7c6a
Compare
#308 and #309 merged twelve seconds apart and their contracts disagree. `projectStore.saveDocument` returns a boolean and never rejects now, so the `try`/`catch` #309 put around it went dead: nothing type-checks wrong, no test in either PR could see it, and `main` went red on the one test that could. What shipped: an agent edit whose write failed stayed on screen with `dirty` set, was never rolled back, and `applyAgentDocumentIfCurrent` returned "applied" for a write that never happened. The next unrelated save would then persist the document we had just told the user was rejected -- the exact failure #309 exists to prevent, reintroduced by landing its fix next to the one that changed the signature underneath it. The check is on the returned `false` now, and "save-failed" is its own result rather than being folded into "conflict": the two need different words. A conflict means the project moved and the turn is still in hand, so it offers "Apply anyway"; a failed write means the disk refused, and retrying it would just fail again. `LeftPanel` says what it cost without repeating why -- the store already toasted the native error -- because the assistant's "done, I removed 14 silences" renders either way, and a bare save error next to it leaves the two unconnected. Its `catch` stays for the one throw left on this path, a malformed document from the agent, and no longer mislabels it a conflict. Both halves are pinned: removing the rollback fails the test, and so does returning "applied" without checking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary
Related issue
Fixes #284
Type of change
Release impact
Desktop impact
Screenshots / video
Not included; the visible change is a localized warning toast on a concurrent-edit conflict.
Testing
npx vitest --run src/lib/ai-edition/store/agentDocumentApply.test.ts(3 passed)npm run test(1,680 passed, 1 skipped across 141 files)npx tsc --noEmitnpx tsc -p tsconfig.test.json --noEmitnpx biome checkon the implementation, test, component, and locale filesnpm run docs:checknpm run i18n:checknpm run build-viteAuthored with Codex assistance and manually verified against the issue's concurrent-edit reproduction.
Summary by CodeRabbit
New Features
Localization
Bug Fixes