fix(rich-text-editor): DP-190125 fix RTE paste trailing blank line and code block cursor jump - #1324
fix(rich-text-editor): DP-190125 fix RTE paste trailing blank line and code block cursor jump#1324briandial wants to merge 5 commits into
Conversation
…ursor jump Two cursor/paste fixes for DtRichTextEditor: 1. Add a transformPastedHTML replace to strip trailing <br> before </p>, so pasting content doesn't insert a spurious blank line at the end. 2. After converting a code block back to paragraphs (toggleCodeBlock), remap the cursor anchor with left bias (assoc=-1) instead of the default right bias, so the cursor stays on the current line instead of jumping to the start of the next one.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Strips trailing Overall Judgement: WalkthroughTwo bug fixes in the rich text editor: paste normalization removes trailing ChangesRich Text Editor Fixes
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry Comment |
|
Please add either the |
…ursor jump Two fixes for DtRichTextEditor: 1. Paste trailing blank line: adds a third replace to transformPastedHTML that strips <br> immediately before </p>, so pasting content with a trailing break does not produce a spurious blank line at the end. 2. Code block cursor jump: when toggleCodeBlock converts a multi-line code block back to paragraphs, ProseMirror's default right-biased step mapping moves the cursor to the start of the next paragraph. The fix tracks the cursor position and codeBlock state via editor event listeners, then re-applies the position mapping with left bias (assoc=-1) via a post-transaction setTextSelection call to keep the cursor on the current line.
briandial
changed the title
briandial
added
the
no-visual-test
![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: 942dea50b7
ℹ️ 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".
| .replace(/(<\/\w+>)((<br \/>)+)/g, '$2$1') | ||
| // Strip trailing <br> just before </p> so pasted content doesn't | ||
| // produce an extra blank line at the end of the message. | ||
| .replace(/(<br[^>]*>)\s*<\/p>/gi, '</p>'); |
There was a problem hiding this comment.
When rich clipboard HTML contains an intentional hard break at the end of a non-final paragraph, for example <p>first<br></p><p>second</p>, this global replacement removes that <br> even though it is not the trailing blank line at the end of the paste. Since transformPastedHTML runs on all rich HTML pastes, this loses user-authored line breaks inside pasted content; the cleanup should be limited to the final paragraph/trailing wrapper that causes the extra blank 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 979-982: The trailing-<br> replacement currently uses
.replace(/(<br[^>]*>)\s*<\/p>/gi, '</p>') which strips <br> before every </p>;
change it to only target terminal/end-of-fragment cases by anchoring the regex
to the end of the fragment (for example use a pattern like
/(<br[^>]*>)\s*<\/p>\s*$/i or a lookahead that asserts end-of-string) so only
<br> immediately before the final </p> is removed; update the replace call in
the .replace chain in rich_text_editor.vue where the '(<br[^>]*>)\\s*<\\/p>'
pattern is used.
🪄 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: 0430683e-21a4-4f51-9af9-82f9a02a4537
📒 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)
…asted fragment Scope the <br></p> strip to end-of-string only so intentional hard breaks inside non-final paragraphs are preserved.
briandial
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 366ca3990f
ℹ️ 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 toggling a code block back to paragraphs, the command above replaces the entire code-block node in one replaceWith, so every old cursor position inside that replaced range maps to a boundary of the inserted content. Calling transaction.mapping.map(prevAnchor, -1) here therefore sends a caret from the second or later line back to the start of the converted paragraphs instead of preserving the same line/offset, making the new timeout actively move the cursor to the top for common multi-line code blocks.
Useful? React with 👍 / 👎.
| // doesn't produce an extra blank line at the end of the message. | ||
| // Anchored to end-of-string to avoid removing intentional hard breaks | ||
| // inside non-final paragraphs. | ||
| .replace(/(<br[^>]*>)\s*<\/p>(\s*)$/i, '</p>$2'); |
There was a problem hiding this comment.
This only strips the trailing break when </p> is literally at the end of the raw text/html string. Browser and Office rich clipboards commonly include context after the fragment, such as <!--EndFragment--></body></html>, so a paste whose copied fragment ends with <p>text<br></p> will miss this replacement and still produce the extra blank line the change is meant to remove. The cleanup needs to account for clipboard suffix markup/comments or operate on the parsed final paragraph rather than anchoring to end-of-string.
Useful? React with 👍 / 👎.
|
✔️ Deploy previews ready! |
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 1137-1143: The loop over linkPhoneNumbers can infinite-loop when
an entry is an empty string because indexOf("", searchFrom) returns searchFrom;
update the loop in the block that iterates linkPhoneNumbers (the for (const
number of this.linkPhoneNumbers) loop that uses node.text, searchFrom, idx,
tr.addMark and type.create()) to defensively skip empty strings (e.g., if number
=== "" or number.length === 0 continue) so indexOf is never called with an empty
needle; optionally also ensure searchFrom advances on zero-length matches, but
the primary fix is to ignore empty-number entries before the while loop.
🪄 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: c743235d-a22e-41f5-9dae-11518e67dfaf
📒 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)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 1137-1143: The loop over linkPhoneNumbers can infinite-loop when
an entry is an empty string because indexOf("", searchFrom) returns searchFrom;
update the loop in the block that iterates linkPhoneNumbers (the for (const
number of this.linkPhoneNumbers) loop that uses node.text, searchFrom, idx,
tr.addMark and type.create()) to defensively skip empty strings (e.g., if number
=== "" or number.length === 0 continue) so indexOf is never called with an empty
needle; optionally also ensure searchFrom advances on zero-length matches, but
the primary fix is to ignore empty-number entries before the while loop.
🪄 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: c743235d-a22e-41f5-9dae-11518e67dfaf
📒 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)
🛑 Comments failed to post (1)
packages/dialtone-vue/components/rich_text_editor/rich_text_editor.vue (1)
1137-1143:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty string in
linkPhoneNumberscauses infinite loop.If the array contains
"",indexOf("", searchFrom)always returnssearchFrom, never advancing.Defensive fix
for (const number of this.linkPhoneNumbers) { + if (!number) continue; let searchFrom = 0;📝 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.for (const number of this.linkPhoneNumbers) { if (!number) continue; let searchFrom = 0; let idx; while ((idx = node.text.indexOf(number, searchFrom)) !== -1) { tr.addMark(pos + idx, pos + idx + number.length, type.create()); searchFrom = idx + number.length; }🤖 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.vue` around lines 1137 - 1143, The loop over linkPhoneNumbers can infinite-loop when an entry is an empty string because indexOf("", searchFrom) returns searchFrom; update the loop in the block that iterates linkPhoneNumbers (the for (const number of this.linkPhoneNumbers) loop that uses node.text, searchFrom, idx, tr.addMark and type.create()) to defensively skip empty strings (e.g., if number === "" or number.length === 0 continue) so indexOf is never called with an empty needle; optionally also ensure searchFrom advances on zero-length matches, but the primary fix is to ignore empty-number entries before the while loop.
1 participant
Obligatory GIF (super important!)
🛠️ Type Of Change
📖 Jira Ticket
https://dialpad.atlassian.net/browse/DP-190125
📖 Description
Two targeted bug fixes for
DtRichTextEditor:Paste trailing blank line — pasting content whose HTML ends with
<br></p>produced a spurious blank line at the end of the pasted message. Added a thirdtransformPastedHTMLregex to strip the trailing<br>before</p>is parsed by ProseMirror.Code block cursor jump — after
toggleCodeBlockconverts a multi-line code block back to paragraphs, the cursor jumped to the start of the next paragraph. ProseMirror'sreplaceWithuses right-biased step mapping (assoc=1) by default, which places the cursor at the start of the next paragraph when a\nbecomes a paragraph boundary. The fix re-maps the cursor with left-bias (assoc=-1) using a post-transactionsetTextSelectiondispatch.💡 Context
Both issues were originally surfaced in firespotter and patched as workarounds there. Brad Paugh requested the fixes live here in Dialtone instead, so the workarounds in firespotter can be removed.
Fix 1 —
transformPastedHTMLincreateEditor(). Two other regexes already handle<hr>and misplaced<br />tags on paste; this adds a third:Fix 2 —
addEditorListeners(). TracksprevAnchorandprevInCodeBlockvia theselectionUpdateevent. On any doc-changingtransactionevent that exits a code block, dispatchessetTextSelection(mapping.map(prevAnchor, -1))insetTimeout(0)to correct the cursor position after ProseMirror's step mapping runs.📝 Checklist
📷 Screenshots / GIFs
Both fixes were verified locally via Playwright against Storybook. No visual changes — behavior fixes only.