Add clickable server primitive inspection to show servers#1937
Add clickable server primitive inspection to show servers#1937chelojimenez merged 4 commits intomainfrom
Conversation
dosubot
Bot
added
the
size:XL
✅ 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. |
MCP worker previewPreview worker |
WalkthroughAdds typed server primitives (tools, resources, prompts) with per-collection load status and an optional 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.
🧹 Nitpick comments (4)
mcp/src/tools/showServersCore.ts (2)
466-485:clientNamequietly disappears on the inspect path.
probeMcpServerreceivesclientName: "mcpjam-show-servers", butinspectMcpServerbuilds itsHttpServerConfigwithout forwarding it, so server logs that key off client identity will see a different (or default) name when inspection is on. Minor, but worth aligning if observability on the server side relies on this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/tools/showServersCore.ts` around lines 466 - 485, The inspect path is not preserving the client identity so server-side logs differ; ensure the same clientName ("mcpjam-show-servers") is forwarded to inspection by passing the full probeConfig (including clientName) into inspect or by mapping probeConfig.clientName into the HttpServerConfig that inspect/inspectMcpServer expects. Update the call site where inspect(probeConfig) is invoked (and/or adjust inspectMcpServer to accept/forward clientName) so inspect uses the same clientName as probe (probeConfig → inspect), preserving observability consistency.
207-208: Default behavior just got a lot heavier — confirm this is intended for everyshow_serverscall.When the caller omits both
probeandinspect, you now opt every workspace probe into the full doctor pipeline (probe → connect → list tools → list resources → list prompts). For workspaces with many servers this multiplies request fan-out and tail latency, and any flaky server is now five-ish calls’ worth of opportunity to time out instead of one. If the lightweightprobeMcpServerpath is still acceptable for callers that don't need primitives (e.g. the legacy text-only renderer), consider gating inspection behind an explicit input flag rather than auto-upgrading the default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/tools/showServersCore.ts` around lines 207 - 208, Currently inspectWithDefault auto-upgrades omitted inspect/probe calls into the heavy inspectMcpServer path; change that so inspection is only enabled when explicitly requested: remove the implicit fallback and set inspectWithDefault = inspect ?? undefined (or check an explicit boolean flag like enableInspect and only use inspectMcpServer when that flag is true), updating callers of show_servers / the probe call site to pass the new enableInspect flag if they want the full doctor pipeline; reference inspectWithDefault, inspect, probe, probeMcpServer, and inspectMcpServer when making the change.mcp/src/ui/show-servers.tsx (2)
310-339: Heading inside<button>collapses heading semantics.
<h2>is flow content and is not valid as a<button>descendant — assistive tech and heading-navigation tooling will lose the heading anchor for each server. Consider rendering the server name as a<span>(or moving the<h2>outside the clickable region and labelling the button viaaria-labelledby) so the inventory remains heading-navigable.♻️ Minimal swap to keep semantics intact
- <h2 className="truncate text-sm font-semibold text-foreground"> + <span className="truncate text-sm font-semibold text-foreground"> {server.name} - </h2> + </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 310 - 339, The server list item uses an <h2> (server.name) inside the clickable <button>, which breaks heading semantics; either move the heading outside the button and reference it from the button via aria-labelledby, or replace the <h2> with a non-heading inline element (e.g., <span>) and keep the heading outside for navigability. Update the JSX around the button that renders server.name (currently the <h2> in the component that also uses props/vars like selected, onSelect, displayUrl, StatusDot, STATUS_LABELS, server.status) so that headings remain real headings in the document flow—if you keep the heading outside, give the button an aria-labelledby pointing to the heading id; if you swap to a <span>, remove the <h2> and keep the accessible labeling via aria-label or aria-labelledby as appropriate.
287-301:stopPropagationhere is decorative.The copy button is now a sibling of the select button rather than nested inside it, so a click on copy never reaches
onSelectto begin with. The call is harmless, just noise — feel free to drop it (andevent: MouseEvent<HTMLButtonElement>) if you want a tidier signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 287 - 301, The copyUrl handler contains an unnecessary stopPropagation and event parameter because the copy button is now a sibling to the select button; remove the MouseEvent parameter from the copyUrl signature and delete the event.stopPropagation() call inside copyUrl, and update any call sites to invoke copyUrl() with no argument; keep the rest of the logic (clipboard write, setCopied state, timeout, and catch) unchanged and ensure the function name copyUrl is used to locate the handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcp/src/tools/showServersCore.ts`:
- Around line 466-485: The inspect path is not preserving the client identity so
server-side logs differ; ensure the same clientName ("mcpjam-show-servers") is
forwarded to inspection by passing the full probeConfig (including clientName)
into inspect or by mapping probeConfig.clientName into the HttpServerConfig that
inspect/inspectMcpServer expects. Update the call site where
inspect(probeConfig) is invoked (and/or adjust inspectMcpServer to
accept/forward clientName) so inspect uses the same clientName as probe
(probeConfig → inspect), preserving observability consistency.
- Around line 207-208: Currently inspectWithDefault auto-upgrades omitted
inspect/probe calls into the heavy inspectMcpServer path; change that so
inspection is only enabled when explicitly requested: remove the implicit
fallback and set inspectWithDefault = inspect ?? undefined (or check an explicit
boolean flag like enableInspect and only use inspectMcpServer when that flag is
true), updating callers of show_servers / the probe call site to pass the new
enableInspect flag if they want the full doctor pipeline; reference
inspectWithDefault, inspect, probe, probeMcpServer, and inspectMcpServer when
making the change.
In `@mcp/src/ui/show-servers.tsx`:
- Around line 310-339: The server list item uses an <h2> (server.name) inside
the clickable <button>, which breaks heading semantics; either move the heading
outside the button and reference it from the button via aria-labelledby, or
replace the <h2> with a non-heading inline element (e.g., <span>) and keep the
heading outside for navigability. Update the JSX around the button that renders
server.name (currently the <h2> in the component that also uses props/vars like
selected, onSelect, displayUrl, StatusDot, STATUS_LABELS, server.status) so that
headings remain real headings in the document flow—if you keep the heading
outside, give the button an aria-labelledby pointing to the heading id; if you
swap to a <span>, remove the <h2> and keep the accessible labeling via
aria-label or aria-labelledby as appropriate.
- Around line 287-301: The copyUrl handler contains an unnecessary
stopPropagation and event parameter because the copy button is now a sibling to
the select button; remove the MouseEvent parameter from the copyUrl signature
and delete the event.stopPropagation() call inside copyUrl, and update any call
sites to invoke copyUrl() with no argument; keep the rest of the logic
(clipboard write, setCopied state, timeout, and catch) unchanged and ensure the
function name copyUrl is used to locate the handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95ed4c92-8a99-41f0-9ccb-befda9ee8d26
⛔ Files ignored due to path filters (1)
mcp/src/generated/McpAppsHtml.bundled.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
mcp/src/shared/show-servers.tsmcp/src/tools/showServersCore.tsmcp/src/ui/show-servers.tsxmcp/tests/showServersCore.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
mcp/src/ui/show-servers.tsx (3)
467-540: A small accessibility courtesy for the disclosure pattern.
PrimitiveDropdownis a textbook disclosure widget —aria-expandedis wired up correctly and the chevron rotates in sympathy. Two refinements would round it out for assistive tech:
- Pair the trigger and panel with
aria-controls/idso screen readers can announce the relationship.- Mark the chevron as
aria-hiddensince its meaning is already conveyed byaria-expanded.Neither blocks the feature; they simply tighten the semantics now that the primitives surface is non-trivial.
♻️ Suggested tightening
function PrimitiveDropdown<TItem>({ label, icon: Icon, collection, emptyLabel, getKey, renderItem, }: { label: string; icon: ComponentType<{ className?: string }>; collection: ServerPrimitiveCollection<TItem>; emptyLabel: string; getKey: (item: TItem) => string; renderItem: (item: TItem) => ReactNode; }) { const [open, setOpen] = useState(false); const hasItems = collection.items.length > 0; + const panelId = useId(); @@ <button type="button" onClick={() => setOpen((value) => !value)} aria-expanded={open} + aria-controls={panelId} className="flex w-full cursor-pointer items-center justify-between gap-3 p-4 text-left transition-colors hover:bg-accent/50 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" > @@ <ChevronDown + aria-hidden="true" className={cn( "h-4 w-4 text-muted-foreground transition-transform", open && "rotate-180" )} /> @@ {open ? ( - <div className="border-t border-border/60 px-4 py-2"> + <div id={panelId} className="border-t border-border/60 px-4 py-2">(Don't forget to add
useIdto the React import.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 467 - 540, The disclosure button in PrimitiveDropdown should explicitly link to its panel and hide the decorative chevron for screen readers: generate a stable id with useId inside PrimitiveDropdown, set that id on the panel container (the div rendered when open), add aria-controls={panelId} to the button alongside the existing aria-expanded, and mark the ChevronDown icon aria-hidden="true"; keep the existing open state and class rotation logic unchanged.
241-263: Selection-reset effect is sound; a one-liner of derivation would be even tidier.The effect correctly clears
selectedServerIdwhen the previously selected server disappears from the payload (e.g., after a workspace refresh). It does the job and avoids stale-detail-screen states.If you'd like to shave the extra render that the effect introduces, the same outcome can be achieved by deriving the selected server first and only short-circuiting to the detail screen when it actually exists:
♻️ Optional simplification
- const [selectedServerId, setSelectedServerId] = useState<string | undefined>( - undefined - ); - - useEffect(() => { - if ( - selectedServerId === undefined || - payload.servers.some((server) => server.id === selectedServerId) - ) { - return; - } - - setSelectedServerId(undefined); - }, [payload.servers, selectedServerId]); - - const selectedServer = payload.servers.find( - (server) => server.id === selectedServerId - ); - - if (selectedServer) { + const [selectedServerId, setSelectedServerId] = useState<string | undefined>( + undefined + ); + + const selectedServer = + selectedServerId !== undefined + ? payload.servers.find((server) => server.id === selectedServerId) + : undefined; + + if (selectedServer) {That keeps the back action functional and lets a stale id naturally fall through to the grid without a second render pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 241 - 263, The useEffect that clears selectedServerId when the server disappears causes an extra render; instead remove that effect and derive the selectedServer directly from payload.servers (using payload.servers.find(server => server.id === selectedServerId)), then short-circuit to render ServerDetailScreen only when selectedServer is truthy; keep the onBack handler calling setSelectedServerId(undefined) so the back action continues to work and allow a stale id to naturally fall through to the grid without an extra effect-induced render.
326-374: Two stacked buttons in shared territory — defensible, but the copy control deserves a hand-off.The copy button is now a sibling of the card-level select button rather than a child, which sidesteps the invalid-nested-
<button>HTML pitfall — nicely done. Two small polish items worth considering:
event.stopPropagation()incopyUrlis benign but unnecessary, since the copy button is no longer nested insideonSelect's button — clicks never bubble through the select handler. Safe to keep as a defensive belt-and-suspenders, but noting it for the next reader who wonders why it's there.- The absolutely-positioned copy button visually overlays the URL block of the card-wide button. With
position: absolutebut no explicitz-index, paint order alone keeps it clickable; if a future tweak introduces stacking contexts on the inner button, the copy affordance could become inert. A defensivez-10(or equivalent) on the copy button would future-proof it.♻️ Optional hardening
<button aria-label={`Copy URL for ${server.name}`} title={`Copy URL for ${server.name}`} type="button" onClick={copyUrl} - className="absolute bottom-5 right-5 cursor-pointer p-1 text-muted-foreground/60 transition-colors hover:text-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" + className="absolute bottom-5 right-5 z-10 cursor-pointer p-1 text-muted-foreground/60 transition-colors hover:text-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 326 - 374, The copy button sits alongside the card-level onSelect button so stopPropagation in copyUrl is unnecessary and the absolute-positioned copy control needs an explicit stacking context; remove the defensive event.stopPropagation() call inside the copyUrl handler and add a high z-index utility (e.g., "z-10") to the copy button element (the button that uses aria-label/ title `Copy URL for ${server.name}`) to ensure it remains clickable even if inner elements gain stacking contexts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp/src/ui/show-servers.tsx`:
- Around line 520-535: The empty-state message always shows emptyLabel even when
collection.status indicates a failure; update the JSX inside the expanded panel
(the block that currently renders emptyLabel) to check collection.status and,
when status is "skipped" or "error" and a collection.statusDetail exists, render
that statusDetail (or a small status-aware fallback message) instead of
emptyLabel; keep existing behavior for successful/empty runs, and use the same
rendering conventions as renderItem/getKey so styling and semantics remain
consistent.
---
Nitpick comments:
In `@mcp/src/ui/show-servers.tsx`:
- Around line 467-540: The disclosure button in PrimitiveDropdown should
explicitly link to its panel and hide the decorative chevron for screen readers:
generate a stable id with useId inside PrimitiveDropdown, set that id on the
panel container (the div rendered when open), add aria-controls={panelId} to the
button alongside the existing aria-expanded, and mark the ChevronDown icon
aria-hidden="true"; keep the existing open state and class rotation logic
unchanged.
- Around line 241-263: The useEffect that clears selectedServerId when the
server disappears causes an extra render; instead remove that effect and derive
the selectedServer directly from payload.servers (using
payload.servers.find(server => server.id === selectedServerId)), then
short-circuit to render ServerDetailScreen only when selectedServer is truthy;
keep the onBack handler calling setSelectedServerId(undefined) so the back
action continues to work and allow a stale id to naturally fall through to the
grid without an extra effect-induced render.
- Around line 326-374: The copy button sits alongside the card-level onSelect
button so stopPropagation in copyUrl is unnecessary and the absolute-positioned
copy control needs an explicit stacking context; remove the defensive
event.stopPropagation() call inside the copyUrl handler and add a high z-index
utility (e.g., "z-10") to the copy button element (the button that uses
aria-label/ title `Copy URL for ${server.name}`) to ensure it remains clickable
even if inner elements gain stacking contexts.
🪄 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: 2760b05b-1a7e-4c3b-8c68-fbc485c52397
⛔ Files ignored due to path filters (1)
mcp/src/generated/McpAppsHtml.bundled.tsis excluded by!**/generated/**
📒 Files selected for processing (1)
mcp/src/ui/show-servers.tsx
| {open ? ( | ||
| <div className="border-t border-border/60 px-4 py-2"> | ||
| {hasItems ? ( | ||
| <ul> | ||
| {collection.items.map((item) => ( | ||
| <li | ||
| key={getKey(item)} | ||
| className="border-t border-border/50 py-3 first:border-t-0" | ||
| > | ||
| {renderItem(item)} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| ) : ( | ||
| <p className="py-3 text-sm text-muted-foreground">{emptyLabel}</p> | ||
| )} |
There was a problem hiding this comment.
The empty-state copy ignores why the list is empty.
When collection.status is "skipped" or "error" and items is empty, the panel still renders the cheerful "No tools discovered." line. The header already shows a status badge and (optionally) statusDetail, but inside the expanded panel the user gets a message that contradicts the failure state. A small tweak — surfacing the status detail or a status-aware empty message — keeps the disclosure honest when inspection didn't actually run.
🐛 Suggested status-aware empty state
- {hasItems ? (
+ {hasItems ? (
<ul>
{collection.items.map((item) => (
<li
key={getKey(item)}
className="border-t border-border/50 py-3 first:border-t-0"
>
{renderItem(item)}
</li>
))}
</ul>
) : (
- <p className="py-3 text-sm text-muted-foreground">{emptyLabel}</p>
+ <p className="py-3 text-sm text-muted-foreground">
+ {collection.status === "loaded"
+ ? emptyLabel
+ : (collection.statusDetail ??
+ `${PRIMITIVE_STATUS_LABELS[collection.status]} — no items available.`)}
+ </p>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp/src/ui/show-servers.tsx` around lines 520 - 535, The empty-state message
always shows emptyLabel even when collection.status indicates a failure; update
the JSX inside the expanded panel (the block that currently renders emptyLabel)
to check collection.status and, when status is "skipped" or "error" and a
collection.statusDetail exists, render that statusDetail (or a small
status-aware fallback message) instead of emptyLabel; keep existing behavior for
successful/empty runs, and use the same rendering conventions as
renderItem/getKey so styling and semantics remain consistent.
chelojimenez
temporarily deployed
to
preview-pr-1937
GitHub Actions
Inactive
Internal previewPreview URL: https://mcp-inspector-pr-1937.up.railway.app |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
mcp/src/ui/show-servers.tsx (4)
311-325:event.stopPropagation()is now ornamental.The copy-URL
<button>is a sibling of the card's selection<button>, not a descendant — clicks no longer bubble through one to the other. ThestopPropagationcall is harmless but misleading to future readers who may infer a nested-interactive layout that no longer exists.♻️ Drop the no-op
- const copyUrl = async (event: MouseEvent<HTMLButtonElement>) => { - event.stopPropagation(); - + const copyUrl = async () => { if (!server.url) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 311 - 325, Remove the misleading no-op stopPropagation call in the copyUrl handler: edit the copyUrl function to delete the event.stopPropagation() line (since the copy button is a sibling of the card selection button and clicks do not bubble into it). Keep the rest of copyUrl intact (navigator.clipboard.writeText, setCopied(true), timeout fallback and catch) so behavior is unchanged but the code no longer implies a nested interactive layout.
488-519: Tighten the disclosure semantics witharia-controls.The toggle exposes
aria-expandedbut doesn't point assistive tech at the panel it governs. A stable id wired toaria-controlsrounds out the disclosure pattern:♻️ Wire up aria-controls
function PrimitiveDropdown<TItem>({ ... }) { const [open, setOpen] = useState(false); + const panelId = useId(); const hasItems = collection.items.length > 0; return ( <section className="overflow-hidden rounded-lg border border-border/60 bg-card/60"> <button type="button" onClick={() => setOpen((value) => !value)} aria-expanded={open} + aria-controls={panelId} className="..." > ... </button> {open ? ( - <div className="border-t border-border/60 px-4 py-2"> + <div id={panelId} className="border-t border-border/60 px-4 py-2"> ... </div> ) : null} </section> ); }(Add
useIdto the existingreactimport.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 488 - 519, The button toggle exposes aria-expanded but lacks aria-controls; import useId from React, generate a stable id (e.g., const panelId = useId()) near the component top, add aria-controls={panelId} to the button that uses setOpen/open, and ensure the corresponding panel element (the collapsible container that shows collection items / uses Badge/PRIMITIVE_STATUS_LABELS) has id={panelId} so assistive tech can associate the control with the panel.
238-264: Selection-reset effect: correct but a touch eager.The reset effect re-runs on every new
payload.serversreference, even when the selected server is still present, which is harmless but wasteful. ComputingselectedServerfirst and reacting to its absence avoids the extra work and reads slightly cleaner:♻️ Derive then reconcile
- const [selectedServerId, setSelectedServerId] = useState<string | undefined>( - undefined - ); - - useEffect(() => { - if ( - selectedServerId === undefined || - payload.servers.some((server) => server.id === selectedServerId) - ) { - return; - } - - setSelectedServerId(undefined); - }, [payload.servers, selectedServerId]); - - const selectedServer = payload.servers.find( - (server) => server.id === selectedServerId - ); + const [selectedServerId, setSelectedServerId] = useState<string | undefined>( + undefined + ); + + const selectedServer = + selectedServerId !== undefined + ? payload.servers.find((server) => server.id === selectedServerId) + : undefined; + + useEffect(() => { + if (selectedServerId !== undefined && !selectedServer) { + setSelectedServerId(undefined); + } + }, [selectedServer, selectedServerId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 238 - 264, Compute selectedServer before the effect and make the effect only reconcile when that computed value is missing: move the selectedServer = payload.servers.find(...) above the useEffect, then change the useEffect to depend on selectedServer (and remove payload.servers from deps) and call setSelectedServerId(undefined) only when selectedServer is undefined and selectedServerId is not; update references to selectedServerId, selectedServer, payload.servers, useEffect, and setSelectedServerId accordingly.
82-97: Simplify by passing justhostContextto avoid unnecessary fallback calls.The
hostContextstate is seeded fromapp.getHostContext()in the effect below (line 105), making the per-render fallback redundant. While the hook's internaluseRefguard prevents problematic re-applications, passing a potentially fresh object on each render is unnecessary. Just pass the state value directly.♻️ Drop the per-render fallback
- useHostStyles(app, hostContext ?? app?.getHostContext()); + useHostStyles(app, hostContext);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 82 - 97, The code passes useHostStyles(app, hostContext ?? app?.getHostContext()) which redundantly falls back to app?.getHostContext() each render; change it to call useHostStyles(app, hostContext) so the hook receives the hostContext state directly (remove the per-render fallback expression), ensuring useHostStyles, hostContext, and app.getHostContext() are the referenced symbols to locate the change.mcpjam-inspector/client/src/components/ServersTab.tsx (1)
1485-1485: Tidy reuse of the shared skeleton — LGTM.A clean swap to the shared
ServersLoadingSkeleton. One soft suggestion: if any tests or telemetry rely on identifying the loading container, consider passing adata-testid(akin to howshow-servers.tsxcalls it) to keep the loading state addressable.- const renderLoadingContent = () => <ServersLoadingSkeleton />; + const renderLoadingContent = () => ( + <ServersLoadingSkeleton data-testid="servers-tab-loading" /> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/ServersTab.tsx` at line 1485, The loading container lacks a test identifier; update the renderLoadingContent usage of ServersLoadingSkeleton to pass a data-testid prop (matching the pattern used in show-servers.tsx) so tests/telemetry can target the loading state—locate the renderLoadingContent arrow function and add the data-testid attribute to the ServersLoadingSkeleton element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@design-system/src/components/servers-loading-skeleton.tsx`:
- Around line 1-8: The file is using React.ComponentProps<"div"> in the
ServersLoadingSkeletonProps interface but React is not imported; add the
namespace import "import * as React from 'react';" (or the project convention
equivalent) at the top of the file so React is in scope for the
React.ComponentProps type used in ServersLoadingSkeletonProps and any other type
references in this module.
---
Nitpick comments:
In `@mcp/src/ui/show-servers.tsx`:
- Around line 311-325: Remove the misleading no-op stopPropagation call in the
copyUrl handler: edit the copyUrl function to delete the event.stopPropagation()
line (since the copy button is a sibling of the card selection button and clicks
do not bubble into it). Keep the rest of copyUrl intact
(navigator.clipboard.writeText, setCopied(true), timeout fallback and catch) so
behavior is unchanged but the code no longer implies a nested interactive
layout.
- Around line 488-519: The button toggle exposes aria-expanded but lacks
aria-controls; import useId from React, generate a stable id (e.g., const
panelId = useId()) near the component top, add aria-controls={panelId} to the
button that uses setOpen/open, and ensure the corresponding panel element (the
collapsible container that shows collection items / uses
Badge/PRIMITIVE_STATUS_LABELS) has id={panelId} so assistive tech can associate
the control with the panel.
- Around line 238-264: Compute selectedServer before the effect and make the
effect only reconcile when that computed value is missing: move the
selectedServer = payload.servers.find(...) above the useEffect, then change the
useEffect to depend on selectedServer (and remove payload.servers from deps) and
call setSelectedServerId(undefined) only when selectedServer is undefined and
selectedServerId is not; update references to selectedServerId, selectedServer,
payload.servers, useEffect, and setSelectedServerId accordingly.
- Around line 82-97: The code passes useHostStyles(app, hostContext ??
app?.getHostContext()) which redundantly falls back to app?.getHostContext()
each render; change it to call useHostStyles(app, hostContext) so the hook
receives the hostContext state directly (remove the per-render fallback
expression), ensuring useHostStyles, hostContext, and app.getHostContext() are
the referenced symbols to locate the change.
In `@mcpjam-inspector/client/src/components/ServersTab.tsx`:
- Line 1485: The loading container lacks a test identifier; update the
renderLoadingContent usage of ServersLoadingSkeleton to pass a data-testid prop
(matching the pattern used in show-servers.tsx) so tests/telemetry can target
the loading state—locate the renderLoadingContent arrow function and add the
data-testid attribute to the ServersLoadingSkeleton element.
🪄 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: f1e17e78-e0d9-40b9-9b7c-9d40d2258f2e
⛔ Files ignored due to path filters (1)
mcp/src/generated/McpAppsHtml.bundled.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
design-system/src/components/servers-loading-skeleton.tsxmcp/src/ui/show-servers.tsxmcpjam-inspector/client/src/components/ServersTab.tsx
| import { cn } from "../cn"; | ||
| import { Skeleton } from "./skeleton"; | ||
|
|
||
| interface ServersLoadingSkeletonProps extends React.ComponentProps<"div"> { | ||
| cardClassName?: string; | ||
| cardCount?: number; | ||
| gridClassName?: string; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether React is globally typed in the design-system package or imported elsewhere in the file's neighbors.
fd -t f 'tsconfig*.json' design-system | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'
echo "--- sibling component imports ---"
fd -t f -e tsx -e ts . design-system/src/components | head -20 | xargs -I{} sh -c 'echo "== {} =="; head -5 "{}"'Repository: MCPJam/inspector
Length of output: 4712
🏁 Script executed:
# Check the full servers-loading-skeleton.tsx file
cat design-system/src/components/servers-loading-skeleton.tsx
# Check aspect-ratio and collapsible for their full import statements
echo "=== aspect-ratio.tsx imports ==="
head -15 design-system/src/components/aspect-ratio.tsx
echo "=== collapsible.tsx imports ==="
head -15 design-system/src/components/collapsible.tsxRepository: MCPJam/inspector
Length of output: 1714
Add missing React import for type reference.
React.ComponentProps<"div"> requires React to be in scope. Add the import at the top to match the codebase convention:
♻️ Add namespace import
+import * as React from "react";
import { cn } from "../cn";
import { Skeleton } from "./skeleton";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@design-system/src/components/servers-loading-skeleton.tsx` around lines 1 -
8, The file is using React.ComponentProps<"div"> in the
ServersLoadingSkeletonProps interface but React is not imported; add the
namespace import "import * as React from 'react';" (or the project convention
equivalent) at the top of the file so React is in scope for the
React.ComponentProps type used in ServersLoadingSkeletonProps and any other type
references in this module.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsx (1)
836-885: Unconditionally settingtheme: resolvedThemeis the right move.Dropping the previous "preserve
baseHostContext.themeif it's literallylight/dark" branch is the correct simplification — that branch is now subsumed byconfiguredHostThemeflowing throughresolvedThemewhenever the playground is active, and outside the playground we explicitly don't want a stale draft value to override the host's theme. The memo deps already includeresolvedTheme, so propagation throughsetHostContext(line 1274) is wired up.One small consistency note for a future pass — not this PR:
chatgpt-app-renderer.tsxstill uses the olderextractHostTheme(draftHostContext) ?? themeModepattern. Worth aligning when you next touch that file so both renderers honoruseChatboxHostTheme()identically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsx` around lines 836 - 885, Keep the simplified behavior: set theme: resolvedTheme unconditionally inside the McpUiHostContext created in the useMemo (symbol: hostContext), and ensure resolvedTheme remains in the memo dependency array (it already is) so updates propagate via setHostContext; remove any leftover conditional branch that tried to preserve baseHostContext.theme when it was "light"/"dark". For consistency in a follow-up change, update the other renderer (symbol: chatgpt-app-renderer and its draftHostContext handling) to derive theme the same way (use the same resolvedTheme/useChatboxHostTheme pattern rather than extractHostTheme(draftHostContext) ?? themeMode) so both renderers behave identically.mcp/src/ui/show-servers.tsx (1)
350-395: Tame the screen-reader name on the whole-card button.By promoting the entire card to a
<button>, every descendant text node — name, version, status word, URL — gets concatenated into the button's accessible name. On screens with several entries, that's a verbose mouthful per card. Consider giving the button an explicitaria-labelledby(pointing to the<h2>) or anaria-labellikeOpen ${server.name} details, so AT users hear an actionable label while sighted users keep all the visual context.♻️ Suggested concise accessible name
<button type="button" onClick={onSelect} + aria-label={`Open details for ${server.name}`} className="block h-full w-full cursor-pointer rounded-xl p-4 text-left focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/show-servers.tsx` around lines 350 - 395, The card's top-level button (onClick handler on the Card wrapper / onSelect) exposes every descendant text as the accessible name; change it to an explicit accessible name by adding either aria-label={`Open ${server.name} details`} or aria-labelledby pointing to the heading id—give the <h2> a stable id (e.g., `server-title-${server.id}`) and set the button's aria-labelledby to that id so screen readers announce a concise actionable label while keeping the visible title/version/status/URL intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcp/src/ui/show-servers.tsx`:
- Around line 350-395: The card's top-level button (onClick handler on the Card
wrapper / onSelect) exposes every descendant text as the accessible name; change
it to an explicit accessible name by adding either aria-label={`Open
${server.name} details`} or aria-labelledby pointing to the heading id—give the
<h2> a stable id (e.g., `server-title-${server.id}`) and set the button's
aria-labelledby to that id so screen readers announce a concise actionable label
while keeping the visible title/version/status/URL intact.
In
`@mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsx`:
- Around line 836-885: Keep the simplified behavior: set theme: resolvedTheme
unconditionally inside the McpUiHostContext created in the useMemo (symbol:
hostContext), and ensure resolvedTheme remains in the memo dependency array (it
already is) so updates propagate via setHostContext; remove any leftover
conditional branch that tried to preserve baseHostContext.theme when it was
"light"/"dark". For consistency in a follow-up change, update the other renderer
(symbol: chatgpt-app-renderer and its draftHostContext handling) to derive theme
the same way (use the same resolvedTheme/useChatboxHostTheme pattern rather than
extractHostTheme(draftHostContext) ?? themeMode) so both renderers behave
identically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b37bf320-46bd-453e-ae65-b452bc3d33f3
⛔ Files ignored due to path filters (1)
mcp/src/generated/McpAppsHtml.bundled.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
mcp/src/ui/show-servers.tsxmcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsxmcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsx
1 participant
Summary
show_serverspayloads with compact tool, resource, and prompt summaries.Testing
npm run test -w @mcpjam/mcp -- showServersCorenpm run typecheck -w @mcpjam/mcpnpm run test -w @mcpjam/mcpgit diff --checkNote
Medium Risk
Adds new server-inspection data to the
show_serverspayload and new UI navigation to drill into per-server details, which could affect tool output compatibility and increase probe/doctor call surface area.Overview
Enhances
show_serversto optionally inspect reachable HTTPS servers and return compacttools,resources, andpromptssummaries (with per-collectionloaded/skipped/errorstatus and details) alongside existing reachability data.Updates the bundled
show-serversUI to make server cards clickable and present a master-detail view that renders the inspected primitives (plus copy-URL and richer status messaging), and introduces a reusableServersLoadingSkeletoncomponent for loading states.Extends test coverage in
showServersCore.test.tsto validate the new inspection-derived payload fields and existing probe/authorization outcomes.Reviewed by Cursor Bugbot for commit d3c3a2c. Bugbot is set up for automated code reviews on this repo. Configure here.