diff --git a/src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx b/src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx index c0f3bdad7..7c4201ead 100644 --- a/src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx +++ b/src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx @@ -5,8 +5,11 @@ // run underneath the backdrop — Delete destroying the selected region, Ctrl+O stacking a second // aria-modal dialog, `?` stacking the shortcuts dialog on top of the one already there. // -// The guard reads `isDialogOpen()` (EditorDialogsContext, answered from a ref) and -// `isConfigOpen`. Both are asserted through the shell's real keydown handler here. +// The guard asks `isModalOpen()` (lib/ai-edition/modalGuard) — one question about the screen, +// not one flag per dialog. The flag version named the two dialogs whose open state lived in a +// context and missed every modal the shell owns as plain `useState`, so Z/T/C kept adding +// regions under the Export modal (issue #434). The modal opened below is one of those: its +// state is a `useState` in the shell, exactly like Export's. import "@testing-library/jest-dom"; import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; @@ -46,20 +49,12 @@ vi.mock("@/contexts/I18nContext", () => ({ useScopedT: () => (key: string) => key, })); -import { EditorDialogsProvider, useEditorDialogActions } from "@/contexts/EditorDialogsContext"; +import { EditorDialogsProvider } from "@/contexts/EditorDialogsContext"; import { NewEditorShell } from "./NewEditorShell"; -let dialogActions: ReturnType | null = null; - -function CaptureDialogActions() { - dialogActions = useEditorDialogActions(); - return null; -} - function renderShell() { return render( - , ); @@ -72,7 +67,6 @@ function pressOnBody(init: KeyboardEventInit) { beforeEach(() => { openConfig.mockClear(); - dialogActions = null; // No preload in jsdom, and no scrolling either; the chat transcript pins itself to the // bottom on every render. (window as unknown as { electronAPI?: unknown }).electronAPI = { @@ -122,6 +116,18 @@ afterEach(() => { (window as unknown as { electronAPI?: unknown }).electronAPI = undefined; }); +/** + * Ctrl+O is handled before the `hasProject` gate, so it opens the project picker whatever the + * editor's state — the cheapest way to put a real, shell-owned modal on screen. Its handler is + * async (it awaits the unsaved-changes prompt before opening the picker), hence the async act: + * a synchronous assertion would pass whether the guard is there or not. + */ +async function openShellModal() { + await act(async () => { + pressOnBody({ key: "o", ctrlKey: true }); + }); +} + describe("NewEditorShell shortcuts, with a dialog over the editor", () => { it("routes ? to the shortcuts dialog while nothing is open", () => { renderShell(); @@ -131,49 +137,46 @@ describe("NewEditorShell shortcuts, with a dialog over the editor", () => { expect(openConfig).toHaveBeenCalledTimes(1); }); - it("suppresses ? once a dialog owns the screen, and resumes when it closes", () => { + it("opens the project picker on Ctrl+O while nothing is open", async () => { renderShell(); - act(() => { - dialogActions?.openDialog("providers"); - }); - pressOnBody({ key: "?" }); - expect(openConfig).not.toHaveBeenCalled(); + await openShellModal(); - act(() => { - dialogActions?.closeDialog(); - }); - pressOnBody({ key: "?" }); - expect(openConfig).toHaveBeenCalledTimes(1); + // The provider dialog is mounted in App.tsx, not here, so the shell renders no dialog of + // its own unless Ctrl+O got through. + expect(screen.getByRole("dialog")).toBeInTheDocument(); }); - // Ctrl+O is handled before the `hasProject` gate, so it fired whatever the editor's state — - // this is the one that put a SECOND aria-modal dialog on screen, both of them emitting the - // hardcoded `id="modal-title"`. Its handler is async (it awaits the unsaved-changes prompt - // before opening the picker), hence the async act: a synchronous assertion here would pass - // whether the guard is there or not. - it("opens the project picker on Ctrl+O while nothing is open", async () => { + // The #434 shape: a modal the shell owns as local state, which no context knows about. `?` + // is the observable because it is the one shortcut that survives the `hasProject` gate — + // the keys the issue reports (Z, T, C) sit further down the same handler, behind the same + // single `return`. + it("suppresses ? while a modal the shell itself owns is open, and resumes when it closes", async () => { renderShell(); - await act(async () => { - pressOnBody({ key: "o", ctrlKey: true }); - }); + await openShellModal(); + pressOnBody({ key: "?" }); + expect(openConfig).not.toHaveBeenCalled(); - // The provider dialog is mounted in App.tsx, not here, so the shell renders no dialog of - // its own unless Ctrl+O got through. - expect(screen.getByRole("dialog")).toBeInTheDocument(); + // ModalShell listens for Escape on `document`, so this closes the picker for real + // rather than reaching into the shell's state. + fireEvent.keyDown(document, { key: "Escape" }); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + + pressOnBody({ key: "?" }); + expect(openConfig).toHaveBeenCalledTimes(1); }); - it("suppresses Ctrl+O once a dialog owns the screen", async () => { + // The other half of the same bug: a shortcut that opens a dialog stacked a SECOND one on + // screen under the first, both of them emitting the hardcoded `id="modal-title"`. + it("does not stack a second dialog when Ctrl+N fires under an open modal", async () => { renderShell(); - act(() => { - dialogActions?.openDialog("providers"); - }); + await openShellModal(); await act(async () => { - pressOnBody({ key: "o", ctrlKey: true }); + pressOnBody({ key: "n", ctrlKey: true }); }); - expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + expect(screen.getAllByRole("dialog")).toHaveLength(1); }); }); diff --git a/src/components/ai-edition/NewEditorShell.tsx b/src/components/ai-edition/NewEditorShell.tsx index 3d6978309..8c9ad63b5 100644 --- a/src/components/ai-edition/NewEditorShell.tsx +++ b/src/components/ai-edition/NewEditorShell.tsx @@ -13,6 +13,7 @@ import { applyProbedDuration, replaceTimeline as replaceTimelineOp, } from "@/lib/ai-edition/document/timeline"; +import { isModalOpen } from "@/lib/ai-edition/modalGuard"; import { type AxcutClip, documentSchema } from "@/lib/ai-edition/schema"; import { useProjectStore } from "@/lib/ai-edition/store/projectStore"; import { @@ -130,12 +131,12 @@ export function NewEditorShell() { action: "close" | "new" | "open" | "record"; resolve: (choice: UnsavedChoice) => void; } | null>(null); - const { shortcuts, isMac, isConfigOpen, openConfig: openShortcutsConfig } = useShortcuts(); + const { shortcuts, isMac, openConfig: openShortcutsConfig } = useShortcuts(); // The actions half of the dialog context, not the section: this component only ever *opens* // one, and subscribing it to the open state would re-render the whole editor — timeline, - // preview, transport — twice per dialog interaction. `isDialogOpen` answers the keyboard - // handler below from a ref, which is why it can live in a value that never changes. - const { openDialog, isDialogOpen } = useEditorDialogActions(); + // preview, transport — twice per dialog interaction. Whether a dialog is open is a question + // for `isModalOpen`, which answers for every modal rather than for this context's one. + const { openDialog } = useEditorDialogActions(); // Transcription is local and every transcript-driven feature (Smart cuts, // captions, the transcript pane) needs one, so the editor produces them by // itself instead of waiting for the user to find the button. This hook is @@ -861,9 +862,10 @@ export function NewEditorShell() { // A modal owns the screen. Its own controls are buttons, not text fields, so the two // guards above let every editor shortcut through underneath it: Delete destroyed the // selected region behind the backdrop, Ctrl+O stacked a second `aria-modal` dialog on - // top, and `?` stacked the shortcuts dialog. Both flags are reachable now that the - // open state is lifted out of the components that used to own it (#420). - if (isDialogOpen() || isConfigOpen) return; + // top, and `?` stacked the shortcuts dialog. One question about the screen, not one + // flag per dialog — the flag version knew only about the two dialogs whose open state + // happened to live in a context, so Z/T/C kept adding regions under Export (#434). + if (isModalOpen()) return; const ctrl = e.ctrlKey || e.metaKey; if (ctrl && e.key === "s") { e.preventDefault(); @@ -1049,8 +1051,6 @@ export function NewEditorShell() { saveDocument, copiedClipId, openShortcutsConfig, - isConfigOpen, - isDialogOpen, shortcuts, isMac, togglePlay, diff --git a/src/components/ui/dialog.tsx b/src/components/ui/dialog.tsx index 319cd8ee6..c80cea306 100644 --- a/src/components/ui/dialog.tsx +++ b/src/components/ui/dialog.tsx @@ -35,6 +35,11 @@ const DialogContent = React.forwardRef< void; closeDialog: () => void; - /** A live answer without a subscription — for event handlers, never for rendering. */ - isDialogOpen: () => boolean; } // `undefined` is the "no provider above me" marker, so that `null` stays free to mean the real @@ -49,24 +50,11 @@ export function useEditorDialogActions(): EditorDialogsActions { export function EditorDialogsProvider({ children }: { children: ReactNode }) { const [section, setSection] = useState(null); - // Mirrored so `isDialogOpen` can read the current section without the actions value having - // to depend on it. Written by the two openers, not during render and not from an effect: - // during render a discarded one would leave the ref claiming a dialog that never committed, - // and from an effect a keystroke landing between the click and the commit would still get - // the previous answer. `setSection` is called from nowhere else, so the two cannot drift. - const sectionRef = useRef(null); const actions = useMemo( () => ({ - openDialog: (next) => { - sectionRef.current = next; - setSection(next); - }, - closeDialog: () => { - sectionRef.current = null; - setSection(null); - }, - isDialogOpen: () => sectionRef.current !== null, + openDialog: (next) => setSection(next), + closeDialog: () => setSection(null), }), [], ); diff --git a/src/lib/ai-edition/modalGuard.test.tsx b/src/lib/ai-edition/modalGuard.test.tsx new file mode 100644 index 000000000..ddf28450d --- /dev/null +++ b/src/lib/ai-edition/modalGuard.test.tsx @@ -0,0 +1,61 @@ +// @vitest-environment jsdom +// `isModalOpen` is only as good as the attribute it looks for, and that attribute lives in two +// components rather than in this module. So the contract is asserted against the real ones: +// `ModalShell` (every modal in the ai-edition tree — Export, Open project, New project, Edit +// clip, the unsaved-changes prompt, the AI providers dialog) and `ui/dialog`'s Radix content +// (the shortcuts dialog and the drop-error dialog). Radix 1.1.15 emits `role="dialog"` and no +// `aria-modal` of its own, which is why `dialog.tsx` states it. + +import "@testing-library/jest-dom"; +import { cleanup, render } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@/contexts/I18nContext", () => ({ + useScopedT: () => (key: string) => key, +})); + +import { ModalShell } from "@/components/ai-edition/Modals"; +import { Dialog, DialogContent } from "@/components/ui/dialog"; +import { isModalOpen } from "./modalGuard"; + +afterEach(cleanup); + +const noop = () => { + /* nothing to close in these tests */ +}; + +describe("isModalOpen", () => { + it("is false with nothing on screen", () => { + render(
the editor
); + + expect(isModalOpen()).toBe(false); + }); + + it("is true while a ModalShell is open, false once it closes", () => { + const { rerender } = render( + + + , + ); + expect(isModalOpen()).toBe(true); + + rerender( + + + , + ); + expect(isModalOpen()).toBe(false); + }); + + it("is true while a ui/dialog content is open", () => { + render( + + + + + , + ); + + expect(isModalOpen()).toBe(true); + }); +}); diff --git a/src/lib/ai-edition/modalGuard.ts b/src/lib/ai-edition/modalGuard.ts new file mode 100644 index 000000000..6ac2f2fad --- /dev/null +++ b/src/lib/ai-edition/modalGuard.ts @@ -0,0 +1,24 @@ +// One central answer to "is a modal on screen?", for the window-level keydown handlers that +// own the editor's shortcuts (`NewEditorShell`) and its undo/redo (`store/undo`). +// +// It asks the DOM instead of enumerating open-state flags. The flag version covered exactly +// the two dialogs whose open state had been lifted into a context for unrelated reasons (#420) +// — the AI providers dialog and the shortcuts dialog — and silently missed every modal that +// kept its `useState` where it was: Export, Open project, New project, Edit clip, the +// unsaved-changes prompt (issue #434). Each new modal was a new special case nobody would +// remember to add. +// +// Every modal in this tree already announces itself the same way: `ModalShell` and the +// hand-rolled portal in `LeftPanel` render `aria-modal="true"`, and `ui/dialog` passes the same +// attribute to Radix's content. So one selector answers for all of them, including the ones +// that do not exist yet. + +/** + * True while a modal owns the screen. + * + * For window-level event handlers, never for rendering: it reads the live DOM, so a component + * that called it during render would not re-render when the answer changed. + */ +export function isModalOpen(): boolean { + return document.querySelector('[aria-modal="true"]') !== null; +} diff --git a/src/lib/ai-edition/store/undo.modalGuard.test.tsx b/src/lib/ai-edition/store/undo.modalGuard.test.tsx new file mode 100644 index 000000000..ef7066c27 --- /dev/null +++ b/src/lib/ai-edition/store/undo.modalGuard.test.tsx @@ -0,0 +1,88 @@ +// @vitest-environment jsdom +// Undo/redo are bound on `window` here, not in `NewEditorShell` — the shell explicitly defers +// Ctrl+Z / Ctrl+Y to this listener. So the shell's modal guard never runs for them, and until +// this listener asked the same question, Ctrl+Z rewrote the document under every open modal, +// including the ones the shell already suppressed everything else for (issue #434, compounding +// with #433). + +import "@testing-library/jest-dom"; +import { cleanup, fireEvent, render } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { type AxcutDocument, axcutSchemaVersion } from "../schema"; +import { useProjectStore } from "./projectStore"; +import { clearHistory, pushHistory, useUndoRedoShortcuts } from "./undo"; + +function doc(title: string): AxcutDocument { + return { + schemaVersion: axcutSchemaVersion, + project: { + id: "proj_test", + title, + createdAt: "2026-06-25T10:00:00.000Z", + updatedAt: "2026-06-25T10:00:00.000Z", + }, + assets: [], + transcript: null, + transcripts: [], + timeline: { + clips: [], + gaps: [], + trimRanges: [], + muteRanges: [], + speedRanges: [], + captionRanges: [], + }, + annotations: [], + zoomRanges: [], + legacyEditor: null, + }; +} + +const onAfter = vi.fn(); + +/** `aria-modal="true"` on a `role="dialog"` is what every modal in the editor renders — see + * `ModalShell` and `ui/dialog`, both pinned in `modalGuard.test.tsx`. */ +function Harness({ modal }: { modal: boolean }) { + useUndoRedoShortcuts(onAfter); + return modal ?
: null; +} + +beforeEach(() => { + onAfter.mockClear(); + clearHistory(); + useProjectStore.getState().clear(); + // One edit in the past, so a working Ctrl+Z has something to roll back to. + useProjectStore.setState({ projectId: "proj_test", document: doc("after") }); + pushHistory({ projectId: "proj_test", doc: doc("before") }); +}); + +afterEach(() => { + cleanup(); + clearHistory(); + useProjectStore.getState().clear(); +}); + +describe("useUndoRedoShortcuts under a modal", () => { + it("undoes on Ctrl+Z with nothing on screen", () => { + render(); + + fireEvent.keyDown(document.body, { key: "z", ctrlKey: true }); + + expect(onAfter).toHaveBeenCalledTimes(1); + expect(useProjectStore.getState().document?.project.title).toBe("before"); + }); + + it("leaves the document alone while a modal owns the screen", () => { + const { rerender } = render(); + + fireEvent.keyDown(document.body, { key: "z", ctrlKey: true }); + + expect(onAfter).not.toHaveBeenCalled(); + expect(useProjectStore.getState().document?.project.title).toBe("after"); + + // Nothing was consumed either: the edit is still undoable once the modal is gone. + rerender(); + fireEvent.keyDown(document.body, { key: "z", ctrlKey: true }); + expect(useProjectStore.getState().document?.project.title).toBe("before"); + }); +}); diff --git a/src/lib/ai-edition/store/undo.ts b/src/lib/ai-edition/store/undo.ts index 81ab02159..80cda0d8d 100644 --- a/src/lib/ai-edition/store/undo.ts +++ b/src/lib/ai-edition/store/undo.ts @@ -5,6 +5,7 @@ // free so it works in any renderer. import { useEffect, useRef } from "react"; +import { isModalOpen } from "../modalGuard"; import { useProjectStore } from "./projectStore"; type Snapshot = { projectId: string; doc: unknown }; @@ -66,6 +67,10 @@ export function useUndoRedoShortcuts(onAfter: () => void) { const onKey = (e: KeyboardEvent) => { if (e.target instanceof HTMLTextAreaElement || e.target instanceof HTMLInputElement) return; if (e.target instanceof HTMLElement && e.target.isContentEditable) 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. + if (isModalOpen()) return; const ctrl = e.ctrlKey || e.metaKey; if (ctrl && e.shiftKey && e.key.toLowerCase() === "z") { e.preventDefault();