wrongly persisted servers through project switch#2015
wrongly persisted servers through project switch#2015ignaciojimenezr wants to merge 2 commits intomainfrom
Conversation
dosubot
Bot
added
the
size:L
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
ignaciojimenezr
temporarily deployed
to
preview-pr-2015
GitHub Actions
Inactive
✅ 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. |
Internal previewPreview URL: https://mcp-inspector-pr-2015.up.railway.app |
WalkthroughAdds a typed 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 820-838: The sign-out currently only disconnects servers whose
connectionStatus === "connected", leaving "connecting" or "oauth-flow" attempts
running; update handleSignOut to collect server names where
appState.servers[serverName].connectionStatus is not "disconnected" (or
explicitly include "connected", "connecting", "oauth-flow", etc.) and call
handleDisconnect for all of them before proceeding to signOut, ensuring you
still await Promise.allSettled on connected/connecting servers; references:
handleSignOut, appState.servers, handleDisconnect, signOut.
In `@mcpjam-inspector/client/src/hooks/use-app-state.ts`:
- Around line 501-524: The effect disconnects servers by bare serverName from
previousServersRef.current when effectiveActiveProjectId changes, which can
incorrectly close homonymous servers in other projects; update the logic to
include project context by either (a) passing the previous project id
(previousActiveProjectIdRef.current or a captured previousActiveProjectId
variable) into handleDisconnect (e.g. change handleDisconnect signature to
accept projectId + serverName) or (b) filtering previousServersRef.current to
only those servers that belong to the previous project before calling
handleDisconnect; update all callsites and types for handleDisconnect and adjust
logger calls (logger.info) to include the project id so disconnects target the
correct transport.
🪄 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: e5e8db09-956b-428e-88bb-620dcbd2b058
📒 Files selected for processing (6)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/components/sidebar/sidebar-user.tsxmcpjam-inspector/client/src/hooks/__tests__/use-app-state.test.tsxmcpjam-inspector/client/src/hooks/use-app-state.ts
| useEffect(() => { | ||
| const previousActiveProjectId = previousActiveProjectIdRef.current; | ||
| previousActiveProjectIdRef.current = effectiveActiveProjectId; | ||
|
|
||
| if ( | ||
| previousActiveProjectId === effectiveActiveProjectId || | ||
| previousActiveProjectId === "none" | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| for (const [serverName, server] of Object.entries( | ||
| previousServersRef.current | ||
| )) { | ||
| if (server.connectionStatus === "connected") { | ||
| logger.info("Disconnecting server on project change", { | ||
| serverName, | ||
| from: previousActiveProjectId, | ||
| to: effectiveActiveProjectId, | ||
| }); | ||
| void handleDisconnect(serverName); | ||
| } | ||
| } | ||
| }, [effectiveActiveProjectId, handleDisconnect, logger]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- use-server-state.ts ---\n'
fd -i 'use-server-state.ts' mcpjam-inspector/client/src -x sed -n '1,260p' {}
printf '\n--- handleDisconnect definitions and call sites ---\n'
rg -n -C4 '\bhandleDisconnect\b' mcpjam-inspector/client/src
printf '\n--- places where runtime/project servers are keyed by bare name ---\n'
rg -n -C3 'Record<string,\s*ServerWithName>|servers:\s*Record<string|Object\.entries\(.*servers' mcpjam-inspector/client/srcRepository: MCPJam/inspector
Length of output: 156
🏁 Script executed:
cat mcpjam-inspector/client/src/hooks/use-server-state.ts | head -300Repository: MCPJam/inspector
Length of output: 9577
🏁 Script executed:
rg -n 'handleDisconnect' mcpjam-inspector/client/src -A 5 -B 2Repository: MCPJam/inspector
Length of output: 25698
🏁 Script executed:
rg -n 'Record<string' mcpjam-inspector/client/src/hooksRepository: MCPJam/inspector
Length of output: 10116
🏁 Script executed:
rg -n 'servers' mcpjam-inspector/client/src/hooks/use-app-state.ts | head -30Repository: MCPJam/inspector
Length of output: 1199
Project-agnostic disconnect leaves homonymous servers ambiguous.
When the active project changes, this effect runs after effectiveActiveProjectId has already shifted, then disconnects all previously connected servers using only their bare names. The registry—both appState.servers and project.servers—is keyed solely by server name. If two projects each contain github, linear, or other common names, calling handleDisconnect(serverName) has no way to close the right transport; the teardown targets the global registry entry without project context, risking cross-project interference.
for (const [serverName, server] of Object.entries(
previousServersRef.current
)) {
if (server.connectionStatus === "connected") {
void handleDisconnect(serverName); // ← no project context
}
}
Pass the previous project ID (or a connection-scoped identifier) to the disconnect handler, or filter disconnects to only servers that belong to the previous project.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/hooks/use-app-state.ts` around lines 501 - 524,
The effect disconnects servers by bare serverName from
previousServersRef.current when effectiveActiveProjectId changes, which can
incorrectly close homonymous servers in other projects; update the logic to
include project context by either (a) passing the previous project id
(previousActiveProjectIdRef.current or a captured previousActiveProjectId
variable) into handleDisconnect (e.g. change handleDisconnect signature to
accept projectId + serverName) or (b) filtering previousServersRef.current to
only those servers that belong to the previous project before calling
handleDisconnect; update all callsites and types for handleDisconnect and adjust
logger calls (logger.info) to include the project id so disconnects target the
correct transport.
There was a problem hiding this comment.
can't have projects live connections coexisting
There was a problem hiding this comment.
@ignaciojimenezr, understood — if the architecture enforces that only one project may hold live connections at a time, cross-project name collisions are structurally impossible, making the bare-name disconnect perfectly safe. My concern doesn't apply here.
✏️ Learnings added
Learnt from: ignaciojimenezr
Repo: MCPJam/inspector PR: 2015
File: mcpjam-inspector/client/src/hooks/use-app-state.ts:501-524
Timestamp: 2026-05-05T00:05:01.321Z
Learning: In the MCPJam inspector codebase (`mcpjam-inspector/client/src/hooks/use-app-state.ts`), only one project can have live MCP server connections at any given time — projects cannot coexist with active connections. Therefore, disconnecting servers by bare server name (without project context) on project switch is intentional and safe; there is no risk of cross-project interference via homonymous server names.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsx`:
- Around line 1586-1635: Change the test so it proves onSignOut awaits the
disconnect promises by making handleDisconnect return controllable deferred
promises (not immediate resolved values) and only resolving them after onSignOut
is invoked; specifically, replace handleDisconnect.mockResolvedValue(undefined)
with a mockImplementation that for each server id
("connected","connecting","oauth") returns a Promise whose resolve functions you
capture (e.g., deferreds), call sidebarProps.onSignOut(...), then resolve those
deferreds and assert signOut was invoked only after the deferreds resolved
(e.g., check signOut was called and/or use invocation order assertions after
resolving the disconnect promises); keep using mockUseAppState, mockUseAuth,
handleDisconnect, signOut, and sidebarProps.onSignOut to locate the changes.
🪄 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: 03ca7f70-c99b-4caf-b173-e73691c14fd1
📒 Files selected for processing (4)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-app-state.test.tsxmcpjam-inspector/client/src/hooks/use-app-state.ts
| it("disconnects in-flight servers before signing out from the sidebar", async () => { | ||
| clearHostedOAuthPendingState(); | ||
| clearChatboxSession(); | ||
| window.history.replaceState({}, "", "/#servers"); | ||
|
|
||
| const signOut = vi.fn(); | ||
| const handleDisconnect = vi.fn().mockResolvedValue(undefined); | ||
| mockUseAuth.mockReturnValue({ | ||
| ...mockWorkOsAuthState, | ||
| signOut, | ||
| }); | ||
| mockUseAppState.mockReturnValue({ | ||
| ...createAppStateMock(), | ||
| appState: { | ||
| servers: { | ||
| connected: { connectionStatus: "connected" }, | ||
| connecting: { connectionStatus: "connecting" }, | ||
| oauth: { connectionStatus: "oauth-flow" }, | ||
| disconnected: { connectionStatus: "disconnected" }, | ||
| }, | ||
| selectedServer: "none", | ||
| selectedMultipleServers: [], | ||
| }, | ||
| handleDisconnect, | ||
| }); | ||
|
|
||
| render(<App />); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockMCPSidebar).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| const lastCall = | ||
| mockMCPSidebar.mock.calls[mockMCPSidebar.mock.calls.length - 1]; | ||
| const sidebarProps = lastCall?.[0] as unknown as { | ||
| onSignOut?: (options?: { returnTo?: string }) => Promise<void>; | ||
| }; | ||
|
|
||
| await act(async () => { | ||
| await sidebarProps.onSignOut?.({ returnTo: "https://example.test" }); | ||
| }); | ||
|
|
||
| expect(handleDisconnect).toHaveBeenCalledWith("connected"); | ||
| expect(handleDisconnect).toHaveBeenCalledWith("connecting"); | ||
| expect(handleDisconnect).toHaveBeenCalledWith("oauth"); | ||
| expect(handleDisconnect).not.toHaveBeenCalledWith("disconnected"); | ||
| expect(signOut).toHaveBeenCalledWith({ returnTo: "https://example.test" }); | ||
| expect(signOut.mock.invocationCallOrder[0]).toBeGreaterThan( | ||
| Math.max(...handleDisconnect.mock.invocationCallOrder.slice(0, 3)) | ||
| ); |
There was a problem hiding this comment.
Make this test prove the await, not just the call order.
Right now it only shows signOut happens after the three handleDisconnect calls are started. It would still pass if handleSignOut stopped awaiting Promise.allSettled. A deferred disconnect promise here would pin the intended behavior down.
Suggested test hardening
- const handleDisconnect = vi.fn().mockResolvedValue(undefined);
+ let resolveDisconnect!: () => void;
+ const pendingDisconnect = new Promise<void>((resolve) => {
+ resolveDisconnect = resolve;
+ });
+ const handleDisconnect = vi.fn().mockImplementation((serverName: string) =>
+ serverName === "oauth" ? pendingDisconnect : Promise.resolve()
+ );
@@
- await act(async () => {
- await sidebarProps.onSignOut?.({ returnTo: "https://example.test" });
- });
+ const signOutPromise = sidebarProps.onSignOut?.({
+ returnTo: "https://example.test",
+ });
+
+ expect(signOut).not.toHaveBeenCalled();
+
+ await act(async () => {
+ resolveDisconnect();
+ await signOutPromise;
+ });
@@
- expect(signOut.mock.invocationCallOrder[0]).toBeGreaterThan(
- Math.max(...handleDisconnect.mock.invocationCallOrder.slice(0, 3))
- );
+ expect(signOut).toHaveBeenCalledWith({ returnTo: "https://example.test" });📝 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.
| it("disconnects in-flight servers before signing out from the sidebar", async () => { | |
| clearHostedOAuthPendingState(); | |
| clearChatboxSession(); | |
| window.history.replaceState({}, "", "/#servers"); | |
| const signOut = vi.fn(); | |
| const handleDisconnect = vi.fn().mockResolvedValue(undefined); | |
| mockUseAuth.mockReturnValue({ | |
| ...mockWorkOsAuthState, | |
| signOut, | |
| }); | |
| mockUseAppState.mockReturnValue({ | |
| ...createAppStateMock(), | |
| appState: { | |
| servers: { | |
| connected: { connectionStatus: "connected" }, | |
| connecting: { connectionStatus: "connecting" }, | |
| oauth: { connectionStatus: "oauth-flow" }, | |
| disconnected: { connectionStatus: "disconnected" }, | |
| }, | |
| selectedServer: "none", | |
| selectedMultipleServers: [], | |
| }, | |
| handleDisconnect, | |
| }); | |
| render(<App />); | |
| await waitFor(() => { | |
| expect(mockMCPSidebar).toHaveBeenCalled(); | |
| }); | |
| const lastCall = | |
| mockMCPSidebar.mock.calls[mockMCPSidebar.mock.calls.length - 1]; | |
| const sidebarProps = lastCall?.[0] as unknown as { | |
| onSignOut?: (options?: { returnTo?: string }) => Promise<void>; | |
| }; | |
| await act(async () => { | |
| await sidebarProps.onSignOut?.({ returnTo: "https://example.test" }); | |
| }); | |
| expect(handleDisconnect).toHaveBeenCalledWith("connected"); | |
| expect(handleDisconnect).toHaveBeenCalledWith("connecting"); | |
| expect(handleDisconnect).toHaveBeenCalledWith("oauth"); | |
| expect(handleDisconnect).not.toHaveBeenCalledWith("disconnected"); | |
| expect(signOut).toHaveBeenCalledWith({ returnTo: "https://example.test" }); | |
| expect(signOut.mock.invocationCallOrder[0]).toBeGreaterThan( | |
| Math.max(...handleDisconnect.mock.invocationCallOrder.slice(0, 3)) | |
| ); | |
| it("disconnects in-flight servers before signing out from the sidebar", async () => { | |
| clearHostedOAuthPendingState(); | |
| clearChatboxSession(); | |
| window.history.replaceState({}, "", "/#servers"); | |
| const signOut = vi.fn(); | |
| let resolveDisconnect!: () => void; | |
| const pendingDisconnect = new Promise<void>((resolve) => { | |
| resolveDisconnect = resolve; | |
| }); | |
| const handleDisconnect = vi.fn().mockImplementation((serverName: string) => | |
| serverName === "oauth" ? pendingDisconnect : Promise.resolve() | |
| ); | |
| mockUseAuth.mockReturnValue({ | |
| ...mockWorkOsAuthState, | |
| signOut, | |
| }); | |
| mockUseAppState.mockReturnValue({ | |
| ...createAppStateMock(), | |
| appState: { | |
| servers: { | |
| connected: { connectionStatus: "connected" }, | |
| connecting: { connectionStatus: "connecting" }, | |
| oauth: { connectionStatus: "oauth-flow" }, | |
| disconnected: { connectionStatus: "disconnected" }, | |
| }, | |
| selectedServer: "none", | |
| selectedMultipleServers: [], | |
| }, | |
| handleDisconnect, | |
| }); | |
| render(<App />); | |
| await waitFor(() => { | |
| expect(mockMCPSidebar).toHaveBeenCalled(); | |
| }); | |
| const lastCall = | |
| mockMCPSidebar.mock.calls[mockMCPSidebar.mock.calls.length - 1]; | |
| const sidebarProps = lastCall?.[0] as unknown as { | |
| onSignOut?: (options?: { returnTo?: string }) => Promise<void>; | |
| }; | |
| const signOutPromise = sidebarProps.onSignOut?.({ | |
| returnTo: "https://example.test", | |
| }); | |
| expect(signOut).not.toHaveBeenCalled(); | |
| await act(async () => { | |
| resolveDisconnect(); | |
| await signOutPromise; | |
| }); | |
| expect(handleDisconnect).toHaveBeenCalledWith("connected"); | |
| expect(handleDisconnect).toHaveBeenCalledWith("connecting"); | |
| expect(handleDisconnect).toHaveBeenCalledWith("oauth"); | |
| expect(handleDisconnect).not.toHaveBeenCalledWith("disconnected"); | |
| expect(signOut).toHaveBeenCalledWith({ returnTo: "https://example.test" }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsx` around lines
1586 - 1635, Change the test so it proves onSignOut awaits the disconnect
promises by making handleDisconnect return controllable deferred promises (not
immediate resolved values) and only resolving them after onSignOut is invoked;
specifically, replace handleDisconnect.mockResolvedValue(undefined) with a
mockImplementation that for each server id ("connected","connecting","oauth")
returns a Promise whose resolve functions you capture (e.g., deferreds), call
sidebarProps.onSignOut(...), then resolve those deferreds and assert signOut was
invoked only after the deferreds resolved (e.g., check signOut was called and/or
use invocation order assertions after resolving the disconnect promises); keep
using mockUseAppState, mockUseAuth, handleDisconnect, signOut, and
sidebarProps.onSignOut to locate the changes.
2 participants
What this PR does