feat: faster remote browser loading #860
Conversation
WalkthroughReplaces canvas/screencast-based browser rendering with a DOM renderer and loading indicator, gates DOM snapshot emission behind an initial-load quiet period with progressive loading events, changes initial tabs from Changes
Sequence Diagram(s)sequenceDiagram
participant RecordingPage
participant GlobalInfoStore
participant BrowserWindow
participant DOMBrowserRenderer
participant RemoteBrowser
participant DOMLoadingIndicator
RecordingPage->>GlobalInfoStore: setIsDOMMode(true)
RecordingPage->>BrowserWindow: mount (DOM mode enabled)
BrowserWindow->>RemoteBrowser: request/currentSnapshot?
alt currentSnapshot available
BrowserWindow->>DOMBrowserRenderer: render(snapshot)
DOMBrowserRenderer->>BrowserWindow: rendered DOM + highlights
else initial load / no snapshot
RemoteBrowser->>BrowserWindow: domLoadingProgress events (0→100%)
BrowserWindow->>DOMLoadingIndicator: show loading progress
end
Note over RemoteBrowser,BrowserWindow: RemoteBrowser emits progressive domLoadingProgress during initial quiet period and gates DOM snapshots until it completes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
RohitR311
added
Type: Enhancement
![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 (3)
src/components/browser/BrowserContent.tsx(2 hunks)src/components/browser/BrowserWindow.tsx(4 hunks)src/pages/RecordingPage.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/RecordingPage.tsx (2)
src/context/globalInfo.tsx (1)
useGlobalInfoStore(186-186)src/components/recorder/RightSidePanel.tsx (1)
RightSidePanel(38-790)
src/components/browser/BrowserWindow.tsx (1)
src/components/recorder/DOMBrowserRenderer.tsx (1)
DOMBrowserRenderer(147-1060)
| const { socket } = useSocketStore(); | ||
|
|
||
| const [tabs, setTabs] = useState<string[]>(["current"]); | ||
| const [tabs, setTabs] = useState<string[]>(["Loading..."]); |
There was a problem hiding this comment.
Prevent phantom “Loading...” tab from breaking tab indices.
By keeping the placeholder when getCurrentTabs() returns an empty list you end up appending the first real tab after "Loading...". UI indices (1, 2, …) then no longer match the backend’s tab indices (0, 1, …), so actions like changeTab(1) or closeTab(1) target the wrong tab or fail outright. Please allow the empty array to overwrite the placeholder (or otherwise drop the placeholder before appending) so the frontend stays in sync with the backend.
Apply this diff to restore the previous behavior:
- if (response && response.length > 0) {
- setTabs(response);
- }
+ if (Array.isArray(response)) {
+ setTabs(response);
+ }Also applies to: 128-130
🤖 Prompt for AI Agents
In src/components/browser/BrowserContent.tsx around lines 16 and 128-130, the
initial state uses a "Loading..." placeholder which is kept when
getCurrentTabs() returns an empty array, causing frontend tab indices to shift;
remove the placeholder by initializing tabs to an empty array ([]) and/or change
the logic that applies backend results so that when getCurrentTabs() returns []
it overwrites any placeholder instead of appending to it (i.e., replace the tabs
array wholesale with the backend array or drop the placeholder before
concatenation) so frontend indices stay aligned with backend indices.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/browser-management/classes/RemoteBrowser.ts(6 hunks)
🔇 Additional comments (5)
server/src/browser-management/classes/RemoteBrowser.ts (5)
204-208: LGTM - clear state management for initial load tracking.The new fields appropriately track the initial load progress flow with clear naming and correct types.
440-444: Appropriate guard to prevent duplicate snapshots during initial load.The condition correctly suppresses DOM snapshots until the initial load sequence completes, which aligns with the new progress indicator flow.
450-452: Verify intended behavior: initial load progress shows only once per browser instance.The
hasShownInitialLoaderflag is set totrueon the first document request but never reset. This means the initial load progress indicator will only display during the first navigation after browser startup, not for subsequent navigations within the same session.If this is intentional (progress indicator only for browser spin-up, not subsequent navigations), consider adding a comment explaining this behavior. If subsequent navigations should also show progress, the flag needs to be reset appropriately.
499-506: LGTM - clean progress emission helper.The method appropriately encapsulates progress event emission with all necessary context (rounded progress, pending requests, user ID, timestamp).
557-696: Well-structured initialization progress milestones.The progress emissions at key initialization steps (0% → 20% → 40% → 60%) provide clear feedback during browser startup. These milestones transition smoothly into the 60-95% range during initial page load, culminating in 100% when complete.
| ) { | ||
| const isDocumentRequest = response.request().resourceType() === "document"; | ||
|
|
||
| if (!this.hasShownInitialLoader && isDocumentRequest && !url.includes("about:blank")) { |
There was a problem hiding this comment.
Fragile URL check could match unintended substrings.
Using url.includes("about:blank") could incorrectly match legitimate URLs that contain "about:blank" as a substring (e.g., https://example.com/page?redirect=about:blank).
Consider using a more precise check:
- if (!this.hasShownInitialLoader && isDocumentRequest && !url.includes("about:blank")) {
+ if (!this.hasShownInitialLoader && isDocumentRequest && url !== "about:blank") {📝 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 (!this.hasShownInitialLoader && isDocumentRequest && !url.includes("about:blank")) { | |
| if (!this.hasShownInitialLoader && isDocumentRequest && url !== "about:blank") { |
| this.progressInterval = setInterval(() => { | ||
| const elapsed = Date.now() - this.networkWaitStartTime; | ||
| const navigationProgress = Math.min((elapsed / this.INITIAL_LOAD_QUIET_PERIOD) * 40, 35); | ||
| const totalProgress = 60 + navigationProgress; | ||
| this.emitLoadingProgress(totalProgress, this.pendingNetworkRequests.length); | ||
| }, 500); |
There was a problem hiding this comment.
Memory leak: progressInterval not cleaned up in shutdown/cleanup methods.
The progressInterval is created here but only cleared within the setTimeout callback (lines 482-485). If switchOff(), stopDOM(), or other cleanup methods are called before the timeout fires, the interval will continue running indefinitely.
Add cleanup in the relevant methods:
// In switchOff() method around line 1738
if (this.progressInterval) {
clearInterval(this.progressInterval);
this.progressInterval = null;
}
// In stopDOM() method around line 1703
if (this.progressInterval) {
clearInterval(this.progressInterval);
this.progressInterval = null;
}🤖 Prompt for AI Agents
In server/src/browser-management/classes/RemoteBrowser.ts around lines 466 to
471, an interval (progressInterval) is started but only cleared inside a later
setTimeout callback, causing a potential memory leak if shutdown/cleanup methods
run first; add code in switchOff() (around line 1738) and stopDOM() (around line
1703) to check if this.progressInterval is set, call
clearInterval(this.progressInterval) and then set this.progressInterval to null
so the interval is always cleaned up during shutdown.
| <Route | ||
| path="/recording-setup" | ||
| element={<div />} | ||
| /> |
There was a problem hiding this comment.
Add loading state and fallback handling for better UX.
The empty div provides no user feedback during the redirect flow and contradicts this PR's objective of introducing inline loading progress indicators. Additionally, if the redirect conditions at lines 58-68 fail (e.g., missing or mismatched session parameters), users are left staring at a blank page with no guidance.
Consider showing a loading indicator or message while the redirect logic executes, and add a fallback for cases where the redirect doesn't occur.
Example improvement:
- <Route
- path="/recording-setup"
- element={<div />}
- />
+ <Route
+ path="/recording-setup"
+ element={
+ <div style={{ display: 'flex', justifyContent: 'center', alignItems: 'center', height: '100vh' }}>
+ <div>Setting up recording session...</div>
+ </div>
+ }
+ />📝 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.
| <Route | |
| path="/recording-setup" | |
| element={<div />} | |
| /> | |
| <Route | |
| path="/recording-setup" | |
| element={ | |
| <div style={{ display: 'flex', justifyContent: 'center', alignItems: 'center', height: '100vh' }}> | |
| <div>Setting up recording session...</div> | |
| </div> | |
| } | |
| /> |
|
@RohitR311 resolve conflicts |
|
@amhsirak Resolved |
What this PR does?
Sunsets the old spinning browser loader. Introduces inline loading progress indicator for browser spin up.
2025-11-03.13-35-09.online-video-cutter.com.mp4
Summary by CodeRabbit
New Features
UI/UX Improvements
Bug Fixes