Render generated images inline in chat#3984
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| function generatedImageReference(value: string): string | null { | ||
| const reference = value.trim(); | ||
| const segments = reference.split("/"); |
There was a problem hiding this comment.
🟡 Medium components/ChatMarkdown.tsx:896
generatedImageReference splits the reference only on /, so a Windows-style path like images\1.jpg produces a single segment and is rejected even though the server accepts and normalizes that same artifact path. This causes inline generated-image rendering to fail for Windows-style references. Consider normalizing \ to / before splitting and validating, matching the server's behavior.
| function generatedImageReference(value: string): string | null { | |
| const reference = value.trim(); | |
| const segments = reference.split("/"); | |
| function generatedImageReference(value: string): string | null { | |
| const reference = value.trim().replace(/\\/g, "/"); | |
| const segments = reference.split("/"); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ChatMarkdown.tsx around lines 896-898:
`generatedImageReference` splits the reference only on `/`, so a Windows-style path like `images\1.jpg` produces a single segment and is rejected even though the server accepts and normalizes that same artifact path. This causes inline generated-image rendering to fail for Windows-style references. Consider normalizing `\` to `/` before splitting and validating, matching the server's behavior.
There was a problem hiding this comment.
Addressed in 391fa4da8: normalize generated-image path separators before validation.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces a new user-facing feature (inline image rendering) with new components and server-side logic. Additionally, there are multiple unresolved review comments identifying bugs including Windows path handling issues, broken fenced code block rendering, and incorrect artifact correlation within turns. You can customize Macroscope's approvability policy. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a679e683b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const rawOutput = asUnknownRecord(data?.rawOutput); | ||
| const artifactPath = rawOutput?.path; |
There was a problem hiding this comment.
When the matching activity comes from Codex app-server image generation, CodexAdapter stores the raw item payload as payload.data (see apps/server/src/provider/Layers/CodexAdapter.ts:476-484), and the generated file is exposed as data.item.savedPath for imageGeneration (or data.item.path for imageView), not as data.rawOutput.path. With this lookup limited to rawOutput.path, a valid assistant code span such as images/1.jpg never matches Codex's generated-image activity, so assets.createUrl returns not found and the primary Codex generated images stay rendered as plain code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2c613968b: resolve Codex imageGeneration.savedPath and imageView.path values.
| code({ node, className: codeClassName, children, ...props }) { | ||
| const artifactReference = codeClassName | ||
| ? null | ||
| : generatedImageReference(plainHastText(node) ?? ""); |
There was a problem hiding this comment.
With this code component registered, fenced-code children passed to the pre renderer are React elements of this function rather than the literal "code" element that extractCodeBlock requires (onlyChild.type !== "code"). For any fenced code block, the pre renderer falls back to raw <pre> and skips the existing MarkdownCodeBlock/Shiki path, removing code-block chrome and highlighting; if a fence contains an image-looking path it can also render an artifact preview inside the block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in bd0756a80: add regression coverage for enhanced fenced-code rendering.
| const reference = value.trim(); | ||
| const segments = reference.split("/"); |
There was a problem hiding this comment.
For Windows-style relative artifact references such as images\\1.jpg, this client matcher splits only on /, so it returns null before the server-side backslash normalization can run and the image remains plain code. The server accepts backslashes in normalizeThreadArtifactReference, so normalize them here too to keep generated images working when providers format relative paths with Windows separators.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0dcfdaf6d: add focused Windows-path normalization coverage.
| for (const activity of [...thread.activities].toReversed()) { | ||
| if (activity.turnId !== turnId) continue; |
There was a problem hiding this comment.
When a single turn generates the same relative artifact name more than once (for example two image-generation calls that both report images/1.jpg from different session directories), this reverse scan always picks the last activity in that turn for every assistant message carrying that turnId. Earlier messages in the turn then resolve to a later artifact, so the UI can show the wrong generated image; the lookup needs a message/activity ordering discriminator rather than turnId alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c6a0a2a70: correlate artifact lookup to the assistant message's activity window.
| for (const activity of thread.activities) { | ||
| if (activity.turnId !== turnId) continue; | ||
| const createdAt = Date.parse(activity.createdAt); | ||
| if (Number.isNaN(createdAt) || createdAt <= window.after || createdAt > window.before) continue; |
There was a problem hiding this comment.
Activity window excludes mid-stream tools
Medium Severity
The artifactMessageWindow function uses an assistant message's createdAt as the upper bound for matching activities. For streaming messages, this createdAt is from the first chunk. Activities, like image generation, that occur later in the same streaming message have a createdAt after this bound, causing findThreadArtifactPath to incorrectly exclude them and preventing inline previews from resolving.
Reviewed by Cursor Bugbot for commit fdf4006. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 43f9e5e. Configure here.
| input.resource.turnId, | ||
| input.resource.messageId, | ||
| input.resource.path, | ||
| ); |
There was a problem hiding this comment.
Non-transactional thread detail read
Medium Severity
The new thread-artifact branch loads thread messages and activities via getThreadDetailById, which issues several parallel projection queries without a single sql.withTransaction. A projector update between those reads can yield a mismatched message list and activity list, so artifact correlation may fail or briefly match the wrong window.
Triggered by learned rule: Wrap related projection snapshot reads in a single transaction
Reviewed by Cursor Bugbot for commit 43f9e5e. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43f9e5ea14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const activity of thread.activities) { | ||
| if (activity.turnId !== turnId) continue; | ||
| const createdAt = Date.parse(activity.createdAt); | ||
| if (Number.isNaN(createdAt) || createdAt <= window.after || createdAt > window.before) continue; |
There was a problem hiding this comment.
When an image-generating activity is the first item after a previous assistant segment but both provider notifications land in the same millisecond, this exclusive lower bound drops it (createdAt <= window.after). The projection stores assistant updatedAt and activity createdAt from provider event timestamps, so same-ms adjacent events are possible under load; then findThreadArtifactPath returns null even though the path belongs to the next assistant message, leaving the generated image as plain code. Use event sequence/tie-breaking, or otherwise distinguish equality, instead of rejecting it here.
Useful? React with 👍 / 👎.
| const data = asUnknownRecord(payload?.data); | ||
| const rawOutput = asUnknownRecord(data?.rawOutput); | ||
| const item = asUnknownRecord(data?.item); | ||
| const paths = [rawOutput?.path]; |
There was a problem hiding this comment.
When an ACP provider/tool reports an arbitrary rawOutput.path, it is copied directly into payload.data.rawOutput (see apps/server/src/provider/acp/AcpRuntimeModel.ts:345-347), and this resolver now authorizes that path as a thread artifact if the assistant later mentions the matching image basename. In that scenario a tool activity can cause assets.createUrl to sign and serve any readable local image file outside the workspace/session directories, not just generated images. Please restrict this to trusted generated-image item shapes or validate the resolved path is under the provider's artifact/session storage.
Useful? React with 👍 / 👎.
1 participant
Summary
Why
Generated images currently appear as code-formatted paths such as
images/1.jpg, so users must leave the conversation to inspect them. These files live in provider session storage rather than the project workspace or managed attachment store.The new thread-artifact request carries the thread, turn, and relative image path needed to authorize the matching projected tool output. After that authorization, the server delegates signing and serving to the existing exact workspace-file asset implementation. Turn correlation prevents an older message from resolving a newer artifact with the same relative path.
Validation
pnpm exec vp checkpnpm exec vp run typecheckpnpm exec vp test apps/server/src/assets/AssetAccess.test.ts apps/web/src/components/chat/MessagesTimeline.test.tsx(19 tests passed)Note
Render generated images inline in chat as expandable thumbnails
thread-artifactvariant toAssetResourceand a newassetsCreateUrlRPC handler that resolves artifact paths from thread activity history within the assistant message time window.normalizeGeneratedImageReference(client) andnormalizeThreadArtifactReference(server) to validate and normalize inline code image references.ChatMarkdownto detect inline code containing valid image references and render them as lazy-loaded thumbnails via a newMarkdownThreadArtifactImagecomponent; clicking opens the expanded image view.AssistantTimelineRowpasses turn/message context toChatMarkdownto scope artifact resolution.<code>rendering.Macroscope summarized 43f9e5e.
Note
Medium Risk
New authorization path maps client-supplied paths to filesystem locations via activity correlation; ambiguous matches are rejected, but incorrect windowing could deny or mis-associate images.
Overview
Assistant messages can show generated image paths (e.g.
`images/1.jpg`) as inline thumbnails instead of plain code, with click-to-expand via the existing lightbox.A new
thread-artifactAssetResourcecarriesthreadId,turnId,messageId, and a relative image path.assetsCreateUrlloads thread detail, runsfindThreadArtifactPathto match the reference against projected tool activities within the assistant message time window (CodexrawOutput,imageGeneration,imageView), and fails closed on ambiguous suffix matches. A signed URL is then issued through the existing exactworkspace-filepath under the resolved session directory.issueAssetUrlno longer acceptsthread-artifactdirectly.ChatMarkdowndetects un-fenced inline code that passesnormalizeGeneratedImageReference, requests URLs viaMarkdownThreadArtifactImage, andMessagesTimelinepasses turn/message context.extractCodeBlockis adjusted so fenced blocks still use the enhanced code path.Reviewed by Cursor Bugbot for commit 43f9e5e. Bugbot is set up for automated code reviews on this repo. Configure here.