Skip to content

feat: add MCP handshake test mode to Test Connection - #16

Open
Altamimi-Dev wants to merge 1 commit into
contextforge-org:mainfrom
Altamimi-Dev:5649-mcp-handshake-test-mode
Open

feat: add MCP handshake test mode to Test Connection#16
Altamimi-Dev wants to merge 1 commit into
contextforge-org:mainfrom
Altamimi-Dev:5649-mcp-handshake-test-mode

Conversation

@Altamimi-Dev

Copy link
Copy Markdown

Depends on IBM/mcp-context-forge#5934

Summary

UI half of the MCP handshake test, split out of IBM/mcp-context-forge#5934 now that the client lives in this repo — same pattern as #15.

Test Connection currently only proves a URL answers HTTP. This adds a second mode that proves the target actually speaks MCP.

  • Mode toggleHTTP request keeps the existing raw-request behavior; MCP handshake calls the new POST /v1/mcp-servers/test-handshake. Method, content type and body inputs are hidden in handshake mode since they don't apply.
  • Detail rows on success — server name and version, protocol version, negotiation path (server/discover or initialize), and credential source (stored server credentials / form headers / none).
  • Count badges — first-page tools/resources/prompts counts, rendered as 3+ tools when the backend's countsPartial flags a truncated listing (nextCursor present).
  • Failure-class badge — Transport / Protocol negotiation / Authentication / Invalid response, alongside the backend's actionable error copy.
  • Raw response — collapsible, truncated preview of the negotiated payload.
  • Cancellation — the in-flight request is aborted on unmount, on Cancel, and when switching modes, so a stale response can't land on the new mode.

i18n

