feat: enable client-side notifications for all runs #736
Conversation
WalkthroughAdds a new Socket.IO namespace (/queued-run) on the server, introduces a dedicated queueSocket in the React context with connect/disconnect APIs and run-completed handling, and refactors MainPage to use the shared queue socket, adding initialContent to props and managing lifecycle and notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SocketContext
participant Server as Server (/queued-run)
Client->>SocketContext: connectToQueueSocket(userId, onRunCompleted)
SocketContext->>Server: connect (namespace: /queued-run?userId=...)
alt userId present
Server-->>SocketContext: connect ack
Server->>Server: join room user-{userId}
else missing userId
Server-->>SocketContext: disconnect
end
sequenceDiagram
participant Server as Server (/queued-run)
participant Room as room user-{userId}
participant SocketContext
participant MainPage
Server->>Room: emit run-completed(data)
Room-->>SocketContext: run-completed(data)
SocketContext->>MainPage: invoke onRunCompleted(data)
MainPage->>MainPage: update state, queued set, notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 3
🧹 Nitpick comments (3)
src/context/socket.tsx (2)
72-80: Avoid dual state sources for the queue socket unless requiredYou update both React state and the singleton store (
socketStore.queueSocket = newQueueSocket). Unless something else reads from the store outside React, keeping one source of truth (React state) is simpler.If the store write is not needed, remove it to reduce surface for inconsistencies.
94-102: Simplify unmount cleanup to use the provided APIOptional: delegate cleanup to
disconnectQueueSocket()for consistency (also clears the callback ref). This avoids capturing a stalequeueSocketin the effect closure.- useEffect(() => { - return () => { - if (queueSocket) { - queueSocket.disconnect(); - } - }; - }, [queueSocket]); + useEffect(() => { + return () => { + disconnectQueueSocket(); + }; + }, [disconnectQueueSocket]);src/pages/MainPage.tsx (1)
221-227: Optional: localize the fallback labelInstead of hardcoding 'Unknown Robot', consider a translation key to stay consistent with i18n.
- const robotName = completionData.robotName || 'Unknown Robot'; + const robotName = completionData.robotName || t('common.unknown_robot', 'Unknown Robot');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
server/src/server.ts(1 hunks)src/context/socket.tsx(3 hunks)src/pages/MainPage.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
server/src/server.ts (1)
server/src/routes/storage.ts (1)
processQueuedRuns(1044-1044)
src/context/socket.tsx (1)
server/src/server.ts (1)
io(80-80)
src/pages/MainPage.tsx (1)
src/context/socket.tsx (1)
useSocketStore(25-25)
🔇 Additional comments (3)
server/src/server.ts (1)
163-177: Events are already emitted on the '/queued-run' namespace — no change requiredVerified: the server defines and exports io and the worker emits to the '/queued-run' namespace (not the default namespace).
Files to note:
- server/src/server.ts — exports io (export const io = new Server(server)) and registers io.of('/queued-run').on('connection', ...) (namespace setup at ~lines 163–177).
- server/src/pgboss-worker.ts — imports { io as serverIo } and emits run-completed to the namespace: serverIo.of('/queued-run').to(
user-${data.userId}).emit('run-completed', ...) (examples at lines 437–438, 496–497, 547–548, 617–618).- server/src/browser-management/controller.ts — uses io.of(id) for browser-specific namespaces (expected behavior).
Conclusion: the original concern is addressed in the codebase; no changes required.
src/context/socket.tsx (1)
7-14: Good API surface for queue socket and callback-based run completion handlingThe additional queueSocket in context, plus connect/disconnect APIs and the callback ref, is a clean separation from the existing browser-run socket and avoids stale closures.
Also applies to: 17-19, 29-31
src/pages/MainPage.tsx (1)
18-18: Imports align with the new centralized queue socket APIUsing
useSocketStorehere keeps socket lifecycle in one place and reduces per-component socket management.
| io.of('/queued-run').on('connection', (socket) => { | ||
| const userId = socket.handshake.query.userId as string; | ||
|
|
||
| if (userId) { | ||
| socket.join(`user-${userId}`); | ||
| logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`); | ||
|
|
||
| socket.on('disconnect', () => { | ||
| logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`); | ||
| }); | ||
| } else { | ||
| logger.log('warn', `Client connected to queued-run namespace without userId: ${socket.id}`); | ||
| socket.disconnect(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not trust client-supplied userId; validate and type-guard handshake input before joining rooms
Currently, a client can arbitrarily provide any userId via query and subscribe to another user's room, which is a PII leakage risk. Also, socket.handshake.query.userId as string may be an array at runtime. Add runtime guards and avoid trusting client-provided identifiers without authentication.
Refactor to bind the namespace to a variable, export it for emitters, and add strict parsing of userId:
-io.of('/queued-run').on('connection', (socket) => {
- const userId = socket.handshake.query.userId as string;
-
- if (userId) {
- socket.join(`user-${userId}`);
- logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`);
-
- socket.on('disconnect', () => {
- logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`);
- });
- } else {
- logger.log('warn', `Client connected to queued-run namespace without userId: ${socket.id}`);
- socket.disconnect();
- }
-});
+const queuedRunNsp = io.of('/queued-run');
+export const queuedRunNamespace = queuedRunNsp;
+
+queuedRunNsp.on('connection', (socket) => {
+ const raw = socket.handshake.query?.userId as unknown;
+ const userId = Array.isArray(raw) ? raw[0] : raw;
+
+ if (typeof userId === 'string' && userId.trim().length > 0) {
+ socket.join(`user-${userId}`);
+ logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`);
+
+ socket.on('disconnect', () => {
+ logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`);
+ });
+ } else {
+ logger.log('warn', `Client connected to queued-run namespace without a valid userId: ${socket.id}`);
+ socket.disconnect();
+ }
+});Follow-up: Strongly consider authenticating the socket (e.g., via a signed token in handshake.auth, or reading the server session and deriving userId on the server) instead of accepting it from the client. This prevents room hijacking. If you want, I can provide a middleware example for JWT/session validation.
📝 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.
| io.of('/queued-run').on('connection', (socket) => { | |
| const userId = socket.handshake.query.userId as string; | |
| if (userId) { | |
| socket.join(`user-${userId}`); | |
| logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`); | |
| socket.on('disconnect', () => { | |
| logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`); | |
| }); | |
| } else { | |
| logger.log('warn', `Client connected to queued-run namespace without userId: ${socket.id}`); | |
| socket.disconnect(); | |
| } | |
| }); | |
| const queuedRunNsp = io.of('/queued-run'); | |
| export const queuedRunNamespace = queuedRunNsp; | |
| queuedRunNsp.on('connection', (socket) => { | |
| const raw = socket.handshake.query?.userId as unknown; | |
| const userId = Array.isArray(raw) ? raw[0] : raw; | |
| if (typeof userId === 'string' && userId.trim().length > 0) { | |
| socket.join(`user-${userId}`); | |
| logger.log('info', `Client joined queued-run namespace for user: ${userId}, socket: ${socket.id}`); | |
| socket.on('disconnect', () => { | |
| logger.log('info', `Client disconnected from queued-run namespace: ${socket.id}`); | |
| }); | |
| } else { | |
| logger.log('warn', `Client connected to queued-run namespace without a valid userId: ${socket.id}`); | |
| socket.disconnect(); | |
| } | |
| }); |
| const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, { | ||
| transports: ["websocket"], | ||
| rejectUnauthorized: false, | ||
| query: { userId } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove ineffective/insecure option and prefer authenticated handshake over query
rejectUnauthorized: false has no effect in the browser and is misleading. Also, passing userId in the query is easily spoofed; prefer a signed token via auth or rely on server-side session.
Apply this minimal cleanup:
- const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, {
- transports: ["websocket"],
- rejectUnauthorized: false,
- query: { userId }
- });
+ const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, {
+ transports: ["websocket"],
+ query: { userId }
+ });Longer-term: move to auth: { token } and validate on the server in a namespace middleware.
📝 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.
| const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, { | |
| transports: ["websocket"], | |
| rejectUnauthorized: false, | |
| query: { userId } | |
| }); | |
| const newQueueSocket = io(`${SERVER_ENDPOINT}/queued-run`, { | |
| transports: ["websocket"], | |
| query: { userId } | |
| }); |
🤖 Prompt for AI Agents
In src/context/socket.tsx around lines 51 to 56, remove the ineffective and
misleading rejectUnauthorized: false option and stop sending userId in the query
(it is easily spoofed); instead, pass authentication via the socket auth payload
(e.g. auth: { token }) or rely on server-side session authentication—update the
io(...) options to remove rejectUnauthorized and move/replace query: { userId }
with a secure auth field or omit client-side identity entirely, and ensure
server-side namespace middleware validates the token/session.
| useEffect(() => { | ||
| if (user?.id) { | ||
| const handleRunCompleted = (completionData: any) => { | ||
| setRerenderRuns(true); | ||
|
|
||
| if (queuedRuns.has(completionData.runId)) { | ||
| setQueuedRuns(prev => { | ||
| const newSet = new Set(prev); | ||
| newSet.delete(completionData.runId); | ||
| return newSet; | ||
| }); | ||
| } | ||
|
|
||
| const robotName = completionData.robotName || 'Unknown Robot'; | ||
|
|
||
| if (completionData.status === 'success') { | ||
| notify('success', t('main_page.notifications.interpretation_success', { name: robotName })); | ||
| } else { | ||
| notify('error', t('main_page.notifications.interpretation_failed', { name: robotName })); | ||
| } | ||
| }; | ||
|
|
||
| connectToQueueSocket(user.id, handleRunCompleted); | ||
|
|
||
| return () => { | ||
| disconnectQueueSocket(); | ||
| }; | ||
| } | ||
| }, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, queuedRuns, setQueuedRuns]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent unnecessary socket reconnections caused by queuedRuns in deps; use an idempotent delete
Including queuedRuns in the dependency array will reconnect the queue socket on every state change. Instead, remove the membership check and always attempt to delete within a functional setter, then drop queuedRuns from deps.
Apply this diff:
useEffect(() => {
if (user?.id) {
const handleRunCompleted = (completionData: any) => {
setRerenderRuns(true);
-
- if (queuedRuns.has(completionData.runId)) {
- setQueuedRuns(prev => {
- const newSet = new Set(prev);
- newSet.delete(completionData.runId);
- return newSet;
- });
- }
+ setQueuedRuns(prev => {
+ const newSet = new Set(prev);
+ newSet.delete(completionData.runId); // idempotent
+ return newSet;
+ });
const robotName = completionData.robotName || 'Unknown Robot';
if (completionData.status === 'success') {
notify('success', t('main_page.notifications.interpretation_success', { name: robotName }));
} else {
notify('error', t('main_page.notifications.interpretation_failed', { name: robotName }));
}
};
- connectToQueueSocket(user.id, handleRunCompleted);
+ connectToQueueSocket(String(user.id), handleRunCompleted);
return () => {
disconnectQueueSocket();
};
}
- }, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, queuedRuns, setQueuedRuns]);
+ }, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, setQueuedRuns]);Notes:
- Casting
user.idto string prevents accidental type mismatches with the server expectation. - Dropping
queuedRunsfrom deps avoids reconnect churn while still correctly updating the set.
📝 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.
| useEffect(() => { | |
| if (user?.id) { | |
| const handleRunCompleted = (completionData: any) => { | |
| setRerenderRuns(true); | |
| if (queuedRuns.has(completionData.runId)) { | |
| setQueuedRuns(prev => { | |
| const newSet = new Set(prev); | |
| newSet.delete(completionData.runId); | |
| return newSet; | |
| }); | |
| } | |
| const robotName = completionData.robotName || 'Unknown Robot'; | |
| if (completionData.status === 'success') { | |
| notify('success', t('main_page.notifications.interpretation_success', { name: robotName })); | |
| } else { | |
| notify('error', t('main_page.notifications.interpretation_failed', { name: robotName })); | |
| } | |
| }; | |
| connectToQueueSocket(user.id, handleRunCompleted); | |
| return () => { | |
| disconnectQueueSocket(); | |
| }; | |
| } | |
| }, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, queuedRuns, setQueuedRuns]); | |
| useEffect(() => { | |
| if (user?.id) { | |
| const handleRunCompleted = (completionData: any) => { | |
| setRerenderRuns(true); | |
| setQueuedRuns(prev => { | |
| const newSet = new Set(prev); | |
| newSet.delete(completionData.runId); // idempotent | |
| return newSet; | |
| }); | |
| const robotName = completionData.robotName || 'Unknown Robot'; | |
| if (completionData.status === 'success') { | |
| notify('success', t('main_page.notifications.interpretation_success', { name: robotName })); | |
| } else { | |
| notify('error', t('main_page.notifications.interpretation_failed', { name: robotName })); | |
| } | |
| }; | |
| connectToQueueSocket(String(user.id), handleRunCompleted); | |
| return () => { | |
| disconnectQueueSocket(); | |
| }; | |
| } | |
| }, [user?.id, connectToQueueSocket, disconnectQueueSocket, t, setRerenderRuns, setQueuedRuns]); |
🤖 Prompt for AI Agents
In src/pages/MainPage.tsx around lines 208 to 237, the effect reconnects the
socket on every queuedRuns state change and uses a membership check before
deleting; remove queuedRuns from the dependency array, change the handler to
always delete the runId using the functional state setter (no prior has()
check), and cast user.id to string when calling connectToQueueSocket to avoid
type mismatches; keep all other dependencies (connectToQueueSocket,
disconnectQueueSocket, t, setRerenderRuns, setQueuedRuns) in the array and
ensure disconnectQueueSocket is still called in the cleanup.
2 participants
What this PR does?
Enables client side run completion or failure notifications for all queued run execution.
Summary by CodeRabbit
New Features
Refactor