Chat sessions#1628
Merged
chelojimenez merged 9 commits intomainfrom Mar 17, 2026
Merged
Conversation
dosubot
Bot
added
the
size:L
|
🚅 Environment inspector-pr-1628 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
|
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 (19)
WalkthroughThis PR renames client endpoints/payloads from 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mcpjam-inspector/server/routes/web/chat-v2.ts (1)
226-235:⚠️ Potential issue | 🟠 MajorReject requests that include both
shareTokenandsandboxToken.Current precedence silently chooses
"serverShare"when both tokens are present, which can misclassify session origin and persistence metadata.Proposed fix
const { messages, model, systemPrompt, temperature, requireToolApproval, selectedServerIds, shareToken, sandboxToken, } = body; + + if (shareToken && sandboxToken) { + throw new WebRouteError( + 400, + ErrorCode.VALIDATION_ERROR, + "shareToken and sandboxToken are mutually exclusive", + ); + }Also applies to: 324-330
🤖 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` around lines 226 - 235, When parsing the request (where body is destructured into messages, model, systemPrompt, temperature, requireToolApproval, selectedServerIds, shareToken, sandboxToken) add an early validation that rejects the request with a 400 Bad Request if both shareToken and sandboxToken are present; do the same in the later decision branch that currently prefers "serverShare" when both tokens exist (the block around the logic that classifies session origin/persistence). Return a clear error message indicating mutually exclusive tokens and ensure both places short-circuit before any default precedence logic runs.mcpjam-inspector/client/src/hooks/__tests__/useSharedChatWidgetCapture.test.tsx (1)
30-53: 🛠️ Refactor suggestion | 🟠 MajorAlign this test with repository mock presets.
This setup uses bespoke inline mocks instead of the shared test presets expected for client tests.
As per coding guidelines:
mcpjam-inspector/client/**/__tests__/*.test.{ts,tsx}: "Use mock presets fromclient/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__/useSharedChatWidgetCapture.test.tsx` around lines 30 - 53, The test is using bespoke inline mocks (vi.useFakeTimers setup, manual mockGenerateSnapshotUploadUrl/mockCreateWidgetSnapshot implementations, and overriding global.fetch, plus direct useWidgetDebugStore.setState) instead of the repository's shared test presets; replace the inline mock setup by importing and applying the client test presets (mcpApiPresets and storePresets from the client test mocks) at the start of the beforeEach for the useSharedChatWidgetCapture suite, remove the custom mockGenerateSnapshotUploadUrl, mockCreateWidgetSnapshot and global.fetch overrides, and initialize widget store state via the storePresets API so the preset implementations provide consistent mocked behavior for generateSnapshotUploadUrl, createWidgetSnapshot and network fetches used by the hook.mcpjam-inspector/client/src/hooks/use-chat-session.ts (1)
363-378:⚠️ Potential issue | 🟠 MajorAdd
sandboxTokento thebuildHostedBodyfunction.The hosted path in
/api/web/chat-v2destructuressandboxTokenfrom the request body and uses it to determine the session'ssourceTypeduring persistence. However,buildHostedBody()spreadsshareTokenbut omitshostedSandboxToken, preventing sandbox session tracking from working correctly.Fix
return { workspaceId: hostedWorkspaceId, chatSessionId, selectedServerIds: hostedSelectedServerIds, accessScope: "chat_v2" as const, ...(hostedShareToken ? { shareToken: hostedShareToken } : {}), + ...(hostedSandboxToken ? { sandboxToken: hostedSandboxToken } : {}), ...(hostedOAuthTokens && Object.keys(hostedOAuthTokens).length > 0 ? { oauthTokens: hostedOAuthTokens } : {}), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-chat-session.ts` around lines 363 - 378, The hosted request body built by buildHostedBody is missing sandboxToken (hostedSandboxToken), so add sandboxToken: hostedSandboxToken to the returned object for the hosted branch; include it similarly to shareToken (conditionally spread only when present) and keep the existing selectedServerIds, accessScope, and oauthTokens logic so the server can detect sandbox sessions when persisting sourceType.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.ts (1)
329-331: Make retry gating less brittle than raw message matching.
message.includes("Session not found")is fragile; prefer a stable error code/classification from the mutation response when available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.ts` around lines 329 - 331, The retry gating currently checks a fragile substring on the error text in useSharedChatWidgetCapture (message.includes("Session not found")); instead, read a stable error identifier from the mutation/response object (e.g., error.code, error.name, or response.error.code) and use that to decide retries for the given toolCallId and retryCountRef; update the conditional that references message.includes("Session not found") to check the canonical error field (with a safe fallback to the message only if the canonical field is missing) so the retry logic uses a durable error code/classification rather than raw text.
🤖 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/useSharedChatThreads.ts`:
- Around line 12-17: The contract now makes userId, visitorDisplayName, modelId
and firstMessagePreview optional, so update useSharedChatThreads (the hook that
maps/returns thread objects) to supply defensive defaults for any missing values
before UI consumption: e.g., normalize undefined visitorDisplayName to
"Visitor", modelId to "default-model" or a human-readable model name,
firstMessagePreview to an empty string or "(no preview)", and userId to a stable
placeholder like "unknown" or null-safe handling; ensure the returned thread
shape from useSharedChatThreads always contains these fallback values so
downstream render paths never receive undefined and can render predictably.
In `@mcpjam-inspector/server/utils/chat-ingestion.ts`:
- Around line 81-83: The code currently captures and logs the full ingestion
response body via responseText (set from await response.text()) along with
response.status; change this to avoid storing/logging full response bodies by
replacing responseText with a bounded and sanitized diagnostic: read the
response text but truncate it to a small length (e.g., first 200 chars),
normalize whitespace/newlines, and mask obvious secrets (e.g., API keys, tokens,
emails) before logging, or alternatively log only response.status plus
response.headers or a short status message; update the assignment that sets
responseText and any log callers to use this truncated/sanitized value instead
of the full body.
- Around line 36-74: The POST to `${convexUrl}/ingest-chat` uses a plain fetch
(const response = await fetch(...)) with no timeout; wrap the fetch in an
AbortController: create controller = new AbortController(), pass signal:
controller.signal into the fetch options, start a timer (e.g. setTimeout) that
calls controller.abort() after a configurable timeout (choose a sensible default
or read from options), and clearTimeout(controller) after fetch completes; also
ensure you handle the abort case (catch AbortError) so ingestion failures due to
timeout are surfaced/handled consistently.
In `@mcpjam-inspector/server/utils/mcpjam-stream-handler.ts`:
- Around line 689-691: The MCP route call to handleMCPJamFreeChatModel is
currently passing skipChatIngestion: true and no onConversationComplete handler,
so MCP conversations are never persisted; either add the same conditional
persistence callback used in web routes or document the intentional exclusion.
Specifically, update the handleMCPJamFreeChatModel invocation to accept an
onConversationComplete function (matching the signature used in
web/routes/web/chat-v2.ts) and call your backend persistence when chatSessionId
is present, or add a clear comment near the call site (and keep
skipChatIngestion semantics) explaining why MCP conversations must not be
persisted; refer to the handleMCPJamFreeChatModel call, the skipChatIngestion
flag, and onConversationComplete callback to implement this change.
---
Outside diff comments:
In
`@mcpjam-inspector/client/src/hooks/__tests__/useSharedChatWidgetCapture.test.tsx`:
- Around line 30-53: The test is using bespoke inline mocks (vi.useFakeTimers
setup, manual mockGenerateSnapshotUploadUrl/mockCreateWidgetSnapshot
implementations, and overriding global.fetch, plus direct
useWidgetDebugStore.setState) instead of the repository's shared test presets;
replace the inline mock setup by importing and applying the client test presets
(mcpApiPresets and storePresets from the client test mocks) at the start of the
beforeEach for the useSharedChatWidgetCapture suite, remove the custom
mockGenerateSnapshotUploadUrl, mockCreateWidgetSnapshot and global.fetch
overrides, and initialize widget store state via the storePresets API so the
preset implementations provide consistent mocked behavior for
generateSnapshotUploadUrl, createWidgetSnapshot and network fetches used by the
hook.
In `@mcpjam-inspector/client/src/hooks/use-chat-session.ts`:
- Around line 363-378: The hosted request body built by buildHostedBody is
missing sandboxToken (hostedSandboxToken), so add sandboxToken:
hostedSandboxToken to the returned object for the hosted branch; include it
similarly to shareToken (conditionally spread only when present) and keep the
existing selectedServerIds, accessScope, and oauthTokens logic so the server can
detect sandbox sessions when persisting sourceType.
In `@mcpjam-inspector/server/routes/web/chat-v2.ts`:
- Around line 226-235: When parsing the request (where body is destructured into
messages, model, systemPrompt, temperature, requireToolApproval,
selectedServerIds, shareToken, sandboxToken) add an early validation that
rejects the request with a 400 Bad Request if both shareToken and sandboxToken
are present; do the same in the later decision branch that currently prefers
"serverShare" when both tokens exist (the block around the logic that classifies
session origin/persistence). Return a clear error message indicating mutually
exclusive tokens and ensure both places short-circuit before any default
precedence logic runs.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.ts`:
- Around line 329-331: The retry gating currently checks a fragile substring on
the error text in useSharedChatWidgetCapture (message.includes("Session not
found")); instead, read a stable error identifier from the mutation/response
object (e.g., error.code, error.name, or response.error.code) and use that to
decide retries for the given toolCallId and retryCountRef; update the
conditional that references message.includes("Session not found") to check the
canonical error field (with a safe fallback to the message only if the canonical
field is missing) so the retry logic uses a durable error code/classification
rather than raw text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c46c95a-4bf5-4438-824c-e72c7d0c2da9
📒 Files selected for processing (10)
mcpjam-inspector/client/src/hooks/__tests__/useSharedChatWidgetCapture.test.tsxmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/client/src/hooks/useSharedChatThreads.tsmcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.tsmcpjam-inspector/server/routes/mcp/chat-v2.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/utils/__tests__/chat-ingestion.test.tsmcpjam-inspector/server/utils/chat-ingestion.tsmcpjam-inspector/server/utils/mcpjam-stream-handler.tsmcpjam-inspector/server/utils/shared-chat-persistence.ts
💤 Files with no reviewable changes (1)
- mcpjam-inspector/server/utils/shared-chat-persistence.ts
Comment on lines
+12
to
17
| userId?: string; | ||
| visitorDisplayName?: string; | ||
| modelId?: string; | ||
| messageCount: number; | ||
| firstMessagePreview: string; | ||
| firstMessagePreview?: string; | ||
| startedAt: number; |
Contributor
There was a problem hiding this comment.
Optionalized thread fields need defensive UI fallbacks downstream.
Now that these fields are nullable in the contract, any direct render paths should default missing values (e.g., model/display name) to avoid blank/undefined UI.
🤖 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 12 -
17, The contract now makes userId, visitorDisplayName, modelId and
firstMessagePreview optional, so update useSharedChatThreads (the hook that
maps/returns thread objects) to supply defensive defaults for any missing values
before UI consumption: e.g., normalize undefined visitorDisplayName to
"Visitor", modelId to "default-model" or a human-readable model name,
firstMessagePreview to an empty string or "(no preview)", and userId to a stable
placeholder like "unknown" or null-safe handling; ensure the returned thread
shape from useSharedChatThreads always contains these fallback values so
downstream render paths never receive undefined and can render predictably.
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
Introduces new persistence/network calls and changes hosted auth/session token handling (including sandbox/shared flows), which could affect chat streaming completion and Convex ingestion reliability.
Overview
Chat requests now carry a
chatSessionId(and hosted requests may includesandboxToken), enabling session-based persistence and sandbox/shared flows across client and server.On the server, both
web/chat-v2andmcp/chat-v2persist completed conversations to Convex via a newpersistChatSessionToConvexhelper (replacingsaveThreadToConvex), including BYOKstreamTextcompletions viaonFinish; MCPJam streaming skips per-step ingestion and instead persists once at the end of the agent loop. Client shared-chat hooks are updated to query/mutatechatSessions:*APIs, treat sandbox sessions like shared sessions for guest behavior and widget capture, and add retry handling when snapshot creation returnsnullor reports a missing session.Written by Cursor Bugbot for commit 1718f18. This will update automatically on new commits. Configure here.