feat: add hooks for sse client #221
Conversation
WalkthroughThis change introduces a hook mechanism to the SSE transport client, enabling custom functions to be executed before sending HTTP requests and after receiving HTTP responses. Two new function types, Changes
✨ Finishing Touches
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. 🪧 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
♻️ Duplicate comments (1)
client/transport/sse.go (1)
396-401: Same potential issue with hook invocation timing.Similar to the
SendRequestmethod, theOnAfterResponsehooks inSendNotificationare invoked before checking the error from the HTTP request. As mentioned earlier, verify if this is the desired behavior.
🧹 Nitpick comments (3)
client/transport/sse.go (3)
77-83: Initialize hooks field in constructor.While the code will work as is, it's a good practice to initialize the hooks field with empty slices to avoid potential nil pointer issues if hooks are accessed before being set.
smc := &SSE{ baseURL: parsedURL, httpClient: &http.Client{}, responses: make(map[int64]chan *JSONRPCResponse), endpointChan: make(chan struct{}), headers: make(map[string]string), + hooks: SSEHooks{ + OnBeforeRequest: []OnBeforeRequestFunc{}, + OnAfterResponse: []OnAfterResponseFunc{}, + }, }
20-401: Consider adding utility methods for individual hook registration.The current implementation only allows setting all hooks at once with
WithSSEHooks. Consider adding utility methods for adding individual hooks without replacing the entire set, which would provide more flexibility.// Add a single OnBeforeRequest hook func WithSSEBeforeRequestHook(hook OnBeforeRequestFunc) ClientOption { return func(sc *SSE) { sc.hooks.OnBeforeRequest = append(sc.hooks.OnBeforeRequest, hook) } } // Add a single OnAfterResponse hook func WithSSEAfterResponseHook(hook OnAfterResponseFunc) ClientOption { return func(sc *SSE) { sc.hooks.OnAfterResponse = append(sc.hooks.OnAfterResponse, hook) } }
20-401: Consider adding example usage in the package documentation.Since this is a significant new feature, it would be helpful to include example usage in the package documentation or in code comments to guide users on how to effectively use the hooks.
You could add a package-level example or document with sample code showing how to use the hooks for common use cases such as logging, metrics collection, or authentication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/sse.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/transport/sse.go (2)
mcp/types.go (1)
Request(103-116)client/client.go (1)
ClientOption(27-27)
🔇 Additional comments (7)
client/transport/sse.go (7)
20-24: Good function type definitions with clear documentation.The new hook function types are well-defined with appropriate parameters and concise documentation. The comment for
OnAfterResponseFunccorrectly notes that whenerris not nil,respmay be nil, which is important for proper error handling in the hooks.
26-30: Well-structured hook container allowing multiple hooks.The
SSEHooksstruct design is clean and allows for registering multiple hooks of each type, which provides flexibility for composing different hook behaviors.
47-47: Appropriate field addition to SSE struct.The hooks field is properly added to the existing struct without disrupting the existing functionality.
62-67: Clear option function for setting hooks.The
WithSSEHooksfunction follows the established client option pattern in the codebase, and the comment clearly indicates that it replaces any existing hooks.
285-290: Well-implemented pre-request hook invocation.The
OnBeforeRequesthooks are properly invoked with nil checks before sending the HTTP request. The sequential invocation allows for multiple hooks to process or modify the request.
306-311: Hook invocation location forOnAfterResponsemight need adjustment.The
OnAfterResponsehooks are invoked before checking the error from the HTTP request. This means that if the HTTP request fails, the hooks will see an error but still run before the function returns. Consider if this is the desired behavior, as it might be cleaner to place the hook invocation after the error check.Please verify that this is the desired behavior for the
OnAfterResponsehooks. If you want the hooks to be called regardless of errors (which seems to be the case based on the function signature that includes an error parameter), then the current implementation is correct. If you want to skip hook invocation on errors, consider moving the hook invocation after the error check.
387-392: Consistent hook implementation inSendNotification.The
OnBeforeRequesthooks implementation inSendNotificationmatches the one inSendRequest, maintaining consistency across the codebase.
|
add hooks for sse client: