fix: address CodeRabbit review findings (distZip gate, socket leak, send overload, outbox cap) - #173
Merged
Merged
Conversation
- cli(next): gate DistService instantiation behind distZip !== false, matching the Vite plugin path so sync is correctly disabled when distZip: false - cli(next): destroy unhandled upgrade sockets instead of leaving them open - pp-ws-server: implement send(payload) overload — non-string payloads were silently dropped; now broadcast as raw JSON like the string overload - hot-context: cap outbox at 50 entries (drop oldest) to prevent unbounded memory growth during prolonged disconnects - test(load-pp-data): await server.close() callbacks to prevent flaky parallel tests - test(pp-ws-server): yield an event-loop tick before asserting broadcast isolation to eliminate the race condition in the per-client isolation test
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
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
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
Addresses 6 findings from the CodeRabbit review of PR #171, covering two production bugs, one silent data loss issue, one memory growth issue, and two test reliability fixes.
🔧 Fixes
src/cli.ts—distZip: falseignored inpp-dev nextThe Vite plugin path correctly gates
DistServiceinstantiation behinddistZip !== false, but thenextcommand always created it regardless. AddeddistZipto the destructuredppDevConfig(default:true) and wrappednew DistService(...)in the same conditional, so template sync is properly disabled whendistZip: false.src/cli.ts— unhandled upgrade sockets left openWhen neither
hotServernornextUpgradeHandlerclaimed an HTTP upgrade, the socket was left open indefinitely. Addedsocket.destroy()in the else branch to release it immediately.src/lib/pp-ws-server.ts—send(payload)overload silently dropped messagesViteWsFacadedeclares twosendoverloads —send(event, data?)andsend(payload: unknown)— but the implementation only handled the string case. Non-string payloads were silently discarded. The payload overload now broadcasts the pre-built object as raw JSON to all connected clients, matching Vite's broadcast API.src/client/hot-context.ts— unbounded offline outboxMessages queued while the WebSocket was disconnected accumulated in an unbounded array, then all flushed at once on reconnect. Capped the outbox at 50 entries — oldest entry is dropped when the limit is reached.
🧪 Test reliability fixes
tests/integration/middleware/load-pp-data.spec.ts—server.close()not awaitedAll four
finallyblocks calledserver.close()without waiting for the callback, causing flaky failures in parallel test runs. Promisified all four calls.tests/integration/middleware/pp-ws-server.spec.ts— race condition in isolation testThe broadcast isolation assertion (
otherReceived === false) ran immediately after the sender received its response, before the other client had a chance to process any unintended broadcast. Added asetImmediateyield to let the event loop flush before asserting.🧪 Testing
npm run test:unit— 171 passednpm run test:integration— 39 passedMerge Request:
origin/pp-3449-coderabbit-fixes→origin/develop