Open
Conversation
dosubot
bot
added
the
size:XXL
|
🚅 Environment inspector-pr-1657 in triumphant-alignment has no services deployed. 1 service not affected by this PR
|
Contributor
Author
✅ 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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds workspace-scoped client configuration end-to-end: new ClientConfigTab UI, a Zustand store (useClientConfigStore), client-config data model/utilities, and workspace.clientConfig plumbing. Threads workspace clientCapabilities into hosted API context and hosted request payloads, includes clientConfigSyncPending guards that block connect/reconnect actions until remote echo completes, and surfaces reconnect warnings in server UI (cards, modals, detail). Moves UI globals (theme, displayMode, locale, timeZone, device capabilities, safe-area insets) to host-context-backed client config. Adds browser-safe SDK entry ( 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 |
… into add-client-config
![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: 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/components/shared/DisplayContextHeader.tsx (1)
1-8:⚠️ Potential issue | 🟡 MinorStale docblock reference.
The header comment states the component "Reads/writes to useUIPlaygroundStore for state management," yet it now also derives theme, locale, timezone, display modes, and capabilities from
useClientConfigStore. A brief update would align documentation with reality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/shared/DisplayContextHeader.tsx` around lines 1 - 8, Update the stale docblock on DisplayContextHeader to reflect current state sources: mention that it reads/writes UI state via useUIPlaygroundStore and also derives theme, locale, timezone, display modes, and capabilities from useClientConfigStore; keep the brief description of responsibilities (device, locale, timezone, CSP, capabilities, safe area) and note both stores by name (useUIPlaygroundStore and useClientConfigStore) so the comment accurately documents where state comes from.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/state/mcp-api.ts (1)
55-56: Superfluous nullish coalescing.The
?? undefinedis redundant—when the cast yieldsundefined, the fallback merely returnsundefinedagain.✨ Simplified expression
- (serverConfig.capabilities as Record<string, unknown> | undefined) ?? - undefined, + serverConfig.capabilities as Record<string, unknown> | undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/state/mcp-api.ts` around lines 55 - 56, The expression "(serverConfig.capabilities as Record<string, unknown> | undefined) ?? undefined" is redundant; remove the "?? undefined" fallback so the value is simply "(serverConfig.capabilities as Record<string, unknown> | undefined)". Update the code where the cast of serverConfig.capabilities occurs (the cast expression using Record<string, unknown> | undefined) to return the cast value directly, eliminating the unnecessary nullish coalescing.mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx (1)
71-79: Use the shared client test presets for this new store mock.This hand-rolled
useClientConfigStorestub will drift as the store evolves. Please hang it off the shared presets inclient/src/test/mocks/instead of maintaining another bespoke state shape here. As per coding guidelines,mcpjam-inspector/client/**/__tests__/*.test.{ts,tsx}: Use mock presets fromclient/src/test/mocks/includingmcpApiPresetsandstorePresetsin client tests.Also applies to: 138-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx` around lines 71 - 79, Replace the hand-rolled mockClientConfigStoreState and any local useClientConfigStore stub with the shared test presets: import and use storePresets (and mcpApiPresets where relevant) instead of constructing the draftConfig shape locally; update references in the test to pull the client config from storePresets (e.g., use storePresets.clientConfig or the equivalent preset) so the test uses the shared mocks and stays in sync with store evolution.
🤖 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/chat-v2/thread/chatgpt-app-renderer.tsx`:
- Around line 628-638: The widget globals are missing the host timeZone
extracted from draftHostContext, so update the places that compute locale/theme
(e.g., where draftHostContext is read via useClientConfigStore and locale is
computed with extractHostLocale and theme with extractHostTheme) to also extract
a timeZone (e.g., const hostTimeZone = draftHostContext?.timeZone ??
Intl.DateTimeFormat().resolvedOptions().timeZone) and include that hostTimeZone
in the payload sent to the ChatGPT widget (the openai:set_globals/global object
built near resolvedTheme); apply the same change in the other occurrences you
noted (around the blocks at 897-915, 1395-1424, 1452-1486) so the widget
receives timeZone alongside locale/theme/capabilities/safe-area.
In
`@mcpjam-inspector/client/src/components/chat-v2/thread/parts/__tests__/display-modes.test.tsx`:
- Around line 47-59: Replace the bespoke mutable store mock
(mockClientConfigStoreState) and the vi.mock of useClientConfigStore with the
shared test presets: import and use storePresets (and mcpApiPresets if needed)
from client/src/test/mocks, then apply the appropriate preset that configures
draftConfig.availableDisplayModes instead of defining
mockClientConfigStoreState; update references to useClientConfigStore in this
test to rely on the preset so the test uses the centralized storePresets pattern
rather than custom wiring.
In `@mcpjam-inspector/client/src/components/ServersTab.tsx`:
- Around line 31-32: The reconnect warning is comparing only workspace-level
capabilities (via workspaceClientCapabilitiesNeedReconnect /
getDefaultClientCapabilities) but use-server-state connects using the merged
capabilities of server.config.capabilities + workspace profile, so servers with
per-server overrides can falsely show or hide the reconnect banner; update the
check in ServersTab (and the similar logic at lines ~240-259) to compute the
effective client capabilities by merging getDefaultClientCapabilities() (or
workspace profile) with each server's server.config.capabilities (the same merge
logic used in use-server-state) and pass that merged object into
workspaceClientCapabilitiesNeedReconnect (or use the same predicate) so the
reconnect banner reflects per-server effective capabilities rather than
workspace-only capabilities.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/components/shared/DisplayContextHeader.tsx`:
- Around line 1-8: Update the stale docblock on DisplayContextHeader to reflect
current state sources: mention that it reads/writes UI state via
useUIPlaygroundStore and also derives theme, locale, timezone, display modes,
and capabilities from useClientConfigStore; keep the brief description of
responsibilities (device, locale, timezone, CSP, capabilities, safe area) and
note both stores by name (useUIPlaygroundStore and useClientConfigStore) so the
comment accurately documents where state comes from.
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/__tests__/mcp-apps-renderer.test.tsx`:
- Around line 71-79: Replace the hand-rolled mockClientConfigStoreState and any
local useClientConfigStore stub with the shared test presets: import and use
storePresets (and mcpApiPresets where relevant) instead of constructing the
draftConfig shape locally; update references in the test to pull the client
config from storePresets (e.g., use storePresets.clientConfig or the equivalent
preset) so the test uses the shared mocks and stays in sync with store
evolution.
In `@mcpjam-inspector/client/src/state/mcp-api.ts`:
- Around line 55-56: The expression "(serverConfig.capabilities as
Record<string, unknown> | undefined) ?? undefined" is redundant; remove the "??
undefined" fallback so the value is simply "(serverConfig.capabilities as
Record<string, unknown> | undefined)". Update the code where the cast of
serverConfig.capabilities occurs (the cast expression using Record<string,
unknown> | undefined) to return the cast value directly, eliminating the
unnecessary nullish coalescing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bbae418-dbb9-4fca-8936-2cf935ee1a57
📒 Files selected for processing (72)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/AuthTab.tsxmcpjam-inspector/client/src/components/PromptsTab.tsxmcpjam-inspector/client/src/components/ResourceTemplatesTab.tsxmcpjam-inspector/client/src/components/ResourcesTab.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/SettingsTab.tsxmcpjam-inspector/client/src/components/TasksTab.tsxmcpjam-inspector/client/src/components/ToolsTab.tsxmcpjam-inspector/client/src/components/__tests__/PromptsTab.test.tsxmcpjam-inspector/client/src/components/__tests__/ResourceTemplatesTab.test.tsxmcpjam-inspector/client/src/components/__tests__/ResourcesTab.test.tsxmcpjam-inspector/client/src/components/__tests__/ServersTab.test.tsxmcpjam-inspector/client/src/components/__tests__/TasksTab.test.tsxmcpjam-inspector/client/src/components/__tests__/ToolsTab.test.tsxmcpjam-inspector/client/src/components/chat-v2/shared/model-helpers.tsmcpjam-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/connection/ServerConnectionCard.tsxmcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/connection/ServerInfoContent.tsxmcpjam-inspector/client/src/components/connection/__tests__/server-card-utils.test.tsmcpjam-inspector/client/src/components/connection/server-card-utils.tsmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/components/oauth/utils.tsmcpjam-inspector/client/src/components/setting/CustomProviderConfigDialog.tsxmcpjam-inspector/client/src/components/shared/DisplayContextHeader.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__/AppBuilderTab.test.tsxmcpjam-inspector/client/src/components/ui-playground/hooks/useServerKey.tsmcpjam-inspector/client/src/hooks/hosted/use-hosted-api-context.tsmcpjam-inspector/client/src/hooks/use-app-state.tsmcpjam-inspector/client/src/hooks/use-custom-providers.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/use-workspace-state.tsmcpjam-inspector/client/src/hooks/useWorkspaces.tsmcpjam-inspector/client/src/lib/__tests__/hosted-web-context.test.tsmcpjam-inspector/client/src/lib/apis/mcp-prompts-api.tsmcpjam-inspector/client/src/lib/apis/mcp-resource-templates-api.tsmcpjam-inspector/client/src/lib/apis/mcp-tasks-api.tsmcpjam-inspector/client/src/lib/apis/mcp-tools-api.tsmcpjam-inspector/client/src/lib/apis/web/__tests__/context.guest-fallback.test.tsmcpjam-inspector/client/src/lib/apis/web/context.tsmcpjam-inspector/client/src/lib/apis/web/servers-api.tsmcpjam-inspector/client/src/lib/client-config.tsmcpjam-inspector/client/src/lib/hosted-tab-policy.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/state/__tests__/mcp-api.hosted.test.tsmcpjam-inspector/client/src/state/app-types.tsmcpjam-inspector/client/src/state/mcp-api.tsmcpjam-inspector/client/src/state/server-helpers.tsmcpjam-inspector/client/src/state/storage.tsmcpjam-inspector/client/src/stores/client-config-store.tsmcpjam-inspector/client/src/test/factories.tsmcpjam-inspector/server/routes/web/auth.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/routes/web/xray-payload.tssdk/package.jsonsdk/src/browser.tssdk/src/index.tssdk/src/mcp-client-manager/MCPClientManager.tssdk/src/mcp-client-manager/capabilities.tssdk/src/mcp-client-manager/index.tssdk/tests/MCPClientManager.test.tssdk/tests/browser-entry.test.tssdk/tsup.config.ts
mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx
Show resolved
Hide resolved
mcpjam-inspector/client/src/components/chat-v2/thread/parts/__tests__/display-modes.test.tsx
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@coderabbitai review pls |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/components/ServersTab.tsx (1)
243-267: MemoizeworkspaceDesiredCapabilitiesto prevent unnecessary recalculations.
getDefaultClientCapabilities()returns a fresh object on each invocation. When the workspace lacks configured capabilities, the fallback triggers a new object reference every render, causingreconnectWarningByServerNameto recompute regardless of actual changes.♻️ Proposed fix
+ const workspaceDesiredCapabilities = useMemo( + () => + (workspaces[activeWorkspaceId]?.clientConfig?.clientCapabilities as + | Record<string, unknown> + | undefined) ?? + (getDefaultClientCapabilities() as Record<string, unknown>), + [workspaces, activeWorkspaceId], + ); - const workspaceDesiredCapabilities = - (workspaces[activeWorkspaceId]?.clientConfig?.clientCapabilities as - | Record<string, unknown> - | undefined) ?? - (getDefaultClientCapabilities() as Record<string, unknown>); const reconnectWarningByServerName = useMemo( () => Object.fromEntries(🤖 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 243 - 267, workspaceDesiredCapabilities is computed inline and falls back to getDefaultClientCapabilities() which returns a new object each render, forcing reconnectWarningByServerName to recompute; wrap the workspaceDesiredCapabilities computation in a useMemo (e.g., memoize the result of either workspaces[activeWorkspaceId]?.clientConfig?.clientCapabilities or getDefaultClientCapabilities()) and use the minimal dependencies (such as activeWorkspaceId and the workspace's clientConfig.clientCapabilities) so the fallback default object isn't recreated every render and reconnectWarningByServerName (the useMemo that builds reconnectWarningByServerName) only recalculates when actual capability inputs change.mcpjam-inspector/client/src/test/mocks/stores.ts (1)
159-206: Keep this mock aligned with the real client-config store surface.
MockClientConfigStoreStatestops at the data fields, butmcpjam-inspector/client/src/stores/client-config-store.ts:5-27also exposes mutators likesetSectionText,patchHostContext,resetToBaseline, andmarkSaved. Any test that reusesstorePresets.clientConfig()against a component selecting one of those actions will getundefinedat call time. Addingvi.fn()stubs here keeps the preset drop-in compatible.🧪 Suggested shape update
export type MockClientConfigStoreState = { activeWorkspaceId: string | null; defaultConfig: WorkspaceClientConfig | null; savedConfig: WorkspaceClientConfig | undefined; draftConfig: WorkspaceClientConfig | null; clientCapabilitiesText: string; hostContextText: string; clientCapabilitiesError: string | null; hostContextError: string | null; isSaving: boolean; isDirty: boolean; + loadWorkspaceConfig: ReturnType<typeof vi.fn>; + setSectionText: ReturnType<typeof vi.fn>; + patchHostContext: ReturnType<typeof vi.fn>; + resetSectionToDefault: ReturnType<typeof vi.fn>; + resetToBaseline: ReturnType<typeof vi.fn>; + markSaving: ReturnType<typeof vi.fn>; + markSaved: ReturnType<typeof vi.fn>; }; @@ return { activeWorkspaceId: null, defaultConfig: null, savedConfig: undefined, draftConfig, clientCapabilitiesText: stringifyJson( draftConfig?.clientCapabilities ?? {}, ), hostContextText: stringifyJson(draftConfig?.hostContext ?? {}), clientCapabilitiesError: null, hostContextError: null, isSaving: false, isDirty: false, + loadWorkspaceConfig: vi.fn(), + setSectionText: vi.fn(), + patchHostContext: vi.fn(), + resetSectionToDefault: vi.fn(), + resetToBaseline: vi.fn(), + markSaving: vi.fn(), + markSaved: vi.fn(), ...overrides, }; }🤖 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 159 - 206, The mock store state lacks the same action/mutator fns as the real client-config store, so tests using storePresets.clientConfig() that call methods like setSectionText, patchHostContext, resetToBaseline, or markSaved will see undefined; update createMockClientConfigStoreState / MockClientConfigStoreState to include those mutator properties as vi.fn() stubs (e.g., add setSectionText, patchHostContext, resetToBaseline, markSaved) and merge overrides so the preset remains drop-in compatible with the real store API.
🤖 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/chat-v2/thread/chatgpt-app-renderer.tsx`:
- Around line 759-766: The extracted capability/inset objects created by
extractHostDeviceCapabilities and extractHostSafeAreaInsets are unstable each
render causing downstream effects to restart; wrap those calls in useMemo
(memoize capabilities and safeAreaInsets based on draftHostContext and
isPlaygroundActive/playground* or DEFAULT_* inputs) so their references only
change when inputs change, and in the hosted-widget bootstrap/fetch effect add
the same cancellation guard used in the local path (check a mounted/cancelled
flag before mutating state or performing duplicate bootstrap work) to prevent
state races and duplicated bootstraps when that effect is restarted mid-flight.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/ServersTab.tsx`:
- Around line 243-267: workspaceDesiredCapabilities is computed inline and falls
back to getDefaultClientCapabilities() which returns a new object each render,
forcing reconnectWarningByServerName to recompute; wrap the
workspaceDesiredCapabilities computation in a useMemo (e.g., memoize the result
of either workspaces[activeWorkspaceId]?.clientConfig?.clientCapabilities or
getDefaultClientCapabilities()) and use the minimal dependencies (such as
activeWorkspaceId and the workspace's clientConfig.clientCapabilities) so the
fallback default object isn't recreated every render and
reconnectWarningByServerName (the useMemo that builds
reconnectWarningByServerName) only recalculates when actual capability inputs
change.
In `@mcpjam-inspector/client/src/test/mocks/stores.ts`:
- Around line 159-206: The mock store state lacks the same action/mutator fns as
the real client-config store, so tests using storePresets.clientConfig() that
call methods like setSectionText, patchHostContext, resetToBaseline, or
markSaved will see undefined; update createMockClientConfigStoreState /
MockClientConfigStoreState to include those mutator properties as vi.fn() stubs
(e.g., add setSectionText, patchHostContext, resetToBaseline, markSaved) and
merge overrides so the preset remains drop-in compatible with the real store
API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0818bba6-608b-45f2-8fad-2b67a681dec5
📒 Files selected for processing (7)
mcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/__tests__/ServersTab.test.tsxmcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxmcpjam-inspector/client/src/components/chat-v2/thread/parts/__tests__/display-modes.test.tsxmcpjam-inspector/client/src/state/__tests__/mcp-api.hosted.test.tsmcpjam-inspector/client/src/test/mocks/index.tsmcpjam-inspector/client/src/test/mocks/stores.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- mcpjam-inspector/client/src/state/tests/mcp-api.hosted.test.ts
- mcpjam-inspector/client/src/components/chat-v2/thread/parts/tests/display-modes.test.tsx
- mcpjam-inspector/client/src/components/tests/ServersTab.test.tsx
Comment on lines
+759
to
+766
| const capabilities = extractHostDeviceCapabilities( | ||
| draftHostContext, | ||
| isPlaygroundActive ? playgroundCapabilities : DEFAULT_CAPABILITIES, | ||
| ); | ||
| const safeAreaInsets = extractHostSafeAreaInsets( | ||
| draftHostContext, | ||
| isPlaygroundActive ? playgroundSafeAreaInsets : DEFAULT_SAFE_AREA_INSETS, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the target file
find . -name "chatgpt-app-renderer.tsx" -type fRepository: MCPJam/inspector
Length of output: 140
🏁 Script executed:
# Check the extractor implementation details
find . -name "client-config.ts" -type fRepository: MCPJam/inspector
Length of output: 110
🏁 Script executed:
# Check lines 759-766 in the target file
sed -n '755,770p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 754
🏁 Script executed:
# Check the extractor implementations
sed -n '190,235p' ./mcpjam-inspector/client/src/lib/client-config.tsRepository: MCPJam/inspector
Length of output: 1292
🏁 Script executed:
# Check file size to determine how to read it
wc -l ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 145
🏁 Script executed:
# Check the effect dependencies around lines 905-939
sed -n '900,945p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 1341
🏁 Script executed:
# Check the effect dependencies around lines 1462-1500
sed -n '1455,1505p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 1459
🏁 Script executed:
# Check where useWidgetFetch is called
grep -n "useWidgetFetch" ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 197
🏁 Script executed:
# Check the useWidgetFetch hook implementation starting at line 240
sed -n '240,350p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 4491
🏁 Script executed:
# Check how useWidgetFetch is called (around line 821)
sed -n '815,850p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 1000
🏁 Script executed:
# Look for the rest of the useWidgetFetch effect that handles widget fetch logic
sed -n '350,450p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 3804
🏁 Script executed:
# Search for the hosted widget loading logic mentioned in the review
grep -n "loadHostedChatGptWidget\|isCancelled" ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx | head -20Repository: MCPJam/inspector
Length of output: 416
🏁 Script executed:
# Check the hosted widget loading section around line 462
sed -n '455,530p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 2490
🏁 Script executed:
# Check if there's a dependency array for the main fetch effect in useWidgetFetch
sed -n '380,560p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx | grep -A 100 "useEffect.*storeWidgetData" | tail -50Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Verify the exact location where hosted path lacks isCancelled guard
sed -n '462,505p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 1513
🏁 Script executed:
# Find the complete effect with storeWidgetData and its dependency array
sed -n '384,560p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx | tail -100Repository: MCPJam/inspector
Length of output: 3506
🏁 Script executed:
# Get the dependency array for the storeWidgetData effect
sed -n '540,575p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 1380
🏁 Script executed:
# Double-check if useMemo is already imported
grep -n "useMemo\|useCallback\|useState\|useEffect\|useRef" ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx | head -20Repository: MCPJam/inspector
Length of output: 999
🏁 Script executed:
# Verify the exact hosted path doesn't have isCancelled guard before setWidgetUrl
sed -n '462,505p' ./mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx | grep -B 10 "setWidgetUrl.*blobUrl"Repository: MCPJam/inspector
Length of output: 422
Memoize extracted capability and inset objects to prevent unnecessary effect reruns and state races.
The extractors return fresh object literals on each render. When draftHostContext supplies overrides, these unstable references cause the effects at lines 905–939 and 1462–1500 to re-fire unnecessarily. More critically, the hosted widget path lacks a cancellation guard before mutating state (compare line 520's guard in the local path), risking duplicate bootstrap work when the fetch effect restarts mid-flight.
Proposed fix
+ const capabilityFallback = isPlaygroundActive
+ ? playgroundCapabilities
+ : DEFAULT_CAPABILITIES;
+ const safeAreaFallback = isPlaygroundActive
+ ? playgroundSafeAreaInsets
+ : DEFAULT_SAFE_AREA_INSETS;
+
- const capabilities = extractHostDeviceCapabilities(
- draftHostContext,
- isPlaygroundActive ? playgroundCapabilities : DEFAULT_CAPABILITIES,
- );
- const safeAreaInsets = extractHostSafeAreaInsets(
- draftHostContext,
- isPlaygroundActive ? playgroundSafeAreaInsets : DEFAULT_SAFE_AREA_INSETS,
- );
+ const capabilities = useMemo(
+ () => extractHostDeviceCapabilities(draftHostContext, capabilityFallback),
+ [draftHostContext, capabilityFallback],
+ );
+ const safeAreaInsets = useMemo(
+ () => extractHostSafeAreaInsets(draftHostContext, safeAreaFallback),
+ [draftHostContext, safeAreaFallback],
+ );📝 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
| const capabilities = extractHostDeviceCapabilities( | |
| draftHostContext, | |
| isPlaygroundActive ? playgroundCapabilities : DEFAULT_CAPABILITIES, | |
| ); | |
| const safeAreaInsets = extractHostSafeAreaInsets( | |
| draftHostContext, | |
| isPlaygroundActive ? playgroundSafeAreaInsets : DEFAULT_SAFE_AREA_INSETS, | |
| ); | |
| const capabilityFallback = isPlaygroundActive | |
| ? playgroundCapabilities | |
| : DEFAULT_CAPABILITIES; | |
| const safeAreaFallback = isPlaygroundActive | |
| ? playgroundSafeAreaInsets | |
| : DEFAULT_SAFE_AREA_INSETS; | |
| const capabilities = useMemo( | |
| () => extractHostDeviceCapabilities(draftHostContext, capabilityFallback), | |
| [draftHostContext, capabilityFallback], | |
| ); | |
| const safeAreaInsets = useMemo( | |
| () => extractHostSafeAreaInsets(draftHostContext, safeAreaFallback), | |
| [draftHostContext, safeAreaFallback], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx`
around lines 759 - 766, The extracted capability/inset objects created by
extractHostDeviceCapabilities and extractHostSafeAreaInsets are unstable each
render causing downstream effects to restart; wrap those calls in useMemo
(memoize capabilities and safeAreaInsets based on draftHostContext and
isPlaygroundActive/playground* or DEFAULT_* inputs) so their references only
change when inputs change, and in the hosted-widget bootstrap/fetch effect add
the same cancellation guard used in the local path (check a mounted/cancelled
flag before mutating state or performing duplicate bootstrap work) to prevent
state races and duplicated bootstraps when that effect is restarted mid-flight.
…nt-config Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx (1)
139-157: PreferstorePresetsto this inline store reset.This now hard-codes the full
useClientConfigStoreshape in the test, so new defaults can drift silently from production. Using the shared preset keeps the fixture aligned in one place.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/hooks/__tests__/use-server-state.test.tsx` around lines 139 - 157, Replace the inline reset of useClientConfigStore in the beforeEach with the shared test preset from storePresets: import storePresets from the test mocks and call useClientConfigStore.setState(storePresets.clientConfigStore) (or the appropriate property name on storePresets that holds the useClientConfigStore defaults) so the test uses the centralized store preset instead of hard-coding the full shape.mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx (1)
151-165: Please pull this reset fromstorePresets.This setup duplicates the full
useClientConfigStoredefault state, which makes the test brittle whenever the store gains a field. Using the shared preset keeps these fixtures synchronized with the real client defaults.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/hooks/__tests__/use-workspace-state.test.tsx` around lines 151 - 165, The test is manually duplicating the full useClientConfigStore default state; instead import the shared preset from storePresets and use that to reset the store (e.g. replace the literal useClientConfigStore.setState({...}) call with useClientConfigStore.setState(storePresets.<client-config-reset-preset>)); update the test imports to include storePresets and reference the appropriate client config reset preset from storePresets so the fixture stays in sync with the real defaults.mcpjam-inspector/client/src/components/client-config/__tests__/ClientConfigTab.test.tsx (1)
37-94: Consider adding a complementary positive test case.The suite validates that no warning appears when capabilities align, yet the converse—where
initializationInfo.clientCapabilitiesdiverges from the workspace profile—goes unexercised here.ServersTab.test.tsxcovers both directions; mirroring that symmetry would strengthen this file's coverage.🤖 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/__tests__/ClientConfigTab.test.tsx` around lines 37 - 94, Add a complementary negative-match test that asserts the "Needs reconnect" warning is shown when a server's initializationInfo.clientCapabilities does not match the workspace profile: create a new it block in ClientConfigTab.test.tsx that sets up defaultConfig via resetClientConfigStore and a server entry similar to the existing test but with initializationInfo.clientCapabilities set to a deliberately different object (e.g., modify mergeWorkspaceClientCapabilities output or use an extra/removed capability), render <ClientConfigTab ... /> and assert that screen.getByText("Needs reconnect") (or screen.queryByText(...).toBeInTheDocument()) is present; reference the existing helpers getDefaultClientCapabilities, mergeWorkspaceClientCapabilities, and the <ClientConfigTab> component to locate where to add this test.
🤖 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/hooks/use-workspace-state.ts`:
- Around line 181-185: The cleanup currently calls
clearPendingClientConfigSync() but doesn't settle the outstanding
remoteEchoPromise, allowing handleUpdateClientConfig and useClientConfigStore to
hang; update the cleanup to also settle (resolve or reject) the pending promise
exposed by clearPendingClientConfigSync/remoteEchoPromise so any awaiting
handlers (e.g., handleUpdateClientConfig) are unblocked and the saving state in
useClientConfigStore is cleared—locate the pending promise/reference in
clearPendingClientConfigSync and ensure the cleanup either calls its reject with
a descriptive Abort/Unmount error or resolves it with a terminal value, and
update any related promise creation/consumers so they handle this settled case
safely.
In `@mcpjam-inspector/client/src/lib/client-config.ts`:
- Around line 245-286: In extractHostDeviceCapabilities and
extractHostSafeAreaInsets, validate each nested field before returning it: for
extractHostDeviceCapabilities (deviceCapabilities), ensure hover and touch are
booleans (use typeof === "boolean") and fall back to
DEFAULT_HOST_DEVICE_CAPABILITIES values when they are absent or not booleans;
for extractHostSafeAreaInsets (safeAreaInsets), ensure top/right/bottom/left are
numbers and finite (use typeof === "number" && Number.isFinite(value)) and fall
back to DEFAULT_HOST_SAFE_AREA_INSETS for any invalid or missing fields; keep
the existing container checks (object and not array) but replace the direct
property coalescing with per-field validation to avoid accepting strings like
"false" or "12".
In `@mcpjam-inspector/client/src/stores/client-config-store.ts`:
- Around line 233-284: The mutators patchHostContext, resetSectionToDefault and
resetToBaseline must no-op while a remote-echo/save is in flight to avoid
clobbering pendingSavedConfig; add a guard at the top of each setter callback
that checks for the in-flight marker (e.g. state.pendingSavedConfig or a
saveInFlight flag) and simply return the unchanged state when present.
Specifically, in patchHostContext use the existing set((state) => { ... })
callback and early-return state if state.pendingSavedConfig (or
state.saveInFlight) is truthy; do the same in resetSectionToDefault before
computing defaultConfig/nextState, and in resetToBaseline before calling
resetFromConfig, so loadWorkspaceConfig’s re-application of pendingSavedConfig
won’t lose UI/theme changes. Ensure you reference and preserve draftConfig and
pendingSavedConfig variables in those checks.
- Around line 205-230: In setSectionText, when JSON parsing fails you must mark
the state dirty and preserve the last valid draft so the UI doesn't show "Saved"
incorrectly; update the catch branch in setSectionText to return the existing
state plus [textField]: text, [errorField]: (error message), and set isDirty:
true (while leaving draftConfig / the values returned by setSectionValue
untouched), referencing setSectionText, setSectionValue, draftConfig and isDirty
to locate the change.
In `@sdk/src/mcp-client-manager/capabilities.ts`:
- Around line 42-50: getDefaultClientCapabilities currently omits the
elicitation property while other code paths normalize it to an object; update
getDefaultClientCapabilities to include elicitation: {} in the returned
ClientCapabilityOptions so callers that use this helper advertise the same
capability shape as the normalized baseline (edit the
getDefaultClientCapabilities function to add elicitation: {} alongside
extensions).
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/client-config/__tests__/ClientConfigTab.test.tsx`:
- Around line 37-94: Add a complementary negative-match test that asserts the
"Needs reconnect" warning is shown when a server's
initializationInfo.clientCapabilities does not match the workspace profile:
create a new it block in ClientConfigTab.test.tsx that sets up defaultConfig via
resetClientConfigStore and a server entry similar to the existing test but with
initializationInfo.clientCapabilities set to a deliberately different object
(e.g., modify mergeWorkspaceClientCapabilities output or use an extra/removed
capability), render <ClientConfigTab ... /> and assert that
screen.getByText("Needs reconnect") (or
screen.queryByText(...).toBeInTheDocument()) is present; reference the existing
helpers getDefaultClientCapabilities, mergeWorkspaceClientCapabilities, and the
<ClientConfigTab> component to locate where to add this test.
In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx`:
- Around line 139-157: Replace the inline reset of useClientConfigStore in the
beforeEach with the shared test preset from storePresets: import storePresets
from the test mocks and call
useClientConfigStore.setState(storePresets.clientConfigStore) (or the
appropriate property name on storePresets that holds the useClientConfigStore
defaults) so the test uses the centralized store preset instead of hard-coding
the full shape.
In `@mcpjam-inspector/client/src/hooks/__tests__/use-workspace-state.test.tsx`:
- Around line 151-165: The test is manually duplicating the full
useClientConfigStore default state; instead import the shared preset from
storePresets and use that to reset the store (e.g. replace the literal
useClientConfigStore.setState({...}) call with
useClientConfigStore.setState(storePresets.<client-config-reset-preset>));
update the test imports to include storePresets and reference the appropriate
client config reset preset from storePresets so the fixture stays in sync with
the real defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc0a7976-c534-4eb2-a37d-6abd50bfaa46
📒 Files selected for processing (18)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ServersTab.tsxmcpjam-inspector/client/src/components/__tests__/ServersTab.test.tsxmcpjam-inspector/client/src/components/client-config/ClientConfigTab.tsxmcpjam-inspector/client/src/components/client-config/__tests__/ClientConfigTab.test.tsxmcpjam-inspector/client/src/components/shared/DisplayContextHeader.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/hosted/use-hosted-api-context.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/use-workspace-state.tsmcpjam-inspector/client/src/lib/__tests__/hosted-web-context.test.tsmcpjam-inspector/client/src/lib/apis/web/context.tsmcpjam-inspector/client/src/lib/client-config.tsmcpjam-inspector/client/src/state/mcp-api.tsmcpjam-inspector/client/src/stores/client-config-store.tssdk/src/mcp-client-manager/capabilities.tssdk/tests/browser-entry.test.ts
✅ Files skipped from review due to trivial changes (1)
- sdk/tests/browser-entry.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- mcpjam-inspector/client/src/state/mcp-api.ts
- mcpjam-inspector/client/src/lib/tests/hosted-web-context.test.ts
- mcpjam-inspector/client/src/hooks/hosted/use-hosted-api-context.ts
- mcpjam-inspector/client/src/hooks/use-server-state.ts
- mcpjam-inspector/client/src/App.tsx
Comment on lines
+245
to
+286
| export function extractHostDeviceCapabilities( | ||
| hostContext?: Record<string, unknown>, | ||
| fallback: HostDeviceCapabilities = DEFAULT_HOST_DEVICE_CAPABILITIES, | ||
| ): HostDeviceCapabilities { | ||
| const value = hostContext?.deviceCapabilities; | ||
| if (!value || typeof value !== "object" || Array.isArray(value)) { | ||
| return fallback; | ||
| } | ||
|
|
||
| const capabilities = value as { | ||
| hover?: boolean; | ||
| touch?: boolean; | ||
| }; | ||
|
|
||
| return { | ||
| hover: capabilities.hover ?? fallback.hover, | ||
| touch: capabilities.touch ?? fallback.touch, | ||
| }; | ||
| } | ||
|
|
||
| export function extractHostSafeAreaInsets( | ||
| hostContext?: Record<string, unknown>, | ||
| fallback: HostSafeAreaInsets = DEFAULT_HOST_SAFE_AREA_INSETS, | ||
| ): HostSafeAreaInsets { | ||
| const value = hostContext?.safeAreaInsets; | ||
| if (!value || typeof value !== "object" || Array.isArray(value)) { | ||
| return fallback; | ||
| } | ||
|
|
||
| const insets = value as { | ||
| top?: number; | ||
| right?: number; | ||
| bottom?: number; | ||
| left?: number; | ||
| }; | ||
|
|
||
| return { | ||
| top: insets.top ?? fallback.top, | ||
| right: insets.right ?? fallback.right, | ||
| bottom: insets.bottom ?? fallback.bottom, | ||
| left: insets.left ?? fallback.left, | ||
| }; |
Contributor
There was a problem hiding this comment.
Validate nested hostContext fields before returning them.
These helpers only validate the container object. Values like { deviceCapabilities: { hover: "false" } } or { safeAreaInsets: { top: "12" } } currently flow through as truthy strings instead of falling back, which can flip capability toggles and corrupt inset math. Please type-check each property, and only accept finite numbers for the inset fields.
🛠️ Sketch of a safer extraction
const capabilities = value as {
hover?: boolean;
touch?: boolean;
};
return {
- hover: capabilities.hover ?? fallback.hover,
- touch: capabilities.touch ?? fallback.touch,
+ hover:
+ typeof capabilities.hover === "boolean"
+ ? capabilities.hover
+ : fallback.hover,
+ touch:
+ typeof capabilities.touch === "boolean"
+ ? capabilities.touch
+ : fallback.touch,
};+ const readInset = (next: unknown, fallbackValue: number) =>
+ typeof next === "number" && Number.isFinite(next)
+ ? next
+ : fallbackValue;
+
return {
- top: insets.top ?? fallback.top,
- right: insets.right ?? fallback.right,
- bottom: insets.bottom ?? fallback.bottom,
- left: insets.left ?? fallback.left,
+ top: readInset(insets.top, fallback.top),
+ right: readInset(insets.right, fallback.right),
+ bottom: readInset(insets.bottom, fallback.bottom),
+ left: readInset(insets.left, fallback.left),
};📝 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
| export function extractHostDeviceCapabilities( | |
| hostContext?: Record<string, unknown>, | |
| fallback: HostDeviceCapabilities = DEFAULT_HOST_DEVICE_CAPABILITIES, | |
| ): HostDeviceCapabilities { | |
| const value = hostContext?.deviceCapabilities; | |
| if (!value || typeof value !== "object" || Array.isArray(value)) { | |
| return fallback; | |
| } | |
| const capabilities = value as { | |
| hover?: boolean; | |
| touch?: boolean; | |
| }; | |
| return { | |
| hover: capabilities.hover ?? fallback.hover, | |
| touch: capabilities.touch ?? fallback.touch, | |
| }; | |
| } | |
| export function extractHostSafeAreaInsets( | |
| hostContext?: Record<string, unknown>, | |
| fallback: HostSafeAreaInsets = DEFAULT_HOST_SAFE_AREA_INSETS, | |
| ): HostSafeAreaInsets { | |
| const value = hostContext?.safeAreaInsets; | |
| if (!value || typeof value !== "object" || Array.isArray(value)) { | |
| return fallback; | |
| } | |
| const insets = value as { | |
| top?: number; | |
| right?: number; | |
| bottom?: number; | |
| left?: number; | |
| }; | |
| return { | |
| top: insets.top ?? fallback.top, | |
| right: insets.right ?? fallback.right, | |
| bottom: insets.bottom ?? fallback.bottom, | |
| left: insets.left ?? fallback.left, | |
| }; | |
| export function extractHostDeviceCapabilities( | |
| hostContext?: Record<string, unknown>, | |
| fallback: HostDeviceCapabilities = DEFAULT_HOST_DEVICE_CAPABILITIES, | |
| ): HostDeviceCapabilities { | |
| const value = hostContext?.deviceCapabilities; | |
| if (!value || typeof value !== "object" || Array.isArray(value)) { | |
| return fallback; | |
| } | |
| const capabilities = value as { | |
| hover?: boolean; | |
| touch?: boolean; | |
| }; | |
| return { | |
| hover: | |
| typeof capabilities.hover === "boolean" | |
| ? capabilities.hover | |
| : fallback.hover, | |
| touch: | |
| typeof capabilities.touch === "boolean" | |
| ? capabilities.touch | |
| : fallback.touch, | |
| }; | |
| } | |
| export function extractHostSafeAreaInsets( | |
| hostContext?: Record<string, unknown>, | |
| fallback: HostSafeAreaInsets = DEFAULT_HOST_SAFE_AREA_INSETS, | |
| ): HostSafeAreaInsets { | |
| const value = hostContext?.safeAreaInsets; | |
| if (!value || typeof value !== "object" || Array.isArray(value)) { | |
| return fallback; | |
| } | |
| const insets = value as { | |
| top?: number; | |
| right?: number; | |
| bottom?: number; | |
| left?: number; | |
| }; | |
| const readInset = (next: unknown, fallbackValue: number) => | |
| typeof next === "number" && Number.isFinite(next) | |
| ? next | |
| : fallbackValue; | |
| return { | |
| top: readInset(insets.top, fallback.top), | |
| right: readInset(insets.right, fallback.right), | |
| bottom: readInset(insets.bottom, fallback.bottom), | |
| left: readInset(insets.left, fallback.left), | |
| }; | |
| } |
🤖 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` around lines 245 - 286, In
extractHostDeviceCapabilities and extractHostSafeAreaInsets, validate each
nested field before returning it: for extractHostDeviceCapabilities
(deviceCapabilities), ensure hover and touch are booleans (use typeof ===
"boolean") and fall back to DEFAULT_HOST_DEVICE_CAPABILITIES values when they
are absent or not booleans; for extractHostSafeAreaInsets (safeAreaInsets),
ensure top/right/bottom/left are numbers and finite (use typeof === "number" &&
Number.isFinite(value)) and fall back to DEFAULT_HOST_SAFE_AREA_INSETS for any
invalid or missing fields; keep the existing container checks (object and not
array) but replace the direct property coalescing with per-field validation to
avoid accepting strings like "false" or "12".
Comment on lines
+205
to
+230
| setSectionText: (section, text) => { | ||
| set((state) => { | ||
| const textField = | ||
| section === "clientCapabilities" | ||
| ? "clientCapabilitiesText" | ||
| : "hostContextText"; | ||
| const errorField = | ||
| section === "clientCapabilities" | ||
| ? "clientCapabilitiesError" | ||
| : "hostContextError"; | ||
|
|
||
| try { | ||
| const parsed = parseRecordJson(text); | ||
| return { | ||
| ...setSectionValue(state, section, parsed), | ||
| [textField]: text, | ||
| [errorField]: null, | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| [textField]: text, | ||
| [errorField]: | ||
| error instanceof Error ? error.message : "Invalid JSON", | ||
| }; | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Invalid JSON can still read as “Saved.”
When parsing fails, this branch updates the raw text and error but leaves draftConfig and isDirty unchanged. The result is that the tab can show Saved and disable the baseline Reset even though the visible JSON no longer matches persisted state. Dirtiness needs to account for raw text/error state, not only the last valid object.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/stores/client-config-store.ts` around lines 205 -
230, In setSectionText, when JSON parsing fails you must mark the state dirty
and preserve the last valid draft so the UI doesn't show "Saved" incorrectly;
update the catch branch in setSectionText to return the existing state plus
[textField]: text, [errorField]: (error message), and set isDirty: true (while
leaving draftConfig / the values returned by setSectionValue untouched),
referencing setSectionText, setSectionValue, draftConfig and isDirty to locate
the change.
Comment on lines
+233
to
+284
| patchHostContext: (patch) => { | ||
| set((state) => { | ||
| const currentHostContext = state.draftConfig?.hostContext ?? {}; | ||
| const nextHostContext = { | ||
| ...currentHostContext, | ||
| ...patch, | ||
| }; | ||
| const nextState = setSectionValue( | ||
| state, | ||
| "hostContext", | ||
| nextHostContext, | ||
| ); | ||
| return { | ||
| ...nextState, | ||
| hostContextText: stringifyJson(nextHostContext), | ||
| hostContextError: null, | ||
| }; | ||
| }); | ||
| }, | ||
|
|
||
| resetSectionToDefault: (section) => { | ||
| set((state) => { | ||
| const defaultConfig = state.defaultConfig; | ||
| if (!defaultConfig) { | ||
| return {}; | ||
| } | ||
|
|
||
| const nextValue = defaultConfig[section]; | ||
| const nextState = setSectionValue(state, section, nextValue); | ||
| return { | ||
| ...nextState, | ||
| ...(section === "clientCapabilities" | ||
| ? { | ||
| clientCapabilitiesText: stringifyJson(nextValue), | ||
| clientCapabilitiesError: null, | ||
| } | ||
| : { | ||
| hostContextText: stringifyJson(nextValue), | ||
| hostContextError: null, | ||
| }), | ||
| }; | ||
| }); | ||
| }, | ||
|
|
||
| resetToBaseline: () => { | ||
| set((state) => | ||
| resetFromConfig( | ||
| state.activeWorkspaceId, | ||
| state.defaultConfig, | ||
| state.savedConfig, | ||
| ), | ||
| ); |
Contributor
There was a problem hiding this comment.
Block config mutations during the remote-echo window.
These mutators still rewrite draftConfig while a save is in flight. When the echoed config arrives, loadWorkspaceConfig reapplies pendingSavedConfig, so any theme/display/safe-area changes made in that window are silently lost.
🛠️ Minimal guard
patchHostContext: (patch) => {
set((state) => {
+ if (state.isSaving) {
+ return state;
+ }
const currentHostContext = state.draftConfig?.hostContext ?? {};
const nextHostContext = {
...currentHostContext,
...patch,
};
@@
resetSectionToDefault: (section) => {
set((state) => {
+ if (state.isSaving) {
+ return state;
+ }
const defaultConfig = state.defaultConfig;
if (!defaultConfig) {
return {};
}
@@
resetToBaseline: () => {
set((state) =>
- resetFromConfig(
- state.activeWorkspaceId,
- state.defaultConfig,
- state.savedConfig,
- ),
+ state.isSaving
+ ? state
+ : resetFromConfig(
+ state.activeWorkspaceId,
+ state.defaultConfig,
+ state.savedConfig,
+ ),
);
},📝 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
| patchHostContext: (patch) => { | |
| set((state) => { | |
| const currentHostContext = state.draftConfig?.hostContext ?? {}; | |
| const nextHostContext = { | |
| ...currentHostContext, | |
| ...patch, | |
| }; | |
| const nextState = setSectionValue( | |
| state, | |
| "hostContext", | |
| nextHostContext, | |
| ); | |
| return { | |
| ...nextState, | |
| hostContextText: stringifyJson(nextHostContext), | |
| hostContextError: null, | |
| }; | |
| }); | |
| }, | |
| resetSectionToDefault: (section) => { | |
| set((state) => { | |
| const defaultConfig = state.defaultConfig; | |
| if (!defaultConfig) { | |
| return {}; | |
| } | |
| const nextValue = defaultConfig[section]; | |
| const nextState = setSectionValue(state, section, nextValue); | |
| return { | |
| ...nextState, | |
| ...(section === "clientCapabilities" | |
| ? { | |
| clientCapabilitiesText: stringifyJson(nextValue), | |
| clientCapabilitiesError: null, | |
| } | |
| : { | |
| hostContextText: stringifyJson(nextValue), | |
| hostContextError: null, | |
| }), | |
| }; | |
| }); | |
| }, | |
| resetToBaseline: () => { | |
| set((state) => | |
| resetFromConfig( | |
| state.activeWorkspaceId, | |
| state.defaultConfig, | |
| state.savedConfig, | |
| ), | |
| ); | |
| patchHostContext: (patch) => { | |
| set((state) => { | |
| if (state.isSaving) { | |
| return state; | |
| } | |
| const currentHostContext = state.draftConfig?.hostContext ?? {}; | |
| const nextHostContext = { | |
| ...currentHostContext, | |
| ...patch, | |
| }; | |
| const nextState = setSectionValue( | |
| state, | |
| "hostContext", | |
| nextHostContext, | |
| ); | |
| return { | |
| ...nextState, | |
| hostContextText: stringifyJson(nextHostContext), | |
| hostContextError: null, | |
| }; | |
| }); | |
| }, | |
| resetSectionToDefault: (section) => { | |
| set((state) => { | |
| if (state.isSaving) { | |
| return state; | |
| } | |
| const defaultConfig = state.defaultConfig; | |
| if (!defaultConfig) { | |
| return {}; | |
| } | |
| const nextValue = defaultConfig[section]; | |
| const nextState = setSectionValue(state, section, nextValue); | |
| return { | |
| ...nextState, | |
| ...(section === "clientCapabilities" | |
| ? { | |
| clientCapabilitiesText: stringifyJson(nextValue), | |
| clientCapabilitiesError: null, | |
| } | |
| : { | |
| hostContextText: stringifyJson(nextValue), | |
| hostContextError: null, | |
| }), | |
| }; | |
| }); | |
| }, | |
| resetToBaseline: () => { | |
| set((state) => | |
| state.isSaving | |
| ? state | |
| : resetFromConfig( | |
| state.activeWorkspaceId, | |
| state.defaultConfig, | |
| state.savedConfig, | |
| ), | |
| ); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/stores/client-config-store.ts` around lines 233 -
284, The mutators patchHostContext, resetSectionToDefault and resetToBaseline
must no-op while a remote-echo/save is in flight to avoid clobbering
pendingSavedConfig; add a guard at the top of each setter callback that checks
for the in-flight marker (e.g. state.pendingSavedConfig or a saveInFlight flag)
and simply return the unchanged state when present. Specifically, in
patchHostContext use the existing set((state) => { ... }) callback and
early-return state if state.pendingSavedConfig (or state.saveInFlight) is
truthy; do the same in resetSectionToDefault before computing
defaultConfig/nextState, and in resetToBaseline before calling resetFromConfig,
so loadWorkspaceConfig’s re-application of pendingSavedConfig won’t lose
UI/theme changes. Ensure you reference and preserve draftConfig and
pendingSavedConfig variables in those checks.
Comment on lines
+42
to
+50
| export function getDefaultClientCapabilities(): ClientCapabilityOptions { | ||
| return { | ||
| extensions: { | ||
| [MCP_UI_EXTENSION_ID]: { | ||
| mimeTypes: [MCP_UI_RESOURCE_MIME_TYPE], | ||
| }, | ||
| }, | ||
| } as ClientCapabilityOptions; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the default helper aligned with the normalized baseline.
Line 42 returns a default object without elicitation, but Lines 52-64 and Lines 66-99 always normalize that field to {}. Any caller that falls back straight to getDefaultClientCapabilities() will advertise a different capability set than the merge path, which can skew reconnect checks and hosted initialize payloads.
🛠️ Suggested fix
export function getDefaultClientCapabilities(): ClientCapabilityOptions {
return {
+ elicitation: {},
extensions: {
[MCP_UI_EXTENSION_ID]: {
mimeTypes: [MCP_UI_RESOURCE_MIME_TYPE],
},
},
} as ClientCapabilityOptions;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/mcp-client-manager/capabilities.ts` around lines 42 - 50,
getDefaultClientCapabilities currently omits the elicitation property while
other code paths normalize it to an object; update getDefaultClientCapabilities
to include elicitation: {} in the returned ClientCapabilityOptions so callers
that use this helper advertise the same capability shape as the normalized
baseline (edit the getDefaultClientCapabilities function to add elicitation: {}
alongside extensions).
… into add-client-config
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| lastForcedDisplayModeRef.current = effectiveDisplayMode; | ||
| setDisplayMode(effectiveDisplayMode); | ||
| }, [effectiveDisplayMode, requestedDisplayMode, setDisplayMode]); |
There was a problem hiding this comment.
themeModeRef not updated between init and late assignment
Medium Severity
The themeModeRef is initialized with themeMode (the raw preferences value) on line 440, but the old themeModeRef.current = themeMode sync line was removed. The replacement assignment themeModeRef.current = resolvedTheme now happens much later at line 690. The ref is initialized with themeMode but conceptually now represents resolvedTheme (which can differ when draftHostContext overrides the theme). On the first render, themeModeRef.current will be themeMode instead of resolvedTheme when fetchMcpAppsWidgetContent reads it at line 514, potentially sending the wrong theme to the widget content fetch.
Additional Locations (1)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sdk/src/mcp-client-manager/capabilities.ts (1)
42-50:⚠️ Potential issue | 🟠 MajorDefault capability helper still drifts from normalized baseline.
Line 42 through Line 50 omit
elicitation, while Line 59 through Line 61 guarantee it elsewhere. This shape mismatch can skew capability equality checks depending on call path.Proposed fix
export function getDefaultClientCapabilities(): ClientCapabilityOptions { return { + elicitation: {}, extensions: { [MCP_UI_EXTENSION_ID]: { mimeTypes: [MCP_UI_RESOURCE_MIME_TYPE], }, }, } as ClientCapabilityOptions; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/mcp-client-manager/capabilities.ts` around lines 42 - 50, getDefaultClientCapabilities currently omits the elicitation property causing shape mismatches; update getDefaultClientCapabilities to include the elicitation key with the same normalized baseline shape used elsewhere (so equality checks are consistent) — add an elicitation entry (e.g., an empty object matching the ClientCapabilityOptions elicitation shape) alongside extensions in getDefaultClientCapabilities to mirror the guaranteed presence of elicitation in other code paths.
🤖 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/hooks/__tests__/use-workspace-state.test.tsx`:
- Around line 397-399: The test's timeout assertion uses the wrong error text;
update the assertion for savePromise in use-workspace-state.test.tsx so it
matches the actual rejection message from the hook (originating in
useWorkspaceState) — either change the expected string to "Client config update
timed out - no echo received from server." or relax the check to match the
timeout error generically (e.g., use a case-insensitive regex like /timed out/i)
so it aligns with the error thrown by the save/persist logic.
---
Duplicate comments:
In `@sdk/src/mcp-client-manager/capabilities.ts`:
- Around line 42-50: getDefaultClientCapabilities currently omits the
elicitation property causing shape mismatches; update
getDefaultClientCapabilities to include the elicitation key with the same
normalized baseline shape used elsewhere (so equality checks are consistent) —
add an elicitation entry (e.g., an empty object matching the
ClientCapabilityOptions elicitation shape) alongside extensions in
getDefaultClientCapabilities to mirror the guaranteed presence of elicitation in
other code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58496316-80e3-468e-ac0b-2bf77e75b3cf
📒 Files selected for processing (15)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxmcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsxmcpjam-inspector/client/src/components/client-config/WorkspaceClientConfigSync.tsxmcpjam-inspector/client/src/components/client-config/__tests__/WorkspaceClientConfigSync.test.tsxmcpjam-inspector/client/src/components/shared/DisplayContextHeader.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-server-state.tsmcpjam-inspector/client/src/hooks/use-workspace-state.tssdk/src/mcp-client-manager/MCPClientManager.tssdk/src/mcp-client-manager/capabilities.tssdk/src/mcp-client-manager/types.tssdk/tests/MCPClientManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- sdk/src/mcp-client-manager/MCPClientManager.ts
- mcpjam-inspector/client/src/hooks/tests/use-server-state.test.tsx
- mcpjam-inspector/client/src/hooks/use-server-state.ts
- mcpjam-inspector/client/src/hooks/use-workspace-state.ts
- mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx
Comment on lines
+397
to
+399
| await expect(savePromise).rejects.toThrow( | ||
| "Timed out waiting for workspace client config to sync.", | ||
| ); |
Contributor
There was a problem hiding this comment.
Error message mismatch in timeout assertion.
The test expects "Timed out waiting for workspace client config to sync." but the relevant code snippet (context snippet 2, lines 548-570 of use-workspace-state.ts) shows the rejection message is "Client config update timed out - no echo received from server.". This assertion will fail.
Proposed fix
await expect(savePromise).rejects.toThrow(
- "Timed out waiting for workspace client config to sync.",
+ "Client config update timed out - no echo received from server.",
);📝 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
| await expect(savePromise).rejects.toThrow( | |
| "Timed out waiting for workspace client config to sync.", | |
| ); | |
| await expect(savePromise).rejects.toThrow( | |
| "Client config update timed out - no echo received from server.", | |
| ); |
🤖 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 397 - 399, The test's timeout assertion uses the wrong error text;
update the assertion for savePromise in use-workspace-state.test.tsx so it
matches the actual rejection message from the hook (originating in
useWorkspaceState) — either change the expected string to "Client config update
timed out - no echo received from server." or relax the check to match the
timeout error generically (e.g., use a case-insensitive regex like /timed out/i)
so it aligns with the error thrown by the save/persist logic.
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
1 participant
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.
Note
Medium Risk
Adds a new per-workspace client configuration surface that feeds into server connection initialization and hosted API request context, which can affect connectivity and widget rendering if misconfigured. Changes are mostly additive but touch connection/reconnect flows and hosted request building, increasing behavioral risk.
Overview
Adds a per-workspace "Client Config" (aka client profile) feature (gated by
client-config-enabled) with a new tab to editclientCapabilitiesandhostContext, plus background syncing (WorkspaceClientConfigSync) to keep a local draft aligned with saved workspace config.Wires the workspace client profile into runtime behavior: server connect/reconnect now merges workspace+server capabilities into both
capabilitiesandclientCapabilities, UI widgets (MCP Apps + ChatGPT widgets) derive locale/timezone/theme/device/safe-area/display-modes fromhostContext, and display-mode controls are additionally constrained by host-advertised available modes.Introduces sync/consistency safeguards: client-config saves can wait for a Convex “remote echo” with timeout, connect/reconnect and hosted workspace requests are blocked while a save is pending, and servers/UI surface “Needs reconnect” warnings when saved client capabilities differ from the last initialization payload. Also updates many imports to use
@mcpjam/sdk/browsertypes and extends tests accordingly.Written by Cursor Bugbot for commit ad1f02d. This will update automatically on new commits. Configure here.