From 81798b4353758d26a6b8af1f3dede657a5f32bbb Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Thu, 17 Sep 2026 09:12:27 -0500 Subject: [PATCH 01/19] Fix(vim): Escape ends a block edit in normal mode After a visual-block I, A or c, Escape applied the text to every row but left Vim in insert mode (#803). CodeMirror runs every keymap from a single DOM handler placed at the first keymap provider, and the editor mounts one (markdown snippets) ahead of vim(), so every keymap binding sees a key before the Vim plugin does. Two Escape bindings were spending it. defaultKeymap binds Escape to simplifySelection, which reports the key handled whenever there is more than one range or a non-empty one. Since block edits became real multi-cursors (#792) that is exactly the state Vim is in when Escape should end the edit: CodeMirror collapsed the cursors and Vim never saw the key. The same binding made visual block need two presses, and left a v or V selection through the selection listener with the cursor on CodeMirror's head instead of the character Vim keeps it on. Escape now defers to Vim in insert, replace and visual mode. Normal mode keeps the native command, because there Vim itself hands Escape back to the editor so stray extra cursors can still collapse. The completion keymap's closeCompletion reports the key handled while any source is merely pending, and every typed character puts all sources into that state for the activateOnTyping debounce (about 100ms) even when none will match. An Escape pressed right after the last character was lost to a completion nobody could see, with a single cursor too: easy to hit with Escape on Caps Lock, and what kept a block insert stuck for fast typists even once the first binding was out of the way. A pending query is still cancelled, but only a visible popup keeps the key. A visible popup still takes the first Escape and stays in insert mode, as before. The #792 suite asserted a return to normal mode and passed falsely: it mounted vim() ahead of any keymap, the reverse of the app, so Vim got Escape first. The mount now mirrors the app's handler order, and with the fix removed nine tests fail. Fixes #803. --- .../src/lib/cm-completion-nav-arrows.test.ts | 70 +++++++++++++ .../app-core/src/lib/cm-completion-nav.ts | 31 +++++- .../src/lib/cm-vim-default-keymap.test.ts | 38 +++++++- .../app-core/src/lib/cm-vim-default-keymap.ts | 49 ++++++++-- .../src/lib/cm-vim-visual-block.test.ts | 97 +++++++++++++++++++ 5 files changed, 274 insertions(+), 11 deletions(-) diff --git a/packages/app-core/src/lib/cm-completion-nav-arrows.test.ts b/packages/app-core/src/lib/cm-completion-nav-arrows.test.ts index 27b75a19..56f606c2 100644 --- a/packages/app-core/src/lib/cm-completion-nav-arrows.test.ts +++ b/packages/app-core/src/lib/cm-completion-nav-arrows.test.ts @@ -2,6 +2,7 @@ import { autocompletion, + completionStatus, currentCompletions, selectedCompletionIndex, startCompletion, @@ -116,3 +117,72 @@ describe('completion arrow navigation', () => { view.destroy() }) }) + +// #803: this keymap runs ahead of the Vim plugin, so whatever it reports as +// handled never reaches Vim. Only a popup the user can see may take Escape. +describe('completion Escape', () => { + function mountWithEscapeProbe(): { view: EditorView; reached: () => number } { + let reached = 0 + const view = new EditorView({ + state: EditorState.create({ + doc: 'line one\n@', + selection: { anchor: 10 }, + extensions: [ + autocompletion({ defaultKeymap: false, override: [source] }), + completionNavKeymap, + completionKeymapExtension, + keymap.of([ + { + key: 'Escape', + run: () => { + reached += 1 + return true + } + } + ]) + ] + }), + parent: document.body + }) + return { view, reached: () => reached } + } + + it('closes a visible popup and keeps the key', async () => { + const { view, reached } = mountWithEscapeProbe() + startCompletion(view) + await settle(view) + + press(view, 'Escape') + + expect(currentCompletions(view.state).length).toBe(0) + expect(reached()).toBe(0) + + view.destroy() + }) + + it('cancels a query that is only pending and lets the key through', () => { + const { view, reached } = mountWithEscapeProbe() + // A typed character marks every source pending for the activateOnTyping + // debounce; nothing is on screen yet. + view.dispatch({ ...view.state.replaceSelection('x'), userEvent: 'input.type' }) + expect(completionStatus(view.state)).toBe('pending') + expect(currentCompletions(view.state).length).toBe(0) + + press(view, 'Escape') + + expect(reached()).toBe(1) + expect(completionStatus(view.state)).toBe(null) + + view.destroy() + }) + + it('lets the key through when nothing is pending or open', () => { + const { view, reached } = mountWithEscapeProbe() + + press(view, 'Escape') + + expect(reached()).toBe(1) + + view.destroy() + }) +}) diff --git a/packages/app-core/src/lib/cm-completion-nav.ts b/packages/app-core/src/lib/cm-completion-nav.ts index b4f56625..cb7fda8b 100644 --- a/packages/app-core/src/lib/cm-completion-nav.ts +++ b/packages/app-core/src/lib/cm-completion-nav.ts @@ -1,7 +1,9 @@ import { acceptCompletion, + closeCompletion, completionKeymap, completionStatus, + currentCompletions, moveCompletionSelection, selectedCompletion } from '@codemirror/autocomplete' @@ -22,9 +24,32 @@ import { EditorView, keymap, type KeyBinding } from '@codemirror/view' * cm-vim-default-keymap.ts.) Use this in place of the raw `completionKeymap`. */ const MAC_TEXT_ENTRY_CHORDS = new Set(['Alt-`', 'Alt-i']) -export const completionKeymapForEditor: readonly KeyBinding[] = completionKeymap.filter( - (binding) => !(typeof binding.mac === 'string' && MAC_TEXT_ENTRY_CHORDS.has(binding.mac)) -) + +/** + * Escape is spent on a completion only when there is a popup to close (#803). + * + * The stock `closeCompletion` reports the key handled whenever any source is + * not inactive, and every typed character puts all sources into "pending" for + * the `activateOnTyping` debounce (about 100ms) even when none of them will + * match. This keymap runs at `Prec.highest`, ahead of the Vim plugin, so an + * Escape pressed right after the last character was swallowed by a completion + * nobody could see and Vim stayed in insert mode: easy to hit with Escape on + * Caps Lock, and what kept a block `I` in insert mode for fast typists. + * + * A pending query is still cancelled, so a popup cannot open after the mode + * has changed, but the key then falls through to whoever owns it. + */ +export function closeVisibleCompletion(view: EditorView): boolean { + const visible = currentCompletions(view.state).length > 0 + closeCompletion(view) + return visible +} + +export const completionKeymapForEditor: readonly KeyBinding[] = completionKeymap + .filter((binding) => !(typeof binding.mac === 'string' && MAC_TEXT_ENTRY_CHORDS.has(binding.mac))) + .map((binding) => + binding.key === 'Escape' ? { ...binding, run: closeVisibleCompletion } : binding + ) /** * Mount this instead of spreading the bindings into an editor's general diff --git a/packages/app-core/src/lib/cm-vim-default-keymap.test.ts b/packages/app-core/src/lib/cm-vim-default-keymap.test.ts index 7695fc2b..73f57b02 100644 --- a/packages/app-core/src/lib/cm-vim-default-keymap.test.ts +++ b/packages/app-core/src/lib/cm-vim-default-keymap.test.ts @@ -1,6 +1,6 @@ // @vitest-environment jsdom import { afterEach, describe, expect, it } from 'vitest' -import { EditorState } from '@codemirror/state' +import { EditorSelection, EditorState } from '@codemirror/state' import { EditorView, keymap, type KeyBinding } from '@codemirror/view' import { vim } from '@replit/codemirror-vim' import { markdown, markdownLanguage } from '@codemirror/lang-markdown' @@ -71,7 +71,12 @@ describe('vim arrow bindings defer to the Vim plugin (issue #287)', () => { const view = new EditorView({ state: EditorState.create({ doc, - extensions: [vim(), keymap.of([...vimAwareDefaultKeymap(true)])] + extensions: [ + vim(), + // The editors enable this for Vim block edits (#792). + EditorState.allowMultipleSelections.of(true), + keymap.of([...vimAwareDefaultKeymap(true)]) + ] }), parent: document.body }) @@ -127,6 +132,35 @@ describe('vim arrow bindings defer to the Vim plugin (issue #287)', () => { expect(arrowRun('Enter')(view)).toBe(true) expect(view.state.doc.toString()).toBe('\nhello') }) + + // #803: Escape is the inverse of the keys above. `simplifySelection` reports + // it handled for several ranges or a non-empty one, which is exactly what a + // block insert and a visual selection look like, so Vim never left the mode. + it('defers Escape in insert and visual mode, where Vim has a mode to leave', () => { + const multi = EditorSelection.create([EditorSelection.cursor(0), EditorSelection.cursor(6)]) + const view = mountVim('hello\nworld') + press(view, 'i', 73) // enter insert mode + view.dispatch({ selection: multi }) + expect(arrowRun('Escape')(view)).toBe(false) + expect(view.state.selection.ranges.length).toBe(2) // left for Vim to collapse + + const visual = mountVim('hello world') + press(visual, 'v', 86) // enter visual mode + expect(arrowRun('Escape')(visual)).toBe(false) + }) + + it('keeps the native Escape in normal mode so stray extra cursors still collapse', () => { + const view = mountVim('hello\nworld') + view.dispatch({ + selection: EditorSelection.create([EditorSelection.cursor(0), EditorSelection.cursor(6)]) + }) + expect(arrowRun('Escape')(view)).toBe(true) + expect(view.state.selection.ranges.length).toBe(1) + }) + + it('drops preventDefault on Escape in Vim mode, or deferring would still consume it', () => { + expect(vimAwareDefaultKeymap(true).find((b) => b.key === 'Escape')?.preventDefault).toBe(false) + }) }) // The markdown language keymap (Enter → insertNewlineContinueMarkup at diff --git a/packages/app-core/src/lib/cm-vim-default-keymap.ts b/packages/app-core/src/lib/cm-vim-default-keymap.ts index 2a2e586b..486662e4 100644 --- a/packages/app-core/src/lib/cm-vim-default-keymap.ts +++ b/packages/app-core/src/lib/cm-vim-default-keymap.ts @@ -110,17 +110,54 @@ function deferKeysToVim( }) } -/** Vim-mode keymap: emacs chords stripped, motion keys made Vim-aware (see above). */ -const vimModeKeymap: readonly KeyBinding[] = deferKeysToVim( - defaultKeymapWithoutMacEmacs, - VIM_MOTION_KEYS +/** + * Escape belongs to Vim while Vim has a mode to leave (#803). + * + * `defaultKeymap` binds Escape to `simplifySelection`, which reports the key + * handled whenever there is more than one range or a non-empty one, and (like + * every binding here) runs ahead of the Vim plugin. Since block edits became + * real multi-cursors (#792), that is exactly the state Vim is in when Escape + * should end a block `I`/`A`/`c`: CodeMirror collapsed the cursors, Vim never + * saw the key, and the editor stayed in insert mode. Visual block needed two + * presses for the same reason, and a plain `v`/`V` selection was left through + * the selection listener with the cursor on CodeMirror's head instead of the + * character Vim keeps it on. + * + * This is the inverse of `deferKeysToVim`: Vim owns Escape in insert, replace + * and visual mode. Normal mode keeps the native command, because there Vim + * itself hands Escape back to the editor so stray extra cursors can collapse. + */ +function deferEscapeToVim(bindings: readonly KeyBinding[]): KeyBinding[] { + return bindings.map((binding) => { + if (binding.key !== 'Escape' || !binding.run) return binding + const native = binding.run + return { + ...binding, + // Same constraint as above: preventDefault would consume the key even + // when the command defers. + preventDefault: false, + run: (view: EditorView): boolean => { + const vim = ( + getCM(view) as { state?: { vim?: { insertMode?: boolean; visualMode?: boolean } } } | null + )?.state?.vim + if (vim && (vim.insertMode || vim.visualMode)) return false + return native(view) + } + } + }) +} + +/** Vim-mode keymap: emacs chords stripped, motion keys and Escape made Vim-aware (see above). */ +const vimModeKeymap: readonly KeyBinding[] = deferEscapeToVim( + deferKeysToVim(defaultKeymapWithoutMacEmacs, VIM_MOTION_KEYS) ) /** * CodeMirror's `defaultKeymap`, made Vim-aware: in Vim mode the macOS * emacs-style control chords are stripped so Vim's ``/``/``/… - * bindings work, and the arrow keys defer to Vim in normal/visual mode so they - * move/extend like `hjkl`; with Vim off the full keymap is used unchanged. + * bindings work, the arrow keys defer to Vim in normal/visual mode so they + * move/extend like `hjkl`, and Escape defers to Vim in insert/visual mode so it + * leaves the mode; with Vim off the full keymap is used unchanged. */ export function vimAwareDefaultKeymap(vimMode: boolean): readonly KeyBinding[] { return vimMode ? vimModeKeymap : defaultKeymap diff --git a/packages/app-core/src/lib/cm-vim-visual-block.test.ts b/packages/app-core/src/lib/cm-vim-visual-block.test.ts index fbc052a4..bb21250d 100644 --- a/packages/app-core/src/lib/cm-vim-visual-block.test.ts +++ b/packages/app-core/src/lib/cm-vim-visual-block.test.ts @@ -1,15 +1,33 @@ // @vitest-environment jsdom +import { autocompletion, completionStatus } from '@codemirror/autocomplete' import { markdown, markdownLanguage } from '@codemirror/lang-markdown' import { EditorState } from '@codemirror/state' import { EditorView, keymap } from '@codemirror/view' import { getCM, Vim, vim } from '@replit/codemirror-vim' import { afterEach, describe, expect, it } from 'vitest' +import { completionKeymapExtension } from './cm-completion-nav' import { registerDisplayLineMotion } from './cm-vim-display-line' import { vimAwareDefaultKeymap, vimAwareMarkdownKeymap } from './cm-vim-default-keymap' import { vimVisualHighlightExtension } from './cm-vim-visual-highlight' import { vimClipboardPasteExtension } from './cm-vim-clipboard' +// jsdom has no layout. The #803 tests wait on the completion debounce, which +// lets CodeMirror's next-frame measurement run, and without these two methods +// it throws from a deferred callback that Vitest reports as an unhandled error +// (the same trap cm-vim-ime-guard.test.ts documents). Empty geometry is enough. +const rangeProto = Range.prototype as Range & { + getClientRects?: () => DOMRectList + getBoundingClientRect?: () => DOMRect +} +if (typeof rangeProto.getClientRects !== 'function') { + rangeProto.getClientRects = () => + ({ length: 0, item: () => null, [Symbol.iterator]: [][Symbol.iterator] }) as unknown as DOMRectList +} +if (typeof rangeProto.getBoundingClientRect !== 'function') { + rangeProto.getBoundingClientRect = () => new DOMRect(0, 0, 0, 0) +} + const views: EditorView[] = [] afterEach(() => { @@ -24,7 +42,15 @@ function mount(doc: string, anchor = 0): EditorView { doc, selection: { anchor }, extensions: [ + // CM6 runs every keymap from one DOM handler, placed at the FIRST + // keymap provider. The app mounts one (markdown snippets) ahead of + // vim(), so all bindings see a key before Vim does. Mounting vim() + // first here once hid #803: Vim got Escape before `simplifySelection`. + keymap.of([]), vim(), + // Typed text puts every source into "pending", as in the app. + autocompletion({ defaultKeymap: false, override: [() => null] }), + completionKeymapExtension, vimVisualHighlightExtension, vimClipboardPasteExtension, markdown({ base: markdownLanguage, addKeymap: false }), @@ -146,6 +172,77 @@ describe('Vim visual-block editing (#792)', () => { }) }) +describe('Escape ends a Vim block operation in normal mode (#803)', () => { + const vimState = (view: EditorView) => getCM(view)?.state.vim + + async function completionIdle(view: EditorView): Promise { + const deadline = Date.now() + 5_000 + while (completionStatus(view.state) !== null) { + if (Date.now() > deadline) throw new Error('completion never settled') + await new Promise((resolve) => setTimeout(resolve, 20)) + } + } + + it.each([ + ['I', '- ', '- one\n- two\n- three'], + ['A', '!', 'o!ne\nt!wo\nt!hree'], + ['c', 'X', 'Xne\nXwo\nXhree'] + ])('leaves insert mode with one cursor after block %s', async (key, text, expected) => { + const view = mount('one\ntwo\nthree') + selectFirstColumn(view) + + press(view, key) + expect(view.state.selection.ranges.length).toBe(3) + view.dispatch({ ...view.state.replaceSelection(text), userEvent: 'input.type' }) + // Settled, so only `simplifySelection` stands between Escape and Vim; the + // pending-query case has its own test below. + await completionIdle(view) + press(view, 'Escape') + + expect(view.state.doc.toString()).toBe(expected) + expect(vimState(view)?.insertMode).toBe(false) + expect(vimState(view)?.visualMode).toBe(false) + expect(view.state.selection.ranges.length).toBe(1) + }) + + it('is not swallowed by a completion query that is only pending', () => { + const view = mount('one\ntwo\nthree') + selectFirstColumn(view) + press(view, 'I') + view.dispatch({ ...view.state.replaceSelection('- '), userEvent: 'input.type' }) + // No popup exists yet: the sources are inside the activateOnTyping debounce. + expect(completionStatus(view.state)).toBe('pending') + + press(view, 'Escape') + + expect(vimState(view)?.insertMode).toBe(false) + expect(completionStatus(view.state)).toBe(null) + }) + + it('leaves visual block in a single press', () => { + const view = mount('one\ntwo\nthree') + selectFirstColumn(view) + + press(view, 'Escape') + + expect(vimState(view)?.visualMode).toBe(false) + expect(view.state.selection.ranges.length).toBe(1) + expect(view.state.selection.main.empty).toBe(true) + }) + + it('leaves a characterwise selection with the cursor on the last selected character', () => { + const view = mount('one\ntwo\nthree') + press(view, 'v') + press(view, 'l') + + press(view, 'Escape') + + expect(vimState(view)?.visualMode).toBe(false) + // Vim keeps the cursor on `n`; CodeMirror's own collapse would land past it. + expect(view.state.selection.main.head).toBe(1) + }) +}) + describe('Vim line-boundary insertion outside visual mode', () => { it.each([ ['I', ' !one\ntwo'], From 86d95760a933df429d9181d4569d0c88e73940b0 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Thu, 17 Sep 2026 09:12:41 -0500 Subject: [PATCH 02/19] Fix(tooling): resolve electron from the desktop workspace in the CDP scripts Four scripts that drive the built app over CDP could no longer start: vim-editor-smoke, sidebar-vim-smoke, editor-improvements-smoke and perf-editor-scroll all died with "Cannot find module 'electron'" before launching anything, so `npm run test:vim-editor` and its siblings were dead. Each one did createRequire(import.meta.url) and required electron from the repo root. electron is declared by the desktop workspace and is no longer hoisted to the root node_modules, and hoisting was never a contract. They now resolve it from apps/desktop/package.json, the pattern perf-desktop-runtime.mjs already used. ws and the other root-level requires are untouched, and pdf-export-smoke.py already looked electron up from the desktop workspace. With this, test:vim-editor passes 12 of 12, test:editor-improvements 19 of 19, and perf:editor-scroll runs to completion. test:sidebar-vim launches too; the failures it then reported were a real sidebar bug, fixed next. --- tooling/scripts/editor-improvements-smoke.mjs | 6 ++++-- tooling/scripts/perf-editor-scroll.mjs | 6 ++++-- tooling/scripts/sidebar-vim-smoke.mjs | 6 ++++-- tooling/scripts/vim-editor-smoke.mjs | 6 ++++-- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/tooling/scripts/editor-improvements-smoke.mjs b/tooling/scripts/editor-improvements-smoke.mjs index 7490c67f..d2d992d8 100644 --- a/tooling/scripts/editor-improvements-smoke.mjs +++ b/tooling/scripts/editor-improvements-smoke.mjs @@ -20,10 +20,12 @@ import { dirname, join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import WebSocket from 'ws' -const require = createRequire(import.meta.url) -const electronPath = require('electron') const scriptDir = dirname(fileURLToPath(import.meta.url)) const repoRoot = resolve(scriptDir, '..', '..') +// electron is a dependency of the desktop workspace, not of the repo root; +// hoisting is not a contract, so resolve it from where it is declared. +const requireDesktop = createRequire(resolve(repoRoot, 'apps/desktop/package.json')) +const electronPath = requireDesktop('electron') const desktopOutMain = resolve(repoRoot, 'apps/desktop/out/main/index.js') const npmCommand = process.platform === 'win32' ? 'npm.cmd' : 'npm' const skipBuild = process.env.ZEN_EDITOR_IMPROVEMENTS_SKIP_BUILD === '1' diff --git a/tooling/scripts/perf-editor-scroll.mjs b/tooling/scripts/perf-editor-scroll.mjs index ff312873..109b000e 100644 --- a/tooling/scripts/perf-editor-scroll.mjs +++ b/tooling/scripts/perf-editor-scroll.mjs @@ -30,10 +30,12 @@ import zlib from 'node:zlib' import { randomFillSync } from 'node:crypto' import WebSocket from 'ws' -const require = createRequire(import.meta.url) -const electronPath = require('electron') const scriptDir = dirname(fileURLToPath(import.meta.url)) const repoRoot = resolve(scriptDir, '..', '..') +// electron is a dependency of the desktop workspace, not of the repo root; +// hoisting is not a contract, so resolve it from where it is declared. +const requireDesktop = createRequire(resolve(repoRoot, 'apps/desktop/package.json')) +const electronPath = requireDesktop('electron') const desktopOutMain = resolve(repoRoot, 'apps/desktop/out/main/index.js') const outPath = process.argv[2] || process.env.ZEN_EDITOR_PERF_OUT || join(scriptDir, 'perf-editor-scroll.json') const LINES = Number(process.env.ZEN_EDITOR_PERF_LINES || 700) diff --git a/tooling/scripts/sidebar-vim-smoke.mjs b/tooling/scripts/sidebar-vim-smoke.mjs index 8352f40f..b327793b 100644 --- a/tooling/scripts/sidebar-vim-smoke.mjs +++ b/tooling/scripts/sidebar-vim-smoke.mjs @@ -33,10 +33,12 @@ import { dirname, join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import WebSocket from 'ws' -const require = createRequire(import.meta.url) -const electronPath = require('electron') const scriptDir = dirname(fileURLToPath(import.meta.url)) const repoRoot = resolve(scriptDir, '..', '..') +// electron is a dependency of the desktop workspace, not of the repo root; +// hoisting is not a contract, so resolve it from where it is declared. +const requireDesktop = createRequire(resolve(repoRoot, 'apps/desktop/package.json')) +const electronPath = requireDesktop('electron') const desktopOutMain = resolve(repoRoot, 'apps/desktop/out/main/index.js') const npmCommand = process.platform === 'win32' ? 'npm.cmd' : 'npm' diff --git a/tooling/scripts/vim-editor-smoke.mjs b/tooling/scripts/vim-editor-smoke.mjs index 2f6f0c0c..b79e5b72 100644 --- a/tooling/scripts/vim-editor-smoke.mjs +++ b/tooling/scripts/vim-editor-smoke.mjs @@ -21,10 +21,12 @@ import { dirname, join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import WebSocket from 'ws' -const require = createRequire(import.meta.url) -const electronPath = require('electron') const scriptDir = dirname(fileURLToPath(import.meta.url)) const repoRoot = resolve(scriptDir, '..', '..') +// electron is a dependency of the desktop workspace, not of the repo root; +// hoisting is not a contract, so resolve it from where it is declared. +const requireDesktop = createRequire(resolve(repoRoot, 'apps/desktop/package.json')) +const electronPath = requireDesktop('electron') const desktopOutMain = resolve(repoRoot, 'apps/desktop/out/main/index.js') const npmCommand = process.platform === 'win32' ? 'npm.cmd' : 'npm' const skipBuild = process.env.ZEN_VIM_EDITOR_SKIP_BUILD === '1' From 6f3b623e4cbc8a7d7526ff4319873d73df6d16a6 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Thu, 17 Sep 2026 09:12:56 -0500 Subject: [PATCH 03/19] Fix(sidebar): the Vim cursor survives a trip to the editor and back In a folder large enough to be windowed (240 notes and up), walking the sidebar with j/k, opening a note with Enter and coming back with Ctrl+W h lost the cursor: nothing was highlighted, the next j showed nothing, the one after landed on Assets, and k on Trash. Every sidebar row carries a data-sidebar-idx handed out by a mutable counter, and the Vim cursor is stored as one of those numbers. Only Sidebar's own render restarts the counter, but FolderTreeRoot, FolderTreeContents and SubTree read it during theirs and hold state of their own, so they also render alone. A lone render read whatever the counter had been left at, and two things leave it wrong: - A full Sidebar render ends on the sidebar's total row count. FolderTreeContents re-renders alone on every sidebar focus change (its progressive entry limit resets when `progressive` flips with sidebarFocused), so the notes jumped from 10..1009 to 1010..2009 and the stored cursor matched no row. It stayed hidden in the simple keyboard flow by luck: the focus effect usually writes the cursor in the same batch and forces a full render. It bites when the cursor is already on the target row, which is exactly "return to the note I just opened". - React also runs Sidebar's body and then bails out without rendering its children. That restarts the counter and leaves it just past Sidebar's inline rows, so the next lone render numbered notes from 8 and collided with Quick Notes (8) and Inbox (9). The second case is why a "Sidebar rendered" flag on the counter, the first shape this fix took, cannot tell the two kinds of render apart. Only a prop can, because props change exactly when the parent really rendered the child. useStableSidebarIdxBase remembers the counter value a tree component starts from and restarts from it whenever the component renders again with the same idxPass. Each parent creates a new pass per render, and the prop is required in TreeRenderProps so the compiler flags a call site that forgets it. DateNotesNav holds no state and needs no guard. `npm run test:sidebar-vim` was reporting this as 4 failures of 12 and had not drifted: with the fix it passes 12 of 12 with no selector changes. The fourth failure was a knock-on, since with no cursor the script fell back to idx 0 and its gg wait became trivially true. The script did have one real gap: it never set ZENNOTES_CONFIG_DIR, so it read the developer's real config.toml and only passed where that file happened to enable Vim. It now seeds its own, like every other CDP script. --- packages/app-core/src/components/Sidebar.tsx | 29 ++- .../src/lib/sidebar-idx-counter.test.ts | 168 ++++++++++++++++++ .../app-core/src/lib/sidebar-idx-counter.ts | 56 ++++++ tooling/scripts/sidebar-vim-smoke.mjs | 18 +- 4 files changed, 263 insertions(+), 8 deletions(-) create mode 100644 packages/app-core/src/lib/sidebar-idx-counter.test.ts create mode 100644 packages/app-core/src/lib/sidebar-idx-counter.ts diff --git a/packages/app-core/src/components/Sidebar.tsx b/packages/app-core/src/components/Sidebar.tsx index 364426fb..74ed703c 100644 --- a/packages/app-core/src/components/Sidebar.tsx +++ b/packages/app-core/src/components/Sidebar.tsx @@ -95,6 +95,11 @@ import { type DragPayload, } from "../lib/dnd"; import { setSidebarDragPayload } from "../lib/sidebar-drag-preview"; +import { + useStableSidebarIdxBase, + type SidebarIdxCounter, + type SidebarIdxPass, +} from "../lib/sidebar-idx-counter"; import { manualOrderCompare, parentDirOf } from "../lib/manual-order"; import { resolveSystemFolderLabels } from "../lib/system-folder-labels"; import { assetTabPath } from "../lib/asset-tabs"; @@ -2788,8 +2793,11 @@ export function Sidebar(): JSX.Element { const isSidebarFocused = focusedPanel === "sidebar"; // Mutable counter reset on each render — assigns sequential data-sidebar-idx to each item. - const idxCounter = useRef<{ value: number }>({ value: 0 }); + const idxCounter = useRef({ value: 0 }); idxCounter.current.value = 0; + // New on every render, so the tree components below can tell "Sidebar + // rendered me" from "I re-rendered alone" (see useStableSidebarIdxBase). + const idxPass: SidebarIdxPass = {}; const vimCursor = isSidebarFocused ? sidebarCursorIndex : -1; const vaultHeaderIdx = canSwitchVaults ? idxCounter.current.value++ : -1; const vaultHeaderVimHighlight = vimCursor === vaultHeaderIdx; @@ -3344,6 +3352,7 @@ export function Sidebar(): JSX.Element { onSelectItem={handleSidebarItemSelect} dragPayloadForItem={dragPayloadForItem} idxCounter={idxCounter.current} + idxPass={idxPass} vimCursor={vimCursor} sidebarFocused={isSidebarFocused} groupByKind={groupByKind} @@ -3426,6 +3435,7 @@ export function Sidebar(): JSX.Element { onSelectItem={handleSidebarItemSelect} dragPayloadForItem={dragPayloadForItem} idxCounter={idxCounter.current} + idxPass={idxPass} vimCursor={vimCursor} sidebarFocused={isSidebarFocused} groupByKind={groupByKind} @@ -3459,6 +3469,7 @@ export function Sidebar(): JSX.Element { onSelectItem={handleSidebarItemSelect} dragPayloadForItem={dragPayloadForItem} idxCounter={idxCounter.current} + idxPass={idxPass} vimCursor={vimCursor} sidebarFocused={isSidebarFocused} groupByKind={groupByKind} @@ -4402,9 +4413,7 @@ function countNotesInTree(node: TreeNode): number { /* ---------- Tree rendering ---------- */ /** Mutable counter threaded through tree rendering for sequential data-sidebar-idx attributes. */ -interface IdxCounter { - value: number; -} +type IdxCounter = SidebarIdxCounter; interface TreeRenderProps { folder: NoteFolder; @@ -4440,6 +4449,7 @@ interface TreeRenderProps { dragPayloadForItem: (item: SidebarSelectionItem) => DragPayload; /** Sequential index counter for vim navigation data attributes. */ idxCounter: IdxCounter; + idxPass: SidebarIdxPass; /** The highlighted cursor index when sidebar is vim-focused (-1 if not focused). */ vimCursor: number; /** Whether the sidebar currently owns keyboard focus. */ @@ -4472,6 +4482,7 @@ function FolderTreeContents({ onSelectItem, dragPayloadForItem, idxCounter, + idxPass, vimCursor, sidebarFocused, groupByKind, @@ -4481,6 +4492,9 @@ function FolderTreeContents({ tree: TreeNode; depth: number; } & TreeRenderProps): JSX.Element { + // This component re-renders alone whenever its entry limit resets, which a + // sidebar focus change does every time. + const childIdxPass = useStableSidebarIdxBase(idxCounter, idxPass); const entries = useMemo( () => getTreeRenderEntries(tree, showNotes, sortComparator, groupByKind), [tree, showNotes, sortComparator, groupByKind], @@ -4564,6 +4578,7 @@ function FolderTreeContents({ onSelectItem={onSelectItem} dragPayloadForItem={dragPayloadForItem} idxCounter={idxCounter} + idxPass={childIdxPass} vimCursor={vimCursor} sidebarFocused={sidebarFocused} groupByKind={groupByKind} @@ -4644,6 +4659,7 @@ function FolderTreeRoot({ onSelectItem, dragPayloadForItem, idxCounter, + idxPass, vimCursor, sidebarFocused, groupByKind, @@ -4657,6 +4673,7 @@ function FolderTreeRoot({ * revealed on hover. Used to surface a quick "+" for Quick Notes. */ headerAction?: JSX.Element; } & TreeRenderProps): JSX.Element { + const childIdxPass = useStableSidebarIdxBase(idxCounter, idxPass); const rootKey = `${folder}:`; const isCollapsed = collapsed.has(rootKey); const total = countNotesInTree(tree); @@ -4750,6 +4767,7 @@ function FolderTreeRoot({ onSelectItem={onSelectItem} dragPayloadForItem={dragPayloadForItem} idxCounter={idxCounter} + idxPass={childIdxPass} vimCursor={vimCursor} sidebarFocused={sidebarFocused} groupByKind={groupByKind} @@ -4783,11 +4801,13 @@ function SubTree({ onSelectItem, dragPayloadForItem, idxCounter, + idxPass, vimCursor, sidebarFocused, groupByKind, showSidebarChevrons, }: { node: TreeNode; depth: number } & TreeRenderProps): JSX.Element { + const childIdxPass = useStableSidebarIdxBase(idxCounter, idxPass); const key = `${folder}:${node.subpath}`; const isCollapsed = collapsed.has(key); const iconOption = resolveFolderIconOption( @@ -4971,6 +4991,7 @@ function SubTree({ onSelectItem={onSelectItem} dragPayloadForItem={dragPayloadForItem} idxCounter={idxCounter} + idxPass={childIdxPass} vimCursor={vimCursor} sidebarFocused={sidebarFocused} groupByKind={groupByKind} diff --git a/packages/app-core/src/lib/sidebar-idx-counter.test.ts b/packages/app-core/src/lib/sidebar-idx-counter.test.ts new file mode 100644 index 00000000..029fb789 --- /dev/null +++ b/packages/app-core/src/lib/sidebar-idx-counter.test.ts @@ -0,0 +1,168 @@ +// @vitest-environment jsdom + +import { act, createElement, useState, type ReactElement } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + useStableSidebarIdxBase, + type SidebarIdxCounter, + type SidebarIdxPass +} from './sidebar-idx-counter' + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + +// The sidebar in miniature: a parent that restarts the counter on every render +// and numbers one row of its own, then tree components that number theirs from +// the shared counter and can re-render without the parent. +describe('sidebar row numbering across lone re-renders', () => { + let host: HTMLDivElement + let root: Root + let counter: SidebarIdxCounter + const triggers: Record void> = {} + + const row = (idx: number, name: string): ReactElement => + createElement('span', { key: name, 'data-idx': idx, 'data-name': name }) + + // Like FolderTreeContents, a component builds its nested tree components + // while it renders, so they re-render whenever it does. + function Leafy({ name, guarded, nested, pass }: { + name: string + guarded: boolean + nested?: string + pass: SidebarIdxPass + }): ReactElement { + const [, setTick] = useState(0) + // `guarded` is fixed for the life of a mounted tree, so hook order holds. + const childPass = guarded ? useStableSidebarIdxBase(counter, pass) : {} + triggers[name] = () => setTick((tick) => tick + 1) + const first = counter.value++ + const second = counter.value++ + return createElement( + 'div', + null, + row(first, `${name}-1`), + row(second, `${name}-2`), + nested + ? createElement(Leafy, { key: nested, name: nested, guarded, pass: childPass }) + : null + ) + } + + /** What Sidebar's body does to the counter before any child renders. */ + const runParentBody = (extraRow: boolean): ReactElement[] => { + counter.value = 0 + const rows = [row(counter.value++, 'header')] + if (extraRow) rows.push(row(counter.value++, 'extra')) + return rows + } + + function Parent({ guarded, extraRow }: { guarded: boolean; extraRow: boolean }): ReactElement { + const rows = runParentBody(extraRow) + const pass: SidebarIdxPass = {} + return createElement( + 'div', + null, + ...rows, + createElement(Leafy, { key: 'outer', name: 'outer', guarded, nested: 'inner', pass }), + createElement(Leafy, { key: 'tail', name: 'tail', guarded, pass }) + ) + } + + const numbering = (): Record => + Object.fromEntries( + [...host.querySelectorAll('[data-idx]')].map((el) => [ + el.dataset.name as string, + Number(el.dataset.idx) + ]) + ) + + const render = (props: { guarded: boolean; extraRow?: boolean }): void => { + act(() => root.render(createElement(Parent, { extraRow: false, ...props }))) + } + + beforeEach(() => { + counter = { value: 0 } + host = document.createElement('div') + document.body.append(host) + root = createRoot(host) + }) + + afterEach(() => { + act(() => root.unmount()) + host.remove() + }) + + const sequential = { + header: 0, + 'outer-1': 1, + 'outer-2': 2, + 'inner-1': 3, + 'inner-2': 4, + 'tail-1': 5, + 'tail-2': 6 + } + + it('models the bug: without the guard a lone re-render continues from the end of the last pass', () => { + render({ guarded: false }) + expect(numbering()).toEqual(sequential) + + act(() => triggers.outer()) + + // The whole subtree jumped by the sidebar's row count, so a stored cursor + // of 1 now points at nothing. + expect(numbering()['outer-1']).toBe(7) + expect(numbering()['inner-1']).toBe(9) + }) + + it('keeps every row number when a tree component re-renders alone', () => { + render({ guarded: true }) + expect(numbering()).toEqual(sequential) + + act(() => triggers.outer()) + expect(numbering()).toEqual(sequential) + + // A nested component on its own, and a later sibling, behave the same. + act(() => triggers.inner()) + act(() => triggers.tail()) + act(() => triggers.outer()) + expect(numbering()).toEqual(sequential) + }) + + // React can run Sidebar's body and then bail out without rendering a single + // child. The counter is left just past Sidebar's own rows, and the next lone + // re-render must not mistake that for the start of its own numbering. + it('ignores a parent body run whose children never rendered', () => { + render({ guarded: true }) + + runParentBody(false) + act(() => triggers.inner()) + expect(numbering()).toEqual(sequential) + + runParentBody(false) + act(() => triggers.tail()) + expect(numbering()).toEqual(sequential) + }) + + it('still renumbers on a full pass when rows above it change', () => { + render({ guarded: true }) + act(() => triggers.outer()) + + render({ guarded: true, extraRow: true }) + + expect(numbering()).toEqual({ + header: 0, + extra: 1, + 'outer-1': 2, + 'outer-2': 3, + 'inner-1': 4, + 'inner-2': 5, + 'tail-1': 6, + 'tail-2': 7 + }) + + // And the new numbers are the ones a later lone re-render holds on to. + act(() => triggers.inner()) + expect(numbering()['inner-1']).toBe(4) + expect(numbering()['tail-1']).toBe(6) + }) +}) diff --git a/packages/app-core/src/lib/sidebar-idx-counter.ts b/packages/app-core/src/lib/sidebar-idx-counter.ts new file mode 100644 index 00000000..89624310 --- /dev/null +++ b/packages/app-core/src/lib/sidebar-idx-counter.ts @@ -0,0 +1,56 @@ +import { useRef } from 'react' + +/** + * Mutable counter threaded through the sidebar tree while it renders, handing + * out the sequential `data-sidebar-idx` every row carries. The Vim cursor is + * stored as one of these numbers, so a row must keep its number for as long as + * the rows above it have not changed. Sidebar restarts it from 0 each render. + */ +export interface SidebarIdxCounter { + value: number +} + +/** + * Identity of one render of a component that numbers rows. A tree component + * receives a new one each time its parent renders it, and keeps seeing the + * same one when it re-renders on its own. Compared by reference only. + */ +export type SidebarIdxPass = object + +/** + * Keep a tree component's row numbers stable when it re-renders on its own. + * + * The counter is only right for a component that renders as part of its + * parent's render. One that re-renders alone (its progressive entry limit + * resetting as the sidebar gains or loses focus, a drag hover) used to read + * whatever the counter had been left at, and two things leave it wrong: + * + * - A full Sidebar render ends on the sidebar's total row count, so every row + * under the component jumped by that much. The stored Vim cursor then + * matched no row: coming back to the sidebar after opening a note hid the + * cursor, and the next j/k clamped to a position near the end of the list + * and landed on Assets or Trash. + * - React also runs Sidebar's body and then bails out without rendering its + * children, which restarts the counter and leaves it just past Sidebar's own + * rows. Rows numbered from there collide with the folder rows above them. + * + * That second case is why a counter-side "Sidebar rendered" flag cannot tell + * the two kinds of render apart; only a prop can, because props change exactly + * when the parent really rendered this component. So: on a new `parentPass`, + * remember the counter value this component starts from; on the same one, + * restart from the remembered value. Call it before the component's first read + * of the counter, and hand the returned pass to every tree component it + * renders, which makes their numbering follow the same rule. + */ +export function useStableSidebarIdxBase( + counter: SidebarIdxCounter, + parentPass: SidebarIdxPass +): SidebarIdxPass { + const seen = useRef<{ pass: SidebarIdxPass; base: number } | null>(null) + if (seen.current?.pass === parentPass) { + counter.value = seen.current.base + } else { + seen.current = { pass: parentPass, base: counter.value } + } + return {} +} diff --git a/tooling/scripts/sidebar-vim-smoke.mjs b/tooling/scripts/sidebar-vim-smoke.mjs index b327793b..cc2f7966 100644 --- a/tooling/scripts/sidebar-vim-smoke.mjs +++ b/tooling/scripts/sidebar-vim-smoke.mjs @@ -85,13 +85,17 @@ async function seedVault(root) { await Promise.all(files.slice(i, i + 100).map(([p, b]) => writeFile(p, b))) } } -async function seedUserData(userDataRoot, vaultRoot) { - await mkdir(userDataRoot, { recursive: true }) +async function seedUserData(userDataRoot, configRoot, vaultRoot) { + await Promise.all([mkdir(userDataRoot, { recursive: true }), mkdir(configRoot, { recursive: true })]) await writeFile(join(userDataRoot, 'zennotes.config.json'), JSON.stringify({ workspaceMode: 'local', vaultRoot, remoteWorkspace: null, remoteWorkspaceProfileId: null, remoteWorkspaceProfiles: [], windowState: { x: 60, y: 60, width: 1280, height: 860, isMaximized: false }, zoomFactor: 1, quickCaptureHotkey: '' }, null, 2)) + // Portable prefs live in config.toml, outside userData. Without a config dir + // of its own the app reads (and would write) the developer's real one, and + // the j/k checks below only pass where that file happens to enable Vim. + await writeFile(join(configRoot, 'config.toml'), '[vim]\nenabled = true\n') } function getFreePort() { @@ -172,13 +176,19 @@ async function main() { const tempRoot = await mkdtemp(join(tmpdir(), 'zennotes-sidebar-vim-')) const vaultRoot = join(tempRoot, 'vault') const userDataRoot = join(tempRoot, 'user-data') + const configRoot = join(tempRoot, 'config') await seedVault(vaultRoot) - await seedUserData(userDataRoot, vaultRoot) + await seedUserData(userDataRoot, configRoot, vaultRoot) const port = await getFreePort() const child = spawn(electronPath, [`--remote-debugging-port=${port}`, desktopOutMain], { cwd: repoRoot, - env: { ...process.env, ELECTRON_DISABLE_SECURITY_WARNINGS: '1', ZENNOTES_USER_DATA_PATH: userDataRoot }, + env: { + ...process.env, + ELECTRON_DISABLE_SECURITY_WARNINGS: '1', + ZENNOTES_USER_DATA_PATH: userDataRoot, + ZENNOTES_CONFIG_DIR: configRoot + }, stdio: ['ignore', 'pipe', 'pipe'] }) let log = '' From 50ab2560f703baee04c209b833fdcf3a2614e9a9 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Thu, 17 Sep 2026 09:13:19 -0500 Subject: [PATCH 04/19] Fix(palette): typing goes to the search a command opened, not the note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running "Search Text in Vault…" from the command palette with the editor focused opened the search with the caret still in the note, so the query was typed into the note instead (ZenNotes/zennotesandroid#65). "Search Notes…" and "Open Note Outline…" did the same. Reported on Android in Edit mode, but the cause is in the shared core and desktop had it too. After a command runs, the palette hands DOM focus back to the editor unless it knows the command left something open on top. That list named Settings and three palettes and was missing the note search, vault text search and outline palettes. focusEditorNormalMode focuses on the next animation frame, which is after the opened palette's mount effect focused its input, so the editor took focus back a moment later. The overlays are lazy-loaded, which hid this on the first open of a session (the chunk mounted after the refocus) and broke every later one. In Read mode the editor is hidden and cannot take focus, which is why only Edit mode showed it. The list now lives in command-palette-mode.ts as COMMAND_OVERLAY_FLAGS behind shouldRefocusEditorAfterCommand, and a test requires every `…Open` boolean in the store to be classified, so a new overlay cannot silently regress to typing into the note behind it. Store flags cannot see everything: "Publish Note" opens its dialog through publish-note-requests without awaiting it, and the editor was taking focus behind that dialog as well. The caller now reports it next to the Cloud conflict review, as `dialogOutsideStoreOpen`. Prompts, confirms and date pickers need no entry, because their commands await the answer and they are closed by the time this runs. Verified on a desktop build with real key events, three runs per command since the first one after launch was the lucky one: the five palettes keep focus in their input and the note stays untouched, the Publish Note dialog keeps focus, and a command that lands on the editor is still handed focus. --- .../src/components/CommandPalette.tsx | 24 +++++---- .../src/lib/command-palette-mode.test.ts | 54 ++++++++++++++++++- .../app-core/src/lib/command-palette-mode.ts | 42 +++++++++++++++ 3 files changed, 109 insertions(+), 11 deletions(-) diff --git a/packages/app-core/src/components/CommandPalette.tsx b/packages/app-core/src/components/CommandPalette.tsx index 811e4ade..60c8ed91 100644 --- a/packages/app-core/src/components/CommandPalette.tsx +++ b/packages/app-core/src/components/CommandPalette.tsx @@ -14,7 +14,10 @@ import { import { rankItems } from '../lib/fuzzy-score' import { isPaletteNextKey, isPalettePreviousKey } from '../lib/palette-nav' import { isImeComposing } from '../lib/ime' -import { canReturnToCommandList } from '../lib/command-palette-mode' +import { + canReturnToCommandList, + shouldRefocusEditorAfterCommand +} from '../lib/command-palette-mode' import { THEMES, type ThemeFamily, type ThemeMode, type ThemeOption } from '../lib/themes' import { buildVaultSwitcherEntries, @@ -26,6 +29,7 @@ import { runWorkflowById } from '../lib/workflow-trigger' import type { WorkflowIndexEntry } from '../lib/workflow-index' import { focusEditorNormalMode } from '../lib/editor-focus' import { useCloudSyncStatusStore } from '../lib/cloud-auto-sync' +import { getPublishNoteRequest } from '../lib/publish-note-requests' import { Modal } from './ui/Modal' type Mode = 'main' | 'theme' | 'vault' | 'workflow' @@ -304,16 +308,16 @@ export function CommandPalette(): JSX.Element { // explorer), and the editor's own focus-on-`focusedPanel` effect is a // single, no-retry `view.focus()` that races the palette unmount. Mirror // closePalette's focus restore; the retry wins that race. Skipped when the - // command opened the Settings modal so we don't pull focus behind it, and - // likewise for the Cloud conflict queue, whose dialog claims focus itself. - const s = useStore.getState() + // command opened Settings or another palette (search, vault text search, + // outline, …) so we don't pull focus behind it, and likewise for the Cloud + // conflict queue and the Publish Note dialog, which claim focus themselves + // and are tracked outside the store. if ( - s.focusedPanel === 'editor' && - !s.settingsOpen && - !s.embedDrawingPaletteOpen && - !s.templatePaletteOpen && - !s.bufferPaletteOpen && - !useCloudSyncStatusStore.getState().conflictReviewOpen + shouldRefocusEditorAfterCommand( + useStore.getState(), + useCloudSyncStatusStore.getState().conflictReviewOpen || + getPublishNoteRequest() !== null + ) ) focusEditorNormalMode() } catch (err) { diff --git a/packages/app-core/src/lib/command-palette-mode.test.ts b/packages/app-core/src/lib/command-palette-mode.test.ts index ed35a6a5..5b38dd9f 100644 --- a/packages/app-core/src/lib/command-palette-mode.test.ts +++ b/packages/app-core/src/lib/command-palette-mode.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from 'vitest' -import { canReturnToCommandList } from './command-palette-mode' +import { useStore } from '../store' +import { + COMMAND_OVERLAY_FLAGS, + canReturnToCommandList, + shouldRefocusEditorAfterCommand +} from './command-palette-mode' describe('canReturnToCommandList', () => { it('steps back when a sub-mode was entered from the command list', () => { @@ -17,3 +22,50 @@ describe('canReturnToCommandList', () => { expect(canReturnToCommandList('main', 'main')).toBe(false) }) }) + +describe('shouldRefocusEditorAfterCommand', () => { + const onEditor = { + focusedPanel: 'editor', + settingsOpen: false, + searchOpen: false, + vaultTextSearchOpen: false, + bufferPaletteOpen: false, + outlinePaletteOpen: false, + templatePaletteOpen: false, + embedDrawingPaletteOpen: false + } + + it('hands focus to the editor when a command lands on it', () => { + expect(shouldRefocusEditorAfterCommand(onEditor, false)).toBe(true) + }) + + it('leaves focus alone when the command landed elsewhere', () => { + expect(shouldRefocusEditorAfterCommand({ ...onEditor, focusedPanel: 'sidebar' }, false)).toBe( + false + ) + expect(shouldRefocusEditorAfterCommand({ ...onEditor, focusedPanel: null }, false)).toBe(false) + }) + + // "Search Text in Vault…", "Search Notes…" and "Open Note Outline…" run from + // the editor used to open with the caret still in the note. + // (zennotesandroid#65) + it.each(COMMAND_OVERLAY_FLAGS)('does not pull focus behind an open %s', (flag) => { + expect(shouldRefocusEditorAfterCommand({ ...onEditor, [flag]: true }, false)).toBe(false) + }) + + // The Cloud conflict review and the Publish Note dialog are opened without + // being awaited and live outside the store, so the caller reports them. + it('does not pull focus behind a dialog tracked outside the store', () => { + expect(shouldRefocusEditorAfterCommand(onEditor, true)).toBe(false) + }) + + // A new `…Open` overlay has to be classified here, or a command that opens + // it silently regresses to typing into the note behind it. + it('covers every overlay flag in the store', () => { + const notCommandOverlays = ['sidebarOpen', 'noteListOpen', 'commandPaletteOpen'] + const openFlags = Object.entries(useStore.getState()) + .filter(([key, value]) => key.endsWith('Open') && typeof value === 'boolean') + .map(([key]) => key) + expect(openFlags.sort()).toEqual([...COMMAND_OVERLAY_FLAGS, ...notCommandOverlays].sort()) + }) +}) diff --git a/packages/app-core/src/lib/command-palette-mode.ts b/packages/app-core/src/lib/command-palette-mode.ts index bb68de85..5be44379 100644 --- a/packages/app-core/src/lib/command-palette-mode.ts +++ b/packages/app-core/src/lib/command-palette-mode.ts @@ -17,3 +17,45 @@ export function canReturnToCommandList( ): boolean { return mode !== 'main' && initialMode === 'main' } + +/** + * Store flags for the palettes and modals a command can open. Each claims + * focus for its own input when it mounts. + */ +export const COMMAND_OVERLAY_FLAGS = [ + 'settingsOpen', + 'searchOpen', + 'vaultTextSearchOpen', + 'bufferPaletteOpen', + 'outlinePaletteOpen', + 'templatePaletteOpen', + 'embedDrawingPaletteOpen' +] as const + +export type CommandOverlayFlag = (typeof COMMAND_OVERLAY_FLAGS)[number] + +/** + * Whether the command palette should hand DOM focus to the editor once a + * command has run. + * + * Only when the command landed on the editor and left nothing open on top of + * it. `focusEditorNormalMode` focuses on the next animation frame, which is + * after the opened palette's mount effect focused its input, so refocusing + * behind an overlay sends the user's typing into the note instead of the + * search field. The overlays are lazy-loaded, which hid this on the first open + * of a session (the chunk mounted after the refocus) and broke every later + * one. (zennotesandroid#65) + * + * `dialogOutsideStoreOpen` covers the dialogs a command opens WITHOUT awaiting + * them and that are tracked outside the main store, where the flag list above + * cannot see them: the Cloud conflict review and the Publish Note dialog. The + * caller reports those. Prompts, confirms and date pickers need no entry: their + * commands await the answer, so they are closed by the time this runs. + */ +export function shouldRefocusEditorAfterCommand( + state: { focusedPanel: string | null } & Record, + dialogOutsideStoreOpen: boolean +): boolean { + if (state.focusedPanel !== 'editor' || dialogOutsideStoreOpen) return false + return !COMMAND_OVERLAY_FLAGS.some((flag) => state[flag]) +} From 9a6f5b639d8ba88d14ee4def5e0225c06d255fd5 Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Thu, 17 Sep 2026 09:13:30 -0500 Subject: [PATCH 05/19] Fix(settings): Settings takes the keyboard instead of leaving it in the note With Settings open, typing still edited the note behind it. It happened whether Settings was opened with Mod+, or from the command palette, because nothing pulled focus back: Settings simply never took it. It is the one dialog that draws its own backdrop and panel instead of sitting in the shared Modal shell, so it never got what the shell gives every other dialog, where opening moves focus into the panel, Tab cycles inside it, and closing hands focus back to the opener. Wrapping Settings in would have changed how it looks (backdrop blur, top offset, corner radius, border against ring), so the shell's focus logic moves out of ModalRoot into two exported helpers, useDialogFocus and trapDialogTab. ModalRoot uses them itself, so the 21 dialogs on the shell behave exactly as before, and Settings calls them on its own panel rather than keeping a copy. The panel also gains dialog semantics (role, aria-modal, a label) and tabIndex -1 so it can be a focus target. Opening lands on the settings search, the first thing a keyboard user reaches for. On a touch device a focused input would raise the on-screen keyboard over the list the user is about to tap, so the panel takes focus there instead, the same rule PromptModal already follows. Escape, the backdrop and Done still close through closeSettings, which returns focus to the editor as before. Verified on a desktop build with real key events, Vim off and on, and with a touch device emulated: focus lands inside Settings from both ways of opening it, typing stays there and never reaches the note, 70 Tab presses never leave the window, Escape closes it and the caret is back in the note. --- .../app-core/src/components/SettingsModal.tsx | 18 ++++- .../app-core/src/components/ui/Modal.test.ts | 80 ++++++++++++++++++- packages/app-core/src/components/ui/Modal.tsx | 67 +++++++++------- 3 files changed, 135 insertions(+), 30 deletions(-) diff --git a/packages/app-core/src/components/SettingsModal.tsx b/packages/app-core/src/components/SettingsModal.tsx index 4b70abef..44feb2dc 100644 --- a/packages/app-core/src/components/SettingsModal.tsx +++ b/packages/app-core/src/components/SettingsModal.tsx @@ -132,6 +132,8 @@ import { promptApp } from "../lib/prompt-requests"; import { isImeComposing } from "../lib/ime"; import { RemoteWorkspaceProfileModal } from "./RemoteWorkspaceProfileModal"; import { Button } from "./ui/Button"; +import { trapDialogTab, useDialogFocus } from "./ui/Modal"; +import { isTouchPrimaryDevice } from "../lib/cm-vim-ime-guard"; import { ignoredKeyTokenFromEvent, setIgnoredKeysRecorderActive, @@ -1192,6 +1194,14 @@ export function SettingsModal(): JSX.Element { }; const ref = useRef(null); + const navSearchRef = useRef(null); + // Settings draws its own backdrop and panel, so it never got the focus + // handling the shared Modal shell gives every other dialog: the keyboard + // stayed on the editor underneath and typing edited the note behind the + // open window. Opening lands on the settings search, the first thing a + // keyboard user reaches for. On a touch device a focused input would raise + // the on-screen keyboard over the panel, so the panel takes focus instead. + useDialogFocus(ref, isTouchPrimaryDevice() ? ref : navSearchRef); const settingsSearchHighlightTimerRef = useRef(null); const [initialSettingsTarget] = useState(consumeSettingsTarget); const [activeCategory, setActiveCategory] = useState( @@ -5258,8 +5268,13 @@ export function SettingsModal(): JSX.Element { >
e.stopPropagation()} + onKeyDown={(e) => trapDialogTab(e, ref.current)} >