Skip to content

Fix: connecting a server no longer un-toggles your other servers#2298

Open
ignaciojimenezr wants to merge 4 commits into
mainfrom
fix/playground-selection-wiped-on-connect
Open

Fix: connecting a server no longer un-toggles your other servers#2298
ignaciojimenezr wants to merge 4 commits into
mainfrom
fix/playground-selection-wiped-on-connect

Conversation

@ignaciojimenezr
Copy link
Copy Markdown
Collaborator

@ignaciojimenezr ignaciojimenezr commented May 28, 2026

  • When you connect a server, the app used to reset your server toggles to match your active Client's saved server list. Any server you'd switched on by hand got switched off — even though it stayed connected (still green in the dashboard). The toggle just silently flipped off.

  • Why it happened: connecting a server can make the active Client's required-server list re-resolve, and the code replaced your whole selection with that list, dropping anything you'd added manually.

  • The fix: switching to a different Client still snaps the selection to that Client's servers (unchanged). But within the same Client, re-resolving the list now merges in what's needed instead of replacing — so your manual toggles are never wiped.

  • Added a regression test covering the exact case: a manually-toggled server survives connecting another server.

Screen.Recording.2026-05-27.at.7.15.54.PM.mov

The playground/chat multi-select was reseeded from the active client's
required-server set whenever that set re-resolved — e.g. connecting
another server updates the project catalog so a client-referenced id
that didn't resolve before now does. Within the same host scope this
REPLACED the selection, silently dropping servers the user had toggled
on by hand: they stayed connected but their toggle flipped off.

Now the seed only REPLACES on a host-scope change (or the first resolve
for a surface); within the same scope it MERGES newly-required servers
in without removing the user's manual picks. Gated on a component-scoped
ref instead of shared module state so the hook's several mounts don't
thrash.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 28, 2026
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@dosubot dosubot Bot added the bug Something isn't working label May 28, 2026
@chelojimenez
Copy link
Copy Markdown
Contributor

chelojimenez commented May 28, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 28, 2026

Review Change Stack

Walkthrough

This PR refactors useAutoConnectProjectServers to preserve user-selected servers within a project scope. Instead of replacing the selection whenever requiredNames changes, the hook now seeds selection only when entering a new project scope, then merges subsequent required-server updates into the existing selection. Module-scoped refs track the latest selection state and which scope was last seeded. A regression test verifies that manually selected servers persist and merge with newly required servers when requiredNames updates mid-session.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 28, 2026

Internal preview

Preview URL: https://mcp-inspector-pr-2298.up.railway.app
Deployed commit: 80c2bdd
PR head commit: 2508374
Backend target: staging fallback.
Health: ❌ Convex unreachable — see upsert-preview job logs (staging may need convex deploy)
Access is employee-only in non-production environments.

The Servers popover used to render only servers in the project catalog
(activeProject.servers). The Tools pane already had the right view — it
reads from appState.servers, so any server connected on mcpjam-backend's
MCPClientManager shows up. The popover didn't, so connected servers that
hadn't synced into the catalog yet looked disconnected (or were absent),
and toggling the visible one made a previously-hidden connected server
"appear" on re-render.

Adds a `displayServerConfigs` derived map = catalog ∪ runtime-connected,
and routes chat-input's `allServerConfigs` (ChatTabV2 + Playground via
playgroundServerSelectorProps) through it. Keeps `activeServerSelectorProps`
(the header picker that also drives the OAuth Debugger / XAA tabs) on
the catalog-only `projectServers` so the existing cross-project / cross-
org leak guard for OAuth-sensitive surfaces stays intact.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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/App.tsx (1)

2758-2790: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Scope runtime-expanded server list to Playground-only here.

Line 2764 currently feeds displayServerConfigs into both App Builder and Playground selectors; App Builder is single-select and still resolves core selected config from catalog-backed state, so runtime-only entries can become selectable before they are resolvable.

Suggested patch
   const playgroundServerSelectorProps = useMemo(():
     | PlaygroundServerSelectorProps
     | undefined => {
     if (activeTab !== "app-builder" && activeTab !== "playground")
       return undefined;
     return {
-      serverConfigs: displayServerConfigs,
+      serverConfigs:
+        activeTab === "playground" ? displayServerConfigs : projectServers,
       selectedServer: appState.selectedServer,
       selectedMultipleServers: appState.selectedMultipleServers,
@@
   }, [
     activeTab,
     displayServerConfigs,
+    projectServers,
     appState.selectedServer,
     appState.selectedMultipleServers,
🤖 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/App.tsx` around lines 2758 - 2790,
playgroundServerSelectorProps currently passes displayServerConfigs to both App
Builder and Playground, allowing runtime-only (catalog-less) entries to be
selectable in App Builder; change the serverConfigs value in
playgroundServerSelectorProps so only the Playground receives the
runtime-expanded list and App Builder receives a catalog-backed list (e.g., when
activeTab === "playground" use displayServerConfigs, otherwise pass a
filtered/alternative list that excludes runtime-only entries). Update the
serverConfigs expression in the playgroundServerSelectorProps useMemo (and keep
isMultiSelectEnabled based on activeTab) so App Builder never gets runtime-only
configs.
🤖 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.

Outside diff comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 2758-2790: playgroundServerSelectorProps currently passes
displayServerConfigs to both App Builder and Playground, allowing runtime-only
(catalog-less) entries to be selectable in App Builder; change the serverConfigs
value in playgroundServerSelectorProps so only the Playground receives the
runtime-expanded list and App Builder receives a catalog-backed list (e.g., when
activeTab === "playground" use displayServerConfigs, otherwise pass a
filtered/alternative list that excludes runtime-only entries). Update the
serverConfigs expression in the playgroundServerSelectorProps useMemo (and keep
isMultiSelectEnabled based on activeTab) so App Builder never gets runtime-only
configs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b1438205-d565-4342-a873-47ce66f1a3bc

📥 Commits

Reviewing files that changed from the base of the PR and between 42b3178 and 24e318f.

📒 Files selected for processing (3)
  • mcpjam-inspector/client/src/App.tsx
  • mcpjam-inspector/client/src/hooks/use-app-state.ts
  • mcpjam-inspector/client/src/hooks/use-server-state.ts

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

Labels

bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants