Handle images from updateModelContext and setWidgetState#1531
Open
mikechao wants to merge 4 commits intoMCPJam:mainfrom
Open
Handle images from updateModelContext and setWidgetState#1531mikechao wants to merge 4 commits intoMCPJam:mainfrom
mikechao wants to merge 4 commits intoMCPJam:mainfrom
Conversation
dosubot
bot
added
the
size:XXL
Contributor
✅ 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
Author
|
An example of chat before the fix 
An example of chat after the fix |
Contributor
|
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 a new useWidgetStateSync hook (exporting ModelContextItem and sync APIs) that serializes async widget-state updates, provides epoch-based reset/cancel, and exposes refs for in-flight work and model-context queueing. Introduces mcp-ui modules to build widget state parts and model-context messages (including file resolution and data-URL conversion). Refactors ChatTabV2 and PlaygroundMain to await widget-state conversion, enqueue updates via the hook, include built model-context messages and FileUIParts on submit, and surface submit errors with toast. Adds comprehensive tests and test mocks for the new flows. 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: 6
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts (1)
193-196: Resolve uploaded file IDs concurrently to reduce widget-sync latency.Serial
awaitin this loop scales linearly with the number of images and slows submit paths unnecessarily.⚡ Suggested refactor
- for (const fileId of extractUploadedFileIds(state)) { - const filePart = await resolveFilePart(fileId).catch(() => null); - if (filePart) parts.push(filePart); - } + const resolvedFileParts = await Promise.all( + extractUploadedFileIds(state).map((fileId) => + resolveFilePart(fileId).catch(() => null), + ), + ); + for (const filePart of resolvedFileParts) { + if (filePart) parts.push(filePart); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts` around lines 193 - 196, The loop that resolves uploaded file parts does serial awaits and should be converted to concurrent resolution: map extractUploadedFileIds(state) to an array of promises calling resolveFilePart(fileId) with a catch that returns null, await Promise.all on that array, then iterate the resulting array and push only non-null results into parts; update the code around extractUploadedFileIds, resolveFilePart and parts to use this concurrent pattern so widget-sync latency no longer scales linearly with number of images.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/hooks/__tests__/use-widget-state-sync.test.ts`:
- Around line 1-11: Remove the ad-hoc hoisted mock (buildWidgetStatePartsMock)
and the vi.mock call; instead import the project's shared client test presets
(mcpApiPresets and storePresets) from client/src/test/mocks and register/apply
them in the test setup (e.g., in beforeEach) using the repository's test mock
helper so the standard presets provide the buildWidgetStateParts mock and other
client mocks rather than the local vi.hoisted mock and vi.mock.
In `@mcpjam-inspector/client/src/hooks/use-widget-state-sync.ts`:
- Around line 160-165: The current useEffect clears widgetStateQueue
optimistically before async work finishes; update the effect so it calls
enqueueWidgetStateSync(widgetStateQueue) and only calls setWidgetStateQueue([])
after the returned promise resolves successfully, and on rejection preserve the
existing queue (or requeue/fall back and log) so failed syncs are not dropped;
adjust the effect that references status, widgetStateQueue,
enqueueWidgetStateSync, and setWidgetStateQueue to await the promise and handle
success vs error paths instead of clearing immediately.
In
`@mcpjam-inspector/client/src/lib/mcp-ui/__tests__/model-context-messages.test.ts`:
- Around line 1-9: Replace the ad-hoc local mocking in the test with the
repo-wide client mock presets: import and apply the shared mcpApiPresets and
storePresets (from client/src/test/mocks/) in the test's beforeEach setup
instead of the current manual vi.restoreAllMocks/spyOn block; then remove the
local-only mocking of resolveFilePart unless the presets do not already cover
it—if they don't, add a short supplemental mock for
widgetStateMessages.resolveFilePart.mockResolvedValue(null) alongside applying
mcpApiPresets and storePresets.
In
`@mcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.hosted.test.ts`:
- Around line 19-24: Replace the ad-hoc vi.doMock calls that stub
"@/lib/session-token" and "@/lib/config" (the vi.doMock(...) lines) with the
shared test mock presets: import and apply mcpApiPresets and storePresets from
the test mocks and configure them to return authFetchMock and HOSTED_MODE: true
instead of directly mocking those modules; update the test setup to call the
preset helpers (mcpApiPresets, storePresets) so hosted-mode behavior and
authFetch are wired via the central client mocks rather than the inline
vi.doMock calls.
In
`@mcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.test.ts`:
- Around line 1-13: This test defines bespoke mocks (authFetchMock and a local
HOSTED_MODE mock) instead of using the shared client mock presets; replace the
bespoke mocks by importing and applying the shared presets (mcpApiPresets and
storePresets) from the client test mocks and remove the local vi.hoisted
authFetchMock and the manual vi.mock("@/lib/config") call. Specifically, import
mcpApiPresets and storePresets at the top of
openai-widget-state-messages.test.ts and invoke/apply them (the shared presets
export will set up authFetch and config mocks for you) so the suite uses the
canonical client mocks rather than custom fixtures.
In `@mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts`:
- Around line 168-172: The current branch returns literal "undefined"/"null"
when modelContent exists but is unset; update the check in the isRecord(state)
&& "modelContent" in state branch to first guard against modelContent being null
or undefined (e.g., modelContent == null) before treating it as a string—only
return modelContent when typeof modelContent === "string" AND modelContent is
not null/undefined (and not the literal "undefined"/"null" if you prefer
stricter guarding); otherwise fall back to the sanitized model-visible state
path (use safeStringify or the existing fallback logic) so isRecord, state,
modelContent, and safeStringify are used to locate and fix this behavior.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts`:
- Around line 193-196: The loop that resolves uploaded file parts does serial
awaits and should be converted to concurrent resolution: map
extractUploadedFileIds(state) to an array of promises calling
resolveFilePart(fileId) with a catch that returns null, await Promise.all on
that array, then iterate the resulting array and push only non-null results into
parts; update the code around extractUploadedFileIds, resolveFilePart and parts
to use this concurrent pattern so widget-sync latency no longer scales linearly
with number of images.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f41c78c4-cb76-4c21-98db-a386ce8ae894
📒 Files selected for processing (9)
mcpjam-inspector/client/src/components/ChatTabV2.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsxmcpjam-inspector/client/src/hooks/__tests__/use-widget-state-sync.test.tsmcpjam-inspector/client/src/hooks/use-widget-state-sync.tsmcpjam-inspector/client/src/lib/mcp-ui/__tests__/model-context-messages.test.tsmcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.hosted.test.tsmcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.test.tsmcpjam-inspector/client/src/lib/mcp-ui/model-context-messages.tsmcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts
mcpjam-inspector/client/src/hooks/__tests__/use-widget-state-sync.test.ts
Outdated
Show resolved
Hide resolved
mcpjam-inspector/client/src/lib/mcp-ui/__tests__/model-context-messages.test.ts
Show resolved
Hide resolved
mcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.hosted.test.ts
Outdated
Show resolved
Hide resolved
mcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.test.ts
Show resolved
Hide resolved
…sues - switch widget-state-related tests to shared preset-driven client mocks - clear widgetStateQueue only after successful async sync - keep queued updates on sync failure - guard nullish/invalid modelContent fallback behavior - resolve uploaded file parts concurrently
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
mcpjam-inspector/client/src/hooks/__tests__/use-widget-state-sync.test.ts (1)
136-163: Add a queue-flush failure test to lock retry semantics.This suite validates successful flushes, but not the rejection path where queued items must remain for retry. A targeted failure-case test would protect this reliability contract.
Based on learnings, all changes should include tests using Vitest.
🤖 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-widget-state-sync.test.ts` around lines 136 - 163, Add a new vitest test in use-widget-state-sync.test.ts that simulates a failed flush when status transitions to "ready": mock or stub the downstream send/enqueue used inside useWidgetStateSync (e.g., the function that ultimately calls setMessages or the internal enqueue routine referenced by widgetStateSyncRef) to reject, then transition from "streaming" to "ready" via rerender and await the widgetStateSyncRef promise; assert that the queued item added via setWidgetStateQueue remains in the queue (no messages pushed) and that subsequent successful send will clear it, thus locking the retry semantics; reference the hook useWidgetStateSync, methods setWidgetStateQueue and widgetStateSyncRef, and the external setter setMessages when locating where to inject the mock.mcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.test.ts (1)
194-203: Consider relocatingresolveFileParttest to its own describe block.This test exercises
resolveFilePartdirectly, yet resides within thedescribe("buildWidgetStateParts")block. A dedicated siblingdescribe("resolveFilePart")would clarify the test organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.test.ts` around lines 194 - 203, The test exercising resolveFilePart is currently nested inside the describe("buildWidgetStateParts") block; move that it("returns null when all endpoint requests throw") into a new sibling describe("resolveFilePart") block to improve organization and clarity, keeping the same test body and mocks (e.g., clientRuntimeMocks.authFetchMock.mockRejectedValue and the resolveFilePart call) and ensure any shared setup/teardown remains available or is duplicated/adjusted within the new describe.mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts (1)
198-202: Redundant but harmless defensive catch.
resolveFilePartalready returnsnullon failure internally, making the outer.catch(() => null)technically redundant. However, this defensive layer is benign and guards against unforeseen changes toresolveFilePart's error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts` around lines 198 - 202, Redundant defensive .catch: remove the outer .catch(() => null) from the Promise.map so the code becomes Promise.all(extractUploadedFileIds(state).map((fileId) => resolveFilePart(fileId))) — this keeps behavior concise because resolveFilePart already returns null on failure; reference resolveFilePart, extractUploadedFileIds and the fileParts const and ensure tests still pass or add a small unit test verifying fileParts contains nulls on failed resolutions.
🤖 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/test/mocks/client-runtime.ts`:
- Line 36: The test mock merge currently uses
Object.assign(clientRuntimeMocks.mcpApiMock, mcpApi) which mutates a long-lived
clientRuntimeMocks.mcpApiMock and allows keys from prior presets to leak; before
merging, reset or replace clientRuntimeMocks.mcpApiMock (e.g., set it to a fresh
empty object) and then merge the new mcpApi into that fresh object so only keys
present in the current preset remain (locate the call to Object.assign and
update it to reset clientRuntimeMocks.mcpApiMock first).
---
Nitpick comments:
In `@mcpjam-inspector/client/src/hooks/__tests__/use-widget-state-sync.test.ts`:
- Around line 136-163: Add a new vitest test in use-widget-state-sync.test.ts
that simulates a failed flush when status transitions to "ready": mock or stub
the downstream send/enqueue used inside useWidgetStateSync (e.g., the function
that ultimately calls setMessages or the internal enqueue routine referenced by
widgetStateSyncRef) to reject, then transition from "streaming" to "ready" via
rerender and await the widgetStateSyncRef promise; assert that the queued item
added via setWidgetStateQueue remains in the queue (no messages pushed) and that
subsequent successful send will clear it, thus locking the retry semantics;
reference the hook useWidgetStateSync, methods setWidgetStateQueue and
widgetStateSyncRef, and the external setter setMessages when locating where to
inject the mock.
In
`@mcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.test.ts`:
- Around line 194-203: The test exercising resolveFilePart is currently nested
inside the describe("buildWidgetStateParts") block; move that it("returns null
when all endpoint requests throw") into a new sibling
describe("resolveFilePart") block to improve organization and clarity, keeping
the same test body and mocks (e.g.,
clientRuntimeMocks.authFetchMock.mockRejectedValue and the resolveFilePart call)
and ensure any shared setup/teardown remains available or is duplicated/adjusted
within the new describe.
In `@mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts`:
- Around line 198-202: Redundant defensive .catch: remove the outer .catch(() =>
null) from the Promise.map so the code becomes
Promise.all(extractUploadedFileIds(state).map((fileId) =>
resolveFilePart(fileId))) — this keeps behavior concise because resolveFilePart
already returns null on failure; reference resolveFilePart,
extractUploadedFileIds and the fileParts const and ensure tests still pass or
add a small unit test verifying fileParts contains nulls on failed resolutions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1f8c268-ef95-48eb-bdee-95d21ee06d59
📒 Files selected for processing (8)
mcpjam-inspector/client/src/hooks/__tests__/use-widget-state-sync.test.tsmcpjam-inspector/client/src/hooks/use-widget-state-sync.tsmcpjam-inspector/client/src/lib/mcp-ui/__tests__/model-context-messages.test.tsmcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.hosted.test.tsmcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.test.tsmcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.tsmcpjam-inspector/client/src/test/mocks/client-runtime.tsmcpjam-inspector/client/src/test/mocks/widget-state-sync.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/client/src/lib/mcp-ui/tests/model-context-messages.test.ts
- mcpjam-inspector/client/src/lib/mcp-ui/tests/openai-widget-state-messages.hosted.test.ts
… state parts - reset client runtime mcpApi mock before applying each preset to prevent key leakage - remove redundant catch in widget state file-part Promise.all path - reorganize openai-widget-state-messages tests by moving resolveFilePart failure case into a dedicated describe block
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts (1)
198-200: Avoid unbounded parallel blob→dataURL conversions.Lines 198-200 resolve every file concurrently; with many/large images this can cause avoidable memory pressure in the browser.
Proposed refactor (sequential, memory-friendlier)
- const fileParts = await Promise.all( - extractUploadedFileIds(state).map((fileId) => resolveFilePart(fileId)), - ); - - for (const filePart of fileParts) { - if (filePart) parts.push(filePart); - } + for (const fileId of extractUploadedFileIds(state)) { + const filePart = await resolveFilePart(fileId); + if (filePart) parts.push(filePart); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts` around lines 198 - 200, The current code calls resolveFilePart for every ID concurrently via Promise.all over extractUploadedFileIds(state), causing unbounded memory use for many/large files; change this to a sequential (or bounded-concurrency) approach by iterating the array from extractUploadedFileIds(state) and awaiting resolveFilePart(fileId) for each item, pushing results into the fileParts array (instead of using Promise.all). Update the block that defines fileParts so it builds the array incrementally (or uses a simple concurrency limiter) to avoid resolving all blobs at once.
🤖 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/lib/mcp-ui/openai-widget-state-messages.ts`:
- Around line 17-19: The isValidUploadedFileId type guard currently enforces the
wrong pattern; update the validator used in isValidUploadedFileId to accept
OpenAI-style file IDs (prefix "file-" plus opaque alphanumeric/URL-safe chars)
instead of "file_+hex". Replace the regex check in isValidUploadedFileId with
one that matches /^file-[A-Za-z0-9_-]+$/ (or a permissive /^file-[\w-]+$/) so
legitimate OpenAI file IDs are accepted while keeping the typeof value ===
"string" guard.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.ts`:
- Around line 198-200: The current code calls resolveFilePart for every ID
concurrently via Promise.all over extractUploadedFileIds(state), causing
unbounded memory use for many/large files; change this to a sequential (or
bounded-concurrency) approach by iterating the array from
extractUploadedFileIds(state) and awaiting resolveFilePart(fileId) for each
item, pushing results into the fileParts array (instead of using Promise.all).
Update the block that defines fileParts so it builds the array incrementally (or
uses a simple concurrency limiter) to avoid resolving all blobs at once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89deaa39-9593-47cf-88c8-a3af2330e46a
📒 Files selected for processing (3)
mcpjam-inspector/client/src/lib/mcp-ui/__tests__/openai-widget-state-messages.test.tsmcpjam-inspector/client/src/lib/mcp-ui/openai-widget-state-messages.tsmcpjam-inspector/client/src/test/mocks/client-runtime.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/client/src/lib/mcp-ui/tests/openai-widget-state-messages.test.ts
- mcpjam-inspector/client/src/test/mocks/client-runtime.ts
…part resolution - accept non-empty string file IDs in isValidUploadedFileId to support current OpenAI file ID formats - replace Promise.all file-part resolution with sequential resolveFilePart iteration to avoid unbounded concurrent blob loading - keep existing widget-state message behavior and pass targeted mcp-ui/widget-sync test suites
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
2 participants
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.
This PR addresses handling of images from widgets that call updateModelContext and setWidgetState.
The issue
Current when a widget uses updateModelContext or setWidgetState with an image the image is treated as text.
Example of a call using updateModelContext
The payload it generates
{ "jsonrpc": "2.0", "id": 1, "method": "ui/update-model-context", "params": { "content": [ { "type": "text", "text": "User uploaded an image from the file upload widget." }, { "type": "image", "data": "/9j/4AAQSkZJRgABAQAAAQABAAD/..truncated base64 string of image”, "mimeType": "image/jpeg" } ] } }Example of a setWidgetState call
The payload it generates:
{ "type": "openai:setWidgetState", "toolId": "playground-7kZXNqf6UYa5QMDb", "state": { "modelContent": "User uploaded an image from the file upload widget.", "privateContent": { "rawFileId": "file_8a45d63b-79d4-4eff-a178-c0ce4078c7c6" }, "imageIds": [ "file_8a45d63b-79d4-4eff-a178-c0ce4078c7c6" ] } }The fix
Two new modules were introduced to handle image content properly in both paths.
openai-widget-state-messages.tsbuildWidgetStatePartsnow returns a mixedpartsarray: a text summary followed by one{ type: "file" }part per image, rather than a single flat text string.extractUploadedFileIdsreadsimageIdsfrom widget state (the canonical Apps SDK field) andresolveFilePartfetches each ID from the local file endpoint, converting the blob to a base64 data URL so it can be attached as afilepart.buildWidgetStateTextstripsprivateContentbefore serialising state to text, so UI-only data is never sent to the model.model-context-messages.tscontextToSyncPartsiteratescontent[]and maps eachimage(oraudio)ContentBlockto a{ type: "file", mediaType, url }part instead of ignoring or stringifying it.contextToPartshandles the ChatGPT extension path: it resolvesimageIdsviaresolveFilePart, and falls back to HTTP image URLs fromstructuredContent.privateContent.selectedImagesonly when no file IDs could be resolved (to avoid duplicates).structuredContentcontaining onlyprivateContent) are filtered out before being sent to the model.Testing
I tested the fix by using my upload-mcp which just sending the image and a small bit of text to the model. A version of it is currently deploy at https://upload-mcp-def1fcb1.alpic.live/ if you need to test it out.
Tested with various models