Add word-processor keyboard and mouse actions to docs editor#64
Merged
Conversation
Canvas-based document editor now supports standard text editing shortcuts: word-by-word navigation (Opt/Ctrl+Arrow), word/line deletion, double/triple click selection, Shift+Click, clipboard (copy/paste/cut), document start/end navigation, strikethrough, and clear formatting. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds keyboard and mouse interaction features (word/line/document navigation, multi‑click selection, clipboard copy/cut/paste), word‑boundary utilities with CJK support, strikethrough style support and rendering, tests, and two new documentation notes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TextEditor
participant WordBoundary
participant Model
participant Canvas
User->>TextEditor: Alt+Right / Alt+Left (word navigation)
TextEditor->>WordBoundary: findNextWordBoundary(text, offset) / findPrevWordBoundary(text, offset)
WordBoundary-->>TextEditor: boundary offset
TextEditor->>Model: update cursor/selection
TextEditor->>Canvas: request render(layout, paginatedLayout, ...)
Canvas-->>User: updated view
sequenceDiagram
actor User
participant TextEditor
participant Clipboard
participant Model
participant Canvas
User->>TextEditor: Cmd+C / Cmd+X (copy/cut)
TextEditor->>Model: read selection plaintext
Model-->>TextEditor: plaintext
TextEditor->>Clipboard: write text/plain
TextEditor->>Model: (on cut) snapshot + delete selection
TextEditor->>Canvas: render updated document
User->>TextEditor: Cmd+V (paste)
TextEditor->>Clipboard: read text/plain
Clipboard-->>TextEditor: plaintext
TextEditor->>Model: delete selection, insert text (split into blocks if newline)
TextEditor->>Canvas: render updated document
Canvas-->>User: display pasted content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 88.2s
Verification: verify:integrationResult: ✅ PASS |
Home/End and Cmd+Left/Right now move to the start/end of the visual (wrapped) line instead of the paragraph. This matches standard word processor behavior where long paragraphs wrap into multiple visual lines. Cmd+Backspace also deletes to the visual line start. Co-Authored-By: Claude Opus 4.6 <[email protected]>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/docs/src/view/text-editor.ts (1)
700-723:⚠️ Potential issue | 🟠 MajorUse rendered-line boundaries here, not block boundaries.
These helpers always target offset
0/getBlockTextLength(block), so in a wrapped paragraph Cmd+Left/Right/Home/End and Cmd+Backspace jump or delete to the paragraph bounds instead of the current visual line. You already resolve page/line layout elsewhere in this file; reuse that to compute the active line’s start/end offsets.Also applies to: 755-768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 700 - 723, handleHome and handleEnd currently use block boundaries (offset 0 and getBlockTextLength) which jumps to paragraph bounds; instead, use the editor's rendered-line/layout helpers already used elsewhere in this file to compute the active visual line's start and end offsets for the current cursor position and use those offsets when constructing newPos, updating selection, moving the cursor, and requesting render. Apply the same change to the other command-key handlers that use block boundaries (the Cmd+Left/Cmd+Right/Cmd+Backspace handlers around the 755-768 region) so all command-line navigation/deletion uses rendered-line boundaries rather than whole-block offsets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 239-242: The issue is that mod and wordMod are both derived
incorrectly (mod = ctrlKey || metaKey; wordMod = altKey) causing Ctrl on
Windows/Linux to be treated like Meta on Mac; fix by separating platform-aware
modifiers: compute isMac (e.g. via navigator.platform/userAgent) and set mod =
isMac ? metaKey : ctrlKey and set wordMod = isMac ? altKey : ctrlKey so
Ctrl+Left/Right/Backspace/Delete on Windows/Linux uses word navigation correctly
while Mac keeps Option as the word modifier; update any uses of mod and wordMod
in the key handling code (the key/ctrlKey/metaKey/altKey references and the
Backspace/Arrow branches) to rely on these new values.
- Around line 239-243: The keyboard handler uses the raw KeyboardEvent.key so
Shift+letter yields uppercase and breaks single-letter shortcut matching; create
a normalized key variable (e.g., lowercase single-character keys) before the
switch and use that in all comparisons (replace uses of key in the switch with
the normalized variable), ensuring you handle non-single-character keys
unchanged; apply the same normalization at the other handler occurrence (the
block referenced around lines 352-356) and keep existing modifiers (mod,
wordMod) logic intact.
In `@packages/docs/src/view/word-boundary.ts`:
- Around line 35-40: categorize currently uses the ASCII-only /\w/ check which
misclassifies non-ASCII letters; update categorize (the function that returns
CharCategory and uses isCJK) to detect Unicode letters and digits by using
Unicode property escapes (match Unicode Letter, Mark and Number categories with
the 'u' flag) instead of /\w/, and return CharCategory.Word for those matches so
accented letters, combining marks and non-Latin scripts are treated as words.
---
Outside diff comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 700-723: handleHome and handleEnd currently use block boundaries
(offset 0 and getBlockTextLength) which jumps to paragraph bounds; instead, use
the editor's rendered-line/layout helpers already used elsewhere in this file to
compute the active visual line's start and end offsets for the current cursor
position and use those offsets when constructing newPos, updating selection,
moving the cursor, and requesting render. Apply the same change to the other
command-key handlers that use block boundaries (the
Cmd+Left/Cmd+Right/Cmd+Backspace handlers around the 755-768 region) so all
command-line navigation/deletion uses rendered-line boundaries rather than
whole-block offsets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e243630b-8a22-4676-958a-21ce8af01eb9
📒 Files selected for processing (7)
docs/tasks/active/20260322-docs-keyboard-mouse-lessons.mddocs/tasks/active/20260322-docs-keyboard-mouse-todo.mdpackages/docs/src/model/types.tspackages/docs/src/view/doc-canvas.tspackages/docs/src/view/text-editor.tspackages/docs/src/view/word-boundary.tspackages/docs/test/view/word-boundary.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/docs/src/view/text-editor.ts (2)
239-239:⚠️ Potential issue | 🟠 MajorNormalize printable keys before the switch.
Line 239 uses the raw
KeyboardEvent.key, so shifted letters arrive as uppercase.Mod+Shift+Xwill never matchcase 'x', and the same root cause affects any other shifted letter shortcut.Minimal fix
- const { key, ctrlKey, metaKey, shiftKey, altKey } = e; + const { ctrlKey, metaKey, shiftKey, altKey } = e; + const key = e.key.length === 1 ? e.key.toLowerCase() : e.key;Does the DOM `KeyboardEvent.key` value for printable letter keys become uppercase when Shift is held, while non-printable keys such as `ArrowLeft` keep their standard names?Also applies to: 352-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` at line 239, The code reads the raw KeyboardEvent.key into the local variable key (const { key, ctrlKey, metaKey, shiftKey, altKey } = e) and then switches on it, which fails for shifted printable letters; normalize printable single-character letter keys to a canonical form before the switch (e.g., add a small helper normalizeKey(rawKey) that returns rawKey.toLowerCase() when /^[A-Za-z]$/.test(rawKey) and otherwise returns rawKey), then replace the destructured key or use const normKey = normalizeKey(key) and switch on normKey; apply the same change to the other shortcut-handling block that also switches on key (the region mentioned around the later cases 352-356).
239-242:⚠️ Potential issue | 🟠 MajorSplit command shortcuts from navigation modifiers.
Line 240 still uses
ctrlKey || metaKeyfor every special branch, while Line 242 hard-codes word navigation toaltKey. On Windows/Linux that makesCtrl+Left/RightandCtrl+Backspacetake the Cmd/line path,Ctrl+Deletefall back to plain delete, andCtrl+Up/Downjump to the document ends. Please break this into separate shortcut vs. word/line/document modifiers instead of reusingmodhere.Also applies to: 253-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 239 - 242, The handler currently conflates shortcut modifier and navigation modifiers by using mod (ctrlKey || metaKey) for shortcuts and hard-coding wordMod = altKey, causing wrong behavior on Windows/Linux; change to three distinct booleans: shortcutMod (ctrlKey || metaKey) for shortcut/command actions, wordMod (on Mac use altKey, otherwise use ctrlKey) for word-level navigation, and line/document modifiers as needed (e.g., shortcutMod or a dedicated lineMod) so Up/Down and Delete/Backspace branches use the correct modifier. Update the variables replacing mod and wordMod (the existing mod and wordMod in the key handler) and apply the same split to the subsequent key-branch logic referenced in the block covering lines 253-327 so each branch checks the appropriate modifier boolean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 614-618: The current handleLineBackspace path calls
this.doc.deleteBackward(pos) unconditionally which assumes a previous block
exists and can dereference blocks[-1]; add a guard that detects document-start
(e.g., pos.offset === 0 && pos.blockIndex === 0) and no-ops instead of calling
this.doc.deleteBackward(), so only call invalidateLayout() and deleteBackward
when there is a previous block to merge; apply the same guard to the other
similar site around lines 769-771, or alternatively harden Doc.deleteBackward()
to safely handle offsets at the very start of the document — reference
handleLineBackspace, this.doc.deleteBackward, this.cursor.moveTo, and
invalidateLayout when making the fix.
- Around line 795-803: The clearFormatting() command mutates the document via
this.doc.applyInlineStyle(range, ...) without recording an undo snapshot; call
the editor snapshot method (e.g., this.saveSnapshot()) immediately after
verifying the selection (after checking this.selection.hasSelection() and
this.selection.range but before calling this.doc.applyInlineStyle) so the change
is pushed to the undo stack; update clearFormatting() to invoke saveSnapshot()
prior to applyInlineStyle() and then proceed with the existing style clears.
- Around line 955-965: The caret-at-wrap bug is caused by treating pos.offset ==
lineEnd as belonging to the previous visual line; in the loop over lb.lines and
line.runs (using charsBefore, lineChars, lineStart, lineEnd, pos.offset) change
the boundary check to treat the end offset as belonging to the next visual line
— e.g. replace the current condition (pos.offset >= lineStart && pos.offset <=
lineEnd) with a boundary-safe comparison that uses strict less-than for the end
(pos.offset >= lineStart && pos.offset < lineEnd) or explicitly skip when
pos.offset === lineEnd so Home/End/Cmd+Backspace operate on the next wrapped
line.
---
Duplicate comments:
In `@packages/docs/src/view/text-editor.ts`:
- Line 239: The code reads the raw KeyboardEvent.key into the local variable key
(const { key, ctrlKey, metaKey, shiftKey, altKey } = e) and then switches on it,
which fails for shifted printable letters; normalize printable single-character
letter keys to a canonical form before the switch (e.g., add a small helper
normalizeKey(rawKey) that returns rawKey.toLowerCase() when
/^[A-Za-z]$/.test(rawKey) and otherwise returns rawKey), then replace the
destructured key or use const normKey = normalizeKey(key) and switch on normKey;
apply the same change to the other shortcut-handling block that also switches on
key (the region mentioned around the later cases 352-356).
- Around line 239-242: The handler currently conflates shortcut modifier and
navigation modifiers by using mod (ctrlKey || metaKey) for shortcuts and
hard-coding wordMod = altKey, causing wrong behavior on Windows/Linux; change to
three distinct booleans: shortcutMod (ctrlKey || metaKey) for shortcut/command
actions, wordMod (on Mac use altKey, otherwise use ctrlKey) for word-level
navigation, and line/document modifiers as needed (e.g., shortcutMod or a
dedicated lineMod) so Up/Down and Delete/Backspace branches use the correct
modifier. Update the variables replacing mod and wordMod (the existing mod and
wordMod in the key handler) and apply the same split to the subsequent
key-branch logic referenced in the block covering lines 253-327 so each branch
checks the appropriate modifier boolean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ec3e966-8ea6-45ec-a74c-4281f66c540d
📒 Files selected for processing (1)
packages/docs/src/view/text-editor.ts
Cursor now skips trailing whitespace at line wrap boundaries, matching Google Docs behavior. Moving right past the last visible character on a wrapped line jumps to the next line start. Moving left from a line start skips back over trailing spaces. End key also stops at the last non-space character on wrapped lines. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 962-991: The skipLineWrapSpaces and skipLineWrapSpacesBackward
functions read line boundaries from this.getLayout(), which can be stale right
after edits; update the layout for the specific block synchronously before using
it (e.g., call the existing synchronous relayout/recompute method for a single
block such as this.relayoutBlock(blockId) or
this.recomputeLayoutForBlock(blockId) — whichever exists in the codebase — and
fall back to the current getLayout() only after that) so the functions use
up-to-date line boundaries; apply the same change in both skipLineWrapSpaces and
skipLineWrapSpacesBackward and reference getLayout, skipLineWrapSpaces,
skipLineWrapSpacesBackward, and the blockId parameter to locate where to inject
the synchronous relayout call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4daa78c-8e40-4743-bc9b-1577c3bd933b
📒 Files selected for processing (1)
packages/docs/src/view/text-editor.ts
Trailing whitespace at the end of wrapped lines is no longer rendered, matching Google Docs behavior. The cursor also renders at the next line start when positioned at a trailing space offset on a wrapped line. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The trailing space skip and visual trimming logic made cursor behavior worse. Removing it to re-implement properly in a follow-up PR. Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Normalize key to lowercase for Shift+letter shortcuts - Add platform-aware modifier detection (Mac vs Win/Linux) - Guard document-start before calling deleteBackward() - Add undo snapshot to clearFormatting() - Fix wrap-boundary offset in getVisualLineRange - Use Unicode property escapes for word boundary Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/docs/src/view/text-editor.ts (1)
410-418: Consider callinginvalidateLayout()once before the loop.Currently
invalidateLayout()is called inside the loop for each new line, but a single call before the loop would suffice since it just clears the cache.♻️ Optional optimization
+ if (lines.length > 1) { + this.invalidateLayout(); + } for (let i = 0; i < lines.length; i++) { if (i > 0) { // Create a new block for each line after the first - this.invalidateLayout(); const newBlockId = this.doc.splitBlock(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/text-editor.ts` around lines 410 - 418, Move the call to invalidateLayout() out of the per-line loop and invoke it once before iterating lines (e.g., if lines.length > 1) to avoid repeated cache clears; remove the invalidateLayout() call that currently sits inside the for loop that creates new blocks via this.doc.splitBlock and moves the cursor with this.cursor.moveTo so behavior remains the same but with a single cache invalidation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 390-398: The handleCut implementation may delete selection even
when e.clipboardData is null; update handleCut to first attempt to write the
selected text to the clipboard (use e.clipboardData when present, otherwise fall
back to navigator.clipboard.writeText), and only proceed to saveSnapshot(),
deleteSelection(), and requestRender() after confirming the clipboard write
succeeded (handle promise rejections and abort deletion on failure); reference
handleCut, this.selection.getSelectedText, e.clipboardData,
navigator.clipboard.writeText, saveSnapshot, deleteSelection, and requestRender
to locate and protect the critical path.
---
Nitpick comments:
In `@packages/docs/src/view/text-editor.ts`:
- Around line 410-418: Move the call to invalidateLayout() out of the per-line
loop and invoke it once before iterating lines (e.g., if lines.length > 1) to
avoid repeated cache clears; remove the invalidateLayout() call that currently
sits inside the for loop that creates new blocks via this.doc.splitBlock and
moves the cursor with this.cursor.moveTo so behavior remains the same but with a
single cache invalidation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b52cae0b-8a13-40ce-b253-ddd523c7ed61
📒 Files selected for processing (3)
packages/docs/src/view/doc-canvas.tspackages/docs/src/view/editor.tspackages/docs/src/view/text-editor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/docs/src/view/doc-canvas.ts
Comment on lines
+390
to
+398
| private handleCut = (e: ClipboardEvent): void => { | ||
| if (!this.selection.hasSelection()) return; | ||
| e.preventDefault(); | ||
| const text = this.selection.getSelectedText(this.getLayout()); | ||
| e.clipboardData?.setData('text/plain', text); | ||
| this.saveSnapshot(); | ||
| this.deleteSelection(); | ||
| this.requestRender(); | ||
| }; |
There was a problem hiding this comment.
handleCut may silently lose text if clipboard write fails.
If e.clipboardData is null (rare but possible in certain browser contexts), the optional chaining silently skips the clipboard write, but the subsequent lines still delete the selection. The user would lose their text without it being copied.
🛡️ Proposed fix to guard against clipboard failure
private handleCut = (e: ClipboardEvent): void => {
if (!this.selection.hasSelection()) return;
e.preventDefault();
+ if (!e.clipboardData) return;
const text = this.selection.getSelectedText(this.getLayout());
- e.clipboardData?.setData('text/plain', text);
+ e.clipboardData.setData('text/plain', text);
this.saveSnapshot();
this.deleteSelection();
this.requestRender();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private handleCut = (e: ClipboardEvent): void => { | |
| if (!this.selection.hasSelection()) return; | |
| e.preventDefault(); | |
| const text = this.selection.getSelectedText(this.getLayout()); | |
| e.clipboardData?.setData('text/plain', text); | |
| this.saveSnapshot(); | |
| this.deleteSelection(); | |
| this.requestRender(); | |
| }; | |
| private handleCut = (e: ClipboardEvent): void => { | |
| if (!this.selection.hasSelection()) return; | |
| e.preventDefault(); | |
| if (!e.clipboardData) return; | |
| const text = this.selection.getSelectedText(this.getLayout()); | |
| e.clipboardData.setData('text/plain', text); | |
| this.saveSnapshot(); | |
| this.deleteSelection(); | |
| this.requestRender(); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs/src/view/text-editor.ts` around lines 390 - 398, The handleCut
implementation may delete selection even when e.clipboardData is null; update
handleCut to first attempt to write the selected text to the clipboard (use
e.clipboardData when present, otherwise fall back to
navigator.clipboard.writeText), and only proceed to saveSnapshot(),
deleteSelection(), and requestRender() after confirming the clipboard write
succeeded (handle promise rejections and abort deletion on failure); reference
handleCut, this.selection.getSelectedText, e.clipboardData,
navigator.clipboard.writeText, saveSnapshot, deleteSelection, and requestRender
to locate and protect the critical path.
This was referenced Mar 22, 2026
This was referenced Apr 19, 2026
5 tasks
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
1 participant
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.
Summary
Test plan
pnpm verify:fastpasses (all 1271 tests)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests