Open
Conversation
|
🚅 Environment inspector-pr-1632 in triumphant-alignment has no services deployed. 1 service not affected by this PR
|
Contributor
✅ 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. |
dosubot
bot
added
size:XXL
prathmeshpatel
force-pushed
the
app-registry
branch
from
284adc2 to
69e4070
Compare
Contributor
WalkthroughAdds a first-class "Registry" tab and sidebar item plus a new RegistryTab component and useRegistryServers hook to fetch, enrich, consolidate, filter, and manage registry server connections (including mock mode and connect/disconnect flows). ServersTab gains a featured Quick Connect area with Registry navigation. OAuth flows and mcp-oauth plumbing were extended to support registry-aware Convex token/refresh routing and a registryServerId field; ServerFormData gains oauthCredentialKey and registryServerId. Tests and hosted-tab normalization were updated and several debug log statements were added. Important Merge conflicts detected (Beta)
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![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: 6
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx (1)
6-12: Prefer repository mock presets over ad-hoc test doubles.This hoisted mock setup works, but client tests are easier to maintain when built from shared mock presets rather than local one-offs.
As per coding guidelines:
mcpjam-inspector/client/**/__tests__/*.test.{ts,tsx}: Use mock presets from client/src/test/mocks/ including mcpApiPresets and storePresets in client tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx` around lines 6 - 12, Replace the ad-hoc hoisted mock for toastError, toastSuccess, and handleOAuthCallbackMock with the shared mock presets used across client tests: import and apply the mcpApiPresets and storePresets rather than defining vi.hoisted locals; remove the vi.hoisted block and wire the preset-provided mocks (the presets expose equivalents for toastError/toastSuccess and OAuth handlers) so the test uses the centralized mocks and stays consistent with other tests.mcpjam-inspector/client/src/components/__tests__/RegistryTab.test.tsx (1)
6-19: Use the shared client test presets here.This suite hand-rolls the hook response instead of the standard client presets, so it drifts from the mocked API/store contract the rest of the client tests rely on.
As per coding guidelines "Use mock presets from
client/src/test/mocks/includingmcpApiPresetsandstorePresetsin client tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/__tests__/RegistryTab.test.tsx` around lines 6 - 19, Replace the hand-rolled mockHookReturn in RegistryTab.test.tsx with the shared test presets: import mcpApiPresets and storePresets from client/src/test/mocks and use their registry server data to populate the mocked useRegistryServers return value; keep mockConnect and mockDisconnect as vi.fn() but set registryServers, categories and isLoading from the presets so the mocked hook (useRegistryServers) conforms to the standard client API/store contract used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/components/__tests__/RegistryTab.test.tsx`:
- Around line 74-85: The tests leave localStorage state behind (the
'registry-pending-redirect' key) so make the beforeEach also reset storage;
update the beforeEach in RegistryTab.test.tsx (where mockConnect, mockDisconnect
and mockHookReturn are set) to call
localStorage.removeItem('registry-pending-redirect') or localStorage.clear() so
each test starts with a clean storage state and is order-independent.
In `@mcpjam-inspector/client/src/components/RegistryTab.tsx`:
- Around line 81-92: The code sets localStorage key "registry-pending-redirect"
before calling connect(server) in handleConnect but never clears it if connect
rejects, leaving a stale redirect value; add a catch block around await
connect(server) (or move the setItem to after a successful connect) so that on
rejection you removeItem("registry-pending-redirect") and rethrow the error (or
otherwise handle it) — reference handleConnect, connect(server) and the
"registry-pending-redirect" localStorage key.
In `@mcpjam-inspector/client/src/components/ServersTab.tsx`:
- Around line 532-546: The Card element currently uses onClick for quick-connect
but is not keyboard-accessible; change the interactive Card to be a semantic
button (or render it as a <button> via the Card component API) or add
role="button", tabIndex={0}, and an onKeyDown handler that triggers onConnect
for Enter and Space; ensure you pass type="button" (if rendering a real button)
and preserve the same onConnect payload (name: server.displayName, type: "http",
url: server.transport.url, useOAuth, oauthScopes, clientId, registryServerId:
server._id) and add an accessible label/aria-label using server.displayName.
In `@mcpjam-inspector/client/src/hooks/useRegistryServers.ts`:
- Around line 5-10: DEV_MOCK_REGISTRY is currently hardwired to
import.meta.env.DEV && true which keeps mock mode always enabled in development
and prevents real Convex queries from running; change the constant to only
enable mocks when an explicit opt-in flag is set (e.g.,
import.meta.env.VITE_DEV_MOCK_REGISTRY === 'true' or similar) so that in normal
dev the code uses real Convex paths, and only when the new env flag is present
will useRegistryServers fall back to mocks; update the DEV_MOCK_REGISTRY
declaration and any code paths using it (useRegistryServers) to check the
explicit flag and default to false.
- Around line 248-283: The UI is showing servers as "not_connected" while
connections are still unresolved because connectedRegistryIds defaults to an
empty Set; update the loading condition so it also waits for connections before
rendering real server states by modifying isLoading (which currently only checks
registryServers) to include connections (or the presence of
connectedRegistryIds) in its readiness check; reference the
connectedRegistryIds, connections, registryServers, and isLoading symbols so you
locate and change the loading logic accordingly.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 477-483: The code is directly calling JSON.parse on localStorage
values (e.g., the storedOAuthConfig -> registryServerId extraction) outside any
try/catch which can throw on malformed data; create a small helper (e.g.,
safeParseOAuthConfig(key) or parseStoredOAuthConfig) that reads
localStorage.getItem, wraps JSON.parse in try/catch, and returns undefined on
parse failure, then replace the direct JSON.parse usages (the registryServerId
extraction and the other similar call in the file around the 636-642 area) to
use that helper so malformed/stale values yield undefined instead of throwing.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/__tests__/RegistryTab.test.tsx`:
- Around line 6-19: Replace the hand-rolled mockHookReturn in
RegistryTab.test.tsx with the shared test presets: import mcpApiPresets and
storePresets from client/src/test/mocks and use their registry server data to
populate the mocked useRegistryServers return value; keep mockConnect and
mockDisconnect as vi.fn() but set registryServers, categories and isLoading from
the presets so the mocked hook (useRegistryServers) conforms to the standard
client API/store contract used elsewhere.
In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx`:
- Around line 6-12: Replace the ad-hoc hoisted mock for toastError,
toastSuccess, and handleOAuthCallbackMock with the shared mock presets used
across client tests: import and apply the mcpApiPresets and storePresets rather
than defining vi.hoisted locals; remove the vi.hoisted block and wire the
preset-provided mocks (the presets expose equivalents for
toastError/toastSuccess and OAuth handlers) so the test uses the centralized
mocks and stays consistent with other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fdbe88c-2ca6-4f8d-b092-24f7af6bb0c1
📒 Files selected for processing (13)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/RegistryTab.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/__tests__/RegistryTab.test.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/useRegistryServers.tsmcpjam-inspector/client/src/lib/__tests__/hosted-navigation.test.tsmcpjam-inspector/client/src/lib/__tests__/hosted-tab-policy.test.tsmcpjam-inspector/client/src/lib/hosted-tab-policy.tsmcpjam-inspector/client/src/lib/oauth/__tests__/debug-oauth-client-metadata.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/shared/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/ServersTab.tsx (1)
177-180:as anytype assertions obscure type safety.The query path string cast and argument cast bypass Convex's type inference. Consider importing the generated
apiobject from your Convex functions and using it directly, which provides compile-time validation of query paths and arguments.💡 Preferred pattern
import { api } from "../../convex/_generated/api"; // ... const registryServers = useQuery( api.registryServers.listRegistryServers, isAuthenticated ? {} : "skip", );🤖 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` around lines 177 - 180, The useQuery call in ServersTab uses unsafe any casts for the query key and arguments which bypass Convex's type safety; import the generated api (import { api } from "../../convex/_generated/api") and replace the string/any usage with the function reference api.registryServers.listRegistryServers as the first argument to useQuery, and pass a typed argument (use {} when isAuthenticated is true, otherwise "skip") so TypeScript can infer the correct return type for registryServers and remove the ({} as any) and the string cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/components/ServersTab.tsx`:
- Line 154: The prop onLeaveWorkspace declared on the ServersTab component is
never used—remove it from the props interface/type and from the destructuring in
the ServersTab function to eliminate dead code (ensure there are no references
to onLeaveWorkspace elsewhere in the file or children); if the callback is
actually needed later, instead wire it into the appropriate handler (e.g., pass
to a child or call from a button click), otherwise delete the onLeaveWorkspace
symbol from the component signature and any related imports/uses.
In `@mcpjam-inspector/client/src/hooks/useRegistryServers.ts`:
- Around line 294-313: The connect function currently awaits connectMutation
which, if it throws, prevents onConnect from running; wrap the Convex call in a
try/catch so that errors are caught and do not block the local MCP connection:
call connectMutation(...) inside a try and in the catch log or surface the error
(e.g., via an error handler, toast, or processLogger) while still proceeding to
call onConnect with the same payload; keep the existing
DEV_MOCK_REGISTRY/isAuthenticated/workspaceId guard around the mutation and
ensure you reference the same symbols (connect, connectMutation, onConnect,
DEV_MOCK_REGISTRY, isAuthenticated, workspaceId, RegistryServer) when making the
change.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 22-30: getConvexSiteUrl currently unconditionally replaces
".convex.cloud" which can return an incorrect URL for custom domains; update
getConvexSiteUrl to check that (import.meta as any).env.VITE_CONVEX_URL matches
the expected ".convex.cloud" pattern (e.g. using a regex like
/\.convex\.cloud$/) before performing the replacement, and if it doesn't match
return null (or fall back to a validated site URL). Also normalize/trim any
trailing slashes on the input (VITE_CONVEX_URL) so the replacement validation is
reliable; reference the getConvexSiteUrl function and the
VITE_CONVEX_URL/VITE_CONVEX_SITE_URL env vars.
- Around line 90-104: The refresh-detection logic in the fetch wrapper (around
serializeBody and isRefresh) only inspects object bodies and will miss
URL-encoded or JSON string payloads; update the isRefresh check in the function
that calls serializeBody (the block computing isRefresh and endpoint before
calling originalFetch) to also handle string bodies by parsing them (e.g., try
JSON.parse for JSON strings, and fallback to new URLSearchParams for
form-encoded strings) and then checking parsed.grant_type === "refresh_token";
ensure the parsed value is used only for detection and the original serialized
body is still forwarded in the POST body payload formatting logic used when
calling originalFetch.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/ServersTab.tsx`:
- Around line 177-180: The useQuery call in ServersTab uses unsafe any casts for
the query key and arguments which bypass Convex's type safety; import the
generated api (import { api } from "../../convex/_generated/api") and replace
the string/any usage with the function reference
api.registryServers.listRegistryServers as the first argument to useQuery, and
pass a typed argument (use {} when isAuthenticated is true, otherwise "skip") so
TypeScript can infer the correct return type for registryServers and remove the
({} as any) and the string cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72f55654-a75a-4d24-9cfb-7d68f747a6d3
📒 Files selected for processing (5)
mcpjam-inspector/client/src/components/RegistryTab.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/__tests__/RegistryTab.test.tsxmcpjam-inspector/client/src/hooks/useRegistryServers.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/client/src/components/RegistryTab.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/components/tests/RegistryTab.test.tsx
| activeWorkspaceId: string; | ||
| isLoadingWorkspaces?: boolean; | ||
| onWorkspaceShared?: (sharedWorkspaceId: string) => void; | ||
| onLeaveWorkspace?: () => void; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if onLeaveWorkspace is used anywhere in this component
rg -n 'onLeaveWorkspace' mcpjam-inspector/client/src/components/ServersTab.tsx | grep -v 'onLeaveWorkspace?' | grep -v 'onLeaveWorkspace:' | grep -v 'onLeaveWorkspace,' || echo "No usage found beyond declaration/destructuring"Repository: MCPJam/inspector
Length of output: 107
🏁 Script executed:
cat -n mcpjam-inspector/client/src/components/ServersTab.tsx | head -200Repository: MCPJam/inspector
Length of output: 7716
🏁 Script executed:
cat -n mcpjam-inspector/client/src/components/ServersTab.tsx | tail -n +200 | head -300Repository: MCPJam/inspector
Length of output: 11266
🏁 Script executed:
cat -n mcpjam-inspector/client/src/components/ServersTab.tsx | tail -n +500 | head -300Repository: MCPJam/inspector
Length of output: 10316
onLeaveWorkspace prop is declared but unused.
This callback is destructured in the component but never invoked or passed to any child components. Remove it to eliminate dead code.
🤖 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 154, The prop
onLeaveWorkspace declared on the ServersTab component is never used—remove it
from the props interface/type and from the destructuring in the ServersTab
function to eliminate dead code (ensure there are no references to
onLeaveWorkspace elsewhere in the file or children); if the callback is actually
needed later, instead wire it into the appropriate handler (e.g., pass to a
child or call from a button click), otherwise delete the onLeaveWorkspace symbol
from the component signature and any related imports/uses.
Comment on lines
+22
to
+30
| function getConvexSiteUrl(): string | null { | ||
| const siteUrl = (import.meta as any).env?.VITE_CONVEX_SITE_URL; | ||
| if (siteUrl) return siteUrl; | ||
| const cloudUrl = (import.meta as any).env?.VITE_CONVEX_URL; | ||
| if (cloudUrl && typeof cloudUrl === "string") { | ||
| return cloudUrl.replace(".convex.cloud", ".convex.site"); | ||
| } | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
getConvexSiteUrl may return malformed URL for non-standard deployments.
If VITE_CONVEX_URL contains a custom domain (not .convex.cloud), the replacement produces an unchanged URL that won't route to Convex HTTP actions. Consider validating the replacement succeeded or returning null when the pattern doesn't match.
🛠️ Suggested refinement
function getConvexSiteUrl(): string | null {
const siteUrl = (import.meta as any).env?.VITE_CONVEX_SITE_URL;
if (siteUrl) return siteUrl;
const cloudUrl = (import.meta as any).env?.VITE_CONVEX_URL;
if (cloudUrl && typeof cloudUrl === "string") {
- return cloudUrl.replace(".convex.cloud", ".convex.site");
+ if (cloudUrl.includes(".convex.cloud")) {
+ return cloudUrl.replace(".convex.cloud", ".convex.site");
+ }
}
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts` around lines 22 - 30,
getConvexSiteUrl currently unconditionally replaces ".convex.cloud" which can
return an incorrect URL for custom domains; update getConvexSiteUrl to check
that (import.meta as any).env.VITE_CONVEX_URL matches the expected
".convex.cloud" pattern (e.g. using a regex like /\.convex\.cloud$/) before
performing the replacement, and if it doesn't match return null (or fall back to
a validated site URL). Also normalize/trim any trailing slashes on the input
(VITE_CONVEX_URL) so the replacement validation is reliable; reference the
getConvexSiteUrl function and the VITE_CONVEX_URL/VITE_CONVEX_SITE_URL env vars.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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/hooks/use-server-state.ts (1)
41-75:⚠️ Potential issue | 🟠 MajorPreserve
registryServerIdoutside the first OAuth attempt.Line 720 fixes the initial
initiateOAuth()call, but the rest of the flow still drops this value.saveOAuthConfigToLocalStorage()rewritesmcp-oauth-config-*withoutregistryServerId, and theforceOAuthFlowbranch at Lines 1275-1292 clears storage before re-starting OAuth with only{ serverName, serverUrl }. After an edit, a cached-token reconnect, or a forced re-auth,mcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsloses the registry context and falls back to the generic proxy instead of/registry/oauth/*.Also applies to: 653-725, 1249-1292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 41 - 75, saveOAuthConfigToLocalStorage is dropping registryServerId from stored oauth data which causes mcp-oauth.ts to lose registry context; update saveOAuthConfigToLocalStorage to include formData.registryServerId (when present) into the oauthConfig object (and into clientInfo if appropriate) before JSON.stringify so the mcp-oauth flow can read it later, and adjust the forceOAuthFlow branch (the code that clears and restarts OAuth) to either preserve registryServerId in storage or re-write it when re-starting (keep keys `mcp-oauth-config-${formData.name}` and `mcp-client-${formData.name}` populated with registryServerId so `/registry/oauth/*` routing remains available).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/components/RegistryTab.tsx`:
- Around line 57-64: RegistryTab is pulling categories from useRegistryServers
but never applies them when rendering consolidatedServers; add a UI control
(e.g., a category select or tabs) in RegistryTab that stores the
selectedCategory in local state and filter consolidatedServers by that
selectedCategory before mapping/rendering, using the categories array returned
from useRegistryServers to populate the control; ensure the filter logic
(applied just before the render of consolidatedServers) respects a "All" option
and continues to work with existing connect/disconnect handlers.
- Around line 68-77: The redirect marker is stored using server.displayName but
the code that checks it (useEffect in RegistryTab, the localStorage key
"registry-pending-redirect") looks up servers by their internal key/name
supplied to onConnect; update the write and read of the pending-redirect marker
to use the server key/name that connect() passes (the name property from
onConnect in useRegistryServers.connect) instead of displayName so the lookup
servers[pending] and the cleanup both reference the same identifier; locate
usages in RegistryTab (the useEffect that reads "registry-pending-redirect" and
the code that sets that key) and change them to persist/read server.name (or the
exact property used in onConnect) consistently.
In `@mcpjam-inspector/client/src/hooks/useRegistryServers.ts`:
- Around line 275-280: The sort comparator used when building "ordered"
currently accepts only one parameter and returns -1/1 which yields inconsistent
ordering; update the comparator used on [...variants].sort(...) to a proper
two-parameter comparator (accepting a and b) that returns 0 when a.clientType
=== b.clientType and otherwise returns -1 when a.clientType === "app" and 1
otherwise; locate the sort call that defines "ordered" inside the loop over
groups.values() (and references "variants") and replace the single-arg
comparator with this two-arg comparator to ensure stable, deterministic
ordering.
- Around line 432-443: The disconnect function currently awaits
disconnectMutation and will never call onDisconnect if the mutation rejects, and
it also passes server.displayName which can mismatch connect's
getRegistryServerName(server); modify the disconnect flow in disconnect(server)
to call disconnectMutation inside a try/catch (only when !DEV_MOCK_REGISTRY &&
isAuthenticated && workspaceId) so failures are caught and do not prevent
proceeding, log or swallow the error, and always invoke onDisconnect with
getRegistryServerName(server) (not server.displayName) so the name matches the
connect flow; keep the existing workspace/auth checks around the mutation but
ensure onDisconnect is called unconditionally after the mutation attempt.
- Around line 380-395: The effect currently calls connectMutation(...) and
discards any rejection; update the call inside useEffect so the promise is
handled: after deleting the id from pendingServerIds and invoking
connectMutation({ registryServerId, workspaceId } as any), attach a .catch(err
=> { console.error("connectMutation failed for", registryServerId, err);
setPendingServerIds(prev => { const next = new Map(prev);
next.set(registryServerId, serverName); return next; }); }) (or use async/await
with try/catch) to log the error and re-add the registryServerId to
pendingServerIds so the UI/state can retry; reference the existing symbols
pendingServerIds, liveServers, setPendingServerIds, and connectMutation when
making the change.
In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts`:
- Around line 9-29: Update the hoisted mocks so they use the injected fetch
function from the auth(...) call instead of calling window.fetch directly:
modify the mockSdkAuth implementation to accept the options parameter and call
options.fetchFn (or fallback to global fetch) for proxy-response cases, and
ensure mockFetchToken/mockSelectResourceURL (the mocks used by mcp-oauth tests)
also use the provided fetchFn when present; this keeps tests exercising the
proxy path and prevents real network calls.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 41-75: saveOAuthConfigToLocalStorage is dropping registryServerId
from stored oauth data which causes mcp-oauth.ts to lose registry context;
update saveOAuthConfigToLocalStorage to include formData.registryServerId (when
present) into the oauthConfig object (and into clientInfo if appropriate) before
JSON.stringify so the mcp-oauth flow can read it later, and adjust the
forceOAuthFlow branch (the code that clears and restarts OAuth) to either
preserve registryServerId in storage or re-write it when re-starting (keep keys
`mcp-oauth-config-${formData.name}` and `mcp-client-${formData.name}` populated
with registryServerId so `/registry/oauth/*` routing remains available).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad65afe2-cb57-4551-9668-0110c089ccef
📒 Files selected for processing (13)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/RegistryTab.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/__tests__/RegistryTab.test.tsxmcpjam-inspector/client/src/hooks/__tests__/consolidateServers.test.tsmcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/useRegistryServers.tsmcpjam-inspector/client/src/lib/hosted-oauth-callback.tsmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/state/oauth-orchestrator.tsmcpjam-inspector/shared/types.ts
✅ Files skipped from review due to trivial changes (3)
- mcpjam-inspector/client/src/lib/hosted-oauth-callback.ts
- mcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.ts
- mcpjam-inspector/client/src/components/tests/RegistryTab.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/components/ServersTab.tsx
Comment on lines
+57
to
+64
| const { registryServers, categories, isLoading, connect, disconnect } = | ||
| useRegistryServers({ | ||
| workspaceId, | ||
| isAuthenticated, | ||
| liveServers: servers, | ||
| onConnect, | ||
| onDisconnect, | ||
| }); |
Contributor
There was a problem hiding this comment.
Category filtering is still missing from the tab.
Lines 57-58 pull categories, but the render path at Lines 127-149 always shows the full consolidatedServers array and exposes no filter control. That leaves one of the Registry tab’s promised user-facing actions unimplemented.
Also applies to: 79-82, 127-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/RegistryTab.tsx` around lines 57 - 64,
RegistryTab is pulling categories from useRegistryServers but never applies them
when rendering consolidatedServers; add a UI control (e.g., a category select or
tabs) in RegistryTab that stores the selectedCategory in local state and filter
consolidatedServers by that selectedCategory before mapping/rendering, using the
categories array returned from useRegistryServers to populate the control;
ensure the filter logic (applied just before the render of consolidatedServers)
respects a "All" option and continues to work with existing connect/disconnect
handlers.
Comment on lines
+68
to
+77
| useEffect(() => { | ||
| if (!onNavigate) return; | ||
| const pending = localStorage.getItem("registry-pending-redirect"); | ||
| if (!pending) return; | ||
| const liveServer = servers?.[pending]; | ||
| if (liveServer?.connectionStatus === "connected") { | ||
| localStorage.removeItem("registry-pending-redirect"); | ||
| onNavigate("app-builder"); | ||
| } | ||
| }, [servers, onNavigate]); |
Contributor
There was a problem hiding this comment.
Use the actual connected server key for the redirect marker.
Line 86 persists server.displayName, but the lookup at Line 72 indexes servers by the local server name. In mcpjam-inspector/client/src/hooks/useRegistryServers.ts:416-430, connect() calls onConnect({ name: serverName, ... }), so any case where that derived serverName differs from displayName will never satisfy Lines 72-75. The marker and the cleanup path should use the same server key that connect() hands to app state.
Also applies to: 84-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/RegistryTab.tsx` around lines 68 - 77,
The redirect marker is stored using server.displayName but the code that checks
it (useEffect in RegistryTab, the localStorage key "registry-pending-redirect")
looks up servers by their internal key/name supplied to onConnect; update the
write and read of the pending-redirect marker to use the server key/name that
connect() passes (the name property from onConnect in
useRegistryServers.connect) instead of displayName so the lookup
servers[pending] and the cleanup both reference the same identifier; locate
usages in RegistryTab (the useEffect that reads "registry-pending-redirect" and
the code that sets that key) and change them to persist/read server.name (or the
exact property used in onConnect) consistently.
Comment on lines
+275
to
+280
| for (const variants of groups.values()) { | ||
| // App before text | ||
| const ordered = [...variants].sort((a) => | ||
| a.clientType === "app" ? -1 : 1, | ||
| ); | ||
| result.push({ variants: ordered, hasDualType: variants.length > 1 }); |
Contributor
There was a problem hiding this comment.
Sort comparator signature is incorrect.
The comparator receives only a, ignoring the second element b. This yields inconsistent results when comparing elements of the same type—potentially producing non-deterministic ordering.
🔧 Proper two-parameter comparator
- const ordered = [...variants].sort((a) =>
- a.clientType === "app" ? -1 : 1,
- );
+ const ordered = [...variants].sort((a, b) =>
+ (a.clientType === "app" ? 0 : 1) - (b.clientType === "app" ? 0 : 1),
+ );📝 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
| for (const variants of groups.values()) { | |
| // App before text | |
| const ordered = [...variants].sort((a) => | |
| a.clientType === "app" ? -1 : 1, | |
| ); | |
| result.push({ variants: ordered, hasDualType: variants.length > 1 }); | |
| for (const variants of groups.values()) { | |
| // App before text | |
| const ordered = [...variants].sort((a, b) => | |
| (a.clientType === "app" ? 0 : 1) - (b.clientType === "app" ? 0 : 1), | |
| ); | |
| result.push({ variants: ordered, hasDualType: variants.length > 1 }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/hooks/useRegistryServers.ts` around lines 275 -
280, The sort comparator used when building "ordered" currently accepts only one
parameter and returns -1/1 which yields inconsistent ordering; update the
comparator used on [...variants].sort(...) to a proper two-parameter comparator
(accepting a and b) that returns 0 when a.clientType === b.clientType and
otherwise returns -1 when a.clientType === "app" and 1 otherwise; locate the
sort call that defines "ordered" inside the loop over groups.values() (and
references "variants") and replace the single-arg comparator with this two-arg
comparator to ensure stable, deterministic ordering.
Comment on lines
+380
to
+395
| useEffect(() => { | ||
| if (!isAuthenticated || !workspaceId || DEV_MOCK_REGISTRY) return; | ||
| for (const [registryServerId, serverName] of pendingServerIds) { | ||
| const liveServer = liveServers?.[serverName]; | ||
| if (liveServer?.connectionStatus === "connected") { | ||
| setPendingServerIds((prev) => { | ||
| const next = new Map(prev); | ||
| next.delete(registryServerId); | ||
| return next; | ||
| }); | ||
| connectMutation({ | ||
| registryServerId, | ||
| workspaceId, | ||
| } as any); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Mutation errors in effect are silently discarded.
When connectMutation rejects, the promise is orphaned—no logging, no user feedback. The local connection succeeds while Convex state remains stale.
🛡️ Catch and log the error
setPendingServerIds((prev) => {
const next = new Map(prev);
next.delete(registryServerId);
return next;
});
- connectMutation({
+ connectMutation({
registryServerId,
workspaceId,
- } as any);
+ } as any).catch((err) => {
+ console.error("Failed to persist registry connection:", err);
+ });
}📝 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
| useEffect(() => { | |
| if (!isAuthenticated || !workspaceId || DEV_MOCK_REGISTRY) return; | |
| for (const [registryServerId, serverName] of pendingServerIds) { | |
| const liveServer = liveServers?.[serverName]; | |
| if (liveServer?.connectionStatus === "connected") { | |
| setPendingServerIds((prev) => { | |
| const next = new Map(prev); | |
| next.delete(registryServerId); | |
| return next; | |
| }); | |
| connectMutation({ | |
| registryServerId, | |
| workspaceId, | |
| } as any); | |
| } | |
| } | |
| useEffect(() => { | |
| if (!isAuthenticated || !workspaceId || DEV_MOCK_REGISTRY) return; | |
| for (const [registryServerId, serverName] of pendingServerIds) { | |
| const liveServer = liveServers?.[serverName]; | |
| if (liveServer?.connectionStatus === "connected") { | |
| setPendingServerIds((prev) => { | |
| const next = new Map(prev); | |
| next.delete(registryServerId); | |
| return next; | |
| }); | |
| connectMutation({ | |
| registryServerId, | |
| workspaceId, | |
| } as any).catch((err) => { | |
| console.error("Failed to persist registry connection:", err); | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/hooks/useRegistryServers.ts` around lines 380 -
395, The effect currently calls connectMutation(...) and discards any rejection;
update the call inside useEffect so the promise is handled: after deleting the
id from pendingServerIds and invoking connectMutation({ registryServerId,
workspaceId } as any), attach a .catch(err => { console.error("connectMutation
failed for", registryServerId, err); setPendingServerIds(prev => { const next =
new Map(prev); next.set(registryServerId, serverName); return next; }); }) (or
use async/await with try/catch) to log the error and re-add the registryServerId
to pendingServerIds so the UI/state can retry; reference the existing symbols
pendingServerIds, liveServers, setPendingServerIds, and connectMutation when
making the change.
Comment on lines
+432
to
+443
| async function disconnect(server: RegistryServer) { | ||
| // 1. Remove the connection from Convex (only when authenticated with a workspace) | ||
| if (!DEV_MOCK_REGISTRY && isAuthenticated && workspaceId) { | ||
| await disconnectMutation({ | ||
| registryServerId: server._id, | ||
| workspaceId, | ||
| } as any); | ||
| } | ||
|
|
||
| // 2. Trigger the local MCP disconnection | ||
| onDisconnect?.(server.displayName); | ||
| } |
Contributor
There was a problem hiding this comment.
Two concerns in disconnect flow.
-
Mutation failure blocks local disconnection. If
disconnectMutationrejects,onDisconnectnever fires—mirroring the original connect concern. -
Name mismatch.
connectpassesgetRegistryServerName(server)(e.g.,"Notion (App)"), butdisconnectpassesserver.displayName(e.g.,"Notion"). For servers withclientType, the names won't align.
🔧 Proposed fix
async function disconnect(server: RegistryServer) {
+ const serverName = getRegistryServerName(server);
+
// 1. Remove the connection from Convex (only when authenticated with a workspace)
if (!DEV_MOCK_REGISTRY && isAuthenticated && workspaceId) {
- await disconnectMutation({
- registryServerId: server._id,
- workspaceId,
- } as any);
+ try {
+ await disconnectMutation({
+ registryServerId: server._id,
+ workspaceId,
+ } as any);
+ } catch (err) {
+ console.error("Failed to remove registry connection:", err);
+ // Continue anyway — local disconnection can still proceed
+ }
}
// 2. Trigger the local MCP disconnection
- onDisconnect?.(server.displayName);
+ onDisconnect?.(serverName);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/hooks/useRegistryServers.ts` around lines 432 -
443, The disconnect function currently awaits disconnectMutation and will never
call onDisconnect if the mutation rejects, and it also passes server.displayName
which can mismatch connect's getRegistryServerName(server); modify the
disconnect flow in disconnect(server) to call disconnectMutation inside a
try/catch (only when !DEV_MOCK_REGISTRY && isAuthenticated && workspaceId) so
failures are caught and do not prevent proceeding, log or swallow the error, and
always invoke onDisconnect with getRegistryServerName(server) (not
server.displayName) so the name matches the connect flow; keep the existing
workspace/auth checks around the mutation but ensure onDisconnect is called
unconditionally after the mutation attempt.
Comment on lines
+9
to
29
| const { | ||
| mockDiscoverAuthorizationServerMetadata, | ||
| mockDiscoverOAuthServerInfo, | ||
| mockFetchToken, | ||
| mockSdkAuth, | ||
| mockSelectResourceURL, | ||
| } = vi.hoisted(() => ({ | ||
| mockDiscoverAuthorizationServerMetadata: vi.fn(), | ||
| mockDiscoverOAuthServerInfo: vi.fn(), | ||
| mockFetchToken: vi.fn(), | ||
| mockSdkAuth: vi.fn(), | ||
| mockSelectResourceURL: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@modelcontextprotocol/sdk/client/auth.js", () => ({ | ||
| auth: mockSdkAuth, | ||
| discoverAuthorizationServerMetadata: mockDiscoverAuthorizationServerMetadata, | ||
| discoverOAuthServerInfo: mockDiscoverOAuthServerInfo, | ||
| fetchToken: mockFetchToken, | ||
| selectResourceURL: mockSelectResourceURL, | ||
| })); |
Contributor
There was a problem hiding this comment.
The older auth mocks still bypass the injected fetchFn.
After mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts switched to passing the interceptor via auth(..., { fetchFn }), the proxy-response cases at Lines 147, 180, and 207 still call window.fetch(...) inside mockSdkAuth. Those tests no longer exercise the proxy path and can leak real network calls in Vitest.
🛠️ Suggested adjustment
-mockSdkAuth.mockImplementation(async () => {
- const response = await window.fetch(
- "https://example.com/.well-known/oauth-protected-resource/mcp",
- );
+mockSdkAuth.mockImplementation(async (_provider, options) => {
+ const response = await options.fetchFn!(
+ "https://example.com/.well-known/oauth-protected-resource/mcp",
+ );
if (!response.ok) {
throw new Error(`HTTP ${response.status}`);
}
return "AUTHORIZED";
});Also applies to: 303-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts` around
lines 9 - 29, Update the hoisted mocks so they use the injected fetch function
from the auth(...) call instead of calling window.fetch directly: modify the
mockSdkAuth implementation to accept the options parameter and call
options.fetchFn (or fallback to global fetch) for proxy-response cases, and
ensure mockFetchToken/mockSelectResourceURL (the mocks used by mcp-oauth tests)
also use the provided fetchFn when present; this keeps tests exercising the
proxy path and prevents real network calls.
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
2 participants
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
Added a new Registry tab that displays pre-configured MCP servers from a registry, allowing users to connect with one click and featuring OAuth integration through Convex HTTP actions.
What changed?
RegistryTabcomponent with server cards showing connection status, OAuth badges, and category filteringuseRegistryServershook to fetch registry data and manage connections with live server state/registry/oauth/*endpoints for registry servers