Response time of tool call#1466
Conversation
dosubot
Bot
added
the
size:M
✅ 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExecution timing was added end-to-end. The server records Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/components/ToolsTab.tsx (1)
546-546:⚠️ Potential issue | 🟡 MinorElicitation response timing measures only the elicitation round-trip, not end-to-end.
Date.now()is passed asstartedAthere, sodurationMsinhandleExecutionResponsewill reflect only therespondToElicitationApicall — not the cumulative time from the original tool invocation. If the intent is to show total elapsed time, capture the originalexecutionStartTimeand thread it through the elicitation flow. If per-round-trip is the intent, this is fine but may surprise users.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/components/tools/ResultsPanel.tsx (1)
64-75: Time badge shares the "Valid" badge's green styling — consider a neutral variant.Both the validation "Valid" badge (line 53) and this timing badge use
bg-green-600. Green connotes success; timing is purely informational. A neutral color (e.g.,bg-blue-600or the default badge variant without color override) would avoid implying that a fast response is "good" and prevent visual conflation with validation status.♻️ Suggested diff
<Badge variant="default" - className="bg-green-600 hover:bg-green-700" + className="bg-blue-600 hover:bg-blue-700" >mcpjam-inspector/client/src/components/ToolsTab.tsx (1)
485-494: Network errors bypass duration capture.When
executeToolApithrows (e.g., timeout, DNS failure), thecatchblock sets the error message but never callssetResponseDurationMs. The user sees an error with no timing info. SinceexecutionStartTimeis already in scope, capturing the duration here would provide useful diagnostic signal even on failures.♻️ Suggested fix
} catch (err) { const message = err instanceof Error ? err.message : "Unknown error"; logger.error("Tool execution network error", { toolName: selectedTool, error: message, }); + setResponseDurationMs(Date.now() - executionStartTime); setError(message); } finally {
matteo8p
left a comment
matteo8p
left a comment
There was a problem hiding this comment.
Thanks for putting up a PR for this. I would love to have a feature that shows execution time.
Right now, it's being calculated on the client side React, which I don't think is accurate because it also accounts for time it takes for the Hono server to send it to the client.
Instead, a more accurate way of measuring latency would be to calculate the duration on the Hono server endpoint in /api/mcp/tools/execute, then pass the duration to the client.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
mcpjam-inspector/server/routes/mcp/tools.ts (2)
304-353:getExecutionDurationMs(context)called afterresetExecution— safe but worth a note.
resetExecutionremoves the context from theactiveExecutionsmap and clears its queue/waiter, but does not mutatestartedAtMs. Since you still hold a reference to thecontextobject, the duration calculation remains valid. This is correct, though a brief inline comment would save future readers from the same mental double-take.
367-370: NodurationMsin error responses — intentional?The
catchblock delegates tojsonError, which has no access tocontext.startedAtMs. If timing data on failures would aid debugging (e.g., distinguishing timeouts from instant rejections), consider threadingdurationMsinto the error payload. Not blocking — just a thought for observability.mcpjam-inspector/client/src/components/ToolsTab.tsx (1)
408-437:responseDurationMsnot set on thetask_createdpath — acceptable since the view navigates away.The
window.location.hash = "tasks"redirect at line 435 means theResultsPanel(which displays duration) is never rendered for this case. If the UX ever changes to remain on the tools panel after task creation, this would need revisiting.
EsshUwU
force-pushed
the
response-time
branch
from
93c0861 to
ca708a6
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/components/ToolsTab.tsx (1)
408-437:⚠️ Potential issue | 🟡 Minor
setResponseDurationMsnot called in thetask_createdbranch.Every other branch (
completed,elicitation_required,error) updatesresponseDurationMs, buttask_createdonly logs the duration and then navigates away. Ifwindow.location.hash = "tasks"is ever removed or delayed, the state will be stale. A one-liner for symmetry:Proposed fix
if ("status" in response && response.status === "task_created") { const { task, modelImmediateResponse } = response; + setResponseDurationMs(durationMs); // Track the task locally so it appears in the Tasks tab🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/ToolsTab.tsx` around lines 408 - 437, The task_created branch fails to update the component state with the response duration; call setResponseDurationMs(durationMs ?? undefined) before navigating away so responseDurationMs matches other branches; modify the block handling ("status" in response && response.status === "task_created")—where task, modelImmediateResponse, logger.info, and window.location.hash are used—to invoke setResponseDurationMs with the same durationMs value used in the log just prior to setting window.location.hash = "tasks".mcpjam-inspector/server/routes/mcp/tools.ts (1)
304-353:⚠️ Potential issue | 🟡 Minor
durationMsabsent from error responses — intentional?In the
/executecatch block (Line 368) and/respondcatch block (Line 434),jsonErrordoes not includedurationMs. Every success and elicitation path carries timing data, yet failures—arguably the most diagnostically interesting cases—do not. If the client wants to display duration regardless of outcome, this omission will surface asnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/mcp/tools.ts` around lines 304 - 353, The error responses from the /execute and /respond catch blocks omit timing info; update the jsonError payloads (the calls to jsonError in the catch handlers for these routes) to include durationMs: getExecutionDurationMs(context) so error responses mirror success paths and elicitation responses; locate and modify the catch blocks that call jsonError to add a durationMs field derived via getExecutionDurationMs(context).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@mcpjam-inspector/client/src/components/ToolsTab.tsx`:
- Around line 408-437: The task_created branch fails to update the component
state with the response duration; call setResponseDurationMs(durationMs ??
undefined) before navigating away so responseDurationMs matches other branches;
modify the block handling ("status" in response && response.status ===
"task_created")—where task, modelImmediateResponse, logger.info, and
window.location.hash are used—to invoke setResponseDurationMs with the same
durationMs value used in the log just prior to setting window.location.hash =
"tasks".
In `@mcpjam-inspector/server/routes/mcp/tools.ts`:
- Around line 304-353: The error responses from the /execute and /respond catch
blocks omit timing info; update the jsonError payloads (the calls to jsonError
in the catch handlers for these routes) to include durationMs:
getExecutionDurationMs(context) so error responses mirror success paths and
elicitation responses; locate and modify the catch blocks that call jsonError to
add a durationMs field derived via getExecutionDurationMs(context).
EsshUwU
force-pushed
the
response-time
branch
from
434c45f to
8c23e7e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcpjam-inspector/server/routes/mcp/tools.ts (1)
367-370: Error responses fromjsonErroromitdurationMs.The client's
handleExecutionResponseis wired to extract and displaydurationMson error paths (ToolsTab.tsx, line 438), butjsonErrornever includes it. If you'd like duration visible even on failures, a small addition suffices:Suggested enhancement
} catch (error) { resetExecution(context, () => manager.clearElicitationHandler(serverId)); - return jsonError(c, error, 500); + const details = serializeMcpError(error); + const status = + typeof (error as any)?.status === "number" + ? (error as any).status + : 500; + return c.json( + { + error: details.message as string, + mcpError: details, + durationMs: getExecutionDurationMs(context), + }, + status, + ); }The same pattern applies to the
/respondcatch block (line 434–439).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/mcp/tools.ts` around lines 367 - 370, The error responses returned by the catch blocks call jsonError without including durationMs, so the client's handleExecutionResponse (ToolsTab.tsx) can't show execution duration on failure; compute durationMs (e.g. Date.now() - (context.executionStart ?? Date.now())) inside those catch blocks and pass it into jsonError so the response includes { durationMs } (update the two locations that call jsonError in the catch handlers that also call resetExecution/manager.clearElicitationHandler and the /respond catch block).mcpjam-inspector/client/src/components/ToolsTab.tsx (1)
406-434:responseDurationMsintentionally not set fortask_created— worth a brief comment.Since
window.location.hash = "tasks"navigates the user away immediately, omittingsetResponseDurationMsis correct. A one-line comment clarifying "no need to set duration — navigating away" would prevent a future contributor from adding it "just to be consistent."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/ToolsTab.tsx` around lines 406 - 434, Add a one-line comment explaining why response duration (e.g., responseDurationMs / durationMs) is intentionally not set for the "task_created" branch to avoid future confusion; locate the block handling response.status === "task_created" (inside the same conditional that calls trackTask and logger.info and then sets window.location.hash = "tasks") and insert a brief comment like "no need to set response duration because we navigate away immediately" immediately before the navigation/return so future contributors understand the omission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/components/ToolsTab.tsx`:
- Around line 406-434: Add a one-line comment explaining why response duration
(e.g., responseDurationMs / durationMs) is intentionally not set for the
"task_created" branch to avoid future confusion; locate the block handling
response.status === "task_created" (inside the same conditional that calls
trackTask and logger.info and then sets window.location.hash = "tasks") and
insert a brief comment like "no need to set response duration because we
navigate away immediately" immediately before the navigation/return so future
contributors understand the omission.
In `@mcpjam-inspector/server/routes/mcp/tools.ts`:
- Around line 367-370: The error responses returned by the catch blocks call
jsonError without including durationMs, so the client's handleExecutionResponse
(ToolsTab.tsx) can't show execution duration on failure; compute durationMs
(e.g. Date.now() - (context.executionStart ?? Date.now())) inside those catch
blocks and pass it into jsonError so the response includes { durationMs }
(update the two locations that call jsonError in the catch handlers that also
call resetExecution/manager.clearElicitationHandler and the /respond catch
block).
3 participants
Added response time header in the result panel