Conversation
RohitR311
added
Type: Bug
WalkthroughThe PR implements centralized socket event broadcasting and introduces reference-counted socket management. Server-side changes add Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Client Component
participant Util as Socket Utility
participant Cache as Socket Cache
participant Socket as Socket.IO Client
participant Server as Server
User->>Client: Render/mount component
Client->>Util: getOrCreateBrowserSocket(browserId)
Util->>Cache: Check cache for browserId
alt Socket exists in cache
Cache->>Util: Return cached socket, incr refcount
else Socket not in cache
Util->>Socket: new Socket(apiUrl/browserId)
Socket->>Server: Connect to namespace
Server-->>Socket: Connected
Util->>Cache: Store socket, set refcount=1
end
Util-->>Client: Return socket
Client->>Socket: socket.on('workflowProgress', ...)
Socket->>Server: Listen for events
Server->>Socket: broadcastToNamespace(event)
Socket->>Client: Event received by all listeners
Client->>Client: Update UI
User->>Client: Unmount/complete
Client->>Util: releaseBrowserSocket(browserId)
Util->>Cache: Decrement refcount
alt refcount > 0
Cache->>Util: Socket still in use
else refcount = 0
Util->>Socket: socket.disconnect()
Socket->>Server: Disconnect from namespace
Util->>Cache: Remove socket and refcount
end
Util-->>Client: Released
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/browser-management/controller.ts (1)
339-358:⚠️ Potential issue | 🟠 MajorHandle clients that connect after the dummy-socket timeout.
If the timeout resolves
null, a later real socket is ignored by the already-resolved promise and theRemoteBrowserkeeps the dummy socket, so run events may never reach the client. Mark the initial wait as resolved on timeout and update the existing session for later connections.Proposed fix
const namespace = io.of(id); let clientConnected = false; let connectionTimeout: NodeJS.Timeout; + let browserSession: RemoteBrowser | null = null; const waitForConnection = new Promise<Socket | null>((resolve) => { let initialResolved = false; namespace.on('connection', (socket: Socket) => { if (!initialResolved) { @@ logger.log('info', `Frontend connected to browser ${id} via socket ${socket.id}`); resolve(socket); } else { logger.log('debug', `Additional frontend socket ${socket.id} joined browser ${id} namespace (shared connection)`); + const existingSession = browserPool.getRemoteBrowser(id) ?? browserSession; + if (existingSession) { + existingSession.updateSocket(socket); + socket.emit('ready-for-run'); + } } }); connectionTimeout = setTimeout(() => { - if (!clientConnected) { + if (!clientConnected && !initialResolved) { + initialResolved = true; logger.log('warn', `No client connected to browser ${id} within timeout, proceeding with dummy socket`); resolve(null); } }, 15000); @@ - let browserSession: RemoteBrowser; - if (socket) { logger.log('info', `Using real socket for browser ${id}`); browserSession = new RemoteBrowser(socket, userId, id); } else {Also applies to: 394-415
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/browser-management/classes/RemoteBrowser.ts`:
- Line 260: The change needs to ensure all urlChanged notifications use the
broadcast helper instead of emitting only on this.socket; update the changeTab
method (and the other occurrences around the RemoteBrowser class at the
referenced block ~907-910) to call this.broadcast('urlChanged', { url:
newUrlOrCurrent, userId: this.userId }) wherever you currently call
this.socket.emit('urlChanged', ...), so namespace-wide clients receive
tab-switch URL updates; leave the existing payload structure and userId intact.
In `@src/components/run/RunsTable.tsx`:
- Around line 300-327: The handlers passed inline to the shared socket (from
getOrCreateBrowserSocket) are being removed globally via socket.off('event'),
which unregisters other consumers; instead, extract each listener into a
named/const function (e.g. onConnect, onDebugMessage, onRunCompleted,
onUrlChanged, onDomSnapshotLoading, onConnectError, onDisconnect) when
registering with socket.on, and use socket.off(event, handler) to remove only
those specific handlers in the run completion logic, the inactive path, and the
component unmount cleanup; keep calls to invalidateRuns, setRerenderRuns and
notify inside the onRunCompleted handler but ensure you pass that handler
reference to socket.off so other consumers are not affected.
- Around line 344-347: The disconnect handler currently deletes the socket entry
from activeSocketsRef in the socket.on('disconnect', ...) callback, which breaks
refcount handling in the shared cache; update the disconnect handling so it
pairs with releaseBrowserSocket() instead of directly deleting the map entry (or
remove the delete entirely) to ensure getOrCreateBrowserSocket() and
releaseBrowserSocket() stay balanced and avoid duplicate handlers—modify the
socket.on('disconnect', ...) callback to call releaseBrowserSocket(browserId)
(or omit the deletion) before removing any entry from activeSocketsRef.current
so the refcount is decremented correctly.
In `@src/utils/browserSocket.ts`:
- Around line 11-25: MainPage.tsx is creating browser-scoped sockets directly
via io(`${apiUrl}/${browserId}`) (used for abortSocket and the run-completion
socket) which bypasses the shared cache/refCounts; replace those direct socket
instantiations with getOrCreateBrowserSocket(browserId) so both abortSocket and
the socket used on run completion go through the shared manager (retain existing
variable names like abortSocket and the run-completion socket, remove the direct
io(...) calls, and ensure you no longer set up or manage refCounts manually
there).
🪄 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: c23f1c4c-4ed3-4a14-8ef5-eff809099ae5
📒 Files selected for processing (6)
server/src/browser-management/classes/RemoteBrowser.tsserver/src/browser-management/controller.tsserver/src/workflow-management/classes/Interpreter.tssrc/components/run/ColapsibleRow.tsxsrc/components/run/RunsTable.tsxsrc/utils/browserSocket.ts
| if (this.shouldEmitUrlChange(currentUrl)) { | ||
| this.lastEmittedUrl = currentUrl; | ||
| this.socket.emit('urlChanged', { url: currentUrl, userId: this.userId }); | ||
| this.broadcast('urlChanged', { url: currentUrl, userId: this.userId }); |
There was a problem hiding this comment.
Use the broadcast helper for every urlChanged emission.
Line 260 now broadcasts navigation updates, but changeTab still emits only on this.socket, so additional namespace clients can miss tab-switch URL updates.
Proposed fix
- this.socket.emit('urlChanged', {
+ this.broadcast('urlChanged', {
url: this.currentPage.url(),
userId: this.userId
});Also applies to: 907-910
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/browser-management/classes/RemoteBrowser.ts` at line 260, The
change needs to ensure all urlChanged notifications use the broadcast helper
instead of emitting only on this.socket; update the changeTab method (and the
other occurrences around the RemoteBrowser class at the referenced block
~907-910) to call this.broadcast('urlChanged', { url: newUrlOrCurrent, userId:
this.userId }) wherever you currently call this.socket.emit('urlChanged', ...),
so namespace-wide clients receive tab-switch URL updates; leave the existing
payload structure and userId intact.
| socket.on('connect', () => { | ||
| console.log(`[RunsTable] Connected to browser ${browserId}`); | ||
| console.log(`Connected to browser ${browserId}`); | ||
| }); | ||
|
|
||
| socket.on('debugMessage', (msg: string) => { | ||
| console.log(`[RunsTable] Debug message for ${browserId}:`, msg); | ||
| // Optionally update logs in real-time here | ||
| console.log(`Debug message for ${browserId}:`, msg); | ||
| }); | ||
|
|
||
| socket.on('run-completed', (data: any) => { | ||
| console.log(`[RunsTable] Run completed for ${browserId}:`, data); | ||
| console.log(`Run completed for ${browserId}:`, data); | ||
|
|
||
| // Invalidate cache to show updated run status | ||
| invalidateRuns(); | ||
| setRerenderRuns(true); | ||
|
|
||
| // Show notification | ||
| if (data.status === 'success') { | ||
| notify('success', t('main_page.notifications.interpretation_success', { name: data.robotName || name })); | ||
| } else { | ||
| notify('error', t('main_page.notifications.interpretation_failed', { name: data.robotName || name })); | ||
| } | ||
|
|
||
| socket.disconnect(); | ||
| socket.off('connect'); | ||
| socket.off('debugMessage'); | ||
| socket.off('run-completed'); | ||
| socket.off('urlChanged'); | ||
| socket.off('dom-snapshot-loading'); | ||
| socket.off('connect_error'); | ||
| socket.off('disconnect'); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Socket.IO cleanup calls that remove all listeners for an event.
rg -n -C3 --type ts --type tsx '\.off\(\s*["'\''][^"'\'']+["'\'']\s*\)' srcRepository: getmaxun/maxun
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Search for socket.off calls in the file, adjusted for ripgrep compatibility
rg -n -C3 '\.off\(' src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 1752
🏁 Script executed:
#!/bin/bash
# Get an overview of the RunsTable.tsx file size and structure
wc -l src/components/run/RunsTable.tsx
head -50 src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 2146
🏁 Script executed:
#!/bin/bash
# Check the socket creation and release functions
rg -n 'getOrCreateBrowserSocket\|releaseBrowserSocket' src/components/run/RunsTable.tsx -A 5 -B 2Repository: getmaxun/maxun
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Read the section with handler registration and first cleanup
sed -n '290,330p' src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 1449
🏁 Script executed:
#!/bin/bash
# Look for where socket is obtained and understand the pattern
sed -n '270,305p' src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Check the browserSocket utility to understand socket sharing
cat src/utils/browserSocket.tsRepository: getmaxun/maxun
Length of output: 1371
Unsubscribe only the handlers this component registered.
On a shared socket, socket.off('event') removes every listener for that event, including listeners from other consumers sharing the same browser socket. Since getOrCreateBrowserSocket uses reference counting to share a single socket across multiple components, registering inline handlers and then calling off(event) without the handler reference will remove listeners from other active consumers. Store handler references and call off(event, handler) in completion, inactive, and unmount cleanup paths.
Example pattern
- socket.on('connect', () => {
+ const onConnect = () => {
console.log(`Connected to browser ${browserId}`);
- });
+ };
+
+ socket.on('connect', onConnect);
- socket.off('connect');
+ socket.off('connect', onConnect);Also applies to: 359-366, 375-383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/run/RunsTable.tsx` around lines 300 - 327, The handlers passed
inline to the shared socket (from getOrCreateBrowserSocket) are being removed
globally via socket.off('event'), which unregisters other consumers; instead,
extract each listener into a named/const function (e.g. onConnect,
onDebugMessage, onRunCompleted, onUrlChanged, onDomSnapshotLoading,
onConnectError, onDisconnect) when registering with socket.on, and use
socket.off(event, handler) to remove only those specific handlers in the run
completion logic, the inactive path, and the component unmount cleanup; keep
calls to invalidateRuns, setRerenderRuns and notify inside the onRunCompleted
handler but ensure you pass that handler reference to socket.off so other
consumers are not affected.
| socket.on('disconnect', (reason: string) => { | ||
| console.log(`[RunsTable] Disconnected from browser ${browserId}:`, reason); | ||
| console.log(`Disconnected from browser ${browserId}:`, reason); | ||
| activeSocketsRef.current.delete(browserId); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect socket map deletion paths and nearby releaseBrowserSocket calls.
rg -n -C5 --type ts --type tsx 'activeSocketsRef\.current\.delete\(browserId\)|releaseBrowserSocket\(browserId\)' src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Search for activeSocketsRef deletion and releaseBrowserSocket calls without type restrictions
rg -n -C5 'activeSocketsRef\.current\.delete\(browserId\)|releaseBrowserSocket\(browserId\)' src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 1771
🏁 Script executed:
#!/bin/bash
# Check the full context around lines 344-347
cat -n src/components/run/RunsTable.tsx | sed -n '330,360p'Repository: getmaxun/maxun
Length of output: 1394
🏁 Script executed:
#!/bin/bash
# Find the releaseBrowserSocket function definition
rg -n 'const releaseBrowserSocket|function releaseBrowserSocket' src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check for effect cleanup and component unmount patterns that may handle cleanup
rg -n 'useEffect|cleanup|return \(\)|disconnect' src/components/run/RunsTable.tsx | head -30Repository: getmaxun/maxun
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Find releaseBrowserSocket definition and implementation
rg -n -B2 -A10 'const releaseBrowserSocket|function releaseBrowserSocket' src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for activeSocketsRef initialization and usage pattern
rg -n 'activeSocketsRef' src/components/run/RunsTable.tsx | head -20Repository: getmaxun/maxun
Length of output: 596
🏁 Script executed:
#!/bin/bash
# Search for releaseBrowserSocket in the entire file or imports
rg -n 'releaseBrowserSocket' src/components/run/RunsTable.tsxRepository: getmaxun/maxun
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Check imports at the top of the file
head -50 src/components/run/RunsTable.tsx | cat -nRepository: getmaxun/maxun
Length of output: 2459
🏁 Script executed:
#!/bin/bash
# Check the browserSocket utility file to understand refcount management
cat src/utils/browserSocket.tsRepository: getmaxun/maxun
Length of output: 1371
🏁 Script executed:
#!/bin/bash
# If that doesn't exist, search for it
find src -name '*browserSocket*' -type fRepository: getmaxun/maxun
Length of output: 84
Don't drop the tracked reference on transient disconnects.
The disconnect handler deletes from activeSocketsRef without calling releaseBrowserSocket(), leaving the refcount intact in the shared socket cache. When a transient disconnect occurs and Socket.IO reconnects, or when the effect re-runs, getOrCreateBrowserSocket() will return the cached socket with an incremented refcount. This causes duplicate event handlers to be registered on the same socket since the handlers from the original connection remain active.
All other cleanup paths (lines 328, 366, 383) correctly pair releaseBrowserSocket() with the deletion. Either remove the deletion to preserve the socket entry for reconnects, or call releaseBrowserSocket() before deleting to properly decrement the refcount.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/run/RunsTable.tsx` around lines 344 - 347, The disconnect
handler currently deletes the socket entry from activeSocketsRef in the
socket.on('disconnect', ...) callback, which breaks refcount handling in the
shared cache; update the disconnect handling so it pairs with
releaseBrowserSocket() instead of directly deleting the map entry (or remove the
delete entirely) to ensure getOrCreateBrowserSocket() and releaseBrowserSocket()
stay balanced and avoid duplicate handlers—modify the socket.on('disconnect',
...) callback to call releaseBrowserSocket(browserId) (or omit the deletion)
before removing any entry from activeSocketsRef.current so the refcount is
decremented correctly.
| export function getOrCreateBrowserSocket(browserId: string): Socket { | ||
| if (socketCache.has(browserId)) { | ||
| const count = refCounts.get(browserId) || 0; | ||
| refCounts.set(browserId, count + 1); | ||
| return socketCache.get(browserId)!; | ||
| } | ||
|
|
||
| const socket = io(`${apiUrl}/${browserId}`, { | ||
| transports: ["websocket", "polling"], | ||
| rejectUnauthorized: false, | ||
| }); | ||
|
|
||
| socketCache.set(browserId, socket); | ||
| refCounts.set(browserId, 1); | ||
| return socket; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify browser-scoped Socket.IO clients are routed through src/utils/browserSocket.ts.
rg -n -C3 --type ts --type tsx 'io\s*\(\s*`\$\{apiUrl\}/\$\{browserId\}`|io\s*\(\s*`\$\{apiUrl\}/' srcRepository: getmaxun/maxun
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Fix: Search for direct io() calls with browser namespace patterns
# Using -t=ts for TypeScript files only (tsx files are treated as ts)
rg -n -C3 -t ts 'io\s*\(\s*`\$\{apiUrl\}/\$\{browserId\}`|io\s*\(\s*`\$\{apiUrl\}/' srcRepository: getmaxun/maxun
Length of output: 1295
🏁 Script executed:
#!/bin/bash
# Check the specific file mentioned in the review comment
echo "=== Checking src/pages/MainPage.tsx ==="
if [ -f "src/pages/MainPage.tsx" ]; then
echo "File exists. Checking for io() calls:"
# Get line count to verify file size
wc -l src/pages/MainPage.tsx
echo ""
# Check around the mentioned line ranges (92-97 and 191-194)
echo "Lines 85-100:"
sed -n '85,100p' src/pages/MainPage.tsx
echo ""
echo "Lines 185-200:"
sed -n '185,200p' src/pages/MainPage.tsx
else
echo "File does not exist"
fiRepository: getmaxun/maxun
Length of output: 1216
🏁 Script executed:
#!/bin/bash
# Search for all io() calls to understand usage patterns
echo "=== All io() calls in src directory ==="
rg -n 'io\s*\(' -t ts src | head -50Repository: getmaxun/maxun
Length of output: 495
Route remaining browser socket consumers through the shared manager.
Browser-scoped socket consumers in src/pages/MainPage.tsx bypass the shared manager. Direct io(${apiUrl}/${browserId}) calls at lines 94 and 191 create separate socket instances outside the cache/refCount tracking, allowing multiple connections for the same browser namespace in a single run.
- Line 94:
abortSocketcreation - Line 191: socket creation for run completion
Both should use getOrCreateBrowserSocket(browserId) instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/browserSocket.ts` around lines 11 - 25, MainPage.tsx is creating
browser-scoped sockets directly via io(`${apiUrl}/${browserId}`) (used for
abortSocket and the run-completion socket) which bypasses the shared
cache/refCounts; replace those direct socket instantiations with
getOrCreateBrowserSocket(browserId) so both abortSocket and the socket used on
run completion go through the shared manager (retain existing variable names
like abortSocket and the run-completion socket, remove the direct io(...) calls,
and ensure you no longer set up or manage refCounts manually there).
2 participants
What this PR does?
It was noticed that multiple socket connections were being opened for different components present on the client side for a single run. This is totally unnecessary polling of socket connections. This fix unifies the socket connection and distributes it across components.
Summary by CodeRabbit
Bug Fixes
Refactor