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

[EtienneLescot]

What

While the Export modal is open, pressing Z, T or C still creates zoom / trim / camera regions on the timeline behind it. Same for every other modal the editor shell owns, and Ctrl+Z under any modal — including the two that were otherwise covered.

This replaces the guard's list of open-state flags with one central question — is a modal on screen? — answered from the DOM.

Fixes #434

Root cause

65aa74ef added the guard as:

if (isDialogOpen() || isConfigOpen) return;          // NewEditorShell.tsx:866

Two flags, not two modals:

• isDialogOpen() comes from EditorDialogsContext and answers sectionRef.current !== null, where EditorDialogSection has exactly one member: "providers".
• isConfigOpen comes from ShortcutsContext and is the Keyboard Shortcuts dialog alone.

Those are precisely the two dialogs whose open state had been lifted into a context for an unrelated reason (the app menu, #420). Every other modal keeps its useState in the component that mounts it — const [exportOpen, setExportOpen] = useState(false) sits in NewEditorShell itself, in the same component as the handler, and the guard cannot see it. Same for Open project, New project, Edit clip and the unsaved-changes prompt.

The two guards preceding it only skip HTMLInputElement / HTMLTextAreaElement / isContentEditable targets, and ModalShell focuses a tabIndex={-1} div — a div, not a text field — so Z / T / C reach tl.addZoom / tl.addTrim / tl.addCameraFullscreen untouched. The commit message said as much itself: "Both flags are reachable now that the open state is lifted out of the components that used to own it (#420)" — the fix was scoped to what #420 made reachable, not to the set of modals.

Second instance of the same bug: src/lib/ai-edition/store/undo.ts registers its own window keydown listener with the identical input/textarea/contentEditable guards and no modal guard at all, and NewEditorShell deliberately defers Ctrl+Z / Ctrl+Y to it. So undo/redo mutated the timeline under every modal, including the two the shell did suppress — which is exactly what the issue flags as compounding with #433.

What changed

New src/lib/ai-edition/modalGuard.ts — one predicate both callers can import (undo.ts is a store file, so this lives outside the component tree):

export function isModalOpen(): boolean {
	return document.querySelector('[aria-modal="true"]') !== null;
}

Every modal in this tree already announces itself that way: ModalShell (Export, Open project, New project, Edit clip, unsaved changes, AI providers) and LeftPanel's hand-rolled portal both render aria-modal="true".

src/components/ui/dialog.tsx — added aria-modal="true" to DialogPrimitive.Content. Radix 1.1.15 renders role="dialog" with no aria-modal (verified in node_modules/@radix-ui/react-dialog/dist/index.mjs), and this is what folds ShortcutsConfigDialog and EditorEmptyState's drop-error dialog under the same selector. It is placed before {...props} so a caller can still override it, and both consumers are modal dialogs (no modal={false} anywhere in src/).

Why [aria-modal="true"] and not a [role="dialog"][data-state="open"] fallback: Radix Popover content also renders role="dialog", and popovers are used by ColorField and V4Timeline. A role-based selector would silently suppress editor shortcuts under every color picker.

src/components/ai-edition/NewEditorShell.tsx — the guard becomes if (isModalOpen()) return;. isConfigOpen and isDialogOpen drop out of the destructures and the effect deps.

src/lib/ai-edition/store/undo.ts — the same one-line guard after the target checks.

src/contexts/EditorDialogsContext.tsx — NewEditorShell was the only consumer of isDialogOpen, so it and the sectionRef that existed to mirror the section for it are removed. The context goes back to being a section plus two openers, which is the shape its own comment describes wanting.

Why not the alternatives

• Adding "export" to EditorDialogSection is a second special case and leaves the other modals broken.
• A ModalShell-driven open-modal counter in context is more plumbing, still misses the two Radix dialogs and LeftPanel's portal, and cannot be read from undo.ts without threading a hook through the store layer.
• A target-based check (e.target.closest('[role="dialog"]')) fails exactly when the bug bites: the app menu closes without restoring focus, so e.target is usually document.body — the premise of 65aa74ef's own commit message.

Tests

NewEditorShell.dialogShortcuts.test.tsx used to drive the context (dialogActions.openDialog("providers")), which puts nothing on screen — it asserted the flag, not the behavior. It now opens a modal the shell itself owns via Ctrl+O (local useState, the same shape as Export) and asserts:

• ? does not reach openConfig while it is open, and does once Escape closes it
• Ctrl+N does not stack a second dialog underneath

? 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.

Two new files:

• src/lib/ai-edition/store/undo.modalGuard.test.tsx — Ctrl+Z leaves the document alone while an aria-modal dialog is on screen, and the edit is still undoable once it is gone.
• src/lib/ai-edition/modalGuard.test.tsx — pins the attribute the predicate depends on against the real ModalShell and ui/dialog components, since it lives in them rather than in the guard.

Verified failing without the fix

Neutralizing isModalOpen() to return false (the pre-fix state for these tests: neither flag's dialog is on screen in any of them):

     × leaves the document alone while a modal owns the screen 7ms
     × suppresses ? while a modal the shell itself owns is open, and resumes when it closes 34ms
     × does not stack a second dialog when Ctrl+N fires under an open modal 56ms
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 3 ⎯⎯⎯⎯⎯⎯⎯
AssertionError: expected "vi.fn()" to not be called at all, but actually been called 1 times
AssertionError: expected [ …(2) ] to have a length of 1 but got 2
AssertionError: expected "vi.fn()" to not be called at all, but actually been called 1 times
 Test Files  2 failed (2)
      Tests  3 failed | 3 passed (6)

Stashing only the dialog.tsx change:

     × is true while a ui/dialog content is open 113ms
AssertionError: expected false to be true // Object.is equality
 Test Files  1 failed (1)
      Tests  1 failed | 2 passed (3)

How verified

All run on the final rebased tree (branched from origin/main, rebased onto b1fc616f).

$ npx tsc --noEmit
app-typecheck exit=0

$ npx tsc -p tsconfig.test.json --noEmit
test-typecheck exit=0

$ npm run lint
Checked 671 files in 670ms. No fixes applied.
Found 13 warnings.          # exit 0; all 13 pre-existing, none in the touched files

$ npx vitest --run src/components/ai-edition src/lib/ai-edition src/contexts src/components/ui src/components/video-editor
 Test Files  72 passed (72)
      Tests  822 passed (822)
   Duration  19.49s

The broad targeted run (rather than only the touched files) is deliberate: ui/dialog is a shared primitive, so every consumer of it and of EditorDialogsContext was exercised. Not manually smoke-tested against the packaged app — the issue's repro is a real Export-modal keypress, worth one pass on Windows before release.

Interaction with #433 — read before merging either

claude/fix-433-undo-redo touches two of the same files. The two branches merge with one trivial conflict in src/lib/ai-edition/store/undo.ts (a straight union of the import list and the keydown guard), and the merged tree is fully green: both typechecks, lint, and 816 tests across src/lib/ai-edition src/components/ai-edition src/contexts src/components/ui.

It is green and still broken. A combined-merge check found a semantic conflict that neither branch's tests catch:

#433 stops using Electron's role: "undo" and gives the Edit menu its own CmdOrCtrl+Z accelerator, forwarded over IPC as menu-undo to runUndo in undo.ts. That handler checks only isTextEditingTarget(document.activeElement). There is no isModalOpen() on that path, and a modal's controls are buttons, not text fields — so the check passes and undo() rewrites the document under the open modal. That is verbatim the bug this PR closes.

Reachable on every platform, for two different reasons:

• macOS — unconditional. AppKit matches the menu key equivalent in -[NSApplication sendEvent:] before the event reaches the web contents, so the guarded keydown listener never runs. This PR's guard becomes dead code on macOS once [Bug]: Undo/Redo are advertised in the Keyboard Shortcuts modal but do nothing #433 lands.
• Windows/Linux — this PR's guard is what creates the condition. It returns early without preventDefault(), so the key becomes an unhandled keyboard event, which is exactly how Electron dispatches menu accelerators on those platforms. No modal: the shortcut preventDefaults and the accelerator is suppressed. Modal open: it is not, and the accelerator fires into the unguarded runUndo.

Verified empirically on the merged tree with a throwaway probe (since deleted): the keydown path is correctly guarded and comes back defaultPrevented === false, while runUndo() with an aria-modal="true" node in the DOM rewrote the document and fired the persist callback.

Whichever of the two lands second must carry this, in src/lib/ai-edition/store/undo.ts — text-field check first, so a rename dialog's input still gets the browser's text undo:

if (isTextEditingTarget(window.document.activeElement)) {
	window.document.execCommand?.("undo");
	return;
}
if (isModalOpen()) return;
if (undo()) onAfterRef.current();

…and the same in runRedo, plus a case in #433's "the Edit menu's undo/redo route" describe that appends an aria-modal="true" node and asserts the document is untouched. That block currently clears document.body in its beforeEach, which incidentally guarantees isModalOpen() is false — which is why it passes today.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@EtienneLescot, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2066307d-acc4-4c9d-b7c6-cc7c671ab237

📥 Commits

Reviewing files that changed from the base of the PR and between b1fc616 and 3984ccf.

📒 Files selected for processing (8)

• src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx
• src/components/ai-edition/NewEditorShell.tsx
• src/components/ui/dialog.tsx
• src/contexts/EditorDialogsContext.tsx
• src/lib/ai-edition/modalGuard.test.tsx
• src/lib/ai-edition/modalGuard.ts
• src/lib/ai-edition/store/undo.modalGuard.test.tsx
• src/lib/ai-edition/store/undo.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}