fix: simplify dom render + use minimal css for iframe #670
Conversation
WalkthroughThe rendering logic for rrweb snapshot HTML within an iframe was simplified. The previous approach of injecting extensive CSS, including font-face declarations and external stylesheets, was replaced with a minimal CSS block. The CSS injection method was also streamlined, reducing overall complexity while maintaining core styling and behavior controls. Changes
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/recorder/DOMBrowserRenderer.tsx(1 hunks)
🔇 Additional comments (1)
src/components/recorder/DOMBrowserRenderer.tsx (1)
792-810: LGTM! Excellent simplification of CSS injection.The minimal CSS approach is a significant improvement that maintains core functionality while reducing complexity. The conditional pointer-events for links based on capture mode is particularly well-implemented.
| if (rebuiltHTML.includes("<head>")) { | ||
| rebuiltHTML = rebuiltHTML.replace( | ||
| headTagRegex, | ||
| `<head>${cssInjection}` | ||
| "<head>", | ||
| `<head><base href="${snapshotData.baseUrl}">${minimalCSS}` | ||
| ); | ||
| } else { | ||
| } else if (rebuiltHTML.includes("<html>")) { | ||
| rebuiltHTML = rebuiltHTML.replace( | ||
| /<html[^>]*>/i, | ||
| `<html><head>${cssInjection}</head>` | ||
| "<html>", | ||
| `<html><head><base href="${snapshotData.baseUrl}">${minimalCSS}</head>` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add fallback case for malformed HTML.
The simplified injection logic is much cleaner, but there's a potential edge case where HTML might not contain either <head> or <html> tags, leaving no CSS or base href injected.
Consider adding a fallback case to ensure CSS and base href are always injected:
if (rebuiltHTML.includes("<head>")) {
rebuiltHTML = rebuiltHTML.replace(
"<head>",
`<head><base href="${snapshotData.baseUrl}">${minimalCSS}`
);
} else if (rebuiltHTML.includes("<html>")) {
rebuiltHTML = rebuiltHTML.replace(
"<html>",
`<html><head><base href="${snapshotData.baseUrl}">${minimalCSS}</head>`
);
+} else {
+ // Fallback: prepend to the entire HTML if no head/html tags found
+ rebuiltHTML = `<head><base href="${snapshotData.baseUrl}">${minimalCSS}</head>${rebuiltHTML}`;
}📝 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.
| if (rebuiltHTML.includes("<head>")) { | |
| rebuiltHTML = rebuiltHTML.replace( | |
| headTagRegex, | |
| `<head>${cssInjection}` | |
| "<head>", | |
| `<head><base href="${snapshotData.baseUrl}">${minimalCSS}` | |
| ); | |
| } else { | |
| } else if (rebuiltHTML.includes("<html>")) { | |
| rebuiltHTML = rebuiltHTML.replace( | |
| /<html[^>]*>/i, | |
| `<html><head>${cssInjection}</head>` | |
| "<html>", | |
| `<html><head><base href="${snapshotData.baseUrl}">${minimalCSS}</head>` | |
| ); | |
| } | |
| if (rebuiltHTML.includes("<head>")) { | |
| rebuiltHTML = rebuiltHTML.replace( | |
| "<head>", | |
| `<head><base href="${snapshotData.baseUrl}">${minimalCSS}` | |
| ); | |
| } else if (rebuiltHTML.includes("<html>")) { | |
| rebuiltHTML = rebuiltHTML.replace( | |
| "<html>", | |
| `<html><head><base href="${snapshotData.baseUrl}">${minimalCSS}</head>` | |
| ); | |
| } else { | |
| // Fallback: prepend to the entire HTML if no head/html tags found | |
| rebuiltHTML = `<head><base href="${snapshotData.baseUrl}">${minimalCSS}</head>${rebuiltHTML}`; | |
| } |
🤖 Prompt for AI Agents
In src/components/recorder/DOMBrowserRenderer.tsx around lines 812 to 822, the
current logic injects CSS and base href only if the HTML contains <head> or
<html> tags, missing cases where these tags are absent. Add a fallback else case
to prepend a minimal HTML structure including <head> with the base href and CSS
to the rebuiltHTML string, ensuring the injection always occurs even for
malformed HTML.
amhsirak
added
Type: Bug
ref: #656
Summary by CodeRabbit