fix(editor): surface failed timeline saves - #308
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 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.
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