fix: chrome web data network error display#1115
Open
RohitR311 wants to merge 1 commit into
Open
Conversation
RohitR311
added
Type: Bug
WalkthroughChrome error page navigations (e.g., ChangesChrome Error Page Detection and Overlay
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 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 `@server/src/browser-management/inputHandlers.ts`:
- Around line 315-320: The code is emitting a browser-page-error in the
navigation completion handler when a Chrome error page is detected, but
RemoteBrowser.setupPageEventListeners already emits the same error for this
scenario, causing duplicate emissions. Remove the
activeBrowser.emitBrowserPageError() call from this block (keeping only the
logger.warn line if desired) since the error handling is already managed
elsewhere in the event listener setup.
In `@src/components/recorder/DOMBrowserRenderer.tsx`:
- Around line 948-953: The ChromeWebErrorOverlay component receives an onDismiss
callback prop from its parent but never invokes it, leaving the error overlay
undismissible and users stuck behind the blocking layer. Add a control (such as
a close button) within the ChromeWebErrorOverlay component that calls the
onDismiss callback when clicked to allow users to dismiss the error overlay.
- Around line 914-918: The DOM cleanup in the replayer initialization is
targeting the wrong element selector. The replayer is mounted in the
`#mirror-container` element, but the cleanup code is querying for `#replayer-root`
instead. Update the querySelector call in the cleanup block to target
`#mirror-container` instead of `#replayer-root` so that stale rrweb wrappers are
properly removed and don't accumulate on re-initialization.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1967fbf3-fead-4c8e-876f-38f300644726
📒 Files selected for processing (3)
server/src/browser-management/classes/RemoteBrowser.tsserver/src/browser-management/inputHandlers.tssrc/components/recorder/DOMBrowserRenderer.tsx
Comment on lines
+315
to
+320
| const finalUrl = page.url(); | ||
| if (finalUrl.startsWith('chrome-error://') || finalUrl.startsWith('chrome://chromewebdata')) { | ||
| logger.warn(`Navigation to ${url} silently landed on chrome error page: ${finalUrl}`); | ||
| activeBrowser.emitBrowserPageError(url, 'The page failed to load due to a network error. Please try again later.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid double-emitting browser-page-error for one failed navigation.
RemoteBrowser.setupPageEventListeners already emits this error on main-frame navigation to Chrome error pages. Emitting again here can fire duplicate overlays and conflicting urlChanged payloads.
Suggested fix
- const finalUrl = page.url();
- if (finalUrl.startsWith('chrome-error://') || finalUrl.startsWith('chrome://chromewebdata')) {
- logger.warn(`Navigation to ${url} silently landed on chrome error page: ${finalUrl}`);
- activeBrowser.emitBrowserPageError(url, 'The page failed to load due to a network error. Please try again later.');
- return;
- }
+ const finalUrl = page.url();
+ if (finalUrl.startsWith('chrome-error://') || finalUrl.startsWith('chrome://chromewebdata')) {
+ logger.warn(`Navigation to ${url} silently landed on chrome error page: ${finalUrl}`);
+ // Error emission is already handled by RemoteBrowser.setupPageEventListeners(framenavigated).
+ return;
+ }🤖 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 `@server/src/browser-management/inputHandlers.ts` around lines 315 - 320, The
code is emitting a browser-page-error in the navigation completion handler when
a Chrome error page is detected, but RemoteBrowser.setupPageEventListeners
already emits the same error for this scenario, causing duplicate emissions.
Remove the activeBrowser.emitBrowserPageError() call from this block (keeping
only the logger.warn line if desired) since the error handling is already
managed elsewhere in the event listener setup.
Comment on lines
+914
to
+918
| const container = containerRef.current; | ||
| if (container) { | ||
| const replayerRoot = container.querySelector('#replayer-root') as HTMLElement; | ||
| if (replayerRoot) replayerRoot.innerHTML = ''; | ||
| } |
There was a problem hiding this comment.
Replayer DOM cleanup targets the wrong node.
Replayer is mounted in #mirror-container, but cleanup looks for #replayer-root, so stale rrweb wrappers can remain and stack on re-init.
Suggested fix
const container = containerRef.current;
if (container) {
- const replayerRoot = container.querySelector('`#replayer-root`') as HTMLElement;
- if (replayerRoot) replayerRoot.innerHTML = '';
+ container.querySelectorAll('.replayer-wrapper').forEach((node) => node.remove());
}🤖 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 `@src/components/recorder/DOMBrowserRenderer.tsx` around lines 914 - 918, The
DOM cleanup in the replayer initialization is targeting the wrong element
selector. The replayer is mounted in the `#mirror-container` element, but the
cleanup code is querying for `#replayer-root` instead. Update the querySelector
call in the cleanup block to target `#mirror-container` instead of `#replayer-root`
so that stale rrweb wrappers are properly removed and don't accumulate on
re-initialization.
Comment on lines
+948
to
+953
| {browserPageError && ( | ||
| <ChromeWebErrorOverlay | ||
| message={browserPageError.message} | ||
| onDismiss={() => setBrowserPageError(null)} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Overlay is not actually dismissible (onDismiss is never used).
The parent passes onDismiss, but the overlay has no control that invokes it, so users can get stuck behind the blocking layer unless a new rrweb snapshot arrives.
Suggested fix
<div style={{ display: "flex", gap: "10px", marginTop: "8px" }}>
+ <button
+ onClick={onDismiss}
+ style={{
+ padding: "8px 20px",
+ background: "`#ffffff`",
+ color: "`#333`",
+ border: "1px solid `#d0d0d0`",
+ borderRadius: "6px",
+ fontSize: "14px",
+ fontWeight: "500",
+ cursor: "pointer",
+ }}
+ >
+ Dismiss
+ </button>
<button
onClick={() => {
window.dispatchEvent(new CustomEvent('maxun:trigger-discard'));
}}Also applies to: 958-1035
🤖 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 `@src/components/recorder/DOMBrowserRenderer.tsx` around lines 948 - 953, The
ChromeWebErrorOverlay component receives an onDismiss callback prop from its
parent but never invokes it, leaving the error overlay undismissible and users
stuck behind the blocking layer. Add a control (such as a close button) within
the ChromeWebErrorOverlay component that calls the onDismiss callback when
clicked to allow users to dismiss the error overlay.
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
1 participant
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?
Adds a simple error message in case of network or proxy errors (classic chrome web data error).
Summary by CodeRabbit