ui: Tab nests a list item, ⇧Tab lifts it back out - #131
Merged
Conversation
Nesting a list was the one structural edit the editor had no gesture for. Enter continues a marker and ⌘B wraps a word, but making `- b` a child of `- a` meant counting spaces by hand — and the key everyone reaches for first, Tab, was unbound, so it did what an unbound Tab does in a webview: walked the focus ring straight out of the buffer and into the next pane. Tab is claimed **only with the caret in a list item**. Everywhere else in the note `indentList` returns null, the binding declines, and Tab goes on stepping the focus ring — which is why this isn't CodeMirror's own `indentWithTab`, a one-liner that takes the key outright and takes the keyboard's way out of the editor with it. Indenting a paragraph would make a code block anyway, which nobody means by Tab. Inside a list the key is swallowed even when nothing can move (the first item of a list has nothing to nest under): a gesture that *sometimes* ejects you from the buffer is worse than one that sometimes does nothing. - ui/src/list.ts — the engine, pure and node-testable (the format.ts pattern). It edits leading whitespace and the digits of an ordered marker, and nothing else. The new indent is the previous sibling's **content column**, not a fixed two spaces, because that is where CommonMark puts a child — indent under `1. a` by two and the nesting simply doesn't parse. An item's subtree travels with it, or ⇧Tab would re-parent the children onto whatever the item landed beside. Ordered runs are renumbered, both the one an item left and the one it joined: `2.` nested out of `1. 2. 3.` is a list whose first item says "2.", and renders as "2.". The start number stays the author's where they chose it (a list opening at `5.` goes on opening at `5.`), and the lazy `1. 1. 1.` style is left alone — it renders identically and nothing moved into or out of it. - main.ts — `runListShift`, the CodeMirror half. A caret inside code declines before the engine is asked: a `- item` line in a fence is text, not structure, and `inCodeContext` is the same read the rich paste makes. - bindings.ts / shortcuts.ts — declared once, with a row in the sheet, so the chords are rebindable and findable like every other (obligation 4). editorkeys.ts gains `markdownKeymap` in STOCK_KEYMAPS, which was a real gap: `markdown()` installs it at Prec.high, *above* B2's own chords, and nothing compared it against them. Its ⏎ and ⌫ overlap nothing, but "CodeMirror leaves Tab alone" is only an assertion if every keymap the editor installs is in that list — so editorkeys.test.ts now asserts it directly, and would catch an `indentWithTab` arriving in `defaultKeymap` as much as a markdown binding. list.test.ts covers the gesture in 29 checks, asserting on the Markdown that comes out rather than the change list — including the two shapes that read as bugs when they're wrong: the ordered renumbering, and Tab declining outside a list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oc7gyoL7AccTEF2picayR
Review catch on #130. `groupOf` collected a renumbering run by asking only "ordered or bullet?", but CommonMark starts a new list at every change of *marker* — `1.` then `2)`, or `-` then `*` — so a run could span two lists and the renumbering would walk straight across the boundary. Two failures, one boundary: 1. a 1. a 1) x -> 2) x <- rewritten; `1) x` heads its own list 2) y 3) y 3) z 1) z 1. a 1. a 5) x -> 2) x <- the author's start number, lost 6) y 1) y The second is why the fix isn't only in `groupOf`. `renumber` decides a run's start number by asking whether its lead item *was* the head of its run, and that question has to be about the list as well: `5)` under a `1.` item is a head — its own — and reading the line above as a sibling restarts it at 1. So `Marker` keeps the marker's identity (`kind`), and the two sibling walks gain run-scoped twins that stop at a change of it. `prevSibling` itself is left alone on purpose: "which item am I nested under?" is a question about columns, and `* b` landing under `- a` is the shape the author asked for by pressing Tab (pinned by "the bullet character is the author's"). "Which items share my numbering?" is a question about the list. The two answers part company exactly here, which is why they are now two functions. The bullet half of the boundary was already inert — a run containing a bullet has no numbers, so `renumber` returns before it can do anything — but the walks read the same either way and the comment above them claimed the rule already. list.test.ts pins both documents above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oc7gyoL7AccTEF2picayR
The gesture's contract — a caret in a list never hands Tab back to the focus ring — held only on marker lines. Three places the caret is visibly in a list still ejected it from the buffer: - A continuation line, an item's own wrapped text. `owningItem` now attributes it to the item it belongs to, and the gesture moves that item, subtree and all. A lazy column-0 line reaches its owner through adjacency only: past a blank it is a new paragraph (CommonMark), and a column-0 block starter — a heading, a rule, a fence, a quote — interrupts a paragraph rather than continuing one, so neither claims. - The blank interior to a loose list: claimed and swallowed. Interior means list material directly on both sides with a real item above it in the block — a trailing blank, either blank of a two-blank gap, and the line above a list are document space, and Tab moves on. - A blockquoted list. `> - a` is a bullet behind a container prefix the scanner doesn't parse, so the engine still says null — and main.ts's new `inListItem` gives the syntax tree the last word before the key goes back to the focus ring: claimed-but-inert, the caret being visibly on a bullet. The contained-list edit itself stays unbuilt; the contract holds anyway. list.test.ts covers each: the continuation gesture in both directions, the lazy line, the paragraph and the block starter that are not continuations, the interior blank, the three kinds of document space, and the blockquote null with a comment naming whose case it now is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LWAKbV3HFnjbgejJU6tGB
📝 WalkthroughWalkthroughThe PR adds CommonMark-aware list indentation and outdentation. It connects Tab and Shift-Tab to CodeMirror, preserves editor focus when appropriate, validates keymap conflicts, and documents the new shortcuts. ChangesMarkdown list editing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CodeMirror
participant EditorKeymap
participant ListEngine
CodeMirror->>EditorKeymap: receive Tab or Shift-Tab
EditorKeymap->>ListEngine: invoke indentList or outdentList
ListEngine-->>EditorKeymap: return ListEdit or null
EditorKeymap-->>CodeMirror: apply document changes and selection
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ui/src/list.test.ts (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive
assertan assertion signature to drop the redundant fallbacks.
assertreturnsvoid, so TypeScript does not narrow the checked value. That forcesthere?.changes ?? []andthere?.selFrom ?? 0at lines 183-186 even though line 182 already provedthere !== null. The fallbacks hide a real failure as a passing round trip if the narrowing assumption ever breaks.An
asserts condsignature narrows at the call site.♻️ Proposed signature change
-function assert(cond: boolean, msg: string): void { +function assert(cond: boolean, msg: string): asserts cond { if (!cond) throw new Error(`assertion failed: ${msg}`); }The call sites then simplify, for example:
const there = indentList(start, 6, 6); assert(there !== null, "indent applied"); - const mid = applyChanges(start, there?.changes ?? []); - const back = outdentList(mid, there?.selFrom ?? 0, there?.selTo ?? 0); + const mid = applyChanges(start, there.changes); + const back = outdentList(mid, there.selFrom, there.selTo);🤖 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/list.test.ts` around lines 16 - 18, Update the assert function’s return type to an assertion signature, `asserts cond`, so successful calls narrow the checked condition at call sites. Then remove the redundant optional-chaining and fallback defaults in the round-trip assertions using there, while preserving the existing null check and direct property access.ui/src/editorkeys.test.ts (1)
181-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
installedfrom the editor scope instead of restating it.This list repeats the editor-scope ids from lines 148-154 with
editor.saveremoved. Adding a future editor binding requires updating both lists. If only the first is updated, this check silently stops covering the new chord, and the failure mode is a chord that stops working with no test failure.Derive the list from
DEFAULT_BINDINGSand exclude the one id the document handler owns.♻️ Proposed derivation
- const installed = [ - "format.bold", - "format.italic", - "editor.table", - "editor.paste-plain", - "editor.list.indent", - "editor.list.outdent", - ]; + // Every editor-scope chord except ⌘S, which the document handler owns rather than + // CodeMirror — see the scope comment in bindings.ts. + const installed = DEFAULT_BINDINGS.filter( + (b) => b.scope === "editor" && b.id !== "editor.save", + ).map((b) => b.id);🤖 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 181 - 188, Update the installed-key list in the relevant test to derive editor-scope IDs from DEFAULT_BINDINGS, excluding the editor.save binding owned by the document handler, instead of maintaining a duplicated literal list. Preserve the existing assertions against the derived collection so newly added editor bindings are covered automatically.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@ui/src/editorkeys.test.ts`:
- Around line 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.
In `@ui/src/list.ts`:
- Around line 106-147: Update scan to track fenced-code state while iterating
through document lines, toggling state for fence opener and closer lines before
item classification so both are excluded. When the state indicates the line is
inside a fence, bypass ITEM matching and emit a plain line entry, preventing
list markers within fenced content from becoming Marker objects.
In `@ui/src/main.ts`:
- Around line 2512-2524: Update the inCodeContext branch of runListShift so Tab
is consumed inside code fences instead of returning false and allowing focus to
leave the editor; return true there, or apply the editor’s standard indent unit,
while preserving the existing no-ejection behavior outside code context.
---
Nitpick comments:
In `@ui/src/editorkeys.test.ts`:
- Around line 181-188: Update the installed-key list in the relevant test to
derive editor-scope IDs from DEFAULT_BINDINGS, excluding the editor.save binding
owned by the document handler, instead of maintaining a duplicated literal list.
Preserve the existing assertions against the derived collection so newly added
editor bindings are covered automatically.
In `@ui/src/list.test.ts`:
- Around line 16-18: Update the assert function’s return type to an assertion
signature, `asserts cond`, so successful calls narrow the checked condition at
call sites. Then remove the redundant optional-chaining and fallback defaults in
the round-trip assertions using there, while preserving the existing null check
and direct property access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c2d6eeb-d7b0-4d34-b3b3-7d1458ba38b2
📒 Files selected for processing (7)
ui/src/bindings.tsui/src/editorkeys.test.tsui/src/editorkeys.tsui/src/list.test.tsui/src/list.tsui/src/main.tsui/src/shortcuts.ts
Comment on lines
+160
to
+171
| 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", | ||
| ); |
There was a problem hiding this comment.
🎯 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.
Comment on lines
+106
to
+147
| function scan(doc: string): Ln[] { | ||
| const out: Ln[] = []; | ||
| let from = 0; | ||
| for (const text of doc.split("\n")) { | ||
| const ws = leadingWs(text); | ||
| const blank = ws.length === text.length; | ||
| const indent = measure(ws); | ||
| const m = blank || RULE.test(text) ? null : ITEM.exec(text); | ||
| if (!m) { | ||
| out.push({ from, text, blank, indent }); | ||
| } else { | ||
| // An unmatched group is undefined at runtime, which `RegExpExecArray`'s `string[]` | ||
| // index signature doesn't say — hence the explicit types rather than a destructure | ||
| // that would read as total. | ||
| const lead: string = m[1]; | ||
| const bullet: string | undefined = m[2]; | ||
| const digits: string | undefined = m[3]; | ||
| const delim: string | undefined = m[4]; | ||
| const gap: string | undefined = m[5]; | ||
| const markerText = bullet ?? `${digits}${delim}`; | ||
| const afterMarker = measure(lead + markerText); | ||
| // CommonMark: content sits one column past the marker plus the gap — except that a | ||
| // gap of five or more spaces is code indentation, and an item with no content at | ||
| // all has no gap to measure. Both cases put the content column one past the marker. | ||
| const gapped = gap === undefined ? afterMarker + 1 : measure(lead + markerText + gap); | ||
| const item: Marker = { | ||
| indent, | ||
| indentLen: lead.length, | ||
| content: gapped - afterMarker > 4 ? afterMarker + 1 : gapped, | ||
| // The alternation matched one branch or the other, so one of these is a string. | ||
| kind: bullet ?? delim ?? "", | ||
| }; | ||
| if (digits !== undefined) { | ||
| item.num = Number(digits); | ||
| item.numLen = digits.length; | ||
| } | ||
| out.push({ from, text, blank, indent, item }); | ||
| } | ||
| from += text.length + 1; | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
scan classifies fenced code content as list items.
scan reads every line independently. A line inside a fenced code block that starts with - or 1. becomes a Marker. The engine can then re-indent or renumber that line when it falls inside the moved subtree or a renumbered run. inCodeContext in ui/src/main.ts only guards the caret position, so it does not prevent this.
The header excludes blockquoted lists but says nothing about fences. Track the fence state in scan and skip item classification inside it.
🐛 Proposed fence tracking in `scan`
+/** An open or close fence: three or more backticks or tildes, up to three columns in. */
+const FENCE = /^ {0,3}(`{3,}|~{3,})/;
+
/** Read the document as lines, each classified as a list item or not. */
function scan(doc: string): Ln[] {
const out: Ln[] = [];
let from = 0;
+ let fence: string | null = null;
for (const text of doc.split("\n")) {
const ws = leadingWs(text);
const blank = ws.length === text.length;
const indent = measure(ws);
- const m = blank || RULE.test(text) ? null : ITEM.exec(text);
+ const f = FENCE.exec(text)?.[1];
+ if (fence === null && f !== undefined) fence = f[0];
+ else if (fence !== null && f !== undefined && f[0] === fence) fence = null;
+ const m = fence !== null || blank || RULE.test(text) ? null : ITEM.exec(text);Note: the closing fence is itself inside the fence, so the toggle order above keeps the opener and the closer out of item classification.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 113-113: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/list.ts` around lines 106 - 147, Update scan to track fenced-code
state while iterating through document lines, toggling state for fence opener
and closer lines before item classification so both are excluded. When the state
indicates the line is inside a fence, bypass ITEM matching and emit a plain line
entry, preventing list markers within fenced content from becoming Marker
objects.
Comment on lines
+2512
to
+2524
| * so the no-ejection contract holds there too. And a caret inside code declines before | ||
| * the engine is asked at all: a `- item` line in a fence is text, not structure, and | ||
| * `inCodeContext` is the same read the rich paste makes to keep its hands off code. | ||
| * | ||
| * One range rather than `changeByRange`: an indent moves every offset after it, so a | ||
| * second cursor's edit would be computed against a document the first has already | ||
| * shifted. Multi-cursor nesting is a gesture nobody makes; ⌘B's is one they do. | ||
| */ | ||
| function runListShift( | ||
| view: EditorView, | ||
| shift: (doc: string, from: number, to: number) => ListEdit | null, | ||
| ): boolean { | ||
| if (inCodeContext(view.state)) return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tab inside a code fence moves focus out of the editor.
Line 2524 returns false in a code context. editorkeys.test.ts now asserts that no stock CodeMirror keymap binds Tab, so nothing else claims the key and the browser default runs. The caret then leaves the buffer.
The doc comment explains why the engine must not run there. It does not explain why the key is released rather than swallowed. The no-ejection contract that lines 2507-2512 describe applies just as well inside a fence, where a code block is the one place a user is most likely to expect Tab to indent.
Confirm this is intended. If it is not, return true in the code context, or insert an indent unit there.
🤖 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/main.ts` around lines 2512 - 2524, Update the inCodeContext branch of
runListShift so Tab is consumed inside code fences instead of returning false
and allowing focus to leave the editor; return true there, or apply the editor’s
standard indent unit, while preserving the existing no-ejection behavior outside
code context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nesting a list was the one structural edit the editor had no gesture for.
Enter continues a marker and ⌘B wraps a word, but making
- ba child of- ameant counting spaces by hand — and the key everyone reaches for first,Tab, was unbound, so it did what an unbound Tab does in a webview: walked the
focus ring straight out of the buffer and into the next pane.
Tab is claimed only with the caret in a list item. Everywhere else in the
note
indentListreturns null, the binding declines, and Tab goes on steppingthe focus ring — which is why this isn't CodeMirror's own
indentWithTab, aone-liner that takes the key outright and takes the keyboard's way out of the
editor with it. Indenting a paragraph would make a code block anyway, which
nobody means by Tab. Inside a list the key is swallowed even when nothing can
move (the first item of a list has nothing to nest under): a gesture that
sometimes ejects you from the buffer is worse than one that sometimes does
nothing.
It edits leading whitespace and the digits of an ordered marker, and nothing
else. The new indent is the previous sibling's content column, not a
fixed two spaces, because that is where CommonMark puts a child — indent
under
1. aby two and the nesting simply doesn't parse. An item's subtreetravels with it, or ⇧Tab would re-parent the children onto whatever the item
landed beside. Ordered runs are renumbered, both the one an item left and
the one it joined:
2.nested out of1. 2. 3.is a list whose first itemsays "2.", and renders as "2.". The start number stays the author's where
they chose it (a list opening at
5.goes on opening at5.), and the lazy1. 1. 1.style is left alone — it renders identically and nothing movedinto or out of it.
runListShift, the CodeMirror half. A caret inside code declinesbefore the engine is asked: a
- itemline in a fence is text, notstructure, and
inCodeContextis the same read the rich paste makes.chords are rebindable and findable like every other (obligation 4).
editorkeys.ts gains
markdownKeymapin STOCK_KEYMAPS, which was a real gap:markdown()installs it at Prec.high, above B2's own chords, and nothingcompared it against them. Its ⏎ and ⌫ overlap nothing, but "CodeMirror leaves
Tab alone" is only an assertion if every keymap the editor installs is in that
list — so editorkeys.test.ts now asserts it directly, and would catch an
indentWithTabarriving indefaultKeymapas much as a markdown binding.list.test.ts covers the gesture in 29 checks, asserting on the Markdown that
comes out rather than the change list — including the two shapes that read as
bugs when they're wrong: the ordered renumbering, and Tab declining outside a
list.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_015oc7gyoL7AccTEF2picayR
Summary by CodeRabbit
New Features
Documentation