diff --git a/docs/architecture/04-routing-and-events.md b/docs/architecture/04-routing-and-events.md index 647c8aa..3b14dd3 100644 --- a/docs/architecture/04-routing-and-events.md +++ b/docs/architecture/04-routing-and-events.md @@ -320,7 +320,17 @@ function onKeyDown(e: KeyboardEvent): void { const mod = isMac ? e.metaKey : e.ctrlKey; - // --- Shortcuts: never fire while typing in an input or Monaco ---------- + // Cmd/Ctrl + S -> publish current draft. Checked BEFORE the interactive-context + // gate: it is the canonical "save" shortcut (spec §01D mandates it override the + // browser default), and publishing happens *while editing the draft in Monaco* — + // gating it behind "not typing" would defeat its purpose. + if (mod && !e.shiftKey && e.key.toLowerCase() === 's') { + e.preventDefault(); // override the browser "save page" dialog + useSnippetStore.getState().publishDraft(); + return; + } + + // --- Remaining shortcuts: never fire while typing in an input or Monaco ---- if (isInInteractiveContext()) return; // Cmd/Ctrl + Shift + N -> new snippet @@ -336,16 +346,12 @@ function onKeyDown(e: KeyboardEvent): void { toggleDatasets(); return; } - // Cmd/Ctrl + S -> publish current draft - if (mod && e.key.toLowerCase() === 's') { - e.preventDefault(); // override the browser "save page" dialog - useSnippetStore.getState().publishDraft(); - return; - } - // Cmd/Ctrl + , -> settings (through the coordinator: snapshot + URL sync) + // Cmd/Ctrl + , -> open the editor settings popover. Settings are distributed to + // per-pane disclosure popovers, not a modal (spec §07), so this opens the editor + // cluster (openSettingsPopover) — there is no 'settings' modal to open. if (mod && e.key === ',') { e.preventDefault(); - openModal('settings'); + openSettingsPopover('editor-settings'); return; } } @@ -392,9 +398,13 @@ by inserting a rung at the right priority — never by sprinkling `e.preventDefault()` so Cmd/Ctrl+S does not trigger "save page", Cmd/Ctrl+K does not focus the browser search bar, etc. -Note the asymmetry: **Escape is checked before the interactive-context gate** -(you want Escape to dismiss a modal even while focus is in the editor), whereas -all other shortcuts are checked **after** the gate (so they don't fire mid-typing). +Note the asymmetry: **Escape and Cmd/Ctrl+S are checked before the +interactive-context gate.** Escape, so it dismisses a modal even while focus is in +the editor; Cmd/Ctrl+S, because publishing the draft is something you do _while_ +editing it — gating "save" behind "not typing" would defeat it (and it must +override the browser's "save page" regardless). The remaining shortcuts (new +snippet, toggle Datasets, settings) are checked **after** the gate, so they never +fire mid-typing or steal a key the editor wants (e.g. Monaco's own Cmd+K chord). ### 2.2 The single-source helper: `orchestration/focus-utils.ts` diff --git a/docs/architecture/10-interaction-and-feedback.md b/docs/architecture/10-interaction-and-feedback.md index 1400a7e..897a9fc 100644 --- a/docs/architecture/10-interaction-and-feedback.md +++ b/docs/architecture/10-interaction-and-feedback.md @@ -65,6 +65,13 @@ nature of the message, not by convenience. _inline on the control_ ("Copied") with a polite `aria-live` announcement for assistive tech, never a toast-per-copy. This refines the spec's earlier blanket "every action toasts" (spec §01F/§02/§05, reconciled). +- **An action's confirmation belongs with the action, not its call site.** When an action is + reachable from more than one trigger (e.g. publish is both a toolbar button _and_ + Cmd/Ctrl+S), pair the store mutation and its toast in **one `services/` helper** + (`publishActiveSnippet`) that every trigger calls — otherwise the toast rides one path and + the other publishes silently (the exact inconsistency this rule prevents). The shortcut is + owned globally by the EventRouter (arch 04), so the helper is the only place the outcome is + confirmed. - **The same failure can light up two channels.** An unrenderable spec shows the _same_ message inline in both the editor (§03E) and the preview (§04) — one producer (`PreviewStore`), two subscribers. That's intentional, not duplication. diff --git a/docs/spec/01-application-shell.md b/docs/spec/01-application-shell.md index 4be85b5..f255085 100644 --- a/docs/spec/01-application-shell.md +++ b/docs/spec/01-application-shell.md @@ -67,6 +67,10 @@ Notes: - Cmd/Ctrl + K is a toggle: if the Datasets manager is already open it closes it; otherwise it opens it. - Escape only acts when a modal is open; with no modal open it does nothing. +- Cmd/Ctrl + S publishes the active draft regardless of where focus is — **including + while the editor has focus** — so it behaves as a "save" you reach for mid-edit. The + other shortcuts (new snippet, toggle Datasets, settings) are suppressed while the + user is typing in the editor or an input, so they don't interrupt text entry. - The shortcut actions override the browser's default behavior for those key combinations. ## E. Navigation & Shareable URL State diff --git a/src/app/App.tsx b/src/app/App.tsx index 0ba5cf8..3ff645a 100644 --- a/src/app/App.tsx +++ b/src/app/App.tsx @@ -8,8 +8,7 @@ import { SnippetLibrary } from './components/SnippetLibrary'; import { SpecEditor } from './components/SpecEditor'; import { ThemeToggle } from './components/ThemeToggle'; import { Toaster } from './components/Toaster'; -import { openModal, setConfirm, toggleDatasets } from './modals/ModalCoordinator'; -import { openSettingsPopover } from './stores/SettingsPopoverStore'; +import { openModal, setConfirm } from './modals/ModalCoordinator'; import { confirm } from './stores/ConfirmStore'; import { usePanesStore } from './stores/PanesStore'; import { exportWorkspace, importWorkspace } from './services/transfer'; @@ -47,24 +46,6 @@ export function App() { setConfirm((message) => confirm({ title: 'Discard changes?', message, danger: true })); }, []); - // Cmd/Ctrl+K toggles the Datasets manager (spec §05); Cmd/Ctrl+, opens the - // editor settings popover — the primary preference cluster, now that settings - // are distributed per pane (spec §07). Full key router lands in M6 (arch 04). - useEffect(() => { - const onKey = (e: KeyboardEvent) => { - if (!(e.metaKey || e.ctrlKey)) return; - if (e.key === 'k' || e.key === 'K') { - e.preventDefault(); - toggleDatasets(); - } else if (e.key === ',') { - e.preventDefault(); - openSettingsPopover('editor-settings'); - } - }; - window.addEventListener('keydown', onKey); - return () => window.removeEventListener('keydown', onKey); - }, []); - const handleImportFile = (e: React.ChangeEvent) => { const file = e.target.files?.[0]; // Reset the input so picking the same file again still fires onChange. diff --git a/src/app/components/SpecEditor.tsx b/src/app/components/SpecEditor.tsx index c42b181..0eb9f43 100644 --- a/src/app/components/SpecEditor.tsx +++ b/src/app/components/SpecEditor.tsx @@ -27,6 +27,7 @@ import { openModal } from '../modals/ModalCoordinator'; import { useAppStore } from '../stores/AppStore'; import { confirm } from '../stores/ConfirmStore'; import { hasInlineData } from '../stores/ExtractStore'; +import { publishActiveSnippet } from '../services/snippet-actions'; import { notify } from '../stores/NotificationStore'; import { usePreviewStore } from '../stores/PreviewStore'; import { selectActiveSnippet, selectShownText, useSnippetStore } from '../stores/SnippetStore'; @@ -152,18 +153,9 @@ function EditorToolbar() { (s) => s.activeSnippetId !== null && hasInlineData(s.draftText), ); - const handlePublish = () => { - if (!useSnippetStore.getState().activeSnippetId) return; - useSnippetStore.getState().publish(); - // Success confirmation (spec §03D). Per the council's toast-copy rule - // (docs/architecture/10 → Toast copy), the title states the action and the - // message adds the consequence rather than paraphrasing it. - notify({ - kind: 'success', - title: 'Snippet published', - message: 'Your draft is now the published version.', - }); - }; + // Publish + its success toast live in one place (services/snippet-actions) so + // the button and the Cmd/Ctrl+S shortcut (EventRouter) behave identically. + const handlePublish = publishActiveSnippet; const handleRevert = async () => { const ok = await confirm({ @@ -275,10 +267,10 @@ export function SpecEditor() { } }); - // Cmd/Ctrl+S publishes the current draft (spec §03D → Publish). - editor.addCommand(monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyS, () => { - if (useSnippetStore.getState().activeSnippetId) useSnippetStore.getState().publish(); - }); + // Cmd/Ctrl+S is owned globally by the EventRouter (docs/architecture/04 → + // "bind listeners in exactly one place"), which publishes before the + // interactive-context gate so it works while the editor has focus. Monaco + // binds no default for Cmd+S, so the keystroke bubbles to that one handler. return () => { sub.dispose(); diff --git a/src/app/orchestration/EventRouter.test.ts b/src/app/orchestration/EventRouter.test.ts new file mode 100644 index 0000000..295d834 --- /dev/null +++ b/src/app/orchestration/EventRouter.test.ts @@ -0,0 +1,94 @@ +import { afterEach, beforeEach, describe, expect, test } from 'vitest'; +import { createSnippet } from '@core/snippet'; +import { useAppStore } from '../stores/AppStore'; +import { useSnippetStore } from '../stores/SnippetStore'; +import { startEventRouter, stopEventRouter } from './EventRouter'; + +const T = new Date('2026-06-01T00:00:00Z'); +const active = () => useSnippetStore.getState(); + +/** Dispatch a global keydown with both modifiers set so it fires on any platform. */ +function press(key: string, opts: Partial = {}): void { + window.dispatchEvent( + new KeyboardEvent('keydown', { + key, + bubbles: true, + cancelable: true, + ctrlKey: true, + metaKey: true, + ...opts, + }), + ); +} + +beforeEach(() => { + useSnippetStore.getState().reset(); + useAppStore.getState().setActiveModal(null); + document.body.innerHTML = ''; + window.location.hash = ''; + useSnippetStore.getState().hydrate([createSnippet({ id: 'a', now: T })], 'a'); + startEventRouter(); +}); + +afterEach(() => { + stopEventRouter(); +}); + +describe('shortcut dispatch (spec §01D)', () => { + test('Cmd/Ctrl+Shift+N creates and activates a new snippet', () => { + expect(active().snippets).toHaveLength(1); + press('n', { shiftKey: true }); + expect(active().snippets).toHaveLength(2); + expect(active().activeSnippetId).not.toBe('a'); + }); + + test('Cmd/Ctrl+K toggles the Datasets manager open then closed', async () => { + press('k'); + expect(useAppStore.getState().activeModal).toBe('datasets'); + press('k'); + await Promise.resolve(); // closeModal is async (no unsaved changes → no prompt) + expect(useAppStore.getState().activeModal).toBeNull(); + }); + + test('Cmd/Ctrl+S publishes the active draft', () => { + active().updateDraft('{"published":1}'); + press('s'); + expect(active().snippets.find((s) => s.id === 'a')?.spec).toBe('{"published":1}'); + }); + + test('Escape closes the active modal', async () => { + useAppStore.getState().setActiveModal('datasets'); + press('Escape'); + await Promise.resolve(); + expect(useAppStore.getState().activeModal).toBeNull(); + }); + + test('Escape with no modal open does nothing', () => { + expect(useAppStore.getState().activeModal).toBeNull(); + press('Escape'); // must not throw or change state + expect(useAppStore.getState().activeModal).toBeNull(); + }); +}); + +describe('interactive-context gating', () => { + test('N is suppressed while an input is focused', () => { + const input = document.createElement('input'); + document.body.appendChild(input); + input.focus(); + + press('n', { shiftKey: true }); + + expect(active().snippets).toHaveLength(1); // no new snippet + }); + + test('Cmd/Ctrl+S still publishes while the editor/input is focused (deliberate divergence)', () => { + const input = document.createElement('input'); + document.body.appendChild(input); + input.focus(); + active().updateDraft('{"saved-while-typing":1}'); + + press('s'); + + expect(active().snippets.find((s) => s.id === 'a')?.spec).toBe('{"saved-while-typing":1}'); + }); +}); diff --git a/src/app/orchestration/EventRouter.ts b/src/app/orchestration/EventRouter.ts new file mode 100644 index 0000000..9a83953 --- /dev/null +++ b/src/app/orchestration/EventRouter.ts @@ -0,0 +1,112 @@ +/** + * Global keyboard routing (spec §01D, docs/architecture/04 → "Global Events"). + * + * One module owns the document-level `keydown` listener and dispatches the app's + * shortcuts. Centralizing it keeps ordering explicit and gives one place to + * reason about priority — components never attach their own `window` keydown + * handlers (see the "Don't" list in docs/architecture/04 §2). + * + * The shortcut map (spec §01D), platform-aware (Cmd on Mac, Ctrl elsewhere): + * Cmd/Ctrl + Shift + N → new snippet + * Cmd/Ctrl + K → toggle the Datasets manager + * Cmd/Ctrl + S → publish the active snippet's draft + * Cmd/Ctrl + , → open the editor settings popover + * Escape → close the active modal + * + * Interactive-context gating (the `isInInteractiveContext` seam): + * + * - **Escape** is checked *before* the gate — it must dismiss a modal even while + * focus is in the editor or an input. + * - **Cmd/Ctrl+S** is also checked before the gate. It is the canonical "save" + * shortcut and the spec mandates it override the browser default; publishing is + * something the user does *while editing the draft in Monaco*, so gating it + * behind "not typing" would defeat its purpose. (Deliberate divergence from the + * arch sketch, which placed Cmd+S after the gate.) + * - **N / K / ,** are checked *after* the gate, so they never steal a keystroke + * from the editor or an input (e.g. Monaco's own Cmd+K chord). + */ + +import { closeModal, toggleDatasets } from '../modals/ModalCoordinator'; +import { useAppStore } from '../stores/AppStore'; +import { useSnippetStore } from '../stores/SnippetStore'; +import { openSettingsPopover } from '../stores/SettingsPopoverStore'; +import { publishActiveSnippet } from '../services/snippet-actions'; +import { isInInteractiveContext } from './focus-utils'; + +let started = false; + +const isMac = /Mac|iPhone|iPad|iPod/.test(navigator.platform); + +function modifier(e: KeyboardEvent): boolean { + return isMac ? e.metaKey : e.ctrlKey; +} + +/** Returns true if it consumed the Escape (caller should `preventDefault`). */ +function handleEscape(): boolean { + // Route through the coordinator so the unsaved-change discard prompt runs and + // the URL is cleared. NEVER setActiveModal(null) here — that would silently + // drop in-progress dataset / chart-builder edits. Escape only acts when a + // modal is open; with no modal it does nothing (spec §01D). + if (useAppStore.getState().activeModal) { + void closeModal(); + return true; + } + return false; +} + +function onKeyDown(e: KeyboardEvent): void { + // Escape: highest priority, runs even while the editor / an input has focus. + if (e.key === 'Escape') { + if (handleEscape()) e.preventDefault(); + return; + } + + if (!modifier(e)) return; + const key = e.key.toLowerCase(); + + // Cmd/Ctrl+S: publish. Handled before the interactive-context gate so it works + // while editing the draft, and overrides the browser "save page" dialog. + if (key === 's' && !e.shiftKey) { + e.preventDefault(); + publishActiveSnippet(); + return; + } + + // Everything below must not fire while the user is typing. + if (isInInteractiveContext()) return; + + // Cmd/Ctrl+Shift+N: new snippet (selection drives the URL via the routing + // subscription, so no explicit navigate call is needed). + if (key === 'n' && e.shiftKey) { + e.preventDefault(); + useSnippetStore.getState().createSnippet(); + return; + } + // Cmd/Ctrl+K: toggle the Datasets manager (coordinator owns open/close + URL). + if (key === 'k' && !e.shiftKey) { + e.preventDefault(); + toggleDatasets(); + return; + } + // Cmd/Ctrl+,: open the editor settings popover. Settings are distributed to + // per-pane disclosure popovers, not a modal (spec §07); the editor cluster is + // the primary one (the arch sketch's openModal('settings') predates that). + if (e.key === ',') { + e.preventDefault(); + openSettingsPopover('editor-settings'); + return; + } +} + +/** Bind the global keyboard listener. Idempotent; pair with `stopEventRouter`. */ +export function startEventRouter(): void { + if (started) return; + started = true; + window.addEventListener('keydown', onKeyDown); +} + +/** Unbind the global keyboard listener (teardown for tests). */ +export function stopEventRouter(): void { + window.removeEventListener('keydown', onKeyDown); + started = false; +} diff --git a/src/app/orchestration/focus-utils.test.ts b/src/app/orchestration/focus-utils.test.ts new file mode 100644 index 0000000..1475dbf --- /dev/null +++ b/src/app/orchestration/focus-utils.test.ts @@ -0,0 +1,48 @@ +import { afterEach, describe, expect, test } from 'vitest'; +import { isInInteractiveContext } from './focus-utils'; + +afterEach(() => { + document.body.innerHTML = ''; +}); + +/** Append `el` to the document, focus it, and return it. */ +function mountAndFocus(el: T): T { + document.body.appendChild(el); + el.focus(); + return el; +} + +describe('isInInteractiveContext', () => { + test('false when nothing is focused', () => { + expect(isInInteractiveContext()).toBe(false); + }); + + test('true for a focused ', () => { + mountAndFocus(document.createElement('input')); + expect(isInInteractiveContext()).toBe(true); + }); + + test('true for a focused contenteditable element', () => { + const div = document.createElement('div'); + div.contentEditable = 'true'; + mountAndFocus(div); + expect(isInInteractiveContext()).toBe(true); + }); + + test('true when focus is inside the Monaco editor', () => { + const editor = document.createElement('div'); + editor.className = 'monaco-editor'; + const textarea = document.createElement('textarea'); + textarea.className = 'inputarea'; + editor.appendChild(textarea); + document.body.appendChild(editor); + // Focus an inner node: the .monaco-editor ancestor must still count. + textarea.focus(); + expect(isInInteractiveContext()).toBe(true); + }); + + test('false for a focused