From 1d82585a458ba22509bd9649e4a75fadd11d0118 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Thu, 20 Aug 2026 23:48:35 +0200 Subject: [PATCH 1/2] fix(editor): stop editor shortcuts reaching the timeline under a modal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups to #422, which lifted the AI provider dialog out of the chat panel and mounted it above the mode switch. Four of these are real bugs, three of them older than that PR and only made easier to reach by it. ModalShell dialogs never took focus, and the editor's window-level keydown handler only skips input/textarea/contentEditable targets. The app menu closes without restoring focus — right for a pointer user — so opening AI settings from it left `document.activeElement` on the body and every editor shortcut live underneath the backdrop: Delete destroyed the selected region, Ctrl+O stacked a second aria-modal dialog emitting a duplicate `id="modal-title"`, `?` stacked the shortcuts dialog. The handler now bails while a dialog owns the screen, and ModalShell moves focus into the dialog as it opens, which is also what announces it to a screen reader. Escape in the provider connect form left the dialog entirely, discarding a half-typed API key, instead of stepping back to the grid: ProviderSettings and ModalShell both listened on document and both fired, so the `mode === "form"` branch was dead. ModalShell grows a `closeOnEscape` opt-out for a dialog that handles the key itself. Disconnecting a provider went on showing it as CONNECTED until the dialog was closed and reopened. `snapshot` is not optional on the disconnect result, so the `?? refreshSnapshot()` fallback never ran — and refreshSnapshot is the only thing that calls setSnapshot. ProviderSettingsContext becomes EditorDialogsContext, holding a `section` rather than a boolean per dialog, split into a section context and an actions context whose value never changes identity. That is what stops NewEditorShell — timeline, preview, transport — re-rendering twice per dialog interaction just to hold an opener, and it is the shape the settings unification #420 describes needs anyway: the next section is a member of the union, not a third context and a third provider around App.tsx's editor branch. The panel's re-read on close loses its ref: "not open" already covers the mount and every close in one effect. It is also now tested, which it was not — the PR said as much — along with the fact that opening the dialog must not refresh. Also drops the dead `onActiveProviderChanged` prop and unexports ProviderSettings (ProviderSettingsDialog is its only caller), and corrects two comments the new menu row falsified: AppMenu's claim that every label is a common.actions or shortcuts key, and llm-providers.md's list of the panel's doors, which was missing the quick-pick popover's "full settings" row. --- src/App.tsx | 6 +- .../LeftPanel.providerRefresh.test.tsx | 108 ++++++++++++++++++ src/components/ai-edition/LeftPanel.tsx | 27 ++--- src/components/ai-edition/Modals.tsx | 16 ++- src/components/ai-edition/NewEditorShell.tsx | 20 +++- .../ai-edition/ProviderSettings.test.tsx | 12 +- .../ai-edition/ProviderSettings.tsx | 35 +++--- src/components/ai-edition/v4/EditorTopBar.tsx | 9 +- src/contexts/EditorDialogsContext.tsx | 72 ++++++++++++ src/contexts/ProviderSettingsContext.tsx | 41 ------- .../architecture/llm-providers.md | 11 +- 11 files changed, 263 insertions(+), 94 deletions(-) create mode 100644 src/components/ai-edition/LeftPanel.providerRefresh.test.tsx create mode 100644 src/contexts/EditorDialogsContext.tsx delete mode 100644 src/contexts/ProviderSettingsContext.tsx diff --git a/src/App.tsx b/src/App.tsx index c86fe6f7b..517ad7abd 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -11,8 +11,8 @@ import { NotesWindow } from "./components/launch/NotesWindow.tsx"; import { SourceSelector } from "./components/launch/SourceSelector"; import { Toaster } from "./components/ui/sonner"; import { TooltipProvider } from "./components/ui/tooltip"; +import { EditorDialogsProvider } from "./contexts/EditorDialogsContext"; import { useScopedT } from "./contexts/I18nContext"; -import { ProviderSettingsProvider } from "./contexts/ProviderSettingsContext"; import { ShortcutsProvider } from "./contexts/ShortcutsContext"; import { loadAllCustomFonts } from "./lib/customFonts"; @@ -113,7 +113,7 @@ export default function App() { case "editor": return ( - + @@ -147,7 +147,7 @@ export default function App() { - + ); default: diff --git a/src/components/ai-edition/LeftPanel.providerRefresh.test.tsx b/src/components/ai-edition/LeftPanel.providerRefresh.test.tsx new file mode 100644 index 000000000..6163ed8d3 --- /dev/null +++ b/src/components/ai-edition/LeftPanel.providerRefresh.test.tsx @@ -0,0 +1,108 @@ +// @vitest-environment jsdom +// Issue #420: the provider dialog's open state moved out of `ChatStripPanel` and into +// EditorDialogsContext, and the `onClose` that used to re-read the LLM snapshot went with it. +// Connecting a provider in that dialog is what enables the composer here and what fills the +// model pill, so the panel now refreshes whenever the dialog is NOT open — on mount, and again +// on every close. +// +// That re-read is the one behaviour the lift had to re-establish by hand rather than move, and +// it cannot be seen from the dialog's own tests (they never mount this panel), so it is pinned +// here: refreshed once on mount, not again when the dialog opens, once more when it closes. + +import "@testing-library/jest-dom"; +import { act, cleanup, render } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const llmGetSnapshot = vi.fn(() => + Promise.resolve({ + config: null, + connectedProviders: [], + availableProviders: [], + credentialSummary: [], + }), +); + +vi.mock("@/native/client", () => ({ + nativeBridgeClient: { + aiEdition: { + llmGetSnapshot: () => llmGetSnapshot(), + chatListSessions: () => Promise.resolve([]), + chatBudget: () => Promise.resolve(null), + llmListProviderModels: () => Promise.resolve({ models: [] }), + }, + }, +})); + +// The panel's copy is not what is under test, and an echoing translator keeps this file off +// the critical path of a copy edit. +vi.mock("@/contexts/I18nContext", () => ({ + useI18n: () => ({ + locale: "en", + setLocale: () => { + /* fixed locale */ + }, + }), + useScopedT: () => (key: string) => key, +})); + +import { EditorDialogsProvider, useEditorDialogActions } from "@/contexts/EditorDialogsContext"; +import { LeftPanel } from "./LeftPanel"; + +let dialogActions: ReturnType | null = null; + +/** Hands the test the context's openers, which the app menu and the panel's own gear share. */ +function CaptureDialogActions() { + dialogActions = useEditorDialogActions(); + return null; +} + +beforeEach(() => { + llmGetSnapshot.mockClear(); + dialogActions = null; + // The panel subscribes to streamed chat events on mount; there is no preload in jsdom. + (window as unknown as { electronAPI?: unknown }).electronAPI = { + onAiEditionChatEvent: () => () => { + /* unsubscribe */ + }, + }; + // jsdom implements no scrolling at all, and the transcript pins itself to the bottom on + // every render. + Element.prototype.scrollTo = () => { + /* no scrolling in jsdom */ + }; +}); + +afterEach(() => { + cleanup(); + (window as unknown as { electronAPI?: unknown }).electronAPI = undefined; +}); + +describe("ChatStripPanel, against the lifted provider dialog", () => { + it("re-reads the LLM snapshot when the dialog closes, and not when it opens", async () => { + render( + + + + , + ); + // Mount: the dialog is closed, so the same effect that watches for a close seeds the + // composer's view of the provider config. + await act(async () => { + await Promise.resolve(); + }); + expect(llmGetSnapshot).toHaveBeenCalledTimes(1); + + // Opening it must not refresh — nothing has been connected yet, and the old code's + // `onClose` did not fire here either. + await act(async () => { + dialogActions?.openDialog("providers"); + }); + expect(llmGetSnapshot).toHaveBeenCalledTimes(1); + + // Closing is the event that used to be `onClose` -> `refreshLlm()`. + await act(async () => { + dialogActions?.closeDialog(); + }); + expect(llmGetSnapshot).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/components/ai-edition/LeftPanel.tsx b/src/components/ai-edition/LeftPanel.tsx index 73242c199..06a349ef1 100644 --- a/src/components/ai-edition/LeftPanel.tsx +++ b/src/components/ai-edition/LeftPanel.tsx @@ -2,8 +2,8 @@ import { ArrowLeft, Check, Film, Loader2, MessageSquare, Plus, Search, X } from import { useCallback, useEffect, useRef, useState } from "react"; import { createPortal } from "react-dom"; import { toast } from "sonner"; +import { useEditorDialogActions, useEditorDialogSection } from "@/contexts/EditorDialogsContext"; import { useScopedT } from "@/contexts/I18nContext"; -import { useProviderSettings } from "@/contexts/ProviderSettingsContext"; import type { AxcutAsset } from "@/lib/ai-edition/schema"; import { applyAgentDocumentIfCurrent, @@ -738,8 +738,11 @@ function ChatStripPanel() { const [busy, setBusy] = useState(false); const [llmConfig, setLlmConfig] = useState(null); // The dialog itself is mounted in App.tsx so the app menu can reach it from every mode - // (issue #420); this panel only asks for it to be opened. - const { isProviderSettingsOpen, openProviderSettings } = useProviderSettings(); + // (issue #420); this panel only asks for it to be opened, and watches it close. + const dialogSection = useEditorDialogSection(); + const { openDialog } = useEditorDialogActions(); + const providerSettingsOpen = dialogSection === "providers"; + const openProviderSettings = useCallback(() => openDialog("providers"), [openDialog]); const [chatsOpen, setChatsOpen] = useState(false); const [sessions, setSessions] = useState< Array<{ id: string; title: string; messageCount: number; createdAt: string }> @@ -811,19 +814,13 @@ function ChatStripPanel() { } }, []); + // On mount, and again every time the provider dialog closes. Connecting a provider there is + // what makes the composer usable here and what fills the model pill, and the dialog no + // longer hangs off this component, so there is no onClose to do it from. "Not open" covers + // both events at once, which is why there is no ref here watching for the falling edge. useEffect(() => { - void refreshLlm(); - }, [refreshLlm]); - - // Re-read after the dialog closes. Connecting a provider there is what makes the composer - // usable here, and the dialog no longer hangs off this component, so there is no onClose to - // do it from — the falling edge of the lifted state is the same event. - const providerSettingsWasOpen = useRef(isProviderSettingsOpen); - useEffect(() => { - const wasOpen = providerSettingsWasOpen.current; - providerSettingsWasOpen.current = isProviderSettingsOpen; - if (wasOpen && !isProviderSettingsOpen) void refreshLlm(); - }, [isProviderSettingsOpen, refreshLlm]); + if (!providerSettingsOpen) void refreshLlm(); + }, [providerSettingsOpen, refreshLlm]); // ponytail: subscribe to streamed chat events so the reasoning trace (and // any future streaming text deltas) lands live instead of arriving all at diff --git a/src/components/ai-edition/Modals.tsx b/src/components/ai-edition/Modals.tsx index 749aa8719..6ee0200bd 100644 --- a/src/components/ai-edition/Modals.tsx +++ b/src/components/ai-edition/Modals.tsx @@ -81,6 +81,7 @@ function useEscape(open: boolean, onClose: () => void) { export function ModalShell({ open, onClose, + closeOnEscape = true, title, subtitle, wide, @@ -89,13 +90,26 @@ export function ModalShell({ title: string; subtitle?: string; wide?: boolean; + /** Off for a dialog that handles Escape itself — two listeners both fire for one + * keypress, and this one's `onClose` wins whatever order they registered in. */ + closeOnEscape?: boolean; children: ReactNode; }) { const tc = useScopedT("common"); - useEscape(open, onClose); + const dialogRef = useRef(null); + useEscape(open && closeOnEscape, onClose); + // Move focus into the dialog as it opens. The app menu closes without restoring focus to + // its trigger — right for a pointer user — so otherwise the opener is left on + // document.body: Tab then walks the editor *behind* the backdrop, and a screen reader is + // never told a dialog appeared. + useEffect(() => { + if (open) dialogRef.current?.focus(); + }, [open]); if (!open) return null; return (
void; } | null>(null); - const { shortcuts, isMac, openConfig: openShortcutsConfig } = useShortcuts(); - const { openProviderSettings } = useProviderSettings(); + const { shortcuts, isMac, isConfigOpen, 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(); // 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 @@ -854,6 +858,12 @@ export function NewEditorShell() { const onKey = (e: KeyboardEvent) => { if (e.target instanceof HTMLTextAreaElement || e.target instanceof HTMLInputElement) return; if (e.target instanceof HTMLElement && e.target.isContentEditable) return; + // 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; const ctrl = e.ctrlKey || e.metaKey; if (ctrl && e.key === "s") { e.preventDefault(); @@ -1039,6 +1049,8 @@ export function NewEditorShell() { saveDocument, copiedClipId, openShortcutsConfig, + isConfigOpen, + isDialogOpen, shortcuts, isMac, togglePlay, @@ -1138,7 +1150,7 @@ export function NewEditorShell() { openSettings: handleOpenSettings, renameProject: handleRenameProject, toggleChat: () => setChatOpen((v) => !v), - openProviderSettings, + openProviderSettings: () => openDialog("providers"), showAbout: handleShowAbout, checkForUpdates: handleCheckForUpdates, }} diff --git a/src/components/ai-edition/ProviderSettings.test.tsx b/src/components/ai-edition/ProviderSettings.test.tsx index 86af87d58..3c1debd73 100644 --- a/src/components/ai-edition/ProviderSettings.test.tsx +++ b/src/components/ai-edition/ProviderSettings.test.tsx @@ -1,7 +1,7 @@ // @vitest-environment jsdom // Issue #420: the AI provider dialog used to be a `useState` inside LeftPanel's chat strip, so // it existed only in Edit mode with the chat panel expanded and nothing else could open it. It -// is mounted once now, above the mode switch, and driven by ProviderSettingsContext. +// is mounted once now, above the mode switch, and driven by EditorDialogsContext. // // These tests are about *reach*, not about the dialog's own screens: that the app menu's row // really opens it, that it opens in Media and Rec too, and that the row and the heading are one @@ -10,8 +10,8 @@ import "@testing-library/jest-dom"; import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { EditorDialogsProvider, useEditorDialogActions } from "@/contexts/EditorDialogsContext"; import { I18nProvider } from "@/contexts/I18nContext"; -import { ProviderSettingsProvider, useProviderSettings } from "@/contexts/ProviderSettingsContext"; import { LOCALE_STORAGE_KEY } from "@/i18n/config"; import { type EditorMode, EditorTopBar } from "./v4/EditorTopBar"; @@ -39,7 +39,7 @@ const noop = () => {}; /** The top bar as NewEditorShell builds it: the menu row's action is the context's opener, and * nothing else in `actions` matters here. */ function TopBar({ mode }: { mode: EditorMode }) { - const { openProviderSettings } = useProviderSettings(); + const { openDialog } = useEditorDialogActions(); return ( openDialog("providers"), showAbout: noop, checkForUpdates: noop, }} @@ -71,10 +71,10 @@ function renderEditorChrome(locale: string, mode: EditorMode = "edit") { localStorage.setItem(LOCALE_STORAGE_KEY, locale); return render( - + - + , ); } diff --git a/src/components/ai-edition/ProviderSettings.tsx b/src/components/ai-edition/ProviderSettings.tsx index 6bcf9fa3f..8d66681c7 100644 --- a/src/components/ai-edition/ProviderSettings.tsx +++ b/src/components/ai-edition/ProviderSettings.tsx @@ -13,14 +13,14 @@ // with them. Credentials live in the safeStorage blob (LlmConfigStore) — the // renderer never sees raw keys, only `kind`. // -// ponytail: existing consumers (ProviderSettings used as ) -// must keep working. Internal state is local-only. +// `ProviderSettingsDialog` at the bottom is the only mount, and the only caller of the +// `open` / `onClose` component above it. Internal state is local-only. import { AlertCircle, Check, Loader2, Unplug, X } from "lucide-react"; import { useCallback, useEffect, useState } from "react"; import { toast } from "sonner"; +import { useEditorDialogActions, useEditorDialogSection } from "@/contexts/EditorDialogsContext"; import { useScopedT } from "@/contexts/I18nContext"; -import { useProviderSettings } from "@/contexts/ProviderSettingsContext"; import { nativeBridgeClient } from "@/native/client"; import type { AiEditionLlmConfig, AiEditionLlmSnapshot } from "@/native/contracts"; import { @@ -37,14 +37,9 @@ type Mode = "list" | "form"; interface ProviderSettingsProps { open: boolean; onClose: () => void; - onActiveProviderChanged?: (providerId: string | null) => void; } -export function ProviderSettings({ - open, - onClose, - onActiveProviderChanged, -}: ProviderSettingsProps) { +function ProviderSettings({ open, onClose }: ProviderSettingsProps) { const te = useScopedT("editor"); const [snapshot, setSnapshot] = useState(null); const [mode, setMode] = useState("list"); @@ -90,6 +85,10 @@ export function ProviderSettings({ setError(null); }, [busy]); + // Escape is ours alone — see `closeOnEscape={false}` on the ModalShell below. While + // ModalShell also handled it, both listeners fired for one keypress and its `onClose` won, + // so Escape in the connect form left the dialog entirely (discarding a half-typed key) + // instead of stepping back to the grid, and the branch below was dead. useEffect(() => { if (!open) return; const onKey = (e: KeyboardEvent) => { @@ -138,8 +137,7 @@ export function ProviderSettings({ setApiKey(""); } await nativeBridgeClient.aiEdition.llmSetConfig(config); - const snap = await refreshSnapshot(); - onActiveProviderChanged?.(snap?.config?.provider ?? null); + await refreshSnapshot(); toast.success(te("providerSettings.saved", { provider: active.label })); setMode("list"); setActive(null); @@ -156,8 +154,11 @@ export function ProviderSettings({ setError(null); try { const result = await nativeBridgeClient.aiEdition.llmDisconnect(active.id); - const snap = result.snapshot ?? (await refreshSnapshot()); - onActiveProviderChanged?.(snap.config?.provider ?? null); + // `snapshot` is not optional on the result, so a `?? refreshSnapshot()` fallback here + // never ran — and `refreshSnapshot` is the only thing that calls `setSnapshot`. The + // form and the grid behind it went on showing the provider as CONNECTED until the + // dialog was closed and reopened. + setSnapshot(result.snapshot); toast.success(te("providerSettings.disconnected", { provider: active.label })); } catch (err) { setError(err instanceof Error ? err.message : String(err)); @@ -170,6 +171,7 @@ export function ProviderSettings({ ; + const section = useEditorDialogSection(); + const { closeDialog } = useEditorDialogActions(); + return ; } function ProviderList({ diff --git a/src/components/ai-edition/v4/EditorTopBar.tsx b/src/components/ai-edition/v4/EditorTopBar.tsx index ba5aefdd0..a369da6a8 100644 --- a/src/components/ai-edition/v4/EditorTopBar.tsx +++ b/src/components/ai-edition/v4/EditorTopBar.tsx @@ -278,9 +278,12 @@ function ProjectNameField({ * paid for out of the mode labels, in the most verbose of 13 locales, at the 800px minimum * window width. * - * Every label is an EXISTING common.actions / shortcuts key — the same ones electron/main.ts - * builds the native menu from. Reusing them is what stops the two menus from drifting apart, - * and it is why this component adds no translation work. */ + * No row invents a label. Each one reuses the key of the thing it opens: `common.actions.*` + * for the rows electron/main.ts also builds native menu items from (About, Check for + * Updates), and the dialog's own title key for the rows that open a dialog (`shortcuts.title`, + * `editor.providerSettings.title`). That is what stops this menu from drifting away from the + * native menu on one side and from what its rows actually open on the other — and it is why + * this component adds no translation work. */ /** Shared by the mount seed and the per-open refresh below. A rejection — no preload, browser * mode — resolves to "no", which hides the item rather than shipping a button whose click the * main process would refuse without saying so. */ diff --git a/src/contexts/EditorDialogsContext.tsx b/src/contexts/EditorDialogsContext.tsx new file mode 100644 index 000000000..e05885512 --- /dev/null +++ b/src/contexts/EditorDialogsContext.tsx @@ -0,0 +1,72 @@ +import { createContext, type ReactNode, useContext, useMemo, useRef, useState } from "react"; + +// Which of the editor chrome's own dialogs is open, lifted out of the component that used to +// own it. +// +// The AI provider dialog used to be a `useState` inside `LeftPanel`'s `ChatStripPanel`, which +// mounts only in Edit mode with the chat panel expanded. Nothing outside that component could +// open the dialog, so the app menu had no way to offer it (issue #420). The surfaces that open +// a dialog and the place it is mounted are on different branches of the tree. +// +// One `section` rather than a boolean per dialog. The settings unification this is the first +// step of ends in a single dialog with a sidebar (General / Shortcuts / AI / Devices / About); +// each section it gains is then a member of this union, not another context, another provider +// wrapped around App.tsx's editor branch and another near-identical file. +// +// 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. +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 +// state: mounted, nothing open. +const SectionContext = createContext(undefined); +const ActionsContext = createContext(null); + +export function useEditorDialogSection(): EditorDialogSection | null { + const section = useContext(SectionContext); + if (section === undefined) { + throw new Error("useEditorDialogSection must be used within "); + } + return section; +} + +export function useEditorDialogActions(): EditorDialogsActions { + const ctx = useContext(ActionsContext); + if (!ctx) throw new Error("useEditorDialogActions must be used within "); + return ctx; +} + +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 during render rather than from an effect: a keystroke arriving + // between the state change and the effect would otherwise get the previous answer. + const sectionRef = useRef(section); + sectionRef.current = section; + + const actions = useMemo( + () => ({ + openDialog: (next) => setSection(next), + closeDialog: () => setSection(null), + isDialogOpen: () => sectionRef.current !== null, + }), + [], + ); + + return ( + + {children} + + ); +} diff --git a/src/contexts/ProviderSettingsContext.tsx b/src/contexts/ProviderSettingsContext.tsx deleted file mode 100644 index db4d74d30..000000000 --- a/src/contexts/ProviderSettingsContext.tsx +++ /dev/null @@ -1,41 +0,0 @@ -import { createContext, type ReactNode, useCallback, useContext, useMemo, useState } from "react"; - -// Open/close state for the AI provider settings dialog, lifted out of the chat panel. -// -// It used to be a `useState` inside `LeftPanel`'s `ChatStripPanel`, which mounts only in Edit -// mode with the chat panel expanded. Nothing outside that component could open the dialog, so -// the app menu had no way to offer it (issue #420). The state lives here for the same reason -// `ShortcutsContext` owns `isConfigOpen`: the surfaces that open a dialog and the place it is -// mounted are on different branches of the tree. -// -// Deliberately just the three members. Everything the dialog itself needs — the provider -// snapshot, the credentials, the save — it reads over the native bridge on open. -interface ProviderSettingsContextValue { - isProviderSettingsOpen: boolean; - openProviderSettings: () => void; - closeProviderSettings: () => void; -} - -const ProviderSettingsContext = createContext(null); - -export function useProviderSettings(): ProviderSettingsContextValue { - const ctx = useContext(ProviderSettingsContext); - if (!ctx) throw new Error("useProviderSettings must be used within "); - return ctx; -} - -export function ProviderSettingsProvider({ children }: { children: ReactNode }) { - const [isProviderSettingsOpen, setIsProviderSettingsOpen] = useState(false); - - const openProviderSettings = useCallback(() => setIsProviderSettingsOpen(true), []); - const closeProviderSettings = useCallback(() => setIsProviderSettingsOpen(false), []); - - const value = useMemo( - () => ({ isProviderSettingsOpen, openProviderSettings, closeProviderSettings }), - [isProviderSettingsOpen, openProviderSettings, closeProviderSettings], - ); - - return ( - {children} - ); -} diff --git a/technical-documentation/architecture/llm-providers.md b/technical-documentation/architecture/llm-providers.md index d727ac320..125805b7c 100644 --- a/technical-documentation/architecture/llm-providers.md +++ b/technical-documentation/architecture/llm-providers.md @@ -11,13 +11,14 @@ The provider layer defines model metadata, protects credentials, discovers model | [`electron/ai-edition/deep-agent/chat-model.ts`](../../electron/ai-edition/deep-agent/chat-model.ts) | Per-provider reasoning-effort capability and its LangChain wire options. | | [`electron/native-bridge/services/aiEditionService.ts`](../../electron/native-bridge/services/aiEditionService.ts) | IPC surface: connect / disconnect, snapshot, `llmListProviderModels`. | | [`src/components/ai-edition/ProviderSettings.tsx`](../../src/components/ai-edition/ProviderSettings.tsx) | Renders cards and forms directly from `PROVIDER_DEFINITIONS`. `ProviderSettingsDialog`, in the same file, is its one mount — `App.tsx`, beside `ShortcutsConfigDialog`. | -| [`src/contexts/ProviderSettingsContext.tsx`](../../src/contexts/ProviderSettingsContext.tsx) | Open state for that dialog, so every entry point opens the same one. | +| [`src/contexts/EditorDialogsContext.tsx`](../../src/contexts/EditorDialogsContext.tsx) | Which editor dialog is open (`section`), so every entry point opens the same one. | > **Two doors, one dialog.** The settings dialog is opened from the AI panel — its gear, the -> welcome card's CTA, the model pill with nothing configured, and a send attempt with no -> provider — and from the app menu under the wordmark. Its open state lives in a context rather -> than in `LeftPanel` because that panel mounts only in Edit mode with the chat panel expanded, -> which would have left the menu item dead in Media and Rec (issue #420). +> welcome card's CTA, the model pill with nothing configured, the quick-pick popover's "full +> settings" row, and a send attempt with no provider — and from the app menu under the +> wordmark. Its open state lives in a context rather than in `LeftPanel` because that panel +> mounts only in Edit mode with the chat panel expanded, which would have left the menu item +> dead in Media and Rec (issue #420). > **There is one transport.** `llm-call.ts` (`streamLlm` / `callLlm`) and > `codex-session.ts` were deleted in 1.8.0 along with the two account-backed From b693bb54bac00cd92b45cd8240bb82c0df424857 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Fri, 21 Aug 2026 00:05:45 +0200 Subject: [PATCH 2/2] fix(editor): restore focus when a modal closes, commit the dialog ref in its openers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on the branch. ModalShell took focus on open but never gave it back, which is the mirror of the bug it was added for: the focused node is the one being unmounted, so closing dropped the keyboard user on document.body. It now captures whatever had focus and restores it on cleanup if that element is still connected. Opening from the app menu captures document.body — the menu unmounts its own row before the dialog mounts — so that case restores nothing, which is what it did before; opening from the AI panel's gear now hands focus back to the gear. EditorDialogsContext wrote sectionRef during render. A discarded render would have left the ref claiming a dialog that never committed, and NewEditorShell's keyboard handler reads that ref to decide whether to suppress a shortcut. The two openers write it beside their setSection instead, which also answers a keystroke landing between the click and the commit. The keyboard suppression itself is now covered. NewEditorShell mounts in jsdom with a preload stub, and the test asserts both directions for `?` and Ctrl+O: routed while nothing is open, suppressed while a dialog is, routed again once it closes. Both suppression cases were confirmed to fail with the guard removed — the first draft of the Ctrl+O case passed either way, because that handler awaits the unsaved-changes prompt before opening the picker and a synchronous assertion ran too early to see it. --- src/components/ai-edition/Modals.tsx | 19 +- .../NewEditorShell.dialogShortcuts.test.tsx | 179 ++++++++++++++++++ src/contexts/EditorDialogsContext.tsx | 19 +- 3 files changed, 206 insertions(+), 11 deletions(-) create mode 100644 src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx diff --git a/src/components/ai-edition/Modals.tsx b/src/components/ai-edition/Modals.tsx index 6ee0200bd..cc077efd1 100644 --- a/src/components/ai-edition/Modals.tsx +++ b/src/components/ai-edition/Modals.tsx @@ -98,12 +98,21 @@ export function ModalShell({ const tc = useScopedT("common"); const dialogRef = useRef(null); useEscape(open && closeOnEscape, onClose); - // Move focus into the dialog as it opens. The app menu closes without restoring focus to - // its trigger — right for a pointer user — so otherwise the opener is left on - // document.body: Tab then walks the editor *behind* the backdrop, and a screen reader is - // never told a dialog appeared. + // Move focus into the dialog as it opens, and hand it back to whatever had it when the + // dialog goes. The app menu closes without restoring focus to its trigger — right for a + // pointer user — so otherwise the opener is left on document.body: Tab then walks the + // editor *behind* the backdrop, and a screen reader is never told a dialog appeared. + // Closing has the mirror problem: the focused node is the one being unmounted. useEffect(() => { - if (open) dialogRef.current?.focus(); + if (!open) return; + // Whatever opened this. Often document.body (the app menu unmounts its own row before + // the dialog mounts), in which case restoring is a no-op; the panel's gear is still + // there, and gets it back. + const opener = document.activeElement; + dialogRef.current?.focus(); + return () => { + if (opener instanceof HTMLElement && opener.isConnected) opener.focus(); + }; }, [open]); if (!open) return null; return ( diff --git a/src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx b/src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx new file mode 100644 index 000000000..c0f3bdad7 --- /dev/null +++ b/src/components/ai-edition/NewEditorShell.dialogShortcuts.test.tsx @@ -0,0 +1,179 @@ +// @vitest-environment jsdom +// The editor's shortcuts are bound on `window` and only skip inputs, textareas and +// contentEditable targets. A modal's own controls are buttons, and the app menu closes without +// restoring focus, so with a dialog open `e.target` is document.body and every shortcut used to +// 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. + +import "@testing-library/jest-dom"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const openConfig = vi.fn(); + +// The shortcuts dialog is one of the two things the guard has to suppress, and its opener is +// the cheapest observable in the whole handler: `?` is the only shortcut that survives the +// `hasProject` gate, so it works without loading a project. +vi.mock("@/contexts/ShortcutsContext", async () => { + const { DEFAULT_SHORTCUTS } = await import("@/lib/shortcuts"); + return { + useShortcuts: () => ({ + shortcuts: DEFAULT_SHORTCUTS, + isMac: false, + isConfigOpen: false, + openConfig, + closeConfig: () => { + /* not exercised here */ + }, + setShortcuts: () => { + /* not exercised here */ + }, + persistShortcuts: () => Promise.resolve(true), + }), + }; +}); + +vi.mock("@/contexts/I18nContext", () => ({ + useI18n: () => ({ + locale: "en", + setLocale: () => { + /* fixed locale */ + }, + }), + useScopedT: () => (key: string) => key, +})); + +import { EditorDialogsProvider, useEditorDialogActions } from "@/contexts/EditorDialogsContext"; +import { NewEditorShell } from "./NewEditorShell"; + +let dialogActions: ReturnType | null = null; + +function CaptureDialogActions() { + dialogActions = useEditorDialogActions(); + return null; +} + +function renderShell() { + return render( + + + + , + ); +} + +/** Shortcuts are bound on `window` and read `e.target`; with a modal open that is the body. */ +function pressOnBody(init: KeyboardEventInit) { + fireEvent.keyDown(document.body, init); +} + +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 = { + onAiEditionChatEvent: () => () => { + /* unsubscribe */ + }, + setTitleBarOverlay: () => { + /* no native titlebar */ + }, + setHasUnsavedChanges: () => { + /* no window close guard */ + }, + onRequestCloseConfirm: () => () => { + /* unsubscribe */ + }, + onRequestSaveBeforeClose: () => () => { + /* unsubscribe */ + }, + sendCloseConfirmResponse: () => { + /* nothing is closing this window */ + }, + // The only two other members the editor tree reaches without optional chaining. Both + // are user-driven, not mount-driven; they are here so a stray call is a no-op rather + // than a crash that reads as a failure of the thing under test. + findRecordingCamera: () => Promise.resolve(null), + preparePreviewAudioTrack: () => Promise.resolve(null), + }; + Element.prototype.scrollTo = () => { + /* no scrolling in jsdom */ + }; + // jsdom ships neither; the stage and the timeline both measure themselves. + (globalThis as unknown as { ResizeObserver?: unknown }).ResizeObserver = class { + observe() { + /* never fires: nothing has a layout in jsdom */ + } + unobserve() { + /* see observe */ + } + disconnect() { + /* see observe */ + } + }; +}); + +afterEach(() => { + cleanup(); + (window as unknown as { electronAPI?: unknown }).electronAPI = undefined; +}); + +describe("NewEditorShell shortcuts, with a dialog over the editor", () => { + it("routes ? to the shortcuts dialog while nothing is open", () => { + renderShell(); + + pressOnBody({ key: "?" }); + + expect(openConfig).toHaveBeenCalledTimes(1); + }); + + it("suppresses ? once a dialog owns the screen, and resumes when it closes", () => { + renderShell(); + + act(() => { + dialogActions?.openDialog("providers"); + }); + pressOnBody({ key: "?" }); + expect(openConfig).not.toHaveBeenCalled(); + + act(() => { + dialogActions?.closeDialog(); + }); + pressOnBody({ key: "?" }); + expect(openConfig).toHaveBeenCalledTimes(1); + }); + + // 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 () => { + renderShell(); + + await act(async () => { + pressOnBody({ key: "o", ctrlKey: true }); + }); + + // 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(); + }); + + it("suppresses Ctrl+O once a dialog owns the screen", async () => { + renderShell(); + + act(() => { + dialogActions?.openDialog("providers"); + }); + await act(async () => { + pressOnBody({ key: "o", ctrlKey: true }); + }); + + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); +}); diff --git a/src/contexts/EditorDialogsContext.tsx b/src/contexts/EditorDialogsContext.tsx index e05885512..4da8d9f9a 100644 --- a/src/contexts/EditorDialogsContext.tsx +++ b/src/contexts/EditorDialogsContext.tsx @@ -50,15 +50,22 @@ 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 during render rather than from an effect: a keystroke arriving - // between the state change and the effect would otherwise get the previous answer. - const sectionRef = useRef(section); - sectionRef.current = section; + // 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) => setSection(next), - closeDialog: () => setSection(null), + openDialog: (next) => { + sectionRef.current = next; + setSection(next); + }, + closeDialog: () => { + sectionRef.current = null; + setSection(null); + }, isDialogOpen: () => sectionRef.current !== null, }), [],