fix(rich-text-editor): DP-188940 fix cursor jump after toggleCodeBlock - #1327
fix(rich-text-editor): DP-188940 fix cursor jump after toggleCodeBlock#1327briandial wants to merge 2 commits into
Conversation
briandial
added
the
no-visual-test
|
Fixes cursor jump when toggling multi-line code blocks by tracking the previous selection anchor, remapping it with left-bias when exiting code blocks, and applying setTextSelection via setTimeout(0) to avoid ProseMirror DOM reconciliation overriding the selection. Overall Judgement: WalkthroughThe rich text editor component adds cursor-jump correction for code block toggles. When exiting a code block via transaction, the component remaps the previous selection anchor using left-biased mapping and asynchronously restores the corrected position to prevent unwanted cursor movement. ChangesCursor jump mitigation
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)packages/dialtone-vue/components/rich_text_editor/rich_text_editor.test.jsWarning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry Comment |
|
Please add either the |
After toggleCodeBlock converts a multi-line code block back to paragraphs, the cursor jumped to the start of the next paragraph. ProseMirror's replaceWith uses right-biased step mapping (assoc=1) by default, so each newline-turned-paragraph-boundary pushes the cursor forward. Fix: track prevAnchor and prevInCodeBlock via selectionUpdate, then on any doc-changing transaction that exits a code block, remap the anchor with left-bias (assoc=-1) and dispatch setTextSelection in setTimeout(0) after ProseMirror's own step mapping has run.
0896531 to
364b931
Compare
briandial
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0896531c5c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| }); | ||
| this.editor.on('transaction', ({ editor: txEd, transaction }) => { | ||
| if (!transaction.docChanged || !prevInCodeBlock || txEd.isActive('codeBlock')) return; | ||
| const corrected = transaction.mapping.map(prevAnchor, -1); |
There was a problem hiding this comment.
When toggleCodeBlock exits a code block it replaces the entire codeBlock node with new paragraph nodes (see the tr.replaceWith(...) command in this file), so transaction.mapping.map(prevAnchor, -1) cannot map an old cursor position inside the deleted node to the corresponding text offset in the inserted paragraphs; with left bias it resolves to the start of the replacement. In the scenario this change is meant to fix—toggling a multi-line code block back to paragraphs with the cursor in the middle or on a later line—the timeout will move the cursor to the start of the converted block rather than keeping it on the current line.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/dialtone-vue/components/rich_text_editor/rich_text_editor.vue`:
- Around line 1330-1346: Add a unit/integration test that verifies the cursor
correction logic around exiting a multi-line code block: create an editor
instance, insert a multi-line codeBlock, set the caret to a non-zero anchor
position, simulate toggling codeBlock to paragraph (triggering selectionUpdate
and transaction handlers), and assert that the final selection.anchor equals the
left-biased mapped position (the corrected value computed from prevAnchor using
transaction.mapping.map with assoc=-1) and that any immediate subsequent editor
commands are not overwritten by the deferred setTextSelection; target the
handlers and symbols prevAnchor, prevInCodeBlock, selectionUpdate, transaction,
setTextSelection, and isDestroyed in your test to reproduce and validate the
deferred correction behavior.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 148ae436-e71b-434a-8bc9-ad807c771814
📒 Files selected for processing (1)
packages/dialtone-vue/components/rich_text_editor/rich_text_editor.vue
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
dialpad/ios(manual)dialpad/firespotter(manual)
…lock ghost-paragraph
|
✔️ Deploy previews ready! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/dialtone-vue/components/rich_text_editor/rich_text_editor.test.js`:
- Around line 2185-2186: Replace the loose non-empty assertion with a specific
equality check to ensure the cursor is in the expected paragraph: use the
resolved position via ed.state.doc.resolve(ed.state.selection.anchor) (stored as
$anchor) and assert $anchor.parent.textContent === 'line two' instead of
checking not.toBe(''), so the test fails if the cursor lands in any other
non-empty paragraph.
- Around line 2179-2182: Wrap the fake timer usage around the toggleCodeBlock
call in a try/finally to ensure vi.useRealTimers() always runs: call
vi.useFakeTimers() before ed.commands.toggleCodeBlock(), run vi.runAllTimers()
inside the try block after the command, and call vi.useRealTimers() inside
finally; update the block that currently contains vi.useFakeTimers(),
ed.commands.toggleCodeBlock(), vi.runAllTimers(), vi.useRealTimers() accordingly
so timers are restored even if the command or assertions throw.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: bc79d63a-0aab-46af-a7cb-5556c9364075
📒 Files selected for processing (1)
packages/dialtone-vue/components/rich_text_editor/rich_text_editor.test.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
dialpad/ios(manual)dialpad/firespotter(manual)
| vi.useFakeTimers(); | ||
| ed.commands.toggleCodeBlock(); | ||
| vi.runAllTimers(); | ||
| vi.useRealTimers(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap fake timer usage in try/finally to prevent test pollution.
If an error or assertion failure occurs between useFakeTimers() and useRealTimers(), fake timers will leak into subsequent tests, causing flaky failures.
🔒 Proposed fix
- vi.useFakeTimers();
- ed.commands.toggleCodeBlock();
- vi.runAllTimers();
- vi.useRealTimers();
- await wrapper.vm.$nextTick();
+ vi.useFakeTimers();
+ try {
+ ed.commands.toggleCodeBlock();
+ vi.runAllTimers();
+ await wrapper.vm.$nextTick();
+ } finally {
+ vi.useRealTimers();
+ }🤖 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 `@packages/dialtone-vue/components/rich_text_editor/rich_text_editor.test.js`
around lines 2179 - 2182, Wrap the fake timer usage around the toggleCodeBlock
call in a try/finally to ensure vi.useRealTimers() always runs: call
vi.useFakeTimers() before ed.commands.toggleCodeBlock(), run vi.runAllTimers()
inside the try block after the command, and call vi.useRealTimers() inside
finally; update the block that currently contains vi.useFakeTimers(),
ed.commands.toggleCodeBlock(), vi.runAllTimers(), vi.useRealTimers() accordingly
so timers are restored even if the command or assertions throw.
Source: Coding guidelines
| const $anchor = ed.state.doc.resolve(ed.state.selection.anchor); | ||
| expect($anchor.parent.textContent).not.toBe(''); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider a more specific assertion.
Checking that textContent is not empty correctly validates the cursor didn't jump to the ghost paragraph. However, verifying it equals 'line two' would more precisely confirm the cursor is in the expected paragraph and catch regressions where the cursor lands in a different non-empty paragraph.
♻️ Alternative assertion
const $anchor = ed.state.doc.resolve(ed.state.selection.anchor);
- expect($anchor.parent.textContent).not.toBe('');
+ expect($anchor.parent.textContent).toBe('line two');📝 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.
| const $anchor = ed.state.doc.resolve(ed.state.selection.anchor); | |
| expect($anchor.parent.textContent).not.toBe(''); | |
| const $anchor = ed.state.doc.resolve(ed.state.selection.anchor); | |
| expect($anchor.parent.textContent).toBe('line two'); |
🤖 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 `@packages/dialtone-vue/components/rich_text_editor/rich_text_editor.test.js`
around lines 2185 - 2186, Replace the loose non-empty assertion with a specific
equality check to ensure the cursor is in the expected paragraph: use the
resolved position via ed.state.doc.resolve(ed.state.selection.anchor) (stored as
$anchor) and assert $anchor.parent.textContent === 'line two' instead of
checking not.toBe(''), so the test fails if the cursor lands in any other
non-empty paragraph.
There was a problem hiding this comment.
couple of things I've noticed here.
- If I put the cursor on an individual point within the codeblock it is fixed, however if all the text in the codeblock is highlighted it's the same problem as before
- If you insert a codeblock and then remove it, the cursor now goes to the start of the (prior) codeblock instead of the end. Not a huge problem but I think it feels more natural if it goes to the end (without going to the next line of course)
|
Close? Revisit? |
|
briandial did you see my comments? |
3 participants
Obligatory GIF (super important!)
🛠️ Type Of Change
📖 Jira Ticket
https://dialpad.atlassian.net/browse/DP-188940
📖 Description
After
toggleCodeBlockconverts a multi-line code block back to paragraphs, the cursor jumped to the start of the next line instead of staying on the current line.💡 Context
ProseMirror's
replaceWithuses right-biased step mapping (assoc=1) by default. When a multi-line code block is split into paragraphs, each\nbecomes a paragraph boundary, and right-bias places the cursor at the start of the next paragraph.Fix: track
prevAnchorandprevInCodeBlockvia theselectionUpdateevent. On any doc-changingtransactionthat exits a code block, remap the anchor with left-bias (assoc=-1) and dispatchsetTextSelectioninsetTimeout(0), after ProseMirror's own step mapping has run.The
setTimeout(0)is necessary because setting selection inside a Tiptap chain command is overridden by the EditorView's DOM reconciliation after dispatch. TheisDestroyedguard prevents the callback from running if the component unmounts in the interim.📝 Checklist
📷 Screenshots / GIFs
Verified locally via Playwright against Storybook. No visual changes — behaviour fix only.