fix(webui): Proxy socket.io requests when localhost resolves to multiple addresses (fixes #1062). #1078
Conversation
WalkthroughThe changes update the webui-server's dependencies by adding the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FastifyServer
participant DNS
participant MongoSocketIoServer
User->>FastifyServer: Sends request to /socket.io/
FastifyServer->>MongoSocketIoServer: Handles initialization
MongoSocketIoServer->>DNS: Performs lookup for configured host (all addresses)
DNS-->>MongoSocketIoServer: Returns list of IP addresses
alt Multiple addresses found
MongoSocketIoServer->>FastifyServer: Registers HTTP proxy for /socket.io/
User->>FastifyServer: Request on secondary address
FastifyServer->>FastifyServer: Proxies request to main server address
else Single address found
User->>FastifyServer: Request handled normally
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🧠 Learnings (2)📓 Common learningscomponents/webui/server/src/plugins/MongoSocketIoServer/index.ts (6)🪛 Biome (1.9.4)components/webui/server/src/plugins/MongoSocketIoServer/index.ts[error] 107-109: Template literals are preferred over string concatenation. Unsafe fix: Use a template literal. (lint/style/useTemplate) ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/webui/server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
components/webui/server/package.json(1 hunks)components/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>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
🧠 Learnings (3)
📓 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.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: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
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.
components/webui/server/src/plugins/MongoSocketIoServer/index.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: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: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
components/webui/server/package.json (2)
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
🪛 Biome (1.9.4)
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
[error] 102-104: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (2)
components/webui/server/package.json (1)
28-28: LGTM! Dependency addition looks appropriate.The
@fastify/http-proxydependency is correctly added to support the HTTP proxy functionality for socket.io requests. The version constraint^11.3.0follows semantic versioning best practices.components/webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
7-9: LGTM! Clean import additions.The imports for DNS lookup and HTTP proxy functionality are correctly added and follow the existing import structure.
|
|
||
| // Fastify listens on all resolved addresses for localhost (e.g. `127.0.0.1`, `::1`), but | ||
| // socket.io only uses the main one. When multiple addresses are resolved, we proxy | ||
| // socket.io requests from secondary addresses to the main server. | ||
| dnsLookup(fastify.config.HOST, {all: true}, (err, addresses) => { | ||
| if (null !== err) { | ||
| fastify.log.error(`DNS lookup failed: ${err.message}`); | ||
|
|
||
| return; | ||
| } | ||
| const [firstAddress] = addresses; | ||
| if ("undefined" === typeof firstAddress) { | ||
| fastify.log.error("No addresses found for the provided host."); | ||
|
|
||
| return; | ||
| } | ||
| if (1 < addresses.length) { | ||
| fastify.log.warn( | ||
| `Multiple addresses found for ${fastify.config.HOST}: ${ | ||
| JSON.stringify(addresses)}. ` + | ||
| "Proxying all socket.io requests to the main server." | ||
| ); | ||
| fastify.register(fastifyHttpProxy, { | ||
| upstream: `http://${fastify.config.HOST}:${fastify.config.PORT}/socket.io/`, | ||
| prefix: "/socket.io/", | ||
| }); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix potential circular proxy issue and improve string concatenation.
The DNS lookup implementation is well-structured with proper error handling, but there are two issues:
-
Circular proxy concern: The proxy upstream points to the same
HOST:PORTthat's already being used by the server, which could create a circular proxy loop. -
String concatenation: Use template literals instead of string concatenation for better readability.
Apply this diff to address the template literal issue:
- fastify.log.warn(
- `Multiple addresses found for ${fastify.config.HOST}: ${
- JSON.stringify(addresses)}. ` +
- "Proxying all socket.io requests to the main server."
- );
+ fastify.log.warn(
+ `Multiple addresses found for ${fastify.config.HOST}: ${
+ JSON.stringify(addresses)}. ` +
+ `Proxying all socket.io requests to the main server.`
+ );For the circular proxy issue, please verify the proxy configuration. The current setup proxies requests to the same server instance, which may not work as intended.
Run the following script to verify how the proxy configuration should work:
🏁 Script executed:
#!/bin/bash
# Description: Check the proxy configuration logic and examine similar implementations
# Look for other HTTP proxy usage patterns in the codebase
rg -A 10 -B 5 "fastifyHttpProxy|http-proxy" --type ts
# Check if there are any existing proxy configurations
rg -A 10 -B 5 "upstream.*socket\.io" --type tsLength of output: 4803
Prevent circular proxy loops by proxying to the resolved IP and unify the log message
- Replace the split string concatenation in
fastify.log.warnwith a single template literal. - Use the DNS‐resolved
firstAddress.addressfor theupstreamURL instead offastify.config.HOSTto avoid a proxy calling itself.
Apply this diff in components/webui/server/src/plugins/MongoSocketIoServer/index.ts:
@@ dnsLookup(fastify.config.HOST, {all: true}, (err, addresses) => {
- if (1 < addresses.length) {
- fastify.log.warn(
- `Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. ` +
- "Proxying all socket.io requests to the main server."
- );
- fastify.register(fastifyHttpProxy, {
- upstream: `http://${fastify.config.HOST}:${fastify.config.PORT}/socket.io/`,
- prefix: "/socket.io/",
- });
+ if (addresses.length > 1) {
+ const {address: mainAddress} = addresses[0];
+ fastify.log.warn(
+ `Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. ` +
+ `Proxying all socket.io requests to ${mainAddress}:${fastify.config.PORT}.`
+ );
+ fastify.register(fastifyHttpProxy, {
+ upstream: `http://${mainAddress}:${fastify.config.PORT}/socket.io/`,
+ prefix: "/socket.io/",
+ });
}📝 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.
| // Fastify listens on all resolved addresses for localhost (e.g. `127.0.0.1`, `::1`), but | |
| // socket.io only uses the main one. When multiple addresses are resolved, we proxy | |
| // socket.io requests from secondary addresses to the main server. | |
| dnsLookup(fastify.config.HOST, {all: true}, (err, addresses) => { | |
| if (null !== err) { | |
| fastify.log.error(`DNS lookup failed: ${err.message}`); | |
| return; | |
| } | |
| const [firstAddress] = addresses; | |
| if ("undefined" === typeof firstAddress) { | |
| fastify.log.error("No addresses found for the provided host."); | |
| return; | |
| } | |
| if (1 < addresses.length) { | |
| fastify.log.warn( | |
| `Multiple addresses found for ${fastify.config.HOST}: ${ | |
| JSON.stringify(addresses)}. ` + | |
| "Proxying all socket.io requests to the main server." | |
| ); | |
| fastify.register(fastifyHttpProxy, { | |
| upstream: `http://${fastify.config.HOST}:${fastify.config.PORT}/socket.io/`, | |
| prefix: "/socket.io/", | |
| }); | |
| } | |
| }); | |
| // Fastify listens on all resolved addresses for localhost (e.g. `127.0.0.1`, `::1`), but | |
| // socket.io only uses the main one. When multiple addresses are resolved, we proxy | |
| // socket.io requests from secondary addresses to the main server. | |
| dnsLookup(fastify.config.HOST, {all: true}, (err, addresses) => { | |
| if (null !== err) { | |
| fastify.log.error(`DNS lookup failed: ${err.message}`); | |
| return; | |
| } | |
| const [firstAddress] = addresses; | |
| if ("undefined" === typeof firstAddress) { | |
| fastify.log.error("No addresses found for the provided host."); | |
| return; | |
| } | |
| if (addresses.length > 1) { | |
| const { address: mainAddress } = addresses[0]; | |
| fastify.log.warn( | |
| `Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. ` + | |
| `Proxying all socket.io requests to ${mainAddress}:${fastify.config.PORT}.` | |
| ); | |
| fastify.register(fastifyHttpProxy, { | |
| upstream: `http://${mainAddress}:${fastify.config.PORT}/socket.io/`, | |
| prefix: "/socket.io/", | |
| }); | |
| } | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 102-104: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/MongoSocketIoServer/index.ts between
lines 84 and 112, update the fastify.log.warn call to use a single template
literal instead of string concatenation for clarity. Also, change the upstream
URL in the fastifyHttpProxy registration to use the DNS-resolved
firstAddress.address instead of fastify.config.HOST to prevent circular proxy
loops by ensuring the proxy targets the actual resolved IP address.
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
Outdated
Show resolved
Hide resolved
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
103-106: Prefer template literals over string concatenation (duplicate of earlier feedback)Same style issue previously pointed out in past reviews – switch to a single template literal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/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>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/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.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: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
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.
components/webui/server/src/plugins/MongoSocketIoServer/index.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: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: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
🪛 Biome (1.9.4)
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
[error] 104-106: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
Outdated
Show resolved
Hide resolved
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
Outdated
Show resolved
Hide resolved
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
Outdated
Show resolved
Hide resolved
|
I can't test this since mac (maybe @haiqi96), but from what you described the approach makes sense. Do we need this fix for the release? |
| JSON.stringify(addresses)}. ` + | ||
| "Proxying all socket.io requests to the main server." | ||
| ); | ||
| fastify.register(fastifyHttpProxy, { |
There was a problem hiding this comment.
you may beable to get https/http from fastify.initialConfig.https, so not hardcoded at http
There was a problem hiding this comment.
we don't support HTTPS cert configuration in the Fastify server yet so this may not matter. still, i made the change to show the consideration.
until we add HTTPS support in the fastify server and disable HTTP when deployment maintainers configure so, it's recommended to protect the HTTP server with an HTTPS-enabled Nginx reverse proxy
| // Fastify listens on all resolved addresses for localhost (e.g. `127.0.0.1`, `::1`), but | ||
| // socket.io only uses the main one. When multiple addresses are resolved, we proxy | ||
| // socket.io requests from secondary addresses to the main server. | ||
| dnsLookup(fastify.config.HOST, {all: true}, (err, addresses) => { |
There was a problem hiding this comment.
you could potentially do this with fastify.addresses property instead, but you would need to call in the main.ts file and after fastify.listen(after plugin registration). Overall I think the approach you took where it is done in the this plugin is better.
There was a problem hiding this comment.
right, i believe the Fastify addresses info isn't available until fastify.listen() is called. also as you mentioned the proxy registration within this Mongo plugin was intentional.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
111-118: Proxy still loops back to itself + WebSocket upgrades not forwarded
upstreamtargets${fastify.config.HOST}:${fastify.config.PORT}, i.e. the very server that just registered the proxy. Every/socket.iorequest re-enters the proxy and creates an infinite loop.- Socket.IO relies on WebSocket upgrades but
@fastify/http-proxyonly forwards them whenwebsocket: trueis set.- Including
/socket.io/inupstreamwhile also settingprefix: "/socket.io/"double-appends the path after Fastify strips the prefix.Proposed fix:
- const protocol = fastify.initialConfig.https ? - "https" : - "http"; - - fastify.register(fastifyHttpProxy, { - upstream: `${protocol}://${fastify.config.HOST}:${fastify.config.PORT}/socket.io/`, - prefix: "/socket.io/", - }); + const [{address: primaryIp}] = addresses; // break the self-proxy loop + const protocol = fastify.initialConfig.https ? "https" : "http"; + + fastify.register(fastifyHttpProxy, { + upstream: `${protocol}://${primaryIp}:${fastify.config.PORT}`, + prefix: "/socket.io/", + websocket: true, // forward WS upgrades + });This routes secondary-address traffic straight to the main listener, forwards WS handshakes, and avoids path duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/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>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/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.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: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
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.
components/webui/server/src/plugins/MongoSocketIoServer/index.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: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: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
🪛 Biome (1.9.4)
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
[error] 106-108: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
| fastify.log.warn( | ||
| `Multiple addresses found for ${fastify.config.HOST}: ${ | ||
| JSON.stringify(addresses)}. ` + | ||
| "Proxying all socket.io requests to the main server." |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use a single template literal – current string concatenation violates style guide
Static analysis is flagging the fastify.log.warn message for concatenation. Switching to one template literal keeps the linter happy and is more readable.
- fastify.log.warn(
- `Multiple addresses found for ${fastify.config.HOST}: ${
- JSON.stringify(addresses)}. ` +
- "Proxying all socket.io requests to the main server."
- );
+ fastify.log.warn(
+ `Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. `
+ + "Proxying all socket.io requests to the main server."
+ );📝 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.
| fastify.log.warn( | |
| `Multiple addresses found for ${fastify.config.HOST}: ${ | |
| JSON.stringify(addresses)}. ` + | |
| "Proxying all socket.io requests to the main server." | |
| fastify.log.warn( | |
| `Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. ` | |
| + "Proxying all socket.io requests to the main server." | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 106-108: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/MongoSocketIoServer/index.ts around lines
105 to 108, the log message uses string concatenation which violates the style
guide. Replace the concatenation with a single template literal that includes
all parts of the message inside backticks to improve readability and satisfy the
linter.
There was a problem hiding this comment.
it was broken down to avoid line length violation of max 100 characters
There was a problem hiding this comment.
@junhaoliao I understand - you're balancing the template literal preference with the 100-character line limit. Here's a solution that satisfies both constraints using a multi-line template literal:
fastify.log.warn(
`Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. ` +
`Proxying all socket.io requests to the main server.`
);This keeps each line under 100 characters while using template literals throughout, which should satisfy both the linter and the line length constraint.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:01.662Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
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.
🧠 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.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: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: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
Hey @haiqi96 , could you confirm this is tested on your WSL + Docker Desktop setup? Due to the nature of the issue (i.e., Fastify listens on all addresses when
|
Yes I have tested it in the morning. |
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
Outdated
Show resolved
Hide resolved
|
I tested this on linux, and confirmed does not break anything. If required, this can be merged. I am discussing an alternate approach with junhao. The alternate approach is slightly worse but does not require a new dependency or unelegant code (style is good, but it inevitably it would look strange to a new dev on project) . currently weighing options, but this can be merged and we can decide later as well. |
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai open an issue about making the socket.io transport (e.g., ws only, polling only, or both) configurable in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@junhaoliao I'll create a GitHub issue for making socket.io transport configurable in 🏁 Script executed: Length of output: 1082 Perfect! I've successfully created the GitHub issue for making socket.io transport configurable in Issue created: #1092 The issue includes:
The issue is now ready for discussion and future implementation. 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
115-118: Proxy still risks infinite recursion and drops WebSocket upgradesThe proxy targets
${fastify.config.HOST}:${fastify.config.PORT}– i.e. the very same server that just registered the proxy – so every/socket.io/request re-enters the proxy and loops forever.
Additionally,@fastify/http-proxyonly forwards WebSocket upgrade requests whenwebsocket: trueis supplied; missing this flag breaks Socket.IO on WS transport.- fastify.register(fastifyHttpProxy, { - upstream: `${protocol}://${fastify.config.HOST}:${fastify.config.PORT}/socket.io/`, - prefix: "/socket.io/", - }); + fastify.register(fastifyHttpProxy, { + // Route to the primary resolved address to avoid self-proxying + upstream: `${protocol}://${addresses[0].address}:${fastify.config.PORT}`, + prefix: "/socket.io/", + websocket: true, // essential for Socket.IO upgrades + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/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>.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
components/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.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: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
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.
components/webui/server/src/plugins/MongoSocketIoServer/index.ts (6)
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: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: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
Learnt from: junhaoliao
PR: y-scope/clp#1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:35-41
Timestamp: 2024-11-19T19:52:43.429Z
Learning: For internal APIs in `components/log-viewer-webui/client/src/api/query.js`, runtime validation of parameters may not be necessary since the APIs are not exposed to end users, and JsDoc type annotations may be sufficient.
🪛 Biome (1.9.4)
components/webui/server/src/plugins/MongoSocketIoServer/index.ts
[error] 106-108: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
| fastify.log.warn( | ||
| `Multiple addresses found for ${fastify.config.HOST}: ${ | ||
| JSON.stringify(addresses)}. ` + | ||
| "Proxying all socket.io requests to the main server." |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use a multiline template literal to satisfy both the linter and ≤100-char line limit
- fastify.log.warn(
- `Multiple addresses found for ${fastify.config.HOST}: ${
- JSON.stringify(addresses)}. ` +
- "Proxying all socket.io requests to the main server."
- );
+ fastify.log.warn(
+ `Multiple addresses found for ${fastify.config.HOST}: ${
+ JSON.stringify(addresses)
+ }. Proxying all socket.io requests to the main server.`
+ );📝 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.
| fastify.log.warn( | |
| `Multiple addresses found for ${fastify.config.HOST}: ${ | |
| JSON.stringify(addresses)}. ` + | |
| "Proxying all socket.io requests to the main server." | |
| fastify.log.warn( | |
| `Multiple addresses found for ${fastify.config.HOST}: ${ | |
| JSON.stringify(addresses) | |
| }. Proxying all socket.io requests to the main server.` | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 106-108: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🤖 Prompt for AI Agents
In components/webui/server/src/plugins/MongoSocketIoServer/index.ts around lines
105 to 108, the current log statement concatenates strings and exceeds the
100-character line limit, which violates the linter rules. Refactor the log
message to use a multiline template literal instead of string concatenation,
breaking the message into multiple lines within backticks to satisfy both the
linter and line length constraints.
@davemarco the rabbit has helped to create an issue at #1092 about the transport config option we discussed offline. I believe it will be a good opportunity to revisit the changes in the current PR when we implement #1092 . |
Description
See the analysis at #1062 . This PR implements "Approach 5" which is the final recommendation.
Checklist
breaking change.
Validation performed
On Windows:
dockergroup:sudo usermod -aG docker "$USER".Within the WSL VM:
taskcd build/clp-package/sbin; ./start-clp.sh./compress.sh ~/sample/hive-24hr/i-00c90a0f/Summary by CodeRabbit