fix(ai): prevent stale agent edit overwrites - #309
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ 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.
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