fix(mcp): make provider clients Worker-safe - #238
Merged
Conversation
Summary by CodeRabbit
WalkthroughMCP clients for Excalidraw, Flomo, Jin10, Lingxing, and Luckin now use Sequence Diagram(s)sequenceDiagram
participant JumpServerRuntime
participant StreamableHTTPTransport
participant SSETransport
participant MCPClient
JumpServerRuntime->>MCPClient: Create validated client
JumpServerRuntime->>StreamableHTTPTransport: Attempt connection
StreamableHTTPTransport-->>JumpServerRuntime: Return result or 404/405
JumpServerRuntime->>SSETransport: Retry unsupported endpoint
SSETransport->>MCPClient: Establish SSE session
MCPClient-->>JumpServerRuntime: Execute request
JumpServerRuntime->>MCPClient: Close client
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/providers/jumpserver/runtime.ts (1)
121-181: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching the discovered transport per endpoint.
Every call to
withJumpServerMcpClientre-attempts Streamable HTTP first, and for servers that only support legacy SSE, this means every single request (not just initial validation) pays for a failed Streamable HTTP round trip before falling back. The MCP SDK itself is moving toward a discovery-caching pattern for exactly this scenario in its v2 API (ConnectOptions.prior/PriorDiscovery), confirming this cost is real and worth avoiding on the hot path.Consider caching the successful transport kind keyed by
context.endpoint.href(e.g. a module-levelMap<string, "streamable" | "sse">) so subsequent calls to a known-legacy endpoint skip straight toSSEClientTransport.♻️ Example caching approach
+const transportKindCache = new Map<string, "streamable" | "sse">(); + async function connectJumpServerMcpClient(context: JumpServerMcpContext, headers: Headers): Promise<Client> { + const cacheKey = context.endpoint.href; + if (transportKindCache.get(cacheKey) === "sse") { + return connectLegacyClient(context, headers, cacheKey); + } const streamableClient = createJumpServerMcpClient(); const streamableTransport = new StreamableHTTPClientTransport(context.endpoint, { fetch: context.fetcher, requestInit: { headers, signal: context.signal }, }); try { await streamableClient.connect(streamableTransport, { timeout: requestTimeoutMs }); + transportKindCache.set(cacheKey, "streamable"); return streamableClient; } catch (error) { await streamableClient.close().catch(() => undefined); if (!isUnsupportedStreamableHttp(error)) { throw error; } } - const legacyClient = createJumpServerMcpClient(); - const legacyTransport = new SSEClientTransport(context.endpoint, { - fetch: context.fetcher, - requestInit: { headers, signal: context.signal }, - }); - try { - await legacyClient.connect(legacyTransport, { timeout: requestTimeoutMs }); - return legacyClient; - } catch (error) { - await legacyClient.close().catch(() => undefined); - throw error; - } + transportKindCache.set(cacheKey, "sse"); + return connectLegacyClient(context, headers, cacheKey); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/jumpserver/runtime.ts` around lines 121 - 181, Cache the successful transport type per endpoint in the connection flow. Update connectJumpServerMcpClient to consult a module-level map keyed by context.endpoint.href, skip StreamableHTTPClientTransport for endpoints previously discovered as SSE, and record the transport kind after each successful connection while preserving the existing fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/providers/jumpserver/definition.ts`:
- Around line 21-27: Update the placeholder in the MCP endpoint definition to
use a `/mcp`-style HTTPS example instead of the legacy `/sse` path, aligning it
with the preferred Streamable HTTP transport. Leave the surrounding label,
description, and validation settings unchanged.
---
Nitpick comments:
In `@src/providers/jumpserver/runtime.ts`:
- Around line 121-181: Cache the successful transport type per endpoint in the
connection flow. Update connectJumpServerMcpClient to consult a module-level map
keyed by context.endpoint.href, skip StreamableHTTPClientTransport for endpoints
previously discovered as SSE, and record the transport kind after each
successful connection while preserving the existing fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55b31022-ea69-4200-b29c-9ca354a7c9de
📒 Files selected for processing (8)
src/providers/excalidraw_mcp/runtime.tssrc/providers/flomo/executors.tssrc/providers/jin10/executors.tssrc/providers/jumpserver/definition.tssrc/providers/jumpserver/runtime.test.tssrc/providers/jumpserver/runtime.tssrc/providers/lingxing/runtime.tssrc/providers/luckin_coffee/executors.ts
Comment on lines
+21
to
+27
| label: "MCP Endpoint", | ||
| inputType: "text", | ||
| required: true, | ||
| secret: false, | ||
| placeholder: "https://jumpserver-mcp.example.com/sse", | ||
| description: | ||
| "The SSE endpoint of the official jumpserver/mcp server. Public HTTPS endpoints are supported by default. Private-network, Tailscale, and NetBird endpoints require OOMOL_CONNECT_ALLOW_PRIVATE_NETWORK. Loopback endpoints remain blocked. See https://github.com/jumpserver/mcp.", | ||
| "The Streamable HTTP endpoint, or the legacy SSE endpoint exposed by official jumpserver/mcp deployments. Public HTTPS endpoints are supported by default. Private-network, Tailscale, and NetBird endpoints require OOMOL_CONNECT_ALLOW_PRIVATE_NETWORK. Loopback endpoints remain blocked. See https://github.com/jumpserver/mcp.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Placeholder still models the legacy /sse endpoint.
The description now presents Streamable HTTP as primary, but placeholder (Line 25) still shows https://jumpserver-mcp.example.com/sse, which could nudge users toward configuring the legacy endpoint first. Consider using a /mcp-style example to match the now-preferred transport.
✏️ Suggested placeholder update
- placeholder: "https://jumpserver-mcp.example.com/sse",
+ placeholder: "https://jumpserver-mcp.example.com/mcp",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| label: "MCP Endpoint", | |
| inputType: "text", | |
| required: true, | |
| secret: false, | |
| placeholder: "https://jumpserver-mcp.example.com/sse", | |
| description: | |
| "The SSE endpoint of the official jumpserver/mcp server. Public HTTPS endpoints are supported by default. Private-network, Tailscale, and NetBird endpoints require OOMOL_CONNECT_ALLOW_PRIVATE_NETWORK. Loopback endpoints remain blocked. See https://github.com/jumpserver/mcp.", | |
| "The Streamable HTTP endpoint, or the legacy SSE endpoint exposed by official jumpserver/mcp deployments. Public HTTPS endpoints are supported by default. Private-network, Tailscale, and NetBird endpoints require OOMOL_CONNECT_ALLOW_PRIVATE_NETWORK. Loopback endpoints remain blocked. See https://github.com/jumpserver/mcp.", | |
| label: "MCP Endpoint", | |
| inputType: "text", | |
| required: true, | |
| secret: false, | |
| placeholder: "https://jumpserver-mcp.example.com/mcp", | |
| description: | |
| "The Streamable HTTP endpoint, or the legacy SSE endpoint exposed by official jumpserver/mcp deployments. Public HTTPS endpoints are supported by default. Private-network, Tailscale, and NetBird endpoints require OOMOL_CONNECT_ALLOW_PRIVATE_NETWORK. Loopback endpoints remain blocked. See https://github.com/jumpserver/mcp.", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/providers/jumpserver/definition.ts` around lines 21 - 27, Update the
placeholder in the MCP endpoint definition to use a `/mcp`-style HTTPS example
instead of the legacy `/sse` path, aligning it with the preferred Streamable
HTTP transport. Leave the surrounding label, description, and validation
settings unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.