Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions ui/src/bindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,14 +188,26 @@ export const DEFAULT_BINDINGS = [
{ id: "find.next", label: "Next match", keys: ["Mod-g"], scope: "find" },
{ id: "find.prev", label: "Previous match", keys: ["Mod-Shift-g"], scope: "find" },

// The editor. The first four are handed to CodeMirror's own keymap (ahead of its
// defaults, so they win); ⌘S is the document handler's, and reaches it only because
// CodeMirror leaves Mod-s unbound — editorkeys.test.ts is what keeps that true.
// The editor. All but ⌘S are handed to CodeMirror's own keymap (ahead of its defaults,
// so they win); ⌘S is the document handler's, and reaches it only because CodeMirror
// leaves Mod-s unbound — editorkeys.test.ts is what keeps that true.
{ id: "format.bold", label: "Bold", keys: ["Mod-b"], scope: "editor" },
{ id: "format.italic", label: "Italic", keys: ["Mod-i"], scope: "editor" },
{ id: "editor.table", label: "Insert a table", keys: ["Mod-t"], scope: "editor" },
{ id: "editor.paste-plain", label: "Paste as plain text", keys: ["Mod-Shift-v"], scope: "editor" },
{ id: "editor.save", label: "Save now", keys: ["Mod-s"], scope: "editor" },
// Tab, the one chord here that is also the *platform's*. It is claimed only with the
// caret in a list item (list.ts declines otherwise), so Tab still walks the focus ring
// everywhere else in the buffer. The overlay layer's own `Any-Tab` trap is a sibling
// scope rather than a clash: a dialog holds the keyboard while it's up, so CodeMirror
// never sees the key it would take — which is exactly what the trap is for.
{ id: "editor.list.indent", label: "Nest a list item", keys: ["Tab"], scope: "editor" },
{
id: "editor.list.outdent",
label: "Lift a list item out",
keys: ["Shift-Tab"],
scope: "editor",
},

// The frontmatter drawer — a separate surface from the body editor, hence its own
// scope: ⌘S means "save this drawer" here and "flush the note" there.
Expand Down
42 changes: 36 additions & 6 deletions ui/src/editorkeys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// that binds Mod-e would break ⌘E in the one path a user hits constantly — no error, no
// failing test, just a chord that stopped working. So the assumption is a list now, and
// the list is asserted.
import { DEFAULT_BINDINGS, chordFor, parseChord } from "./bindings.ts";
import { DEFAULT_BINDINGS, chordFor, keystrokes, parseChord } from "./bindings.ts";
import {
STOCK_EDITOR_KEYMAP,
STOCK_KEYMAPS,
Expand Down Expand Up @@ -137,25 +137,55 @@ check("B2 and the editor never meet on a ⌃ keystroke — the emacs bindings ar
});

check("the editor's own B2 chords are the ones installed ahead of the stock keymap", () => {
// ⌘B / ⌘I / ⌘T / ⇧⌘V go into the editor's keymap; ⌘S is the document handler's. All
// five are scope `editor`, which is what makes the overlap check consider them at all
// — a chord filed under the wrong scope would be compared against the wrong keyboard.
// ⌘B / ⌘I / ⌘T / ⇧⌘V and Tab / ⇧Tab go into the editor's keymap; ⌘S is the document
// handler's. All are scope `editor`, which is what makes the overlap check consider
// them at all — a chord filed under the wrong scope would be compared against the
// wrong keyboard.
const editorIds = DEFAULT_BINDINGS.filter((b) => b.scope === "editor").map((b) => b.id);
assertEq(
editorIds,
["format.bold", "format.italic", "editor.table", "editor.paste-plain", "editor.save"],
[
"format.bold",
"format.italic",
"editor.table",
"editor.paste-plain",
"editor.save",
"editor.list.indent",
"editor.list.outdent",
],
"the editor's chords",
);
});

check("Tab reaches the editor's list commands — nothing in the editor binds it first", () => {
// The assumption `editor.list.indent` rests on, and the reason `markdownKeymap` is in
// STOCK_KEYMAPS now: `markdown()` installs it at Prec.high, *above* B2's own chords,
// so a release that gave it a Tab binding would take the key without a word. The stock
// list is also where `indentWithTab` would show up if it were ever added to
// `defaultKeymap` — that one would silently turn Tab back into plain indentation.
const onTab = editorChords().filter((c) => keystrokes(c.spec).some((f) => f.endsWith("Tab")));
assertEq(
onTab.map((c) => `${c.spec} — ${c.source} ${c.command}`),
[],
"stock chords over Tab",
);
Comment on lines +160 to +171

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The Tab guarantee misses platform-specific stock bindings.

editorChords in ui/src/editorkeys.ts line 80 reads b.mac ?? b.key. It never reads b.win or b.linux. A stock binding that declared Tab only under win or linux would not appear in onTab, and this check would still pass.

The comment at lines 161-165 states the assertion protects editor.list.indent from a future stock Tab binding. On Windows and Linux it does not fully do that.

Either widen editorChords to include b.win and b.linux, or narrow the comment to say the guarantee covers mac and the platform-neutral key only.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/editorkeys.test.ts` around lines 160 - 171, Update editorChords in
editorkeys.ts to include platform-specific b.win and b.linux bindings alongside
b.mac and b.key, so the Tab assertion detects stock bindings on every supported
platform. Preserve the existing chord normalization and ensure the
editor.list.indent test’s guarantee applies consistently across platforms.

});

check("chords handed to CodeMirror are ones CodeMirror can parse", () => {
// `chordFor` gives main.ts the registry's spelling and it goes straight into
// `keymap.of` — which works because the syntax is CodeMirror's, with one exception.
// `Any-` is B2's own, and CodeMirror would read it as a modifier named "Any" and bind
// a chord nothing presses. Nothing installed in the editor uses it today; this is what
// notices if that changes, since the failure is otherwise a chord that silently
// stops working.
const installed = ["format.bold", "format.italic", "editor.table", "editor.paste-plain"];
const installed = [
"format.bold",
"format.italic",
"editor.table",
"editor.paste-plain",
"editor.list.indent",
"editor.list.outdent",
];
for (const id of installed) {
const spec = chordFor(id);
parseChord(spec); // our side
Expand Down
8 changes: 8 additions & 0 deletions ui/src/editorkeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
// and left edit mode. The alias is gone (see `Chord.mod`); this is the check that noticed.
import { completionKeymap } from "@codemirror/autocomplete";
import { defaultKeymap, historyKeymap } from "@codemirror/commands";
import { markdownKeymap } from "@codemirror/lang-markdown";
import type { KeyBinding } from "@codemirror/view";
import { type Binding, type Scope, activeBindings, allKeys, keystrokes } from "./bindings.ts";

Expand All @@ -42,6 +43,13 @@ export const STOCK_KEYMAPS: readonly StockKeymap[] = [
// It declines unless the completion menu is actually open, which is what keeps ⏎ a
// newline the rest of the time.
{ source: "completionKeymap", keymap: completionKeymap },
// Installed by `markdown()` itself (its `addKeymap` default), at Prec.high — so above
// B2's own chords. It was missing here, which is the gap this module exists to close:
// ⏎ continuing a list marker and ⌫ eating one are bindings the editor really runs, and
// nothing checked them. It matters more now that B2 binds Tab (`editor.list.indent`) —
// "CodeMirror leaves Tab alone" is only an assertion if every keymap the editor
// installs is in this list.
{ source: "markdownKeymap", keymap: markdownKeymap },
];

/** What main.ts spreads into the editor's keymap after B2's own chords. `completionKeymap`
Expand Down
Loading