-
-
Notifications
You must be signed in to change notification settings - Fork 4
Pass disconnect reason to the onDisconnect hook #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughDisconnect handling now forwards the disconnect reason: the disconnect handler constructs an augmented context by spreading the existing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/hooks.ts (1)
26-27: Consider logging the disconnect reason.The default hook now receives the
reasonbut doesn't log it. Including it could help with debugging disconnect issues.- onDisconnect: ({ client: { id, getData }, logger }) => - logger.debug("Client disconnected", { ...getData(), id }), + onDisconnect: ({ client: { id, getData }, logger, reason }) => + logger.debug("Client disconnected", { ...getData(), id, reason }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/attach.ts(1 hunks)src/hooks.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/hooks.ts (2)
src/handler.ts (2)
Handler(48-48)ClientContext(24-28)src/index.ts (1)
ClientContext(12-12)
🔇 Additional comments (1)
src/attach.ts (1)
121-121: LGTM! Correctly forwards the disconnect reason.The implementation properly captures the
reasonfrom Socket.IO's disconnect event and passes it to theonDisconnecthook by spreading the existing context and adding the reason property. This aligns with the type definition inhooks.ts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/hooks.ts (2)
15-18: Type augmentation implemented correctly.The intersection type properly extends
ClientContext<E, D>with thereason: DisconnectReasonfield, enabling the disconnect handler to receive Socket.IO's disconnect reason.Consider adding a JSDoc comment to document the new
reasonparameter for better developer experience:export interface Hooks<E extends EmissionMap, D extends z.ZodObject> { /** @desc The place for emitting events regardless receiving events */ onConnection: Handler<ClientContext<E, D>, void>; + /** @desc Called when a client disconnects, with the disconnect reason provided by Socket.IO */ onDisconnect: Handler< ClientContext<E, D> & { reason: DisconnectReason }, void >;
30-31: Consider logging the disconnect reason for enhanced debugging.The default implementation could be enhanced to include the disconnect reason in the log output, providing more context during debugging:
- onDisconnect: ({ client: { id, getData }, logger }) => - logger.debug("Client disconnected", { ...getData(), id }), + onDisconnect: ({ client: { id, getData }, logger, reason }) => + logger.debug("Client disconnected", { ...getData(), id, reason }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/hooks.ts (3)
src/emission.ts (1)
EmissionMap(15-15)src/index.ts (2)
EmissionMap(10-10)ClientContext(12-12)src/handler.ts (2)
Handler(48-48)ClientContext(24-28)
🔇 Additional comments (1)
src/hooks.ts (1)
10-10: LGTM! Previous review suggestions implemented correctly.The type-only import of
DisconnectReasonfrom Socket.IO addresses both previous review comments and follows TypeScript best practices.
I noticed that the disconnect reason (https://socket.io/docs/v4/server-api#events-2) is not passed through to the onDisconnect hook. This PR fixes that.
Summary by CodeRabbit
Bug Fixes
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.