Repository navigation
fix(editor): make list toolbar actions work in empty editor - #1629
Conversation
Blank lines were skipped by the line replacement logic, causing ordered and unordered list toolbar actions to do nothing in an empty editor.
List commands were calling getCursor on the raw EditorView retained by the command adapter. Read the current line directly from CodeMirror state so toolbar actions reach the list insertion logic.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The added pnpm test script is likely not running the new deep-nested test file due to non-recursive glob expansion in typical package-script shells.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Fixes the editor toolbar’s ordered/unordered list actions so they work when invoked in an empty (or blank-line) CodeMirror editor, and adds regression coverage plus a small Sass cleanup.
Changes:
- Update list insertion commands to read the active line via CodeMirror selection state and insert a Markdown list marker on blank lines.
- Add Node
--test-based regression tests for ordered and unordered list toolbar commands through the CodeMirror adapter. - Reorder a Sass declaration in the comment component to avoid Sass mixed-declaration warnings.
| File | Description |
|---|---|
| ui/src/components/Editor/utils/codemirror/commands.ts | Inserts list markers on blank lines and reads active line from CodeMirror selection state. |
| ui/src/components/Editor/utils/codemirror/commands.test.cjs | Adds regression tests for list toolbar behavior via the CodeMirror adapter. |
| ui/src/components/Comment/index.scss | Moves border-bottom before nested rules to avoid Sass warnings. |
| ui/package.json | Adds a pnpm test script for running the new Node tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
robinv8
left a comment
robinv8
left a comment
There was a problem hiding this comment.
Thanks @hgaol, nice catch and clean fix! 🙌
The root cause makes sense: createCommandMethods receives the raw CodeMirror EditorView, while getCursor() only exists on the adapter. So insertOrderedList / insertUnorderedList threw before doing anything, and the toolbar list buttons were silently dead (#1620). Reading the line from state.selection.main.head removes that dependency. The new blank-line branch makes the empty-editor case insert 1. / - with the cursor placed after the marker, as a single undoable transaction. The regression tests through the real adapter are a good addition too.
Impact-wise, this only changes the two list actions. Other toolbar buttons are untouched, and the SCSS reorder doesn't change the rendered CSS.
A few non-blocking notes for follow-ups:
toggleOrderedList/toggleUnorderedListand theisHeading/isBlockquote/isCodeBlock/isOrderedList/isUnorderedListhelpers incommands.tsstill calleditor.getCursor()on the raw view, so they'd hit the same error if anything calls them. Could be worth a follow-up.- Now that the non-blank path is reachable, note that
replaceLinesprefixes every non-blank line in the document, not just the current line or selection. That's pre-existing behavior, but users may notice it now. - The new
commands.test.cjsisn't run by CI yet. Fine for now; it would be nice to wire it into a test command later.
LGTM, approving. Thanks again!
Fixes #1620
Proposed Changes
adapter-only
getCursor()method.1.or-) when a list action isused on a blank line.
through the CodeMirror adapter.
mixed-declaration warning.
Verification
cd ui && pnpm testcd ui && pnpm build