-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: handle shift-enter in text #2809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: handle shift-enter in text #2809
Conversation
@rohan-chidurala is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughUpdated extractTextContent to normalize Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/preload/script/api/elements/text.ts (1)
91-95
: Avoid innerHTML to prevent XSS and attribute loss; build DOM safelyAssigning
innerHTML
with rawcontent
enables markup injection and drops any instrumentation attributes on existing nodes, which can contribute to the “glitch” you’re seeing during refreshes. Build a fragment from text nodes and<br>
elements instead.Apply this diff:
function updateTextContent(el: HTMLElement, content: string): void { - // Convert newlines to <br> tags in the DOM - const htmlContent = content.replace(/\n/g, '<br>'); - el.innerHTML = htmlContent; + // Safely rebuild children from text + <br> nodes (no HTML parsing) + const frag = document.createDocumentFragment(); + const parts = content.split('\n'); + for (let i = 0; i < parts.length; i++) { + if (i > 0) frag.appendChild(document.createElement('br')); + frag.appendChild(document.createTextNode(parts[i])); + } + el.replaceChildren(frag); }
🧹 Nitpick comments (3)
apps/web/preload/script/api/elements/text.ts (3)
78-85
: Guard observers during live edits to avoid refresh glitchesEnsure any MutationObserver/bridge ignores subtrees marked with
data-onlook-editing-text="true"
so the editing frame isn’t re-instrumented mid-typing.If not already, gate observer callbacks with:
- skip if
node.closest('[data-onlook-editing-text="true"]')
is truthy.- batch updates until the attribute is removed in
cleanUpElementAfterEditing
.
29-33
: Minor: broaden “editable text” detection only if neededCurrent check allows only text nodes and
<br>
elements. If designers paste formatted text (e.g.,<span>
wrappers), editing will bail. If intentional, ignore; otherwise consider allowing inline wrappers while flattening them inextractTextContent
.
106-108
: Dead parameter
isChildTextEditable(oid)
ignores its argument and always returnstrue
. Either remove the param or implement the lookup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/preload/script/api/elements/text.ts
(1 hunks)
🔇 Additional comments (2)
apps/web/preload/script/api/elements/text.ts (2)
99-99
: Regex fix for
with attributes — LGTMBroader match
/<br[^>]*\/?>/gi
correctly handles<br>
variants with attributes and self-closing forms. Nice, targeted fix.
97-104
: Add unit tests for round-trip and edge casesCover:
<br>
,<br/>
,<br />
,<br data-oid="">
→ newline- Multiple consecutive breaks
- HTML entity decode (
,&
)- Round-trip: text with newlines → DOM → extracted text equals original
I can scaffold Jest tests for
extractTextContent
andupdateTextContent
if helpful.
will take a look thanks @rohan-chidurala ! |
Description
Update the preload script so it can detect
elements with attributes (e.g.,
).
Right now, it’s still buggy when adding multiple line breaks. From what I can tell, the issue happens during sandbox updates/refreshes. The data and file changes look correct, but when the sandbox refreshes, only the current frame you’re working on glitches—the other frames are fine. I couldn’t fully figure out why, so someone else might need to dig deeper.
Related Issues
#2504
Testing
Screenshots (if applicable)
Additional Notes
Important
Fixes handling of
<br>
elements with attributes inextractTextContent()
intext.ts
.extractTextContent()
intext.ts
to handle<br>
elements with attributes by modifying the regex to/\<br[^>]*\/?>/gi
.<br>
with attributes were not converted to newlines.This description was created by
for 1bb61fb. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
tags with attributes (e.g.,
), ensuring consistent newline formatting in extracted text across the app (previews, copies, exports).
tag support without changing public APIs or behavior elsewhere.