From 2688c0fee32e6e2557dd207f71bf17ab3c3e61e1 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 18 Apr 2026 10:08:25 -0700 Subject: [PATCH] feat(editor): preserve Cmd+B for bold in markdown editor (#802) * feat(editor): preserve Cmd+B for bold in markdown editor Carve out bare Cmd/Ctrl+B from the main-process before-input-event interceptor when the TipTap markdown editor is focused, so its bold keymap can run instead of toggling the left sidebar. Focus state is mirrored from renderer to main via a one-way IPC send, with default-deny resets on crash/navigate/destroy and sender validation so only the main window's webContents can mutate the flag. * fix: add oxlint max-lines disable to createMainWindow.ts --- .claude/skills/review-and-submit/SKILL.md | 17 +- .gitignore | 5 + src/main/window/createMainWindow.test.ts | 336 ++++++++++++++++++ src/main/window/createMainWindow.ts | 62 ++++ src/preload/api-types.d.ts | 1 + src/preload/index.ts | 7 + src/renderer/src/App.tsx | 6 + .../components/editor/RichMarkdownEditor.tsx | 21 ++ 8 files changed, 452 insertions(+), 3 deletions(-) diff --git a/.claude/skills/review-and-submit/SKILL.md b/.claude/skills/review-and-submit/SKILL.md index 613df788..e9794d48 100644 --- a/.claude/skills/review-and-submit/SKILL.md +++ b/.claude/skills/review-and-submit/SKILL.md @@ -131,7 +131,18 @@ fi ## Step 2: Create PR and Merge -### 2a. Create PR +### 2a. Remove local design docs + +Design/planning docs are local-only scratch files and must not be checked in. +Delete any untracked markdown files in `docs/` that match design/plan +naming patterns before pushing: + +```bash +git ls-files --others --exclude-standard -- 'docs/*-design.md' 'docs/*-plan.md' 'docs/design-*.md' \ + | xargs -I{} rm -f {} +``` + +### 2b. Create PR **FIRST**: Push the branch to the remote so `gh pr create` doesn't fail with `aborted: you must first push the current branch to a remote`. The @@ -154,7 +165,7 @@ After `/create-pr` completes, extract PR info: gh pr view --json number,url --jq '.number, .url' ``` -### 2b. Wait for CI Checks +### 2c. Wait for CI Checks Wait **2.5 minutes** before the first poll: @@ -188,7 +199,7 @@ fi - `PENDING` → Wait 90s (`sleep 90`, `timeout: 150000`), then poll again - `NO_CHECKS` → Proceed to merge -### 2c. Merge +### 2d. Merge ```bash IS_WORKTREE=false diff --git a/.gitignore b/.gitignore index 63a50e64..83b0cfd2 100644 --- a/.gitignore +++ b/.gitignore @@ -49,6 +49,11 @@ tmp/ design-docs/ .context/ +# Local-only design/planning docs (not checked in) +docs/*-design.md +docs/*-plan.md +docs/design-*.md + # Stably CLI (only docs/ are tracked) .stably/* !.stably/docs/ diff --git a/src/main/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts index 6cf432b6..e9d79417 100644 --- a/src/main/window/createMainWindow.test.ts +++ b/src/main/window/createMainWindow.test.ts @@ -500,6 +500,342 @@ describe('createMainWindow', () => { expect(browserWindowInstance.setWindowButtonPosition).not.toHaveBeenCalled() }) + it('intercepts Cmd+B for sidebar when the markdown editor is not focused', () => { + const windowHandlers: Record void> = {} + const webContents = { + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setBackgroundThrottling: vi.fn(), + invalidate: vi.fn(), + setWindowOpenHandler: vi.fn(), + send: vi.fn(), + isDevToolsOpened: vi.fn(), + openDevTools: vi.fn(), + closeDevTools: vi.fn() + } + const browserWindowInstance = { + webContents, + on: vi.fn(), + isDestroyed: vi.fn(() => false), + isMaximized: vi.fn(() => true), + isFullScreen: vi.fn(() => false), + getSize: vi.fn(() => [1200, 800]), + setSize: vi.fn(), + maximize: vi.fn(), + show: vi.fn(), + loadFile: vi.fn(), + loadURL: vi.fn() + } + browserWindowMock.mockImplementation(function () { + return browserWindowInstance + }) + + createMainWindow(null) + + const preventDefault = vi.fn() + const isDarwin = process.platform === 'darwin' + windowHandlers['before-input-event']( + { preventDefault } as never, + { + type: 'keyDown', + code: 'KeyB', + key: 'b', + meta: isDarwin, + control: !isDarwin, + alt: false, + shift: false + } as never + ) + + expect(preventDefault).toHaveBeenCalledTimes(1) + expect(webContents.send).toHaveBeenCalledWith('ui:toggleLeftSidebar') + }) + + it('skips Cmd+B interception when the markdown editor is focused', () => { + const windowHandlers: Record void> = {} + const webContents = { + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setBackgroundThrottling: vi.fn(), + invalidate: vi.fn(), + setWindowOpenHandler: vi.fn(), + send: vi.fn(), + isDevToolsOpened: vi.fn(), + openDevTools: vi.fn(), + closeDevTools: vi.fn() + } + const browserWindowInstance = { + webContents, + on: vi.fn(), + isDestroyed: vi.fn(() => false), + isMaximized: vi.fn(() => true), + isFullScreen: vi.fn(() => false), + getSize: vi.fn(() => [1200, 800]), + setSize: vi.fn(), + maximize: vi.fn(), + show: vi.fn(), + loadFile: vi.fn(), + loadURL: vi.fn() + } + browserWindowMock.mockImplementation(function () { + return browserWindowInstance + }) + + createMainWindow(null) + + const setFocusedListener = vi + .mocked(ipcMain.on) + .mock.calls.find(([channel]) => channel === 'ui:setMarkdownEditorFocused')?.[1] + expect(setFocusedListener).toBeTypeOf('function') + setFocusedListener?.({ sender: webContents } as never, true) + + const preventDefault = vi.fn() + const isDarwin = process.platform === 'darwin' + windowHandlers['before-input-event']( + { preventDefault } as never, + { + type: 'keyDown', + code: 'KeyB', + key: 'b', + meta: isDarwin, + control: !isDarwin, + alt: false, + shift: false + } as never + ) + + expect(preventDefault).not.toHaveBeenCalled() + expect(webContents.send).not.toHaveBeenCalledWith('ui:toggleLeftSidebar') + }) + + it('still intercepts Cmd+Shift+B and Cmd+Alt+B when the markdown editor is focused', () => { + const windowHandlers: Record void> = {} + const webContents = { + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setBackgroundThrottling: vi.fn(), + invalidate: vi.fn(), + setWindowOpenHandler: vi.fn(), + send: vi.fn(), + isDevToolsOpened: vi.fn(), + openDevTools: vi.fn(), + closeDevTools: vi.fn() + } + const browserWindowInstance = { + webContents, + on: vi.fn(), + isDestroyed: vi.fn(() => false), + isMaximized: vi.fn(() => true), + isFullScreen: vi.fn(() => false), + getSize: vi.fn(() => [1200, 800]), + setSize: vi.fn(), + maximize: vi.fn(), + show: vi.fn(), + loadFile: vi.fn(), + loadURL: vi.fn() + } + browserWindowMock.mockImplementation(function () { + return browserWindowInstance + }) + + createMainWindow(null) + + const setFocusedListener = vi + .mocked(ipcMain.on) + .mock.calls.find(([channel]) => channel === 'ui:setMarkdownEditorFocused')?.[1] + setFocusedListener?.({ sender: webContents } as never, true) + + const isDarwin = process.platform === 'darwin' + + // Cmd+Shift+B is not in the policy allowlist, so no action resolves and no + // preventDefault fires — but the carve-out must not be what lets it through. + const shiftPreventDefault = vi.fn() + windowHandlers['before-input-event']( + { preventDefault: shiftPreventDefault } as never, + { + type: 'keyDown', + code: 'KeyB', + key: 'B', + meta: isDarwin, + control: !isDarwin, + alt: false, + shift: true + } as never + ) + expect(shiftPreventDefault).not.toHaveBeenCalled() + + // Cmd+Alt+B is not a modifier chord in the policy (alt excluded), so the + // policy returns null and no preventDefault fires. Assert the carve-out + // is not what's short-circuiting this — it requires !alt. + const altPreventDefault = vi.fn() + windowHandlers['before-input-event']( + { preventDefault: altPreventDefault } as never, + { + type: 'keyDown', + code: 'KeyB', + key: 'b', + meta: isDarwin, + control: !isDarwin, + alt: true, + shift: false + } as never + ) + expect(altPreventDefault).not.toHaveBeenCalled() + }) + + it('coerces non-boolean setMarkdownEditorFocused payloads to false', () => { + const windowHandlers: Record void> = {} + const webContents = { + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setBackgroundThrottling: vi.fn(), + invalidate: vi.fn(), + setWindowOpenHandler: vi.fn(), + send: vi.fn(), + isDevToolsOpened: vi.fn(), + openDevTools: vi.fn(), + closeDevTools: vi.fn() + } + const browserWindowInstance = { + webContents, + on: vi.fn(), + isDestroyed: vi.fn(() => false), + isMaximized: vi.fn(() => true), + isFullScreen: vi.fn(() => false), + getSize: vi.fn(() => [1200, 800]), + setSize: vi.fn(), + maximize: vi.fn(), + show: vi.fn(), + loadFile: vi.fn(), + loadURL: vi.fn() + } + browserWindowMock.mockImplementation(function () { + return browserWindowInstance + }) + + createMainWindow(null) + + const setFocusedListener = vi + .mocked(ipcMain.on) + .mock.calls.find(([channel]) => channel === 'ui:setMarkdownEditorFocused')?.[1] + + // Seed to true with a legitimate payload, then send a non-boolean and + // assert the flag returns to false by checking Cmd+B resumes interception. + setFocusedListener?.({ sender: webContents } as never, true) + setFocusedListener?.({ sender: webContents } as never, { malicious: true } as never) + + const preventDefault = vi.fn() + const isDarwin = process.platform === 'darwin' + windowHandlers['before-input-event']( + { preventDefault } as never, + { + type: 'keyDown', + code: 'KeyB', + key: 'b', + meta: isDarwin, + control: !isDarwin, + alt: false, + shift: false + } as never + ) + + expect(preventDefault).toHaveBeenCalledTimes(1) + expect(webContents.send).toHaveBeenCalledWith('ui:toggleLeftSidebar') + }) + + it('resets the markdown editor focus flag on renderer crash, navigation, and destroy', () => { + const windowHandlers: Record void> = {} + const webContents = { + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setBackgroundThrottling: vi.fn(), + invalidate: vi.fn(), + setWindowOpenHandler: vi.fn(), + send: vi.fn(), + isDevToolsOpened: vi.fn(), + openDevTools: vi.fn(), + closeDevTools: vi.fn() + } + const browserWindowInstance = { + webContents, + on: vi.fn(), + isDestroyed: vi.fn(() => false), + isMaximized: vi.fn(() => true), + isFullScreen: vi.fn(() => false), + getSize: vi.fn(() => [1200, 800]), + setSize: vi.fn(), + maximize: vi.fn(), + show: vi.fn(), + loadFile: vi.fn(), + loadURL: vi.fn() + } + browserWindowMock.mockImplementation(function () { + return browserWindowInstance + }) + + createMainWindow(null) + + const setFocusedListener = vi + .mocked(ipcMain.on) + .mock.calls.find(([channel]) => channel === 'ui:setMarkdownEditorFocused')?.[1] + const isDarwin = process.platform === 'darwin' + + const cmdBInput = { + type: 'keyDown', + code: 'KeyB', + key: 'b', + meta: isDarwin, + control: !isDarwin, + alt: false, + shift: false + } as never + + const assertInterceptsAfterReset = (): void => { + webContents.send.mockClear() + const preventDefault = vi.fn() + windowHandlers['before-input-event']({ preventDefault } as never, cmdBInput) + expect(preventDefault).toHaveBeenCalledTimes(1) + expect(webContents.send).toHaveBeenCalledWith('ui:toggleLeftSidebar') + } + + // render-process-gone + setFocusedListener?.({ sender: webContents } as never, true) + windowHandlers['render-process-gone']?.() + assertInterceptsAfterReset() + + // did-start-navigation (main frame) + setFocusedListener?.({ sender: webContents } as never, true) + windowHandlers['did-start-navigation']?.({} as never, 'https://example.com/', false, true) + assertInterceptsAfterReset() + + // did-start-navigation (sub-frame) should NOT reset the flag + setFocusedListener?.({ sender: webContents } as never, true) + windowHandlers['did-start-navigation']?.({} as never, 'https://example.com/', false, false) + webContents.send.mockClear() + const subframePreventDefault = vi.fn() + windowHandlers['before-input-event']( + { preventDefault: subframePreventDefault } as never, + cmdBInput + ) + expect(subframePreventDefault).not.toHaveBeenCalled() + expect(webContents.send).not.toHaveBeenCalledWith('ui:toggleLeftSidebar') + + // destroyed + setFocusedListener?.({ sender: webContents } as never, true) + windowHandlers['destroyed']?.() + assertInterceptsAfterReset() + }) + it('ignores duplicate ready-to-show events after startup maximize has already run', () => { const windowHandlers: Record void> = {} const webContents = { diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index dc8b5afa..8b5498ca 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -1,3 +1,4 @@ +/* oxlint-disable max-lines */ import { BrowserWindow, ipcMain, nativeTheme, screen, shell } from 'electron' import { join } from 'path' import { is } from '@electron-toolkit/utils' @@ -262,6 +263,44 @@ export function createMainWindow( event.preventDefault() }) + // Why: mirrors the renderer's markdown-editor focus state so the main-process + // before-input-event handler can skip Cmd/Ctrl+B interception while TipTap + // owns focus. See docs/markdown-cmd-b-bold-design.md. We only carve out + // Cmd+B — terminal and browser-guest focus still get sidebar-toggle, which + // preserves the ^B-to-PTY leak protection rationale in + // shared/window-shortcut-policy.ts:74-77. + let markdownEditorFocused = false + + const markdownFocusChannel = 'ui:setMarkdownEditorFocused' + // Why: coerce to strict boolean and verify the sender. A renderer bug or + // compromised IPC payload must not set the flag to a truthy non-bool (e.g. + // an object) and silently disable the sidebar toggle — default-deny on any + // non-bool. Additionally, only this main window's top-level webContents may + // mutate the flag, so a guest/webview or unrelated sender can't disable the + // Cmd+B sidebar carve-out. + const onMarkdownEditorFocused = (event: Electron.IpcMainEvent, focused: unknown): void => { + if (event.sender !== mainWindow.webContents) { + return + } + markdownEditorFocused = focused === true + } + ipcMain.on(markdownFocusChannel, onMarkdownEditorFocused) + + // Why: renderer can't mirror focus state across a crash/reload/close. + // Default-deny the carve-out so Cmd+B falls back to sidebar-toggle, which is + // the safe behavior when focus context is unknown. Preserves the + // ^B-to-PTY leak invariant from shared/window-shortcut-policy.ts:74-77. + const resetMarkdownEditorFocus = (): void => { + markdownEditorFocused = false + } + mainWindow.webContents.on('render-process-gone', resetMarkdownEditorFocus) + mainWindow.webContents.on('destroyed', resetMarkdownEditorFocus) + mainWindow.webContents.on('did-start-navigation', (_e, _url, _isInPlace, isMainFrame) => { + if (isMainFrame) { + resetMarkdownEditorFocus() + } + }) + mainWindow.webContents.on('before-input-event', (event, input) => { if (input.type !== 'keyDown') { return @@ -277,6 +316,24 @@ export function createMainWindow( return } + // Why: TipTap owns bare Cmd/Ctrl+B for bold while the markdown editor is + // focused — skip interception so its keymap runs. Scoped to the bare chord + // (no Shift/Alt): any extra modifier signals different intent and must + // still resolve through the policy allowlist. Other focus contexts + // (terminal, browser guest) still get sidebar-toggle because ^B would + // otherwise reach xterm.js / guest webContents. + // See docs/markdown-cmd-b-bold-design.md. + const modForBold = process.platform === 'darwin' ? input.meta : input.control + if ( + markdownEditorFocused && + input.code === 'KeyB' && + !input.alt && + !input.shift && + modForBold + ) { + return + } + // Why: keep the main-process interception surface as an explicit allowlist. // Anything outside this helper must continue to the renderer/PTTY so // readline control chords are not silently stolen above the terminal. @@ -369,8 +426,13 @@ export function createMainWindow( ipcMain.on(confirmCloseChannel, onConfirmClose) mainWindow.on('closed', () => { + // Why: default-deny the Cmd+B carve-out after the window is gone so a + // stale-true flag can't leak past subsequent state transitions. Paired + // with the webContents lifecycle resets above. + markdownEditorFocused = false ipcMain.removeListener(trafficLightChannel, onSyncTrafficLights) ipcMain.removeListener(confirmCloseChannel, onConfirmClose) + ipcMain.removeListener(markdownFocusChannel, onMarkdownEditorFocused) }) if (is.dev && process.env.ELECTRON_RENDERER_URL) { diff --git a/src/preload/api-types.d.ts b/src/preload/api-types.d.ts index d3ed60ff..b0da0bea 100644 --- a/src/preload/api-types.d.ts +++ b/src/preload/api-types.d.ts @@ -602,6 +602,7 @@ export type PreloadApi = { getZoomLevel: () => number setZoomLevel: (level: number) => void syncTrafficLights: (zoomFactor: number) => void + setMarkdownEditorFocused: (focused: boolean) => void onFullscreenChanged: (callback: (isFullScreen: boolean) => void) => () => void onWindowCloseRequested: (callback: (data: { isQuitting: boolean }) => void) => () => void confirmWindowClose: () => void diff --git a/src/preload/index.ts b/src/preload/index.ts index 9c1c7bb5..0d4d2272 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1129,6 +1129,13 @@ const api = { setZoomLevel: (level: number): void => webFrame.setZoomLevel(level), syncTrafficLights: (zoomFactor: number): void => ipcRenderer.send('ui:sync-traffic-lights', zoomFactor), + // Why: one-way send (not invoke) so the main-process before-input-event + // handler can read the mirrored flag synchronously without a round-trip. + // The carve-out in createMainWindow.ts uses this to skip Cmd+B interception + // while the markdown editor owns focus, letting TipTap apply bold instead. + setMarkdownEditorFocused: (focused: boolean): void => { + ipcRenderer.send('ui:setMarkdownEditorFocused', focused) + }, onFullscreenChanged: (callback: (isFullScreen: boolean) => void): (() => void) => { const listener = (_event: Electron.IpcRendererEvent, isFullScreen: boolean) => callback(isFullScreen) diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 7b218ffc..646c32fd 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -494,6 +494,12 @@ function App(): React.JSX.Element { // browser guest has focus. The renderer keeps matching handlers for // local-focus cases and to preserve the same guards in one place. + // Why: keep this guard. TipTap's Cmd+B bold binding depends on the + // window-level handler *not* toggling the sidebar when focus lives in an + // editable surface. The main-process before-input-event already carves out + // Cmd+B for the markdown editor (see createMainWindow.ts + + // docs/markdown-cmd-b-bold-design.md), but this renderer-side fallback + // still covers the blur→press IPC race and any non-carved editable surface. if (isEditableTarget(e.target)) { return } diff --git a/src/renderer/src/components/editor/RichMarkdownEditor.tsx b/src/renderer/src/components/editor/RichMarkdownEditor.tsx index b9e14fe0..8ce356a0 100644 --- a/src/renderer/src/components/editor/RichMarkdownEditor.tsx +++ b/src/renderer/src/components/editor/RichMarkdownEditor.tsx @@ -162,6 +162,16 @@ export default function RichMarkdownEditor({ return false } }, + onFocus: () => { + // Why: mirror TipTap focus into the main process so the before-input-event + // Cmd+B carve-out in createMainWindow.ts lets the bold keymap run instead + // of intercepting the chord for sidebar toggle. + // See docs/markdown-cmd-b-bold-design.md. + window.api.ui.setMarkdownEditorFocused(true) + }, + onBlur: () => { + window.api.ui.setMarkdownEditorFocused(false) + }, onCreate: ({ editor: nextEditor }) => { // Why: markdown soft line breaks produce paragraphs with embedded `\n` chars. // Normalizing them into separate paragraph nodes on load ensures Cmd+X (and @@ -234,6 +244,17 @@ export default function RichMarkdownEditor({ editorRef.current = editor ?? null }, [editor]) + // Why: TipTap's onBlur may not fire on unmount paths (tab close, HMR, + // component teardown while focused), leaving the main-process flag stale at + // `true` and silently disabling Cmd+B sidebar-toggle until the next editor + // focus/blur cycle. Force a `false` on unmount as a belt-and-braces reset. + // See docs/markdown-cmd-b-bold-design.md "Stale-flag recovery". + useEffect(() => { + return () => { + window.api.ui.setMarkdownEditorFocused(false) + } + }, []) + // Why: use useLayoutEffect (synchronous cleanup) so the pending serialization // flush runs before useEditor's cleanup destroys the editor instance on tab // switch or mode change. React runs layout-effect cleanups before effect