[UI-REWRITE]: Add virtual server component tool testing - #90
gandhipratik203 wants to merge 8 commits into
Conversation
724ab19 to
c5de120
Compare
marekdano
left a comment
There was a problem hiding this comment.
Findings
🟠 Accessibility
1. High — src/components/gateways/VirtualServerDetailsPanel.tsx:965
No focus management on tool-test view swap. Entering/leaving the new tool-test view moves no focus and announces nothing to assistive tech. Clicking Test swaps the components list for "Tool test" with no heading, aria-live region, or focus move. Clicking Back to components drops focus to <body> instead of returning it to the row's "Actions for X" trigger. src/components/server-catalog/CatalogResults.tsx already solves this exact pattern with an actionsTriggerRef.current?.focus() call on close — this PR didn't reuse it.
2. Medium — src/components/tools/ToolTryItTab.tsx:192
Disabled switch reason not linked via aria-describedby. The disabled-reason text for the "Live invocation" switch is a plain sibling <p>, not associated with the control. A screen-reader user tabbing to the disabled switch (e.g. missing tools.execute) hears only "Live invocation, switch, dimmed" — the adjacent explanation ("Live invoke requires tools.execute.") is never announced.
🟡 Test Coverage
3. Medium — src/components/gateways/VirtualServerDetailsPanel.tsx:161
normalizePanelTool's snake_case fallback branches untested. The snake_case fallback branches (gateway_slug, display_name, original_name, input_schema, output_schema) are never exercised — every test fixture uses camelCase makeTool(). If the scoped-tools endpoint ever actually sends snake_case (the whole reason this fallback exists), a regression ships silently — blank display names, empty input schemas.
4. Low-Medium — src/components/tools/ToolTryItTab.tsx:118
No test for live-mode toggle-off or loading state. No test toggles Live invocation on and back off, or exercises the permissionsLoading ("checkingAccess") state disabling the new Switch. A regression in handleLiveModeChange's reset of snippetLanguage/preview/invoke state on the live→preview transition, or in the Switch's disabled wiring while permissions are still loading, wouldn't be caught.
5. Low — src/components/tools/ToolLiveInvokeResult.tsx:129
Only 1 of 8 gateway-name fallback branches covered. Only target.gateway_name is covered by ToolLiveInvokeResult.test.tsx. The other 7 (root-level
gateway_name/gatewayName/resolved_gateway_name/resolvedGatewayName, target.gatewaySlug/gateway_slug) are untested.
🔵 Code Quality / Cleanup
6. Medium — src/components/gateways/VirtualServerDetailsPanel.tsx:155
Casing-normalization pattern duplicated across 4 files. normalizePanelTool hand-reads camelCase/snake_case field pairs to paper over backend casing inconsistency — the same ad-hoc pattern is already duplicated in CreateServer.tsx, ServerCatalog.tsx, and src/api/search.ts. Belongs once in a shared response-normalization layer, not re-implemented per component.
7. Low — src/components/gateways/VirtualServerDetailsPanel.tsx:182
getNonEmptyString duplicated verbatim in two new files. Duplicated in this file and in src/components/tools/ToolLiveInvokeResult.tsx:145, both new in this PR. Risk of the two copies silently drifting on a future change to "non-empty" semantics.
8. Low — src/components/gateways/VirtualServerDetailsPanel.tsx:984
getToolLabel prop passed but dead without onSelectTool. getToolLabel={getFriendlyToolLabel} is passed into ToolTryItTab without onSelectTool, so it's dead: toolLabel() is only invoked inside the onSelectTool && branch, which can never render from this call site.
9. Low — src/components/tools/ToolLiveInvokeResult.tsx:129
8-way ?? chain instead of iterating candidate keys. getBackingGatewayName chains 8 explicit fallbacks over field-name variants instead of iterating a list of candidate keys — hard to audit, easy to miss a branch during review.
10. Low — src/components/gateways/VirtualServerDetailsPanel.tsx:753
O(n·m) find() inside render loop over memoized array. testableTool is computed with fetchedTools.find() inside the visibleComponents.map() render loop, where fetchedTools (already memoized via useMemo, line 318) could be indexed once into a Map for O(1) lookups. Low real-world impact, but a free fix.
|
Addressed the review in Kept normalization local to virtual-server tools. The tiny string guard and explicit gateway-name precedence remain unchanged to avoid widening this PR. Local tests and GitHub checks pass. Please re-review. |
marekdano
left a comment
There was a problem hiding this comment.
Findings
🔴 High: Misleading success indicator on failed invoke
File: src/components/tools/ToolLiveInvokeResult.tsx:42
"Answered by {gateway}" renders from a static context value regardless of outcome.
// VirtualServerToolTestView always passes:
resultContext={{ backingGatewayName: tool.gatewaySlug || undefined }}
// ToolLiveInvokeResult.tsx:
const backingGatewayName = context?.backingGatewayName ?? getBackingGatewayName(response);backingGatewayName prefers the static context value unconditionally, so if a scoped live invoke fails (network error, JSON-RPC error, timeout), hasRun is true and result is null, but the panel still renders "Answered by github-mcp" right next to the error message — implying the tool call succeeded when it didn't.
Suggested fix: only derive backingGatewayName from context when the invoke actually succeeded, falling back to getBackingGatewayName(response) only on success.
🔴 High: Federated-tool gating bypassed by empty string
File: src/components/gateways/normalizeVirtualServerTool.ts:13
gatewayId: tool.gatewayId ?? nonEmptyString(record.gateway_id) ?? null,Every sibling field (displayName, originalName, gatewaySlug) routes through nonEmptyString() first. gatewayId does not.
If the backend ever sends gatewayId: "" instead of null/undefined, isFederated = Boolean(tool.gatewayId) in resolveToolLiveInvokeAvailability (ToolLiveInvokeGate.tsx) evaluates to false. A destructive federated tool then falls through to the requiresConfirmation branch (live-invokable with just a confirm dialog) instead of the intended unavailableFederated block.
Suggested fix: nonEmptyString(tool.gatewayId) ??nonEmptyString(record.gateway_id) ?? null to match the other fields.
🟡 Medium: New i18n key added but never wired up
File: src/components/gateways/VirtualServerDetailsPanel.tsx:744
The key gateways.details.loadingComponents was added to en-US, es-ES, and pt-BR locale files, but the component still renders the hardcoded literal:
<span>Loading components...</span>instead of:
<span>{intl.formatMessage({ id: "gateways.details.loadingComponents" })}</span>es-ES/pt-BR users viewing the Components tab while tools/resources/prompts load see the untranslated English string.
vishu-bh
left a comment
There was a problem hiding this comment.
Please address the two inline findings before merging: closing the virtual-server drawer leaves an active live invocation running without cancellation, and the new live-mode label should use the shared UI Label component. The lifecycle issue was reproduced with the real panel/hooks: closing sent no cancellation, while unmounting aborted the request and sent cancellation. Add coverage for closing during an active invocation.
| ].map((source, index, sources) => { | ||
| const isSelected = sourceFilter === source.id; | ||
| const tabButton = ( | ||
| {virtualServerToolTryItEnabled && selectedTestTool ? ( |
There was a problem hiding this comment.
[P2] Cancel active invocation when the drawer closes
This tool-test view remains mounted when open becomes false. Closing the drawer or pressing Escape hides the cancellation control, but the invocation continues: the close reset effect returns early on !open, so the invoke hook never runs its unmount cancellation. Reproduced with the real panel/hooks and a pending invocation: closing left the signal un-aborted and sent zero cancellation calls; unmounting aborted it and sent cancellation. Clear the selected test or unmount/reset this view on close, and add a regression test for closing during an active invocation.
There was a problem hiding this comment.
Fixed in e001097. Closing the drawer now unmounts the tool-test view, aborting the active request and sending cancellation. Tests cover both the close button and Escape.
| {scopedMode && ( | ||
| <div className="flex flex-wrap items-start justify-between gap-4 border-y border-border py-3"> | ||
| <div className="space-y-1"> | ||
| <label |
There was a problem hiding this comment.
[P3] Use the shared Label component
The new live-invocation label uses a raw <label> even though components/ui/label already provides the shared Label primitive. Please use that component here to follow the shared UI component convention and retain consistent styling and accessibility behavior.
There was a problem hiding this comment.
Fixed in e001097. The Live switch now uses the shared Label component, with its accessible association preserved.
|
Addressed the follow-up reviews in |
|
@gandhipratik203 - please rebase this branch on |
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
e001097 to
19f4315
Compare
|
Rebased the branch onto the latest main. |
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
| ); | ||
| } | ||
|
|
||
| function getBackingGatewayName(response: ToolPreviewResponse | undefined): string | undefined { |
There was a problem hiding this comment.
Every fallback in this function is unreachable against the deployed backend, so it always returns undefined.
The /rpc tools/call result is a CallToolResult (mcpgateway/common/models.py:659, aliased to ToolResult at 682), serialized with model_dump(by_alias=True, exclude_none=True). The keys that reach the client are content, isError, structuredContent and _meta. None of them carry a target or a source name.
"Answered by X" renders only from the context value the panel supplies: resultContext={{ backingGatewayName: tool.gatewaySlug }}. That is the client repeating what it already stored on the tool record, not a report of which source handled the call. If the backend resolves the call to a different tool, the line still prints tool.gatewaySlug.
Suggested change: remove this function and the response-parsing path, and to hold the "Answered by" line until [IBM/mcp-context-forge#6416](IBM/mcp-context-forge#6416) adds a field for it.
Note: _meta is not a safe carrier. It is settable by the upstream source, so it cannot be relied on to report which source answered, and the plugin post-invoke path in tool_service.py rebuilds ToolResult without meta, so it is lost whenever a post-invoke plugin rewrites the result.
| `/v1/tools/preview/${encodeURIComponent(validName)}`, | ||
| { | ||
| arguments: args, | ||
| ...(options.serverId ? { server_id: options.serverId } : {}), |
There was a problem hiding this comment.
The deployed backend drops server_id. ToolPreviewRequest (mcpgateway/schemas.py:2103) declares only arguments, and BaseModelWithConfigDict sets extra="ignore" (mcpgateway/utils/base_models.py:96).
A scoped preview returns 200 with a target resolved without any virtual server scoping. The response looks correct and there is no error to notice, so the failure is quiet.
Worth guarding on the client, or at least noting at this call site, since the only thing preventing it is leaving VITE_ENABLE_VIRTUAL_SERVER_TOOL_TRY_IT off until [IBM/mcp-context-forge#6743](IBM/mcp-context-forge#6743) lands.
| if (permissionsLoading) return { state: "checkingAccess" }; | ||
| if (!canExecute) return { state: "missingPermission", permission: "tools.execute" }; | ||
| if (!canUseServers) return { state: "missingPermission", permission: "servers.use" }; | ||
| if (invalidGatewayId || (typeof tool.gatewayId === "string" && !tool.gatewayId.trim())) { |
There was a problem hiding this comment.
Worth checking how often the live path is reachable for virtual server components before more work goes into it.
Any component fetched from an attached MCP server carries a gatewayId, so isFederated at line 44 is true for those rows. From there the only route to available is readOnlyHint at line 52. A federated tool with destructiveHint returns unavailableFederated at line 49, and a federated tool with no annotations returns unavailableFederated at line 53.
Upstream MCP servers frequently send no annotations, so the Live invocation switch will be disabled on most component rows. Neither #6416 nor #6743 changes that, since the gate turns on annotations and on #5437.
The manual mock in the description sets annotations: { readOnlyHint: true } on its only tool, so the documented verification steps all exercise the case that resolves to available. Adding a tool with no annotations to the mock would show what most rows do.
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Refs IBM/mcp-context-forge#6417
Design: IBM/mcp-context-forge#6710
Backend rollout dependencies: IBM/mcp-context-forge#6416, IBM/mcp-context-forge#6743
Summary
server_idin scoped preview requests, live JSON-RPC calls, and generated snippets while preserving direct Tools behavior.associatedToolIdsdisplay-only so incomplete tool records cannot be invoked.Feature flag
VITE_ENABLE_VIRTUAL_SERVER_TOOL_TRY_IT=falseby default. When disabled, no component Test action or scoped tool invocation UI is exposed.Integration and rollout
This PR implements the frontend only. The unit, Playwright, and manual mock tests verify the UI, scoped request payloads, permission states, and result rendering. They do not verify execution against a real backend.
Live invocation sends qualified
tool.nameandparams.server_idto/rpcand uses the existing/rpccancellation path. Scoped preview sendsserver_idto/v1/tools/preview/{name}. Backend contract work is tracked by IBM/mcp-context-forge#6416 and IBM/mcp-context-forge#6743.This frontend PR can be reviewed and merged independently of that backend work. Keep
VITE_ENABLE_VIRTUAL_SERVER_TOOL_TRY_IT=falseuntil the backend contracts are implemented and scoped preview/live flows pass real-backend integration tests. The backend issues block enabling the feature, not merging this frontend implementation.Review follow-up
Verification
npm test -- --reporter=dot(210 files, 3,457 passed, 1 skipped)npm run lintnpm run buildnpm run format:checknpm run e2e -- e2e/tools.spec.ts(38 passed)VITE_ENABLE_VIRTUAL_SERVER_TOOL_TRY_IT=true npm run e2e -- e2e/virtual-servers.spec.ts(45 passed)npm run e2e -- e2e/virtual-servers.spec.ts(41 passed, 4 flag-dependent tests skipped)HEADLESS=1 BASE_URL=http://localhost:5176 node virtual-server-try-it-manual.mjs(virtual server actions: ok)Manual verification
Mock-backed manual test
Setup
Save the script from the next collapsible as
virtual-server-try-it-manual.mjsat the repository root, then run:If Vite selects another port, pass it to Terminal B, for example
BASE_URL=http://localhost:5175 node virtual-server-try-it-manual.mjs.Steps
query=cloudflare,limit=5, and headerX-Tenant-Id=team-a. Click Preview and confirm Preview 200.server_id, the arguments, and forwarded header./api/rpcwithmethod=tools/call, qualifiedparams.name,params.server_id, arguments, and the forwarded header.PERMISSIONS=NO_EXECUTEandPERMISSIONS=NO_SERVERS_USE; Preview remains available while the Live switch is disabled with the relevant permission message.TOOLS=EMPTY; fallback component rows remain visible but do not expose Test.Ctrl-C both terminals to stop.
Mock script
Requires
@playwright/test, which is already a development dependency.