feat(server): Add hooks.AddOnUnregisterSession functionality #175
Conversation
|
""" WalkthroughThis change introduces a new hook mechanism for session unregistration events in the server. A new hook type and associated methods are added to the hooks system, enabling registration and invocation of functions when a client session is unregistered. The server's session unregistration logic is updated to trigger these hooks upon session removal. Additionally, tests are added to verify that both registration and unregistration hooks are correctly called, and to ensure that the absence of hooks does not cause errors during session lifecycle operations. Minor updates were made to propagate context parameters in session unregistration calls across several server components. Changes
Possibly related issues
Possibly related PRs
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/hooks.go (1)
14-15: Consistent hook function signature for session eventsThe new
OnUnregisterSessionHookFuncdoesn't include a context parameter, unlike its counterpartOnRegisterSessionHookFunc(line 12). This creates an inconsistency in the API.Consider adding a context parameter for consistency:
-type OnUnregisterSessionHookFunc func(session ClientSession) +type OnUnregisterSessionHookFunc func(ctx context.Context, session ClientSession)Or alternatively, document why context isn't needed for unregistration hooks if this is intentional.
server/server.go (1)
211-212: Improve type assertion for better error handlingThe current implementation uses a type assertion without checking if the operation succeeded, which could potentially panic if the session map contained non-ClientSession values.
Consider using a more defensive approach:
-session, _ := s.sessions.LoadAndDelete(sessionID) -s.hooks.UnregisterSession(session.(ClientSession)) +if session, ok := s.sessions.LoadAndDelete(sessionID); ok { + if clientSession, ok := session.(ClientSession); ok { + s.hooks.UnregisterSession(clientSession) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/internal/gen/hooks.go.tmplis excluded by!**/gen/**
📒 Files selected for processing (3)
server/hooks.go(3 hunks)server/server.go(1 hunks)server/server_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/hooks.go (1)
server/server.go (1)
ClientSession(54-63)
🔇 Additional comments (3)
server/hooks.go (1)
228-235: LGTM! Method implementation follows established patterns.The implementation of
UnregisterSessionfollows the same patterns as other hook invocation methods with appropriate nil checks.server/server_test.go (2)
1356-1398: Well-structured test for session hooksThe test thoroughly verifies that both the register and unregister session hooks are called with the correct session object when sessions are registered and unregistered.
1400-1413: Good edge case coverage with nil hooks testThis test verifies that registering and unregistering a session on a server without any hooks does not cause errors, which is an important edge case to cover.
Add OnUnregisterSession hook functionality to complement the existing OnRegisterSession hooks, allowing code to run when a client session is being removed from the server. In some cases, the server may want to do additional work when a session has been closed. For example, in the SSE server case where you may end up managing various logs for the duration of the session -- you would want to indicate that the session was finished.
rwjblue-glean
force-pushed
the
rwjblue/push-lwvnsznunykv
branch
from
e0211e4 to
435622d
Compare
Add OnUnregisterSession hook functionality to complement the existing OnRegisterSession hooks, allowing code to run when a client session is being removed from the server. In some cases, the server may want to do additional work when a session has been closed. For example, in the SSE server case where you may end up managing various logs for the duration of the session -- you would want to indicate that the session was finished.
Summary by CodeRabbit
New Features
Tests