Fixes critical WebSocket configuration gaps and async route handler errors.#32
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR trims request JSON bodies before parsing and treats empty bodies differently by method; it constructs and registers the UwsResponse/abort handler earlier in route handlers and passes the response into body-parser initialization; and it exposes/resolves additional uWebSockets adapter options (maxLifetime, maxBackpressure, closeOnBackpressureLimit, sendPingsAutomatically). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant uWS
participant RouteRegistry
participant UwsResponse
participant UwsRequest
Client->>uWS: HTTP request
uWS->>RouteRegistry: invoke shared handler
RouteRegistry->>UwsResponse: construct wrapper (early)
RouteRegistry->>UwsResponse: register _onAbort handler
RouteRegistry->>UwsRequest: init body parser with res
UwsRequest->>RouteRegistry: body parsed / json() (trim -> parse or handle empty)
RouteRegistry->>UwsResponse: invoke matched route handler
UwsResponse-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/http/core/request.ts (1)
1074-1085: Empty-body handling looks reasonable, with one subtle caveat.The trim + method-based branching is sensible: it produces a clearer error than letting
JSON.parse('')throw, andthis.methodis normalized to uppercase in the constructor (line 175), so the comparisons are reliable.One small thing worth being aware of:
this.cachedJson = {}caches a shared object reference. If a handler mutates the returned object, subsequent calls tojson()(orreq.body) on the same request will see the mutation. This matches the existing JSON cache semantics, so it's not a regression — but the empty-body case makes it slightly easier to hit accidentally. If you want belt-and-suspenders, freezing the object would prevent that:🔒 Optional hardening
- if (text === '') { - if (this.method === 'GET' || this.method === 'HEAD' || this.method === 'DELETE') { - this.cachedJson = {} as T; - return this.cachedJson as T; - } + if (text === '') { + if (this.method === 'GET' || this.method === 'HEAD' || this.method === 'DELETE') { + this.cachedJson = Object.freeze({}) as T; + return this.cachedJson as T; + } throw new Error('Invalid JSON: Request body is empty', { cause: new SyntaxError('Unexpected end of JSON input'), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http/core/request.ts` around lines 1074 - 1085, The current empty-body branch assigns this.cachedJson = {} as T, which stores a mutable shared object that handlers could accidentally mutate; update the empty-body handling in the json parsing path (where this.cachedJson is set for GET/HEAD/DELETE) to assign an immutable object instead (e.g., create the empty object and Object.freeze it before assigning to this.cachedJson) so callers receive a read-only empty object while preserving the existing caching behavior in methods like json() / request parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/websocket/adapter/uws.adapter.ts`:
- Around line 128-131: Remove the dead perMessageDeflate option: delete any
reference to perMessageDeflate (e.g., the this.options.perMessageDeflate
initialization at line with perMessageDeflate and any uses elsewhere) and from
the public interface/type so compression is controlled only via the existing
compression option; ensure this.app.ws(...) continues to use
this.options.compression (uWS.SHARED_COMPRESSOR by default), update any
default/docs to reflect that per-message DEFLATE is now determined solely by
compression, and remove or refactor any conditional logic that attempted to read
perMessageDeflate.
In `@src/websocket/interfaces/uws-options.interface.ts`:
- Around line 39-55: The perMessageDeflate option is invalid for uWebSockets.js
v20 and should be removed or mapped to the new compression API: delete the
perMessageDeflate property from the uws options interface (remove the
perMessageDeflate?: boolean declaration) and from ResolvedUwsAdapterOptions, and
instead rely on the compression: CompressOptions field
(WebSocketBehavior/compression) using uWS.DISABLED, uWS.SHARED_COMPRESSOR, or
uWS.DEDICATED_COMPRESSOR_*; if you need backward compatibility, implement logic
in the option resolution (e.g., in the function that produces
ResolvedUwsAdapterOptions) to translate perMessageDeflate === false into
compression = uWS.DISABLED and perMessageDeflate === true into a sensible
default compressor, then remove the docs/examples referencing perMessageDeflate.
---
Nitpick comments:
In `@src/http/core/request.ts`:
- Around line 1074-1085: The current empty-body branch assigns this.cachedJson =
{} as T, which stores a mutable shared object that handlers could accidentally
mutate; update the empty-body handling in the json parsing path (where
this.cachedJson is set for GET/HEAD/DELETE) to assign an immutable object
instead (e.g., create the empty object and Object.freeze it before assigning to
this.cachedJson) so callers receive a read-only empty object while preserving
the existing caching behavior in methods like json() / request parsing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34731568-4373-491f-8c92-de8ec9de3f1c
📒 Files selected for processing (4)
src/http/core/request.tssrc/http/routing/route-registry.tssrc/websocket/adapter/uws.adapter.tssrc/websocket/interfaces/uws-options.interface.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/websocket/interfaces/uws-options.interface.ts (1)
53-79: Remove the dangling duplicate JSDoc abovecompression.Lines 53–56 contain a stray JSDoc block that is immediately followed by another JSDoc block (57–79) on the same
compressionproperty. Only the latter attaches to the field; the former is leftover from the doc expansion and shows up as orphaned commentary in source/IDE tooltips.♻️ Suggested cleanup
- /** - * Compression mode - * `@default` uWS.SHARED_COMPRESSOR - */ /** * Compression mode * * Controls per-message deflate compression for WebSocket messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/websocket/interfaces/uws-options.interface.ts` around lines 53 - 79, Remove the stray/duplicate JSDoc block that precedes the detailed one so the `compression` property has only the comprehensive JSDoc; locate the `compression` property comment blocks (the orphan short JSDoc immediately above the longer multi-line JSDoc) and delete the shorter block, leaving the detailed JSDoc attached to `compression`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/http/core/request.ts`:
- Around line 1076-1086: The inline comment above the empty-body branch in the
json() method is misleading and the code also freezes the shared empty object;
update the comment to say “throw for any method other than GET/HEAD/DELETE” to
reflect actual behavior, and either remove Object.freeze({}) when assigning
this.cachedJson to return a mutable empty object or add/update the json() JSDoc
to document that this.cachedJson is a shared frozen object (call site will get a
TypeError if they try to mutate it); changes should be made in the json()
function where this.cachedJson is assigned and where Object.freeze({}) is used.
---
Nitpick comments:
In `@src/websocket/interfaces/uws-options.interface.ts`:
- Around line 53-79: Remove the stray/duplicate JSDoc block that precedes the
detailed one so the `compression` property has only the comprehensive JSDoc;
locate the `compression` property comment blocks (the orphan short JSDoc
immediately above the longer multi-line JSDoc) and delete the shorter block,
leaving the detailed JSDoc attached to `compression`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 340a6f7d-6527-4818-8754-861c77d4c91d
📒 Files selected for processing (5)
src/http/core/request.tssrc/http/platform/uws-platform.adapter.tssrc/http/routing/route-registry.tssrc/websocket/adapter/uws.adapter.tssrc/websocket/interfaces/uws-options.interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/websocket/adapter/uws.adapter.ts
…nd Attach an abort handler at the beginning of the route handler #30
1 participant
WebSocket Configuration (Fixes #31)
Exposed 4 missing WebSocket options that were used internally but not available in the interface:
maxBackpressure- Maximum buffered bytes per connection (default: 1MB)maxLifetime- Automatically closes WebSocket connections after a specified number of minutes, regardless of activitycloseOnBackpressureLimit- Auto-close slow clients exceeding buffer limitsendPingsAutomatically- Enable automatic ping frames for connection healthAll options now properly documented with
JSDocand applied inUwsAdapterconstructor.Async Route Handler Fix (Fixes #30)
Root cause:
uWebSockets.jsrequires either synchronous response OR abort handler attachment before async operations.Solution: Attach
res._onAbort()handler immediately at the start of route handlers in 3 locations:Summary by CodeRabbit
New Features
Bug Fixes
Refactor