Disable WebSocket by default (opt-in via ENABLE_WEBSOCKET)#3
Open
nanehambardzumyan wants to merge 2 commits into
Open
Disable WebSocket by default (opt-in via ENABLE_WEBSOCKET)#3nanehambardzumyan wants to merge 2 commits into
nanehambardzumyan wants to merge 2 commits into
Conversation
Co-authored-by: Nane Hambardzumyan <nanehambardzumyan@users.noreply.github.com>
Co-authored-by: Nane Hambardzumyan <nanehambardzumyan@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR adds an 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/app.js`:
- Around line 4-7: The browser-side gate in client/app.js is hardcoded off, so
the optional messaging path can never be re-enabled even when the server/runtime
flag is set. Update the ENABLE_WEBSOCKET constant to read from shared
runtime/build configuration instead of using a literal false, and keep the
existing WebSocket connection setup in app.js driven by that shared flag so the
opt-in flow works end to end.
In `@server.js`:
- Around line 26-29: The WebSocket gating in server.js conflates
configuration-disabled and dependency-missing states via isWebSocketAvailable,
so split the logic around WEBSOCKET_ENABLED and the ws import in the startup
block and POST /message handler. Keep a separate flag or branch for “disabled by
config” versus “ws unavailable,” and return a distinct 503 response for each
case so clients only see the install-ws message when the dependency is actually
missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 448c57d2-6a85-4db7-986c-44fd3b5ad1a7
📒 Files selected for processing (3)
client/app.jsserver.jsvite.config.js
💤 Files with no reviewable changes (1)
- vite.config.js
Comment on lines
+4
to
+7
| // WebSocket messaging is disabled by default. The API Simulator UI uses plain | ||
| // HTTP, so the optional alert/messaging channel is opt-in. Set this to true and | ||
| // run the server with ENABLE_WEBSOCKET=true to re-enable it. | ||
| const ENABLE_WEBSOCKET = false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't hardcode the client gate to false.
With this literal, enabling ENABLE_WEBSOCKET=true on the server still never re-enables the browser connection, so the opt-in flow described in this PR cannot work end to end. Read the flag from shared runtime/build config instead of baking in false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/app.js` around lines 4 - 7, The browser-side gate in client/app.js is
hardcoded off, so the optional messaging path can never be re-enabled even when
the server/runtime flag is set. Update the ENABLE_WEBSOCKET constant to read
from shared runtime/build configuration instead of using a literal false, and
keep the existing WebSocket connection setup in app.js driven by that shared
flag so the opt-in flow works end to end.
Comment on lines
+26
to
+29
| const WEBSOCKET_ENABLED = process.env.ENABLE_WEBSOCKET === 'true'; | ||
| let WebSocket = null; | ||
| let isWebSocketAvailable = false; | ||
| if (WEBSOCKET_ENABLED) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate “disabled by config” from “ws unavailable”.
This gate reuses isWebSocketAvailable for both states, so POST /message now tells clients to install ws even when WebSockets were only disabled via ENABLE_WEBSOCKET. Return a config-disabled 503 body separately from the missing-dependency case.
Also applies to: 38-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 26 - 29, The WebSocket gating in server.js conflates
configuration-disabled and dependency-missing states via isWebSocketAvailable,
so split the logic around WEBSOCKET_ENABLED and the ws import in the startup
block and POST /message handler. Keep a separate flag or branch for “disabled by
config” versus “ws unavailable,” and return a distinct 503 response for each
case so clients only see the install-ws message when the dependency is actually
missing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The API Simulator UI communicates over plain HTTP and does not use WebSockets — the
/wschannel andPOST /messagebroadcasting were a template-level feature that the simulator never relies on. This change disables WebSocket support by default and makes it opt-in.Changes
server.js: WebSocket support is now gated behindENABLE_WEBSOCKET. By default thewsmodule is not loaded, the/wsserver is not started, andPOST /messagereturns503. SetENABLE_WEBSOCKET=trueto restore the previous behavior.client/app.js: The browser no longer opens a WebSocket connection to/ws(and no longer enters the 3s reconnect loop). Re-enable by flipping theENABLE_WEBSOCKETconstant and running the server withENABLE_WEBSOCKET=true.vite.config.js: Removed the/wsdev-server proxy. With the backend WS server disabled, this proxy pointed at a dead upstream and produced repeated[vite] ws proxy error: write EPIPEspam whenever any (e.g. stale/cached) client tried to open/ws. Restore this entry if re-enabling WebSocket.Testing
node scripts/smoke-test.mjs— demo CRUD still passescurl -X POST http://localhost:3001/message→503(disabled by default)ENABLE_WEBSOCKET=true PORT=3009 node server.js→POST /messagereturns200and/wsserver starts (re-enable works)/wsconnection attempts against the Vite dev server →0EPIPE / ws-proxy errors (previously spammed every ~3s)GET http://localhost:3000/demo-api/health→200, frontend/→200200 OKforGET /demo-api/healthNetwork WS filter shows 0 WebSocket connections
Simulator returns 200 OK with WebSocket disabled
To show artifacts inline, enable in settings.