fix: cors navigation issue #810
Merged
Conversation
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
WalkthroughThe DOMBrowserRenderer component removes in-iframe error UI and renderError state, updates event listener options to use passive: false, and adds cross-origin handling by recreating the iframe on access failure during rrweb snapshot rendering. Loading indicator logic is simplified to depend solely on isRendered. Cleanup paths mirror the new listener options. Changes
Sequence Diagram(s)sequenceDiagram
participant U as DOMBrowserRenderer
participant IF as Iframe
participant DOC as Iframe Document
participant C as Console
rect rgba(230,240,255,0.5)
note over U: Start rrweb snapshot render
U->>IF: Create/ensure iframe
U->>IF: Attach listeners (wheel/touch*) with { passive: false }
end
alt Access iframe document
U->>IF: Get contentWindow/document
IF-->>U: Document reference
U->>DOC: Inject rrweb snapshot & styles
U-->>U: Mark isRendered = true
else Cross-origin access error
U->>C: console.error(error)
note over U,IF: Recreate iframe preserving attributes/styles
U->>IF: Recreate and mount iframe
U->>IF: Retry get document
IF-->>U: Document reference (if available)
U->>DOC: Inject rrweb snapshot & styles
U-->>U: Mark isRendered = true
end
rect rgba(240,240,240,0.5)
note over U: Unmount/Cleanup
U->>IF: Remove listeners with { passive: false }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/recorder/DOMBrowserRenderer.tsx (2)
432-441: Don’t permanently strip target on anchor clicks; restore or avoid mutationSetting
linkElement.target = ""alters page semantics and isn’t restored. Preventing default + temporary href removal is enough to suppress navigation/tab opening.Patch:
- if (linkElement.target) { - linkElement.target = ""; - } - - const originalHref = linkElement.href; + const originalHref = linkElement.href; + const originalTarget = linkElement.getAttribute('target'); linkElement.removeAttribute("href"); setTimeout(() => { linkElement.setAttribute("href", originalHref); + if (originalTarget !== null) { + linkElement.setAttribute('target', originalTarget); + } }, 100);
876-880: Invalid CSS: quoted cursor value is ignored
cursor: "pointer"is invalid; remove the quotes so the rule applies.- * { - cursor: "pointer" !important; - } + * { + cursor: pointer !important; + }
🧹 Nitpick comments (2)
src/components/recorder/DOMBrowserRenderer.tsx (2)
344-347: Deduplicate listener options to avoid drift between add/removeYou compute the options in three places. Factor it to a helper to ensure symmetry and readability.
- const options: boolean | AddEventListenerOptions = ['wheel', 'touchstart', 'touchmove'].includes(event) - ? { passive: false } - : false; + const options = listenerOptions(event);Add once (near the component top-level):
function listenerOptions(event: string): boolean | AddEventListenerOptions { return ['wheel', 'touchstart', 'touchmove'].includes(event) ? { passive: false } : false; // capture=false }Also applies to: 761-765, 965-972
1009-1045: Add minimal a11y to loading overlayExpose status to AT without behavioral change.
- {!isRendered && ( - <div + {!isRendered && ( + <div + role="status" + aria-live="polite" + aria-busy="true" style={{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/recorder/DOMBrowserRenderer.tsx(6 hunks)
🔇 Additional comments (1)
src/components/recorder/DOMBrowserRenderer.tsx (1)
705-709: Good: wheel prevention guarded by capture modeEnsuring preventDefault only in capture mode (with passive: false) is correct and avoids blocking normal scrolling otherwise.
Comment on lines
+812
to
+839
| try { | ||
| iframeDoc = iframe.contentDocument!; | ||
| if (!iframeDoc) { | ||
| throw new Error("Cannot access iframe document"); | ||
| } | ||
| } catch (crossOriginError) { | ||
| console.warn("Cross-origin iframe access blocked, recreating iframe"); | ||
|
|
||
| const newIframe = document.createElement('iframe'); | ||
| newIframe.style.cssText = iframe.style.cssText; | ||
| newIframe.sandbox = iframe.sandbox.value; | ||
| newIframe.title = iframe.title; | ||
| newIframe.tabIndex = iframe.tabIndex; | ||
| newIframe.id = iframe.id; | ||
|
|
||
| iframe.parentNode?.replaceChild(newIframe, iframe); | ||
| Object.defineProperty(iframeRef, 'current', { | ||
| value: newIframe, | ||
| writable: false, | ||
| enumerable: true, | ||
| configurable: true | ||
| }); | ||
|
|
||
| iframeDoc = newIframe.contentDocument!; | ||
| if (!iframeDoc) { | ||
| throw new Error("Cannot access new iframe document"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Recreating the iframe: ref mutation and sandbox handling will crash and/or no-op
- Don’t redefine refs with Object.defineProperty; it makes
currentread‑only and React will later fail to update it (on re-render/unmount). HTMLIFrameElement.sandboxis a DOMTokenList (read‑only); assigning a string won’t work reliably. Set the attribute instead.
Fix with this minimal patch:
- const newIframe = document.createElement('iframe');
- newIframe.style.cssText = iframe.style.cssText;
- newIframe.sandbox = iframe.sandbox.value;
+ const newIframe = document.createElement('iframe');
+ newIframe.style.cssText = iframe.style.cssText;
+ newIframe.setAttribute('sandbox', iframe.getAttribute('sandbox') ?? '');
newIframe.title = iframe.title;
newIframe.tabIndex = iframe.tabIndex;
newIframe.id = iframe.id;
iframe.parentNode?.replaceChild(newIframe, iframe);
- Object.defineProperty(iframeRef, 'current', {
- value: newIframe,
- writable: false,
- enumerable: true,
- configurable: true
- });
+ iframeRef.current = newIframe;Optional: const newIframe = iframe.cloneNode(false) as HTMLIFrameElement; will also preserve attributes like class, data‑*, etc., then replace and assign iframeRef.current.
📝 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.
Suggested change
| try { | |
| iframeDoc = iframe.contentDocument!; | |
| if (!iframeDoc) { | |
| throw new Error("Cannot access iframe document"); | |
| } | |
| } catch (crossOriginError) { | |
| console.warn("Cross-origin iframe access blocked, recreating iframe"); | |
| const newIframe = document.createElement('iframe'); | |
| newIframe.style.cssText = iframe.style.cssText; | |
| newIframe.sandbox = iframe.sandbox.value; | |
| newIframe.title = iframe.title; | |
| newIframe.tabIndex = iframe.tabIndex; | |
| newIframe.id = iframe.id; | |
| iframe.parentNode?.replaceChild(newIframe, iframe); | |
| Object.defineProperty(iframeRef, 'current', { | |
| value: newIframe, | |
| writable: false, | |
| enumerable: true, | |
| configurable: true | |
| }); | |
| iframeDoc = newIframe.contentDocument!; | |
| if (!iframeDoc) { | |
| throw new Error("Cannot access new iframe document"); | |
| } | |
| } | |
| try { | |
| iframeDoc = iframe.contentDocument!; | |
| if (!iframeDoc) { | |
| throw new Error("Cannot access iframe document"); | |
| } | |
| } catch (crossOriginError) { | |
| console.warn("Cross-origin iframe access blocked, recreating iframe"); | |
| const newIframe = document.createElement('iframe'); | |
| newIframe.style.cssText = iframe.style.cssText; | |
| newIframe.setAttribute('sandbox', iframe.getAttribute('sandbox') ?? ''); | |
| newIframe.title = iframe.title; | |
| newIframe.tabIndex = iframe.tabIndex; | |
| newIframe.id = iframe.id; | |
| iframe.parentNode?.replaceChild(newIframe, iframe); | |
| iframeRef.current = newIframe; | |
| iframeDoc = newIframe.contentDocument!; | |
| if (!iframeDoc) { | |
| throw new Error("Cannot access new iframe document"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/components/recorder/DOMBrowserRenderer.tsx around lines 812 to 839, the
current approach recreates the iframe by using Object.defineProperty to mutate
the ref (making current read‑only) and assigns iframe.sandbox as a string
(DOMTokenList is not a string), which will break React ref updates and sandbox
handling; fix by creating the new iframe (or cloneNode(false) to preserve
attributes), copy needed attributes/styles (style.cssText, title, tabIndex, id,
classes, data-*, etc.), set sandbox using setAttribute('sandbox',
original.getAttribute('sandbox') || '') or manipulate newIframe.sandbox.tokens
properly, replace the old iframe in the DOM, and update the ref by assigning
iframeRef.current = newIframe (not via Object.defineProperty) so React can
update/unmount correctly.
RohitR311
added
Type: Bug
amhsirak
approved these changes
Sep 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does?
Fixes #808
Fixes #807
Fixes #806
Summary by CodeRabbit