fix(editor): suppress editor shortcuts under every modal, not just some

Z, T and C kept adding regions to the timeline behind the Export modal.

The guard added in 65aa74e asked `isDialogOpen() || isConfigOpen` — one
flag for the AI providers dialog, one for the keyboard shortcuts dialog.
Those are the two whose open state had been lifted into a context for an
unrelated reason (#420); every other modal keeps its `useState` in the
component that mounts it, so the guard could not see Export, Open project,
New project, Edit clip or the unsaved-changes prompt. The guards above it
only skip inputs, textareas and contentEditable targets, and ModalShell
focuses a `tabIndex={-1}` div, so every shortcut sailed through.

Ask the screen instead of the flags. `isModalOpen()` looks for an element
with `aria-modal="true"`, which ModalShell, LeftPanel's hand-rolled portal
and — now that `ui/dialog` states it, since Radix 1.1.15 does not — the two
Radix dialogs all render. One predicate, and the modals that do not exist
yet are covered for free.

`store/undo` gets the same line. NewEditorShell defers Ctrl+Z / Ctrl+Y to
that listener's own `window` handler, which had no modal guard at all, so
undo rewrote the document under every modal, including the two the shell
did suppress.

`EditorDialogsActions.isDialogOpen` and the `sectionRef` that mirrored the
section for it had no other caller and go with it.

Fixes #434

Author: EtienneLescot

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<typeof useEditorDialogActions> | null = null;
-
-function CaptureDialogActions() {
-	dialogActions = useEditorDialogActions();
-	return null;
-}
-
 function renderShell() {
 	return render(
 		<EditorDialogsProvider>
-			<CaptureDialogActions />
 			<NewEditorShell />
 		</EditorDialogsProvider>,
 	);
@@ -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);
 	});
 });

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,

src/components/ui/dialog.tsx

@@ -35,6 +35,11 @@ const DialogContent = React.forwardRef<
 		<DialogOverlay />
 		<DialogPrimitive.Content
 			ref={ref}
+			// Radix 1.1.15 renders `role="dialog"` but no `aria-modal`, and a modal `Dialog` is
+			// exactly what this content is. Stated explicitly it also makes these dialogs visible
+			// to `isModalOpen` (lib/ai-edition/modalGuard), the one predicate the editor's global
+			// shortcuts ask before acting. Before `{...props}` so a caller can still override it.
+			aria-modal="true"
 			className={cn(
 				"fixed left-[50%] top-[50%] z-[10000] grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg",
 				className,

src/contexts/EditorDialogsContext.tsx

@@ -1,4 +1,4 @@
-import { createContext, type ReactNode, useContext, useMemo, useRef, useState } from "react";
+import { createContext, type ReactNode, useContext, useMemo, useState } from "react";
 
 // Which of the editor chrome's own dialogs is open, lifted out of the component that used to
 // own it.
@@ -16,16 +16,17 @@ import { createContext, type ReactNode, useContext, useMemo, useRef, useState }
 // Split in two on purpose. The section changes on every open and close, the actions never do.
 // `NewEditorShell` owns the timeline, the preview and the transport, and only ever needs to
 // *open* a dialog — subscribing it to the section would re-render the whole editor twice per
-// dialog interaction, so it takes the actions alone. `isDialogOpen` serves the readers that
-// are event handlers rather than renders: it answers from a ref, which is what lets it sit in
-// a value whose identity never changes.
+// dialog interaction, so it takes the actions alone.
+//
+// Nothing asks this context whether a dialog is open. It briefly answered that for the editor's
+// global shortcuts, which was only ever half an answer — this context knows about its own
+// dialogs and about no others (#434). `isModalOpen` (lib/ai-edition/modalGuard) is where that
+// question is asked now.
 export type EditorDialogSection = "providers";
 
 interface EditorDialogsActions {
 	openDialog: (section: EditorDialogSection) => 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<EditorDialogSection | null>(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<EditorDialogSection | null>(null);
 
 	const actions = useMemo<EditorDialogsActions>(
 		() => ({
-			openDialog: (next) => {
-				sectionRef.current = next;
-				setSection(next);
-			},
-			closeDialog: () => {
-				sectionRef.current = null;
-				setSection(null);
-			},
-			isDialogOpen: () => sectionRef.current !== null,
+			openDialog: (next) => setSection(next),
+			closeDialog: () => setSection(null),
 		}),
 		[],
 	);

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(<div>the editor</div>);
+
+		expect(isModalOpen()).toBe(false);
+	});
+
+	it("is true while a ModalShell is open, false once it closes", () => {
+		const { rerender } = render(
+			<ModalShell open onClose={noop} title="Export">
+				<button type="button">Start export</button>
+			</ModalShell>,
+		);
+		expect(isModalOpen()).toBe(true);
+
+		rerender(
+			<ModalShell open={false} onClose={noop} title="Export">
+				<button type="button">Start export</button>
+			</ModalShell>,
+		);
+		expect(isModalOpen()).toBe(false);
+	});
+
+	it("is true while a ui/dialog content is open", () => {
+		render(
+			<Dialog open>
+				<DialogContent aria-label="Keyboard shortcuts">
+					<button type="button">Reset</button>
+				</DialogContent>
+			</Dialog>,
+		);
+
+		expect(isModalOpen()).toBe(true);
+	});
+});

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;
+}