Merged
Conversation
dosubot
bot
added
the
size:XXL
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
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (63)
WalkthroughAdds first-class sandbox support across client and server. Client changes: new Sandboxes UI (tab, list, editor, create/share dialogs, usage panel), SandboxChatPage, sandbox-session storage utilities, sandbox host-style theming/context, hosted OAuth gate/resume handling, sandbox-aware hosted API context, widget snapshot and chat-session plumbing, UI/style assets, and many tests. Server changes: /sandboxes bootstrap route, sandboxToken propagated through auth, servers, and chat-v2 flows, and shared-chat persistence updated to accept sandbox tokens. |
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/server/routes/web/auth.ts (1)
147-178:⚠️ Potential issue | 🟠 MajorPreserve the intended 400 here.
The mutual-exclusivity guard sits inside the same
tryasfetch, so itsWebRouteError(400, ...)is immediately caught and rewritten as502 SERVER_UNREACHABLE. Move that guard outside thetry, or rethrowWebRouteErrorunchanged.🩹 Minimal fix
let response: Response; - try { - if (options?.shareToken && options?.sandboxToken) { - throw new WebRouteError( - 400, - ErrorCode.VALIDATION_ERROR, - "shareToken and sandboxToken cannot both be provided", - ); - } + if (options?.shareToken && options?.sandboxToken) { + throw new WebRouteError( + 400, + ErrorCode.VALIDATION_ERROR, + "shareToken and sandboxToken cannot both be provided", + ); + } + + try { response = await fetch(`${convexUrl}/web/authorize`, { method: "POST", @@ }); } catch (error) { + if (error instanceof WebRouteError) { + throw error; + } throw new WebRouteError( 502, ErrorCode.SERVER_UNREACHABLE,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/auth.ts` around lines 147 - 178, The 400 WebRouteError thrown by the mutual-exclusivity guard is being caught by the same try that wraps the fetch and turned into a 502; update the code so the guard for options?.shareToken and options?.sandboxToken runs outside the try block (or, alternatively, catch and rethrow WebRouteError unchanged) so that the explicit WebRouteError(400, ErrorCode.VALIDATION_ERROR, ...) from that guard is preserved; locate the guard and the surrounding try that calls fetch(`${convexUrl}/web/authorize`, ...) and move the if (options?.shareToken && options?.sandboxToken) check before entering that try (or add a catch that detects instance of WebRouteError and throws it again) so the correct 400 response is returned.
🧹 Nitpick comments (9)
mcpjam-inspector/client/src/components/sandboxes/SandboxShareSection.tsx (3)
95-97: Type assertions on mutation results merit caution.Casting mutation results with
as SandboxSettingshere and in sibling handlers (handleModeChange,handleInvite,handleRemoveMember) bypasses TypeScript's inference. Should the mutation contract evolve, these casts will silently mask mismatches.Consider narrowing via a type guard or adjusting the mutation hook's return type for stronger guarantees.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/sandboxes/SandboxShareSection.tsx` around lines 95 - 97, Summary: Avoid unsafe TypeScript casts like "as SandboxSettings" on mutation results (seen in rotateSandboxLink and sibling handlers handleModeChange, handleInvite, handleRemoveMember); instead validate the shape or update the mutation hook's return type. Fix: update the mutation hook signature to return Promise<SandboxSettings> (or provide a generic) so callers get correct types, or add a runtime type guard (e.g., isSandboxSettings(result)) and only treat the value as SandboxSettings after the guard passes; locate uses in rotateSandboxLink({ sandboxId: settings.sandboxId }) and the other handlers and remove the "as SandboxSettings" casts, replacing them with the guard check or relying on the corrected hook return type. Ensure the guard verifies required fields of SandboxSettings before proceeding so future API changes fail fast rather than being silently accepted.
344-346: Displaying a placeholder link with an empty token may confuse users.When
settings.link?.urlis absent,buildSandboxLink("", settings.name)renders a visibly incomplete URL. Consider showing explicit placeholder text (e.g., "Link not yet generated") or hiding the section entirely until a valid token exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/sandboxes/SandboxShareSection.tsx` around lines 344 - 346, The current rendering in SandboxShareSection uses buildSandboxLink("", settings.name) when settings.link?.url is missing, producing an incomplete URL; update the JSX so that instead of calling buildSandboxLink with an empty token you either hide the paragraph or display a clear placeholder like "Link not yet generated" when settings.link?.url is falsy. Locate the paragraph in SandboxShareSection that references settings.link?.url and buildSandboxLink and change it to a conditional expression: if settings.link?.url render the real URL (e.g., settings.link.url or buildSandboxLink(token, settings.name)), otherwise render the placeholder text (or skip rendering the <p> entirely) to avoid showing an incomplete link.
132-134: Client-side email validation would refine the UX.Currently,
handleInviteonly checks for non-empty input. Validating email format before submission would spare users a round-trip for obviously malformed addresses.✨ Proposed validation
const handleInvite = async () => { const normalizedEmail = email.trim().toLowerCase(); if (!normalizedEmail) return; + + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!emailRegex.test(normalizedEmail)) { + toast.error("Please enter a valid email address"); + return; + } setIsMutating(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/sandboxes/SandboxShareSection.tsx` around lines 132 - 134, handleInvite currently only checks for an empty input; add client-side email format validation after computing normalizedEmail (in function handleInvite) by testing normalizedEmail against a reliable email regex or a validator utility and return early if it fails. On failure, set the existing invite/error UI state (e.g., setInviteError or similar) with a user-friendly message and avoid calling the invite API; keep the trim().toLowerCase() normalization and only proceed to API calls when the regex/validator accepts the address.mcpjam-inspector/client/src/lib/__tests__/hosted-web-context.test.ts (1)
64-75: Consider adding mutual exclusivity test.The existing test "omits share scope fields when no share token is present" could be complemented with a test verifying behavior when both tokens are present—if that scenario should be prevented or handled gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/__tests__/hosted-web-context.test.ts` around lines 64 - 75, Add a new unit test that covers the case where both an access token and a share token are present to verify mutual exclusivity: call setHostedApiContext with workspaceId and serverIdsByName and provide both getAccessToken (returning a string) and getShareToken (returning a string), then call buildHostedServerRequest("bench") and assert the actual behavior your code should enforce (e.g., throws an error, prefers the share token and omits access fields, or prefers access token and omits share fields). Reference setHostedApiContext and buildHostedServerRequest in the test and assert the expected shape or error to make the mutual-exclusivity contract explicit.mcpjam-inspector/client/src/components/sandboxes/SandboxEditor.tsx (1)
147-172: Array comparison viaJSON.stringifyis functional but brittle.The sorted array comparison works correctly here; however, relying on stringification for equality can obscure intent. A simple length + element comparison would be more explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/sandboxes/SandboxEditor.tsx` around lines 147 - 172, The equality check in hasUnsavedChanges currently uses JSON.stringify on formServerIds and currentServerIds which is brittle; replace it with an explicit comparison that first checks lengths and then checks elements pairwise (e.g., formServerIds.length === currentServerIds.length && formServerIds.every((id, i) => id === currentServerIds[i])). Update the return condition inside the useMemo to use this explicit comparison of selectedServerIds (formServerIds) and sandbox!.servers.map(...).sort() (currentServerIds) instead of JSON.stringify, keeping the existing sort behavior and hooks list unchanged.mcpjam-inspector/client/src/hooks/useSharedChatThreads.ts (1)
44-59: Type assertions (as any) are a systematic workaround for Convex's query typing.This pattern spans the codebase (used in
useProfilePicture,useServerShares, and multiple components). While pragmatic given Convex's constraints, consider consolidating these patterns with shared utility functions or documentation of the expected query contracts to reduce repetition and improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/useSharedChatThreads.ts` around lines 44 - 59, The code repeatedly uses unsafe type assertions ("as any") around Convex queries (see queryName, queryArgs, and useQuery in useSharedChatThreads.ts returning SharedChatThread[]), so replace these ad-hoc casts with a small shared helper or typed wrapper (e.g., a typedUseQuery<T> or buildQueryArgs utility) that centralizes Convex typing logic and returns the correct generic type instead of forcing "any"; update useSharedChatThreads to call that helper for both sandbox and share branches (removing "as any" on queryName/queryArgs) and apply the same helper to other occurrences like useProfilePicture and useServerShares to eliminate repeated unsafe assertions.mcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsx (2)
359-366: Polling loop may miss token arrival.The 15-iteration × 100ms polling window provides ~1.5 seconds for tokens to appear after OAuth completion. On slower networks or OAuth flows that require additional round-trips, this window may expire before
getStoredTokensreflects the new token, leaving the user in a silently incomplete state.Consider an exponential backoff or a longer timeout with fewer iterations. Alternatively, listen for a storage event to detect token arrival deterministically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsx` around lines 359 - 366, The current polling loop in SandboxChatPage (for loop awaiting 15×100ms and calling getStoredTokens(server.serverName) then setOauthRefreshNonce) can miss later token arrivals; replace this fragile short fixed-loop with either a storage-event based watcher that listens for window "storage" changes for the key used by getStoredTokens(server.serverName) and triggers setOauthRefreshNonce when access_token appears, or implement an exponential-backoff retry (e.g., increase delay up to a longer cap and extend total wait time) while still breaking when tokens?.access_token is found; update the code around getStoredTokens and setOauthRefreshNonce to use the chosen deterministic approach and remove the fixed 15-iteration loop.
88-98: Type assertion masks interface conformance.The
as anyon line 92 circumvents type checking for theconfigproperty. IfServerWithNameevolves to require additional fields, this won't surface as a compile-time error.Consider defining a minimal
SandboxServerConfigtype or using a type-safe partial approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsx` around lines 88 - 98, The config field currently uses an unsafe assertion ("as any") which bypasses type checking for ServerWithName; replace it by defining a minimal typed config (e.g., SandboxServerConfig or Partial of the real ServerConfig) that includes url and any required keys, import or declare that type in SandboxChatPage.tsx, then remove the "as any" and assign the object to that typed config so the surrounding object literal (the ServerWithName entry) can be validated by the compiler; update the object using the new type (or Partial<ServerConfig>) and run TypeScript checks to ensure it satisfies ServerWithName without using any casts.mcpjam-inspector/client/src/hooks/useSandboxes.ts (1)
75-78: Type safety bypassed viaas anycasts.The query name and arguments are cast to
any, surrendering compile-time validation. If the Convex function signature changes, these calls will fail at runtime without prior warning.If generated Convex types exist (e.g., from
convex/_generated/api), importing them would restore type safety. Otherwise, consider defining local type guards or Zod schemas for runtime validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/useSandboxes.ts` around lines 75 - 78, The call to useQuery in useSandboxes.ts is bypassing type safety by casting the query key and args to any; replace the any casts by importing and using the generated Convex types (or typings from convex/_generated/api) for the query name and its argument/return types and pass the correctly typed key/args instead of "sandboxes:listSandboxes" as any and ({ workspaceId } as any); update the sandboxes variable typing to use the Convex-generated return type (or a locally defined interface/Zod-validated type) so useQuery<ReturnType> and the query arguments use the proper typed shape, and keep the conditional skip logic (isAuthenticated && workspaceId) but provide a typed skip sentinel per your query hook contract.
🤖 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/connection/hooks/use-server-form.ts`:
- Around line 190-198: The URL parsing must run regardless of whether HTTPS is
enforced: in the validateForm logic inside use-server-form (where HOSTED_MODE
and options?.requireHttps are checked) always attempt to construct new
URL(url.trim()) in a try/catch to validate syntax first, returning "Invalid URL
format" on failure; only after successful parsing, then enforce protocol ===
"https:" when HOSTED_MODE || options?.requireHttps and return "HTTPS is
required" if it's not.
In `@mcpjam-inspector/client/src/components/sandboxes/SandboxEditor.tsx`:
- Around line 93-108: The initializer for modelId in SandboxEditor uses
hostedModels[0]?.id with a fallback "openai/gpt-5-mini", which can still produce
an invalid/empty selection if hostedModels is empty or the fallback isn't
present in availableModels; update the logic that computes hostedModels (from
SUPPORTED_MODELS and isMCPJamProvidedModel) to handle an empty result (e.g.,
choose the first id from availableModels or set modelId to a safe non-empty
default), and add a validation in handleSave to ensure modelId is non-empty and
exists in availableModels before saving (returning or showing an error if
invalid) so users cannot save a sandbox with an invalid modelId.
In `@mcpjam-inspector/client/src/components/sandboxes/SandboxShareSection.tsx`:
- Around line 263-270: The remove button is only revealed on hover, making it
inaccessible to keyboard users; update the Button rendering in
SandboxShareSection to include focus-based reveal classes so keyboard focus
shows the control—specifically, modify the Button's className (currently "size-7
opacity-0 group-hover:opacity-100") to also include
"group-focus-within:opacity-100" (and optionally "group-focus:opacity-100") so
that when the row receives keyboard focus the remove Button (handled by
handleRemoveMember) becomes visible and operable.
In `@mcpjam-inspector/client/src/components/sandboxes/ShareSandboxDialog.tsx`:
- Around line 73-76: The useMemo for activeMembers assumes settings.members is
always an array and will throw if undefined; update the computation in
ShareSandboxDialog (activeMembers/useMemo) to defensively handle missing members
by using a fallback (e.g., (settings.members || []) or optional chaining) before
calling filter, and ensure the dependency array still references
settings.members (or settings) appropriately so memoization remains correct.
In `@mcpjam-inspector/client/src/components/SandboxesTab.tsx`:
- Around line 204-212: The Share button currently calls setRightPaneView("edit")
so it opens the Edit view instead of the share dialog; change the onClick of the
Share Button (the Button rendering the Share2 icon) to call
setRightPaneView("share") (or the exact rightPaneView value used to render
ShareSandboxDialog) so that selecting Share opens ShareSandboxDialog; update any
related conditional that checks rightPaneView in the component to ensure "share"
maps to ShareSandboxDialog.
In `@mcpjam-inspector/server/routes/web/chat-v2.ts`:
- Line 96: The web route currently hard-codes includeMcpToolInventory: true
which differs from the MCP route (omitted) and increases prompt payload; decide
whether tool inventory should be included for web-hosted requests and then
either (a) remove the hard-coded includeMcpToolInventory flag from the web route
to match the MCP behavior, (b) set it to false, or (c) make it configurable
(e.g., use a feature flag or env var) and default to the MCP behavior; update
the code path that constructs the request (look for the web chat-v2 route
handler where includeMcpToolInventory is set) and any relevant tests/docs to
reflect the chosen behavior so web and MCP routes are consistent or
intentionally documented as different.
In `@mcpjam-inspector/server/routes/web/sandboxes.ts`:
- Around line 21-28: Validate the convexUrl scheme before forwarding bearer
tokens: parse convexUrl (e.g., new URL(convexUrl)) and if its protocol is not
'https:' (allowing an explicit dev exception for 'http://' to
localhost/127.0.0.1 if desired) throw the existing WebRouteError with
ErrorCode.INTERNAL_ERROR and a clear message about rejecting non-HTTPS
CONVEX_HTTP_URL; update the convexUrl check block (the code that currently
throws when !convexUrl) to perform this protocol validation and only accept
secure endpoints.
- Around line 36-45: The upstream fetch to `${convexUrl}/sandbox/bootstrap` can
hang indefinitely; modify the handler to use an AbortController with a
configurable timeout (e.g., 5–10s), call controller.abort() after the timer, and
pass signal to the fetch so the request is cancelled on timeout; ensure you
clear the timeout on success and handle AbortError to return a 504/timeout
response instead of letting the request pile up (update the code around
response, fetch, bearerToken, convexUrl and body.token to use the controller and
timeout logic).
In `@mcpjam-inspector/server/utils/__tests__/chat-v2-orchestration.test.ts`:
- Around line 72-88: The test must assert that inventory lookup is skipped by
verifying getToolsForAiSdk (or the method on the mcpClientManager that performs
tool lookup) is not called when the MCP inventory flag is off; update the test
for prepareChatV2 to spy/mock the getToolsForAiSdk function (or the manager
method used in prepareChatV2) and add an expectation that the spy was not
invoked, while keeping the existing assertion that enhancedSystemPrompt equals
"Base prompt."; reference prepareChatV2 and getToolsForAiSdk (or the
mcpClientManager method) to locate where to attach the spy/mock.
In `@mcpjam-inspector/server/utils/chat-v2-orchestration.ts`:
- Around line 87-90: The code is inserting untrusted toolRecord.description into
enhancedSystemPrompt (via truncateToolDescription), which lets external servers
inject system-level instructions; change this by removing description from any
system prompt assembly (references: toolRecord.description,
truncateToolDescription, enhancedSystemPrompt) and instead include only stable
identifiers (e.g., toolRecord.name or id) in the system prompt, or move the full
description into a lower-trust channel/message (e.g., a user/assistant/tool-data
message) that the model is explicitly told to treat as data, not instructions;
apply the same change to the other occurrences noted (around the blocks at lines
similar to 118-122 and 183-189).
---
Outside diff comments:
In `@mcpjam-inspector/server/routes/web/auth.ts`:
- Around line 147-178: The 400 WebRouteError thrown by the mutual-exclusivity
guard is being caught by the same try that wraps the fetch and turned into a
502; update the code so the guard for options?.shareToken and
options?.sandboxToken runs outside the try block (or, alternatively, catch and
rethrow WebRouteError unchanged) so that the explicit WebRouteError(400,
ErrorCode.VALIDATION_ERROR, ...) from that guard is preserved; locate the guard
and the surrounding try that calls fetch(`${convexUrl}/web/authorize`, ...) and
move the if (options?.shareToken && options?.sandboxToken) check before entering
that try (or add a catch that detects instance of WebRouteError and throws it
again) so the correct 400 response is returned.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsx`:
- Around line 359-366: The current polling loop in SandboxChatPage (for loop
awaiting 15×100ms and calling getStoredTokens(server.serverName) then
setOauthRefreshNonce) can miss later token arrivals; replace this fragile short
fixed-loop with either a storage-event based watcher that listens for window
"storage" changes for the key used by getStoredTokens(server.serverName) and
triggers setOauthRefreshNonce when access_token appears, or implement an
exponential-backoff retry (e.g., increase delay up to a longer cap and extend
total wait time) while still breaking when tokens?.access_token is found; update
the code around getStoredTokens and setOauthRefreshNonce to use the chosen
deterministic approach and remove the fixed 15-iteration loop.
- Around line 88-98: The config field currently uses an unsafe assertion ("as
any") which bypasses type checking for ServerWithName; replace it by defining a
minimal typed config (e.g., SandboxServerConfig or Partial of the real
ServerConfig) that includes url and any required keys, import or declare that
type in SandboxChatPage.tsx, then remove the "as any" and assign the object to
that typed config so the surrounding object literal (the ServerWithName entry)
can be validated by the compiler; update the object using the new type (or
Partial<ServerConfig>) and run TypeScript checks to ensure it satisfies
ServerWithName without using any casts.
In `@mcpjam-inspector/client/src/components/sandboxes/SandboxEditor.tsx`:
- Around line 147-172: The equality check in hasUnsavedChanges currently uses
JSON.stringify on formServerIds and currentServerIds which is brittle; replace
it with an explicit comparison that first checks lengths and then checks
elements pairwise (e.g., formServerIds.length === currentServerIds.length &&
formServerIds.every((id, i) => id === currentServerIds[i])). Update the return
condition inside the useMemo to use this explicit comparison of
selectedServerIds (formServerIds) and sandbox!.servers.map(...).sort()
(currentServerIds) instead of JSON.stringify, keeping the existing sort behavior
and hooks list unchanged.
In `@mcpjam-inspector/client/src/components/sandboxes/SandboxShareSection.tsx`:
- Around line 95-97: Summary: Avoid unsafe TypeScript casts like "as
SandboxSettings" on mutation results (seen in rotateSandboxLink and sibling
handlers handleModeChange, handleInvite, handleRemoveMember); instead validate
the shape or update the mutation hook's return type. Fix: update the mutation
hook signature to return Promise<SandboxSettings> (or provide a generic) so
callers get correct types, or add a runtime type guard (e.g.,
isSandboxSettings(result)) and only treat the value as SandboxSettings after the
guard passes; locate uses in rotateSandboxLink({ sandboxId: settings.sandboxId
}) and the other handlers and remove the "as SandboxSettings" casts, replacing
them with the guard check or relying on the corrected hook return type. Ensure
the guard verifies required fields of SandboxSettings before proceeding so
future API changes fail fast rather than being silently accepted.
- Around line 344-346: The current rendering in SandboxShareSection uses
buildSandboxLink("", settings.name) when settings.link?.url is missing,
producing an incomplete URL; update the JSX so that instead of calling
buildSandboxLink with an empty token you either hide the paragraph or display a
clear placeholder like "Link not yet generated" when settings.link?.url is
falsy. Locate the paragraph in SandboxShareSection that references
settings.link?.url and buildSandboxLink and change it to a conditional
expression: if settings.link?.url render the real URL (e.g., settings.link.url
or buildSandboxLink(token, settings.name)), otherwise render the placeholder
text (or skip rendering the <p> entirely) to avoid showing an incomplete link.
- Around line 132-134: handleInvite currently only checks for an empty input;
add client-side email format validation after computing normalizedEmail (in
function handleInvite) by testing normalizedEmail against a reliable email regex
or a validator utility and return early if it fails. On failure, set the
existing invite/error UI state (e.g., setInviteError or similar) with a
user-friendly message and avoid calling the invite API; keep the
trim().toLowerCase() normalization and only proceed to API calls when the
regex/validator accepts the address.
In `@mcpjam-inspector/client/src/hooks/useSandboxes.ts`:
- Around line 75-78: The call to useQuery in useSandboxes.ts is bypassing type
safety by casting the query key and args to any; replace the any casts by
importing and using the generated Convex types (or typings from
convex/_generated/api) for the query name and its argument/return types and pass
the correctly typed key/args instead of "sandboxes:listSandboxes" as any and ({
workspaceId } as any); update the sandboxes variable typing to use the
Convex-generated return type (or a locally defined interface/Zod-validated type)
so useQuery<ReturnType> and the query arguments use the proper typed shape, and
keep the conditional skip logic (isAuthenticated && workspaceId) but provide a
typed skip sentinel per your query hook contract.
In `@mcpjam-inspector/client/src/hooks/useSharedChatThreads.ts`:
- Around line 44-59: The code repeatedly uses unsafe type assertions ("as any")
around Convex queries (see queryName, queryArgs, and useQuery in
useSharedChatThreads.ts returning SharedChatThread[]), so replace these ad-hoc
casts with a small shared helper or typed wrapper (e.g., a typedUseQuery<T> or
buildQueryArgs utility) that centralizes Convex typing logic and returns the
correct generic type instead of forcing "any"; update useSharedChatThreads to
call that helper for both sandbox and share branches (removing "as any" on
queryName/queryArgs) and apply the same helper to other occurrences like
useProfilePicture and useServerShares to eliminate repeated unsafe assertions.
In `@mcpjam-inspector/client/src/lib/__tests__/hosted-web-context.test.ts`:
- Around line 64-75: Add a new unit test that covers the case where both an
access token and a share token are present to verify mutual exclusivity: call
setHostedApiContext with workspaceId and serverIdsByName and provide both
getAccessToken (returning a string) and getShareToken (returning a string), then
call buildHostedServerRequest("bench") and assert the actual behavior your code
should enforce (e.g., throws an error, prefers the share token and omits access
fields, or prefers access token and omits share fields). Reference
setHostedApiContext and buildHostedServerRequest in the test and assert the
expected shape or error to make the mutual-exclusivity contract explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f9c6f7a-0f78-4af8-96ba-96bebf9fb926
📒 Files selected for processing (39)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ChatTabV2.tsxmcpjam-inspector/client/src/components/SandboxesTab.tsxmcpjam-inspector/client/src/components/connection/AddServerModal.tsxmcpjam-inspector/client/src/components/connection/ShareServerDialog.tsxmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/connection/share-usage/ShareUsageDialog.tsxmcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadDetail.tsxmcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadList.tsxmcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsxmcpjam-inspector/client/src/components/mcp-sidebar.tsxmcpjam-inspector/client/src/components/sandboxes/CreateSandboxDialog.tsxmcpjam-inspector/client/src/components/sandboxes/SandboxEditor.tsxmcpjam-inspector/client/src/components/sandboxes/SandboxShareSection.tsxmcpjam-inspector/client/src/components/sandboxes/SandboxUsagePanel.tsxmcpjam-inspector/client/src/components/sandboxes/ShareSandboxDialog.tsxmcpjam-inspector/client/src/hooks/__tests__/use-chat-session.hosted.test.tsxmcpjam-inspector/client/src/hooks/__tests__/useSharedChatWidgetCapture.test.tsxmcpjam-inspector/client/src/hooks/hosted/use-hosted-api-context.tsmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/client/src/hooks/useSandboxes.tsmcpjam-inspector/client/src/hooks/useSharedChatThreads.tsmcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.tsmcpjam-inspector/client/src/hooks/useViews.tsmcpjam-inspector/client/src/lib/__tests__/hosted-navigation.test.tsmcpjam-inspector/client/src/lib/__tests__/hosted-tab-policy.test.tsmcpjam-inspector/client/src/lib/__tests__/hosted-web-context.test.tsmcpjam-inspector/client/src/lib/__tests__/sandbox-session.test.tsmcpjam-inspector/client/src/lib/apis/web/context.tsmcpjam-inspector/client/src/lib/hosted-tab-policy.tsmcpjam-inspector/client/src/lib/sandbox-session.tsmcpjam-inspector/server/routes/web/auth.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/routes/web/index.tsmcpjam-inspector/server/routes/web/sandboxes.tsmcpjam-inspector/server/routes/web/servers.tsmcpjam-inspector/server/utils/__tests__/chat-v2-orchestration.test.tsmcpjam-inspector/server/utils/chat-v2-orchestration.tsmcpjam-inspector/server/utils/shared-chat-persistence.ts
mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+93
to
+108
| const hostedModels = useMemo( | ||
| () => | ||
| SUPPORTED_MODELS.filter((model) => | ||
| isMCPJamProvidedModel(String(model.id)), | ||
| ), | ||
| [], | ||
| ); | ||
|
|
||
| const [name, setName] = useState(sandbox?.name ?? ""); | ||
| const [description, setDescription] = useState(sandbox?.description ?? ""); | ||
| const [systemPrompt, setSystemPrompt] = useState( | ||
| sandbox?.systemPrompt ?? DEFAULT_SYSTEM_PROMPT, | ||
| ); | ||
| const [modelId, setModelId] = useState( | ||
| sandbox?.modelId ?? hostedModels[0]?.id?.toString() ?? "openai/gpt-5-mini", | ||
| ); |
Contributor
There was a problem hiding this comment.
Consider guarding against empty hostedModels array.
If SUPPORTED_MODELS contains no MCP Jam-provided models, hostedModels[0]?.id returns undefined, and the fallback "openai/gpt-5-mini" may not exist in availableModels. This could leave the user with no valid selection.
🛡️ Proposed defensive check
const [modelId, setModelId] = useState(
- sandbox?.modelId ?? hostedModels[0]?.id?.toString() ?? "openai/gpt-5-mini",
+ sandbox?.modelId ?? (hostedModels.length > 0 ? String(hostedModels[0].id) : ""),
);Then validate modelId is non-empty in handleSave alongside other validations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/sandboxes/SandboxEditor.tsx` around
lines 93 - 108, The initializer for modelId in SandboxEditor uses
hostedModels[0]?.id with a fallback "openai/gpt-5-mini", which can still produce
an invalid/empty selection if hostedModels is empty or the fallback isn't
present in availableModels; update the logic that computes hostedModels (from
SUPPORTED_MODELS and isMCPJamProvidedModel) to handle an empty result (e.g.,
choose the first id from availableModels or set modelId to a safe non-empty
default), and add a validation in handleSave to ensure modelId is non-empty and
exists in availableModels before saving (returning or showing an error if
invalid) so users cannot save a sandbox with an invalid modelId.
Comment on lines
+263
to
+270
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="size-7 opacity-0 group-hover:opacity-100" | ||
| onClick={() => void handleRemoveMember(member)} | ||
| > | ||
| <X className="size-3.5" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Keyboard users cannot discover the hidden remove button.
The button's visibility hinges solely on :hover (opacity-0 group-hover:opacity-100). Sighted keyboard users navigating via Tab will encounter an invisible control. Adding group-focus-within:opacity-100 ensures the button reveals when focus enters the row.
♿ Proposed accessibility fix
<Button
variant="ghost"
size="icon"
- className="size-7 opacity-0 group-hover:opacity-100"
+ className="size-7 opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 focus:opacity-100"
onClick={() => void handleRemoveMember(member)}
>📝 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
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="size-7 opacity-0 group-hover:opacity-100" | |
| onClick={() => void handleRemoveMember(member)} | |
| > | |
| <X className="size-3.5" /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="size-7 opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 focus:opacity-100" | |
| onClick={() => void handleRemoveMember(member)} | |
| > | |
| <X className="size-3.5" /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/sandboxes/SandboxShareSection.tsx`
around lines 263 - 270, The remove button is only revealed on hover, making it
inaccessible to keyboard users; update the Button rendering in
SandboxShareSection to include focus-based reveal classes so keyboard focus
shows the control—specifically, modify the Button's className (currently "size-7
opacity-0 group-hover:opacity-100") to also include
"group-focus-within:opacity-100" (and optionally "group-focus:opacity-100") so
that when the row receives keyboard focus the remove Button (handled by
handleRemoveMember) becomes visible and operable.
Comment on lines
+73
to
+76
| const activeMembers = useMemo( | ||
| () => settings.members.filter((member) => !member.revokedAt), | ||
| [settings.members], | ||
| ); |
Contributor
There was a problem hiding this comment.
Guard against undefined members array.
If settings.members is undefined (e.g., during initial load or malformed data), filter would throw. Consider defensive access.
🛡️ Proposed fix
const activeMembers = useMemo(
- () => settings.members.filter((member) => !member.revokedAt),
+ () => (settings.members ?? []).filter((member) => !member.revokedAt),
[settings.members],
);📝 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 activeMembers = useMemo( | |
| () => settings.members.filter((member) => !member.revokedAt), | |
| [settings.members], | |
| ); | |
| const activeMembers = useMemo( | |
| () => (settings.members ?? []).filter((member) => !member.revokedAt), | |
| [settings.members], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/sandboxes/ShareSandboxDialog.tsx`
around lines 73 - 76, The useMemo for activeMembers assumes settings.members is
always an array and will throw if undefined; update the computation in
ShareSandboxDialog (activeMembers/useMemo) to defensively handle missing members
by using a fallback (e.g., (settings.members || []) or optional chaining) before
calling filter, and ensure the dependency array still references
settings.members (or settings) appropriately so memoization remains correct.
Comment on lines
+204
to
+212
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-7 gap-1.5 px-2 text-xs" | ||
| onClick={() => setRightPaneView("edit")} | ||
| > | ||
| <Share2 className="h-3 w-3" /> | ||
| Share | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Share button invokes the wrong handler.
The Share button currently triggers setRightPaneView("edit"), identical to the Edit button. This leaves the ShareSandboxDialog inaccessible despite being wired at lines 277-286.
🔧 Proposed fix
<Button
variant="ghost"
size="sm"
className="h-7 gap-1.5 px-2 text-xs"
- onClick={() => setRightPaneView("edit")}
+ onClick={() => setIsShareOpen(true)}
>
<Share2 className="h-3 w-3" />
Share
</Button>📝 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
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="h-7 gap-1.5 px-2 text-xs" | |
| onClick={() => setRightPaneView("edit")} | |
| > | |
| <Share2 className="h-3 w-3" /> | |
| Share | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="h-7 gap-1.5 px-2 text-xs" | |
| onClick={() => setIsShareOpen(true)} | |
| > | |
| <Share2 className="h-3 w-3" /> | |
| Share | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/SandboxesTab.tsx` around lines 204 -
212, The Share button currently calls setRightPaneView("edit") so it opens the
Edit view instead of the share dialog; change the onClick of the Share Button
(the Button rendering the Share2 icon) to call setRightPaneView("share") (or the
exact rightPaneView value used to render ShareSandboxDialog) so that selecting
Share opens ShareSandboxDialog; update any related conditional that checks
rightPaneView in the component to ensure "share" maps to ShareSandboxDialog.
| temperature, | ||
| requireToolApproval, | ||
| customProviders: body.customProviders, | ||
| includeMcpToolInventory: true, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what includeMcpToolInventory does in prepareChatV2
ast-grep --pattern 'function prepareChatV2($_) {
$$$
}'Repository: MCPJam/inspector
Length of output: 42
🏁 Script executed:
# Read the chat-v2.ts file to see context around line 96
cat -n mcpjam-inspector/server/routes/web/chat-v2.ts | head -120 | tail -40Repository: MCPJam/inspector
Length of output: 1585
🏁 Script executed:
# Search for prepareChatV2 function definition
rg "prepareChatV2" -A 20 -B 2Repository: MCPJam/inspector
Length of output: 18718
🏁 Script executed:
# Search for includeMcpToolInventory usage throughout the codebase
rg "includeMcpToolInventory" -B 3 -A 3Repository: MCPJam/inspector
Length of output: 3125
🏁 Script executed:
# Find and read buildMcpToolInventoryPrompt implementation
rg "buildMcpToolInventoryPrompt" -A 15Repository: MCPJam/inspector
Length of output: 2821
🏁 Script executed:
# Check if there are any comments explaining route differences
grep -B 5 -A 5 "includeMcpToolInventory" mcpjam-inspector/server/routes/mcp/chat-v2.ts | head -30Repository: MCPJam/inspector
Length of output: 42
Confirm tool inventory inclusion is intentional for all web-hosted requests.
The includeMcpToolInventory parameter is unconditionally set to true in the web route, whereas the MCP route omits it entirely. Enabling it adds tool descriptions to the system prompt, increasing payload size. Verify this asymmetry and payload expansion is acceptable for your hosted chat-v2 deployment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/web/chat-v2.ts` at line 96, The web route
currently hard-codes includeMcpToolInventory: true which differs from the MCP
route (omitted) and increases prompt payload; decide whether tool inventory
should be included for web-hosted requests and then either (a) remove the
hard-coded includeMcpToolInventory flag from the web route to match the MCP
behavior, (b) set it to false, or (c) make it configurable (e.g., use a feature
flag or env var) and default to the MCP behavior; update the code path that
constructs the request (look for the web chat-v2 route handler where
includeMcpToolInventory is set) and any relevant tests/docs to reflect the
chosen behavior so web and MCP routes are consistent or intentionally documented
as different.
Comment on lines
+21
to
+28
| const convexUrl = process.env.CONVEX_HTTP_URL; | ||
| if (!convexUrl) { | ||
| throw new WebRouteError( | ||
| 500, | ||
| ErrorCode.INTERNAL_ERROR, | ||
| "Server missing CONVEX_HTTP_URL configuration", | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce HTTPS before forwarding bearer tokens.
This route will happily send the caller's Authorization header to any CONVEX_HTTP_URL, including http://... if the env is misconfigured. That weakens the HTTPS-only posture introduced elsewhere and turns a config mistake into credential leakage. Reject non-HTTPS URLs here, or explicitly scope any exception to local development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/routes/web/sandboxes.ts` around lines 21 - 28,
Validate the convexUrl scheme before forwarding bearer tokens: parse convexUrl
(e.g., new URL(convexUrl)) and if its protocol is not 'https:' (allowing an
explicit dev exception for 'http://' to localhost/127.0.0.1 if desired) throw
the existing WebRouteError with ErrorCode.INTERNAL_ERROR and a clear message
about rejecting non-HTTPS CONVEX_HTTP_URL; update the convexUrl check block (the
code that currently throws when !convexUrl) to perform this protocol validation
and only accept secure endpoints.
Comment on lines
+72
to
+88
| it("does not add MCP tool inventory unless requested", async () => { | ||
| const manager = mockManager({ | ||
| fetch_tasks: { | ||
| description: "Fetch tasks from the task service", | ||
| _serverId: "server-b", | ||
| }, | ||
| }); | ||
|
|
||
| const result = await prepareChatV2({ | ||
| mcpClientManager: manager, | ||
| selectedServers: ["server-b"], | ||
| modelDefinition: { id: "gpt-4.1", provider: "openai" } as any, | ||
| systemPrompt: "Base prompt.", | ||
| }); | ||
|
|
||
| expect(result.enhancedSystemPrompt).toBe("Base prompt."); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert that inventory lookup is skipped when the flag is off.
This test only checks the final prompt. prepareChatV2 could still call getToolsForAiSdk unnecessarily and the test would remain green, masking extra work on the non-inventory path.
Suggested test hardening
it("does not add MCP tool inventory unless requested", async () => {
const manager = mockManager({
fetch_tasks: {
description: "Fetch tasks from the task service",
_serverId: "server-b",
},
});
const result = await prepareChatV2({
mcpClientManager: manager,
selectedServers: ["server-b"],
modelDefinition: { id: "gpt-4.1", provider: "openai" } as any,
systemPrompt: "Base prompt.",
});
expect(result.enhancedSystemPrompt).toBe("Base prompt.");
+ expect(manager.getToolsForAiSdk).not.toHaveBeenCalled();
});📝 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
| it("does not add MCP tool inventory unless requested", async () => { | |
| const manager = mockManager({ | |
| fetch_tasks: { | |
| description: "Fetch tasks from the task service", | |
| _serverId: "server-b", | |
| }, | |
| }); | |
| const result = await prepareChatV2({ | |
| mcpClientManager: manager, | |
| selectedServers: ["server-b"], | |
| modelDefinition: { id: "gpt-4.1", provider: "openai" } as any, | |
| systemPrompt: "Base prompt.", | |
| }); | |
| expect(result.enhancedSystemPrompt).toBe("Base prompt."); | |
| }); | |
| it("does not add MCP tool inventory unless requested", async () => { | |
| const manager = mockManager({ | |
| fetch_tasks: { | |
| description: "Fetch tasks from the task service", | |
| _serverId: "server-b", | |
| }, | |
| }); | |
| const result = await prepareChatV2({ | |
| mcpClientManager: manager, | |
| selectedServers: ["server-b"], | |
| modelDefinition: { id: "gpt-4.1", provider: "openai" } as any, | |
| systemPrompt: "Base prompt.", | |
| }); | |
| expect(result.enhancedSystemPrompt).toBe("Base prompt."); | |
| expect(manager.getToolsForAiSdk).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/utils/__tests__/chat-v2-orchestration.test.ts` around
lines 72 - 88, The test must assert that inventory lookup is skipped by
verifying getToolsForAiSdk (or the method on the mcpClientManager that performs
tool lookup) is not called when the MCP inventory flag is off; update the test
for prepareChatV2 to spy/mock the getToolsForAiSdk function (or the manager
method used in prepareChatV2) and add an expectation that the spy was not
invoked, while keeping the existing assertion that enhancedSystemPrompt equals
"Base prompt."; reference prepareChatV2 and getToolsForAiSdk (or the
mcpClientManager method) to locate where to attach the spy/mock.
Comment on lines
+87
to
+90
| description: | ||
| typeof toolRecord.description === "string" | ||
| ? truncateToolDescription(toolRecord.description) | ||
| : undefined, |
Contributor
There was a problem hiding this comment.
Keep server-controlled descriptions out of the system prompt.
toolRecord.description comes from the connected MCP server, then gets spliced into enhancedSystemPrompt. That lets an untrusted server inject instructions at system priority; truncation and whitespace folding do not mitigate that. Safer options are to list only stable identifiers here, or move descriptions into a lower-trust channel that the model is told to treat as data, not instructions.
🔒 Suggested hardening
interface MCPToolPromptEntry {
name: string;
- description?: string;
}
@@
existing.push({
name,
- description:
- typeof toolRecord.description === "string"
- ? truncateToolDescription(toolRecord.description)
- : undefined,
});
@@
- .map(({ name, description }) =>
- description ? `- ${name}: ${description}` : `- ${name}`,
- )
+ .map(({ name }) => `- ${name}`)
.join("\n");Also applies to: 118-122, 183-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/utils/chat-v2-orchestration.ts` around lines 87 - 90,
The code is inserting untrusted toolRecord.description into enhancedSystemPrompt
(via truncateToolDescription), which lets external servers inject system-level
instructions; change this by removing description from any system prompt
assembly (references: toolRecord.description, truncateToolDescription,
enhancedSystemPrompt) and instead include only stable identifiers (e.g.,
toolRecord.name or id) in the system prompt, or move the full description into a
lower-trust channel/message (e.g., a user/assistant/tool-data message) that the
model is explicitly told to treat as data, not instructions; apply the same
change to the other occurrences noted (around the blocks at lines similar to
118-122 and 183-189).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
mcpjam-inspector/server/routes/web/sandboxes.ts (1)
21-28:⚠️ Potential issue | 🟠 MajorHTTPS validation remains unaddressed for credential-bearing requests.
The prior observation stands:
CONVEX_HTTP_URLis trusted blindly. A misconfiguredhttp://endpoint would leak the bearer token in plaintext.🔒 Proposed guard
const convexUrl = process.env.CONVEX_HTTP_URL; if (!convexUrl) { throw new WebRouteError( 500, ErrorCode.INTERNAL_ERROR, "Server missing CONVEX_HTTP_URL configuration", ); } + const parsedUrl = new URL(convexUrl); + const isLocalDev = + parsedUrl.hostname === "localhost" || + parsedUrl.hostname === "127.0.0.1"; + if (parsedUrl.protocol !== "https:" && !isLocalDev) { + throw new WebRouteError( + 500, + ErrorCode.INTERNAL_ERROR, + "CONVEX_HTTP_URL must use HTTPS in production", + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/sandboxes.ts` around lines 21 - 28, convexUrl is accepted without validating its scheme, which risks leaking bearer tokens over plain HTTP; parse convexUrl (the variable) with the URL constructor and enforce that url.protocol === "https:" (allowing "http:" only for loopback/localhost if you have to), and if the check fails throw a WebRouteError (use the existing WebRouteError/ErrorCode.INTERNAL_ERROR pattern) with a clear message requiring an https CONVEX_HTTP_URL so credential-bearing requests are not sent in plaintext.
🧹 Nitpick comments (5)
mcpjam-inspector/.env.local (1)
1-7: Reorder the env keys to silencedotenv-linter.The updated entries now trigger
UnorderedKeywarnings on Lines 1, 2, 5, 6, and 7. Keeping this file in the linter’s expected order will avoid recurring noise in CI and follow-up diffs.Suggested reorder
-VITE_CONVEX_URL=https://tough-cassowary-291.convex.cloud -CONVEX_URL=https://tough-cassowary-291.convex.cloud -VITE_WORKOS_CLIENT_ID=client_01K4C1TVA6CMQ3G32F1P301A9G -VITE_WORKOS_REDIRECT_URI=mcpjam://oauth/callback -CONVEX_HTTP_URL=https://tough-cassowary-291.convex.site -ENVIRONMENT=local -VITE_DISABLE_POSTHOG_LOCAL=true +CONVEX_HTTP_URL=https://tough-cassowary-291.convex.site +CONVEX_URL=https://tough-cassowary-291.convex.cloud +ENVIRONMENT=local +VITE_CONVEX_URL=https://tough-cassowary-291.convex.cloud +VITE_DISABLE_POSTHOG_LOCAL=true +VITE_WORKOS_CLIENT_ID=client_01K4C1TVA6CMQ3G32F1P301A9G +VITE_WORKOS_REDIRECT_URI=mcpjam://oauth/callback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/.env.local` around lines 1 - 7, dotenv-linter reports UnorderedKey because the environment variables are not in alphabetical order; reorder the entries so the keys are alphabetically sorted (e.g., CONVEX_HTTP_URL, CONVEX_URL, ENVIRONMENT, VITE_CONVEX_URL, VITE_DISABLE_POSTHOG_LOCAL, VITE_WORKOS_CLIENT_ID, VITE_WORKOS_REDIRECT_URI) to satisfy the linter; update the block containing VITE_CONVEX_URL, CONVEX_URL, CONVEX_HTTP_URL, ENVIRONMENT, VITE_DISABLE_POSTHOG_LOCAL, VITE_WORKOS_CLIENT_ID, and VITE_WORKOS_REDIRECT_URI accordingly.mcpjam-inspector/server/routes/web/sandboxes.ts (1)
68-73: Untypedanyobscures the expected shape of upstream responses.Defining a minimal interface sharpens intent and lets TypeScript assist with the subsequent property accesses (
payload.ok,payload.error, etc.).📐 Type refinement
+interface BootstrapUpstreamResponse { + ok?: boolean; + payload?: unknown; + error?: string; + message?: string; +} + - let payload: any = null; + let payload: BootstrapUpstreamResponse | null = null; try { - payload = trimmedResponseText ? JSON.parse(trimmedResponseText) : null; + payload = trimmedResponseText + ? (JSON.parse(trimmedResponseText) as BootstrapUpstreamResponse) + : null; } catch { // ignored }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/sandboxes.ts` around lines 68 - 73, Replace the untyped payload: any with a narrow interface describing the expected upstream shape (e.g. fields you read like ok, error, status, data) and use that interface for the payload variable in the sandboxes.ts parsing block; parse into unknown inside the try/catch, then assert/validate shape before assigning to payload (or use a type guard) so subsequent accesses like payload.ok and payload.error are type-checked against the new interface.mcpjam-inspector/server/routes/web/__tests__/sandboxes.test.ts (1)
1-83: Consider broadening test coverage when time permits.The two scenarios here are valuable, yet the route also handles:
- Successful bootstrap (200 with
payload.ok/payload.payload)- Missing
CONVEX_HTTP_URL(500)- Upstream 401/403 responses
Adding these would bolster regression protection, though the current tests suffice for the critical failure modes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/routes/web/__tests__/sandboxes.test.ts` around lines 1 - 83, Add unit tests to broaden coverage in sandboxes.test.ts: create a "successful bootstrap" test that stubs global fetch to return a 200 JSON body matching the upstream payload shape and assert postJson(..."/api/web/sandboxes/bootstrap"...) yields status 200 and data.payload.ok / data.payload.payload as expected (use createWebTestApp, postJson, expectJson helpers); add a "missing CONVEX_HTTP_URL" test that clears process.env.CONVEX_HTTP_URL before calling the route and asserts a 500 response; and add tests for upstream 401 and 403 by stubbing fetch to return those status codes and asserting the route maps them to the expected client-facing codes/messages. Ensure each test restores env and uses vi.stubGlobal("fetch")/vi.unstubAllGlobals() like the existing tests.mcpjam-inspector/client/src/components/SandboxesTab.tsx (2)
271-277: Create view blocked while servers load.If
serversis undefined (still loading), clicking "New" falls through to subsequent conditions, potentially showing an unrelated state. Consider rendering a dedicated loader whenrightPaneView === "create"and servers are pending.♻️ Proposed enhancement
{rightPaneView === "create" && servers ? ( <SandboxEditor workspaceId={workspaceId} workspaceServers={servers} onBack={() => setRightPaneView("usage")} onSaved={handleCreated} /> + ) : rightPaneView === "create" && !servers ? ( + <div className="flex h-full items-center justify-center"> + <Loader2 className="h-5 w-5 animate-spin text-muted-foreground" /> + </div> ) : !selectedSandboxId ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/SandboxesTab.tsx` around lines 271 - 277, The create pane can fall through when servers are still loading; update the SandboxesTab render logic so when rightPaneView === "create" you first check for servers and render a dedicated loading state (e.g., spinner or "Loading servers...") while servers is undefined, and only render <SandboxEditor workspaceId={workspaceId} workspaceServers={servers} ... /> once servers is present; modify the conditional that currently reads {rightPaneView === "create" && servers ? (...) } to explicitly handle the pending case and return a loader UI when servers is falsy so the "create" view never shows unrelated content while servers load.
182-198: "Try it" button requires two clicks for non-selected sandboxes.When clicked on an unselected sandbox, the button merely selects it rather than opening the link. Consider fetching the link for the clicked sandbox directly, or clarify the UX with a tooltip indicating selection is required first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/SandboxesTab.tsx` around lines 182 - 198, The button currently only opens a link when the row is already selected (using selectedSandbox); update the onClick to attempt to open the clicked sandbox's link directly: check sandbox.link?.token (from the clicked sandbox object) and if present call buildSandboxLink(sandbox.link.token, sandbox.name) and window.open it, otherwise fall back to calling handleSelectSandbox(sandbox.sandboxId) to select the row; keep e.stopPropagation() as-is and preserve behavior for already-selected sandboxes by also checking isSelected.
🤖 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/connection/hooks/use-server-form.ts`:
- Around line 7-10: EditServerModal and several AddServerModal usages fail to
enforce HTTPS because they call useServerForm(server) without forwarding the
requireHttps option; update call sites (EditServerModal, ServersTab,
ActiveServerSelector) to pass requireHttps (e.g., { requireHttps: true } or
derive from HOSTED_MODE) into useServerForm and AddServerModal props, and ensure
useServerForm(server, options) is respected; also mirror SandboxEditor behavior
(which already passes requireHttps) so all add/edit server flows consistently
enforce HTTPS in non-hosted environments.
In `@mcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsx`:
- Around line 344-361: The SANDBOX_OAUTH_PENDING_KEY flag is left set if
initiateOAuth returns without redirecting or throws, so wrap the initiateOAuth
call in a try/catch/finally in SandboxChatPage.tsx (around the existing
localStorage.setItem and the await initiateOAuth(...) call) and ensure
localStorage.removeItem(SANDBOX_OAUTH_PENDING_KEY) is executed when
initiateOAuth fails or returns !result.success; specifically remove the key
inside the !result.success branch and in the catch (or finally) so aborted or
thrown OAuth attempts cannot leave a stale pending flag that breaks later
callback handling.
- Around line 260-267: The early-return branch that handles no OAuth servers
(where oauthServers is computed and you call setOauthRequiredServerIds([]) and
setOauthPreflightError(null)) fails to clear the loading flag; add a call to
setIsCheckingOAuth(false) in that branch so the spinner/loading state is reset
when there are no OAuth servers; update the SandboxChatPage component's
no-server fast path (after computing oauthServers) to call
setIsCheckingOAuth(false) before returning.
In `@mcpjam-inspector/client/src/components/SandboxesTab.tsx`:
- Around line 227-237: The dropdown Delete handler currently calls
handleSelectSandbox(sandbox.sandboxId) then invokes handleDelete(), which races
against React state updates and can delete the previously selected sandbox;
change handleDelete to accept the sandbox (or sandboxId) directly (e.g.,
handleDelete(sandboxId): Promise<void>) and update the DropdownMenuItem onClick
to stopPropagation and call handleDelete(sandbox.sandboxId) (remove the
dependency on handleSelectSandbox for deletion). Update the handleDelete
implementation (and its callers) to use the passed-in id/object instead of
reading selectedSandbox so deletion always targets the intended sandbox.
In `@mcpjam-inspector/server/env.ts`:
- Around line 58-68: loadInspectorEnv currently calls dotenv.config per-file
which mutates process.env and respects existing keys, so when both
mcpjam-inspector/server/app.ts and server/index.ts call loadInspectorEnv the
second call can silently skip overwriting keys; change loadInspectorEnv to
either memoize its result (cache and return the same LoadedInspectorEnv on
subsequent calls) or read/merge all env files into a single plain object and
perform a single assignment to process.env once at startup, and update call
sites (app.ts and index.ts) to use the cached/merged value instead of invoking
dotenv.config multiple times; reference loadInspectorEnv, dotenv.config,
getInspectorEnvFileNames and the call sites in app.ts/index.ts when making this
change.
In `@mcpjam-inspector/server/index.ts`:
- Around line 10-15: The file is missing the join import from the path module
used later; update the import line that currently imports dirname to also import
join (i.e., add join to the destructured import from "path") so the join(...)
calls in this module resolve; locate the import statement that includes dirname
and add join alongside it to fix the runtime error when join is referenced.
---
Duplicate comments:
In `@mcpjam-inspector/server/routes/web/sandboxes.ts`:
- Around line 21-28: convexUrl is accepted without validating its scheme, which
risks leaking bearer tokens over plain HTTP; parse convexUrl (the variable) with
the URL constructor and enforce that url.protocol === "https:" (allowing "http:"
only for loopback/localhost if you have to), and if the check fails throw a
WebRouteError (use the existing WebRouteError/ErrorCode.INTERNAL_ERROR pattern)
with a clear message requiring an https CONVEX_HTTP_URL so credential-bearing
requests are not sent in plaintext.
---
Nitpick comments:
In `@mcpjam-inspector/.env.local`:
- Around line 1-7: dotenv-linter reports UnorderedKey because the environment
variables are not in alphabetical order; reorder the entries so the keys are
alphabetically sorted (e.g., CONVEX_HTTP_URL, CONVEX_URL, ENVIRONMENT,
VITE_CONVEX_URL, VITE_DISABLE_POSTHOG_LOCAL, VITE_WORKOS_CLIENT_ID,
VITE_WORKOS_REDIRECT_URI) to satisfy the linter; update the block containing
VITE_CONVEX_URL, CONVEX_URL, CONVEX_HTTP_URL, ENVIRONMENT,
VITE_DISABLE_POSTHOG_LOCAL, VITE_WORKOS_CLIENT_ID, and VITE_WORKOS_REDIRECT_URI
accordingly.
In `@mcpjam-inspector/client/src/components/SandboxesTab.tsx`:
- Around line 271-277: The create pane can fall through when servers are still
loading; update the SandboxesTab render logic so when rightPaneView === "create"
you first check for servers and render a dedicated loading state (e.g., spinner
or "Loading servers...") while servers is undefined, and only render
<SandboxEditor workspaceId={workspaceId} workspaceServers={servers} ... /> once
servers is present; modify the conditional that currently reads {rightPaneView
=== "create" && servers ? (...) } to explicitly handle the pending case and
return a loader UI when servers is falsy so the "create" view never shows
unrelated content while servers load.
- Around line 182-198: The button currently only opens a link when the row is
already selected (using selectedSandbox); update the onClick to attempt to open
the clicked sandbox's link directly: check sandbox.link?.token (from the clicked
sandbox object) and if present call buildSandboxLink(sandbox.link.token,
sandbox.name) and window.open it, otherwise fall back to calling
handleSelectSandbox(sandbox.sandboxId) to select the row; keep
e.stopPropagation() as-is and preserve behavior for already-selected sandboxes
by also checking isSelected.
In `@mcpjam-inspector/server/routes/web/__tests__/sandboxes.test.ts`:
- Around line 1-83: Add unit tests to broaden coverage in sandboxes.test.ts:
create a "successful bootstrap" test that stubs global fetch to return a 200
JSON body matching the upstream payload shape and assert
postJson(..."/api/web/sandboxes/bootstrap"...) yields status 200 and
data.payload.ok / data.payload.payload as expected (use createWebTestApp,
postJson, expectJson helpers); add a "missing CONVEX_HTTP_URL" test that clears
process.env.CONVEX_HTTP_URL before calling the route and asserts a 500 response;
and add tests for upstream 401 and 403 by stubbing fetch to return those status
codes and asserting the route maps them to the expected client-facing
codes/messages. Ensure each test restores env and uses
vi.stubGlobal("fetch")/vi.unstubAllGlobals() like the existing tests.
In `@mcpjam-inspector/server/routes/web/sandboxes.ts`:
- Around line 68-73: Replace the untyped payload: any with a narrow interface
describing the expected upstream shape (e.g. fields you read like ok, error,
status, data) and use that interface for the payload variable in the
sandboxes.ts parsing block; parse into unknown inside the try/catch, then
assert/validate shape before assigning to payload (or use a type guard) so
subsequent accesses like payload.ok and payload.error are type-checked against
the new interface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0dd0fcf-1a9b-41c3-be8c-a69f1bd23a40
📒 Files selected for processing (17)
mcpjam-inspector/.env.localmcpjam-inspector/client/src/components/SandboxesTab.tsxmcpjam-inspector/client/src/components/connection/ShareServerDialog.tsxmcpjam-inspector/client/src/components/connection/hooks/__tests__/use-server-form.test.tsmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsxmcpjam-inspector/client/src/components/hosted/SharedServerChatPage.tsxmcpjam-inspector/client/src/lib/__tests__/sandbox-session.test.tsmcpjam-inspector/client/src/lib/__tests__/shared-server-session.test.tsmcpjam-inspector/client/src/lib/sandbox-session.tsmcpjam-inspector/client/src/lib/shared-server-session.tsmcpjam-inspector/server/__tests__/env.test.tsmcpjam-inspector/server/app.tsmcpjam-inspector/server/env.tsmcpjam-inspector/server/index.tsmcpjam-inspector/server/routes/web/__tests__/sandboxes.test.tsmcpjam-inspector/server/routes/web/sandboxes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/components/connection/ShareServerDialog.tsx
Comment on lines
+7
to
+10
| export function useServerForm( | ||
| server?: ServerWithName, | ||
| options?: { requireHttps?: boolean }, | ||
| ) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which callers of useServerForm or AddServerModal pass requireHttps
echo "=== useServerForm callers ==="
rg -n 'useServerForm\(' --type ts --type tsx -g '!*.test.*' | head -30
echo -e "\n=== AddServerModal usages ==="
rg -n '<AddServerModal' --type tsx -g '!*.test.*' | head -20
echo -e "\n=== EditServerModal usages ==="
rg -n '<EditServerModal' --type tsx -g '!*.test.*' | head -20Repository: MCPJam/inspector
Length of output: 253
🏁 Script executed:
#!/bin/bash
# Search for useServerForm callers - avoid specifying unsupported file types
echo "=== useServerForm callers ==="
rg -n 'useServerForm\(' --glob='!*.test.*' mcpjam-inspector/client/src | head -40
echo -e "\n=== AddServerModal usages ==="
rg -n '<AddServerModal' --glob='!*.test.*' mcpjam-inspector/client/src | head -30
echo -e "\n=== EditServerModal usages ==="
rg -n '<EditServerModal' --glob='!*.test.*' mcpjam-inspector/client/src | head -30
echo -e "\n=== Check if HOSTED_MODE is used for requireHttps fallback ==="
rg -n 'requireHttps' --glob='!*.test.*' mcpjam-inspector/client/src -A 3 -B 3 | head -50Repository: MCPJam/inspector
Length of output: 4789
🏁 Script executed:
#!/bin/bash
echo "=== ServersTab AddServerModal usage (around line 534) ==="
sed -n '530,545p' mcpjam-inspector/client/src/components/ServersTab.tsx
echo -e "\n=== ActiveServerSelector AddServerModal usage (around line 334) ==="
sed -n '330,345p' mcpjam-inspector/client/src/components/ActiveServerSelector.tsx
echo -e "\n=== EditServerModal implementation ==="
sed -n '1,60p' mcpjam-inspector/client/src/components/connection/EditServerModal.tsx | tail -40Repository: MCPJam/inspector
Length of output: 2395
EditServerModal and multiple AddServerModal usages fail to enforce HTTPS in non-hosted environments.
EditServerModal calls useServerForm(server) without passing any requireHttps option. Similarly, AddServerModal is used in ServersTab and ActiveServerSelector without the requireHttps prop. While HOSTED_MODE provides a fallback in hosted deployments, this leaves these components permitting HTTP URLs in non-hosted environments—including sandbox flows that require HTTPS for security.
Only SandboxEditor correctly passes requireHttps to AddServerModal. EditServerModal needs an equivalent mechanism, and the other usages should either pass requireHttps explicitly or remove the ability to edit/add servers in contexts requiring HTTPS.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`
around lines 7 - 10, EditServerModal and several AddServerModal usages fail to
enforce HTTPS because they call useServerForm(server) without forwarding the
requireHttps option; update call sites (EditServerModal, ServersTab,
ActiveServerSelector) to pass requireHttps (e.g., { requireHttps: true } or
derive from HOSTED_MODE) into useServerForm and AddServerModal props, and ensure
useServerForm(server, options) is respected; also mirror SandboxEditor behavior
(which already passes requireHttps) so all add/edit server flows consistently
enforce HTTPS in non-hosted environments.
mcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsx
Outdated
Show resolved
Hide resolved
mcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+58
to
+68
| export function loadInspectorEnv(serverDir: string): LoadedInspectorEnv { | ||
| const mode = getInspectorEnvMode(); | ||
| const envDir = resolveInspectorEnvDir(serverDir); | ||
| const loadedFiles: string[] = []; | ||
|
|
||
| for (const fileName of getInspectorEnvFileNames(mode)) { | ||
| const envPath = join(envDir, fileName); | ||
| if (!existsSync(envPath)) continue; | ||
|
|
||
| dotenv.config({ path: envPath }); | ||
| loadedFiles.push(envPath); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
printf '%s\n' '-- loader implementation --'
sed -n '58,70p' mcpjam-inspector/server/env.ts
printf '\n%s\n' '-- loadInspectorEnv call sites --'
rg -n '\bloadInspectorEnv\s*\(' mcpjam-inspector/serverRepository: MCPJam/inspector
Length of output: 918
🌐 Web query:
In dotenv 16.x, does dotenv.config({ path }) overwrite keys that already exist on process.env by default?
💡 Result:
No. In dotenv@16.x, dotenv.config({ path }) does not overwrite keys that already exist on process.env by default. Overwriting only happens if you pass override: true (default is false). [1]
Example: dotenv.config({ path: '.env', override: true }) [1]
Sources:
[1] (npmjs.com)
Citations:
Consolidate loadInspectorEnv() calls to prevent stale environment variables.
This function mutates process.env via dotenv.config({ path }), which respects existing keys and never overwrites them by default. With call sites in both mcpjam-inspector/server/app.ts (line 44) and mcpjam-inspector/server/index.ts (line 208), a process executing both paths will apply only the first loader's variables; the second load silently skips any keys already present, potentially locking the server to the wrong Convex deployment.
Either memoize the result of the first call, or merge all env files into a single object before assigning once at startup from a single bootstrap path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/env.ts` around lines 58 - 68, loadInspectorEnv
currently calls dotenv.config per-file which mutates process.env and respects
existing keys, so when both mcpjam-inspector/server/app.ts and server/index.ts
call loadInspectorEnv the second call can silently skip overwriting keys; change
loadInspectorEnv to either memoize its result (cache and return the same
LoadedInspectorEnv on subsequent calls) or read/merge all env files into a
single plain object and perform a single assignment to process.env once at
startup, and update call sites (app.ts and index.ts) to use the cached/merged
value instead of invoking dotenv.config multiple times; reference
loadInspectorEnv, dotenv.config, getInspectorEnvFileNames and the call sites in
app.ts/index.ts when making this change.
Comment on lines
+10
to
15
| import { readFileSync } from "fs"; | ||
| import { dirname } from "path"; | ||
| import { fileURLToPath } from "url"; | ||
| import { MCPClientManager } from "@mcpjam/sdk"; | ||
| import { loadInspectorEnv, warnOnConvexDevMisconfiguration } from "./env"; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,20p' mcpjam-inspector/server/index.ts
printf '\n-- join call sites --\n'
rg -n '\bjoin\s*\(' mcpjam-inspector/server/index.tsRepository: MCPJam/inspector
Length of output: 888
Restore the missing join import.
Line 369 uses join(...) from the "path" module, but it is not imported at line 12.
Proposed fix
-import { dirname } from "path";
+import { dirname, join } from "path";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/index.ts` around lines 10 - 15, The file is missing
the join import from the path module used later; update the import line that
currently imports dirname to also import join (i.e., add join to the
destructured import from "path") so the join(...) calls in this module resolve;
locate the import statement that includes dirname and add join alongside it to
fix the runtime error when join is referenced.
![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 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| useEffect(() => { | ||
| setIsExpanded(displayMode !== "collapsed"); | ||
| }, [displayMode, text]); |
There was a problem hiding this comment.
Reasoning expansion state resets on every text change
Low Severity
The useEffect that resets isExpanded includes text in its dependency array. This means any change to text (e.g., during streaming) resets the user's manual expand/collapse action. If a user collapses a "collapsible" section, the next text update re-expands it; if they expand a "collapsed" section, it re-collapses. The text dependency appears unintentional — only displayMode changes warrant resetting the expansion state.
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 hosted sandbox chat surface and refactors hosted OAuth callback handling, affecting navigation/session restoration and OAuth completion paths. Risk is mainly around hosted routing/OAuth edge cases and new sandbox bootstrap flows interacting with storage and auth.
Overview
Adds hosted Sandboxes support. Introduces a new
Sandboxestab for authenticated users to list/create/edit/duplicate/delete sandboxes and copy/open sandbox share links.Enables sandbox share-link chat in hosted mode. Adds
SandboxChatPagewith/sandbox/...routing, session persistence, bootstrap via/api/web/sandboxes/bootstrap, per-sandbox model/system-prompt settings, and an OAuth gate for required sandbox servers.Refactors hosted OAuth callback handling. Replaces shared-only pending-key logic with a unified hosted callback context/pending marker, writes resume markers with sanitized error messages, and ensures a loading screen blocks hosted CTAs during callback handling (with new tests).
Improves trace/chat rendering for sandboxes. Adds sandbox host-style theming (ChatGPT/Claude) for composer, bubbles, avatars, and widget replay protocol selection; adds configurable
ReasoningDisplayMode(including hidden/collapsible) and updates trace adapter to optionally attach readable tool results to tool parts (used for sandbox usage views).Misc. Makes server URL validation always parse URLs and optionally require HTTPS; updates share usage dialog to support multiple source types; updates local
.env.localConvex endpoints.Written by Cursor Bugbot for commit 84bc1a1. This will update automatically on new commits. Configure here.