Conversation
✅ 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. |
dosubot
bot
added
the
size:XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds shared-chat viewing UI (ShareUsageDialog, ShareUsageThreadList, ShareUsageThreadDetail) and hooks (useSharedChatThreads, useSharedChatWidgetCapture). Threads chatSessionId through client-side hosted transports, useChatSession, and the shared request shape. Exposes prefersBorder in widget debug state. Adds server-side saveThreadToConvex and an onConversationComplete callback path in the MCPJam stream handler to persist full histories when provided. Removes model-message scrubbing for specific MCPJam flows. Introduces tests for widget capture, stream handling, and hosted chat-session behavior. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx (1)
883-888:⚠️ Potential issue | 🟠 MajorAvoid clobbering live widget state when
prefersBorderupdates.Adding
prefersBorderhere makes this effect run again once the widget metadata resolves, but the payload still includeswidgetState: initialWidgetState ?? null. BecausesetWidgetDebugInfotreats explicitnullas authoritative, that rerun wipes any state already written viasetWidgetState.Proposed fix
useEffect(() => { if (!toolName) return; setWidgetDebugInfo(resolvedToolCallId, { toolName, protocol: "openai-apps", - widgetState: initialWidgetState ?? null, + ...(initialWidgetState !== undefined + ? { widgetState: initialWidgetState } + : {}), prefersBorder, globals: { theme: themeMode, displayMode: effectiveDisplayMode, maxHeight: maxHeight ?? undefined,Also applies to: 900-913
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx` around lines 883 - 888, The effect is clobbering live widget state because setWidgetDebugInfo is being called with widgetState: initialWidgetState ?? null which writes an authoritative null; change the payload so widgetState is omitted (or set to undefined) when initialWidgetState is null/undefined to avoid wiping state. Update the call to setWidgetDebugInfo in the block that uses resolvedToolCallId/toolName/protocol to only include the widgetState property when initialWidgetState is defined (and do the same fix for the similar block around the code at the 900-913 region), referencing setWidgetDebugInfo, resolvedToolCallId, initialWidgetState and setWidgetState to locate the spots to change.
🧹 Nitpick comments (6)
mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageDialog.tsx (1)
33-33: Consider resettingselectedThreadIdwhenshareIdchanges.Should the user navigate to a different server's usage view, the previously selected thread ID may linger, referencing a thread that no longer pertains to the new
shareId. A targeted reset would ensure UI consistency.♻️ Proposed enhancement
+import { useEffect, useState } from "react"; -import { useState } from "react";Then add an effect:
useEffect(() => { setSelectedThreadId(null); }, [shareId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageDialog.tsx` at line 33, Reset selectedThreadId when the component's shareId changes to avoid showing a stale thread: add an effect in ShareUsageDialog that watches the shareId prop and calls setSelectedThreadId(null) when shareId changes so the previous selection doesn't persist across different shares.mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadList.tsx (1)
103-105: Keep relative timestamps fresh while the panel is open.
formatDistanceToNow(...)is evaluated only when this component rerenders, so labels like “5 minutes ago” will quietly go stale in a long-lived dialog. A lightweight timer or sharednowstate would keep this view accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadList.tsx` around lines 103 - 105, The relative timestamps in ShareUsageThreadList use formatDistanceToNow(thread.lastActivityAt) which only updates on re-render, so implement a lightweight updater: add a local now state (e.g., now via useState) and a useEffect that sets a setInterval (e.g., every 30–60s) to update that now state and clears the interval on unmount; then pass the now (or just rely on the state change) into the render where formatDistanceToNow is called so the label refreshes while the panel is open. Ensure the effect is inside the ShareUsageThreadList component and the interval is cleaned up to avoid leaks.mcpjam-inspector/client/src/hooks/__tests__/use-chat-session.hosted.test.tsx (1)
39-126: Prefer the shared client test presets over a bespoke mock matrix.This hosted fixture redefines nearly the whole auth/store/API surface inline, which will drift faster than the repo’s common test scaffolding. Starting from the shared presets and overriding only the hosted-specific pieces would keep this setup leaner and more stable.
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__/use-chat-session.hosted.test.tsx` around lines 39 - 126, The test redefines most auth/store/API mocks inline (e.g., useAiProviderKeys, useCustomProviders, usePersistedModel, useSharedChatWidgetCapture, useChat, getToolsMetadata, countTextTokens), which duplicates repo-wide presets; replace the bespoke mock matrix by importing and applying the shared test presets (including mcpApiPresets and storePresets) from the project test mocks and only override the hosted-specific pieces (HOSTED_MODE and any hosted-only return values like selectedModelId or lastTransportOptions) so the file uses the common mock scaffolding and only customizes the minimal hosted behavior.mcpjam-inspector/client/src/hooks/__tests__/useSharedChatWidgetCapture.test.tsx (1)
192-212: MissingprefersBorderin second test's widget state.The first test (line 96) includes
prefersBorder: false, but the second test (lines 196-211) omits it. While JavaScript treatsundefinedas falsy, explicitly settingprefersBorderwould ensure test consistency with the production interface.🧪 Add missing field
{ toolCallId: "call-1", toolName: "search", protocol: "mcp-apps", widgetState: null, + prefersBorder: false, globals: { theme: "dark", displayMode: "inline", }, widgetHtml: "<div>Widget</div>", updatedAt: Date.now(), },🤖 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 192 - 212, The test's widget state set via useWidgetDebugStore.setState is missing the prefersBorder property for the "call-1" widget entry; update that Map entry to include prefersBorder: false (matching the first test) so the test shape matches the production interface and prevents subtle falsy/undefined differences when the widget is read by useSharedChatWidgetCapture or related selectors.mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadDetail.tsx (1)
104-110: Type cast obscures potential structural mismatches.The
messages as anycast bypasses type checking. Consider defining a minimal interface for the blob-fetched message structure, or using a runtime validator to ensure the fetched data conforms to expectations before adaptation.💡 Type-safe alternative
+// At top of file or in a types module +interface BlobMessage { + role: string; + content?: unknown; + // Add other expected fields +} + const adaptedTrace = useMemo(() => { if (!messages) return null; + // Optional: validate structure at runtime return adaptTraceToUiMessages({ - trace: { messages: messages as any, widgetSnapshots }, + trace: { messages: messages as BlobMessage[], widgetSnapshots }, }); }, [messages, widgetSnapshots]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadDetail.tsx` around lines 104 - 110, The code is masking potential shape mismatches by casting messages to any before passing to adaptTraceToUiMessages; remove the unsafe cast and instead define a minimal TypeScript interface for the fetched message blob (e.g. MessageBlob or FetchedMessage[]) or implement a runtime type guard/validator that verifies the required fields, then in the useMemo block check/validate messages (using the type guard or mapping to the typed shape) before calling adaptTraceToUiMessages({ trace: { messages: validatedMessages, widgetSnapshots } }); update adaptedTrace logic to handle validation failure (null or error) so adaptTraceToUiMessages always receives a correctly typed structure.mcpjam-inspector/server/utils/shared-chat-persistence.ts (1)
73-88: Consider adding a fetch timeout for resilience.The fetch call lacks a timeout, which could cause the callback to hang if Convex becomes unresponsive. Since this runs asynchronously after stream completion, a hung request won't block the user—but it may exhaust connection pools or delay cleanup.
⏱️ Add AbortController with timeout
try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); + const response = await fetch(`${convexUrl}/shared-chat/save-thread`, { method: "POST", headers: { "content-type": "application/json", authorization: `Bearer ${bearerToken}`, }, body: JSON.stringify({ chatSessionId, shareToken, messages, messageCount, firstMessagePreview: getFirstMessagePreview(messages), ...(modelId ? { modelId } : {}), }), + signal: controller.signal, }); + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/server/utils/shared-chat-persistence.ts` around lines 73 - 88, The fetch to `${convexUrl}/shared-chat/save-thread` needs an AbortController-based timeout to avoid hung requests: create an AbortController, pass controller.signal to fetch, start a setTimeout to call controller.abort() after a reasonable timeout (e.g., 5–10s), and clear the timeout when the fetch resolves; also update the try/catch to properly handle AbortError (or treat it as a non-fatal failure) so the code using bearerToken, messages, messageCount, getFirstMessagePreview, and modelId doesn't leak resources or keep connections open.
🤖 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/share-usage/ShareUsageThreadList.tsx`:
- Around line 78-85: The button in ShareUsageThreadList.tsx only conveys
selection via color; update the button element (the control rendered in
ShareUsageThreadList where onClick={onSelect} and isSelected is used) to expose
the active thread to assistive tech by adding an explicit ARIA state—e.g., add
aria-pressed={isSelected} or aria-current={isSelected ? "true" : undefined}—so
screen readers can identify the active conversation.
- Around line 22-34: The component currently treats threads === undefined as
"loading"; update the hook useSharedChatThreadList to explicitly return
loading/error states (e.g., isLoading, isError, error and optionally refetch)
instead of only { threads }, then change ShareUsageThreadList to branch on
isLoading (render existing skeleton), isError (render an error UI with the error
message and a retry action using refetch if provided), and only render the
threads list when !isLoading && !isError && threads is present; reference
useSharedChatThreadList and the ShareUsageThreadList component to locate the
changes.
In `@mcpjam-inspector/server/utils/mcpjam-stream-handler.ts`:
- Around line 52-54: The current flow lets a failing stream call
onConversationComplete (or let its rejection propagate) and block
onStreamComplete; change the control flow so teardown always runs and
conversation persistence only happens for successful runs: wrap the stream
processing in try/catch/finally (or promise.then/.catch/.finally) so that
onConversationComplete is invoked only on the success path and any errors from
onConversationComplete are caught and do not prevent calling onStreamComplete;
apply the same pattern to the other occurrences of
onConversationComplete/onFinish/onStreamComplete in this file (the other similar
blocks referenced) and ensure onStreamComplete runs in the finally block for
unconditional cleanup.
---
Outside diff comments:
In
`@mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsx`:
- Around line 883-888: The effect is clobbering live widget state because
setWidgetDebugInfo is being called with widgetState: initialWidgetState ?? null
which writes an authoritative null; change the payload so widgetState is omitted
(or set to undefined) when initialWidgetState is null/undefined to avoid wiping
state. Update the call to setWidgetDebugInfo in the block that uses
resolvedToolCallId/toolName/protocol to only include the widgetState property
when initialWidgetState is defined (and do the same fix for the similar block
around the code at the 900-913 region), referencing setWidgetDebugInfo,
resolvedToolCallId, initialWidgetState and setWidgetState to locate the spots to
change.
---
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageDialog.tsx`:
- Line 33: Reset selectedThreadId when the component's shareId changes to avoid
showing a stale thread: add an effect in ShareUsageDialog that watches the
shareId prop and calls setSelectedThreadId(null) when shareId changes so the
previous selection doesn't persist across different shares.
In
`@mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadDetail.tsx`:
- Around line 104-110: The code is masking potential shape mismatches by casting
messages to any before passing to adaptTraceToUiMessages; remove the unsafe cast
and instead define a minimal TypeScript interface for the fetched message blob
(e.g. MessageBlob or FetchedMessage[]) or implement a runtime type
guard/validator that verifies the required fields, then in the useMemo block
check/validate messages (using the type guard or mapping to the typed shape)
before calling adaptTraceToUiMessages({ trace: { messages: validatedMessages,
widgetSnapshots } }); update adaptedTrace logic to handle validation failure
(null or error) so adaptTraceToUiMessages always receives a correctly typed
structure.
In
`@mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadList.tsx`:
- Around line 103-105: The relative timestamps in ShareUsageThreadList use
formatDistanceToNow(thread.lastActivityAt) which only updates on re-render, so
implement a lightweight updater: add a local now state (e.g., now via useState)
and a useEffect that sets a setInterval (e.g., every 30–60s) to update that now
state and clears the interval on unmount; then pass the now (or just rely on the
state change) into the render where formatDistanceToNow is called so the label
refreshes while the panel is open. Ensure the effect is inside the
ShareUsageThreadList component and the interval is cleaned up to avoid leaks.
In
`@mcpjam-inspector/client/src/hooks/__tests__/use-chat-session.hosted.test.tsx`:
- Around line 39-126: The test redefines most auth/store/API mocks inline (e.g.,
useAiProviderKeys, useCustomProviders, usePersistedModel,
useSharedChatWidgetCapture, useChat, getToolsMetadata, countTextTokens), which
duplicates repo-wide presets; replace the bespoke mock matrix by importing and
applying the shared test presets (including mcpApiPresets and storePresets) from
the project test mocks and only override the hosted-specific pieces (HOSTED_MODE
and any hosted-only return values like selectedModelId or lastTransportOptions)
so the file uses the common mock scaffolding and only customizes the minimal
hosted behavior.
In
`@mcpjam-inspector/client/src/hooks/__tests__/useSharedChatWidgetCapture.test.tsx`:
- Around line 192-212: The test's widget state set via
useWidgetDebugStore.setState is missing the prefersBorder property for the
"call-1" widget entry; update that Map entry to include prefersBorder: false
(matching the first test) so the test shape matches the production interface and
prevents subtle falsy/undefined differences when the widget is read by
useSharedChatWidgetCapture or related selectors.
In `@mcpjam-inspector/server/utils/shared-chat-persistence.ts`:
- Around line 73-88: The fetch to `${convexUrl}/shared-chat/save-thread` needs
an AbortController-based timeout to avoid hung requests: create an
AbortController, pass controller.signal to fetch, start a setTimeout to call
controller.abort() after a reasonable timeout (e.g., 5–10s), and clear the
timeout when the fetch resolves; also update the try/catch to properly handle
AbortError (or treat it as a non-fatal failure) so the code using bearerToken,
messages, messageCount, getFirstMessagePreview, and modelId doesn't leak
resources or keep connections open.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b7284b5-1bc5-4de3-b872-5870de46a82d
📒 Files selected for processing (20)
mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxmcpjam-inspector/client/src/components/chat-v2/thread/mcp-apps/mcp-apps-renderer.tsxmcpjam-inspector/client/src/components/connection/ShareServerDialog.tsxmcpjam-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/hooks/__tests__/use-chat-session.hosted.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-chat-session.minimal-mode.test.tsxmcpjam-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/client/src/stores/widget-debug-store.tsmcpjam-inspector/server/routes/mcp/chat-v2.tsmcpjam-inspector/server/routes/web/auth.tsmcpjam-inspector/server/routes/web/chat-v2.tsmcpjam-inspector/server/utils/__tests__/mcpjam-stream-handler.test.tsmcpjam-inspector/server/utils/mcpjam-stream-handler.tsmcpjam-inspector/server/utils/shared-chat-persistence.tsmcpjam-inspector/shared/chat-v2.ts
| if (threads === undefined) { | ||
| return ( | ||
| <div className="space-y-3 p-3"> | ||
| {Array.from({ length: 4 }).map((_, i) => ( | ||
| <div key={i} className="space-y-2 rounded-lg border p-3"> | ||
| <div className="h-4 w-2/3 animate-pulse rounded bg-muted" /> | ||
| <div className="h-3 w-full animate-pulse rounded bg-muted" /> | ||
| <div className="h-3 w-1/3 animate-pulse rounded bg-muted" /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Separate loading from failure states.
Line 22 treats every undefined result as “still loading”. Since useSharedChatThreadList only exposes { threads } in mcpjam-inspector/client/src/hooks/useSharedChatThreads.ts:32-43, a failed or invalid query is indistinguishable here and falls back to the skeleton instead of an error or retry UI. Please thread explicit loading/error state through the hook and branch on that instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadList.tsx`
around lines 22 - 34, The component currently treats threads === undefined as
"loading"; update the hook useSharedChatThreadList to explicitly return
loading/error states (e.g., isLoading, isError, error and optionally refetch)
instead of only { threads }, then change ShareUsageThreadList to branch on
isLoading (render existing skeleton), isError (render an error UI with the error
message and a retry action using refetch if provided), and only render the
threads list when !isLoading && !isError && threads is present; reference
useSharedChatThreadList and the ShareUsageThreadList component to locate the
changes.
| <button | ||
| type="button" | ||
| onClick={onSelect} | ||
| className={`w-full rounded-lg border p-3 text-left transition-colors ${ | ||
| isSelected | ||
| ? "border-primary/50 bg-primary/5" | ||
| : "border-transparent hover:bg-muted/50" | ||
| }`} |
There was a problem hiding this comment.
Expose the active thread to assistive tech.
The current selection is only expressed through color. Screen readers will encounter identical buttons and won’t know which conversation is active. Add an explicit state on the control, e.g. aria-pressed={isSelected} or aria-current={isSelected ? "true" : undefined}.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageThreadList.tsx`
around lines 78 - 85, The button in ShareUsageThreadList.tsx only conveys
selection via color; update the button element (the control rendered in
ShareUsageThreadList where onClick={onSelect} and isSelected is used) to expose
the active thread to assistive tech by adding an explicit ARIA state—e.g., add
aria-pressed={isSelected} or aria-current={isSelected ? "true" : undefined}—so
screen readers can identify the active conversation.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| return nextMessages; | ||
| }); | ||
| }, | ||
| [baseSetMessages], |
There was a problem hiding this comment.
Side effects inside React state updater break StrictMode
Medium Severity
The setMessages wrapper performs side effects inside the baseSetMessages state updater function: it mutates skipNextForkDetectionRef.current to false and calls queueMicrotask(() => setChatSessionId(generateId())). React state updater functions must be pure. In StrictMode, React double-invokes updaters — the first call sets skipNextForkDetectionRef.current = false, so the second call incorrectly sees false instead of true, defeating the skip mechanism during resetChat and the auth effect. This can cause an unintended fork (extra setChatSessionId) in development, and the pattern is fragile even in production.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
mcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.ts (3)
253-258: Response structure assumed without validation.Casting the JSON response to
{ storageId?: string }trusts the upstream contract. A malformed response yields an opaque "did not return a storageId" error, obscuring root cause.🛡️ Optional: add minimal shape validation
- const result = (await response.json()) as { storageId?: string }; - if (!result.storageId) { - throw new Error("Snapshot upload did not return a storageId"); - } + const result: unknown = await response.json(); + const storageId = + result && typeof result === "object" && "storageId" in result + ? (result as { storageId: unknown }).storageId + : undefined; + if (typeof storageId !== "string") { + throw new Error( + `Snapshot upload response invalid: ${JSON.stringify(result)}`, + ); + } - return result.storageId; + return storageId;🤖 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 253 - 258, The code in useSharedChatWidgetCapture currently casts response.json() to { storageId?: string } and throws a generic error when storageId is missing; instead, explicitly validate the parsed JSON shape: parse the response with try/catch to surface JSON parse errors, verify that the parsed object has a storageId of type string (e.g., check result && typeof result.storageId === 'string'), and if validation fails throw an error that includes the raw/parsing details (or the stringified response body) to aid debugging; update the logic around the result variable and the return from the function so only a validated storageId is returned.
26-32: DJB2 suffices here, though collisions are theoretically possible.For deduplication of widget HTML within a single session, the 32-bit hash collision risk is negligible. If snapshot volumes grow substantially, consider a stronger hash (e.g., SHA-256 truncated).
🤖 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 26 - 32, The current hashString function implements a 32-bit DJB2-style hash which is fine for session-level deduplication but has theoretical collision risk; update the code around function hashString(value: string) to add a clear comment that it intentionally uses a 32-bit DJB2-like hash for performance/space reasons, document the collision tradeoff, and add a TODO and small hook (e.g., a comment suggesting/pointing to a future sha256-truncate replacement or a named function like computeStrongHash) so it’s easy to switch to a stronger algorithm if snapshot volume grows.
155-160: Consider typed Convex function references.The
as anycasts circumvent TypeScript's safeguards. If generated types for these mutations exist (or will), importing them preserves compile-time validation of argument shapes.🔧 Proposed improvement
+import { api } from "@/convex/_generated/api"; + export function useSharedChatWidgetCapture({ ... - const generateSnapshotUploadUrl = useMutation( - "sharedChatThreads:generateSnapshotUploadUrl" as any, - ); - const createWidgetSnapshot = useMutation( - "sharedChatThreads:createWidgetSnapshot" as any, - ); + const generateSnapshotUploadUrl = useMutation( + api.sharedChatThreads.generateSnapshotUploadUrl, + ); + const createWidgetSnapshot = useMutation( + api.sharedChatThreads.createWidgetSnapshot, + );🤖 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 155 - 160, The two useMutation calls currently cast the function names to any, removing type safety; replace the "as any" casts by importing and using the appropriate Convex-generated mutation types (or the typed mutation helpers) and pass those to useMutation so the calls for generateSnapshotUploadUrl and createWidgetSnapshot are strongly typed; specifically locate the constants generateSnapshotUploadUrl and createWidgetSnapshot and change their useMutation arguments to the generated mutation type references (imported from your Convex _generated types or equivalent) so TypeScript validates the argument shapes at compile time.
🤖 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/useSharedChatWidgetCapture.ts`:
- Around line 110-114: The viewport object in useSharedChatWidgetCapture (the
`viewport` variable using `globals.maxWidth` and `globals.maxHeight`)
incorrectly represents host-imposed constraints as actual viewport dimensions
and also checks `maxWidth` which is never set; change this to expose a
constraints-style field (e.g., `viewportConstraints` or `constraints`) that only
includes known host constraint values (use `globals.maxHeight` if present and
drop the dead `globals.maxWidth` guard), or leave `viewport` undefined and add a
new `constraints` object containing `{ maxHeight: globals.maxHeight }` so
consumers are not misled into thinking these are real viewport dimensions.
Ensure references to `viewport` in this module are updated to use the new name
or pattern.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.ts`:
- Around line 253-258: The code in useSharedChatWidgetCapture currently casts
response.json() to { storageId?: string } and throws a generic error when
storageId is missing; instead, explicitly validate the parsed JSON shape: parse
the response with try/catch to surface JSON parse errors, verify that the parsed
object has a storageId of type string (e.g., check result && typeof
result.storageId === 'string'), and if validation fails throw an error that
includes the raw/parsing details (or the stringified response body) to aid
debugging; update the logic around the result variable and the return from the
function so only a validated storageId is returned.
- Around line 26-32: The current hashString function implements a 32-bit
DJB2-style hash which is fine for session-level deduplication but has
theoretical collision risk; update the code around function hashString(value:
string) to add a clear comment that it intentionally uses a 32-bit DJB2-like
hash for performance/space reasons, document the collision tradeoff, and add a
TODO and small hook (e.g., a comment suggesting/pointing to a future
sha256-truncate replacement or a named function like computeStrongHash) so it’s
easy to switch to a stronger algorithm if snapshot volume grows.
- Around line 155-160: The two useMutation calls currently cast the function
names to any, removing type safety; replace the "as any" casts by importing and
using the appropriate Convex-generated mutation types (or the typed mutation
helpers) and pass those to useMutation so the calls for
generateSnapshotUploadUrl and createWidgetSnapshot are strongly typed;
specifically locate the constants generateSnapshotUploadUrl and
createWidgetSnapshot and change their useMutation arguments to the generated
mutation type references (imported from your Convex _generated types or
equivalent) so TypeScript validates the argument shapes at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a25a889d-f4c3-4885-8515-1d7b4f3363be
📒 Files selected for processing (2)
mcpjam-inspector/client/src/components/connection/share-usage/ShareUsageDialog.tsxmcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.ts
| viewport: | ||
| typeof globals.maxWidth === "number" && | ||
| typeof globals.maxHeight === "number" | ||
| ? { width: globals.maxWidth, height: globals.maxHeight } | ||
| : undefined, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate where maxWidth/maxHeight are set to understand their semantics
rg -n -C3 "maxWidth|maxHeight" --type=ts --type=tsx -g '!node_modules' -g '!*.test.*'Repository: MCPJam/inspector
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for maxWidth/maxHeight across all TS/TSX files
rg -n -C3 "maxWidth|maxHeight" --type=ts -g '!node_modules' -g '!*.test.*'Repository: MCPJam/inspector
Length of output: 41260
🏁 Script executed:
#!/bin/bash
# Also check the specific file mentioned in the review
cat mcpjam-inspector/client/src/hooks/useSharedChatWidgetCapture.tsRepository: MCPJam/inspector
Length of output: 10973
🏁 Script executed:
#!/bin/bash
# Search for maxWidth assignments or setters on globals/WidgetGlobals
rg "maxWidth\s*[:=]|\.maxWidth\s*=" --type=ts -g '!node_modules' -B2 -A2Repository: MCPJam/inspector
Length of output: 2727
🏁 Script executed:
#!/bin/bash
# Search for where globals/WidgetGlobals are constructed or assigned
rg "setWidgetGlobals|WidgetGlobals\s*{" --type=ts -B1 -A5 | head -100Repository: MCPJam/inspector
Length of output: 191
🏁 Script executed:
#!/bin/bash
# Search for where globals/WidgetGlobals are constructed or assigned
rg "setWidgetGlobals|WidgetGlobals" --type=ts -B1 -A5 | grep -A5 "maxHeight\|maxWidth" | head -80Repository: MCPJam/inspector
Length of output: 1418
🏁 Script executed:
#!/bin/bash
# Search for all places where WidgetGlobals objects are created/assigned
rg "globals\s*:\s*\{|setWidgetGlobals" --type=ts -A10 | grep -E "maxWidth|maxHeight|globals|setWidgetGlobals" | head -60Repository: MCPJam/inspector
Length of output: 3817
🏁 Script executed:
#!/bin/bash
# Check the specific lines where globals are set in chatgpt-app-renderer.tsx
sed -n '888,920p' mcpjam-inspector/client/src/components/chat-v2/thread/chatgpt-app-renderer.tsxRepository: MCPJam/inspector
Length of output: 781
🏁 Script executed:
#!/bin/bash
# Verify WidgetGlobals interface definition
sed -n '63,72p' mcpjam-inspector/client/src/stores/widget-debug-store.tsRepository: MCPJam/inspector
Length of output: 344
Viewport derivation conflates constraint with dimensions.
The viewport object stores maxHeight and maxWidth from host globals, but these represent host-imposed constraints (e.g., ChatGPT's ~500px max-height for inline mode), not actual viewport dimensions. This semantic mismatch could mislead snapshot consumers about the display environment.
Additionally, maxWidth is never set in the codebase, making the type guard for it dead code that will always evaluate to undefined.
🤖 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
110 - 114, The viewport object in useSharedChatWidgetCapture (the `viewport`
variable using `globals.maxWidth` and `globals.maxHeight`) incorrectly
represents host-imposed constraints as actual viewport dimensions and also
checks `maxWidth` which is never set; change this to expose a constraints-style
field (e.g., `viewportConstraints` or `constraints`) that only includes known
host constraint values (use `globals.maxHeight` if present and drop the dead
`globals.maxWidth` guard), or leave `viewport` undefined and add a new
`constraints` object containing `{ maxHeight: globals.maxHeight }` so consumers
are not misled into thinking these are real viewport dimensions. Ensure
references to `viewport` in this module are updated to use the new name or
pattern.
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.
| queueMicrotask(() => { | ||
| setChatSessionId(generateId()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fork detection may lose messages when session ID changes
High Severity
When shouldForkChatSession detects a branch (user trimmed messages), it sets trimmed messages via baseSetMessages and then schedules setChatSessionId(generateId()) via queueMicrotask. Since useChat is keyed by id: chatSessionId, changing the session ID causes useChat to switch to a new, empty message store — discarding the trimmed messages the user intended to keep. The trimmed messages were written to the old session's store, while the new session starts empty. Compare with resetChat, which intentionally clears messages after changing the session ID; here the clearing is unintentional.
Additional Locations (1)
This reverts commit af33a45.
1 participant
Note
Medium Risk
Touches streaming chat completion flow and adds background persistence/uploads to Convex plus new UI for viewing stored conversations; failures could impact chat completion teardown/persistence or shared-link behavior.
Overview
Adds hosted shared-chat persistence by sending
chatSessionIdfrom the client, saving completed conversation history to Convex, and capturing/rendering widget snapshots (HTML + tool I/O + CSP/permissions) for later replay.Extends the Share Server dialog with a new Usage view that lists visitor conversations and lets owners open a thread detail viewer that replays messages with captured widgets. Also wires
prefersBorderinto widget debug info so snapshots preserve widget border preference.Written by Cursor Bugbot for commit 8e1b36d. This will update automatically on new commits. Configure here.