fix(playground): let the lead client be unchecked in the compare picker#3370
Conversation
Unchecking the lead client (the one badged GLOBAL) did nothing visible
except silently delete a different client from the compare line-up.
The lead is stored apart from the line-up, and `usePersistedHost`
re-derives the line-up on every read. `deriveSelectedHostIds` has a
defensive branch for external surfaces that move the lead without
rotating the array ("lead not in array -> replace slot 0"), but it can't
tell that apart from the user removing the lead from the array. So:
stored [claude, slackbot, chatgpt] + lead claude
-> uncheck claude -> stored [slackbot, chatgpt]
-> derive re-inserts claude at slot 0 -> [claude, chatgpt]
Claude never leaves and slackbot is gone. Repeating the click eats one
more client each time until only the un-removable lead is left, which
reads as "clicking it unchecked everything".
`setSelectedHostIds` is an explicit in-app write, so it is authoritative
about the line-up: when it drops the current lead from a non-empty
array, the lead now follows to the new slot 0. Clearing to `[]` stays a
non-lead-change — that path means "fall back to the lead as the single
implicit selection", so the lead has to survive it.
One change in the hook covers both surfaces (chat-input ClientSelector
and the header MultiHostPicker); both route their writes through it.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
GitHub Actions
Inactive
dosubot
Bot
added
size:M
✅ 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughUpdated 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
mcpjam-inspector/client/src/components/chat-v2/__tests__/client-selector.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/client/src/hooks/__tests__/use-persisted-host.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/client/src/hooks/use-persisted-host.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
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 |
Internal previewPreview URL: https://mcp-inspector-pr-3370.up.railway.app |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
prathmeshpatel
left a comment
prathmeshpatel
left a comment
There was a problem hiding this comment.
LGTM, thanks for the quick iteration
GitHub Actions
Inactive
3 participants
Problem
In the Playground client picker with Multiple clients on, the lead client — the one badged
GLOBAL— cannot be unchecked. Clicking its row appears to do nothing, except that a different client silently disappears from the line-up. Clicking repeatedly eats one client per click until only the un-removable lead is left, which reads as "clicking it unchecked everything".Root cause
Not in the picker UI. The lead is stored apart from the compare line-up (
mcp-previewed-host-idvsmcp-inspector-selected-hosts:{projectId}), andusePersistedHostre-derives the line-up on every read.deriveSelectedHostIdshas a deliberate defensive branch for external surfaces (global host bar, project setup) that move the lead without rotating the array: "lead not in array → replace slot 0 with it, never grow". It cannot tell that case apart from "the user just removed the lead from the array", so:Fix
setSelectedHostIdsis an explicit in-app write, so it is authoritative about the line-up: when it drops the current lead from a non-empty array, the lead now follows to the new slot 0.Clearing to
[]deliberately stays a non-lead-change — that path (PlaygroundMain.handleMultiModelEnabledChange) means "fall back to the lead as the single implicit selection", so the lead has to survive it.One change in the hook covers both surfaces: the chat-input
ClientSelectorand the headerMultiHostPickerboth route their writes through it. The count-preservation contract and the external-savePreviewedHostIdderivation branches are untouched.Testing
mainexactly as described (['a','c']instead of['b','c']).use-persisted-host,MultiHostPicker,client-selector,PlaygroundMain.multi-host— 59 tests.GLOBAL, and leaves the third intact; unchecking again collapses cleanly to the survivor.Notes / not in scope
MultiHostPicker.test.tsx:297). The checkbox row is the removal path and now works; happy to make the two consistent if preferred.GLOBALbadge just means "lead / slot 0" and reads as confusing. Renaming it toLeadis a one-line change plus a test — left out to keep this diff to the bug.Summary by cubic
Fixes the Playground compare picker so the
Globallead can be unchecked without deleting other clients. Removing the lead from a non-empty lineup promotes the next client; clearing the list keeps the lead for single-host fallback.setSelectedHostIdsnow updates the lead to the new slot 0 when the current lead is dropped; clearing to[]preserves the lead.ClientSelectorandMultiHostPickerviausePersistedHost.usePersistedHostand 1 forClientSelector; affected suites are green.Written for commit 3bc0282. Summary will update on new commits.