fix(editor): surface failed timeline saves - #308
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 (10)
📝 WalkthroughWalkthroughTimeline saves now use a shared mutation helper. Save failures show an error toast and return failure results. Sequential operations continue processing. Direct mutations stop dependent state updates and restore live-edit documents when persistence fails. ChangesTimeline save handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/ai-edition/store/useSequentialTimelineOps.test.ts`:
- Around line 148-163: Update the test around the sequential apply calls to
capture the result of the second operation, assert that it is non-null, and
verify that saveDocument receives the second write. Keep the existing
first-result and toast-error assertions, while ensuring the test distinguishes a
genuinely successful second operation from one resolving to null.
In `@src/lib/ai-edition/store/useTimeline.ts`:
- Around line 519-523: Remove the revision increment from both failed-live-edit
rollback setState calls in src/lib/ai-edition/store/useTimeline.ts at lines
519-523 and 631-635. Keep the conditional document match and rollback
document/dirty updates unchanged; these rollback paths must not modify revision.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fcbfd92-ad30-4fc0-b840-49c1c151e412
📒 Files selected for processing (7)
src/components/ai-edition/NewEditorShell.tsxsrc/components/ai-edition/v4/V4Timeline.tsxsrc/lib/ai-edition/store/timelineSave.tssrc/lib/ai-edition/store/useSequentialTimelineOps.test.tssrc/lib/ai-edition/store/useSequentialTimelineOps.tssrc/lib/ai-edition/store/useTimeline.test.tssrc/lib/ai-edition/store/useTimeline.ts
|
@coderabbitai review Removed the unrelated |
|
|
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Reviewed at the PR head (ffa0cc9f). Verdict: needs work — one user-visible regression introduced by the third commit, plus the boundary sits one level below where it would actually cover everything.
The saveTimelineMutation boundary and the optimistic-rollback shape are both well built, and the test changes are genuinely strengthened, not weakened — I checked, since the diff shows +/- on existing assertions. The replaced useSequentialTimelineOps assertions swapped firstSettled/expect(err.message) for expect(firstResult).toBeNull() plus a toast assertion plus a new toHaveBeenNthCalledWith(2, secondResult), and commit 2 added the revision === 3 assertion. All three new tests genuinely fail against main. Both test files correctly carry the // @vitest-environment jsdom pragma. Verified locally: both typecheck configs clean, biome clean, 66/66 in store/, 109/109 in components/ai-edition.
1. The third commit's partial revert ships a contradictory toast
chore(editor): keep auto-zoom change scoped reverted the caller-side guard in V4Timeline but kept the producer change in addZoomsBulk (useTimeline.ts:229). The two halves now disagree.
Click Auto-enhance on a project whose file is locked or on a full disk:
addZoomsBulktoasts "Save failed" and returns0instead of throwing.V4Timeline.tsx:1061-1064then unconditionally runstoast.success(t("toolbar.addedAutoZoomPlural", { count: 0 })).- The user sees "Added 0 automatic zooms" stacked on top of "Save failed", and no zoom exists.
Before this PR the throw reached the catch at V4Timeline:1065 and produced a single correct toolbar.autoZoomFailed toast. This is a regression, and CI cannot see it. Either restore the line commit ffa0cc9f removed:
const added = await tl.addZoomsBulk(suggestions);
if (added === 0) return;…or make addZoomsBulk keep throwing. The current pairing is the worst of both. Worth a test that mocks a failing save and asserts toast.success is not called.
2. The file this PR fixes still has an uncaught detached save
useTimeline.ts:144-155 — the dimension-backfill effect still calls the raw store save inside void (async () => { … })() with no .catch:
Open a project whose assets have no probed video.width/height (any project migrated from before dimensions were probed) while the project file is read-only → useProjectStore.getState().saveDocument(...) throws → unhandled promise rejection in the renderer, no toast, no log. This is exactly the class the PR header claims to close, and the new memoised saveDocument wrapper at :101 makes the file look uniformly covered, which is how the next reader will miss it.
The effect deliberately reads getState() to stay out of the dep array, so pass the function through the same way NewEditorShell does with persist:
await saveTimelineMutation(useProjectStore.getState().saveDocument, { ...doc });3. The boundary is one rung below the function every save already routes through
This is the main structural note. projectStore.saveDocument (projectStore.ts:214) is the single function all ~40 save call sites already flow through. Putting the catch+toast there (returning boolean) would be a smaller total diff than the current one and would cover every path. As written, the timeline hook is protected and every sibling writer of the same document is still silent — all reached by void-ed callers, so they also emit unhandled rejections:
useEditorSettings.ts:51,68— callersRightPanes.tsx:1409,1424,1440,1455,1648-1650,1768-1820,PreviewCanvas.tsx:374,V4Timeline.tsx:1360,1404,1422, andEditorTopBar.tsx:240which does not evenvoidituseCaptions.ts:68,85,94,111— caption settings and saved translations, viaCaptionsPane.tsx:189-510CaptionsPane.tsx:169,LeftPanel.tsx:879,transcriptionStore.ts:325,398NewEditorShell.tsx:680— picking Save in the unsaved-changes dialog swallows the error in a barecatch {}and resolves"cancel", so the window silently refuses to close with no message
Changing the wallpaper or a caption font on a locked project still loses the edit with zero feedback. Only five callers currently depend on the throw for control flow (NewEditorShell.tsx:322, 617, 652, 680, projectStore.ts:172).
If you would rather keep the scope tight, that is reasonable — but then at minimum route useEditorSettings.commit/set and useCaptions.set/commit/saveTranslation/deleteTranslation through saveTimelineMutation in this PR, so the gap is not silently inherited.
4. Rollback refs are never reset when the project changes
useTimeline.ts:112-115 — the four rollback/live refs outlive the project.
Drag a zoom focus point in project A; ZoomFocusOverlay returns null at :91 the moment focusMode flips to "auto", so endDrag/onFocusCommit never runs and zoomFocusRollbackRef still holds A's document. Open project B, hit the reset-focus button (FloatingInspector.tsx:583-584), and let that save fail: the state.document === doc check passes and the store is set to project A's document while projectId is B. The next successful save writes A's content into B.
Secondary cost: two full AxcutDocument snapshots pinned indefinitely per hook instance, and annotations can carry base64 image data URLs.
useEffect(() => {
zoomFocusRollbackRef.current = null;
zoomFocusLiveRef.current = null;
annotationRollbackRef.current = null;
annotationLiveRef.current = null;
}, [projectId]);…or guard the rollback with rollback.project.id === doc.project.id.
5. Rollback asserts a cleanliness it cannot prove
useTimeline.ts:518-524 and :630-636 set dirty: false unconditionally, but the rollback target is not guaranteed to be the last saved document.
Type into an annotation → commitAnnotationChange fires, its save still in flight → type again → updateAnnotationLive sets annotationRollbackRef = D1 (the unsaved doc from the first, failed commit) → the second save also fails → the store is set to D1 with dirty: false. dirty is the only input to beforeunload (NewEditorShell.tsx:283) and to electronAPI.setHasUnsavedChanges (:297), so the app now claims there is nothing to save and closes without prompting, dropping D1. Same shape with an interleaved useEditorSettings.setLive.
Simplest fix is to drop dirty: false from both updaters — leaving dirty as-is errs toward prompting, which is the safe direction.
6. Hardcoded English string in a 13-locale app
timelineSave.ts:18-21 raises toast.error("Save failed", …) in English. Every other toast from the timeline UI (V4Timeline, FloatingInspector) goes through t()/ts(). A French user gets "Save failed" next to "Zoom automatique ajouté", and npm run i18n:check cannot catch it because no locale file is touched — so it ships.
The module is outside a component, so either take the message as a parameter, or move the toast to the already-localised call sites. At minimum add the key to src/i18n/locales/*/timeline.json.
Nits
timelineSave.ts:16-22vsNewEditorShell.tsx:613-624—handleSavealready contains the identicaltoast.error("Save failed", { description: err instanceof Error ? err.message : String(err) })block, verbatim, and was not switched over. It also loses the newconsole.errorbreadcrumb.if (await saveTimelineMutation(saveDocument, doc)) toast.success("Project saved");useSequentialTimelineOps.ts:29,52-56—apply()now returnsnullfor both "no project loaded" (a no-op) and "the save failed" (user-visible error), with no way to distinguish them. Fine today since both callersvoidit. Also, the header comment now claims it "keeps detached UI calls from emitting unhandled rejections" — that is only true for save failures; a throw fromapplyTimelineOperationor the dynamicimport()still rejectsqueued, and both call sites arevoid-ed (NewEditorShell.tsx:559, 573).useTimeline.test.ts— the newdescribe("useTimeline save failures")block has abeforeEachbut noafterEach(() => vi.clearAllMocks()), unlike every sibling describe. Harmless only because it is currently last in the file.- Pre-existing but adjacent to this PR's theme:
NewEditorShell.tsx:1301-1302— Apply in the Edit Clip modal fires two concurrent saves built from the same staledocument, so changing both the source range and the crop silently loses one. With this PR the failure case also stacks two identical "Save failed" toasts for one click. This is the exact raceuseSequentialTimelineOpsexists to prevent; those two calls just don't go through it.
§1 is the one I'd call blocking — it's a regression this PR introduces. §2 and §3 are the difference between "the timeline hook is covered" and the PR's stated goal.
ffa0cc9 to
0a381c5
Compare
|
Picking this up — @arhxam hasn't come back on the review. Pushed to the same branch, rebased onto §1 — the contradictory toast. Blocking, and fixed. §3 — the boundary. Taken, and it drove the rest. The catch, the log and the toast are in §2 — falls out of §3. The dimension-backfill effect calls the store directly, so it is covered without the effect being touched. §4 — rollback refs outliving the project. Fixed, and it is the finding I would have been most annoyed to ship: your walkthrough reproduces. Cleared on §5 — §6 — hardcoded English. Gone. The toast goes through the existing Nits. On the five throw-dependent callers. None lost behaviour — the close handler still answers Merge order with #309. #309 adds a Verification. 1810 passed / 5 skipped; both typecheck configs, biome, i18n and docs clean, and no new biome warnings (12 before and after). Mutation-tested: making |
`transcriptionStore` had the only copy of "read the stored locale, validate it, translate" -- the thing any store needs to report to the user in their language. The project store is about to need it too, so it moves to `@/i18n/toastText` and takes the namespace as an argument instead of hardcoding `editor`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four findings from the review, all of them real.
**The boundary was one rung too low.** `saveTimelineMutation` wrapped the
timeline hook, so the timeline was covered while every sibling writer of
the same document stayed silent -- and they are all reached by `void`-ed
callers, so they emitted unhandled rejections with no toast, no log and no
clue. Changing the wallpaper or a caption font on a read-only project lost
the edit with zero feedback, through `useEditorSettings`, `useCaptions`,
`CaptionsPane`, `LeftPanel` and `transcriptionStore`. Picking Save in the
unsaved-changes dialog swallowed the error in a bare `catch {}` and
resolved "cancel", so the window silently refused to close.
`projectStore.saveDocument` is the one function all of them already call.
The catch, the log and the toast live there now and it returns a boolean
instead of throwing, which covers every path -- including the two direct
store saves the hook could never see, one of which is the dimension
backfill that fires on any project migrated from before dimensions were
probed. `timelineSave.ts` is gone; this is a smaller diff than the wrapper
it replaces. `dirty` is deliberately left set on failure: it is the only
input to the `beforeunload` guard and to `setHasUnsavedChanges`, so a
failed write is the last moment to claim there is nothing to save.
The five callers that used the throw for control flow now read the
boolean, and none of them lost behaviour: the close-request handler still
answers false, the unsaved dialog still cancels, Ctrl+N and Ctrl+O still
stay put. Their local "Save failed" / "Rename failed" toasts are gone --
a second, English-only copy of what the store now says in the user's
language, which is the other half of this: the toast read `"Save failed"`
hardcoded in a 13-locale app, and no locale file was touched, so
`i18n:check` could not see it. It goes through `project.failedToSave` now.
**Auto-enhance congratulated itself on a failed write.** `addZoomsBulk`
was changed to toast and return 0 instead of throwing, but its caller kept
toasting success unconditionally -- so a locked file produced "Added 0
automatic zooms" stacked on the failure, with no zoom anywhere. Before,
the throw reached the caller's `catch` and produced one correct message.
The caller guards on 0 again, and the producer's contract is now pinned by
a test.
**A rollback outlived its project.** `ZoomFocusOverlay` unmounts the
instant `focusMode` flips to "auto", so `endDrag` never runs and the
pre-drag snapshot stayed in the ref. Open another project, reset the focus,
let that save fail, and project A's document was restored into project B --
the next successful save then wrote A over B on disk. The refs are cleared
when the project changes, which also stops pinning two whole documents per
hook instance when annotations can carry base64 image data URLs.
**And the rollback claimed a cleanliness it could not prove.** It set
`dirty: false`, but the rollback target is the document the drag started
from, not the last SAVED one: with two commits in flight the first one's
unsaved document is what gets restored. Saying "clean" there let the window
close on real work without prompting.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0a381c5 to
797755d
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
Save failedtoast with the native write error while resolving detached mutation promises safelyThe issue's detached-call grep now only reports calls whose implementations pass through this resolving boundary; the two direct
state.saveDocumentresults are gone.Related issue
Fixes #282
Type of change
Release impact
Desktop impact
Screenshots / video
Not included; the visible change is an existing Sonner error toast when a forced save fails.
Testing
npx vitest --run src/lib/ai-edition/store/useTimeline.test.ts src/lib/ai-edition/store/useSequentialTimelineOps.test.ts(26 passed)npm run test(1,679 passed, 1 skipped)npx tsc --noEmitnpx tsc -p tsconfig.test.json --noEmitnpx biome checkon all seven changed filesnpm run docs:checknpm run i18n:checknpm run build-viteAuthored with Codex assistance and manually reviewed against every acceptance criterion in #282.
Summary by CodeRabbit
Bug Fixes
Tests