🐛(frontend) sanitize pasted and dropped content in document title#2210
Open
🐛(frontend) sanitize pasted and dropped content in document title#2210
Conversation
Ovgodd
force-pushed
the
fix/pasting-in-title
branch
from
c2c0cb5 to
fbd7166
Compare
|
Size Change: 0 B Total Size: 4.27 MB 📦 View Changed
|
AntoLC
reviewed
Apr 14, 2026
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdates CHANGELOG formatting and adds a fix: the document title contentEditable now intercepts paste and drop at the capture phase, prevents default behavior, extracts only plain text from clipboard/drag data, and inserts a plain Text node at the current selection (adjusting focus/range as needed). Adds onPasteCapture and onDropCapture handlers plus an insertPlainText helper. Adds a Playwright e2e test that verifies rich/HTML clipboard payloads are inserted as plain text. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx`:
- Around line 162-168: After inserting the text node into the range (using
selection.getRangeAt(0), range.insertNode(document.createTextNode(plainText))),
explicitly move the caret to after the inserted node: obtain the node you
inserted (the new Text node), call range.setStartAfter(thatNode) and then
collapse(true), then update selection.removeAllRanges() and
selection.addRange(range) so the caret is positioned after the pasted text;
reference the existing variables selection, range, and plainText in DocTitle.tsx
when making this change.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: 55242805-6bdf-4fc8-896a-fb1d9bd0ed6c
📒 Files selected for processing (2)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx
Collaborator
Author
Yes of course |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts (1)
147-173: Consider also covering the drop path.The PR introduces both
onPasteCaptureandonDropCapturesanitization, but only paste is exercised here. A sibling case dispatching adropevent withDataTransfercarryingtext/plain+text/htmlwould prevent future regressions in the drop handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts` around lines 147 - 173, Add a sibling test to exercise the drop path similar to the paste test: create a test (e.g., "it drops plain text in the title without keeping formatting") that reuses createDoc and the same Document title textbox selector ('[aria-label="Document title"]'), programmatically dispatches a 'drop' event on that element with a DataTransfer containing both 'text/plain' and 'text/html' entries, blurs the title and asserts the textbox text equals the plain text, and reference the onDropCapture sanitization alongside onPasteCapture to ensure drop handling is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts`:
- Around line 147-173: The test "it pastes plain text in the title without
keeping formatting" currently uses toHaveText which only checks textContent and
would pass even if HTML formatting was preserved; update the assertion to verify
the rendered markup instead—locate the test and the docTitle element (query
selector '[aria-label="Document title"]' / variable docTitle) and assert that
the element's innerHTML does not contain formatting tags (e.g., no <b> or <em>
descendants) or explicitly compare innerHTML to the expected plain text
representation so the paste handler is validated to strip HTML.
---
Nitpick comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts`:
- Around line 147-173: Add a sibling test to exercise the drop path similar to
the paste test: create a test (e.g., "it drops plain text in the title without
keeping formatting") that reuses createDoc and the same Document title textbox
selector ('[aria-label="Document title"]'), programmatically dispatches a 'drop'
event on that element with a DataTransfer containing both 'text/plain' and
'text/html' entries, blurs the title and asserts the textbox text equals the
plain text, and reference the onDropCapture sanitization alongside
onPasteCapture to ensure drop handling is covered.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: c1debf8e-e73b-4e26-9b6d-84890676c347
📒 Files selected for processing (1)
src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts
AntoLC
approved these changes
Apr 21, 2026
Collaborator
AntoLC
left a comment
AntoLC
left a comment
There was a problem hiding this comment.
You should fix this comments before merging, I think it is right what the rabbit says, the test has no purpose as it is:
#2210 (comment)
Collaborator
Author
Oh Yes I did nt see it, thanks |
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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts`:
- Around line 147-175: Add a new E2E test mirroring the paste test but
dispatching a synthetic drop event to exercise the onDropCapture/handleDrop path
in DocTitle.tsx: when focusing the title textbox (same selector used in the
existing test), create a DataTransfer with both 'text/plain' and 'text/html',
dispatch a 'drop' event on the element (ensure preventDefault path is triggered
by using a real Clipboard/DataTransfer object), blur and then assert the title
text equals the plain text and that no formatting tags (b, em, strong, i) exist;
this ensures handleDrop (and its use of preventDefault() and MIME selection) is
covered alongside onPasteCapture.
🪄 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 UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de03a214-3c52-4ba2-9d12-afba4d0e05ba
📒 Files selected for processing (1)
src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts
Ovgodd
force-pushed
the
fix/pasting-in-title
branch
from
4b87fcf to
3a01630
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts (1)
147-175:⚠️ Potential issue | 🟡 MinorAdd coverage for the drop path too.
This only exercises
onPasteCapture;onDropCapturecan still regress independently while the PR objective covers both pasted and dropped content.You can verify the current gap with:
#!/bin/bash # Description: Check whether the document-title e2e spec dispatches a drop event. rg -n -C3 "new DragEvent\\(['\"]drop['\"]|dispatchEvent\\([^\\n]*drop|dataTransfer.*drop" src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts` around lines 147 - 175, The test currently only triggers onPasteCapture; add a parallel step that dispatches a drop event to exercise onDropCapture: after the paste block (or instead of it) locate the same element via document.querySelector('[aria-label="Document title"]'), create a DataTransfer, set 'text/plain' and 'text/html' payloads, then dispatch a DragEvent('drop', { dataTransfer: dt, bubbles: true }) on that element and assert the same expectations (docTitle text equals 'Pasted plain text' and no formatting tags). Ensure you reference the same locator (docTitle / '[aria-label="Document title"]') so the drop path covering onDropCapture is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx`:
- Around line 156-179: The insertPlainText function currently manipulates the
global selection and must be constrained to the title element: change
insertPlainText to accept a target HTMLElement (e.g., insertPlainText(target:
HTMLElement, plainText: string)), then in that function get the selection,
ensure selection.rangeCount>0 and that the range's commonAncestorContainer (or
both startContainer/endContainer) is contained within the target (use
target.contains(range.commonAncestorContainer) or similar) before calling
range.deleteContents()/insertNode()/collapse(); if the range is outside the
target, bail out. Update handlePaste and handleDrop to pass event.currentTarget
(cast to HTMLElement/HTMLSpanElement) as the first arg when calling
insertPlainText so pastes/drops only modify the title element.
---
Duplicate comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts`:
- Around line 147-175: The test currently only triggers onPasteCapture; add a
parallel step that dispatches a drop event to exercise onDropCapture: after the
paste block (or instead of it) locate the same element via
document.querySelector('[aria-label="Document title"]'), create a DataTransfer,
set 'text/plain' and 'text/html' payloads, then dispatch a DragEvent('drop', {
dataTransfer: dt, bubbles: true }) on that element and assert the same
expectations (docTitle text equals 'Pasted plain text' and no formatting tags).
Ensure you reference the same locator (docTitle / '[aria-label="Document
title"]') so the drop path covering onDropCapture is exercised.
🪄 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 UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8aa7ae7f-1ad5-48c0-ba89-b75dabc21f4f
📒 Files selected for processing (3)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.tssrc/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx
Ovgodd
force-pushed
the
fix/pasting-in-title
branch
2 times, most recently
from
8b11522 to
0cce307
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts (1)
147-175:⚠️ Potential issue | 🟡 MinorAdd drop coverage alongside the paste case.
This spec still only exercises
paste, sohandleDropcan regress unnoticed. Please mirror the same plain-text assertion with a synthetic drop event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts` around lines 147 - 175, The test "it pastes plain text in the title without keeping formatting" only exercises paste and misses coverage for handleDrop, so add a parallel synthetic drop case that targets the same element (docTitle / [aria-label="Document title"]) and dispatches a 'drop' event with a DataTransfer containing text/plain = 'Pasted plain text' and text/html = '<b><em>Pasted plain text</em></b>'; then blur the field and assert the visible text equals 'Pasted plain text' and that no formatting tags exist (locator('b, em, strong, i') has count 0), mirroring the existing paste flow to ensure handleDrop preserves plain-text behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx`:
- Around line 156-172: The current insertPlainText function bails out when
window.getSelection() is null or selection.rangeCount === 0, causing unfocused
paste/drop to be dropped; instead, when selection is missing or empty, create a
new collapsed Range inside the provided target (e.g., use
document.createRange(), range.selectNodeContents(target) then
range.collapse(false)), add it to the selection, and continue with
deleteContents/insertNode logic; update the logic in insertPlainText to
initialize and attach that collapsed range to the selection whenever selection
is empty so the paste/drop inserts into target rather than being ignored.
---
Duplicate comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts`:
- Around line 147-175: The test "it pastes plain text in the title without
keeping formatting" only exercises paste and misses coverage for handleDrop, so
add a parallel synthetic drop case that targets the same element (docTitle /
[aria-label="Document title"]) and dispatches a 'drop' event with a DataTransfer
containing text/plain = 'Pasted plain text' and text/html = '<b><em>Pasted plain
text</em></b>'; then blur the field and assert the visible text equals 'Pasted
plain text' and that no formatting tags exist (locator('b, em, strong, i') has
count 0), mirroring the existing paste flow to ensure handleDrop preserves
plain-text 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 UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0d8b9a7-e8dc-468b-a533-5ce8ace3a674
📒 Files selected for processing (3)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.tssrc/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx
Comment on lines
+156
to
+172
| const insertPlainText = (plainText: string, target: HTMLElement) => { | ||
| const selection = window.getSelection(); | ||
| if (!selection || selection.rangeCount === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const range = selection.getRangeAt(0); | ||
| if (!target.contains(range.commonAncestorContainer)) { | ||
| target.focus(); | ||
| range.selectNodeContents(target); | ||
| range.collapse(false); | ||
| selection.removeAllRanges(); | ||
| selection.addRange(range); | ||
| } | ||
| range.deleteContents(); | ||
| range.insertNode(document.createTextNode(plainText)); | ||
| }; |
There was a problem hiding this comment.
Handle the empty-selection path.
On Line 158, this returns after preventDefault(), so an unfocused paste/drop is silently discarded. If this path is reachable, initialize a collapsed range on target instead of bailing out.
🐛 Proposed fix
- if (!selection || selection.rangeCount === 0) {
- return;
- }
-
- const range = selection.getRangeAt(0);
+ if (!selection) {
+ return;
+ }
+
+ const range =
+ selection.rangeCount > 0 ? selection.getRangeAt(0) : document.createRange();
+ if (selection.rangeCount === 0) {
+ target.focus();
+ range.selectNodeContents(target);
+ range.collapse(false);
+ selection.removeAllRanges();
+ selection.addRange(range);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx`
around lines 156 - 172, The current insertPlainText function bails out when
window.getSelection() is null or selection.rangeCount === 0, causing unfocused
paste/drop to be dropped; instead, when selection is missing or empty, create a
new collapsed Range inside the provided target (e.g., use
document.createRange(), range.selectNodeContents(target) then
range.collapse(false)), add it to the selection, and continue with
deleteContents/insertNode logic; update the logic in insertPlainText to
initialize and attach that collapsed range to the selection whenever selection
is empty so the paste/drop inserts into target rather than being ignored.
0cce307 to
622151c
Compare
Prevent rich-text formatting when pasting or dropping into document title.
Ovgodd
force-pushed
the
fix/pasting-in-title
branch
from
622151c to
0f3db93
Compare
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.
Purpose
Fix document title input behavior so pasted or dragged content is treated as plain text only.
titlepaste.mp4
Proposal
DocTitleand inserttext/plaininstead of rich content.DocTitleand inserttext/plainonly.