Move host context to the preview surface#1928
Conversation
chelojimenez
temporarily deployed
to
preview-pr-1928
GitHub Actions
Inactive
dosubot
Bot
added
the
size:XXL
✅ 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-1928.up.railway.app |
WalkthroughThe change splits workspace configuration into separate connection-config and host-context drafts with new types and composition helpers. It adds a dedicated 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: 3
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-workspace-state.ts (1)
866-896:⚠️ Potential issue | 🔴 CriticalDo not reuse one pending-sync slot for both save paths.
Line 867 clears the single
pendingClientConfigSyncRefbefore registering the next waiter. With separate connection and host-context saves now funneled through this helper, save B can reject save A; save A'scatchthen callsclearPendingClientConfigSync()again and can tear down save B as well. The end state is both stores falling intofailSave()even if Convex persisted the writes. Queue these per workspace, or serialize saves until the prior remote echo completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts` around lines 866 - 896, The current logic in use-workspace-state.ts reuses pendingClientConfigSyncRef for all saves, so clearPendingClientConfigSync() can cancel a different save; change this to track pending syncs per workspace (e.g., replace pendingClientConfigSyncRef with a Map keyed by workspaceId) or serialize saves per workspace by awaiting an existing pending promise before creating a new remoteEchoPromise; ensure the new structure stores the same fields (workspaceId, expectedSerializedConfig, resolve, reject, timeoutId), update clearPendingClientConfigSync to clear only the entry for the current workspaceId, and keep convexUpdateClientConfig/remoteEchoPromise flow and controller.failSave behavior but scoped so one save cannot reject another workspace's pending save.
🧹 Nitpick comments (6)
mcpjam-inspector/client/src/hooks/use-workspace-client-config-sync-pending.ts (1)
4-17: Consider a name that reflects the dual subscription.The hook now ORs in host-context sync as well, so the
ClientConfig-only label slightly understates its scope. Something likeuseWorkspaceConfigSyncPendingoruseWorkspaceSavePendingwould more faithfully advertise that both connection-config and host-context echoes are gating the result. Purely cosmetic — behavior is sound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-workspace-client-config-sync-pending.ts` around lines 4 - 17, The hook useWorkspaceClientConfigSyncPending now also checks host-context sync, so rename the function to a clearer name (e.g. useWorkspaceConfigSyncPending or useWorkspaceSavePending) and update its export and all imports to match; ensure you update the function identifier in the declaration and any places that import or call useWorkspaceClientConfigSyncPending so consumers compile and behavior remains unchanged (the internal logic using useClientConfigStore and useHostContextStore stays the same).mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx (1)
200-212: Add one regression for host-context pending.This setup now mirrors the split-store model, but the suite still only proves the client-config half of
useWorkspaceClientConfigSyncPending. A targeted case withuseHostContextStore.setState({ pendingWorkspaceId: "default", isAwaitingRemoteEcho: true })would lock in the new branch.🤖 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 200 - 212, Add a regression test exercising the host-context pending branch by initializing the host store with useHostContextStore.setState({ pendingWorkspaceId: "default", isAwaitingRemoteEcho: true, ...other required keys }) and then asserting that useWorkspaceClientConfigSyncPending (or the hook under test) reports the pending state for that workspace; locate references to useHostContextStore, pendingWorkspaceId, isAwaitingRemoteEcho and useWorkspaceClientConfigSyncPending in the test file and add a targeted case mirroring the split-store model to validate the new branch.mcpjam-inspector/client/src/components/shared/HostContextHeader.tsx (2)
150-152: Hoist session-stable fallbacks out of the render path.
navigator.languageand the resolved time zone don't change for the lifetime of the tab, yet they're recomputed on every render. Lifting them to module scope (or wrapping inuseMemo) is cheap polish.♻️ Suggested tweak
- const fallbackLocale = navigator.language || "en-US"; - const fallbackTimeZone = - Intl.DateTimeFormat().resolvedOptions().timeZone || "UTC"; + const fallbackLocale = useMemo(() => navigator.language || "en-US", []); + const fallbackTimeZone = useMemo( + () => Intl.DateTimeFormat().resolvedOptions().timeZone || "UTC", + [], + );Or simply declare them as module-level constants outside the component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/shared/HostContextHeader.tsx` around lines 150 - 152, Move the per-tab constant computations out of the render path by hoisting navigator.language and Intl.DateTimeFormat().resolvedOptions().timeZone to module-level constants (e.g., replace the in-component fallbackLocale and fallbackTimeZone with module-scope constants) or memoize them once (e.g., via useMemo called at component init); update references to fallbackLocale and fallbackTimeZone in HostContextHeader so the values are computed only once for the lifetime of the tab.
405-407: Tooltip backticks render literally.The tooltip body renders the string verbatim, so users see the backticks rather than monospace-styled
hostContext. Either drop them or wrap the identifier in a<code>element.♻️ Suggested tweak
- <p className="text-xs text-muted-foreground"> - Edit raw `hostContext` JSON - </p> + <p className="text-xs text-muted-foreground"> + Edit raw <code>hostContext</code> JSON + </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/shared/HostContextHeader.tsx` around lines 405 - 407, The tooltip text currently includes backticks literally in the JSX paragraph inside HostContextHeader (the <p className="text-xs text-muted-foreground"> element) and should instead render the identifier as code or plain text; update the string "Edit raw `hostContext` JSON" to either "Edit raw hostContext JSON" or wrap the identifier in a <code> element (e.g., Edit raw <code>hostContext</code> JSON) so users see it styled as code rather than showing backticks.mcpjam-inspector/client/src/lib/client-config.ts (1)
102-102: Mark the alias deprecated to nudge callers forward.
buildDefaultHostContextexists purely as a transitional re-export. A@deprecatedtag turns it into a self-extinguishing migration aid rather than a permanent twin name.♻️ Suggested tweak
+/** `@deprecated` Use {`@link` buildDefaultWorkspaceHostContext} instead. */ export const buildDefaultHostContext = buildDefaultWorkspaceHostContext;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/client-config.ts` at line 102, Add a JSDoc deprecation tag to the transitional alias export so callers are nudged to the new name; update the export of buildDefaultHostContext (which re-exports buildDefaultWorkspaceHostContext) by adding a /** `@deprecated` Use buildDefaultWorkspaceHostContext instead. */ comment immediately above the export declaration, keeping the alias but marking it deprecated and mentioning the preferred symbol buildDefaultWorkspaceHostContext.mcpjam-inspector/client/src/test/mocks/stores.ts (1)
336-344: Preset silently dropsavailableDisplayModesif overrides provide their owndraftHostContext.A test that does
hostContextWithDisplayModes(["inline"], { draftHostContext: { theme: "dark" } })will lose theavailableDisplayModesit just asked for, because the override replaces the whole object. Merging keeps the preset's intent intact.♻️ Suggested tweak
hostContextWithDisplayModes: ( availableDisplayModes: HostDisplayMode[], overrides: Partial<MockHostContextStoreState> = {}, ) => createMockHostContextStoreState({ - draftHostContext: { availableDisplayModes }, ...overrides, + draftHostContext: { + availableDisplayModes, + ...(overrides.draftHostContext ?? {}), + }, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/test/mocks/stores.ts` around lines 336 - 344, The preset function hostContextWithDisplayModes currently passes draftHostContext: { availableDisplayModes } and then spreads overrides which can replace draftHostContext entirely; instead merge the passed availableDisplayModes into any override.draftHostContext so the preset value is preserved — e.g. call createMockHostContextStoreState with draftHostContext: { ...(overrides.draftHostContext || {}), availableDisplayModes } and spread the remaining overrides, referencing hostContextWithDisplayModes, createMockHostContextStoreState, draftHostContext, and availableDisplayModes to locate the change.
🤖 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/client-config/ClientConfigTab.tsx`:
- Around line 178-187: The per-section "Reset" Button currently only checks
isDirty but still allows clicks during save/sync; update the Reset buttons to
include the full lock condition (disabled={isSaving || syncPending || !isDirty})
so they are disabled while isSaving or syncPending are true (same as the main
Save button), and ensure any per-section reset handlers (the Reset Button
instances in ClientConfigTab.tsx and the similar buttons referenced around the
212-219 and 232 locations) respect that disabled state so they cannot mutate
draft state during remote-echo wait.
In `@mcpjam-inspector/client/src/components/evals/use-eval-trace-tool-context.ts`:
- Around line 80-82: The sync-pending check currently always uses
appState.activeWorkspaceId, which is wrong for callers that pass a different
workspace; update the call to use the effectiveHostedWorkspaceId variable
instead so useWorkspaceClientConfigSyncPending(effectiveHostedWorkspaceId)
checks the correct store entry (locate the call site near
useWorkspaceClientConfigSyncPending and replace appState.activeWorkspaceId with
effectiveHostedWorkspaceId).
In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts`:
- Around line 926-952: The two resolvers resolvePersistedConnectionConfig and
resolvePersistedHostContext currently pick only saved* and default* values;
update them to prefer any workspace-specific pending saves first: check
clientConfigStore.pendingSavedConfig (and
hostContextStore.pendingSavedHostContext) for an entry for the given workspaceId
and return that if present before falling back to savedConfig/defaultConfig or
savedHostContext/defaultHostContext; ensure you reference the workspaceId-scoped
pending slice (not a global pending) so pending in-flight changes are composed
into composeWorkspaceClientConfig / pickWorkspaceHostContext rather than being
overwritten.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts`:
- Around line 866-896: The current logic in use-workspace-state.ts reuses
pendingClientConfigSyncRef for all saves, so clearPendingClientConfigSync() can
cancel a different save; change this to track pending syncs per workspace (e.g.,
replace pendingClientConfigSyncRef with a Map keyed by workspaceId) or serialize
saves per workspace by awaiting an existing pending promise before creating a
new remoteEchoPromise; ensure the new structure stores the same fields
(workspaceId, expectedSerializedConfig, resolve, reject, timeoutId), update
clearPendingClientConfigSync to clear only the entry for the current
workspaceId, and keep convexUpdateClientConfig/remoteEchoPromise flow and
controller.failSave behavior but scoped so one save cannot reject another
workspace's pending save.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/shared/HostContextHeader.tsx`:
- Around line 150-152: Move the per-tab constant computations out of the render
path by hoisting navigator.language and
Intl.DateTimeFormat().resolvedOptions().timeZone to module-level constants
(e.g., replace the in-component fallbackLocale and fallbackTimeZone with
module-scope constants) or memoize them once (e.g., via useMemo called at
component init); update references to fallbackLocale and fallbackTimeZone in
HostContextHeader so the values are computed only once for the lifetime of the
tab.
- Around line 405-407: The tooltip text currently includes backticks literally
in the JSX paragraph inside HostContextHeader (the <p className="text-xs
text-muted-foreground"> element) and should instead render the identifier as
code or plain text; update the string "Edit raw `hostContext` JSON" to either
"Edit raw hostContext JSON" or wrap the identifier in a <code> element (e.g.,
Edit raw <code>hostContext</code> JSON) so users see it styled as code rather
than showing backticks.
In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx`:
- Around line 200-212: Add a regression test exercising the host-context pending
branch by initializing the host store with useHostContextStore.setState({
pendingWorkspaceId: "default", isAwaitingRemoteEcho: true, ...other required
keys }) and then asserting that useWorkspaceClientConfigSyncPending (or the hook
under test) reports the pending state for that workspace; locate references to
useHostContextStore, pendingWorkspaceId, isAwaitingRemoteEcho and
useWorkspaceClientConfigSyncPending in the test file and add a targeted case
mirroring the split-store model to validate the new branch.
In
`@mcpjam-inspector/client/src/hooks/use-workspace-client-config-sync-pending.ts`:
- Around line 4-17: The hook useWorkspaceClientConfigSyncPending now also checks
host-context sync, so rename the function to a clearer name (e.g.
useWorkspaceConfigSyncPending or useWorkspaceSavePending) and update its export
and all imports to match; ensure you update the function identifier in the
declaration and any places that import or call
useWorkspaceClientConfigSyncPending so consumers compile and behavior remains
unchanged (the internal logic using useClientConfigStore and useHostContextStore
stays the same).
In `@mcpjam-inspector/client/src/lib/client-config.ts`:
- Line 102: Add a JSDoc deprecation tag to the transitional alias export so
callers are nudged to the new name; update the export of buildDefaultHostContext
(which re-exports buildDefaultWorkspaceHostContext) by adding a /** `@deprecated`
Use buildDefaultWorkspaceHostContext instead. */ comment immediately above the
export declaration, keeping the alias but marking it deprecated and mentioning
the preferred symbol buildDefaultWorkspaceHostContext.
In `@mcpjam-inspector/client/src/test/mocks/stores.ts`:
- Around line 336-344: The preset function hostContextWithDisplayModes currently
passes draftHostContext: { availableDisplayModes } and then spreads overrides
which can replace draftHostContext entirely; instead merge the passed
availableDisplayModes into any override.draftHostContext so the preset value is
preserved — e.g. call createMockHostContextStoreState with draftHostContext: {
...(overrides.draftHostContext || {}), availableDisplayModes } and spread the
remaining overrides, referencing hostContextWithDisplayModes,
createMockHostContextStoreState, draftHostContext, and availableDisplayModes to
locate the change.
🪄 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: 144111b4-615c-42b0-b3ee-e13c1e4f8a17
📒 Files selected for processing (35)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/ViewsTab.tsxmcpjam-inspector/client/src/components/chat-v2/thread/__tests__/chatgpt-app-renderer.test.tsxmcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.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.tsxmcpjam-inspector/client/src/components/chat-v2/thread/parts/__tests__/display-modes.test.tsxmcpjam-inspector/client/src/components/chat-v2/thread/parts/tool-part.tsxmcpjam-inspector/client/src/components/client-config/ClientConfigTab.tsxmcpjam-inspector/client/src/components/client-config/WorkspaceClientConfigSync.tsxmcpjam-inspector/client/src/components/client-config/__tests__/ClientConfigTab.test.tsxmcpjam-inspector/client/src/components/client-config/__tests__/WorkspaceClientConfigSync.test.tsxmcpjam-inspector/client/src/components/evals/use-eval-trace-tool-context.tsmcpjam-inspector/client/src/components/shared/DisplayContextHeader.tsxmcpjam-inspector/client/src/components/shared/HostContextDialog.tsxmcpjam-inspector/client/src/components/shared/HostContextHeader.tsxmcpjam-inspector/client/src/components/shared/__tests__/HostContextHeader.test.tsxmcpjam-inspector/client/src/components/shared/host-context-constants.tsmcpjam-inspector/client/src/components/shared/host-context-picker-bodies.tsxmcpjam-inspector/client/src/components/ui-playground/AppBuilderTab.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsxmcpjam-inspector/client/src/components/ui-playground/SafeAreaEditor.tsxmcpjam-inspector/client/src/components/ui-playground/__tests__/PlaygroundMain.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-app-state.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsxmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/use-workspace-client-config-sync-pending.tsmcpjam-inspector/client/src/hooks/use-workspace-state.tsmcpjam-inspector/client/src/lib/client-config.tsmcpjam-inspector/client/src/stores/client-config-store.tsmcpjam-inspector/client/src/stores/host-context-store.tsmcpjam-inspector/client/src/test/mocks/stores.ts
💤 Files with no reviewable changes (1)
- mcpjam-inspector/client/src/components/shared/DisplayContextHeader.tsx
| disabled={isSaving || syncPending || !isDirty} | ||
| className="h-7 text-xs" | ||
| > | ||
| Reset | ||
| </Button> | ||
| <Button | ||
| size="sm" | ||
| onClick={handleSave} | ||
| disabled={isSaving || !isDirty} | ||
| disabled={isSaving || syncPending || !isDirty} | ||
| className="h-7 text-xs" |
There was a problem hiding this comment.
Disable section-level Reset while save/sync lock is active
The per-section Reset button still mutates state while the rest of the editor is locked (isSaving || syncPending). That can alter drafts during remote-echo wait and undermine the sync guard.
Suggested patch
<button
type="button"
className="flex items-center gap-1 text-[11px] text-muted-foreground transition-colors hover:text-foreground"
onClick={() => resetSectionToDefault(section.key)}
+ disabled={isSaving || syncPending}
>
<RotateCcw className="h-2.5 w-2.5" />
Reset
</button>Also applies to: 212-219, 232-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/client-config/ClientConfigTab.tsx`
around lines 178 - 187, The per-section "Reset" Button currently only checks
isDirty but still allows clicks during save/sync; update the Reset buttons to
include the full lock condition (disabled={isSaving || syncPending || !isDirty})
so they are disabled while isSaving or syncPending are true (same as the main
Save button), and ensure any per-section reset handlers (the Reset Button
instances in ClientConfigTab.tsx and the similar buttons referenced around the
212-219 and 232 locations) respect that disabled state so they cannot mutate
draft state during remote-echo wait.
| const isClientConfigSyncPending = useWorkspaceClientConfigSyncPending( | ||
| appState.activeWorkspaceId, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites for useEvalTraceToolContext:"
rg -n -C4 '\buseEvalTraceToolContext\s*\(' --type ts --type tsx
echo
echo "Nearby workspaceId/sharedWorkspaceId usage:"
rg -n -C3 '\bworkspaceId\b|\bsharedWorkspaceId\b|\bactiveWorkspaceId\b' --type ts --type tsxRepository: MCPJam/inspector
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Call sites for useEvalTraceToolContext ==="
rg -n -C4 'useEvalTraceToolContext\s*\(' --type ts
echo ""
echo "=== Hook parameter definition ==="
rg -n -B5 -A15 'function useEvalTraceToolContext|const useEvalTraceToolContext' --type ts
echo ""
echo "=== isClientConfigSyncPending usage within context hook ==="
rg -n -B3 -A3 'isClientConfigSyncPending' mcpjam-inspector/client/src/components/evals/
echo ""
echo "=== useWorkspaceClientConfigSyncPending hook implementation ==="
cat mcpjam-inspector/client/src/hooks/use-workspace-client-config-sync-pending.ts 2>/dev/null | head -25Repository: MCPJam/inspector
Length of output: 10079
Pass the workspace parameter to the sync-pending check.
Lines 80-82 always check sync status for appState.activeWorkspaceId, but the hook accepts an optional workspaceId parameter that can differ from the active workspace (as shown in test cases at line 129). When a caller requests context for a non-active workspace, this pending gate will wait on the wrong store entry. Change to:
const isClientConfigSyncPending = useWorkspaceClientConfigSyncPending(
effectiveHostedWorkspaceId,
);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/evals/use-eval-trace-tool-context.ts`
around lines 80 - 82, The sync-pending check currently always uses
appState.activeWorkspaceId, which is wrong for callers that pass a different
workspace; update the call to use the effectiveHostedWorkspaceId variable
instead so useWorkspaceClientConfigSyncPending(effectiveHostedWorkspaceId)
checks the correct store entry (locate the call site near
useWorkspaceClientConfigSyncPending and replace appState.activeWorkspaceId with
effectiveHostedWorkspaceId).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
chelojimenez
temporarily deployed
to
preview-pr-1928
GitHub Actions
Inactive
There was a problem hiding this comment.
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/__tests__/use-server-state.test.tsx (1)
1151-1156:⚠️ Potential issue | 🟡 MinorAdd store resets to beforeEach in the OAuth callback in-flight dispatch suite.
All three sibling suites reset
useClientConfigStoreanduseHostContextStorein theirbeforeEach, but this one omits them. Since Zustand stores are module-level singletons, omitting resets risks state bleed (e.g., straypendingWorkspaceIdorisAwaitingRemoteEcho: truefrom prior tests) causing flakes when run in isolation or as part of the full suite. Mirror the store resets used above:Example reset pattern from sibling suites
useClientConfigStore.setState({ activeWorkspaceId: null, defaultConfig: null, savedConfig: undefined, draftConfig: null, connectionDefaultsText: "{}", isAwaitingRemoteEcho: false, }); useHostContextStore.setState({ activeWorkspaceId: null, defaultHostContext: {}, });🤖 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 1151 - 1156, The OAuth callback in-flight dispatch test suite's beforeEach is missing resets for the Zustand stores, which can cause state bleed; update the beforeEach inside the "useServerState OAuth callback in-flight dispatch" describe to call useClientConfigStore.setState(...) to reset activeWorkspaceId, defaultConfig, savedConfig, draftConfig, connectionDefaultsText to "{}" and isAwaitingRemoteEcho to false, and call useHostContextStore.setState(...) to reset activeWorkspaceId to null and defaultHostContext to {} so the store state matches the sibling suites.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx (1)
162-171: Silent no-op callbacks risk misleading upstream callers.
onDeviceTypeChange,onLocaleChange, andonTimeZoneChange(and their corresponding value props) remain inPlaygroundMainPropsbut are now destructured into_-prefixed bindings and never invoked —HostContextHeadermutates the store directly, so any parent still subscribing to these callbacks will silently stop receiving notifications. The "kept for backward compatibility" comment is accurate about the values, but the callbacks are a behavior change disguised as a no-op.Two reasonable paths:
- Remove the now-defunct fields from the public interface so callers get a TypeScript error and can migrate to the host-context store, or
- Subscribe to the host-context store inside
PlaygroundMainand bridge changes back through these callbacks until the next breaking-change window.Worth at least a brief deprecation note in the prop docs if neither is in scope for this PR.
Also applies to: 230-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx` around lines 162 - 171, PlaygroundMainProps currently keeps onDeviceTypeChange, onLocaleChange, and onTimeZoneChange but PlaygroundMain destructures them into _-prefixed variables and never calls them (HostContextHeader mutates the host store), causing silent no-op behavior; either remove these callback props from the public PlaygroundMainProps interface (and add a deprecation note in the prop docs) so callers get a TypeScript error to migrate to the host-context store, or implement a bridging subscription inside PlaygroundMain (e.g., in the PlaygroundMain component) that listens to the HostContextHeader/host-context store and invokes onDeviceTypeChange, onLocaleChange, and onTimeZoneChange when the corresponding values change; update references to PlaygroundMainProps, PlaygroundMain, and HostContextHeader accordingly and apply the same change for the other occurrence noted (around the second block at lines 230-240).mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx (1)
185-212: A trio of identical seedings begs for a helper.The three
beforeEachblocks (lines 185–212, 876–903, 1028–1055) resetuseClientConfigStoreanduseHostContextStoreto byte-for-byte identical baselines. Hoisting these into a smallresetConfigStores()helper would shrink the file, eliminate three places to update when either store gains a field, and make the intent of each suite easier to read.♻️ Sketch of the extraction
+function resetConfigStores() { + useClientConfigStore.setState({ + activeWorkspaceId: null, + defaultConfig: null, + savedConfig: undefined, + draftConfig: null, + connectionDefaultsText: "{}", + clientCapabilitiesText: "{}", + clientCapabilitiesError: null, + connectionDefaultsError: null, + isSaving: false, + isDirty: false, + pendingWorkspaceId: null, + pendingSavedConfig: undefined, + isAwaitingRemoteEcho: false, + }); + useHostContextStore.setState({ + activeWorkspaceId: null, + defaultHostContext: {}, + savedHostContext: undefined, + draftHostContext: {}, + hostContextText: "{}", + hostContextError: null, + isSaving: false, + isDirty: false, + pendingWorkspaceId: null, + pendingSavedHostContext: undefined, + isAwaitingRemoteEcho: false, + }); +}Then each
beforeEachsimplifies to a singleresetConfigStores();call.🤖 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 185 - 212, The three identical beforeEach seedings repeatedly call useClientConfigStore.setState(...) and useHostContextStore.setState(...) with the same baseline; extract this into a single helper function (e.g., resetConfigStores) that encapsulates those two setState calls and any shared default values, then replace each beforeEach block with a call to resetConfigStores(); ensure the helper initializes the same fields shown (activeWorkspaceId, defaultConfig, savedConfig, draftConfig, connectionDefaultsText, clientCapabilitiesText, clientCapabilitiesError, connectionDefaultsError, isSaving, isDirty, pendingWorkspaceId, pendingSavedConfig, isAwaitingRemoteEcho and the host equivalents) so tests remain behaviorally identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx`:
- Around line 1151-1156: The OAuth callback in-flight dispatch test suite's
beforeEach is missing resets for the Zustand stores, which can cause state
bleed; update the beforeEach inside the "useServerState OAuth callback in-flight
dispatch" describe to call useClientConfigStore.setState(...) to reset
activeWorkspaceId, defaultConfig, savedConfig, draftConfig,
connectionDefaultsText to "{}" and isAwaitingRemoteEcho to false, and call
useHostContextStore.setState(...) to reset activeWorkspaceId to null and
defaultHostContext to {} so the store state matches the sibling suites.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx`:
- Around line 162-171: PlaygroundMainProps currently keeps onDeviceTypeChange,
onLocaleChange, and onTimeZoneChange but PlaygroundMain destructures them into
_-prefixed variables and never calls them (HostContextHeader mutates the host
store), causing silent no-op behavior; either remove these callback props from
the public PlaygroundMainProps interface (and add a deprecation note in the prop
docs) so callers get a TypeScript error to migrate to the host-context store, or
implement a bridging subscription inside PlaygroundMain (e.g., in the
PlaygroundMain component) that listens to the HostContextHeader/host-context
store and invokes onDeviceTypeChange, onLocaleChange, and onTimeZoneChange when
the corresponding values change; update references to PlaygroundMainProps,
PlaygroundMain, and HostContextHeader accordingly and apply the same change for
the other occurrence noted (around the second block at lines 230-240).
In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx`:
- Around line 185-212: The three identical beforeEach seedings repeatedly call
useClientConfigStore.setState(...) and useHostContextStore.setState(...) with
the same baseline; extract this into a single helper function (e.g.,
resetConfigStores) that encapsulates those two setState calls and any shared
default values, then replace each beforeEach block with a call to
resetConfigStores(); ensure the helper initializes the same fields shown
(activeWorkspaceId, defaultConfig, savedConfig, draftConfig,
connectionDefaultsText, clientCapabilitiesText, clientCapabilitiesError,
connectionDefaultsError, isSaving, isDirty, pendingWorkspaceId,
pendingSavedConfig, isAwaitingRemoteEcho and the host equivalents) so tests
remain behaviorally identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07dd9d98-889e-4162-8974-35519fd99002
📒 Files selected for processing (6)
mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsxmcpjam-inspector/client/src/hooks/__tests__/use-app-state.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-server-state.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/client/src/hooks/tests/use-app-state.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/client/src/hooks/use-app-state.ts
- mcpjam-inspector/client/src/hooks/use-server-state.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e04e5af96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clientConfigStore.savedConfig ?? | ||
| clientConfigStore.defaultConfig ?? | ||
| pickWorkspaceConnectionConfig(workspaceClientConfig) |
There was a problem hiding this comment.
resolvePersistedConnectionConfig always prefers clientConfigStore.savedConfig/defaultConfig without checking which workspace those values belong to. During a workspace switch (or any call where workspaceId differs from the store’s activeWorkspaceId), handleUpdateHostContext can compose and save the new workspace’s clientConfig with the previous workspace’s connection defaults/capabilities, overwriting the target workspace with stale data. This helper should only reuse store values when activeWorkspaceId === workspaceId, otherwise fall back to the target workspace’s persisted config.
Useful? React with 👍 / 👎.
…t-out-of-connection # Conflicts: # mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mcpjam-inspector/client/src/hooks/use-workspace-state.ts (1)
1016-1046:⚠️ Potential issue | 🔴 CriticalCross-slice resolvers can still re-emit a stale sibling during an in-flight save.
The supersession added in
persistWorkspaceClientConfigonly short-circuits the local sync tracker; it does not prevent a sibling save from composing a stale slice into the actual backend write. Consider this sequence on a single workspace:
handleUpdateClientConfigbegins → store setspendingSavedConfig = newConn(andsavedConfigis still old until echo).- Before the echo arrives,
handleUpdateHostContextfires.- The new save calls
resolvePersistedConnectionConfig(ws)→ returnsclientConfigStore.savedConfig ?? defaultConfig(old) sincependingSavedConfigis ignored.composeWorkspaceClientConfig({ connectionConfig: oldConn, hostContext: newHC, fallback: workspaceClientConfig })— fallback is also old (echo hadn't arrived) and is preempted by the explicitconnectionConfig.convexUpdateClientConfigwrites{ oldConn, newHC }, clobberingnewConnserver-side. The reverse path (host-context in-flight, connection save begins) has the same shape.Prefer the workspace-scoped pending slice before falling back to
saved*/default*so the in-flight sibling is preserved through the compose.🛡️ Suggested direction
const resolvePersistedConnectionConfig = useCallback( (workspaceId: string): WorkspaceConnectionConfigDraft => { const clientConfigStore = useClientConfigStore.getState(); const workspaceClientConfig = effectiveWorkspaces[workspaceId]?.clientConfig; const scopedStoreConfig = doesStoreSliceBelongToWorkspace( clientConfigStore.activeWorkspaceId, workspaceId, ) - ? clientConfigStore.savedConfig ?? clientConfigStore.defaultConfig + ? (clientConfigStore.isAwaitingRemoteEcho && + clientConfigStore.pendingWorkspaceId === workspaceId + ? clientConfigStore.pendingSavedConfig + : undefined) ?? + clientConfigStore.savedConfig ?? + clientConfigStore.defaultConfig : undefined; return scopedStoreConfig ?? pickWorkspaceConnectionConfig(workspaceClientConfig); }, [effectiveWorkspaces], ); const resolvePersistedHostContext = useCallback( (workspaceId: string): WorkspaceHostContextDraft => { const hostContextStore = useHostContextStore.getState(); const workspaceClientConfig = effectiveWorkspaces[workspaceId]?.clientConfig; const scopedStoreHostContext = doesStoreSliceBelongToWorkspace( hostContextStore.activeWorkspaceId, workspaceId, ) - ? hostContextStore.savedHostContext ?? hostContextStore.defaultHostContext + ? (hostContextStore.isAwaitingRemoteEcho && + hostContextStore.pendingWorkspaceId === workspaceId + ? hostContextStore.pendingSavedHostContext + : undefined) ?? + hostContextStore.savedHostContext ?? + hostContextStore.defaultHostContext : undefined; return scopedStoreHostContext ?? pickWorkspaceHostContext(workspaceClientConfig); }, [effectiveWorkspaces], );A regression test where two saves on the same workspace overlap (one connection, one host-context, with the second beginning before the first’s echo) would catch this — the existing two-save coverage uses different
workspaceIds, which avoids the cross-slice path entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts` around lines 1016 - 1046, The resolvers resolvePersistedConnectionConfig and resolvePersistedHostContext currently ignore in-flight pending slices (e.g., pendingSavedConfig/pendingSavedHostContext) and fall back to saved*/default* which lets a concurrent sibling save recompose stale data; update both resolvers to first check the workspace-scoped pending slice from the respective stores (clientConfigStore.pendingSavedConfig or hostContextStore.pendingSavedHostContext) and return that when it belongs to the workspace, before falling back to savedConfig/savedHostContext or defaultConfig/defaultHostContext so in-flight updates from handleUpdateClientConfig/handleUpdateHostContext are preserved during composeWorkspaceClientConfig/convexUpdateClientConfig.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/hooks/use-workspace-state.ts (1)
942-965: Inline timeout teardown duplicatesclearPendingClientConfigSync.The setTimeout body re-implements the same map cleanup that
clearPendingClientConfigSyncalready encapsulates, drifting from the single source of truth for pending teardown. Delegating keeps the two cleanup paths aligned (theclearTimeouton a fired timer is a harmless no-op).♻️ Proposed consolidation
- const timeoutId = setTimeout(() => { - pendingClientConfigSyncRef.current.delete(pendingSyncId); - if ( - pendingClientConfigSyncByWorkspaceRef.current.get(workspaceId) === - pendingSyncId - ) { - pendingClientConfigSyncByWorkspaceRef.current.delete(workspaceId); - } - reject(new Error(WORKSPACE_CLIENT_CONFIG_SYNC_TIMEOUT_ERROR_MESSAGE)); - }, CLIENT_CONFIG_SYNC_ECHO_TIMEOUT_MS); + const timeoutId = setTimeout(() => { + clearPendingClientConfigSync( + pendingSyncId, + new Error(WORKSPACE_CLIENT_CONFIG_SYNC_TIMEOUT_ERROR_MESSAGE), + ); + }, CLIENT_CONFIG_SYNC_ECHO_TIMEOUT_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts` around lines 942 - 965, The timeout callback duplicates map cleanup logic already in clearPendingClientConfigSync; replace the manual deletion inside the setTimeout handler with a single call to clearPendingClientConfigSync(pendingSyncId) and then call reject(new Error(WORKSPACE_CLIENT_CONFIG_SYNC_TIMEOUT_ERROR_MESSAGE)); ensure pendingClientConfigSyncRef and pendingClientConfigSyncByWorkspaceRef are still populated as before and that you pass the same pendingSyncId to clearPendingClientConfigSync so the existing teardown (including clearTimeout) remains the single source of truth.mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx (1)
680-1022: Tests cover the cross-workspace path but leave the same-workspace sibling-save case untested.The new compose tests (
"composes host-context saves...","composes connection-config saves...") deliberately setuseClientConfigStore/useHostContextStoreactiveWorkspaceIdto a different workspace than the save target, sodoesStoreSliceBelongToWorkspacereturnsfalseand the resolvers fall through topickWorkspace*from the workspace clientConfig. Likewise,"keeps a newer workspace save pending when an older save times out"uses two distinct workspace ids. None of these exercise the path where a connection save and a host-context save overlap on the same workspace — the scenario flagged on the hook’s resolvers. Worth adding a regression test there once that path is settled.🤖 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-workspace-state.test.tsx` around lines 680 - 1022, Add a new test that exercises the same-workspace overlap path: set useClientConfigStore and useHostContextStore activeWorkspaceId to the same workspace id as the save target, then call result.current.handleUpdateClientConfig(...) and result.current.handleUpdateHostContext(...) for that same workspace to simulate overlapping saves; verify the resolvers take the "doesStoreSliceBelongToWorkspace" true branch by asserting updateClientConfigMock was called with composed clientConfig (or that pendingWorkspaceId/isAwaitingRemoteEcho behave as expected) and that the promises resolve/reject according to the timeout behavior (use vi.useFakeTimers and vi.advanceTimersByTimeAsync as in existing tests). Ensure you reference the existing helper functions createAppState, renderUseWorkspaceState and the methods handleUpdateClientConfig and handleUpdateHostContext when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts`:
- Around line 1016-1046: The resolvers resolvePersistedConnectionConfig and
resolvePersistedHostContext currently ignore in-flight pending slices (e.g.,
pendingSavedConfig/pendingSavedHostContext) and fall back to saved*/default*
which lets a concurrent sibling save recompose stale data; update both resolvers
to first check the workspace-scoped pending slice from the respective stores
(clientConfigStore.pendingSavedConfig or
hostContextStore.pendingSavedHostContext) and return that when it belongs to the
workspace, before falling back to savedConfig/savedHostContext or
defaultConfig/defaultHostContext so in-flight updates from
handleUpdateClientConfig/handleUpdateHostContext are preserved during
composeWorkspaceClientConfig/convexUpdateClientConfig.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx`:
- Around line 680-1022: Add a new test that exercises the same-workspace overlap
path: set useClientConfigStore and useHostContextStore activeWorkspaceId to the
same workspace id as the save target, then call
result.current.handleUpdateClientConfig(...) and
result.current.handleUpdateHostContext(...) for that same workspace to simulate
overlapping saves; verify the resolvers take the
"doesStoreSliceBelongToWorkspace" true branch by asserting
updateClientConfigMock was called with composed clientConfig (or that
pendingWorkspaceId/isAwaitingRemoteEcho behave as expected) and that the
promises resolve/reject according to the timeout behavior (use vi.useFakeTimers
and vi.advanceTimersByTimeAsync as in existing tests). Ensure you reference the
existing helper functions createAppState, renderUseWorkspaceState and the
methods handleUpdateClientConfig and handleUpdateHostContext when adding the
test.
In `@mcpjam-inspector/client/src/hooks/use-workspace-state.ts`:
- Around line 942-965: The timeout callback duplicates map cleanup logic already
in clearPendingClientConfigSync; replace the manual deletion inside the
setTimeout handler with a single call to
clearPendingClientConfigSync(pendingSyncId) and then call reject(new
Error(WORKSPACE_CLIENT_CONFIG_SYNC_TIMEOUT_ERROR_MESSAGE)); ensure
pendingClientConfigSyncRef and pendingClientConfigSyncByWorkspaceRef are still
populated as before and that you pass the same pendingSyncId to
clearPendingClientConfigSync so the existing teardown (including clearTimeout)
remains the single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f36d261-a4ee-461f-8b17-131266519f39
📒 Files selected for processing (4)
mcpjam-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.tsxmcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsxmcpjam-inspector/client/src/hooks/use-workspace-state.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsx
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 97a92ef. Configure here.
| useLocalFallback, | ||
| convexUpdateClientConfig, | ||
| clearPendingClientConfigSync, | ||
| convexUpdateClientConfig, |
There was a problem hiding this comment.
Concurrent saves can lose data via shared pending tracker
Medium Severity
Both handleUpdateClientConfig and handleUpdateHostContext call persistWorkspaceClientConfig, which shares pendingClientConfigSyncByWorkspaceRef keyed by workspace ID. If both are called for the same workspace before the first echo returns, the second call supersedes the first via clearPendingClientConfigSync. Each call composes the full WorkspaceClientConfig by reading the saved (not in-flight) slice from the other store via resolvePersistedConnectionConfig/resolvePersistedHostContext, so the second mutation overwrites the first's changes at the Convex layer. The UI syncPending guards mitigate this today, but a future programmatic caller could trigger data loss.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 97a92ef. Configure here.
1 participant
Summary
hostContextis now edited from the preview/runtime surface instead of Connection Settings.HostContext*.WorkspaceClientConfig.Testing
Note
Medium Risk
Moderate risk because it refactors workspace client-config persistence into two independently saved slices (connection config vs
hostContext) with new pending-sync coordination, which could affect save/echo timing or overwrite behavior across workspaces.Overview
Moves
hostContextediting to the preview surface. Connection Settings (ClientConfigTab) now only edits connection defaults + client capabilities, while a newHostContextHeader+HostContextDialogprovide live draft edits and persistence of rawhostContextJSON from the playground/views surfaces.Splits state and persistence paths. Introduces
useHostContextStoreanduseWorkspaceClientConfigSyncPendingto coordinate pending remote-echo saves across both slices, and updatesuse-workspace-stateto persist connection-config and host-context changes independently while still composing a fullWorkspaceClientConfigpayload for Convex.Updates preview/widget consumers (
PlaygroundMain, renderers, tool parts, safe-area editor, views/app-builder tabs) and refreshes unit tests to read from the new host-context store and cover the new save/sync behavior.Reviewed by Cursor Bugbot for commit 97a92ef. Bugbot is set up for automated code reviews on this repo. Configure here.