Skip to content

Commit f2ef034

Browse files
fix(studio): one Cmd+Z pressed inside the preview takes one undo step, not three (#4824)
* fix(studio): one Cmd+Z pressed inside the preview takes one undo step, not three * fix(studio): one undo per key press, tracked, with the preview's extra history listener removed
1 parent af314cb commit f2ef034

3 files changed

Lines changed: 83 additions & 54 deletions

File tree

‎packages/studio/src/hooks/appHotkeysDispatch.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,7 @@ import { STUDIO_PLAIN_KEYS } from "../player/components/studioShortcuts";
1414
// following useTimelineDeleteOps's precedent. Pure functions, no hooks — the
1515
// hook still owns the actual keydown listeners and calls into these.
1616

17-
/** Exported so useAppHotkeys's own history-only preview listener can reuse
18-
* the same undo/redo key arbitration without duplicating it. */
19-
export function handleUndoRedoKey(
20-
event: KeyboardEvent,
21-
onUndo: () => void,
22-
onRedo: () => void,
23-
): boolean {
17+
function handleUndoRedoKey(event: KeyboardEvent, onUndo: () => void, onRedo: () => void): boolean {
2418
const key = event.key.toLowerCase();
2519
if (key === "z" && !event.shiftKey) {
2620
event.preventDefault();

‎packages/studio/src/hooks/useAppHotkeys.previewForwarding.test.tsx‎

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,15 @@ import type { DomEditSelection } from "../components/editor/domEditing";
77
import { usePlayerStore } from "../player/store/playerStore";
88
import { useAppHotkeys } from "./useAppHotkeys";
99
import { mountReactHarness } from "./domSelectionTestHarness";
10+
import { trackStudioEvent } from "../utils/studioTelemetry";
11+
12+
vi.mock("../utils/studioTelemetry", () => ({ trackStudioEvent: vi.fn() }));
1013

1114
(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
1215

1316
const domDelete = vi.fn(async () => undefined);
17+
const historyUndo = vi.fn(async () => ({ ok: false }));
18+
const historyRedo = vi.fn(async () => ({ ok: false }));
1419
let root: Root | null = null;
1520
let sync: ((iframe: HTMLIFrameElement | null) => void) | null = null;
1621

@@ -35,8 +40,8 @@ function Harness() {
3540
domEditSelectionRef: selectionRef,
3641
clearDomSelectionRef: useRef<() => void>(() => undefined),
3742
editHistory: {
38-
undo: vi.fn(async () => ({ ok: false })),
39-
redo: vi.fn(async () => ({ ok: false })),
43+
undo: historyUndo,
44+
redo: historyRedo,
4045
state: { undo: [], redo: [] },
4146
},
4247
readOptionalProjectFile: vi.fn(async () => ""),
@@ -62,6 +67,9 @@ afterEach(() => {
6267
document.body.innerHTML = "";
6368
usePlayerStore.getState().reset();
6469
domDelete.mockClear();
70+
historyUndo.mockClear();
71+
historyRedo.mockClear();
72+
vi.mocked(trackStudioEvent).mockClear();
6573
});
6674

6775
describe("preview iframe hotkey forwarding", () => {
@@ -87,4 +95,61 @@ describe("preview iframe hotkey forwarding", () => {
8795

8896
expect(domDelete).toHaveBeenCalledTimes(1);
8997
});
98+
99+
function mountWithPreview(): Window & typeof globalThis {
100+
root = mountReactHarness(<Harness />);
101+
const iframe = document.createElement("iframe");
102+
document.body.append(iframe);
103+
act(() => sync?.(iframe));
104+
const inner = iframe.contentWindow as (Window & typeof globalThis) | null;
105+
if (!inner) throw new Error("expected an iframe window");
106+
return inner;
107+
}
108+
109+
async function press(inner: Window & typeof globalThis, init: KeyboardEventInit) {
110+
await act(async () => {
111+
inner.document.body.dispatchEvent(
112+
new inner.KeyboardEvent("keydown", { ...init, bubbles: true }),
113+
);
114+
});
115+
}
116+
117+
it("takes one tracked undo step per Cmd+Z pressed inside the preview", async () => {
118+
await press(mountWithPreview(), { key: "z", ctrlKey: true });
119+
120+
expect(historyUndo).toHaveBeenCalledTimes(1);
121+
expect(trackStudioEvent).toHaveBeenCalledTimes(1);
122+
expect(trackStudioEvent).toHaveBeenCalledWith("keyboard_shortcut", { action: "undo" });
123+
});
124+
125+
it.each([
126+
["Shift+Cmd+Z", { key: "Z", metaKey: true, shiftKey: true }],
127+
["Ctrl+Y", { key: "y", ctrlKey: true }],
128+
])("takes one tracked redo step per %s pressed inside the preview", async (_, init) => {
129+
await press(mountWithPreview(), init);
130+
131+
expect(historyRedo).toHaveBeenCalledTimes(1);
132+
expect(trackStudioEvent).toHaveBeenCalledTimes(1);
133+
expect(trackStudioEvent).toHaveBeenCalledWith("keyboard_shortcut", { action: "redo" });
134+
});
135+
136+
it("stops handling preview keys once the app unmounts", async () => {
137+
root = mountReactHarness(<Harness />);
138+
const iframe = document.createElement("iframe");
139+
document.body.append(iframe);
140+
const inner = iframe.contentWindow as (Window & typeof globalThis) | null;
141+
if (!inner) throw new Error("expected an iframe window");
142+
const add = vi.spyOn(inner, "addEventListener");
143+
const remove = vi.spyOn(inner, "removeEventListener");
144+
act(() => sync?.(iframe));
145+
act(() => root?.unmount());
146+
root = null;
147+
148+
await press(inner, { key: "z", ctrlKey: true });
149+
150+
expect(historyUndo).not.toHaveBeenCalled();
151+
// happy-dom removes a listener whatever its capture flag; a browser does not, so pin the flag.
152+
expect(add).toHaveBeenCalled();
153+
expect(remove.mock.calls).toEqual(add.mock.calls);
154+
});
90155
});

‎packages/studio/src/hooks/useAppHotkeys.ts‎

Lines changed: 15 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,7 @@ import {
88
applyCaptionModelToIframe,
99
isCaptionPreviewVisible,
1010
} from "../captions/components/CaptionOverlayUtils";
11-
import { shouldIgnoreHistoryShortcut } from "../utils/studioHelpers";
12-
import {
13-
type HotkeyCallbacks,
14-
dispatchModifierKey,
15-
dispatchPlainKey,
16-
handleUndoRedoKey,
17-
} from "./appHotkeysDispatch";
11+
import { type HotkeyCallbacks, dispatchModifierKey, dispatchPlainKey } from "./appHotkeysDispatch";
1812
import {
1913
useEditHistoryActions,
2014
type EditHistoryHandle,
@@ -36,9 +30,14 @@ function safeAddListener(t: EventTarget | null, type: string, h: EventListener,
3630
/* cross-origin */
3731
}
3832
}
39-
function safeRemoveListener(t: EventTarget | null, type: string, h: EventListener) {
33+
function safeRemoveListener(
34+
t: EventTarget | null,
35+
type: string,
36+
h: EventListener,
37+
capture = false,
38+
) {
4039
try {
41-
t?.removeEventListener(type, h);
40+
t?.removeEventListener(type, h, capture);
4241
} catch {
4342
/* cross-origin */
4443
}
@@ -232,52 +231,23 @@ export function useAppHotkeys({
232231

233232
// ── Preview iframe forwarding ──
234233

235-
const handleHistoryHotkey = useCallback((event: KeyboardEvent) => {
236-
if (!(event.metaKey || event.ctrlKey) || shouldIgnoreHistoryShortcut(event.target)) return;
237-
handleUndoRedoKey(
238-
event,
239-
() => void cbRef.current.handleUndo(),
240-
() => void cbRef.current.handleRedo(),
241-
);
242-
}, []);
243-
244234
/**
245-
* Give the preview iframe the app's hotkeys, because a keypress lands in
246-
* whichever document has focus and clicking the canvas puts focus in there.
247-
*
248-
* Must run on every iframe LOAD, not once when the element mounts: a reload
249-
* keeps the same element (so no ref callback) and the same WindowProxy (so an
250-
* identity check sees no change) while replacing the inner window that holds
251-
* the listeners. Attaching once left Delete dead in the canvas after the first
252-
* reload — press it with a selection and nothing happened, no toast, nothing
253-
* to explain it — while undo/redo kept working because they re-attached here.
235+
* Give the preview iframe the app's hotkeys: clicking the canvas puts focus in there.
236+
* Runs on every iframe LOAD: a reload keeps the element and WindowProxy but replaces the
237+
* inner window holding the listeners, which once left Delete dead after the first reload.
254238
*/
255239
const syncPreviewHotkeys = useCallback(
256240
(iframe: HTMLIFrameElement | null) => {
257241
previewHistoryCleanupRef.current?.();
258242
previewHistoryCleanupRef.current = null;
259243
const win = iframeContentWindow(iframe);
260-
let doc: Document | null = null;
261-
try {
262-
doc = iframe?.contentDocument ?? null;
263-
} catch {
264-
doc = null;
265-
}
266-
if (!win && !doc) return;
267-
const handler = handleHistoryHotkey as EventListener;
244+
if (!win) return;
268245
const appHandler = handleAppKeyDown as EventListener;
269-
safeAddListener(win, "keydown", handler, true);
270-
// Window only: the history pair also listens on the document, and a
271-
// capture listener on both would run the app handler twice per press.
246+
// Window only: a capture listener on the document too would run it twice per press.
272247
safeAddListener(win, "keydown", appHandler, true);
273-
doc?.addEventListener("keydown", handleHistoryHotkey, true);
274-
previewHistoryCleanupRef.current = () => {
275-
safeRemoveListener(win, "keydown", handler);
276-
safeRemoveListener(win, "keydown", appHandler);
277-
doc?.removeEventListener("keydown", handleHistoryHotkey, true);
278-
};
248+
previewHistoryCleanupRef.current = () => safeRemoveListener(win, "keydown", appHandler, true);
279249
},
280-
[handleAppKeyDown, handleHistoryHotkey],
250+
[handleAppKeyDown],
281251
);
282252

283253
useEffect(

0 commit comments

Comments
 (0)