From 1e2cf876a14e094e39172b5d6333119daf2a24b3 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Fri, 21 Aug 2026 13:48:11 +0200 Subject: [PATCH 01/13] fix(editor): make undo/redo actually apply Ctrl+Z did nothing in the editor. The undo stack was only ever written by `projectStore.setDocument`, but every edit the user makes -- add a region (Z/T/C/S), delete one (Ctrl+D), rename the project, every timeline op -- goes through `saveDocument` instead. `past` stayed empty, `undo()` returned on its first line, and `useUndoRedoShortcuts` had already called `preventDefault()`, so the key was swallowed in silence. Record the outgoing document in one shared helper used by both writes, with `{ history: false }` for writes the user did not make: probe backfills, background transcripts, the camera auto-link, and the persist an undo itself triggers. Two further defects that would have defeated undo even once the stack filled: - `setDocument` pushed from `void import("./undo").then(...)`, a LATER microtask, so the push landed after `undo()` had re-armed its synchronous `enabled` guard. An undo's own write was recorded as a fresh edit and `pushHistory` cleared `future` on the way past: redo was gone before the user could reach it, and Ctrl+Z degraded into a one-deep A/B toggle. The stacks move to a dependency-free `undoStack.ts` so the store can push with a static import, and `undo`/`redo` now restore through `setState` directly rather than back through a recording write -- which removes the guard, and the race with it. The dynamic import bought nothing anyway: `NewEditorShell` already pulls `undo.ts` into the same chunk statically. - The Edit menu's `undo`/`redo` roles registered CmdOrCtrl+Z accelerators, letting the native menu's web-editing undo shadow the renderer handler. They keep their menu entries with `registerAccelerator: false`. Also: `onAfter` was an empty placeholder, so an undone document never reached disk; a live drag pushed a snapshot per pointermove and evicted real history behind sixty one-pixel steps; and `e.key === "z"` was case-sensitive, unlike the redo branches beside it, so Caps Lock broke Ctrl+Z. `undo.ts` had no test at all, which is why CI stayed green through all of this. Adds one: a save is recorded and reverted, redo reapplies it, the undo's own write is not recorded, and undo walks back more than one level. Seven of the nine fail against the old store. Fixes #433 --- electron/main.ts | 18 +- src/components/ai-edition/NewEditorShell.tsx | 15 +- .../ai-edition/store/agentDocumentApply.ts | 8 +- src/lib/ai-edition/store/projectStore.ts | 66 ++++++-- .../ai-edition/store/transcriptionStore.ts | 38 +++-- src/lib/ai-edition/store/undo.test.ts | 160 ++++++++++++++++++ src/lib/ai-edition/store/undo.ts | 58 +++---- src/lib/ai-edition/store/undoStack.ts | 34 ++++ src/lib/ai-edition/store/useCaptions.ts | 12 +- src/lib/ai-edition/store/useEditorSettings.ts | 13 +- src/lib/ai-edition/store/useTimeline.ts | 62 ++++--- 11 files changed, 388 insertions(+), 96 deletions(-) create mode 100644 src/lib/ai-edition/store/undo.test.ts create mode 100644 src/lib/ai-edition/store/undoStack.ts diff --git a/electron/main.ts b/electron/main.ts index 9992a27c7..b02d50246 100644 --- a/electron/main.ts +++ b/electron/main.ts @@ -275,8 +275,22 @@ function setupApplicationMenu() { { label: mainT("common", "actions.edit") || "Edit", submenu: [ - { role: "undo", label: mainT("common", "actions.undo") || "Undo" }, - { role: "redo", label: mainT("common", "actions.redo") || "Redo" }, + // `registerAccelerator: false` on both: the roles run `webContents.undo()`, + // the WEB EDITING undo, which does nothing outside a focused text field — + // and registering their accelerators lets the native menu eat Ctrl+Z / + // Ctrl+Shift+Z before the renderer's document-level handler + // (`useUndoRedoShortcuts`) ever sees the keydown. The items stay, greyed + // shortcut text and all, so text fields keep their menu entries. + { + role: "undo", + label: mainT("common", "actions.undo") || "Undo", + registerAccelerator: false, + }, + { + role: "redo", + label: mainT("common", "actions.redo") || "Redo", + registerAccelerator: false, + }, { type: "separator" }, { role: "cut", label: mainT("common", "actions.cut") || "Cut" }, { role: "copy", label: mainT("common", "actions.copy") || "Copy" }, diff --git a/src/components/ai-edition/NewEditorShell.tsx b/src/components/ai-edition/NewEditorShell.tsx index 4026266cb..1d650d5cf 100644 --- a/src/components/ai-edition/NewEditorShell.tsx +++ b/src/components/ai-edition/NewEditorShell.tsx @@ -160,8 +160,14 @@ export function NewEditorShell() { [transcriptions], ); const tl = useTimeline(); + // An undo only puts the restored document back in the store and marks it dirty, + // so without this the reverted state never reached disk: close the window and the + // edit the user just undid came back. `history: false` is load-bearing — a + // recording save here would push the restored document straight back onto the + // stack and clear the redo the undo had just created. useUndoRedoShortcuts(() => { - // ponytail: placeholder, wire when undo stack merges with history + const doc = useProjectStore.getState().document; + if (doc) void useProjectStore.getState().saveDocument(doc, { history: false }); }); const [copiedClipId, setCopiedClipId] = useState(null); const [projectSummaries, setProjectSummaries] = useState([]); @@ -368,7 +374,10 @@ export function NewEditorShell() { [{ startSec: 0, endSec: known }], "Auto-created full-duration clip", ); - void state.saveDocument(next); + // `history: false` for both writes in this callback: they are the probed + // duration being folded into the document on load, not something the user + // did — an undo landing on one of them would empty their timeline. + void state.saveDocument(next, { history: false }); return; } // Hand the probed duration to the pure document layer: it patches only the @@ -379,7 +388,7 @@ export function NewEditorShell() { // nothing is waiting, so there is nothing to guard here. const next = applyProbedDuration(doc, assetId, known); if (next !== doc) { - void state.saveDocument(next); + void state.saveDocument(next, { history: false }); } }, [setSourceDuration], diff --git a/src/lib/ai-edition/store/agentDocumentApply.ts b/src/lib/ai-edition/store/agentDocumentApply.ts index 781abbc6a..52b2c1b1a 100644 --- a/src/lib/ai-edition/store/agentDocumentApply.ts +++ b/src/lib/ai-edition/store/agentDocumentApply.ts @@ -24,10 +24,10 @@ export async function applyAgentDocumentIfCurrent( const parsed = ensureDocument(document); const previous = store.document; const previousDirty = store.dirty; - // Both calls, not just the save. `setDocument` is the only thing that pushes the - // outgoing document onto the undo stack, so deleting it as "redundant next to - // saveDocument, which sets `document` too" silently breaks Ctrl+Z after an agent edit. - // `saveDocument` is what reaches the disk. + // Both calls, not just the save. `setDocument` puts the agent's document on screen + // NOW, without waiting on the disk round-trip `saveDocument` awaits. Both record the + // outgoing document on the undo stack, and recording it twice is not a risk: the + // second sees the document the first already installed and skips. store.setDocument(parsed); if (await store.saveDocument(parsed)) return "applied"; diff --git a/src/lib/ai-edition/store/projectStore.ts b/src/lib/ai-edition/store/projectStore.ts index 4b48ccd27..85245f595 100644 --- a/src/lib/ai-edition/store/projectStore.ts +++ b/src/lib/ai-edition/store/projectStore.ts @@ -10,6 +10,7 @@ import { } from "../document/timeline"; import { type AxcutAsset, type AxcutDocument, documentSchema } from "../schema"; import { probeVideoDimensions } from "../timeline/duration"; +import { clearHistory, pushHistory } from "./undoStack"; // ponytail: thin Zustand wrapper over the native-bridge client. Keeps the // current project + revision counter in renderer memory; mutations round-trip @@ -17,6 +18,19 @@ import { probeVideoDimensions } from "../timeline/duration"; export type ProjectStatus = "idle" | "loading" | "ready" | "error"; +export interface DocumentWriteOptions { + /** + * Record the outgoing document on the undo stack. Defaults to `true`: a write + * is a user edit unless the caller says otherwise. + * + * Pass `false` for writes the user never asked for -- probe backfills, + * transcripts arriving from a background job, the restore an undo itself + * persists. Those must not become Ctrl+Z steps, and a persist that re-recorded + * the document it just restored would undo the undo. + */ + history?: boolean; +} + export interface ProjectState { projectId: string | null; document: AxcutDocument | null; @@ -50,8 +64,8 @@ export interface ProjectState { * an unhandled rejection in the renderer with no toast, no log and no clue -- * change a caption font on a read-only project and the edit was simply gone. */ - saveDocument: (document: AxcutDocument) => Promise; - setDocument: (document: AxcutDocument) => void; + saveDocument: (document: AxcutDocument, opts?: DocumentWriteOptions) => Promise; + setDocument: (document: AxcutDocument, opts?: DocumentWriteOptions) => void; replaceTimeline: (intervals: Interval[], reason: string) => Promise; restoreFullTimeline: () => Promise; setSourceDuration: (sec: number) => void; @@ -65,6 +79,25 @@ function parseDocument(value: unknown): AxcutDocument { return documentSchema.parse(value); } +/** + * Record `prev` as the state Ctrl+Z returns to, unless the caller opted out or + * this write is not a change (`commit`-style re-saves hand back the very object + * the store already holds, and a live drag's `setLive` then `commit` pair would + * otherwise take two undos to reverse one gesture). + * + * Synchronous on purpose -- see the header of `undoStack.ts` for what the + * deferred `import("./undo")` this replaced did to redo. + */ +function recordHistory( + prev: AxcutDocument | null, + next: AxcutDocument, + opts?: DocumentWriteOptions, +) { + if (opts?.history === false) return; + if (!prev || prev === next) return; + pushHistory({ projectId: prev.project.id, doc: structuredClone(prev) }); +} + export const useProjectStore = create((set, get) => ({ projectId: null, document: null, @@ -94,7 +127,7 @@ export const useProjectStore = create((set, get) => ({ dirty: false, lastSavedAt: new Date(), }); - void import("./undo").then(({ clearHistory }) => clearHistory()); + clearHistory(); } catch (error) { set({ status: "error", @@ -120,7 +153,7 @@ export const useProjectStore = create((set, get) => ({ dirty: false, lastSavedAt: new Date(), }); - void import("./undo").then(({ clearHistory }) => clearHistory()); + clearHistory(); return document; } catch (error) { set({ @@ -198,7 +231,10 @@ export const useProjectStore = create((set, get) => ({ // Only adopt the linked document if it actually reached disk -- otherwise // the caller is handed a document claiming a camera link the file does not // have. The store has already told the user the write failed. - if (await get().saveDocument(next)) document = parseDocument(next); + // `history: false`: linking a camera is part of adding the asset, not an + // edit of its own -- and `get().document` here is still the pre-add document, + // so recording it would make Ctrl+Z jump back past the import. + if (await get().saveDocument(next, { history: false })) document = parseDocument(next); } // success:false just means no camera was found for this asset — // the normal case for a plain imported video. Nothing to surface. @@ -239,7 +275,14 @@ export const useProjectStore = create((set, get) => ({ }); }, - async saveDocument(document) { + async saveDocument(document, opts) { + // Snapshot BEFORE the await, while `get().document` is still the pre-edit one. + // This is where undo history actually comes from: the editor writes through + // `saveDocument` for every user edit -- add a region, delete one, rename the + // project, every timeline op -- and `setDocument` is reserved for the handful + // of live/optimistic paths. Recording only in `setDocument` left `past` empty + // for everything the user does, so Ctrl+Z was a no-op (#433). + recordHistory(get().document, document, opts); try { const result = await nativeBridgeClient.aiEdition.save(document); if (!result.success || !result.document) { @@ -268,15 +311,8 @@ export const useProjectStore = create((set, get) => ({ } }, - setDocument(document) { - const prev = get().document; - if (prev && prev !== document) { - // ponytail: push snapshot to undo history. Defer import to avoid - // pulling the undo module into the store at module-load time. - void import("./undo").then(({ pushHistory }) => { - pushHistory({ projectId: prev.project.id, doc: structuredClone(prev) }); - }); - } + setDocument(document, opts) { + recordHistory(get().document, document, opts); set({ document, revision: get().revision + 1, diff --git a/src/lib/ai-edition/store/transcriptionStore.ts b/src/lib/ai-edition/store/transcriptionStore.ts index d851bd4a8..64d910b13 100644 --- a/src/lib/ai-edition/store/transcriptionStore.ts +++ b/src/lib/ai-edition/store/transcriptionStore.ts @@ -318,21 +318,25 @@ async function persistPermanentFailure( // Best-effort bookkeeping: `saveDocument` reports its own failures and resolves // false rather than throwing, and a note on the asset is not worth a second // message on top of the one the user already got. - const persisted = await project.saveDocument({ - ...doc, - assets: doc.assets.map((a) => - a.id === assetId - ? { - ...a, - transcriptionFailure: { - kind, - message: failure.message, - at: new Date().toISOString(), - }, - } - : a, - ), - }); + const persisted = await project.saveDocument( + { + ...doc, + assets: doc.assets.map((a) => + a.id === assetId + ? { + ...a, + transcriptionFailure: { + kind, + message: failure.message, + at: new Date().toISOString(), + }, + } + : a, + ), + }, + // Bookkeeping, not an edit — it must not become an undo step. + { history: false }, + ); if (!persisted) { console.warn("[transcription] could not persist the failure on the asset"); } @@ -397,6 +401,9 @@ async function runJob(assetId: string, job: TranscriptionJob): Promise { } // One save: the transcript, and (on a successful retry) the removal of // the verdict remembered on the asset. + // `history: false`: a transcript landing from a background job is not an edit + // the user made, and making it the target of the next Ctrl+Z would both surprise + // them and throw the transcript away. await useProjectStore.getState().saveDocument( withTranscript( { @@ -407,6 +414,7 @@ async function runJob(assetId: string, job: TranscriptionJob): Promise { }, transcript, ), + { history: false }, ); dropJob(assetId, runId); if (job.manual) toast.success(toastText("mediaStage.transcriptReady")); diff --git a/src/lib/ai-edition/store/undo.test.ts b/src/lib/ai-edition/store/undo.test.ts new file mode 100644 index 000000000..4666a2b56 --- /dev/null +++ b/src/lib/ai-edition/store/undo.test.ts @@ -0,0 +1,160 @@ +// @vitest-environment jsdom +// +// Regression cover for #433: undo/redo silently did nothing. `undo.ts` had no +// test at all, which is exactly why CI stayed green while Ctrl+Z was dead — +// the history stack was only ever written by `setDocument`, and every edit the +// editor actually makes (add a region, delete one, rename the project) goes +// through `saveDocument`. + +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createEmptyDocument } from "../schema"; +import { useProjectStore } from "./projectStore"; +import { clearHistory, redo, undo } from "./undo"; +import { future, past } from "./undoStack"; + +const saveMock = vi.hoisted(() => vi.fn()); + +vi.mock("@/native/client", () => ({ + nativeBridgeClient: { + aiEdition: { save: saveMock }, + }, +})); + +const PROJECT_ID = "project_1"; + +function titled(title: string) { + return createEmptyDocument({ projectId: PROJECT_ID, title }); +} + +function currentTitle(): string | undefined { + return useProjectStore.getState().document?.project.title; +} + +describe("undo/redo", () => { + beforeEach(() => { + useProjectStore.getState().clear(); + clearHistory(); + saveMock.mockReset(); + // The bridge hands the document back; `saveDocument` re-parses what it returns, + // so the store ends up holding a structurally equal but distinct object — same + // as in the app. + saveMock.mockImplementation(async (document: unknown) => ({ success: true, document })); + useProjectStore.setState({ projectId: PROJECT_ID, document: titled("Original") }); + }); + + it("records a saveDocument edit and reverts it", async () => { + // The path every region add / delete / rename takes. It recorded nothing + // before this fix, so `past` stayed empty and `undo()` returned false. + await useProjectStore.getState().saveDocument(titled("Renamed")); + expect(currentTitle()).toBe("Renamed"); + expect(past).toHaveLength(1); + + expect(undo()).toBe(true); + expect(currentTitle()).toBe("Original"); + }); + + it("reapplies the edit on redo", async () => { + const renamed = titled("Renamed"); + await useProjectStore.getState().saveDocument(renamed); + + expect(undo()).toBe(true); + expect(currentTitle()).toBe("Original"); + expect(redo()).toBe(true); + expect(currentTitle()).toBe("Renamed"); + }); + + it("does not record the undo's own write as a new edit", async () => { + // The bug that made redo unreachable: the restore used to go back through + // `setDocument`, whose deferred `import("./undo")` pushed in a later microtask — + // after the re-entrancy guard had already been re-armed. The pre-undo document + // landed on `past` and `pushHistory` cleared `future`, so one Ctrl+Z turned the + // history into an A/B toggle with redo permanently gone. + await useProjectStore.getState().saveDocument(titled("Renamed")); + expect(past).toHaveLength(1); + + expect(undo()).toBe(true); + await Promise.resolve(); + await Promise.resolve(); + + expect(past).toHaveLength(0); + expect(future).toHaveLength(1); + }); + + it("walks back more than one level", async () => { + // The microtask race silently capped this at one: the second Ctrl+Z replayed + // the document the first had just pushed back on, so undo oscillated. + await useProjectStore.getState().saveDocument(titled("Second")); + await useProjectStore.getState().saveDocument(titled("Third")); + expect(currentTitle()).toBe("Third"); + + expect(undo()).toBe(true); + expect(currentTitle()).toBe("Second"); + expect(undo()).toBe(true); + expect(currentTitle()).toBe("Original"); + expect(undo()).toBe(false); + + expect(redo()).toBe(true); + expect(currentTitle()).toBe("Second"); + expect(redo()).toBe(true); + expect(currentTitle()).toBe("Third"); + }); + + it("skips history for writes the user did not make", async () => { + // Probe backfills, background transcripts and the persist an undo itself + // triggers all opt out — a Ctrl+Z that reverted one of those would either do + // nothing visible or throw the transcript away. + await useProjectStore.getState().saveDocument(titled("Backfilled"), { history: false }); + + expect(past).toHaveLength(0); + expect(undo()).toBe(false); + expect(currentTitle()).toBe("Backfilled"); + }); + + it("keeps the redo entry when the restored document is persisted", async () => { + // What `NewEditorShell` does in `useUndoRedoShortcuts`'s callback. Without + // `history: false` there, the persist re-records the restored document and + // wipes the redo the undo just created. + await useProjectStore.getState().saveDocument(titled("Renamed")); + expect(undo()).toBe(true); + + const restored = useProjectStore.getState().document; + if (!restored) throw new Error("no document to persist"); + await useProjectStore.getState().saveDocument(restored, { history: false }); + + expect(future).toHaveLength(1); + expect(redo()).toBe(true); + expect(currentTitle()).toBe("Renamed"); + }); + + it("drops the redo stack once a new edit lands on an undone document", async () => { + await useProjectStore.getState().saveDocument(titled("Renamed")); + expect(undo()).toBe(true); + expect(future).toHaveLength(1); + + await useProjectStore.getState().saveDocument(titled("Different branch")); + + expect(future).toHaveLength(0); + expect(redo()).toBe(false); + }); + + it("refuses to restore a snapshot from another project", async () => { + await useProjectStore.getState().saveDocument(titled("Renamed")); + useProjectStore.setState({ projectId: "project_2" }); + + expect(undo()).toBe(false); + expect(past).toHaveLength(0); + expect(future).toHaveLength(0); + }); + + it("marks the document dirty so the restore can be persisted", async () => { + await useProjectStore.getState().saveDocument(titled("Renamed")); + expect(useProjectStore.getState().dirty).toBe(false); + + const revisionBefore = useProjectStore.getState().revision; + expect(undo()).toBe(true); + + expect(useProjectStore.getState().dirty).toBe(true); + // Consumers repaint off `document`; `revision` is what the agent-apply guard reads. + expect(useProjectStore.getState().revision).toBe(revisionBefore + 1); + }); +}); diff --git a/src/lib/ai-edition/store/undo.ts b/src/lib/ai-edition/store/undo.ts index 80cda0d8d..875faaaaa 100644 --- a/src/lib/ai-edition/store/undo.ts +++ b/src/lib/ai-edition/store/undo.ts @@ -1,31 +1,33 @@ -// Lightweight undo/redo for the project document. The store fires -// `documentChanged` whenever `setDocument` is called; subscribers record -// snapshots up to a bounded history. Cmd+Z / Cmd+Shift+Z use the snapshot -// stack to roll back / roll forward. Designed to be small and side-effect -// free so it works in any renderer. +// Lightweight undo/redo for the project document. Every write that goes through +// `projectStore`'s `saveDocument` / `setDocument` records the outgoing document +// on the stack in `undoStack.ts` (callers opt out with `{ history: false }` for +// background writes). Ctrl+Z / Ctrl+Shift+Z / Ctrl+Y walk that stack. +// +// The restore writes `useProjectStore.setState` DIRECTLY rather than calling +// back into `setDocument`, which is what makes redo work: routing it through a +// recording write meant an undo re-recorded the document it had just replaced +// and cleared `future` on the way past, so redo was gone before the user could +// reach for it and Ctrl+Z degraded into an A/B toggle. Writing the state is also +// all a repaint needs — the timeline, preview and shell all subscribe to +// `s.document`. import { useEffect, useRef } from "react"; import { isModalOpen } from "../modalGuard"; +import type { AxcutDocument } from "../schema"; import { useProjectStore } from "./projectStore"; +import { clearHistory, future, past } from "./undoStack"; -type Snapshot = { projectId: string; doc: unknown }; +export { clearHistory, pushHistory } from "./undoStack"; -const MAX_HISTORY = 50; -const past: Snapshot[] = []; -const future: Snapshot[] = []; - -let enabled = true; - -export function pushHistory(snapshot: Snapshot) { - if (!enabled) return; - past.push(snapshot); - if (past.length > MAX_HISTORY) past.shift(); - future.length = 0; -} - -export function clearHistory() { - past.length = 0; - future.length = 0; +/** Put a snapshot back on screen without recording it as a new edit. `dirty` is + * set because the document on disk is no longer the one in memory — the caller + * of `useUndoRedoShortcuts` persists it. */ +function restore(doc: unknown) { + useProjectStore.setState((state) => ({ + document: doc as AxcutDocument, + revision: state.revision + 1, + dirty: true, + })); } export function undo(): boolean { @@ -38,9 +40,7 @@ export function undo(): boolean { } const doc = state.document; if (doc) future.push({ projectId: prev.projectId, doc: structuredClone(doc) }); - enabled = false; - state.setDocument(prev.doc as never); - enabled = true; + restore(prev.doc); return true; } @@ -54,9 +54,7 @@ export function redo(): boolean { } const doc = state.document; if (doc) past.push({ projectId: doc.project.id, doc: structuredClone(doc) }); - enabled = false; - state.setDocument(next.doc as never); - enabled = true; + restore(next.doc); return true; } @@ -82,7 +80,9 @@ export function useUndoRedoShortcuts(onAfter: () => void) { if (redo()) onAfterRef.current(); return; } - if (ctrl && e.key === "z") { + // `toLowerCase()`, like the two branches above: with Caps Lock on the browser + // reports "Z", and the bare `=== "z"` here fell through to nothing at all. + if (ctrl && e.key.toLowerCase() === "z") { e.preventDefault(); if (undo()) onAfterRef.current(); return; diff --git a/src/lib/ai-edition/store/undoStack.ts b/src/lib/ai-edition/store/undoStack.ts new file mode 100644 index 000000000..8cf1c3b56 --- /dev/null +++ b/src/lib/ai-edition/store/undoStack.ts @@ -0,0 +1,34 @@ +// The undo/redo snapshot stacks, in their own module so `projectStore` can push +// to them with a STATIC import. +// +// `setDocument` used to `void import("./undo").then(({ pushHistory }) => ...)`, +// which put the push in a LATER microtask. `undo()` restored its snapshot inside +// a synchronous `enabled = false` / `enabled = true` bracket, so by the time the +// deferred push ran the guard was armed again: an undo's own write was recorded +// as a fresh edit, `pushHistory` wiped `future`, and redo could never fire. The +// import bought nothing either — `NewEditorShell` already pulls `undo.ts` into +// the same chunk statically. +// +// This module deliberately imports nothing from the store, so `projectStore -> +// undoStack` is a leaf edge and there is no cycle to reason about. `undo.ts` +// re-exports `pushHistory` / `clearHistory` from here for existing callers. + +export type Snapshot = { projectId: string; doc: unknown }; + +const MAX_HISTORY = 50; + +export const past: Snapshot[] = []; +export const future: Snapshot[] = []; + +/** Record a document as the state to return to. Drops the redo stack: history + * branched the moment a new edit landed on top of an undone one. */ +export function pushHistory(snapshot: Snapshot) { + past.push(snapshot); + if (past.length > MAX_HISTORY) past.shift(); + future.length = 0; +} + +export function clearHistory() { + past.length = 0; + future.length = 0; +} diff --git a/src/lib/ai-edition/store/useCaptions.ts b/src/lib/ai-edition/store/useCaptions.ts index 981be3cf8..dabf3f8d9 100644 --- a/src/lib/ai-edition/store/useCaptions.ts +++ b/src/lib/ai-edition/store/useCaptions.ts @@ -4,7 +4,7 @@ // writes only (for sliders), `commit` flushes. The document stays the single // source of truth — nothing is cached here. -import { useCallback, useMemo } from "react"; +import { useCallback, useMemo, useRef } from "react"; import { type CaptionCue, type CaptionSettings, @@ -17,6 +17,7 @@ import { putCaptionTranslation, removeCaptionTranslation, } from "../captions"; +import type { AxcutDocument } from "../schema"; import { useProjectStore } from "./projectStore"; export interface UseCaptionsResult { @@ -70,11 +71,18 @@ export function useCaptions(): UseCaptionsResult { [setDocument, saveDocument], ); + // See `useEditorSettings.setLive`: one undo step per slider drag, not one per + // pointer move. `liveDocRef` holds what this hook last wrote, so only a write + // landing on someone else's document opens a new history entry. + const liveDocRef = useRef(null); + const setLive = useCallback( (patch: CaptionSettingsPatch) => { const doc = useProjectStore.getState().document; if (!doc) return; - setDocument(patchCaptionSettings(doc, patch)); + const next = patchCaptionSettings(doc, patch); + setDocument(next, { history: liveDocRef.current !== doc }); + liveDocRef.current = next; }, [setDocument], ); diff --git a/src/lib/ai-edition/store/useEditorSettings.ts b/src/lib/ai-edition/store/useEditorSettings.ts index 8dade1065..9af58dd79 100644 --- a/src/lib/ai-edition/store/useEditorSettings.ts +++ b/src/lib/ai-edition/store/useEditorSettings.ts @@ -12,7 +12,8 @@ // the patch through `patchEditorSettings`, and persists via the store. No // extra state, no caches — the document is the single source of truth. -import { useCallback, useMemo } from "react"; +import { useCallback, useMemo, useRef } from "react"; +import type { AxcutDocument } from "../schema"; import { type EditorSettingsPatch, type EditorSettingsSnapshot, @@ -53,11 +54,19 @@ export function useEditorSettings(): UseEditorSettingsResult { [setDocument, saveDocument], ); + // The document this hook's own last `setLive` produced. A slider drag fires one + // `setLive` per pointer move, and recording each of them buried the real history + // under sixty one-pixel steps; only the first write of a drag — the one editing a + // document this callback did not produce — is a state worth returning to. + const liveDocRef = useRef(null); + const setLive = useCallback( (patch: EditorSettingsPatch) => { const doc = useProjectStore.getState().document; if (!doc) return; - setDocument(patchEditorSettings(doc, patch)); + const next = patchEditorSettings(doc, patch); + setDocument(next, { history: liveDocRef.current !== doc }); + liveDocRef.current = next; }, [setDocument], ); diff --git a/src/lib/ai-edition/store/useTimeline.ts b/src/lib/ai-edition/store/useTimeline.ts index 788a8a4fc..c1fb293bf 100644 --- a/src/lib/ai-edition/store/useTimeline.ts +++ b/src/lib/ai-edition/store/useTimeline.ts @@ -137,10 +137,11 @@ export function useTimeline() { // (collectNativeFormats), the output resolution (referenceClipDims) and the export badges all // read it, so an unpopulated one silently drops that clip from ALL of them — which is why a // cropped clip could show under ORIGINAL while an un-probed 16:9 sibling was missing entirely. - // Probe once on load and persist via saveDocument (which doesn't touch undo/dirty), so the fix - // sticks and every consumer agrees without each re-probing on its own (what the export dialog - // used to do). Attempt each asset at most once per session, even on failure, so a file that - // can't be probed doesn't spin the effect on every document change. + // Probe once on load and persist via saveDocument with `history: false` (a write the user + // never made must not be what the next Ctrl+Z reverses), so the fix sticks and every consumer + // agrees without each re-probing on its own (what the export dialog used to do). Attempt each + // asset at most once per session, even on failure, so a file that can't be probed doesn't + // spin the effect on every document change. const probedAssetIdsRef = useRef>(new Set()); useEffect(() => { if (!document) return; @@ -185,22 +186,27 @@ export function useTimeline() { // Re-read fresh state so a concurrent edit made while probing isn't stomped. const current = useProjectStore.getState().document; if (!current) return; - await useProjectStore.getState().saveDocument({ - ...current, - assets: current.assets.map((a) => { - const found = probed[a.id]; - if (!found) return a; - return { - ...a, - ...(found.video - ? { video: { codec: "unknown", fps: 0, ...a.video, ...found.video } } - : {}), - ...(found.camera && a.cameraTrack - ? { cameraTrack: { ...a.cameraTrack, ...found.camera } } - : {}), - }; - }), - }); + // `history: false` — see the comment above: a backfill nobody asked for must + // not become the thing the next Ctrl+Z reverses. + await useProjectStore.getState().saveDocument( + { + ...current, + assets: current.assets.map((a) => { + const found = probed[a.id]; + if (!found) return a; + return { + ...a, + ...(found.video + ? { video: { codec: "unknown", fps: 0, ...a.video, ...found.video } } + : {}), + ...(found.camera && a.cameraTrack + ? { cameraTrack: { ...a.cameraTrack, ...found.camera } } + : {}), + }; + }), + }, + { history: false }, + ); })(); return () => { cancelled = true; @@ -548,14 +554,19 @@ export function useTimeline() { (id: string, focus: { cx: number; cy: number }) => { const doc = useProjectStore.getState().document; if (!doc) return; - if (zoomFocusLiveRef.current !== doc) zoomFocusRollbackRef.current = doc; + // The first live write of a drag is the one editing a document this callback + // did not itself produce — so it is the pre-drag state, and the only one worth + // recording. Without the flag a pointermove-frequency drag pushed ~60 snapshots + // a second and evicted the real history behind it. + const dragStart = zoomFocusLiveRef.current !== doc; + if (dragStart) zoomFocusRollbackRef.current = doc; const next: AxcutDocument = { ...doc, zoomRanges: patchPillById(doc.zoomRanges, id, { focus: { cx: finiteFraction(focus.cx), cy: finiteFraction(focus.cy) }, }) as AxcutDocument["zoomRanges"], }; - setDocument(next); + setDocument(next, { history: dragStart }); zoomFocusLiveRef.current = next; }, [setDocument], @@ -665,12 +676,15 @@ export function useTimeline() { (id: string, patch: Partial) => { const doc = useProjectStore.getState().document; if (!doc) return; - if (annotationLiveRef.current !== doc) annotationRollbackRef.current = doc; + // One undo step per drag, not per pointermove — same reasoning as + // `updateZoomFocusLive` above. + const dragStart = annotationLiveRef.current !== doc; + if (dragStart) annotationRollbackRef.current = doc; const next: AxcutDocument = { ...doc, annotations: patchPillById(doc.annotations, id, patch), }; - setDocument(next); + setDocument(next, { history: dragStart }); annotationLiveRef.current = next; }, [setDocument], From f85a82d57fafd5c2005e02b9d63361a3bb745c74 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Fri, 21 Aug 2026 14:26:44 +0200 Subject: [PATCH 02/13] fix(editor): record undo history only after a write lands, and force the call site to say Four defects in the first #433 fix, all of the same shape: the undo stack was being written from places that had not earned an entry. `DocumentWriteOptions.history` is now REQUIRED. Defaulting it to `true` is what let `probeAndCorrectClip` push a background probe onto the stack by saying nothing -- and `addAsset` never populates `durationSec`, so every freshly imported asset lands at the 60s placeholder and fires that probe. The first Ctrl+Z after a drop snapped the clip back to a 60s placeholder instead of removing it, and a probe resolving after an undo ran `pushHistory`, which clears `future`: redo destroyed by a write the user never made. Defaulting to `false` instead would recreate #433 itself the next time somebody added an edit, so there is no default at all. Omitting it is a compile error, which is what caught the remaining fifty call sites. `saveDocument` records BELOW the await, once the write is known to have landed. It resolves false on a handled failure -- a read-only project -- and callers already read that as "the edit did not happen". Recording above the try left `past` holding a snapshot identical to the live document with `future` wiped: the next Ctrl+Z visibly did nothing and redo was gone. That is #433's own symptom, reintroduced by the fix for it. `historyBase` names the document Ctrl+Z returns to when the store no longer holds it. The zoom-focus and annotation drags write every pointermove through `setDocument` with `history: false` and hand the pre-drag document to the commit, so a gesture is one undo step recorded once the save succeeds -- and a failed commit, whose rollback is a `setState` that could never have popped an entry, now has nothing to pop. `applyAgentDocumentIfCurrent` uses the same shape, so a rejected agent edit no longer leaves a phantom step and a cleared redo behind. `useCaptions` and `useEditorSettings` follow suit. electron/main.ts: `registerAccelerator: false` was a no-op where it mattered. Electron annotates the field `@platform linux,win32`, so on darwin it is ignored and AppKit still matches the menu's Cmd+Z inside `-[NSApplication sendEvent:]`, before the key event reaches the web contents -- the renderer's keydown handler never runs, and Ctrl+Z did nothing. On Windows and Linux the roles were never the problem: menu accelerators there dispatch from the unhandled-keyboard-event path, after the renderer, which the existing `preventDefault()` already suppresses. So Undo/Redo stop being roles: they own the accelerator and forward `menu-undo` / `menu-redo` to the editor, which applies the same text-field rule its keydown path applies. `electron/edit-menu.ts` holds the submenu so it can be tested; `sendEditorUndoRedo` falls back to `webContents.undo()` when the focused window is not the editor, and never opens one. Tests, each verified to fail against the change it covers: - useTimeline: the probe stays off the stack; a probe landing after an undo leaves redo intact; a failed focus-drag commit leaves no step and keeps redo; a committed drag is exactly one step. - undo: a failed write records nothing, keeps `future`, and still records once a retry lands; a commit records the base it names; the Edit-menu handlers undo the document and leave a focused text field alone. - agentDocumentApply: a rejected agent edit leaves no step and keeps redo; an applied one is exactly one step. - edit-menu: Undo/Redo carry their own accelerator, no role, no `registerAccelerator`, and dispatch to the editor. Fixes #433 --- electron/edit-menu.test.ts | 73 +++++++ electron/edit-menu.ts | 62 ++++++ electron/electron-env.d.ts | 4 + electron/main.ts | 51 +++-- electron/preload.ts | 14 ++ src/components/ai-edition/CaptionsPane.tsx | 11 +- src/components/ai-edition/NewEditorShell.tsx | 58 ++++-- .../store/agentDocumentApply.test.ts | 86 ++++++-- .../ai-edition/store/agentDocumentApply.ts | 22 +- src/lib/ai-edition/store/projectStore.test.ts | 12 +- src/lib/ai-edition/store/projectStore.ts | 82 ++++++-- src/lib/ai-edition/store/undo.test.ts | 145 ++++++++++++- src/lib/ai-edition/store/undo.ts | 53 ++++- src/lib/ai-edition/store/useCaptions.ts | 27 ++- src/lib/ai-edition/store/useEditorSettings.ts | 20 +- .../store/useSequentialTimelineOps.test.ts | 12 +- .../store/useSequentialTimelineOps.ts | 12 +- src/lib/ai-edition/store/useTimeline.test.ts | 194 ++++++++++++++++++ src/lib/ai-edition/store/useTimeline.ts | 107 ++++++---- 19 files changed, 869 insertions(+), 176 deletions(-) create mode 100644 electron/edit-menu.test.ts create mode 100644 electron/edit-menu.ts diff --git a/electron/edit-menu.test.ts b/electron/edit-menu.test.ts new file mode 100644 index 000000000..47ff19121 --- /dev/null +++ b/electron/edit-menu.test.ts @@ -0,0 +1,73 @@ +// Regression cover for the macOS half of #433. +// +// The Edit menu used to carry `role: "undo"` / `role: "redo"` with +// `registerAccelerator: false`. That field is documented `@platform linux,win32`, +// so on darwin it does nothing at all: AppKit still matches the menu's Cmd+Z key +// equivalent inside `-[NSApplication sendEvent:]`, before the key event reaches +// the web contents, and the editor's own keydown handler never runs. +// +// These tests pin the shape that actually reaches the renderer on every platform: +// an explicit accelerator and a click that dispatches to the editor. + +import { describe, expect, it, vi } from "vitest"; +import { buildEditMenuSubmenu, type EditorUndoRedoChannel } from "./edit-menu"; + +function build() { + const dispatch = vi.fn<(channel: EditorUndoRedoChannel) => void>(); + const items = buildEditMenuSubmenu({ + label: (_key, fallback) => fallback, + dispatch, + }); + return { items, dispatch }; +} + +describe("buildEditMenuSubmenu", () => { + it("owns Cmd+Z itself instead of leaning on registerAccelerator", () => { + const { items } = build(); + const undoItem = items.find((i) => i.label === "Undo"); + + expect(undoItem?.accelerator).toBe("CmdOrCtrl+Z"); + // The two things that made the previous version a no-op on macOS. + expect(undoItem?.role).toBeUndefined(); + expect(undoItem?.registerAccelerator).toBeUndefined(); + }); + + it("owns Shift+Cmd+Z for redo on the same terms", () => { + const { items } = build(); + const redoItem = items.find((i) => i.label === "Redo"); + + expect(redoItem?.accelerator).toBe("Shift+CmdOrCtrl+Z"); + expect(redoItem?.role).toBeUndefined(); + expect(redoItem?.registerAccelerator).toBeUndefined(); + }); + + it("routes both to the editor renderer, which owns the document's undo stack", () => { + // `webContents.undo()` -- what the roles ran -- is the WEB EDITING undo. It does + // nothing outside a focused text field, so on macOS Cmd+Z was swallowed by a menu + // item that could not have serviced it anyway. + const { items, dispatch } = build(); + + items + .find((i) => i.label === "Undo") + ?.click?.( + // The click signature carries a menu item, a window and the event; none of + // them are read here. + undefined as never, + undefined as never, + undefined as never, + ); + expect(dispatch).toHaveBeenCalledWith("menu-undo"); + + items + .find((i) => i.label === "Redo") + ?.click?.(undefined as never, undefined as never, undefined as never); + expect(dispatch).toHaveBeenCalledWith("menu-redo"); + }); + + it("leaves the clipboard items as roles", () => { + // They act on the focused text selection, which is exactly what the roles do -- + // and nothing in the editor shadows them. + const { items } = build(); + expect(items.map((i) => i.role).filter(Boolean)).toEqual(["cut", "copy", "paste", "selectAll"]); + }); +}); diff --git a/electron/edit-menu.ts b/electron/edit-menu.ts new file mode 100644 index 000000000..69e798a2b --- /dev/null +++ b/electron/edit-menu.ts @@ -0,0 +1,62 @@ +// The application menu's Edit submenu, split out of `main.ts` so it can be tested +// (same shape as `about.ts`). +// +// Undo/Redo are deliberately NOT `role: "undo"` / `role: "redo"`. +// +// Those roles run `webContents.undo()`, the WEB EDITING undo, which does nothing +// at all outside a focused text field. On macOS their Cmd+Z key equivalent is +// matched by AppKit inside `-[NSApplication sendEvent:]`, BEFORE the key event +// reaches the web contents, so the editor's own document-level handler +// (`useUndoRedoShortcuts`) never sees the keydown and Ctrl+Z silently did +// nothing (#433). +// +// `registerAccelerator: false` does not fix that. Electron annotates the field +// `@platform linux,win32` (see `MenuItemConstructorOptions` in electron.d.ts), +// so on darwin it is ignored outright and the menu keeps the key equivalent. +// And on Windows and Linux the roles were never the problem: menu accelerators +// there are dispatched from the unhandled-keyboard-event path, i.e. AFTER the +// renderer, which the renderer's own `preventDefault()` already suppresses. +// +// So the items own the accelerator on every platform and forward to the editor +// renderer, which applies exactly the rule its keydown path applies: a focused +// text field gets the browser's text undo, anything else gets the document undo. +// `dispatch` is what falls back to `webContents.undo()` when the focused window +// is not the editor at all. + +import type { MenuItemConstructorOptions } from "electron"; + +/** IPC channels the Edit menu forwards to the editor renderer. */ +export type EditorUndoRedoChannel = "menu-undo" | "menu-redo"; + +export interface EditMenuOptions { + /** Localised label for `key`, falling back to `fallback` when untranslated. */ + label: (key: string, fallback: string) => string; + /** Route an undo/redo request to whichever window should service it. */ + dispatch: (channel: EditorUndoRedoChannel) => void; +} + +export function buildEditMenuSubmenu({ + label, + dispatch, +}: EditMenuOptions): MenuItemConstructorOptions[] { + return [ + { + label: label("actions.undo", "Undo"), + accelerator: "CmdOrCtrl+Z", + click: () => dispatch("menu-undo"), + }, + { + label: label("actions.redo", "Redo"), + accelerator: "Shift+CmdOrCtrl+Z", + click: () => dispatch("menu-redo"), + }, + { type: "separator" }, + // The clipboard roles keep theirs: they act on the focused text selection, + // which is precisely what `webContents.cut/copy/paste` do, and the editor has + // no document-level meaning for them to shadow. + { role: "cut", label: label("actions.cut", "Cut") }, + { role: "copy", label: label("actions.copy", "Copy") }, + { role: "paste", label: label("actions.paste", "Paste") }, + { role: "selectAll", label: label("actions.selectAll", "Select All") }, + ]; +} diff --git a/electron/electron-env.d.ts b/electron/electron-env.d.ts index 89494dd0d..4eb288e14 100644 --- a/electron/electron-env.d.ts +++ b/electron/electron-env.d.ts @@ -383,6 +383,10 @@ interface Window { onMenuLoadProject: (callback: () => void) => () => void; onMenuSaveProject: (callback: () => void) => () => void; onMenuSaveProjectAs: (callback: () => void) => () => void; + /** Edit > Undo / Redo. On macOS the menu is the only route Cmd+Z has to the + * renderer at all — see `electron/edit-menu.ts`. */ + onMenuUndo: (callback: () => void) => () => void; + onMenuRedo: (callback: () => void) => () => void; quitApp: () => void; setTitleBarOverlay: (color: string, symbolColor: string) => void; getPlatform: () => string; diff --git a/electron/main.ts b/electron/main.ts index b02d50246..5a755b824 100644 --- a/electron/main.ts +++ b/electron/main.ts @@ -33,6 +33,7 @@ import { import { parseCliArgs } from "./cli/args"; import { runCli } from "./cli/cliMain"; import { isDiagnosticModeEnabled, mainLogBuffer } from "./diagnostics/main-log-buffer"; +import { buildEditMenuSubmenu, type EditorUndoRedoChannel } from "./edit-menu"; import { loadAndRegisterGlobalShortcut, registerOpenAppShortcut, @@ -189,6 +190,25 @@ function sendEditorMenuAction( targetWindow.webContents.send(channel); } +/** + * Route the Edit menu's Undo/Redo to whoever should service it. + * + * Unlike `sendEditorMenuAction` this never CREATES an editor window: Cmd+Z is not + * a request to open the editor. And when the focused window is not the editor -- + * the launch window, the notes window -- the web-editing undo the `undo` role used + * to provide is the right one after all, so fall through to it. + */ +function sendEditorUndoRedo(channel: EditorUndoRedoChannel) { + const targetWindow = BrowserWindow.getFocusedWindow() ?? mainWindow; + if (!targetWindow || targetWindow.isDestroyed()) return; + if (!isEditorWindow(targetWindow)) { + if (channel === "menu-undo") targetWindow.webContents.undo(); + else targetWindow.webContents.redo(); + return; + } + targetWindow.webContents.send(channel); +} + function setupApplicationMenu() { const isMac = process.platform === "darwin"; const template: Electron.MenuItemConstructorOptions[] = []; @@ -274,32 +294,11 @@ function setupApplicationMenu() { }, { label: mainT("common", "actions.edit") || "Edit", - submenu: [ - // `registerAccelerator: false` on both: the roles run `webContents.undo()`, - // the WEB EDITING undo, which does nothing outside a focused text field — - // and registering their accelerators lets the native menu eat Ctrl+Z / - // Ctrl+Shift+Z before the renderer's document-level handler - // (`useUndoRedoShortcuts`) ever sees the keydown. The items stay, greyed - // shortcut text and all, so text fields keep their menu entries. - { - role: "undo", - label: mainT("common", "actions.undo") || "Undo", - registerAccelerator: false, - }, - { - role: "redo", - label: mainT("common", "actions.redo") || "Redo", - registerAccelerator: false, - }, - { type: "separator" }, - { role: "cut", label: mainT("common", "actions.cut") || "Cut" }, - { role: "copy", label: mainT("common", "actions.copy") || "Copy" }, - { role: "paste", label: mainT("common", "actions.paste") || "Paste" }, - { - role: "selectAll", - label: mainT("common", "actions.selectAll") || "Select All", - }, - ], + // Built in `edit-menu.ts` — read its header for why Undo/Redo are not roles. + submenu: buildEditMenuSubmenu({ + label: (key, fallback) => mainT("common", key) || fallback, + dispatch: sendEditorUndoRedo, + }), }, { label: mainT("common", "actions.view") || "View", diff --git a/electron/preload.ts b/electron/preload.ts index 2705e4ba0..6aff16407 100644 --- a/electron/preload.ts +++ b/electron/preload.ts @@ -354,6 +354,20 @@ contextBridge.exposeInMainWorld("electronAPI", { ipcRenderer.on("menu-save-project-as", listener); return () => ipcRenderer.removeListener("menu-save-project-as", listener); }, + // The Edit menu's Undo/Redo. On macOS this is the ONLY way Cmd+Z reaches the + // renderer: AppKit matches the menu's key equivalent before the key event is + // delivered to the web contents, so the document-level keydown handler never + // runs. See `electron/edit-menu.ts`. + onMenuUndo: (callback: () => void) => { + const listener = () => callback(); + ipcRenderer.on("menu-undo", listener); + return () => ipcRenderer.removeListener("menu-undo", listener); + }, + onMenuRedo: (callback: () => void) => { + const listener = () => callback(); + ipcRenderer.on("menu-redo", listener); + return () => ipcRenderer.removeListener("menu-redo", listener); + }, quitApp: () => { ipcRenderer.send("app-quit"); }, diff --git a/src/components/ai-edition/CaptionsPane.tsx b/src/components/ai-edition/CaptionsPane.tsx index 2bcd8c5ab..983852f1d 100644 --- a/src/components/ai-edition/CaptionsPane.tsx +++ b/src/components/ai-edition/CaptionsPane.tsx @@ -181,10 +181,13 @@ export function CaptionsPane() { const clearLegacyCaptionAnnotations = async () => { const doc = useProjectStore.getState().document; if (!doc) return; - await saveDocument({ - ...doc, - annotations: doc.annotations.filter((a) => a.annotationSource !== "auto-caption"), - }); + await saveDocument( + { + ...doc, + annotations: doc.annotations.filter((a) => a.annotationSource !== "auto-caption"), + }, + { history: true }, + ); }; return ( diff --git a/src/components/ai-edition/NewEditorShell.tsx b/src/components/ai-edition/NewEditorShell.tsx index 1d650d5cf..fd8607cd4 100644 --- a/src/components/ai-edition/NewEditorShell.tsx +++ b/src/components/ai-edition/NewEditorShell.tsx @@ -165,7 +165,7 @@ export function NewEditorShell() { // edit the user just undid came back. `history: false` is load-bearing — a // recording save here would push the restored document straight back onto the // stack and clear the redo the undo had just created. - useUndoRedoShortcuts(() => { + const { runUndo, runRedo } = useUndoRedoShortcuts(() => { const doc = useProjectStore.getState().document; if (doc) void useProjectStore.getState().saveDocument(doc, { history: false }); }); @@ -317,7 +317,7 @@ export function NewEditorShell() { const doc = useProjectStore.getState().document; // The store already toasted the reason; answering false is what keeps the // window open on top of it. - if (doc) return await saveDocument(doc); + if (doc) return await saveDocument(doc, { history: true }); return true; }); @@ -651,15 +651,20 @@ export function NewEditorShell() { const handleSave = useCallback(async () => { const doc = useProjectStore.getState().document; if (!doc) return; - if (await saveDocument(doc)) toast.success("Project saved"); + if (await saveDocument(doc, { history: true })) toast.success("Project saved"); }, [saveDocument]); // Native File menu (electron/main.ts) → v4 actions. The menu is shown via // Menu.setApplicationMenu and dispatches these IPC events; the old editor // listened to them, but the v4 shell replaced it, leaving the File items // dead. Wire them to the same handlers the top-bar buttons use so the - // File/Edit/View menu bar works again (Edit/View items use Electron roles). + // File/Edit/View menu bar works again (the View items still use Electron roles). // The v4 editor has no separate "Save As" location, so it maps to Save. + // + // Edit > Undo/Redo are here too, and not roles: on macOS the menu's Cmd+Z key + // equivalent is matched by AppKit before the key event reaches the renderer, so + // this subscription is the ONLY thing that makes Ctrl+Z work there. See + // `electron/edit-menu.ts`. useEffect(() => { const api = window.electronAPI; if (!api) return; @@ -668,18 +673,20 @@ export function NewEditorShell() { api.onMenuLoadProject?.(() => setOpenProjectOpen(true)), api.onMenuSaveProject?.(() => void handleSave()), api.onMenuSaveProjectAs?.(() => void handleSave()), + api.onMenuUndo?.(runUndo), + api.onMenuRedo?.(runRedo), ]; return () => { for (const unsub of unsubscribers) unsub?.(); }; - }, [handleSave]); + }, [handleSave, runUndo, runRedo]); const handleRenameProject = useCallback( async (title: string) => { const doc = useProjectStore.getState().document; if (!doc) return; if (title === doc.project.title) return; - await saveDocument({ ...doc, project: { ...doc.project, title } }); + await saveDocument({ ...doc, project: { ...doc.project, title } }, { history: true }); }, [saveDocument], ); @@ -703,7 +710,7 @@ export function NewEditorShell() { // A failed save cancels the action that prompted this dialog. The store has // already said why -- which is what the bare `catch {}` here used to swallow, // leaving the window refusing to close with nothing on screen explaining it. - if (doc && !(await saveDocument(doc))) { + if (doc && !(await saveDocument(doc, { history: true }))) { resolve("cancel"); return; } @@ -797,24 +804,33 @@ export function NewEditorShell() { ); if (snapshot.kind === "zoom") { - await saveDocument({ - ...doc, - zoomRanges: [...doc.zoomRanges, ...anchored] as typeof doc.zoomRanges, - }); + await saveDocument( + { + ...doc, + zoomRanges: [...doc.zoomRanges, ...anchored] as typeof doc.zoomRanges, + }, + { history: true }, + ); } else if (snapshot.kind === "annotation") { - await saveDocument({ - ...doc, - annotations: [...doc.annotations, ...anchored] as typeof doc.annotations, - }); + await saveDocument( + { + ...doc, + annotations: [...doc.annotations, ...anchored] as typeof doc.annotations, + }, + { history: true }, + ); } else { // speed and cameraFullscreen are both plain spans on legacyEditor. const key = snapshot.kind === "speed" ? "speedRegions" : "cameraFullscreenRegions"; const legacy = (doc.legacyEditor as Record) ?? {}; const prev = (legacy[key] as unknown[]) ?? []; - await saveDocument({ - ...doc, - legacyEditor: { ...legacy, [key]: [...prev, ...anchored] }, - }); + await saveDocument( + { + ...doc, + legacyEditor: { ...legacy, [key]: [...prev, ...anchored] }, + }, + { history: true }, + ); } toast.success("Region pasted"); // `tl` belongs here now that the trim branch calls tl.addTrim: useTimeline @@ -889,7 +905,7 @@ export function NewEditorShell() { if (choice === "save") { const doc = useProjectStore.getState().document; // Stay put if the save did not land -- the store has already said why. - if (doc && !(await saveDocument(doc))) return; + if (doc && !(await saveDocument(doc, { history: true }))) return; } setNewProjectOpen(true); })(); @@ -903,7 +919,7 @@ export function NewEditorShell() { if (choice === "save") { const doc = useProjectStore.getState().document; // Stay put if the save did not land -- the store has already said why. - if (doc && !(await saveDocument(doc))) return; + if (doc && !(await saveDocument(doc, { history: true }))) return; } setOpenProjectOpen(true); })(); diff --git a/src/lib/ai-edition/store/agentDocumentApply.test.ts b/src/lib/ai-edition/store/agentDocumentApply.test.ts index 18e95f723..a7b3fe698 100644 --- a/src/lib/ai-edition/store/agentDocumentApply.test.ts +++ b/src/lib/ai-edition/store/agentDocumentApply.test.ts @@ -3,6 +3,8 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { createEmptyDocument } from "../schema"; import { applyAgentDocumentIfCurrent, runAgentTurn } from "./agentDocumentApply"; import { useProjectStore } from "./projectStore"; +import { clearHistory, redo, undo } from "./undo"; +import { future, past } from "./undoStack"; const saveMock = vi.hoisted(() => vi.fn()); @@ -15,6 +17,7 @@ vi.mock("@/native/client", () => ({ describe("applyAgentDocumentIfCurrent", () => { beforeEach(() => { useProjectStore.getState().clear(); + clearHistory(); saveMock.mockReset(); }); @@ -40,10 +43,13 @@ describe("applyAgentDocumentIfCurrent", () => { project: { ...before.project, title: "Agent edit" }, }; useProjectStore.setState({ projectId: "project_1", document: before, revision: 4 }); - useProjectStore.getState().setDocument({ - ...before, - project: { ...before.project, title: "Manual edit" }, - }); + useProjectStore.getState().setDocument( + { + ...before, + project: { ...before.project, title: "Manual edit" }, + }, + { history: true }, + ); await expect(applyAgentDocumentIfCurrent(agentResult, 4)).resolves.toBe("conflict"); @@ -70,6 +76,55 @@ describe("applyAgentDocumentIfCurrent", () => { expect(useProjectStore.getState().dirty).toBe(false); }); + it("leaves no undo step behind a rejected agent edit", async () => { + // The rollback is a `setState` by design, so it cannot pop an entry the apply + // already pushed. Recording on the optimistic `setDocument` therefore left a + // phantom Ctrl+Z step for an edit that never reached disk -- and `pushHistory` + // had cleared `future` on the way in, so the user's redo went with it. + const before = createEmptyDocument({ projectId: "project_1", title: "Before" }); + useProjectStore.setState({ projectId: "project_1", document: before, revision: 4 }); + + // A real edit and an undo first, so there is a redo entry to lose. + saveMock.mockResolvedValueOnce({ + success: true, + document: { ...before, project: { ...before.project, title: "User edit" } }, + }); + await useProjectStore + .getState() + .saveDocument( + { ...before, project: { ...before.project, title: "User edit" } }, + { history: true }, + ); + expect(undo()).toBe(true); + expect(past).toHaveLength(0); + expect(future).toHaveLength(1); + + saveMock.mockResolvedValue({ success: false, error: "EACCES" }); + const agentResult = { ...before, project: { ...before.project, title: "Agent edit" } }; + await expect(applyAgentDocumentIfCurrent(agentResult)).resolves.toBe("save-failed"); + + expect(past).toHaveLength(0); + expect(future).toHaveLength(1); + expect(redo()).toBe(true); + expect(useProjectStore.getState().document?.project.title).toBe("User edit"); + }); + + it("records exactly one undo step for an agent edit that lands", async () => { + // Two writes, one step: the optimistic `setDocument` opts out and the save + // names the pre-agent document as its base. Losing the step altogether would + // be the same class of bug in the other direction. + const before = createEmptyDocument({ projectId: "project_1", title: "Before" }); + const agentResult = { ...before, project: { ...before.project, title: "Agent edit" } }; + useProjectStore.setState({ projectId: "project_1", document: before, revision: 4 }); + saveMock.mockImplementation(async (document) => ({ success: true, document })); + + await expect(applyAgentDocumentIfCurrent(agentResult, 4)).resolves.toBe("applied"); + + expect(past).toHaveLength(1); + expect(undo()).toBe(true); + expect(useProjectStore.getState().document?.project.title).toBe("Before"); + }); + it("still rejects when the agent hands back something that is not a document", async () => { // The one throw left on this path, and the reason the caller keeps a try/catch. const before = createEmptyDocument({ projectId: "project_1", title: "Before" }); @@ -99,6 +154,7 @@ describe("applyAgentDocumentIfCurrent", () => { describe("runAgentTurn", () => { beforeEach(() => { useProjectStore.getState().clear(); + clearHistory(); saveMock.mockReset(); }); @@ -112,10 +168,13 @@ describe("runAgentTurn", () => { const { result, applyDocument } = await runAgentTurn(async (documentSnapshot) => { // A background transcription landing mid-turn, which is the common case. - useProjectStore.getState().setDocument({ - ...before, - project: { ...before.project, title: "Manual edit" }, - }); + useProjectStore.getState().setDocument( + { + ...before, + project: { ...before.project, title: "Manual edit" }, + }, + { history: true }, + ); return { document: { ...documentSnapshot, project: { ...before.project, title: "Agent edit" } }, }; @@ -133,10 +192,13 @@ describe("runAgentTurn", () => { saveMock.mockImplementation(async (document) => ({ success: true, document })); const { applyDocument } = await runAgentTurn(async () => { - useProjectStore.getState().setDocument({ - ...before, - project: { ...before.project, title: "Manual edit" }, - }); + useProjectStore.getState().setDocument( + { + ...before, + project: { ...before.project, title: "Manual edit" }, + }, + { history: true }, + ); return { document: { ...before, project: { ...before.project, title: "Agent edit" } } }; }); diff --git a/src/lib/ai-edition/store/agentDocumentApply.ts b/src/lib/ai-edition/store/agentDocumentApply.ts index 52b2c1b1a..6b0715fd5 100644 --- a/src/lib/ai-edition/store/agentDocumentApply.ts +++ b/src/lib/ai-edition/store/agentDocumentApply.ts @@ -25,19 +25,27 @@ export async function applyAgentDocumentIfCurrent( const previous = store.document; const previousDirty = store.dirty; // Both calls, not just the save. `setDocument` puts the agent's document on screen - // NOW, without waiting on the disk round-trip `saveDocument` awaits. Both record the - // outgoing document on the undo stack, and recording it twice is not a risk: the - // second sees the document the first already installed and skips. - store.setDocument(parsed); - if (await store.saveDocument(parsed)) return "applied"; + // NOW, without waiting on the disk round-trip `saveDocument` awaits. + // + // Only the SAVE records history, and `historyBase` is what makes that possible: by + // the time it runs, the store already holds the agent's document, so its own idea + // of "previous" is useless. Recording on the `setDocument` instead put the entry on + // the stack before the write was known to have landed — and the rollback below, a + // `setState` by design, could not take it back off. A rejected agent edit left a + // phantom Ctrl+Z step and a cleared `future` for something that never happened. + store.setDocument(parsed, { history: false }); + if (await store.saveDocument(parsed, { history: true, historyBase: previous })) { + return "applied"; + } // The edits are on screen by now. Leaving them there while the caller says they // were not applied tells the user two opposite things at once, and worse: `dirty` // is set, so the next unrelated save would quietly persist the document we just // said was rejected. // - // Restored through `setState` rather than `setDocument`, so the rejected document - // does not land on the undo stack. `revision` keeps the bump: it did move, and + // Restored through `setState` rather than `setDocument`, so the restore itself is + // not recorded — and there is nothing on the stack to take back off either, because + // the write above records only on success. `revision` keeps the bump: it did move, and // leaving it forward makes any in-flight guard read "conflict", which is the safe // direction to be wrong in. // diff --git a/src/lib/ai-edition/store/projectStore.test.ts b/src/lib/ai-edition/store/projectStore.test.ts index 7435eb733..9ad09572e 100644 --- a/src/lib/ai-edition/store/projectStore.test.ts +++ b/src/lib/ai-edition/store/projectStore.test.ts @@ -312,7 +312,9 @@ describe("useProjectStore", () => { bridgeMocks.save.mockResolvedValue({ success: false, error: "EACCES" }); const edited = { ...sampleDoc, project: { ...sampleDoc.project, title: "Edited" } }; - await expect(useProjectStore.getState().saveDocument(edited)).resolves.toBe(false); + await expect( + useProjectStore.getState().saveDocument(edited, { history: true }), + ).resolves.toBe(false); expect(toastMocks.error).toHaveBeenCalledWith("Failed to save project", { description: "EACCES", @@ -329,7 +331,9 @@ describe("useProjectStore", () => { useProjectStore.setState({ projectId: "proj_test", document: sampleDoc, dirty: true }); bridgeMocks.save.mockRejectedValue(new Error("bridge is gone")); - await expect(useProjectStore.getState().saveDocument(sampleDoc)).resolves.toBe(false); + await expect( + useProjectStore.getState().saveDocument(sampleDoc, { history: true }), + ).resolves.toBe(false); expect(toastMocks.error).toHaveBeenCalledWith("Failed to save project", { description: "bridge is gone", }); @@ -340,7 +344,9 @@ describe("useProjectStore", () => { useProjectStore.setState({ projectId: "proj_test", document: sampleDoc, dirty: true }); bridgeMocks.save.mockResolvedValue({ success: true, document: saved }); - await expect(useProjectStore.getState().saveDocument(saved)).resolves.toBe(true); + await expect(useProjectStore.getState().saveDocument(saved, { history: true })).resolves.toBe( + true, + ); expect(toastMocks.error).not.toHaveBeenCalled(); const state = useProjectStore.getState(); diff --git a/src/lib/ai-edition/store/projectStore.ts b/src/lib/ai-edition/store/projectStore.ts index 85245f595..a5432e784 100644 --- a/src/lib/ai-edition/store/projectStore.ts +++ b/src/lib/ai-edition/store/projectStore.ts @@ -20,15 +20,38 @@ export type ProjectStatus = "idle" | "loading" | "ready" | "error"; export interface DocumentWriteOptions { /** - * Record the outgoing document on the undo stack. Defaults to `true`: a write - * is a user edit unless the caller says otherwise. + * Record the outgoing document on the undo stack. * - * Pass `false` for writes the user never asked for -- probe backfills, - * transcripts arriving from a background job, the restore an undo itself - * persists. Those must not become Ctrl+Z steps, and a persist that re-recorded - * the document it just restored would undo the undo. + * REQUIRED, and deliberately not defaulted either way. A default is a decision + * nobody makes: defaulting to `true` let `probeAndCorrectClip` push a background + * probe's document onto the stack simply by not mentioning it, so the first + * Ctrl+Z after a drop snapped the clip back to its 60s placeholder instead of + * removing it -- and defaulting to `false` would recreate #433 itself the next + * time somebody added an edit. Making it a compile error to omit is the only + * version a new call site cannot get wrong by saying nothing. + * + * `true` for anything the user did. `false` for writes they never asked for: + * probe backfills, transcripts arriving from a background job, the camera + * auto-link, the optimistic half of an optimistic-write-then-save pair, and the + * persist an undo itself triggers (which would otherwise undo the undo). + */ + history: boolean; + /** + * The document Ctrl+Z should return to, when it is NOT the one the store holds + * at the moment of the write. + * + * A live drag writes every pointermove straight into the store with + * `history: false`, so by the time the pointerup commit runs, the "previous" + * document the store holds is the dragged one -- recording that would make the + * gesture un-undoable. The commit passes the PRE-DRAG document here instead, and + * because `saveDocument` records only after the write succeeded, a failed commit + * records nothing at all and its rollback has nothing to pop. + * + * `null` means "there is no state to go back to" and records nothing. Omitting + * the field falls back to the store's current document, which is what an + * ordinary edit wants. */ - history?: boolean; + historyBase?: AxcutDocument | null; } export interface ProjectState { @@ -64,8 +87,8 @@ export interface ProjectState { * an unhandled rejection in the renderer with no toast, no log and no clue -- * change a caption font on a read-only project and the edit was simply gone. */ - saveDocument: (document: AxcutDocument, opts?: DocumentWriteOptions) => Promise; - setDocument: (document: AxcutDocument, opts?: DocumentWriteOptions) => void; + saveDocument: (document: AxcutDocument, opts: DocumentWriteOptions) => Promise; + setDocument: (document: AxcutDocument, opts: DocumentWriteOptions) => void; replaceTimeline: (intervals: Interval[], reason: string) => Promise; restoreFullTimeline: () => Promise; setSourceDuration: (sec: number) => void; @@ -80,10 +103,19 @@ function parseDocument(value: unknown): AxcutDocument { } /** - * Record `prev` as the state Ctrl+Z returns to, unless the caller opted out or - * this write is not a change (`commit`-style re-saves hand back the very object - * the store already holds, and a live drag's `setLive` then `commit` pair would - * otherwise take two undos to reverse one gesture). + * The document a write should record as the state Ctrl+Z returns to. Read + * SYNCHRONOUSLY, before any await: after one, `get().document` is whatever the + * write installed. + */ +function historyBaseFor(opts: DocumentWriteOptions, current: AxcutDocument | null) { + return opts.historyBase !== undefined ? opts.historyBase : current; +} + +/** + * Push `prev` onto the undo stack, unless the caller opted out or this write is + * not a change (`commit`-style re-saves hand back the very object the store + * already holds, and an optimistic `setDocument` then `saveDocument` pair would + * otherwise take two undos to reverse one edit). * * Synchronous on purpose -- see the header of `undoStack.ts` for what the * deferred `import("./undo")` this replaced did to redo. @@ -91,9 +123,9 @@ function parseDocument(value: unknown): AxcutDocument { function recordHistory( prev: AxcutDocument | null, next: AxcutDocument, - opts?: DocumentWriteOptions, + opts: DocumentWriteOptions, ) { - if (opts?.history === false) return; + if (!opts.history) return; if (!prev || prev === next) return; pushHistory({ projectId: prev.project.id, doc: structuredClone(prev) }); } @@ -276,13 +308,13 @@ export const useProjectStore = create((set, get) => ({ }, async saveDocument(document, opts) { - // Snapshot BEFORE the await, while `get().document` is still the pre-edit one. + // Read BEFORE the await, while `get().document` is still the pre-edit one. // This is where undo history actually comes from: the editor writes through // `saveDocument` for every user edit -- add a region, delete one, rename the // project, every timeline op -- and `setDocument` is reserved for the handful // of live/optimistic paths. Recording only in `setDocument` left `past` empty // for everything the user does, so Ctrl+Z was a no-op (#433). - recordHistory(get().document, document, opts); + const base = historyBaseFor(opts, get().document); try { const result = await nativeBridgeClient.aiEdition.save(document); if (!result.success || !result.document) { @@ -295,6 +327,14 @@ export const useProjectStore = create((set, get) => ({ dirty: false, lastSavedAt: new Date(), }); + // Recorded HERE, below the write, and not above it. `saveDocument` resolves + // false on a handled failure (a read-only project) and callers read that as + // "the edit did not happen". Recording first left `past` holding a snapshot + // identical to the live document and `future` wiped, so the next Ctrl+Z + // visibly did nothing and redo was gone -- #433's own symptom, re-created by + // the fix for it. Nothing between the `set` above and this line awaits, so no + // undo can observe the half-applied state. + recordHistory(base, document, opts); return true; } catch (error) { // Logged as well as toasted: a toast is gone in five seconds, and "my edit @@ -312,7 +352,9 @@ export const useProjectStore = create((set, get) => ({ }, setDocument(document, opts) { - recordHistory(get().document, document, opts); + // No await to sit above: this write cannot fail, so recording it up front is + // the same thing as recording it afterwards. + recordHistory(historyBaseFor(opts, get().document), document, opts); set({ document, revision: get().revision + 1, @@ -324,14 +366,14 @@ export const useProjectStore = create((set, get) => ({ const doc = get().document; if (!doc) throw new Error("No project loaded"); const next = replaceTimelineOp(doc, intervals, reason); - await get().saveDocument(next); + await get().saveDocument(next, { history: true }); }, async restoreFullTimeline() { const doc = get().document; if (!doc) throw new Error("No project loaded"); const next = restoreFullTimelineOp(doc); - await get().saveDocument(next); + await get().saveDocument(next, { history: true }); }, setSourceDuration(sec) { diff --git a/src/lib/ai-edition/store/undo.test.ts b/src/lib/ai-edition/store/undo.test.ts index 4666a2b56..b85d805b6 100644 --- a/src/lib/ai-edition/store/undo.test.ts +++ b/src/lib/ai-edition/store/undo.test.ts @@ -6,10 +6,11 @@ // editor actually makes (add a region, delete one, rename the project) goes // through `saveDocument`. +import { act, renderHook } from "@testing-library/react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { createEmptyDocument } from "../schema"; import { useProjectStore } from "./projectStore"; -import { clearHistory, redo, undo } from "./undo"; +import { clearHistory, redo, undo, useUndoRedoShortcuts } from "./undo"; import { future, past } from "./undoStack"; const saveMock = vi.hoisted(() => vi.fn()); @@ -45,7 +46,7 @@ describe("undo/redo", () => { it("records a saveDocument edit and reverts it", async () => { // The path every region add / delete / rename takes. It recorded nothing // before this fix, so `past` stayed empty and `undo()` returned false. - await useProjectStore.getState().saveDocument(titled("Renamed")); + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); expect(currentTitle()).toBe("Renamed"); expect(past).toHaveLength(1); @@ -55,7 +56,7 @@ describe("undo/redo", () => { it("reapplies the edit on redo", async () => { const renamed = titled("Renamed"); - await useProjectStore.getState().saveDocument(renamed); + await useProjectStore.getState().saveDocument(renamed, { history: true }); expect(undo()).toBe(true); expect(currentTitle()).toBe("Original"); @@ -69,7 +70,7 @@ describe("undo/redo", () => { // after the re-entrancy guard had already been re-armed. The pre-undo document // landed on `past` and `pushHistory` cleared `future`, so one Ctrl+Z turned the // history into an A/B toggle with redo permanently gone. - await useProjectStore.getState().saveDocument(titled("Renamed")); + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); expect(past).toHaveLength(1); expect(undo()).toBe(true); @@ -83,8 +84,8 @@ describe("undo/redo", () => { it("walks back more than one level", async () => { // The microtask race silently capped this at one: the second Ctrl+Z replayed // the document the first had just pushed back on, so undo oscillated. - await useProjectStore.getState().saveDocument(titled("Second")); - await useProjectStore.getState().saveDocument(titled("Third")); + await useProjectStore.getState().saveDocument(titled("Second"), { history: true }); + await useProjectStore.getState().saveDocument(titled("Third"), { history: true }); expect(currentTitle()).toBe("Third"); expect(undo()).toBe(true); @@ -114,7 +115,7 @@ describe("undo/redo", () => { // What `NewEditorShell` does in `useUndoRedoShortcuts`'s callback. Without // `history: false` there, the persist re-records the restored document and // wipes the redo the undo just created. - await useProjectStore.getState().saveDocument(titled("Renamed")); + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); expect(undo()).toBe(true); const restored = useProjectStore.getState().document; @@ -127,18 +128,18 @@ describe("undo/redo", () => { }); it("drops the redo stack once a new edit lands on an undone document", async () => { - await useProjectStore.getState().saveDocument(titled("Renamed")); + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); expect(undo()).toBe(true); expect(future).toHaveLength(1); - await useProjectStore.getState().saveDocument(titled("Different branch")); + await useProjectStore.getState().saveDocument(titled("Different branch"), { history: true }); expect(future).toHaveLength(0); expect(redo()).toBe(false); }); it("refuses to restore a snapshot from another project", async () => { - await useProjectStore.getState().saveDocument(titled("Renamed")); + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); useProjectStore.setState({ projectId: "project_2" }); expect(undo()).toBe(false); @@ -147,7 +148,7 @@ describe("undo/redo", () => { }); it("marks the document dirty so the restore can be persisted", async () => { - await useProjectStore.getState().saveDocument(titled("Renamed")); + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); expect(useProjectStore.getState().dirty).toBe(false); const revisionBefore = useProjectStore.getState().revision; @@ -157,4 +158,126 @@ describe("undo/redo", () => { // Consumers repaint off `document`; `revision` is what the agent-apply guard reads. expect(useProjectStore.getState().revision).toBe(revisionBefore + 1); }); + + describe("a write that failed", () => { + // `saveDocument` resolves false on a handled failure -- a read-only project -- + // and every caller reads that as "the edit did not happen". Recording history + // ABOVE the await recorded it anyway, and nothing ever took it back off. + + it("records no undo step, so Ctrl+Z is not left doing nothing", async () => { + // The exact symptom #433 was filed for, re-created by the first fix for it: + // `past` held a snapshot identical to the document on screen, so the next + // Ctrl+Z restored what was already there. + saveMock.mockResolvedValueOnce({ success: false, error: "EACCES" }); + + expect( + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }), + ).toBe(false); + + expect(currentTitle()).toBe("Original"); + expect(past).toHaveLength(0); + expect(undo()).toBe(false); + }); + + it("leaves the redo stack alone", async () => { + // Worse than a wasted step: `pushHistory` clears `future` on its way past, so + // a failed write destroyed a redo the user had already earned. + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); + expect(undo()).toBe(true); + expect(future).toHaveLength(1); + + saveMock.mockResolvedValueOnce({ success: false, error: "EACCES" }); + expect( + await useProjectStore.getState().saveDocument(titled("Rejected"), { history: true }), + ).toBe(false); + + expect(future).toHaveLength(1); + expect(redo()).toBe(true); + expect(currentTitle()).toBe("Renamed"); + }); + + it("records the step once a retry lands", async () => { + // Not recording is only correct if the record still happens when the write + // eventually succeeds -- otherwise the fix trades one dead Ctrl+Z for another. + saveMock.mockResolvedValueOnce({ success: false, error: "EACCES" }); + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); + + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); + + expect(currentTitle()).toBe("Renamed"); + expect(past).toHaveLength(1); + expect(undo()).toBe(true); + expect(currentTitle()).toBe("Original"); + }); + }); + + it("records the pre-drag document a commit names, not the one on screen", async () => { + // `historyBase`. A live drag writes every pointermove into the store with + // `history: false`, so by commit time the store's own "previous" document is the + // dragged one -- recording that would leave the gesture un-undoable. + const dragged = titled("Dragged"); + const preDrag = useProjectStore.getState().document; + useProjectStore.getState().setDocument(dragged, { history: false }); + expect(past).toHaveLength(0); + + await useProjectStore.getState().saveDocument(dragged, { history: true, historyBase: preDrag }); + + expect(past).toHaveLength(1); + expect(undo()).toBe(true); + expect(currentTitle()).toBe("Original"); + }); +}); + +describe("the Edit menu's undo/redo route", () => { + // On macOS the native menu is the ONLY path Cmd+Z has to the renderer: AppKit + // matches the menu item's key equivalent before the key event reaches the web + // contents, so the keydown listener never runs. `electron/main.ts` therefore + // forwards `menu-undo` / `menu-redo` over IPC, and `NewEditorShell` wires them to + // these handlers. The Electron half is pinned in `electron/edit-menu.test.ts`. + beforeEach(() => { + useProjectStore.getState().clear(); + clearHistory(); + saveMock.mockReset(); + saveMock.mockImplementation(async (document: unknown) => ({ success: true, document })); + useProjectStore.setState({ projectId: PROJECT_ID, document: titled("Original") }); + window.document.body.innerHTML = ""; + }); + + it("undoes the document and persists the restore", async () => { + const persist = vi.fn(); + const { result } = renderHook(() => useUndoRedoShortcuts(persist)); + await useProjectStore.getState().saveDocument(titled("Renamed"), { history: true }); + + act(() => { + result.current.runUndo(); + }); + + expect(currentTitle()).toBe("Original"); + expect(persist).toHaveBeenCalledOnce(); + + act(() => { + result.current.runRedo(); + }); + expect(currentTitle()).toBe("Renamed"); + }); + + it("leaves a focused text field to the browser's own text undo", () => { + // Same rule the keydown path applies, and the one thing the `undo` role was + // still good for. + const persist = vi.fn(); + const { result } = renderHook(() => useUndoRedoShortcuts(persist)); + past.push({ projectId: PROJECT_ID, doc: titled("Older") }); + + const input = window.document.createElement("input"); + window.document.body.appendChild(input); + input.focus(); + + act(() => { + result.current.runUndo(); + }); + + expect(currentTitle()).toBe("Original"); + expect(past).toHaveLength(1); + expect(persist).not.toHaveBeenCalled(); + }); }); diff --git a/src/lib/ai-edition/store/undo.ts b/src/lib/ai-edition/store/undo.ts index 875faaaaa..3c1800f9f 100644 --- a/src/lib/ai-edition/store/undo.ts +++ b/src/lib/ai-edition/store/undo.ts @@ -11,7 +11,7 @@ // all a repaint needs — the timeline, preview and shell all subscribe to // `s.document`. -import { useEffect, useRef } from "react"; +import { useCallback, useEffect, useRef } from "react"; import { isModalOpen } from "../modalGuard"; import type { AxcutDocument } from "../schema"; import { useProjectStore } from "./projectStore"; @@ -58,13 +58,36 @@ export function redo(): boolean { return true; } -export function useUndoRedoShortcuts(onAfter: () => void) { +/** + * Whether the document undo must keep its hands off: inside a text field the + * browser's own text undo is the one the user means. The keydown path checks the + * event target, the menu path checks `activeElement` — same rule, two entry + * points, so it lives in one function. + */ +function isTextEditingTarget(node: EventTarget | null): boolean { + if (node instanceof HTMLTextAreaElement || node instanceof HTMLInputElement) return true; + return node instanceof HTMLElement && node.isContentEditable; +} + +export interface UndoRedoHandlers { + /** + * Undo, applying the same text-field rule the keyboard path applies. + * + * Wired to the native Edit menu, which on macOS is the ONLY route Cmd+Z has: + * AppKit matches the menu's key equivalent before the key event reaches the web + * contents, so the keydown listener below never runs there. See + * `electron/edit-menu.ts`. + */ + runUndo: () => void; + runRedo: () => void; +} + +export function useUndoRedoShortcuts(onAfter: () => void): UndoRedoHandlers { const onAfterRef = useRef(onAfter); onAfterRef.current = onAfter; useEffect(() => { const onKey = (e: KeyboardEvent) => { - if (e.target instanceof HTMLTextAreaElement || e.target instanceof HTMLInputElement) return; - if (e.target instanceof HTMLElement && e.target.isContentEditable) return; + if (isTextEditingTarget(e.target)) return; // `NewEditorShell` hands Ctrl+Z / Ctrl+Y to this listener instead of handling them, // so its modal guard never runs for them: without this one, undo kept rewriting the // document under every open modal, including the ones the shell does suppress. @@ -91,4 +114,26 @@ export function useUndoRedoShortcuts(onAfter: () => void) { window.addEventListener("keydown", onKey); return () => window.removeEventListener("keydown", onKey); }, []); + + // `execCommand` is the only handle the renderer has on the browser's text undo, + // and it is what the `undo` menu ROLE reached through `webContents.undo()`. It is + // deprecated and absent under jsdom, hence the optional call: losing text undo in + // a field is survivable, a TypeError from a menu click is not. + const runUndo = useCallback(() => { + if (isTextEditingTarget(window.document.activeElement)) { + window.document.execCommand?.("undo"); + return; + } + if (undo()) onAfterRef.current(); + }, []); + + const runRedo = useCallback(() => { + if (isTextEditingTarget(window.document.activeElement)) { + window.document.execCommand?.("redo"); + return; + } + if (redo()) onAfterRef.current(); + }, []); + + return { runUndo, runRedo }; } diff --git a/src/lib/ai-edition/store/useCaptions.ts b/src/lib/ai-edition/store/useCaptions.ts index dabf3f8d9..b7e05c3e6 100644 --- a/src/lib/ai-edition/store/useCaptions.ts +++ b/src/lib/ai-edition/store/useCaptions.ts @@ -65,23 +65,29 @@ export function useCaptions(): UseCaptionsResult { const doc = useProjectStore.getState().document; if (!doc) return; const next = patchCaptionSettings(doc, patch); - setDocument(next); - await saveDocument(next); + // The optimistic write is not the edit — the save is. Only the one that can + // fail records, and it names `doc` as what Ctrl+Z returns to because by then + // the store already holds `next`. + setDocument(next, { history: false }); + await saveDocument(next, { history: true, historyBase: doc }); }, [setDocument, saveDocument], ); // See `useEditorSettings.setLive`: one undo step per slider drag, not one per // pointer move. `liveDocRef` holds what this hook last wrote, so only a write - // landing on someone else's document opens a new history entry. + // landing on someone else's document opens a new history entry -- and that entry + // is `liveBaseRef`, handed to the commit rather than recorded on the spot. const liveDocRef = useRef(null); + const liveBaseRef = useRef(null); const setLive = useCallback( (patch: CaptionSettingsPatch) => { const doc = useProjectStore.getState().document; if (!doc) return; const next = patchCaptionSettings(doc, patch); - setDocument(next, { history: liveDocRef.current !== doc }); + if (liveDocRef.current !== doc) liveBaseRef.current = doc; + setDocument(next, { history: false }); liveDocRef.current = next; }, [setDocument], @@ -90,7 +96,10 @@ export function useCaptions(): UseCaptionsResult { const commit = useCallback(async () => { const doc = useProjectStore.getState().document; if (!doc) return; - await saveDocument(doc); + const base = liveBaseRef.current; + liveBaseRef.current = null; + liveDocRef.current = null; + await saveDocument(doc, { history: true, historyBase: base }); }, [saveDocument]); const saveTranslation = useCallback( @@ -98,8 +107,8 @@ export function useCaptions(): UseCaptionsResult { const doc = useProjectStore.getState().document; if (!doc) return; const next = putCaptionTranslation(doc, input); - setDocument(next); - await saveDocument(next); + setDocument(next, { history: false }); + await saveDocument(next, { history: true, historyBase: doc }); }, [setDocument, saveDocument], ); @@ -115,8 +124,8 @@ export function useCaptions(): UseCaptionsResult { getCaptionSettings(cleared).language === language ? patchCaptionSettings(cleared, { language: null }) : cleared; - setDocument(next); - await saveDocument(next); + setDocument(next, { history: false }); + await saveDocument(next, { history: true, historyBase: doc }); }, [setDocument, saveDocument], ); diff --git a/src/lib/ai-edition/store/useEditorSettings.ts b/src/lib/ai-edition/store/useEditorSettings.ts index 9af58dd79..032ec8230 100644 --- a/src/lib/ai-edition/store/useEditorSettings.ts +++ b/src/lib/ai-edition/store/useEditorSettings.ts @@ -48,8 +48,11 @@ export function useEditorSettings(): UseEditorSettingsResult { const doc = useProjectStore.getState().document; if (!doc) return; const next = patchEditorSettings(doc, patch); - setDocument(next); - await saveDocument(next); + // The optimistic write is not the edit — the save is. Only the one that can + // fail records, and it names `doc` as what Ctrl+Z returns to because by then + // the store already holds `next`. + setDocument(next, { history: false }); + await saveDocument(next, { history: true, historyBase: doc }); }, [setDocument, saveDocument], ); @@ -57,15 +60,19 @@ export function useEditorSettings(): UseEditorSettingsResult { // The document this hook's own last `setLive` produced. A slider drag fires one // `setLive` per pointer move, and recording each of them buried the real history // under sixty one-pixel steps; only the first write of a drag — the one editing a - // document this callback did not produce — is a state worth returning to. + // document this callback did not produce — is a state worth returning to. It is + // held in `liveBaseRef` and recorded by `commit`, not here: a snapshot pushed + // mid-drag is on the stack whether or not the commit that follows ever lands. const liveDocRef = useRef(null); + const liveBaseRef = useRef(null); const setLive = useCallback( (patch: EditorSettingsPatch) => { const doc = useProjectStore.getState().document; if (!doc) return; const next = patchEditorSettings(doc, patch); - setDocument(next, { history: liveDocRef.current !== doc }); + if (liveDocRef.current !== doc) liveBaseRef.current = doc; + setDocument(next, { history: false }); liveDocRef.current = next; }, [setDocument], @@ -74,7 +81,10 @@ export function useEditorSettings(): UseEditorSettingsResult { const commit = useCallback(async () => { const doc = useProjectStore.getState().document; if (!doc) return; - await saveDocument(doc); + const base = liveBaseRef.current; + liveBaseRef.current = null; + liveDocRef.current = null; + await saveDocument(doc, { history: true, historyBase: base }); }, [saveDocument]); return { settings, hasDocument, set, setLive, commit }; diff --git a/src/lib/ai-edition/store/useSequentialTimelineOps.test.ts b/src/lib/ai-edition/store/useSequentialTimelineOps.test.ts index 13b097255..a72b146bd 100644 --- a/src/lib/ai-edition/store/useSequentialTimelineOps.test.ts +++ b/src/lib/ai-edition/store/useSequentialTimelineOps.test.ts @@ -74,7 +74,7 @@ describe("useSequentialTimelineOps", () => { // Mirror the real store: write the saved doc back so the next // call in the queue sees the latest committed state, and report // success the way `projectStore.saveDocument` does. - useProjectStore.getState().setDocument(doc); + useProjectStore.getState().setDocument(doc, { history: true }); callOrder.push(doc.timeline.trimRanges[0]?.startSec.toString() ?? "empty"); return true; }); @@ -128,7 +128,7 @@ describe("useSequentialTimelineOps", () => { .fn<(doc: AxcutDocument) => Promise>() .mockResolvedValueOnce(false) .mockImplementationOnce(async (doc) => { - useProjectStore.getState().setDocument(doc); + useProjectStore.getState().setDocument(doc, { history: true }); return true; }); @@ -162,7 +162,7 @@ describe("useSequentialTimelineOps", () => { expect(secondResult).not.toBeNull(); // The queue survived the first failure — both saves were attempted. expect(saveDocument).toHaveBeenCalledTimes(2); - expect(saveDocument).toHaveBeenNthCalledWith(2, secondResult); + expect(saveDocument).toHaveBeenNthCalledWith(2, secondResult, { history: true }); }); it("runs an enqueued write after the op ahead of it has committed", async () => { @@ -170,7 +170,7 @@ describe("useSequentialTimelineOps", () => { useProjectStore.setState({ document: seed }); const saveDocument = vi.fn(async (doc: AxcutDocument) => { - useProjectStore.getState().setDocument(doc); + useProjectStore.getState().setDocument(doc, { history: true }); return true; }); @@ -203,7 +203,7 @@ describe("useSequentialTimelineOps", () => { useProjectStore.setState({ document: seed }); const saveDocument = vi.fn(async (doc: AxcutDocument) => { - useProjectStore.getState().setDocument(doc); + useProjectStore.getState().setDocument(doc, { history: true }); return true; }); @@ -261,7 +261,7 @@ describe("useSequentialTimelineOps", () => { useProjectStore.setState({ document: seed }); const saveDocument = vi.fn(async (doc: AxcutDocument) => { - useProjectStore.getState().setDocument(doc); + useProjectStore.getState().setDocument(doc, { history: true }); return true; }); diff --git a/src/lib/ai-edition/store/useSequentialTimelineOps.ts b/src/lib/ai-edition/store/useSequentialTimelineOps.ts index b77756bf4..45742754e 100644 --- a/src/lib/ai-edition/store/useSequentialTimelineOps.ts +++ b/src/lib/ai-edition/store/useSequentialTimelineOps.ts @@ -30,7 +30,7 @@ import { useCallback, useRef } from "react"; import type { AxcutTimelineOperation } from "@/lib/ai-edition/document/operations"; import type { AxcutDocument } from "@/lib/ai-edition/schema"; -import { useProjectStore } from "./projectStore"; +import { type DocumentWriteOptions, useProjectStore } from "./projectStore"; export interface SequentialTimelineOps { /** @@ -63,8 +63,12 @@ export function useSequentialTimelineOps(options: { /** Used only when the project store has no document yet. */ fallbackDocument: AxcutDocument | null; /** Persist a document, resolving false if the write failed (already reported). - * The hook awaits this before unblocking the queue. */ - saveDocument: (doc: AxcutDocument) => Promise; + * The hook awaits this before unblocking the queue. + * + * Typed with the store's own options parameter rather than a one-argument + * narrowing of it: every queued op here is a user edit, and the hook has to be + * able to say so. */ + saveDocument: (doc: AxcutDocument, opts: DocumentWriteOptions) => Promise; }): SequentialTimelineOps { const { fallbackDocument, saveDocument } = options; const saveQueueRef = useRef>(Promise.resolve()); @@ -94,7 +98,7 @@ export function useSequentialTimelineOps(options: { const doc = useProjectStore.getState().document ?? fallbackDocument; if (!doc) return null; const applied = applyTimelineOperation(doc, op); - return (await saveDocument(applied.document)) ? applied.document : null; + return (await saveDocument(applied.document, { history: true })) ? applied.document : null; }), [enqueue, fallbackDocument, saveDocument], ); diff --git a/src/lib/ai-edition/store/useTimeline.test.ts b/src/lib/ai-edition/store/useTimeline.test.ts index 939fc7c9d..08ab1c0da 100644 --- a/src/lib/ai-edition/store/useTimeline.test.ts +++ b/src/lib/ai-edition/store/useTimeline.test.ts @@ -5,6 +5,8 @@ import { I18nProvider } from "@/contexts/I18nContext"; import type { AxcutDocument } from "../schema"; import { axcutSchemaVersion } from "../schema"; import { useProjectStore } from "./projectStore"; +import { clearHistory, redo, undo } from "./undo"; +import { future, past } from "./undoStack"; import { useTimeline } from "./useTimeline"; /** @@ -898,3 +900,195 @@ describe("useTimeline save failures", () => { expect(useProjectStore.getState().document?.zoomRanges).toHaveLength(0); }); }); + +describe("useTimeline undo history", () => { + // `addAsset` (electron/ai-edition/document-service.ts) never writes `durationSec`, + // so EVERY freshly imported asset lands at the 60s placeholder and fires the + // background probe. Anything the probe records is therefore on the undo stack of + // every single drop, which is what makes these two the common case and not an + // edge one. + const unprobed = { + id: "asset_3", + kind: "video" as const, + label: "fresh-import.mp4", + originalPath: "/tmp/fresh-import.mp4", + durationSec: undefined, + // No `video` either: that is what `addAsset` produces, and it is what makes + // `probeAndCorrectClip` save even once the clip it came for is gone. + cameraTrack: null, + }; + + const docWithZoom: AxcutDocument = { + ...sampleDoc, + zoomRanges: [ + { + id: "zoom_a", + startMs: 1000, + endMs: 3000, + depth: 3, + focus: { cx: 0.5, cy: 0.5 }, + focusMode: "manual", + clipId: "clip_a", + sourceStartSec: 1, + sourceEndSec: 3, + }, + ], + }; + + beforeEach(() => { + useProjectStore.getState().clear(); + clearHistory(); + for (const mock of Object.values(bridgeMocks)) mock.mockReset(); + probeVideoDurationMock.mockReset(); + probeVideoDimensionsMock.mockResolvedValue({ width: 1920, height: 1080 }); + bridgeMocks.save.mockImplementation(async (doc: typeof sampleDoc) => ({ + success: true, + document: doc, + })); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + function seed(document: AxcutDocument) { + useProjectStore.setState({ + projectId: "proj_test", + document, + revision: 1, + status: "ready", + error: null, + }); + } + + it("keeps the background duration probe off the undo stack", async () => { + // Without `{ history: false }` on `probeAndCorrectClip`'s save, dropping a clip + // left `past` = [beforeDrop, dropWithPlaceholderClip]: the first Ctrl+Z snapped + // the clip back to a 60s placeholder instead of removing it. + seed({ ...sampleDoc, assets: [...sampleDoc.assets, unprobed] }); + probeVideoDurationMock.mockResolvedValue(5); + const { result } = renderTimeline(); + + await act(async () => { + await result.current.insertClipAt("asset_3", 1); + }); + await waitFor(() => { + const inserted = useProjectStore + .getState() + .document?.timeline.clips.find((c) => c.assetId === "asset_3"); + expect(inserted?.sourceEndSec).toBe(5); + }); + + // One step for the drop the user made, none for the probe that corrected it. + expect(past).toHaveLength(1); + act(() => { + expect(undo()).toBe(true); + }); + expect(useProjectStore.getState().document?.timeline.clips).toHaveLength(1); + }); + + it("does not let a probe landing after an undo destroy the redo", async () => { + // The probe is detached from the drop, so it can resolve at any point — including + // after the user has already pressed Ctrl+Z. A recording save there ran + // `pushHistory`, which clears `future` on its way past: redo was gone, wiped by a + // write the user never made and never saw. + seed({ ...sampleDoc, assets: [...sampleDoc.assets, unprobed] }); + let landProbe!: (durationSec: number) => void; + probeVideoDurationMock.mockReturnValue( + new Promise((resolvePromise) => { + landProbe = resolvePromise; + }), + ); + const { result } = renderTimeline(); + + await act(async () => { + await result.current.insertClipAt("asset_3", 1); + }); + expect(past).toHaveLength(1); + + act(() => { + expect(undo()).toBe(true); + }); + expect(useProjectStore.getState().document?.timeline.clips).toHaveLength(1); + expect(future).toHaveLength(1); + + await act(async () => { + landProbe(5); + // The clip is gone, so only the dimensions half of the probe still has + // anything to write — and that is exactly the write that used to be recorded. + await new Promise((resolveTick) => setTimeout(resolveTick, 0)); + }); + + expect(past).toHaveLength(0); + expect(future).toHaveLength(1); + act(() => { + expect(redo()).toBe(true); + }); + expect(useProjectStore.getState().document?.timeline.clips).toHaveLength(2); + }); + + it("leaves no undo step behind a focus drag whose commit failed", async () => { + // The drag used to push its pre-drag document from the FIRST `setDocument`. When + // the commit then failed, `commitZoomFocus` restored that same document through + // `setState` — which cannot pop the entry. `past` was left holding a snapshot + // identical to what was on screen (a Ctrl+Z that visibly does nothing) and + // `future` had already been wiped by the push. + seed(docWithZoom); + const { result } = renderTimeline(); + + // An edit and an undo, so there is a redo entry to lose. + await act(async () => { + await result.current.updateZoomDepth("zoom_a", 5); + }); + act(() => { + expect(undo()).toBe(true); + }); + expect(past).toHaveLength(0); + expect(future).toHaveLength(1); + + const beforeDrag = useProjectStore.getState().document; + bridgeMocks.save.mockResolvedValue({ success: false, error: "disk full" }); + act(() => { + result.current.updateZoomFocusLive("zoom_a", { cx: 0.2, cy: 0.3 }); + result.current.updateZoomFocusLive("zoom_a", { cx: 0.25, cy: 0.35 }); + }); + await act(async () => { + await result.current.commitZoomFocus(); + }); + + expect(useProjectStore.getState().document).toBe(beforeDrag); + expect(past).toHaveLength(0); + expect(future).toHaveLength(1); + act(() => { + expect(redo()).toBe(true); + }); + expect(useProjectStore.getState().document?.zoomRanges[0].depth).toBe(5); + }); + + it("records one undo step for a whole focus drag once it commits", async () => { + // The other half of the same change: moving the record to the commit must not + // lose it. Sixty pointermoves, one Ctrl+Z, back to where the drag started. + seed(docWithZoom); + const { result } = renderTimeline(); + + act(() => { + for (let i = 0; i < 60; i++) { + result.current.updateZoomFocusLive("zoom_a", { cx: 0.2 + i / 1000, cy: 0.3 }); + } + }); + expect(past).toHaveLength(0); + + await act(async () => { + await result.current.commitZoomFocus(); + }); + + expect(past).toHaveLength(1); + act(() => { + expect(undo()).toBe(true); + }); + expect(useProjectStore.getState().document?.zoomRanges[0].focus).toEqual({ + cx: 0.5, + cy: 0.5, + }); + }); +}); diff --git a/src/lib/ai-edition/store/useTimeline.ts b/src/lib/ai-edition/store/useTimeline.ts index c1fb293bf..859adbfe9 100644 --- a/src/lib/ai-edition/store/useTimeline.ts +++ b/src/lib/ai-edition/store/useTimeline.ts @@ -241,7 +241,7 @@ export function useTimeline() { ...document, zoomRanges: [...document.zoomRanges, ...anchored] as AxcutDocument["zoomRanges"], }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -273,7 +273,7 @@ export function useTimeline() { ...document, zoomRanges: [...document.zoomRanges, ...anchored] as AxcutDocument["zoomRanges"], }; - if (!(await saveDocument(next))) return 0; + if (!(await saveDocument(next, { history: true }))) return 0; return suggestions.length; }, [document, saveDocument], @@ -314,7 +314,7 @@ export function useTimeline() { ], }, }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -361,7 +361,7 @@ export function useTimeline() { ...created, ] as unknown as AxcutDocument["annotations"], }; - if (!(await saveDocument(next))) return; + if (!(await saveDocument(next, { history: true }))) return; // Select the freshly added annotation so its inspector opens and it shows a // selection box on the canvas, ready to be retyped over. const newId = created[0]?.id ?? ann.id; @@ -394,7 +394,7 @@ export function useTimeline() { ], }, }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -433,7 +433,7 @@ export function useTimeline() { ], }, }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -469,7 +469,7 @@ export function useTimeline() { ), }, }; - await saveDocument(nextDoc); + await saveDocument(nextDoc, { history: true }); }, [document, saveDocument], ); @@ -512,10 +512,13 @@ export function useTimeline() { origin: prev?.origin ?? ("user" as const), }; }); - await saveDocument({ - ...doc, - timeline: { ...doc.timeline, trimRanges: [...others, ...rebuilt] }, - }); + await saveDocument( + { + ...doc, + timeline: { ...doc.timeline, trimRanges: [...others, ...rebuilt] }, + }, + { history: true }, + ); }, [saveDocument], ); @@ -540,7 +543,7 @@ export function useTimeline() { () => createId("zoom"), ) as AxcutDocument["zoomRanges"], }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -555,18 +558,20 @@ export function useTimeline() { const doc = useProjectStore.getState().document; if (!doc) return; // The first live write of a drag is the one editing a document this callback - // did not itself produce — so it is the pre-drag state, and the only one worth - // recording. Without the flag a pointermove-frequency drag pushed ~60 snapshots - // a second and evicted the real history behind it. - const dragStart = zoomFocusLiveRef.current !== doc; - if (dragStart) zoomFocusRollbackRef.current = doc; + // did not itself produce, so it is the pre-drag state — the one thing worth + // returning to. It is remembered, not recorded: `commitZoomFocus` hands it to + // `saveDocument` as `historyBase`, so the whole gesture becomes ONE undo step + // and only once the write landed. Recording it here instead left the entry + // behind when the commit failed and rolled the document back to that very + // document — a Ctrl+Z that visibly did nothing, with `future` already wiped. + if (zoomFocusLiveRef.current !== doc) zoomFocusRollbackRef.current = doc; const next: AxcutDocument = { ...doc, zoomRanges: patchPillById(doc.zoomRanges, id, { focus: { cx: finiteFraction(focus.cx), cy: finiteFraction(focus.cy) }, }) as AxcutDocument["zoomRanges"], }; - setDocument(next, { history: dragStart }); + setDocument(next, { history: false }); zoomFocusLiveRef.current = next; }, [setDocument], @@ -578,7 +583,10 @@ export function useTimeline() { const rollback = zoomFocusRollbackRef.current; zoomFocusRollbackRef.current = null; zoomFocusLiveRef.current = null; - if (!(await saveDocument(doc)) && rollback) { + // `historyBase: rollback` — the pre-drag document, not the one the store holds + // (that is the dragged one, written live). `null` when no live write happened, + // which records nothing, which is right: nothing changed. + if (!(await saveDocument(doc, { history: true, historyBase: rollback })) && rollback) { useProjectStore.setState((state) => // `dirty` is deliberately NOT cleared. The rollback target is the last document // this drag started from, which is not the same as the last SAVED one: with two @@ -602,7 +610,7 @@ export function useTimeline() { depth, }) as AxcutDocument["zoomRanges"], }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -621,7 +629,7 @@ export function useTimeline() { rotationPreset, }) as AxcutDocument["zoomRanges"], }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -643,7 +651,7 @@ export function useTimeline() { focusMode, }) as AxcutDocument["zoomRanges"], }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -664,7 +672,7 @@ export function useTimeline() { () => createId("ann"), ), }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -676,15 +684,14 @@ export function useTimeline() { (id: string, patch: Partial) => { const doc = useProjectStore.getState().document; if (!doc) return; - // One undo step per drag, not per pointermove — same reasoning as - // `updateZoomFocusLive` above. - const dragStart = annotationLiveRef.current !== doc; - if (dragStart) annotationRollbackRef.current = doc; + // One undo step per drag, recorded by the commit once it lands — same + // reasoning, and the same failed-commit hole, as `updateZoomFocusLive` above. + if (annotationLiveRef.current !== doc) annotationRollbackRef.current = doc; const next: AxcutDocument = { ...doc, annotations: patchPillById(doc.annotations, id, patch), }; - setDocument(next, { history: dragStart }); + setDocument(next, { history: false }); annotationLiveRef.current = next; }, [setDocument], @@ -696,7 +703,9 @@ export function useTimeline() { const rollback = annotationRollbackRef.current; annotationRollbackRef.current = null; annotationLiveRef.current = null; - if (!(await saveDocument(doc)) && rollback) { + // See `commitZoomFocus`: the pre-drag document is the undo target, and it is + // recorded only if this write succeeds. + if (!(await saveDocument(doc, { history: true, historyBase: rollback })) && rollback) { useProjectStore.setState((state) => // `dirty` is deliberately NOT cleared. The rollback target is the last document // this drag started from, which is not the same as the last SAVED one: with two @@ -734,7 +743,7 @@ export function useTimeline() { ), }, }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -764,7 +773,7 @@ export function useTimeline() { ), }, }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -786,7 +795,7 @@ export function useTimeline() { speedRegions: patchPillById(prev, id, { speed }), }, }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [document, saveDocument], ); @@ -795,7 +804,8 @@ export function useTimeline() { async (kind: RegionKind, id: string) => { if (!document) return; // One shared mutator with the agent's removeTrim / removeModifier tools. - if (!(await saveDocument(removeRegionInDocument(document, kind, id)))) return; + if (!(await saveDocument(removeRegionInDocument(document, kind, id), { history: true }))) + return; if (selection?.id === id) setSelection(null); setMultiSelection((prev) => prev.filter((h) => h.id !== id)); }, @@ -846,7 +856,7 @@ export function useTimeline() { ? { ...legacy, speedRegions: prevSpeed, cameraFullscreenRegions: prevCameraFullscreen } : document.legacyEditor, }; - if (!(await saveDocument(next))) return; + if (!(await saveDocument(next, { history: true }))) return; setSelection(null); setMultiSelection([]); }, @@ -931,7 +941,7 @@ export function useTimeline() { ), }, }; - await saveDocument(next); + await saveDocument(next, { history: true }); }, [saveDocument], ); @@ -996,11 +1006,20 @@ export function useTimeline() { ...(needsDims ? { video: { codec: "unknown", fps: 0, ...a.video, ...probedDims } } : {}), }; }); - await state.saveDocument({ - ...doc, - assets: nextAssets, - timeline: { ...doc.timeline, clips: nextClips }, - }); + // `history: false`. Nothing about this write is a user action: `addAsset` never + // populates `durationSec`, so EVERY freshly imported asset lands at the 60s + // placeholder and fires this probe. Recording it put a placeholder-length clip + // on the undo stack a beat after the drop, so the first Ctrl+Z snapped the clip + // back to 60s instead of removing it — and a probe resolving after the user + // had already undone wiped `future`, destroying redo from a background write. + await state.saveDocument( + { + ...doc, + assets: nextAssets, + timeline: { ...doc.timeline, clips: nextClips }, + }, + { history: false }, + ); }, [], ); @@ -1047,7 +1066,7 @@ export function useTimeline() { timeline: { ...currentDoc.timeline, clips: newClips }, }; const finalDoc = rederiveRegionMs(next, newClips); - if (!(await saveDocument(finalDoc))) return; + if (!(await saveDocument(finalDoc, { history: true }))) return; setClipSelection(newClip.id); // If we used the placeholder, kick off the probe in the background. @@ -1076,7 +1095,7 @@ export function useTimeline() { async (clipId: string, toIndex: number) => { if (!document) return; if (!document.timeline.clips.some((c) => c.id === clipId)) return; - await saveDocument(moveClipInDocument(document, clipId, toIndex)); + await saveDocument(moveClipInDocument(document, clipId, toIndex), { history: true }); }, [document, saveDocument], ); @@ -1092,7 +1111,7 @@ export function useTimeline() { // original, so its index in the result is the original's index + 1. const insertedIndex = document.timeline.clips.findIndex((c) => c.id === clipId) + 1; const next = duplicateClipInDocument(document, clipId, "user", "Duplicated clip"); - if (!(await saveDocument(next))) return; + if (!(await saveDocument(next, { history: true }))) return; setClipSelection(next.timeline.clips[insertedIndex]?.id ?? null); }, [document, saveDocument], @@ -1102,7 +1121,7 @@ export function useTimeline() { async (clipId: string) => { if (!document) return; // One shared mutator with the agent's removeClip tool: reflow survivors + rederive pills. - if (!(await saveDocument(removeClipInDocument(document, clipId)))) return; + if (!(await saveDocument(removeClipInDocument(document, clipId), { history: true }))) return; if (clipSelection === clipId) setClipSelection(null); }, [document, clipSelection, saveDocument], From 643c1cfb4dc02d107d543804ed651980dbe7d7ad Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Fri, 21 Aug 2026 16:50:41 +0200 Subject: [PATCH 03/13] fix(editor): make every write name its own history decision, wrapper or not The auto-import emptied a brand-new project on the first Ctrl+Z. Round 2 made `DocumentWriteOptions.history` required so no call site could record an undo step by saying nothing. It only reaches DIRECT call sites. `projectStore.replaceTimeline(intervals, reason)` hardcoded `{ history: true }` inside itself, where the option is invisible to the signature its callers see -- so no compile error could reach the one caller there is: the unattended recording import the editor runs ON MOUNT. createProject cleared history, addAsset recorded nothing, and the timeline seed pushed the ZERO-CLIP document onto `past`. The user landed in a project they had not edited with `past.length === 1`, their first Ctrl+Z emptied the timeline, and the persist that follows an undo wrote that empty timeline to disk. `replaceTimeline` and `useSequentialTimelineOps.apply` both take the option now; `restoreFullTimeline` had the same hardcode and no callers, so it goes. Closing the class, not the instance. `pushHistory` has exactly one caller, `recordHistory`, which has exactly two, `saveDocument` and `setDocument` -- so their call sites ARE the whole surface, and `documentWriteAudit.test.ts` walks the `src` AST and pins all 62 of them against a table that says, per row, what triggers the write. A new write, a moved one, a changed `history`, a wrapper that stops forwarding, or a direct `pushHistory` fails it with a diff. That is the honest limit: the compiler can force a call site to decide, but "did the user ask for this?" is a judgement, and a judgement can only be written down where a reviewer reads it. `forwarded` is checked structurally at least -- the identifier passed on must be a PARAMETER of the enclosing function, so a local `const opts = { history: true }` does not pass as forwarding. A save already in flight when Ctrl+Z is pressed no longer lands on top of the undo. `saveDocument` records BELOW its await, which is what makes a failed write record nothing -- and what let a stale save install its document over the restored one and push a state FORWARD of the one the user returned to, clearing `future` on the way past: the undo reverted itself and redo was gone. `undo`, `redo` and `clearHistory` now bump a write epoch that `saveDocument` reads either side of its await, and a write whose epoch moved is dropped -- store and history both. The undo wins because it is the more recent instruction; reverting the disk is not that write's job, and the undo's own persist is already queued behind it. `agentDocumentApply` needed the matching guard: `false` means "superseded" as well as "failed" now, and its rollback would have put the pre-agent document over the one the user had just asked to return to. `runUndo` / `runRedo` -- the Edit-menu route this branch added, and the ONLY route Cmd+Z has on macOS -- check `isModalOpen()` after the text-field check, so a rename dialog's input keeps the browser's text undo while a modal's buttons stop undo rewriting the document underneath it. `modalGuard.ts` is byte-identical to the one on claude/fix-434-modal-shortcut-guard, which owns the same guard on the keydown path, so the merge is a union rather than a conflict. Tests, each verified to fail against the change it covers: - recordingImport: the whole hand-off against the real store -- record, import, the duration probe an editor mount fires, Ctrl+Z. Reverted, the clip count goes 1 -> 0 and `past` holds an entry the user never earned. - undo: a held-open save, an undo underneath it, and the save released. Reverted, it returns true, puts "Second" back on `past` (forward of "Original"), wipes `future` and leaves `dirty` false. Plus an `aria-modal="true"` node and a `runUndo` that must not move the document -- reverted, the title reads "Older". (The existing menu-route describe empties `document.body` in `beforeEach`, which is why the tests either side of it pass without a guard.) - agentDocumentApply: an undo overtaking the agent's save. Reverted, it returns "applied"; with the epoch guard but no rollback guard, the user's Ctrl+Z is undone for them. - useSequentialTimelineOps: `apply` forwards `{ history: false }` instead of picking `true`. - documentWriteAudit: verified against all three shapes it exists to catch -- the wrapper hardcode, an undeclared new write, and a `pushHistory` bypass. Fixes #433 --- src/components/ai-edition/NewEditorShell.tsx | 32 +- .../ai-edition/recordingImport.test.ts | 104 ++++- src/components/ai-edition/recordingImport.ts | 9 +- .../store/agentDocumentApply.test.ts | 37 ++ .../ai-edition/store/agentDocumentApply.ts | 9 +- .../store/documentWriteAudit.test.ts | 361 ++++++++++++++++++ src/lib/ai-edition/store/projectStore.ts | 64 +++- src/lib/ai-edition/store/undo.test.ts | 114 ++++++ src/lib/ai-edition/store/undo.ts | 17 +- src/lib/ai-edition/store/undoStack.ts | 30 ++ .../store/useSequentialTimelineOps.test.ts | 60 ++- .../store/useSequentialTimelineOps.ts | 18 +- 12 files changed, 796 insertions(+), 59 deletions(-) create mode 100644 src/lib/ai-edition/store/documentWriteAudit.test.ts diff --git a/src/components/ai-edition/NewEditorShell.tsx b/src/components/ai-edition/NewEditorShell.tsx index fd8607cd4..64987dea9 100644 --- a/src/components/ai-edition/NewEditorShell.tsx +++ b/src/components/ai-edition/NewEditorShell.tsx @@ -575,25 +575,31 @@ export function NewEditorShell() { (target: TrimTarget, startSec: number, endSec: number, reason: string) => { // `clipId` is what keeps the cut on the block the user typed in: with two clips // over the same media, an asset-only trim showed up on both (see `trimAppliesToClip`). - void applyTimelineOp({ - type: "add_trim_range", - assetId: target.assetId, - clipId: target.clipId, - startSec, - endSec, - reason, - }); + void applyTimelineOp( + { + type: "add_trim_range", + assetId: target.assetId, + clipId: target.clipId, + startSec, + endSec, + reason, + }, + { history: true }, + ); }, [applyTimelineOp], ); const handleRemoveTrimRange = useCallback( (trimId: string) => { - void applyTimelineOp({ - type: "remove_trim_range", - trimId, - reason: "Restored from transcript pane.", - }); + void applyTimelineOp( + { + type: "remove_trim_range", + trimId, + reason: "Restored from transcript pane.", + }, + { history: true }, + ); }, [applyTimelineOp], ); diff --git a/src/components/ai-edition/recordingImport.test.ts b/src/components/ai-edition/recordingImport.test.ts index 2c9728d44..a5810e342 100644 --- a/src/components/ai-edition/recordingImport.test.ts +++ b/src/components/ai-edition/recordingImport.test.ts @@ -1,16 +1,34 @@ // @vitest-environment jsdom import { beforeEach, describe, expect, it, vi } from "vitest"; +import { replaceTimeline as replaceTimelineOp } from "@/lib/ai-edition/document/timeline"; +import { type AxcutDocument, createEmptyDocument } from "@/lib/ai-edition/schema"; import { useProjectStore } from "@/lib/ai-edition/store/projectStore"; +import { undo } from "@/lib/ai-edition/store/undo"; +import { past } from "@/lib/ai-edition/store/undoStack"; import { importPendingRecording } from "./recordingImport"; -// The store's own bridge calls are never reached — every action the import uses -// is stubbed below — but importing the store pulls the client in, so stub it. -vi.mock("@/native/client", () => ({ nativeBridgeClient: { aiEdition: {} } })); +// The first describe stubs the store actions, so the bridge is never reached +// there. The second one runs the REAL store against these, which is the only way +// to see what the import leaves on the undo stack. +const bridge = vi.hoisted(() => ({ + create: vi.fn(), + addAsset: vi.fn(), + save: vi.fn(), +})); +vi.mock("@/native/client", () => ({ nativeBridgeClient: { aiEdition: bridge } })); const createProject = vi.fn(async () => undefined); const addAsset = vi.fn(async () => null); const replaceTimeline = vi.fn(async () => undefined); +// Read before anything stubs them: the first describe replaces these actions on the +// live store, and `clear()` resets the DATA, not the actions. +const realActions = { + createProject: useProjectStore.getState().createProject, + addAsset: useProjectStore.getState().addAsset, + replaceTimeline: useProjectStore.getState().replaceTimeline, +}; + /** Stands in for the main-process recording slot: one value, set and read. */ function stubElectronApi(screenVideoPath: string | null) { let session = screenVideoPath ? { screenVideoPath, createdAt: 0 } : null; @@ -86,6 +104,86 @@ describe("importPendingRecording", () => { expect(replaceTimeline).toHaveBeenCalledWith( [{ startSec: 0, endSec: 60 }], "Auto-imported recording", + { history: false }, ); }); }); + +// The whole hand-off, against the real store: stop the recording, land in the +// editor, press Ctrl+Z. +// +// The user has made no edit at this point -- the editor built this project for +// them, unattended, on mount. The seed below used to record itself as an undo +// step because `projectStore.replaceTimeline` hardcoded `{ history: true }` inside +// itself, where the option was invisible to its caller. So a brand-new project +// opened with `past.length === 1`, the first Ctrl+Z restored the state before the +// seed -- an empty timeline -- and `NewEditorShell`'s post-undo persist wrote that +// empty timeline to disk. +describe("what the recording import leaves on the undo stack", () => { + const PROJECT_ID = "project_imported"; + const SCREEN_PATH = "/recordings/recording-1.webm"; + + /** The document the main process actually returns from `addAsset`: an asset with + * no `durationSec` (it stats the file, it does not probe it). */ + function withAsset(): AxcutDocument { + const doc = createEmptyDocument({ projectId: PROJECT_ID, title: "Recording" }); + return { + ...doc, + assets: [ + { + id: "asset_1", + kind: "video", + label: "recording-1.webm", + originalPath: SCREEN_PATH, + cameraTrack: null, + }, + ], + project: { ...doc.project, primaryAssetId: "asset_1" }, + }; + } + + beforeEach(() => { + vi.clearAllMocks(); + useProjectStore.getState().clear(); + useProjectStore.setState(realActions); + past.length = 0; + bridge.create.mockImplementation(async () => ({ + success: true, + document: createEmptyDocument({ projectId: PROJECT_ID, title: "Recording" }), + })); + bridge.addAsset.mockImplementation(async () => ({ success: true, document: withAsset() })); + bridge.save.mockImplementation(async (document: unknown) => ({ success: true, document })); + stubElectronApi(SCREEN_PATH); + }); + + it("leaves it empty: the user has not edited anything yet", async () => { + await importPendingRecording(); + + expect(past).toHaveLength(0); + expect(undo()).toBe(false); + }); + + it("still has its clip after the first Ctrl+Z", async () => { + await importPendingRecording(); + + // The `