fix: mcp-client should also include configurable http headers in the /sse request #100
Conversation
WalkthroughThe changes modify the SSE client implementation in Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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
🔭 Outside diff range comments (1)
client/sse.go (1)
386-396:⚠️ Potential issueMissing custom headers in the Initialize method
The
Initializemethod creates an HTTP request but doesn't set the custom headers fromc.headers. For consistency, the custom headers should be applied here as well.Apply this fix:
req.Header.Set("Content-Type", "application/json") + // set custom http headers + for k, v := range c.headers { + req.Header.Set(k, v) + }
🧹 Nitpick comments (1)
client/sse.go (1)
96-98: Consider refactoring header setting logic to reduce duplicationThe header setting logic is duplicated in both the
StartandsendRequestmethods. Consider extracting this to a helper method to improve maintainability.+ // setHeaders applies custom headers to an HTTP request + func (c *SSEMCPClient) setHeaders(req *http.Request) { + for k, v := range c.headers { + req.Header.Set(k, v) + } + } // In Start method req.Header.Set("Accept", "text/event-stream") req.Header.Set("Cache-Control", "no-cache") req.Header.Set("Connection", "keep-alive") - for k, v := range c.headers { - req.Header.Set(k, v) - } + c.setHeaders(req) // In sendRequest method req.Header.Set("Content-Type", "application/json") // set custom http headers - for k, v := range c.headers { - req.Header.Set(k, v) - } + c.setHeaders(req) // Similarly in Initialize method when fixedAlso applies to: 308-310
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/sse.go(1 hunks)
🔇 Additional comments (3)
client/sse.go (3)
38-38: LGTM!The new field
headers map[string]stringin theSSEMCPClientstruct properly stores custom HTTP headers. This aligns with the PR objective to make HTTP headers configurable.
71-71: LGTM!Good practice to initialize the headers map in the constructor. This prevents nil pointer dereference when headers are accessed later.
96-98: LGTM!The implementation correctly adds support for custom HTTP headers in the SSE request. This fulfills the PR objective by iterating through the headers map and setting each key-value pair on the request.
fix: mcp-client should also include configurable http headers in the /sse request
reasons
Summary by CodeRabbit