All new user-facing copy goes through react-intl (useIntl + intl.formatMessage), with keys added to the mcpServer namespace for en-US, es-ES and pt-BR. The en-US messages are byte-identical to the inline strings they replace, so the ported tests assert unchanged output. Component counts use ICU plural forms ({count, plural, one {# tool} other {# tools}}).

Two things stayed inline deliberately:

  • The handshake Latency: … ms line — identical to the adjacent HTTP-mode line, which is not localized yet. Worth migrating together when this file gets a full localization pass rather than localizing one of the pair.
  • The negotiation-path values server/discover and initialize — protocol identifiers, not prose.

openapi.json

The snapshot gained only .paths."/v1/mcp-servers/test-handshake" plus the GatewayHandshakeRequest / GatewayHandshakeResponse schemas, extracted from the gateway's app.openapi() on the #5934 branch. Everything else is untouched and the spec stays pinned at API v1.0.7 — a wholesale regen would pull in unrelated main-side drift ahead of the next version bump. All $refs in the added fragment (HTTPValidationError) already existed in the snapshot. npm run generate picks up the new endpoint and emits the handshake types.

Verification

  • npm run generate — orval emits GatewayHandshakeRequest/GatewayHandshakeResponse types
  • npm run test — 2821 passed, 1 skipped (157 files); TestConnectionPanel.test.tsx alone is 32 passed, including the 10 new handshake tests (mode switch, success detail rows, countsPartial badge, all four failure classes, abort-on-unmount, error-clearing on mode switch)
  • npm run lint and npm run format:check — clean
  • npm run build — generate + tsc -b + vite build clean
  • git diff --numstat openapi.json280 0, purely additive

One note: npm run i18n:compile fails on this branch, but it fails identically on an untouched checkout of main (Error: No JSON file found in src/i18n/localescompile-folder is pointed at the parent directory rather than the per-locale directories). Pre-existing, and happy to fix it in a separate PR if that's useful.

Relates to IBM/mcp-context-forge#5649 — the backend half is IBM/mcp-context-forge#5934; together they complete the issue.

UI half of the MCP handshake test, split out of
IBM/mcp-context-forge#5934 now that the client lives in this repo.

Test Connection gains a mode toggle. HTTP request keeps the existing
raw-request behavior; MCP handshake calls the new
POST /v1/mcp-servers/test-handshake and reports whether the target
actually speaks MCP:

- Detail rows for server name/version, protocol version, negotiation
  path (server/discover or initialize) and credential source
- Count badges for first-page tools/resources/prompts, rendered as
  "3+ tools" when countsPartial marks the listing truncated
- A failure-class badge (transport / protocol negotiation /
  authentication / invalid response) with the backend's actionable copy
- A collapsible raw-response preview
- Method, content type and body inputs are hidden in handshake mode;
  the in-flight request is aborted on unmount, cancel, and mode switch

New user-facing copy goes through react-intl, with keys added to the
en-US, es-ES and pt-BR mcpServer namespaces. The en-US messages are
byte-identical to the strings they replace. Component counts use ICU
plural forms.

openapi.json gains only the new /v1/mcp-servers/test-handshake path
plus the GatewayHandshakeRequest/GatewayHandshakeResponse schemas,
extracted from the gateway's app.openapi(). The snapshot stays pinned
at API v1.0.7 otherwise, so the generated types pick up the handshake
endpoint without dragging in unrelated spec drift.

Relates to IBM/mcp-context-forge#5649

Signed-off-by: Ahmad Al Tamimi <altamimi.dev@gmail.com>

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Overall Assessment

High-quality, well-structured PR. The implementation is clean, the test coverage is thorough, and the i18n story is complete across three locales. The findings below are mostly minor, with one medium-severity bug and one WCAG failure worth addressing before merge.


🔴 Must Fix

1. handleTest — HTTP payload built unconditionally in handshake mode

File: src/components/servers/TestConnectionPanel.tsx

The GatewayTestRequest payload — including JSON.parse(body) — is fully assembled before the if (mode === "handshake") branch runs. In handshake mode the payload is never sent, but if a user typed an invalid JSON body in HTTP mode, then switched to handshake mode and hit Test, the unconditional JSON.parse(body) throws outside the handshake try/catch, leaving the component stuck with no error message.

// Runs even when mode === "handshake":
let parsedBody: string | Record<string, unknown> | undefined;
if (sendsBodyFor(method) && body.trim()) {
  parsedBody = contentType === "application/json" ? (JSON.parse(body) as ...) : body; // 💥
}
const payload: GatewayTestRequest = { ... };

Fix: Move the if (mode === "handshake") { ... return; } block above the payload construction, or guard the entire payload block with if (mode === "http").


🟡 Should Fix

2. headline — deeply nested ternary is fragile

File: src/components/servers/TestConnectionPanel.tsx

const headline =
  mode === "handshake"
    ? handshakeResponse
      ? handshakeResponse.success ? "Handshake succeeded" : "Handshake failed"
      : error || intl.formatMessage({ id: "mcpServer.testConnection.handshakeFailed" })
    : response
      ? `Status: ${response.statusCode} ${status === "success" ? "OK" : "error"}`
      : error || "Connection failed";

When status === "success" but handshakeResponse is unexpectedly null, the headline silently shows "Handshake failed". Consider extracting into a named function:

function getHandshakeHeadline(
  resp: GatewayHandshakeResponse | null,
  err: string,
  intl: IntlShape,
): string {
  if (!resp) return err || intl.formatMessage({ id: "mcpServer.testConnection.handshakeFailed" });
  return resp.success
    ? intl.formatMessage({ id: "mcpServer.testConnection.handshakeSucceeded" })
    : intl.formatMessage({ id: "mcpServer.testConnection.handshakeFailed" });
}

3. FAILURE_CLASS_MESSAGE_IDS / CREDENTIAL_SOURCE_MESSAGE_IDS — unchecked index access

File: src/components/servers/TestConnectionPanel.tsx

id: FAILURE_CLASS_MESSAGE_IDS[handshakeResponse.failureClass],       // may be undefined id: CREDENTIAL_SOURCE_MESSAGE_IDS[handshakeResponse.credentialSource ?? "none"],

Record<string, string> index access returns string | undefined at runtime. If the backend returns a future enum value not yet in the map, intl.formatMessage({ id: undefined }) will throw or render a raw key. Add a fallback:

id: FAILURE_CLASS_MESSAGE_IDS[handshakeResponse.failureClass]
    ?? `mcpServer.testConnection.failureClass.${handshakeResponse.failureClass}`,

4. openapi.jsonnullable: true is not valid OpenAPI 3.1

File: openapi.json

"GatewayHandshakeRequest": { ..., "nullable": true },
"GatewayHandshakeResponse": { ..., "nullable": true }

nullable is an OpenAPI 3.0 extension. The rest of this spec uses the 3.1 pattern (anyOf: [{...}, {type: "null"}]). Orval is tolerant today but a strict 3.1 parser will reject it. Backend concern for #5934

5. Copy button absent from handshake raw preview

File: src/components/servers/TestConnectionPanel.tsx

The copy-to-clipboard <Button> is gated on responseBodyText, so it is never rendered in handshake mode even when handshakeRawPreview is non-empty. Either add a copy button inside the <details> block or add a comment documenting the intentional omission.


🟢 Nits / Observations

6. DetailRow<dt>/<dd> outside <dl> would be invalid

The <dt> and <dd> elements in DetailRow are valid because they sit inside the <dl> parent in the caller. This is fine — just noting
that the outer <dl> is load-bearing for semantics and must not be removed.

7. countsPartial — non-ICU plural format is intentional but undocumented

"mcpServer.testConnection.countsPartial.tools": "{count}+ tools"

Plain substitution (not plural), so 1+ tools renders as plural even when count is 1. Correct per the PR description, but a short inline comment would prevent a future contributor from "fixing" it.

8. No test for Cancel button during a handshake in-flight request

The HTTP suite covers Cancel (shows during flight, aborts, returns to idle). The handshake suite tests unmount-cancellation but not the Cancel button itself. Low risk — same code path — but easy to add.

9. useCallback dep on intl is safe

useIntl() returns a stable reference in react-intl v6+, so listing intl in the useCallback dependency array will not cause spurious re-renders. No action needed.

10. ⚠️ Missing TabsContent / broken tabtabpanel ARIA relationship

File: src/components/servers/TestConnectionPanel.tsx

<Tabs value={mode} onValueChange={...}>
  <TabsList>
    <TabsTrigger value="http">HTTP request</TabsTrigger>
    <TabsTrigger value="handshake">MCP handshake</TabsTrigger>
  </TabsList>
  {/* No <TabsContent> — content rendered outside the Tabs tree */}
</Tabs>

The tab ARIA role requires each <TabsTrigger> to be associated with a tabpanel via aria-controls. Radix UI generates that relationship automatically when <TabsContent> is present. Without it, screen readers can navigate to the tabs but cannot find the controlled content. This fails WCAG 4.1.2 — Name, Role, Value (Level AA).

Fix — wrap the form content in <TabsContent> panels:

<Tabs value={mode} onValueChange={...}>
  <TabsList>...</TabsList>
  <TabsContent value="http">
    {/* existing left/right grid */}
  </TabsContent>
  <TabsContent value="handshake">
    {/* same grid, handshake mode */}
  </TabsContent>
</Tabs>

📊 Test Coverage Summary

Scenario Covered
Mode toggle — UI fields hidden/shown
Stored-credentials hint visible
Success: identity rows + count badges
Partial counts plural enforcement
Error clearing on mode switch
All 4 failure classes ✅ (it.each)
Unmount cancellation
Cancel button during handshake
rawPreview collapsible renders
Credential source label variants (stored, form)
Path forwarded in handshake payload
Headers forwarded in handshake payload

Summary

Severity # Description
🔴 Must fix 1 JSON.parse throws outside try/catch when switching from HTTP mode
🟡 Should fix 4 Headline ternary, unchecked map access, OpenAPI nullable, missing copy button
🟢 Nit / a11y 5 Missing TabsContent ARIA wiring is the most important of these

Block on: 1 (reproducible bug) and 10 (WCAG 4.1.2 failure). Everything else is polish and can follow in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants