-
Notifications
You must be signed in to change notification settings - Fork 692
feat(server): add option to customize basePath #45
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
Conversation
WalkthroughThe changes introduce a new configuration option for the Changes
Possibly related PRs
✨ 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 (
|
@zoedsoupe This is great! Thanks for the contribution. Would you be able to add one or two simple tests just to show this works as intended? Cheers! |
@ezynda3 done! is that ok? |
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 (1)
server/sse_test.go (1)
418-470
: LGTM - Comprehensive test case for custom basePath functionalityThis test case is well-structured and thoroughly tests the custom basePath functionality:
- Verifies 404 for requests without the basePath
- Confirms successful responses with the basePath
- Includes proper resource cleanup
One small suggestion for line 439: consider using
path.Join
or similar to avoid potential issues with slash concatenation, though the current implementation works as expected with the tested values.- sseURL := fmt.Sprintf("%s/sse", ts.URL+sseServer.basePath) + sseURL := fmt.Sprintf("%s%s/sse", ts.URL, sseServer.basePath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/sse.go
(5 hunks)server/sse_test.go
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/sse.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
server/sse_test.go
27-27: SA5011: possible nil pointer dereference
(staticcheck)
24-24: SA5011(related information): this check suggests that the pointer can be nil
(staticcheck)
30-30: SA5011: possible nil pointer dereference
(staticcheck)
🔇 Additional comments (3)
server/sse_test.go (3)
19-22
: LGTM - Correctly implemented basePath optionThe addition of
WithBasePath("/mcp")
option aligns with the PR's objective to allow customization of the basePath.
30-35
: LGTM - Proper baseURL verificationThe test now correctly checks that the baseURL is composed of both the base URL and the basePath.
🧰 Tools
🪛 golangci-lint (1.62.2)
30-30: SA5011: possible nil pointer dereference
(staticcheck)
36-41
: LGTM - Good test coverage for the new fieldThe new assertion properly verifies that the
basePath
field is correctly set to "/mcp".
Close #44
Summary by CodeRabbit