feat(new-webui): Add client library for real-time MongoDB updates. - #892
Conversation
## Walkthrough
This change introduces a real-time client-server data subscription system using Socket.IO for MongoDB collections. It adds client-side socket and cursor classes, a React hook for live data, updates TypeScript and Vite configurations for path aliasing, and modifies server-side event handling to streamline collection initialization and subscription logic.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `client/package.json` | Added `socket.io-client` and `vite-tsconfig-paths` as dependencies. |
| `client/src/api/socket/MongoCollectionSocket.ts`, `client/src/api/socket/MongoCursorSocket.ts` | Introduced `MongoCollectionSocket` for socket-based collection access and `MongoCursorSocket` for real-time cursor-like data subscriptions. |
| `client/src/api/socket/useCursor.tsx` | Added a React hook `useCursor` for reactive data arrays from `MongoCursorSocket`. |
| `client/src/api/socket/SocketSingleton.ts` | Added singleton pattern to manage a shared Socket.IO client connection. |
| `client/tsconfig/tsconfig.app.json`, `client/vite.config.ts` | Updated TypeScript and Vite configs for path aliasing, WebSocket proxying, and file system serving from parent directory. |
| `common/index.ts` | Removed `"collection::init"` event type, updated `"collection::find::subscribe"` event to include `collectionName`, and changed export statements to `export type`. |
| `server/src/plugins/MongoSocketIoServer/index.ts` | Removed collection init event and listener, added method to get or create watcher collections, updated subscription listener to require collection name and validate collection existence, improving error handling and logging. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant ReactComponent
participant useCursor
participant MongoCursorSocket
participant MongoCollectionSocket
participant SocketIOClient
participant SocketIOServer
participant MongoDB
ReactComponent->>useCursor: Call useCursor(query, deps)
useCursor->>MongoCollectionSocket: query() → MongoCursorSocket
useCursor->>MongoCursorSocket: subscribe(onDataUpdate)
MongoCursorSocket->>SocketIOClient: Emit "collection::find::subscribe"
SocketIOClient->>SocketIOServer: "collection::find::subscribe" (collectionName, query, options)
SocketIOServer->>MongoDB: Check collection existence
alt Collection exists
SocketIOServer->>SocketIOClient: Ack with initial data, queryId
SocketIOServer-->>SocketIOClient: Push "collection::find::update" on data change
SocketIOClient->>MongoCursorSocket: Receive update
MongoCursorSocket->>useCursor: onDataUpdate(data)
useCursor->>ReactComponent: setState(data)
else Collection missing
SocketIOServer->>SocketIOClient: Ack with error
MongoCursorSocket->>useCursor: Throw error
end
ReactComponent-->>useCursor: Unmount
useCursor->>MongoCursorSocket: unsubscribe()
MongoCursorSocket->>SocketIOClient: Emit "collection::find::unsubscribe"
SocketIOClient->>SocketIOServer: "collection::find::unsubscribe"Possibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
components/log-viewer-webui/client/package.json(1 hunks)components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts(1 hunks)components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts(1 hunks)components/log-viewer-webui/client/src/api/socket/useCursor.tsx(1 hunks)components/log-viewer-webui/client/tsconfig/tsconfig.app.json(2 hunks)components/log-viewer-webui/client/vite.config.ts(2 hunks)components/log-viewer-webui/common/index.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/vite.config.tscomponents/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.tscomponents/log-viewer-webui/common/index.tscomponents/log-viewer-webui/client/src/api/socket/MongoCursorSocket.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.tscomponents/log-viewer-webui/client/src/api/socket/useCursor.tsx
🧬 Code Graph Analysis (2)
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts (2)
components/log-viewer-webui/common/index.ts (2)
ServerToClientEvents(80-80)ClientToServerEvents(76-76)components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (1)
MongoCursorSocket(107-107)
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (1)
components/log-viewer-webui/common/index.ts (4)
ServerToClientEvents(80-80)ClientToServerEvents(76-76)QueryId(82-82)Response(79-79)
🪛 Biome (1.9.4)
components/log-viewer-webui/client/tsconfig/tsconfig.app.json
[error] 21-21: Expected a property but instead found '// Map imports from "@common/*" to the shared folder'.
Expected a property here.
(parse)
🔇 Additional comments (9)
components/log-viewer-webui/client/package.json (1)
26-27: Dependencies added correctly for socket integrationThe addition of
socket.io-clientandvite-tsconfig-pathsdependencies properly supports the real-time MongoDB query service integration described in the PR objectives. The socket.io-client enables websocket communication while vite-tsconfig-paths supports the TypeScript path aliasing needed for the common directory imports.components/log-viewer-webui/client/tsconfig/tsconfig.app.json (1)
4-5: Common directory inclusion looks goodAdding the common directory to the TypeScript compilation scope is appropriate for accessing shared types and interfaces between client and server components.
components/log-viewer-webui/client/vite.config.ts (5)
3-3: Import for tsconfig paths plugin added correctlyThe import of the
vite-tsconfig-pathsplugin aligns with the dependency added in package.json and is necessary for TypeScript path resolution in Vite.
9-10: Plugin configuration looks goodThe tsconfig paths plugin is properly configured in the Vite plugins array, enabling the path aliases defined in tsconfig.app.json.
13-13: Server configured to bind on all network interfacesSetting
host: "0.0.0.0"allows the development server to accept connections from any network interface, which is useful for testing across devices but may have security implications in certain environments.
22-26: WebSocket proxy configured correctly for Socket.IOThe WebSocket proxy configuration is correctly set up to forward Socket.IO connections to the backend server, with appropriate options for WebSocket support.
28-31: File system configuration allows access to common directoryThe configuration to serve files from one level up in the directory structure is necessary for accessing the common directory, which contains shared types for the client and server.
components/log-viewer-webui/common/index.ts (2)
32-39: Simplified collection init event signatureThe
"collection::init"event no longer requires a callback parameter, aligning with server-side changes that simplify the collection initialization process. This is a clean simplification that reduces complexity.
75-83: Type-only exports improve type safetyChanging to type-only exports with
export typeis a good practice that prevents type information from being included in the compiled JavaScript bundle, potentially reducing bundle size and improving type safety.
| constructor (collectionName: string) { | ||
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: Add support for user provided domain name (i.e. io("https://server-domain.com")). | ||
| // Implementation could involve parsing server .env file and moving server .env to a | ||
| // common folder. | ||
|
|
||
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: Current implementation creates a new socket connection for each collection. This | ||
| // could be problematic if the number of collections is large since browsers limit the | ||
| // number of web sockets per domain. An simple change is to use Socket.IO namespace | ||
| // feature, and having Socket.IO multiplex the socket per collection namespace. Another | ||
| // more involved option is to implement a shared socket. | ||
| // Namespace reference: https://socket.io/docs/v4/namespaces/ | ||
| this.socket = io(); | ||
| this.socket.emit("collection::init", { | ||
| collectionName: collectionName, | ||
| }); | ||
| console.log(`MongoDB collection:${collectionName} initialized.`); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Socket lifecycle is never closed – risk of resource leaks
MongoCollectionSocket opens a fresh socket per instance (io()), but there is no public disconnect()/close() method or automatic teardown logic (beforeunload, React unmount, etc.).
In long-running UIs that mount/unmount many components (or hot-reload during dev), the browser can easily exceed the ~6 WebSocket per-origin cap and silently drop newer ones.
Diff sketch:
+ /**
+ * Cleanly tears down the socket connection.
+ * Call this when the owning component unmounts.
+ */
+ public disconnect(): void {
+ if (this.socket.connected) {
+ this.socket.disconnect();
+ }
+ }Expose this method (or share a singleton socket—see next comment) and invoke it from the consumer (e.g., inside useEffect cleanup that creates the collection object).
🏁 Script executed:
#!/bin/bash
# Inspect MongoCollectionSocket.ts and search for disconnect/close usage
FILE="components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts"
echo "=== Contents of $FILE ==="
sed -n '1,200p' "$FILE"
echo
echo "=== Searching for 'disconnect' across the repo ==="
rg "disconnect" -n .
echo
echo "=== Searching for 'close' across the repo ==="
rg "close" -n .Length of output: 66430
Add socket teardown to prevent connection leaks
The MongoCollectionSocket constructor creates a new io() connection but never closes it, which can exhaust the browser’s per-origin WebSocket limit in long-running or hot-reloaded UIs.
Please implement and expose a cleanup method and invoke it from the owning component’s unmount logic:
• File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts
– After the existing methods, add:
/**
* Cleanly disconnects the socket.
* Call this when the owning component unmounts.
*/
public disconnect(): void {
if (this.socket.connected) {
this.socket.disconnect();
}
}• Consumer (e.g. in a React component):
const collectionSocket = new MongoCollectionSocket("myCollection");
useEffect(() => {
return () => {
collectionSocket.disconnect();
};
}, []);This ensures sockets are closed when no longer needed.
There was a problem hiding this comment.
When the class is garbage collected, will it automatically disconnect?
There was a problem hiding this comment.
🧩 Analysis chain
I need to research this question about Socket.IO garbage collection behavior to give you an accurate answer.
🌐 Web query:
Does Socket.IO automatically disconnect when the socket object is garbage collected in JavaScript?
💡 Result:
Does Socket.IO Automatically Disconnect When the Socket Object Is Garbage Collected in JavaScript?
Summary:
No, Socket.IO does not automatically disconnect a socket when its JavaScript object is garbage collected. Explicit disconnection is required to properly close the network connection.
Detailed Explanation
1. Socket.IO Connection Lifecycle
- Socket.IO maintains a live connection between the client and server, managed internally by its own event system and timers[1][4].
- To close a connection intentionally, you must explicitly call
socket.disconnect()on the client-side or server-side[5][8].
2. Garbage Collection in JavaScript
- In JavaScript, objects are garbage collected when there are no more references to them, but this does not guarantee any cleanup of side effects or external resources (like network sockets).
- The garbage collector only frees memory; it does not invoke any disconnection logic provided by Socket.IO.
3. Implicit vs. Explicit Disconnection
-
Explicit Disconnection:
- Use
socket.disconnect()to close the connection cleanly and emit thedisconnectevent[5][8]. - Once disconnected, the socket will not try to reconnect (unless you explicitly call
connectagain)[5].
- Use
-
Garbage Collection Alone:
- If the socket object is simply dereferenced and left for garbage collection, the underlying connection may persist until the OS or browser forcibly closes idle or orphaned connections (which could take time and may not be reliable)[1][3][4].
- Socket.IO does not register a finalizer or destructor with the JavaScript garbage collector to disconnect itself automatically.
4. Memory and Resource Management
- Relying on garbage collection to manage network connections is not safe, as network resources (like open TCP connections) can remain open, potentially causing memory leaks or exhausting server resources[9].
- Proper practice is always to call
.disconnect()when you are done using a socket.
5. Analogies and Related Technologies
- Other networking libraries (e.g., in .NET or certain C++ environments) do provide explicit
.close()or.dispose()methods, and recommend their use to ensure proper resource cleanup[6]. - In JavaScript, such deterministic cleanup must be manually invoked.
Conclusion
Socket.IO does not automatically disconnect sockets when their JavaScript object is garbage collected.
To ensure connections are closed and resources are cleaned up, always explicitly call socket.disconnect() when you are finished with a socket object[5][8]. Garbage collection alone will not handle network disconnection.
Citations:
- 1: https://socket.io/docs/v4/tutorial/handling-disconnections
- 2: How do i prevent auto disconnect? socketio/socket.io#4739
- 3: https://socket.io/docs/v4/troubleshooting-connection-issues/
- 4: https://socket.io/docs/v3/client-socket-instance/
- 5: https://socket.io/docs/v4/client-api/
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.dispose?view=net-9.0
- 7: https://socketcluster.io/docs/api-scsocket-client/
- 8: https://socket.io/docs/v3/how-it-works/
- 9: https://forum.dlang.org/thread/qiqgsrpkyuaoivpebmns@forum.dlang.org
- 10: Empty rooms aren't deleted socketio/socket.io#1599
@davemarco No, Socket.IO connections are not automatically disconnected when the object is garbage collected. JavaScript's garbage collector only frees memory; it doesn't handle cleanup of external resources like network connections.
The socket connection will remain open even after the class instance is garbage collected, which can lead to:
- Exhaustion of the browser's WebSocket connection limit (typically ~6 per origin)
- Unnecessary server resource consumption
- Potential memory leaks
You should implement the suggested disconnect() method and ensure it's called when the connection is no longer needed (typically in component unmount logic):
/**
* Cleanly disconnects the socket.
* Call this when the owning component unmounts.
*/
public disconnect(): void {
if (this.socket.connected) {
this.socket.disconnect();
}
}This is standard practice for managing connection lifecycle in networking libraries.
✏️ Learnings added
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:25-43
Timestamp: 2025-05-09T18:06:33.030Z
Learning: Socket.IO connections in JavaScript require explicit disconnection by calling socket.disconnect() and are not automatically cleaned up when the object is garbage collected.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
hmm i imagine this will be difficult to implement in the current useCusor hook. Will the connection automatically disconnect after a certain amount of no activity?
There was a problem hiding this comment.
🧩 Analysis chain
@davemarco Socket.IO does have a built-in ping/pong mechanism, but it doesn't automatically disconnect inactive connections by default. Here's how it works:
- Socket.IO has a "ping timeout" mechanism, but this is primarily to detect connection failures, not application inactivity.
- By default, pings are sent every 25 seconds, and if no pong is received within 20 seconds after a ping, the connection is considered broken.
- These timeouts detect network issues, not application inactivity - a silent but connected client will still maintain its connection.
Regarding implementation in useCursor:
You could modify the useCursor hook to handle the disconnection in its cleanup function:
// In useCursor.tsx
useEffect(() => {
// ... existing subscription logic ...
return () => {
// Cleanup function when hook unmounts
if (cursor) {
cursor.unsubscribe();
// If you had access to the MongoCollectionSocket instance:
// collectionSocket.disconnect();
}
};
}, [dependencies]);The challenge is getting access to the underlying MongoCollectionSocket instance from the cursor. You might need to refactor to either:
- Expose the socket through the cursor
- Use a singleton pattern or context for socket management
- Pass socket lifecycle responsibility to the component using the hook
(●__●)
🌐 Web query:
Does Socket.IO automatically disconnect inactive connections after a timeout period?
💡 Result:
Socket.IO and Automatic Disconnection on Inactivity
Summary:
Yes, Socket.IO does automatically disconnect inactive connections after a timeout period using a built-in heartbeat mechanism. This mechanism relies on periodic "ping" and "pong" messages to detect inactive or lost connections.
How the Timeout Works
-
Heartbeat Mechanism:
The server sends a PING packet to each connected client at regular intervals defined by thepingInterval. The client must reply with a PONG within a time window defined bypingTimeout. If the server does not receive a PONG in time, it considers the connection closed due to inactivity and disconnects the client[4][7][9]. -
Default Values:
pingInterval: 25,000 ms (25 seconds)pingTimeout: 20,000 ms (20 seconds)
These can be adjusted in the server configuration[9].
-
Disconnection Reason:
When a connection is disconnected due to inactivity, the reason is logged as "ping timeout"[4][7][9]. -
Both Client and Server:
- If the client does not receive a ping from the server within
pingInterval + pingTimeout, it also considers the connection lost and disconnects itself[4][7][9].
- If the client does not receive a ping from the server within
Example Configuration (Node.js):
const io = new Server({
pingInterval: 25000,
pingTimeout: 20000,
});You can increase pingTimeout and pingInterval to allow for longer periods of inactivity before disconnecting, but setting these too high may delay the detection of truly lost connections[2][7][9].
Common Scenarios and Limitations
-
Browser/OS Power Saving:
If a browser tab is minimized or a mobile app is backgrounded, timers may be throttled or network connections paused, leading to unexpected disconnects even if the user expects to remain connected[2][1][7]. -
Proxies, Load Balancers, and Network Devices:
In some environments, proxies or load balancers may close inactive WebSocket connections after their own timeout period, potentially overriding Socket.IO's settings[2][8][10].
Key Takeaway
- Socket.IO will automatically disconnect connections that are inactive beyond the configured timeout period, primarily as a safety and resource-saving feature, but this is highly configurable[4][7][9].
- Unexpected disconnects can also occur due to factors outside of Socket.IO, such as browser power-saving, proxies, or network interruptions[2][8][10].
References to the Mechanism in Official Documentation:
“At a given interval (the pingInterval value sent in the handshake) the server sends a PING packet and the client has a few seconds (the pingTimeout value) to send a PONG packet back. If the server does not receive a PONG packet back, it will consider that the connection is closed.”[4]
“The server sends a ping packet every pingInterval ms, and if the client does not answer with a pong within pingTimeout ms, the server considers that the connection is closed.”[7]
Conclusion:
Socket.IO does automatically disconnect clients after a period of inactivity, as determined by its ping/pong heartbeat mechanism and the values of pingInterval and pingTimeout[4][7][9]. These settings can be customized to suit your application's needs.
Citations:
- 1: How do i prevent auto disconnect? socketio/socket.io#4739
- 2: https://socket.io/docs/v4/troubleshooting-connection-issues/
- 3: https://socket.io/docs/v4/tutorial/handling-disconnections
- 4: https://socket.io/docs/v4/how-it-works/
- 5: https://elixirforum.com/t/socket-disconnecting-after-60-sec-without-sending-data/39804
- 6: https://socket.io/docs/v4/emitting-events/
- 7: https://www.pixelstech.net/article/1719122489-the-pitfall-of-websocket-disconnections-caused-by-browser-power-saving-mechanisms
- 8: Socket.io connects and disconnects only for some clients resulting in package lost / unreliability socketio/socket.io#4298
- 9: https://socket.io/docs/v4/server-options/
- 10: https://community.render.com/t/socket-io-in-a-node-app/3051
@davemarco Yes, Socket.IO has a built-in heartbeat mechanism, but it's designed to detect broken connections, not application inactivity:
- By default, the server sends a PING every 25 seconds
- Clients must respond with a PONG within 20 seconds
- If no PONG is received, the connection is closed with "ping timeout" reason
This mechanism only detects network failures. If your connection is healthy but idle, it will remain open indefinitely.
For implementing disconnection in useCursor, you have several options:
-
Refactor to expose the socket: Modify your
MongoCursorSocketto provide access to the underlyingMongoCollectionSocket// In MongoCursorSocket.ts getCollection(): MongoCollectionSocket { return this._collection; // You would need to store this reference }
-
Use a connection manager: Create a singleton that tracks all socket connections
-
React Context provider: Manage connections at a higher level through React Context
The simplest approach would be to pass the collection instance to the cursor and expose a getter:
// When creating the cursor
const cursor = collectionSocket.find(query, options);
useEffect(() => {
// ...subscription logic
return () => {
cursor.unsubscribe();
cursor.getCollection().disconnect();
}
}, []);(x__x)
✏️ Learnings added
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:25-43
Timestamp: 2025-05-09T18:21:05.824Z
Learning: Socket.IO has a built-in ping/pong mechanism that detects connection failures, but it doesn't automatically disconnect truly inactive but connected clients. By default, it sends pings every 25 seconds and expects pongs within 20 seconds, but this is for connection health monitoring, not application inactivity detection.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:25-43
Timestamp: 2025-05-09T18:21:05.824Z
Learning: Socket.IO has a built-in heartbeat mechanism that uses PING/PONG messages (default: 25s interval, 20s timeout) to detect broken connections, but this only detects network failures, not application inactivity. If a connection is healthy but idle, it will remain open indefinitely unless manually disconnected.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
No this looks terrible. will namespace help?
There was a problem hiding this comment.
🧩 Analysis chain
@davemarco Yes, namespaces would help significantly! They're actually the perfect solution for this problem.
Socket.IO namespaces allow multiple logical connections to share a single physical WebSocket connection. This directly addresses your TODO comment about browser connection limits.
Here's how you could implement it:
// SocketManager.ts - Singleton to manage the socket connection
export class SocketManager {
private static instance: SocketManager;
private socket: Socket;
private collectionSockets: Map<string, Socket> = new Map();
private constructor() {
this.socket = io();
}
public static getInstance(): SocketManager {
if (!SocketManager.instance) {
SocketManager.instance = new SocketManager();
}
return SocketManager.instance;
}
public getCollectionSocket(collectionName: string): Socket {
if (!this.collectionSockets.has(collectionName)) {
// Create a namespace for this collection
const collectionSocket = this.socket.io(`/${collectionName}`);
this.collectionSockets.set(collectionName, collectionSocket);
}
return this.collectionSockets.get(collectionName)!;
}
public disconnect(): void {
if (this.socket.connected) {
this.socket.disconnect();
this.collectionSockets.clear();
}
}
}
// Modified MongoCollectionSocket.ts
class MongoCollectionSocket {
private socket: Socket<ServerToClientEvents, ClientToServerEvents>;
constructor(collectionName: string) {
// Get the namespaced socket from the singleton manager
this.socket = SocketManager.getInstance().getCollectionSocket(collectionName);
this.socket.emit("collection::init", { collectionName });
console.log(`MongoDB collection:${collectionName} initialized.`);
}
// Rest of your class...
}Benefits:
- Single physical connection for all collections
- Centralized connection management
- App-level disconnect on shutdown rather than component-level
- No risk of hitting browser connection limits
This avoids the "terrible" approach of having to manage disconnection in every component that uses a collection.
🌐 Web query:
Do Socket.IO namespaces solve the connection disconnection management problem?
💡 Result:
Socket.IO namespaces help organize and separate different communication channels within a single Socket.IO connection. They provide a way to split the connection into multiple logical endpoints, each with its own event handlers and logic. However, namespaces alone do not fully solve connection and disconnection management problems.
Here's a more detailed explanation:
-
Purpose of Namespaces: Namespaces allow you to segment the connection space, which can be useful for separating concerns within your application (e.g., chat, notifications, gaming). Each namespace can have its own event listeners and middleware.
-
Connection Management: When a client connects, it connects to a specific namespace. Socket.IO manages the connection lifecycle for each namespace separately. This means you can handle events like
connect,disconnect, and reconnections within each namespace independently. -
Disconnection Management: Namespaces can help you handle disconnections more granularly because each namespace triggers its own
disconnectevent. However, namespaces don't inherently solve challenges like detecting real connection drops, handling reconnections, or managing resources based on connection state. You still need to implement proper event handlers and logic to manage these cases. -
Limitations: If a client disconnects from the underlying transport (e.g., network issues), all namespaces for that client are affected since they share the underlying connection. So, namespaces don't isolate the physical connection—they just provide logical segmentation over it.
Summary:
- Namespaces improve logical separation and event management per context.
- They provide a framework to handle connection/disconnection events per namespace.
- They do not independently solve all connection and disconnection management problems.
- You still need to write application-level code for robust connection handling, reconnection strategies, and resource cleanup.
If you're looking to manage connection and disconnection issues comprehensively, consider combining namespaces with other Socket.IO features like rooms, middleware, and custom reconnection logic.
@davemarco Yes, Socket.IO namespaces would solve the primary issue mentioned in your TODO comment - they allow multiple logical connections to share a single physical WebSocket connection.
From the TODO in your code:
"An simple change is to use Socket.IO namespace feature, and having Socket.IO multiplex the socket per collection namespace."
Here's how namespaces help:
- Single physical connection: Instead of one connection per collection, all collections use the same underlying connection
- Avoid browser limits: Browsers typically limit connections to 6-8 per domain, namespaces bypass this
- Simpler architecture: Centralized connection management instead of per-component disconnection
Basic implementation approach:
// Create a singleton socket manager
export class SocketManager {
private static instance: SocketManager;
private socket: Socket;
private constructor() {
this.socket = io(); // Single connection
}
public static getInstance(): SocketManager {
if (!SocketManager.instance) {
SocketManager.instance = new SocketManager();
}
return SocketManager.instance;
}
public getCollectionNamespace(collectionName: string): Socket {
return this.socket.io(`/${collectionName}`);
}
// Single disconnect point for application shutdown
public disconnect(): void {
this.socket.disconnect();
}
}This addresses both the connection limit issue and simplifies disconnection management by centralizing it.
Note that server-side changes would also be needed to properly handle these namespaces.
🧠 Learnings used
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.788Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
There was a problem hiding this comment.
I moved to shared socket which should fix the issue
There was a problem hiding this comment.
@davemarco Great! Using a shared socket singleton is the perfect solution for this issue. This implementation:
- Prevents creating multiple unnecessary connections
- Avoids hitting browser connection limits
- Centralizes connection management
- Eliminates the need for per-collection disconnection logic
The getSharedSocket() approach is ideal since it gives you all the benefits of connection pooling without the complexity of implementing namespaces. Nice work addressing this!
(＾◡＾)
🧠 Learnings used
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.788Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
| async subscribe (onDataUpdate: (data: object[]) => void): Promise<void> { | ||
| console.log("Attepting to subscribe to query:", this.#query); | ||
|
|
||
| this.#updateListener = (respArgs: {queryId: number; data: object[]}) => { | ||
| // Server sends updates for multiple queryIDs using the same event name. | ||
| if (this.#queryId === respArgs.queryId) { | ||
| onDataUpdate(respArgs.data); | ||
| } | ||
| }; | ||
|
|
||
| this.#socket.on("collection::find::update", this.#updateListener); | ||
|
|
||
| const response: Response<{queryId: number; initialDocuments: object[]}> = | ||
| await this.#socket.emitWithAck( | ||
| "collection::find::subscribe", | ||
| { | ||
| query: this.#query, | ||
| options: this.#options, | ||
| } | ||
| ); | ||
|
|
||
| if ("error" in response) { | ||
| throw new Error(`Subscription failed: ${response.error}`); | ||
| } | ||
|
|
||
| // Set the initial documents received from the server. | ||
| onDataUpdate(response.data.initialDocuments); | ||
|
|
||
| this.#queryId = response.data.queryId; | ||
| console.log(`Subscribed to queryID:${this.#queryId}.`); | ||
| } |
There was a problem hiding this comment.
Multiple subscribe() calls overwrite listener – guard against misuse
Nothing stops a consumer from calling subscribe twice on the same cursor; the second call overwrites #updateListener without deregistering the first, causing duplicate updates and memory bloat.
Add an early check:
+ if (this.#queryId !== null) {
+ throw new Error("Cursor already subscribed");
+ }and/or expose a subscribed flag.
📝 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.
| async subscribe (onDataUpdate: (data: object[]) => void): Promise<void> { | |
| console.log("Attepting to subscribe to query:", this.#query); | |
| this.#updateListener = (respArgs: {queryId: number; data: object[]}) => { | |
| // Server sends updates for multiple queryIDs using the same event name. | |
| if (this.#queryId === respArgs.queryId) { | |
| onDataUpdate(respArgs.data); | |
| } | |
| }; | |
| this.#socket.on("collection::find::update", this.#updateListener); | |
| const response: Response<{queryId: number; initialDocuments: object[]}> = | |
| await this.#socket.emitWithAck( | |
| "collection::find::subscribe", | |
| { | |
| query: this.#query, | |
| options: this.#options, | |
| } | |
| ); | |
| if ("error" in response) { | |
| throw new Error(`Subscription failed: ${response.error}`); | |
| } | |
| // Set the initial documents received from the server. | |
| onDataUpdate(response.data.initialDocuments); | |
| this.#queryId = response.data.queryId; | |
| console.log(`Subscribed to queryID:${this.#queryId}.`); | |
| } | |
| async subscribe(onDataUpdate: (data: object[]) => void): Promise<void> { | |
| // Guard against multiple subscriptions on the same cursor | |
| + if (this.#queryId !== null) { | |
| + throw new Error("Cursor already subscribed"); | |
| + } | |
| console.log("Attepting to subscribe to query:", this.#query); | |
| this.#updateListener = (respArgs: {queryId: number; data: object[]}) => { | |
| // Server sends updates for multiple queryIDs using the same event name. | |
| if (this.#queryId === respArgs.queryId) { | |
| onDataUpdate(respArgs.data); | |
| } | |
| }; | |
| this.#socket.on("collection::find::update", this.#updateListener); | |
| const response: Response<{queryId: number; initialDocuments: object[]}> = | |
| await this.#socket.emitWithAck( | |
| "collection::find::subscribe", | |
| { | |
| query: this.#query, | |
| options: this.#options, | |
| } | |
| ); | |
| if ("error" in response) { | |
| throw new Error(`Subscription failed: ${response.error}`); | |
| } | |
| // Set the initial documents received from the server. | |
| onDataUpdate(response.data.initialDocuments); | |
| this.#queryId = response.data.queryId; | |
| console.log(`Subscribed to queryID:${this.#queryId}.`); | |
| } |
There was a problem hiding this comment.
This is probably overkill since u would never call subscribe twice
There was a problem hiding this comment.
@davemarco I understand your perspective. If the application code ensures subscribe won't be called twice on the same cursor instance, then the check may indeed be unnecessary.
That said, defensive programming can help prevent subtle bugs if the component is ever reused or the calling code changes. Without the check, calling subscribe twice would leave a dangling event listener that could cause memory leaks.
But I'll defer to your judgment on this one since you know the usage patterns better!
| this.#fastify.log.info( | ||
| `Socket:${socket.id} requested query:${JSON.stringify(query)} ` + | ||
| `with options:${JSON.stringify(options)} to collection:${collectionName}` | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Log string building uses concatenation – template literal is enough
You already start with a template literal; concatenation is unnecessary and less readable.
- this.#fastify.log.info(
- `Socket:${socket.id} requested query:${JSON.stringify(query)} ` +
- `with options:${JSON.stringify(options)} to collection:${collectionName}`
- );
+ this.#fastify.log.info(
+ `Socket:${socket.id} requested query:${JSON.stringify(query)} ` +
+ `with options:${JSON.stringify(options)} to collection:${collectionName}`
+ );(If you keep multi-line, drop the +.)
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@davemarco The bot was suggesting to remove the + concatenation operator between the template literals. The diff visualization didn't show this change correctly, but the comment note "(If you keep multi-line, drop the +.)" clarifies the intention.
The recommendation is to change:
this.#fastify.log.info(
`Socket:${socket.id} requested query:${JSON.stringify(query)} ` +
`with options:${JSON.stringify(options)} to collection:${collectionName}`
);To a single multi-line template literal:
this.#fastify.log.info(
`Socket:${socket.id} requested query:${JSON.stringify(query)}
with options:${JSON.stringify(options)} to collection:${collectionName}`
);This avoids unnecessary string concatenation since template literals already support multi-line strings.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (3)
49-50: Typo: "Attepting" → "Attempting"- console.log("Attepting to subscribe to query:", this.#query); + console.log("Attempting to subscribe to query:", this.#query);
92-95: Unsubscribe emits without awaiting server ACKIf the server rejects the request (e.g., invalid
queryId), the client silently assumes success and clears local state, causing an irrecoverable desync.Consider using
emitWithAckand reverting local cleanup on failure:- unsubscribe (): void { + async unsubscribe (): Promise<void> { if (null === this.#queryId) { console.error("Attempted to unsubscribe, but no active subscription exists."); return; } - this.#socket.emit("collection::find::unsubscribe", { - queryId: this.#queryId, - }); + const response = await this.#socket.emitWithAck("collection::find::unsubscribe", { + queryId: this.#queryId, + }); + + if ("error" in response) { + console.error(`Unsubscribe failed: ${response.error}`); + return; + }
49-73: 🛠️ Refactor suggestionAdd protection against multiple subscribe calls on same cursor
Nothing prevents a consumer from calling
subscribetwice on the same cursor; the second call overwrites#updateListenerwithout deregistering the first, causing duplicate updates and memory bloat.Add an early check at the beginning of the
subscribemethod:async subscribe (onDataUpdate: (data: object[]) => void): Promise<void> { + if (this.#queryId !== null) { + throw new Error("Cursor already subscribed"); + } console.log("Attepting to subscribe to query:", this.#query);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.788Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
🧬 Code Graph Analysis (1)
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (1)
components/log-viewer-webui/common/index.ts (4)
ServerToClientEvents(80-80)ClientToServerEvents(76-76)QueryId(82-82)Response(79-79)
🔇 Additional comments (1)
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (1)
1-108:Details
❌ Incorrect review comment
Consider implementing support for Socket.IO namespaces
Based on the retrieved learning from davemarco, there's a plan to implement Socket.IO namespaces for connection pooling. The MongoCursorSocket class is a good place to prepare for this future enhancement.
This is worth tracking as a TODO for a future update, as implementing piecemeal changes to the connection architecture could lead to coordination issues between client and server.
🌐 Web query:
What are Socket.IO namespaces and how are they used for connection pooling?💡 Result:
Socket.IO namespaces are a way to create separate communication channels on the same physical connection between a client and server. Each namespace is identified by a unique name (typically a string starting with a slash, e.g.,
/chat,/news). They allow you to split the logic of your application over a single shared connection, effectively enabling multiple independent chat rooms or services to coexist.What are Socket.IO Namespaces?
- Separation of concerns: Namespaces allow you to divide your app into different endpoints, each handling its own events and logic.
- Single connection: Even though there are multiple namespaces, the client typically maintains a single underlying WebSocket connection, with multiplexed channels for each namespace.
- Event segregation: Each namespace has its own event handlers both on the client and server sides.
How Socket.IO Namespaces Work
When a client connects to a Socket.IO server, it connects to the default namespace
/by default. To connect to a different namespace, the client explicitly specifies it, for example:const socket = io('/chat');On the server side, you define namespace-specific event handling:
const chatNamespace = io.of('/chat'); chatNamespace.on('connection', (socket) => { console.log('User connected to chat namespace'); socket.on('message', (msg) => { chatNamespace.emit('message', msg); }); });Connection Pooling with Namespaces
- Multiplexing connections: Namespaces allow multiple independent logical connections over a single physical WebSocket connection. This avoids the overhead of establishing multiple connections.
- Efficient resource use: Instead of each service or channel requiring its own TCP connection, namespaces pool these onto one underlying connection, reducing latency and resource use.
- Example use cases: You might have
/chat,/news,/notificationsnamespaces, all using the same underlying connection but logically separated.Summary
Socket.IO namespaces provide a way to architect your real-time application with isolated communication channels over a single WebSocket or long-polling connection. This technique pools multiple logical connections efficiently into a single physical connection, thereby optimizing resource usage and simplifying client-server interaction.
If you'd like, I can provide a code example demonstrating how to implement namespaces for connection pooling in a Socket.IO app!
Ignore Socket.IO namespace support suggestion in MongoCursorSocket
This class already accepts a pre-configuredSocketinstance, so it’s namespace-agnostic by design. Namespace creation and connection pooling should be handled at the socket-initialization layer rather than within this cursor wrapper.Likely an incorrect or invalid review comment.
| /** | ||
| * @param socket | ||
| * @param query | ||
| * @param options | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance JSDoc documentation for constructor parameters
The constructor JSDoc has @PARAM tags but doesn't provide descriptions of what each parameter represents or their expected format. Adding more detailed parameter descriptions would improve code maintainability and developer understanding.
/**
* @param socket
* @param query
* @param options
*/
+/**
+ * @param socket - Socket.IO client instance for server communication
+ * @param query - MongoDB query object defining the filter criteria
+ * @param options - MongoDB query options (like sort, limit, etc.)
+ */Committable suggestion skipped: line range outside the PR's diff.
| this.#socket.off("collection::find::update", this.#updateListener); | ||
| this.#updateListener = null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a guard around unsubscribe listener removal
The current logic checks for this.#updateListener before removing it, which is good defensive programming. However, you might want to add more context in an error log if the listener is unexpectedly missing:
if (this.#updateListener) {
this.#socket.off("collection::find::update", this.#updateListener);
this.#updateListener = null;
+ } else {
+ console.warn(`No update listener found for queryID:${this.#queryId} during unsubscribe.`);
}📝 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.
| this.#socket.off("collection::find::update", this.#updateListener); | |
| this.#updateListener = null; | |
| } | |
| // --- components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts --- | |
| if (this.#updateListener) { | |
| this.#socket.off("collection::find::update", this.#updateListener); | |
| this.#updateListener = null; | |
| } else { | |
| console.warn( | |
| `No update listener found for queryID:${this.#queryId} during unsubscribe.` | |
| ); | |
| } | |
| // ------------------------------------------------------------------------ |
| /** | ||
| * A cursor-like object receiving MongoDB documents over a socket connection. | ||
| */ | ||
| class MongoCursorSocket { | ||
| #socket: Socket<ServerToClientEvents, ClientToServerEvents>; | ||
|
|
||
| #query: object; | ||
|
|
||
| #options: object; | ||
|
|
||
| #queryId: Nullable<QueryId> = null; | ||
|
|
||
|
|
||
| // Listener for data updates from the server. | ||
| #updateListener: Nullable<(respArgs: {queryId: number; data: object[]}) => void> = null; | ||
|
|
||
| /** | ||
| * @param socket | ||
| * @param query | ||
| * @param options | ||
| */ | ||
| constructor ( | ||
| socket: Socket<ServerToClientEvents, ClientToServerEvents>, | ||
| query: object, | ||
| options: object | ||
| ) { | ||
| this.#socket = socket; | ||
| this.#query = query; | ||
| this.#options = options; | ||
| } | ||
|
|
||
| /** | ||
| * Subscribes to query watcher for real-time updates. | ||
| * | ||
| * @param onDataUpdate Handler which sets data updates from the server in react ui component. | ||
| * @throws {Error} if subscription fails. | ||
| */ | ||
| async subscribe (onDataUpdate: (data: object[]) => void): Promise<void> { | ||
| console.log("Attepting to subscribe to query:", this.#query); | ||
|
|
||
| this.#updateListener = (respArgs: {queryId: number; data: object[]}) => { | ||
| // Server sends updates for multiple queryIDs using the same event name. | ||
| if (this.#queryId === respArgs.queryId) { | ||
| onDataUpdate(respArgs.data); | ||
| } | ||
| }; | ||
|
|
||
| this.#socket.on("collection::find::update", this.#updateListener); | ||
|
|
||
| const response: Response<{queryId: number; initialDocuments: object[]}> = | ||
| await this.#socket.emitWithAck( | ||
| "collection::find::subscribe", | ||
| { | ||
| query: this.#query, | ||
| options: this.#options, | ||
| } | ||
| ); | ||
|
|
||
| if ("error" in response) { | ||
| this.#socket.off("collection::find::update", this.#updateListener); | ||
| throw new Error(`Subscription failed: ${response.error}`); | ||
| } | ||
|
|
||
| // Set the initial documents received from the server. | ||
| onDataUpdate(response.data.initialDocuments); | ||
|
|
||
| this.#queryId = response.data.queryId; | ||
| console.log(`Subscribed to queryID:${this.#queryId}.`); | ||
| } | ||
|
|
||
| /** | ||
| * Unsubscribe from the query. | ||
| */ | ||
| unsubscribe (): void { | ||
| if (null === this.#queryId) { | ||
| console.error("Attempted to unsubscribe, but no active subscription exists."); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
|
|
||
| this.#socket.emit("collection::find::unsubscribe", { | ||
| queryId: this.#queryId, | ||
| }); | ||
|
|
||
| if (this.#updateListener) { | ||
| this.#socket.off("collection::find::update", this.#updateListener); | ||
| this.#updateListener = null; | ||
| } | ||
|
|
||
| console.log(`Unsubscribed to queryID:${this.#queryId}.`); | ||
|
|
||
| this.#queryId = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implement a isSubscribed() method for state checking
To help consumers check subscription state without accessing private fields, consider adding a simple method:
+ /**
+ * Returns whether this cursor is currently subscribed.
+ */
+ isSubscribed(): boolean {
+ return this.#queryId !== null;
+ }This would allow consumers to safely check subscription state before calling subscribe/unsubscribe.
📝 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.
| /** | |
| * A cursor-like object receiving MongoDB documents over a socket connection. | |
| */ | |
| class MongoCursorSocket { | |
| #socket: Socket<ServerToClientEvents, ClientToServerEvents>; | |
| #query: object; | |
| #options: object; | |
| #queryId: Nullable<QueryId> = null; | |
| // Listener for data updates from the server. | |
| #updateListener: Nullable<(respArgs: {queryId: number; data: object[]}) => void> = null; | |
| /** | |
| * @param socket | |
| * @param query | |
| * @param options | |
| */ | |
| constructor ( | |
| socket: Socket<ServerToClientEvents, ClientToServerEvents>, | |
| query: object, | |
| options: object | |
| ) { | |
| this.#socket = socket; | |
| this.#query = query; | |
| this.#options = options; | |
| } | |
| /** | |
| * Subscribes to query watcher for real-time updates. | |
| * | |
| * @param onDataUpdate Handler which sets data updates from the server in react ui component. | |
| * @throws {Error} if subscription fails. | |
| */ | |
| async subscribe (onDataUpdate: (data: object[]) => void): Promise<void> { | |
| console.log("Attepting to subscribe to query:", this.#query); | |
| this.#updateListener = (respArgs: {queryId: number; data: object[]}) => { | |
| // Server sends updates for multiple queryIDs using the same event name. | |
| if (this.#queryId === respArgs.queryId) { | |
| onDataUpdate(respArgs.data); | |
| } | |
| }; | |
| this.#socket.on("collection::find::update", this.#updateListener); | |
| const response: Response<{queryId: number; initialDocuments: object[]}> = | |
| await this.#socket.emitWithAck( | |
| "collection::find::subscribe", | |
| { | |
| query: this.#query, | |
| options: this.#options, | |
| } | |
| ); | |
| if ("error" in response) { | |
| this.#socket.off("collection::find::update", this.#updateListener); | |
| throw new Error(`Subscription failed: ${response.error}`); | |
| } | |
| // Set the initial documents received from the server. | |
| onDataUpdate(response.data.initialDocuments); | |
| this.#queryId = response.data.queryId; | |
| console.log(`Subscribed to queryID:${this.#queryId}.`); | |
| } | |
| /** | |
| * Unsubscribe from the query. | |
| */ | |
| unsubscribe (): void { | |
| if (null === this.#queryId) { | |
| console.error("Attempted to unsubscribe, but no active subscription exists."); | |
| return; | |
| } | |
| this.#socket.emit("collection::find::unsubscribe", { | |
| queryId: this.#queryId, | |
| }); | |
| if (this.#updateListener) { | |
| this.#socket.off("collection::find::update", this.#updateListener); | |
| this.#updateListener = null; | |
| } | |
| console.log(`Unsubscribed to queryID:${this.#queryId}.`); | |
| this.#queryId = null; | |
| } | |
| } | |
| /** | |
| * A cursor-like object receiving MongoDB documents over a socket connection. | |
| */ | |
| class MongoCursorSocket { | |
| #socket: Socket<ServerToClientEvents, ClientToServerEvents>; | |
| #query: object; | |
| #options: object; | |
| #queryId: Nullable<QueryId> = null; | |
| // Listener for data updates from the server. | |
| #updateListener: Nullable<(respArgs: {queryId: number; data: object[]}) => void> = null; | |
| /** | |
| * @param socket | |
| * @param query | |
| * @param options | |
| */ | |
| constructor ( | |
| socket: Socket<ServerToClientEvents, ClientToServerEvents>, | |
| query: object, | |
| options: object | |
| ) { | |
| this.#socket = socket; | |
| this.#query = query; | |
| this.#options = options; | |
| } | |
| /** | |
| * Subscribes to query watcher for real-time updates. | |
| * | |
| * @param onDataUpdate Handler which sets data updates from the server in react ui component. | |
| * @throws {Error} if subscription fails. | |
| */ | |
| async subscribe (onDataUpdate: (data: object[]) => void): Promise<void> { | |
| console.log("Attepting to subscribe to query:", this.#query); | |
| this.#updateListener = (respArgs: {queryId: number; data: object[]}) => { | |
| // Server sends updates for multiple queryIDs using the same event name. | |
| if (this.#queryId === respArgs.queryId) { | |
| onDataUpdate(respArgs.data); | |
| } | |
| }; | |
| this.#socket.on("collection::find::update", this.#updateListener); | |
| const response: Response<{queryId: number; initialDocuments: object[]}> = | |
| await this.#socket.emitWithAck( | |
| "collection::find::subscribe", | |
| { | |
| query: this.#query, | |
| options: this.#options, | |
| } | |
| ); | |
| if ("error" in response) { | |
| this.#socket.off("collection::find::update", this.#updateListener); | |
| throw new Error(`Subscription failed: ${response.error}`); | |
| } | |
| // Set the initial documents received from the server. | |
| onDataUpdate(response.data.initialDocuments); | |
| this.#queryId = response.data.queryId; | |
| console.log(`Subscribed to queryID:${this.#queryId}.`); | |
| } | |
| /** | |
| * Unsubscribe from the query. | |
| */ | |
| unsubscribe (): void { | |
| if (null === this.#queryId) { | |
| console.error("Attempted to unsubscribe, but no active subscription exists."); | |
| return; | |
| } | |
| this.#socket.emit("collection::find::unsubscribe", { | |
| queryId: this.#queryId, | |
| }); | |
| if (this.#updateListener) { | |
| this.#socket.off("collection::find::update", this.#updateListener); | |
| this.#updateListener = null; | |
| } | |
| console.log(`Unsubscribed to queryID:${this.#queryId}.`); | |
| this.#queryId = null; | |
| } | |
| /** | |
| * Returns whether this cursor is currently subscribed. | |
| */ | |
| isSubscribed(): boolean { | |
| return this.#queryId !== null; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (2)
50-58: Guard against double-subscription to prevent dangling listeners.Creating a new
#updateListenerwithout first checkingthis.#queryIdenables this cursor to be subscribed twice, leaving the original listener orphaned and leaking memory. A one-liner protects against that:+ if (this.#queryId !== null) { + throw new Error("Cursor already subscribed; call unsubscribe() first."); + }
106-106: Expose anisSubscribed()helper for consumer ergonomics.A trivial predicate improves discoverability and avoids consumers poking private state:
} + + /** + * Returns `true` while the cursor is actively subscribed. + */ + isSubscribed(): boolean { + return this.#queryId !== null; + }components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (2)
257-260: Avoid string concatenation inside template literal.Since a template literal is already used, the
+operators are redundant and hinder readability.- this.#fastify.log.info( - `Socket:${socket.id} requested query:${JSON.stringify(query)} ` + - `with options:${JSON.stringify(options)} to collection:${collectionName}` - ); + this.#fastify.log.info( + `Socket:${socket.id} requested query:${JSON.stringify(query)} ` + + `with options:${JSON.stringify(options)} to collection:${collectionName}` + );
168-170: 🧹 Nitpick (assertive)Spelling: “aysnc/await” → “async/await”.
- // init and subscription events should be serialised by Socket.IO. - // ... - // With no aysnc/await, a race is unlikely since the + // init and subscription events should be serialised by Socket.IO. + // ... + // With no async/await, a race is unlikely since the
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
| /* eslint-disable no-warning-comments */ | ||
| // TODO: The init socket event could race with the subscription event (i.e. the | ||
| // subscription event could run concurrently with the init event) leading to errors due | ||
| // to an uninitialized collection. Consider removing this event entirely and using the | ||
| // subscription event to initialize the collection. If this event remains, do not run any | ||
| // async/await code in this function. With no aysnc/await, a race is unlikely since the | ||
| // init and subscription events should be serialised by Socket.IO. | ||
| socket.data = {...socket.data, collectionName}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function is declared async but must stay synchronous to avoid race; drop async.
The comment explicitly warns against async/await, yet the listener is still marked async, introducing an unnecessary micro-task delay and potential race condition. Removing the async keyword prevents accidental awaits from being added later.
- async #collectionInitListener (
+ #collectionInitListener (
socket: MongoCustomSocket,
requestArgs: {collectionName: string},
- ): Promise<void> {
+ ): void {📝 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.
| /* eslint-disable no-warning-comments */ | |
| // TODO: The init socket event could race with the subscription event (i.e. the | |
| // subscription event could run concurrently with the init event) leading to errors due | |
| // to an uninitialized collection. Consider removing this event entirely and using the | |
| // subscription event to initialize the collection. If this event remains, do not run any | |
| // async/await code in this function. With no aysnc/await, a race is unlikely since the | |
| // init and subscription events should be serialised by Socket.IO. | |
| socket.data = {...socket.data, collectionName}; | |
| #collectionInitListener ( | |
| socket: MongoCustomSocket, | |
| requestArgs: {collectionName: string}, | |
| ): void { | |
| /* eslint-disable no-warning-comments */ | |
| // TODO: The init socket event could race with the subscription event (i.e. the | |
| // subscription event could run concurrently with the init event) leading to errors due | |
| // to an uninitialized collection. Consider removing this event entirely and using the | |
| // subscription event to initialize the collection. If this event remains, do not run any | |
| // async/await code in this function. With no aysnc/await, a race is unlikely since the | |
| // init and subscription events should be serialised by Socket.IO. | |
| socket.data = {...socket.data, collectionName}; | |
| } |
|
@junhaoliao coderabbit brought up something interesting here . Basically the problem is that every time the query changes, say user makes a new search query. The collection will change opening up a new socket. This will potentially make us run out of sockets much faster. Moreover, there is no simple way to disconnect the collection in the hook I propose going to the socket singleton approach discussed earlier where there is only one socket. |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
components/log-viewer-webui/common/index.ts (2)
32-46: 🧹 Nitpick (assertive)Consider discriminating
Response<T>for safer narrowingRight now
Response<T>is a union ofErr|Success<T>with overlapping keys, so consumers must use"error" in reschecks all over the codebase.
A discriminated union with a literal tag (status: "ok" | "err") improves readability and compiler exhaustiveness checks.type Response<T> = | { status: "ok"; data: T } | { status: "err"; error: string; queryId?: QueryId };This would simplify handlers:
if (response.status === "err") { … }
51-58: 🧹 Nitpick (assertive)Event-per-query TODO — add severity or tracking reference
The comment hints at changing
"collection::find::update"to a query-specific channel. Given that the client already filters byqueryId, this is mostly a performance/memory optimisation. Consider turning the TODO into a GitHub issue with context & proposed approach; inline TODOs tend to be forgotten.
♻️ Duplicate comments (1)
components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (1)
55-55:JSON.stringifycan throw on circular structuresThis was raised previously and the code remains unchanged. The risk is low in typical queries, but a defensive wrapper prevents an unhandled exception that breaks subscription flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts(1 hunks)components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts(1 hunks)components/log-viewer-webui/common/index.ts(3 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.tscomponents/log-viewer-webui/common/index.tscomponents/log-viewer-webui/client/src/api/socket/MongoCursorSocket.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.788Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts (3)
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.788Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:25-43
Timestamp: 2025-05-09T18:06:33.030Z
Learning: Socket.IO connections in JavaScript require explicit disconnection by calling socket.disconnect() and are not automatically cleaned up when the object is garbage collected.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:25-43
Timestamp: 2025-05-09T18:21:05.824Z
Learning: Socket.IO has a built-in ping/pong mechanism that detects connection failures, but it doesn't automatically disconnect truly inactive but connected clients. By default, it sends pings every 25 seconds and expects pongs within 20 seconds, but this is for connection health monitoring, not application inactivity detection.
🧬 Code Graph Analysis (1)
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts (3)
components/log-viewer-webui/common/index.ts (2)
ServerToClientEvents(79-79)ClientToServerEvents(75-75)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (2)
collectionName(143-146)collectionName(198-210)components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (1)
MongoCursorSocket(114-114)
🔇 Additional comments (1)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
192-210: 🧹 Nitpick (assertive)
#getOrCreateWatcherCollectioncan race under heavy concurrencyIf two sockets subscribe to the same brand-new collection at nearly the same time, both could evaluate
undefinedand create twoMongoWatcherCollectioninstances before one is cached. Although rare, it wastes memory and may duplicate change-stream watchers.Guard with a
Mapdouble-check or useMap.prototype.get’s return as authoritative afterset:if (false === this.#collections.has(collectionName)) { this.#collections.set( collectionName, new MongoWatcherCollection(collectionName, this.#mongoDb) ); } return this.#collections.get(collectionName)!;Likely an incorrect or invalid review comment.
| constructor (collectionName: string) { | ||
| this.socket = getSharedSocket(); | ||
| this.collectionName = collectionName; | ||
| console.log(`MongoDB collection:${collectionName} initialized.`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose a deterministic teardown path for the collection-level socket
MongoCollectionSocket keeps only a reference returned by getSharedSocket().
Call-sites currently have no way to disconnect or remove listeners when the component tree unmounts (e.g., during hot-reload). Even though the underlying connection is shared, it is still good practice to provide a dispose() / disconnect() that simply delegates to the shared singleton. This allows React cleanup hooks (or unit tests) to release resources explicitly and avoids lingering listeners that keep collections in memory.
Example:
class MongoCollectionSocket {
…
+ /**
+ * Releases any listeners added by this instance.
+ * No-op when the shared socket is already closed.
+ */
+ public disconnect(): void {
+ if (this.socket.connected) {
+ // Only detach collection-specific listeners here
+ // (do not close the underlying shared socket).
+ this.socket.removeAllListeners("collection::find::update");
+ }
+ }
}📝 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.
| constructor (collectionName: string) { | |
| this.socket = getSharedSocket(); | |
| this.collectionName = collectionName; | |
| console.log(`MongoDB collection:${collectionName} initialized.`); | |
| } | |
| // components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts | |
| export class MongoCollectionSocket { | |
| constructor (collectionName: string) { | |
| this.socket = getSharedSocket(); | |
| this.collectionName = collectionName; | |
| console.log(`MongoDB collection:${collectionName} initialized.`); | |
| } | |
| /** | |
| * Releases any listeners added by this instance. | |
| * No-op when the shared socket is already closed. | |
| */ | |
| public disconnect(): void { | |
| if (this.socket.connected) { | |
| // Only detach collection-specific listeners here | |
| // (do not close the underlying shared socket). | |
| this.socket.removeAllListeners("collection::find::update"); | |
| } | |
| } | |
| // …other methods… | |
| } |
There was a problem hiding this comment.
This is only one socket. So the only way to get another socket should disconnect the previous socket. I think it should be okay
There was a problem hiding this comment.
@davemarco I understand your point about using a socket singleton pattern. Since the socket is shared and managed centrally, and since the unsubscription is handled in the useCursor hook, you're right that an explicit disconnect in the MongoCollectionSocket class may not be strictly necessary for the current implementation.
Given that the class isn't directly attaching listeners (those appear to be in the MongoCursorSocket class instead), your current approach should work fine as long as the hook's cleanup is thorough.
✏️ Learnings added
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.158Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| find (query: object, options: object) { | ||
| return new MongoCursorSocket( | ||
| this.socket, | ||
| this.collectionName, | ||
| query, | ||
| options, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Strengthen typing & return signature of find()
query and options are declared as plain object, which forfeits type-safety and IDE autocompletion.
Consider parameterising the class (or at least the method) with generics, or constrain them to Record<string, unknown>.
In addition, the method relies on type inference for its return value. Being explicit helps consumers and documentation tools:
- find (query: object, options: object) {
- return new MongoCursorSocket(
+ find<Q extends Record<string, unknown>, O extends Record<string, unknown>>(
+ query: Q,
+ options: O
+ ): MongoCursorSocket {
+ return new MongoCursorSocket(
this.socket,
this.collectionName,
query,
options,
);
}📝 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.
| find (query: object, options: object) { | |
| return new MongoCursorSocket( | |
| this.socket, | |
| this.collectionName, | |
| query, | |
| options, | |
| ); | |
| } | |
| find<Q extends Record<string, unknown>, O extends Record<string, unknown>>( | |
| query: Q, | |
| options: O | |
| ): MongoCursorSocket { | |
| return new MongoCursorSocket( | |
| this.socket, | |
| this.collectionName, | |
| query, | |
| options, | |
| ); | |
| } |
| #query: object; | ||
|
|
||
| #options: object; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Loose object types reduce safety
Both #query and #options are typed as bare object. This accepts primitives such as new Date() or arrays and blocks introspection in IDEs. Prefer Record<string, unknown> or generics as suggested for MongoCollectionSocket.
| const hasCollection = await this.#hasCollection(collectionName); | ||
| if (false === hasCollection) { | ||
| this.#fastify.log.error(`Collection ${collectionName} does not exist in MongoDB`); | ||
| callback({ | ||
| error: `Collection ${collectionName} does not exist in MongoDB on server`, | ||
| }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const watcherCollection = this.#getOrCreateWatcherCollection(collectionName); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Per-subscription listCollections() impacts latency
#hasCollection() performs an extra round-trip to MongoDB for every subscribe request. Under heavy churn (e.g., dashboards with polling queries) this becomes a hotspot.
Since you already cache MongoWatcherCollection instances, you can cheaply infer existence:
if (!this.#collections.has(collectionName) &&
false === await this.#hasCollection(collectionName)) {
…
}Alternatively memoise the positive/negative result with a TTL.
|
@junhaoliao - okay I made the socket singleton change, to fix the runaway socket problem. Note the singleton was an issue before since the collection name was not part of the hash. But now it is, so should not be an issue. |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/client/src/api/socket/SocketSingleton.ts(1 hunks)components/log-viewer-webui/common/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/api/socket/SocketSingleton.tscomponents/log-viewer-webui/common/index.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.788Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
🪛 GitHub Actions: clp-lint
components/log-viewer-webui/client/src/api/socket/SocketSingleton.ts
[error] 1-1: Imports must be broken into multiple lines if there are more than 1 elements (import-newlines/enforce)
[error] 1-1: Expected 2 empty lines after import statement not followed by another import (import/newline-after-import)
[error] 1-1: Expected a line break after this opening brace (@stylistic/object-curly-newline)
[error] 1-1: There should be no space after '{' (@stylistic/object-curly-spacing)
[error] 1-1: There should be no space before '}' (@stylistic/object-curly-spacing)
[error] 1-1: Expected a line break before this closing brace (@stylistic/object-curly-newline)
[warning] 6-6: Expected a function expression (func-style)
[warning] 6-6: Missing JSDoc comment (jsdoc/require-jsdoc)
[error] 6-6: Missing space before function parentheses (@stylistic/space-before-function-paren)
[warning] 14-14: Expected blank line before this statement (@stylistic/padding-line-between-statements)
[error] 15-15: Newline required at end of file but not found (@stylistic/eol-last)
🔇 Additional comments (4)
components/log-viewer-webui/client/src/api/socket/SocketSingleton.ts (2)
7-7:⚠️ Potential issueUse explicit comparison instead of negation operator
According to the coding guidelines, prefer
false == <expression>rather than!<expression>.- if (!sharedSocket) { + if (null == sharedSocket) {Likely an incorrect or invalid review comment.
1-1:⚠️ Potential issueFix import statement to follow linting guidelines
The import statement needs to be broken into multiple lines since there are multiple elements being imported. Also ensure proper spacing around the curly braces.
-import { io, Socket } from "socket.io-client"; +import { + io, + Socket +} from "socket.io-client";Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 1-1: Imports must be broken into multiple lines if there are more than 1 elements (import-newlines/enforce)
[error] 1-1: Expected 2 empty lines after import statement not followed by another import (import/newline-after-import)
[error] 1-1: Expected a line break after this opening brace (@stylistic/object-curly-newline)
[error] 1-1: There should be no space after '{' (@stylistic/object-curly-spacing)
[error] 1-1: There should be no space before '}' (@stylistic/object-curly-spacing)
[error] 1-1: Expected a line break before this closing brace (@stylistic/object-curly-newline)
components/log-viewer-webui/common/index.ts (2)
36-36: LGTM: Explicit collectionName parameter addedAdding
collectionNameexplicitly to the request arguments is a good improvement for type safety and clarity.
74-82: LGTM: Type-only exports improve modularityChanging to type-only exports is a good practice that better separates types from runtime code, supporting improved type-only imports consistent with your TypeScript configuration updates.
| import { io, Socket } from "socket.io-client"; | ||
|
|
||
| let sharedSocket: Socket | null = null; |
There was a problem hiding this comment.
Add proper line spacing after imports
According to the linting errors, there should be 2 empty lines after the import statement.
import { io, Socket } from "socket.io-client";
+
let sharedSocket: Socket | null = null;📝 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.
| import { io, Socket } from "socket.io-client"; | |
| let sharedSocket: Socket | null = null; | |
| import { io, Socket } from "socket.io-client"; | |
| let sharedSocket: Socket | null = null; |
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 1-1: Imports must be broken into multiple lines if there are more than 1 elements (import-newlines/enforce)
[error] 1-1: Expected 2 empty lines after import statement not followed by another import (import/newline-after-import)
[error] 1-1: Expected a line break after this opening brace (@stylistic/object-curly-newline)
[error] 1-1: There should be no space after '{' (@stylistic/object-curly-spacing)
[error] 1-1: There should be no space before '}' (@stylistic/object-curly-spacing)
[error] 1-1: Expected a line break before this closing brace (@stylistic/object-curly-newline)
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: Consider replacing this with `collection::find::update${number}`, which will | ||
| // limit callbacks being triggered in the client to their respective query IDs. | ||
| "collection::find::update": (respArgs: { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider prioritizing the TODO for query-specific updates
The TODO comment suggests a valuable optimization that would reduce unnecessary client-side processing by limiting callback triggers to specific query IDs. This aligns with the PR objective of preventing potential bugs with the query service.
Based on the PR objectives and the retrieved learning about coordinated client-server changes, consider prioritizing this TODO item in your implementation roadmap. This would help address the concern mentioned in the PR comments about managing socket connections efficiently.
| return collections.some((collection) => collection.name === collectionName); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
fyi i removed this since it could race with subscribe event. Anyways it is probably simpler either way without it.
davemarco
changed the title
davemarco
changed the title
| "useDefineForClassFields": true, | ||
| "baseUrl": "../", | ||
| "paths": { | ||
| "@common/*": ["../common/*"], // Map imports from "@common/*" to the shared folder |
There was a problem hiding this comment.
@hoophalab
as a note, we will revisit this as we implement #822
| ws: true, | ||
| }, | ||
| }, | ||
| fs: { |
There was a problem hiding this comment.
@hoophalab
as a note, we will revisit this as we implement #822
| this.#updateListener = null; | ||
| } | ||
|
|
||
| console.log(`Unsubscribed to queryID:${this.#queryId}.`); |
There was a problem hiding this comment.
| console.log(`Unsubscribed to queryID:${this.#queryId}.`); | |
| console.log(`Unsubscribed to queryID=${this.#queryId}.`); |
There was a problem hiding this comment.
I'm using the other syntax in a lot of places already. Not that I disagree with this syntax, but im using the : in many other places
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
lgtm
i proposed some stylish changes
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
components/log-viewer-webui/client/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.814Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
🔇 Additional comments (1)
components/log-viewer-webui/client/package.json (1)
27-27: Verify socket.io-client version compatibility with server.
Ensure the client version^4.8.1aligns with your Socket.IO server version to avoid runtime mismatches. Also confirm that TypeScript definitions are available—if not, consider adding@types/socket.io-client.
| "react-dom": "^19.0.0", | ||
| "react-router": "^7.4.1", | ||
| "socket.io-client": "^4.8.1", | ||
| "vite-tsconfig-paths": "^5.1.4", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Relocate vite-tsconfig-paths to devDependencies.
The vite-tsconfig-paths plugin is only needed at build time and for local development. Moving it to devDependencies will reduce production install size and clarify its usage scope.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/log-viewer-webui/client/package.json (1)
29-29: Relocatevite-tsconfig-pathsto devDependencies.
As discussed, this plugin is only needed during build and development. Moving it todevDependencieswill reduce production install size and clarify its scope.
Suggest:"dependencies": { - "vite-tsconfig-paths": "^5.1.4", }, "devDependencies": { + "vite-tsconfig-paths": "^5.1.4", }components/log-viewer-webui/client/src/api/socket/SocketSingleton.ts (1)
27-27: 🛠️ Refactor suggestionAdd error and disconnect handlers for socket resilience.
Register handlers immediately after initializing the socket to improve observability and retry logic:sharedSocket = io(); +sharedSocket.on("connect_error", (err) => { + console.error("Socket connection error:", err); +}); +sharedSocket.on("disconnect", (reason) => { + console.warn("Socket disconnected:", reason); +});This will help surface connection issues and guide reconnection strategies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
components/log-viewer-webui/client/package.json(1 hunks)components/log-viewer-webui/client/src/api/socket/SocketSingleton.ts(1 hunks)components/log-viewer-webui/client/vite.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/vite.config.tscomponents/log-viewer-webui/client/src/api/socket/SocketSingleton.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.814Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
🧬 Code Graph Analysis (1)
components/log-viewer-webui/client/src/api/socket/SocketSingleton.ts (1)
components/log-viewer-webui/common/index.ts (2)
ServerToClientEvents(79-79)ClientToServerEvents(75-75)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (5)
components/log-viewer-webui/client/package.json (1)
28-28: Addsocket.io-clientdependency for real-time updates.
Thesocket.io-clientversion^4.8.1aligns with the server setup and is required for the singleton and cursor modules.components/log-viewer-webui/client/vite.config.ts (3)
3-3: Importvite-tsconfig-pathsplugin for alias resolution.
IncludingtsconfigPaths()enables TypeScript path alias support in Vite as configured intsconfig.app.json.
9-12: ApplytsconfigPaths()in plugin list.
Ensures that your@common/*imports are correctly resolved at build and runtime.
23-27: Configure WebSocket proxy for Socket.IO.
The/socket.io/proxy withws: trueis essential for supporting real-time communication between client and server. Looks correct.components/log-viewer-webui/client/src/api/socket/SocketSingleton.ts (1)
1-4: Verify path alias and extension in import.
The import from@common/index.jsuses a.jsextension. Confirm that your bundler and TS configuration correctly resolve this alias to the TypeScript source (.ts), or adjust the alias mapping to omit the extension.
| fs: { | ||
| // allow serving files from one level up (common folder) | ||
| allow: [".."], | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review fs.allow setting for static serving.
Allowing the parent directory ("..") can unintentionally expose files outside the client bundle. Consider restricting this to only the necessary common subfolder or adding explicit path filters to avoid serving sensitive files.
| if (!sharedSocket) { | ||
| // eslint-disable-next-line no-warning-comments |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer explicit null check over negation.
Instead of if (!sharedSocket), use an explicit null comparison for clarity and to avoid negating a potentially complex expression:
-if (!sharedSocket) {
+if (sharedSocket === null) {📝 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 (!sharedSocket) { | |
| // eslint-disable-next-line no-warning-comments | |
| if (sharedSocket === null) { | |
| // eslint-disable-next-line no-warning-comments |
| * query the collection. | ||
| */ | ||
| class MongoCollectionSocket { | ||
| collectionName: string; |
There was a problem hiding this comment.
sorry for missing this in the last batch of comments
does it need to be public? if not, let's change it to private
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(new-webui): Add client library for real-time MongoDB updates.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts (1)
39-46: 🧹 Nitpick (assertive)Improve typing on
findmethod
Thefindsignature currently uses untypedobjectparameters and relies on inferred return types. For better type safety and IDE support, you could:
- Parameterise
queryandoptionswith generics (e.g.Q extends Record<string, unknown>,O extends Record<string, unknown>).- Explicitly declare the return type (e.g.
: MongoCursorSocket<Q, O>if the cursor class is generic).Example:
-find (query: object, options: object) { +find<Q extends Record<string, unknown>, O extends Record<string, unknown>>( + query: Q, + options: O +): MongoCursorSocket<Q, O> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.814Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts (4)
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.814Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:25-43
Timestamp: 2025-05-09T18:06:33.042Z
Learning: Socket.IO connections in JavaScript require explicit disconnection by calling socket.disconnect() and are not automatically cleaned up when the object is garbage collected.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:25-43
Timestamp: 2025-05-09T18:21:05.843Z
Learning: Socket.IO has a built-in ping/pong mechanism that detects connection failures, but it doesn't automatically disconnect truly inactive but connected clients. By default, it sends pings every 25 seconds and expects pongs within 20 seconds, but this is for connection health monitoring, not application inactivity detection.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
🧬 Code Graph Analysis (1)
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts (4)
components/log-viewer-webui/common/index.ts (2)
ServerToClientEvents(79-79)ClientToServerEvents(75-75)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (2)
collectionName(143-146)collectionName(198-210)components/log-viewer-webui/client/src/api/socket/SocketSingleton.ts (1)
getSharedSocket(33-33)components/log-viewer-webui/client/src/api/socket/MongoCursorSocket.ts (1)
MongoCursorSocket(114-114)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (1)
components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts (1)
16-18: Verify ES private-field support
You’re using the#privatefield syntax in TypeScript. Ensure that yourtsconfig.jsontarget (or build toolchain) supports ES2022 private class fields; otherwise, this may result in a compilation error.
| constructor (collectionName: string) { | ||
| this.#socket = getSharedSocket(); | ||
| this.#collectionName = collectionName; | ||
| console.log(`MongoDB collection:${collectionName} initialized.`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replace console.log with proper logger
Using console.log for production code can make debugging and log management harder. Consider using a structured logging utility or debug namespace (e.g. debug, winston, or a custom logger) to control log levels and output channels.
| ClientToServerEvents, | ||
| ServerToClientEvents, | ||
| } from "@common/index.js"; | ||
| import {Socket} from "socket.io-client"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer type-only import for Socket
Consider using a type-only import for the Socket interface to avoid pulling in the module at runtime if it’s only used for type annotations:
-import {Socket} from "socket.io-client";
+import type {Socket} from "socket.io-client";📝 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.
| import {Socket} from "socket.io-client"; | |
| -import {Socket} from "socket.io-client"; | |
| +import type {Socket} from "socket.io-client"; |
| /** | ||
| * Initalizes socket connection to a MongoDB collection on the server. | ||
| * | ||
| * @param collectionName | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix typo and clarify JSDoc in constructor
There's a typo in the JSDoc (Initalizes → Initializes) and the @param lacks a description. Consider updating to:
- * Initalizes socket connection to a MongoDB collection on the server.
+ * Initializes the shared socket connection for a MongoDB collection on the server.
+ *
+ * @param collectionName - The name of the MongoDB collection to subscribe to.📝 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.
| /** | |
| * Initalizes socket connection to a MongoDB collection on the server. | |
| * | |
| * @param collectionName | |
| */ | |
| /** | |
| * Initializes the shared socket connection for a MongoDB collection on the server. | |
| * | |
| * @param collectionName - The name of the MongoDB collection to subscribe to. | |
| */ |
2 participants
Description
This PR is a refactor of #841
PR adds a custom hook to front end which connects to new fastify mongoDB query service #880.
The custom hook (useCursor) should requery, when any of the dependencies have changed. It is meant to be a drop-in replacement for meteor useTracker.
There are some minor server changes as well. Server changes are to prevent, rare, but potential bugs.
Checklist
breaking change.
Validation performed
Replaced app with a new testing component (code below). New component can modify queries with buttons,
testing how it would actually work in new webui (say a different query by user). Subscription and unsubscription worked as expected.
Summary by CodeRabbit