Open
Conversation
dosubot
bot
added
the
size:L
Contributor
Author
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
WalkthroughThis PR converts the 📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/sdk/reference/mcp-client-manager.mdx (1)
188-199:⚠️ Potential issue | 🟡 MinorUpdate the
pingServersignature docs to match the real API.The reference still omits the optional
optionsargument and hardcodes a{}return, while implementation forwardsClient.ping()output.Suggested docs fix
-pingServer(serverId: string): Promise<{}> +pingServer( + serverId: string, + options?: RequestOptions +): Promise<Awaited<ReturnType<Client["ping"]>>>-`Promise<{}>` - Resolves when the server responds to the ping request. +`Promise<Awaited<ReturnType<Client["ping"]>>>` - Resolves with the server ping response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/sdk/reference/mcp-client-manager.mdx` around lines 188 - 199, Update the docs for pingServer to reflect the real API: add the optional second parameter (e.g., options?: ...) to the signature and change the return type from Promise<{}> to the actual type forwarded from Client.ping() (i.e., whatever type Client.ping() returns) so the docs match the implementation that forwards Client.ping() output; ensure the Parameters and Returns sections describe the optional options argument and the concrete return shape used by Client.ping().
🧹 Nitpick comments (3)
mcpjam-inspector/server/routes/mcp/__tests__/servers.test.ts (1)
135-136: Optional: validate timestamp format, not just type.
typeof "string"passes invalid timestamps; parsing check makes this stricter.Optional test hardening
expect(typeof data.latencyMs).toBe("number"); expect(typeof data.checkedAt).toBe("string"); + expect(Number.isNaN(Date.parse(data.checkedAt))).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/mcp/__tests__/servers.test.ts` around lines 135 - 136, The test currently only asserts that data.checkedAt is a string; strengthen it by validating the timestamp format: parse data.checkedAt (e.g., Date.parse or new Date(data.checkedAt)) and assert that the result is a valid date (not NaN) so malformed strings fail; update the assertions around data.checkedAt in the test (alongside the existing check for data.latencyMs) to assert a valid timestamp rather than just typeof "string".mcpjam-inspector/server/routes/mcp/servers.ts (2)
72-87: Consider backoff signaling for 503 health responses.Since this route is polled, adding
Retry-After(and possibly downgrading expected ping failures towarn) can reduce noisy retries and log flood during outages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/mcp/servers.ts` around lines 72 - 87, The catch block in the servers route currently logs errors at error level and returns a 503 without backoff headers; update the error handling inside the catch for the route (the block that calls logger.error and returns c.json) to (1) set a Retry-After header on the response (e.g., c.header("Retry-After", "<seconds>") or equivalent) so clients polling back off when receiving 503, and (2) downgrade noisy transient failures to logger.warn where appropriate (or log both warn plus full error details at debug) while preserving the existing error details in the JSON body (fields like serverId, connectionStatus via getSafeConnectionStatus, healthStatus, latencyMs, checkedAt, and error). Ensure the header is added before returning the c.json response and keep serverId/getSafeConnectionStatus usage unchanged.
11-22: Don’t swallow connection-status errors here.
getConnectionStatusis a pure state lookup; broad catch/fallback to"disconnected"can hide real regressions.Suggested simplification
-function getSafeConnectionStatus( +function getConnectionStatus( mcpClientManager: { getConnectionStatus: (serverId: string) => string; }, serverId: string, ): string { - try { - return mcpClientManager.getConnectionStatus(serverId); - } catch { - return "disconnected"; - } + return mcpClientManager.getConnectionStatus(serverId); }- connectionStatus: getSafeConnectionStatus(mcpClientManager, serverId), + connectionStatus: getConnectionStatus(mcpClientManager, serverId),- ? getSafeConnectionStatus(c.mcpClientManager, serverId) + ? getConnectionStatus(c.mcpClientManager, serverId)Also applies to: 67-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/mcp/servers.ts` around lines 11 - 22, The current getSafeConnectionStatus function swallows all errors from mcpClientManager.getConnectionStatus and returns "disconnected", hiding real bugs; remove the broad try/catch (or narrow it to only handle a specific known error type and rethrow anything else) so that getConnectionStatus errors propagate instead of being converted to "disconnected"; apply the same fix to the analogous block around the code referenced at lines 67-80 so both places use direct mcpClientManager.getConnectionStatus(serverId) (or catch and rethrow unknown errors) rather than masking failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/evals/brightdata/brightdata-unit-test.test.ts`:
- Around line 75-78: The test "SDK Feature: pingServer - verifies server is
responsive" currently asserts an exact empty object from
clientManager.pingServer("brightdata-ecommerce") which over-constrains the
liveness check; change the assertion to simply verify a successful/responsive
result (for example using resolves.toBeDefined() or resolves.not.toBeNull() or
resolves.toBeTruthy()) so the test only checks server responsiveness and not the
exact payload shape.
In `@mcpjam-inspector/client/src/components/connection/ServerConnectionCard.tsx`:
- Around line 214-245: The polling logic allows overlapping calls because
handleHealthCheck increments healthCheckRequestIdRef on every tick even if a
previous getServerHealth is still in-flight; update handleHealthCheck (and the
interval setup that schedules it) to gate new requests when a check is already
running by checking the in-flight state (use isCheckingHealth or a dedicated ref
like isCheckingRef) and return early if true, or replace the fixed setInterval
with a recursive setTimeout that schedules the next call only after the current
getServerHealth completes; ensure healthCheckRequestIdRef, setIsCheckingHealth,
getServerHealth and the interval/timeout scheduler (the code around the existing
useEffect that sets the 30s poll) are updated accordingly so only one network
call is active at a time.
In `@mcpjam-inspector/client/src/lib/apis/mcp-servers-api.ts`:
- Around line 41-48: The current code casts any parsed JSON to
ServerHealthResponse and returns it, allowing arbitrary responses (e.g. {error:
...}) to slip through; instead validate the parsed object shape before returning
it. In the code that reads into the local variable body and returns it, check
that body is an object and has the expected ServerHealthResponse properties
(e.g. a boolean success and the other required fields your type expects) and
only return body when those checks pass; otherwise return the synthetic
unhealthy ServerHealthResponse fallback (the same shape your callers expect) so
ServerConnectionCard receives a deterministic response. Use the existing
variable name body and the ServerHealthResponse type to locate where to add the
validation and fallback logic.
In `@sdk/src/mcp-client-manager/MCPClientManager.ts`:
- Around line 701-707: pingServer currently calls client.ping immediately and
can throw "is not connected" if the client's connection promise (state.promise)
is still pending; fix by awaiting the client's connection/ensureConnected before
pinging: in pingServer (use getClientOrThrow to get client), call or await the
client's ensureConnected() or the internal connection promise (the same
mechanism used by ensureConnected) prior to invoking
client.ping(this.withTimeout(...)) so the health check waits for the client to
be connected.
---
Outside diff comments:
In `@docs/sdk/reference/mcp-client-manager.mdx`:
- Around line 188-199: Update the docs for pingServer to reflect the real API:
add the optional second parameter (e.g., options?: ...) to the signature and
change the return type from Promise<{}> to the actual type forwarded from
Client.ping() (i.e., whatever type Client.ping() returns) so the docs match the
implementation that forwards Client.ping() output; ensure the Parameters and
Returns sections describe the optional options argument and the concrete return
shape used by Client.ping().
---
Nitpick comments:
In `@mcpjam-inspector/server/routes/mcp/__tests__/servers.test.ts`:
- Around line 135-136: The test currently only asserts that data.checkedAt is a
string; strengthen it by validating the timestamp format: parse data.checkedAt
(e.g., Date.parse or new Date(data.checkedAt)) and assert that the result is a
valid date (not NaN) so malformed strings fail; update the assertions around
data.checkedAt in the test (alongside the existing check for data.latencyMs) to
assert a valid timestamp rather than just typeof "string".
In `@mcpjam-inspector/server/routes/mcp/servers.ts`:
- Around line 72-87: The catch block in the servers route currently logs errors
at error level and returns a 503 without backoff headers; update the error
handling inside the catch for the route (the block that calls logger.error and
returns c.json) to (1) set a Retry-After header on the response (e.g.,
c.header("Retry-After", "<seconds>") or equivalent) so clients polling back off
when receiving 503, and (2) downgrade noisy transient failures to logger.warn
where appropriate (or log both warn plus full error details at debug) while
preserving the existing error details in the JSON body (fields like serverId,
connectionStatus via getSafeConnectionStatus, healthStatus, latencyMs,
checkedAt, and error). Ensure the header is added before returning the c.json
response and keep serverId/getSafeConnectionStatus usage unchanged.
- Around line 11-22: The current getSafeConnectionStatus function swallows all
errors from mcpClientManager.getConnectionStatus and returns "disconnected",
hiding real bugs; remove the broad try/catch (or narrow it to only handle a
specific known error type and rethrow anything else) so that getConnectionStatus
errors propagate instead of being converted to "disconnected"; apply the same
fix to the analogous block around the code referenced at lines 67-80 so both
places use direct mcpClientManager.getConnectionStatus(serverId) (or catch and
rethrow unknown errors) rather than masking failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14aa7f08-57c4-4fc8-b415-f0a44faa3754
📒 Files selected for processing (14)
docs/sdk/concepts/connecting-servers.mdxdocs/sdk/reference/mcp-client-manager.mdxexamples/evals/brightdata/brightdata-unit-test.test.tsmcpjam-inspector/client/src/components/connection/ServerConnectionCard.tsxmcpjam-inspector/client/src/components/connection/__tests__/ServerConnectionCard.featureFlags.test.tsxmcpjam-inspector/client/src/components/connection/__tests__/ServerConnectionCard.hosted.test.tsxmcpjam-inspector/client/src/components/connection/__tests__/ServerConnectionCard.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-chat-session.minimal-mode.test.tsxmcpjam-inspector/client/src/lib/apis/mcp-servers-api.tsmcpjam-inspector/server/routes/mcp/__tests__/servers.test.tsmcpjam-inspector/server/routes/mcp/servers.tsmcpjam-inspector/server/routes/web/__tests__/chat-v2.guest.test.tssdk/src/mcp-client-manager/MCPClientManager.tssdk/tests/MCPClientManager.test.ts
Comment on lines
+75
to
+78
| test("SDK Feature: pingServer - verifies server is responsive", async () => { | ||
| await expect( | ||
| clientManager.pingServer("brightdata-ecommerce") | ||
| ).resolves.toEqual({}); |
Contributor
There was a problem hiding this comment.
Avoid over-constraining ping payload in this liveness test.
This test verifies responsiveness; asserting exact {} couples it to payload shape and can fail on harmless protocol changes.
Proposed adjustment
- await expect(
- clientManager.pingServer("brightdata-ecommerce")
- ).resolves.toEqual({});
+ await expect(
+ clientManager.pingServer("brightdata-ecommerce", { timeout: 5000 })
+ ).resolves.toEqual(expect.any(Object));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/evals/brightdata/brightdata-unit-test.test.ts` around lines 75 - 78,
The test "SDK Feature: pingServer - verifies server is responsive" currently
asserts an exact empty object from
clientManager.pingServer("brightdata-ecommerce") which over-constrains the
liveness check; change the assertion to simply verify a successful/responsive
result (for example using resolves.toBeDefined() or resolves.not.toBeNull() or
resolves.toBeTruthy()) so the test only checks server responsiveness and not the
exact payload shape.
Comment on lines
+214
to
+245
| const handleHealthCheck = useCallback(async () => { | ||
| if (!showHealthStatus) { | ||
| return; | ||
| } | ||
|
|
||
| const requestId = ++healthCheckRequestIdRef.current; | ||
| setIsCheckingHealth(true); | ||
|
|
||
| try { | ||
| const result = await getServerHealth(server.name); | ||
| if (healthCheckRequestIdRef.current !== requestId) { | ||
| return; | ||
| } | ||
| setServerHealth(result); | ||
| } catch (error) { | ||
| if (healthCheckRequestIdRef.current !== requestId) { | ||
| return; | ||
| } | ||
| setServerHealth({ | ||
| success: false, | ||
| serverId: server.name, | ||
| connectionStatus: server.connectionStatus, | ||
| healthStatus: "unhealthy", | ||
| checkedAt: new Date().toISOString(), | ||
| error: error instanceof Error ? error.message : "Health check failed", | ||
| }); | ||
| } finally { | ||
| if (healthCheckRequestIdRef.current === requestId) { | ||
| setIsCheckingHealth(false); | ||
| } | ||
| } | ||
| }, [server.connectionStatus, server.name, showHealthStatus]); |
Contributor
There was a problem hiding this comment.
Prevent overlapping poll requests.
The interval fires every 30s regardless of whether the previous getServerHealth() call is still running. Once a check takes longer than the poll interval, each tick increments healthCheckRequestIdRef, so completed responses are discarded as stale and the badge can stay stuck in "checking" while more pings pile up. Please gate the poll on in-flight state, or switch this loop to recursive setTimeout scheduling after each completion.
Also applies to: 273-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/connection/ServerConnectionCard.tsx`
around lines 214 - 245, The polling logic allows overlapping calls because
handleHealthCheck increments healthCheckRequestIdRef on every tick even if a
previous getServerHealth is still in-flight; update handleHealthCheck (and the
interval setup that schedules it) to gate new requests when a check is already
running by checking the in-flight state (use isCheckingHealth or a dedicated ref
like isCheckingRef) and return early if true, or replace the fixed setInterval
with a recursive setTimeout that schedules the next call only after the current
getServerHealth completes; ensure healthCheckRequestIdRef, setIsCheckingHealth,
getServerHealth and the interval/timeout scheduler (the code around the existing
useEffect that sets the 30s poll) are updated accordingly so only one network
call is active at a time.
Comment on lines
+41
to
+48
| let body: ServerHealthResponse | null = null; | ||
| try { | ||
| body = (await res.json()) as ServerHealthResponse; | ||
| } catch {} | ||
|
|
||
| if (body) { | ||
| return body; | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t trust arbitrary JSON as ServerHealthResponse.
This returns any truthy JSON object. A proxy/auth failure that comes back as { error: ... } will slip through here, and ServerConnectionCard will render the neutral "Ping pending" state because success is undefined. Please validate the shape before returning it, and otherwise fall back to the synthetic unhealthy response.
Suggested fix
- let body: ServerHealthResponse | null = null;
+ let body: unknown = null;
try {
- body = (await res.json()) as ServerHealthResponse;
+ body = await res.json();
} catch {}
- if (body) {
- return body;
+ if (
+ typeof body === "object" &&
+ body !== null &&
+ "success" in body &&
+ "serverId" in body &&
+ "connectionStatus" in body &&
+ "healthStatus" in body &&
+ "checkedAt" in body
+ ) {
+ return body as ServerHealthResponse;
}📝 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
| let body: ServerHealthResponse | null = null; | |
| try { | |
| body = (await res.json()) as ServerHealthResponse; | |
| } catch {} | |
| if (body) { | |
| return body; | |
| } | |
| let body: unknown = null; | |
| try { | |
| body = await res.json(); | |
| } catch {} | |
| if ( | |
| typeof body === "object" && | |
| body !== null && | |
| "success" in body && | |
| "serverId" in body && | |
| "connectionStatus" in body && | |
| "healthStatus" in body && | |
| "checkedAt" in body | |
| ) { | |
| return body as ServerHealthResponse; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/apis/mcp-servers-api.ts` around lines 41 -
48, The current code casts any parsed JSON to ServerHealthResponse and returns
it, allowing arbitrary responses (e.g. {error: ...}) to slip through; instead
validate the parsed object shape before returning it. In the code that reads
into the local variable body and returns it, check that body is an object and
has the expected ServerHealthResponse properties (e.g. a boolean success and the
other required fields your type expects) and only return body when those checks
pass; otherwise return the synthetic unhealthy ServerHealthResponse fallback
(the same shape your callers expect) so ServerConnectionCard receives a
deterministic response. Use the existing variable name body and the
ServerHealthResponse type to locate where to add the validation and fallback
logic.
Comment on lines
+701
to
+707
| async pingServer( | ||
| serverId: string, | ||
| options?: RequestOptions | ||
| ): Promise<Awaited<ReturnType<Client["ping"]>>> { | ||
| const client = this.getClientOrThrow(serverId); | ||
| try { | ||
| client.ping(options); | ||
| return await client.ping(this.withTimeout(serverId, options)); |
Contributor
There was a problem hiding this comment.
Await the connection promise before pinging.
pingServer() is now a public async API, but it still skips ensureConnected(). If the server is registered and state.promise is still resolving, this throws is not connected instead of waiting, so the new health check can fail spuriously during startup or reconnect.
Suggested fix
async pingServer(
serverId: string,
options?: RequestOptions
): Promise<Awaited<ReturnType<Client["ping"]>>> {
- const client = this.getClientOrThrow(serverId);
try {
+ await this.ensureConnected(serverId);
+ const client = this.getClientOrThrow(serverId);
return await client.ping(this.withTimeout(serverId, options));
} catch (error) {
throw new Error(📝 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
| async pingServer( | |
| serverId: string, | |
| options?: RequestOptions | |
| ): Promise<Awaited<ReturnType<Client["ping"]>>> { | |
| const client = this.getClientOrThrow(serverId); | |
| try { | |
| client.ping(options); | |
| return await client.ping(this.withTimeout(serverId, options)); | |
| async pingServer( | |
| serverId: string, | |
| options?: RequestOptions | |
| ): Promise<Awaited<ReturnType<Client["ping"]>>> { | |
| try { | |
| await this.ensureConnected(serverId); | |
| const client = this.getClientOrThrow(serverId); | |
| return await client.ping(this.withTimeout(serverId, options)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/mcp-client-manager/MCPClientManager.ts` around lines 701 - 707,
pingServer currently calls client.ping immediately and can throw "is not
connected" if the client's connection promise (state.promise) is still pending;
fix by awaiting the client's connection/ensureConnected before pinging: in
pingServer (use getClientOrThrow to get client), call or await the client's
ensureConnected() or the internal connection promise (the same mechanism used by
ensureConnected) prior to invoking client.ping(this.withTimeout(...)) so the
health check waits for the client to be connected.
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.
Summary
pingServerawait the MCP ping response and expose structured health data from the inspector server routeServerConnectionCardto show live ping health, latency, and manual refresh for connected local serversTesting
npx vitest run client/src/components/connection/__tests__/ServerConnectionCard.test.tsx client/src/components/connection/__tests__/ServerConnectionCard.featureFlags.test.tsx client/src/components/connection/__tests__/ServerConnectionCard.hosted.test.tsx server/routes/mcp/__tests__/servers.test.tsnpx jest tests/MCPClientManager.test.ts -t "should ping the HTTP server"Note
Medium Risk
Changes
MCPClientManager.pingServerfrom sync to async and wires it into a new inspector health endpoint and UI polling, which can affect existing call sites and introduces periodic network requests.Overview
Adds async MCP ping support by making
MCPClientManager.pingServerawait the underlying client ping and updating docs/examples/tests toawaitit.Introduces a server health check API in the inspector (
GET /api/mcp/servers/status/:serverId) that runs a timed ping, returns structured health/latency/checked-at data, and uses503on failures.Updates the inspector UI (
ServerConnectionCard) to poll and display live ping health/latency for connected local servers, with a manual refresh action and accompanying test coverage plus a small client API (getServerHealth) to call the route.Written by Cursor Bugbot for commit 1d6ad54. This will update automatically on new commits. Configure here.