feat(server): add onPeerDisconnect, symmetric with onPeerConnect - #166
Open
dvcolomban wants to merge 1 commit into
Open
feat(server): add onPeerDisconnect, symmetric with onPeerConnect#166dvcolomban wants to merge 1 commit into
dvcolomban wants to merge 1 commit into
Conversation
startHttpAndWs's onDisconnected handler consumed the transport's own disconnect entirely (emitting the session-disconnected event), leaving callers with no way to observe a peer going away — the only socket-close hook is host-functions.ts's _emitSessionDisconnected, and its own docblock says it is @internal and must not widen the public surface. Add onPeerDisconnect(peer, meta), forwarded verbatim to the underlying startHttpAndWs. Unlike onPeerConnect it receives the raw session meta, not a wrapped session — by the time a peer disconnects there is no live RPC client left to attach. createDevServer never forwarded either hook down to startHttpAndWs, so this also adds a passthrough onPeerConnect option there for parity — today a createDevServer caller has no way to reach either callback at all.
✅ Deploy Preview for devfra ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
onPeerDisconnect currently receives a meta object after internal disconnect cleanup mutates it, which can undermine the “raw session meta” semantics for consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adds a public disconnect-time peer hook to the devframe node WebSocket server (startHttpAndWs) and plumbs both connect/disconnect peer hooks through createDevServer, enabling hosts to react to peer lifecycle events without reaching into @internal APIs.
Changes:
- Added
onPeerDisconnecttoStartHttpAndWsOptionsand invoked it from the WS transportonDisconnectedhandler. - Added
onPeerConnect/onPeerDisconnectpassthrough options tocreateDevServer. - Added vitest coverage for hook forwarding at both the
startHttpAndWsandcreateDevServerlayers.
File summaries
| File | Description |
|---|---|
| packages/devframe/src/node/server.ts | Adds onPeerDisconnect option and wires it into the WS disconnect hook. |
| packages/devframe/src/node/tests/server.test.ts | Verifies startHttpAndWs fires connect + disconnect hooks for the same session. |
| packages/devframe/src/adapters/dev.ts | Exposes and forwards peer hook options through createDevServer. |
| packages/devframe/src/adapters/tests/dev.test.ts | Verifies createDevServer forwards both hooks to startHttpAndWs. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+234
to
237
| onDisconnected: (peer, meta) => { | ||
| rpcHost._emitSessionDisconnected(meta) | ||
| options.onPeerDisconnect?.(peer, meta) | ||
| }, |
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.
What
startHttpAndWs'sonDisconnectedhandler consumed the transport's own disconnect entirely — it only fedrpcHost._emitSessionDisconnected(meta)— leaving callers with no way to observe a peer going away. The only socket-close hook that exists ishost-functions.ts's_emitSessionDisconnected, and its own docblock says it's@internaland "must not" widen the public surface. MeanwhileonPeerConnect(the connect-time counterpart) is already public.This adds
onPeerDisconnect?: (peer, meta) => void, forwarded verbatim to the underlyingstartHttpAndWs. UnlikeonPeerConnectit receives the raw session meta, not a wrapped session — by the time a peer disconnects there's no live RPC client left to attach.createDevServernever forwarded either hook down tostartHttpAndWsat all, so this also adds a passthroughonPeerConnectoption there for parity — today acreateDevServercaller has no way to reach either callback.Why this matters
Any host that tracks per-peer state outside the RPC layer (a registry keyed by connection, a presence list, cleanup for something the peer owned) currently has no first-party way to know a peer left — it either reaches for the internal, explicitly-not-for-this
_emitSessionDisconnected, or leaks that state forever.Tests
Two new cases:
packages/devframe/src/node/__tests__/server.test.ts:startHttpAndWsforwards both hooks for the same peer —onPeerConnectfires on open,onPeerDisconnectfires on close with matching session meta (meta.id), and neither fires early.packages/devframe/src/adapters/__tests__/dev.test.ts:createDevServerforwards both options down tostartHttpAndWsend to end.Both verified to fail (0 calls recorded) against the pre-fix code before adding the guard/forwarding.
pnpm --filter devframe exec vitest run— 49 files, 448 tests, all green.tsc --noEmitclean.eslintclean.🤖 Generated with Claude Code