feat(client): render viewed images in expanded tool entries - #7724
feat(client): render viewed images in expanded tool entries#7724SunkenInTime wants to merge 10 commits into
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
UI consistency review found one keyboard-interaction regression and two smaller ownership/consistency issues in the changed web UI. Details inline.
Posted via Macroscope — UI Consistency
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces a new feature enabling workspace image rendering in markdown and expanded tool entries across mobile and web. The scope includes new components, path resolution logic, and loading states — warranting human review for this new capability. You can add or adjust custom eligibility rules. Learn more. |
Agents reference screenshots they saved into the workspace by path (relative, absolute, or file://). Those bytes only exist on the environment host, so both clients rendered a broken or empty box. Web: a custom img renderer resolves workspace paths through the signed workspace-file asset endpoint; remote http(s)/data images keep their existing inline rendering. iOS: the t3-markdown-text module accepts a renderImage hook (provided via context) and ThreadFeed supplies a component that resolves paths against the workspace root, loads via useAssetUrl, and supports tap-to-expand into the existing full-screen viewer. Android: the nitro-markdown fallback path registers an image renderer backed by the same component.
- Surface asset-URL failures as 'Image unavailable' instead of an indefinite spinner (useAssetUrlState on mobile mirrors web's states). - Normalize file:///C:/... drive URIs (drop the URL parser's leading slash) and map file://host/share URIs to UNC paths. - Treat protocol-relative //cdn... image srcs as directly loadable on web and non-workspace on mobile.
…play Review bot follow-ups: the mobile markdown-image parser rewrote the historical file://localhost/... spelling into a UNC path, and the web workspace image's 'block' utility lost to the unlayered .chat-markdown img rule, so its display flipped when loading finished. Also announce the loading placeholder to assistive tech via role=status.
Expanding a 'Viewed (image)' or image-read tool call previously showed only the file path as text. Read-classified entries whose detail is a single workspace image path now render the image itself above the text detail, loaded through the same signed workspace-file asset URL the chat markdown images use. Web rows open the existing lightbox on click; mobile reuses ThreadMarkdownImage with its fullscreen viewer.
Review bot follow-ups: keydown on the preview button bubbled to the row's role=button handler, whose preventDefault cancelled the button's click activation — Enter/Space collapsed the row instead of opening the lightbox. Stop key events at the expanded-body wrapper like pointer events, give the button a focus-visible ring, and swap the hand-rolled loading placeholder for the shared Skeleton with role=status.
94d9de4 to
cb07d87
Compare
There was a problem hiding this comment.
One finding on the new expanded-row image preview: the blanket onKeyDown={stopRowToggle} also swallows window-level keyboard shortcuts while the preview button is focused. Details inline.
Posted via Macroscope — UI Consistency
| // Keys pressed on the expanded body (e.g. Enter on the image | ||
| // preview button) must not reach the row's toggle handler — its | ||
| // preventDefault would also cancel the button's click activation. | ||
| onKeyDown={stopRowToggle} |
There was a problem hiding this comment.
stopRowToggle stops every keydown, not just the row's toggle keys. React dispatches from the root container and SyntheticEvent.stopPropagation() also calls nativeEvent.stopPropagation(), so while the new preview button has focus no keydown reaches the bubble-phase window listeners — e.g. the command-palette keybinding (CommandPalette.tsx), the thread-jump shortcuts (Sidebar.tsx), and the _chat.tsx window handler all stop working until focus leaves the row.
Suggest narrowing the stop to the keys the row actually toggles on (or, equivalently, having the row handler ignore events whose target is not currentTarget) so unrelated shortcuts keep bubbling:
| // Keys pressed on the expanded body (e.g. Enter on the image | |
| // preview button) must not reach the row's toggle handler — its | |
| // preventDefault would also cancel the button's click activation. | |
| onKeyDown={stopRowToggle} | |
| // Enter/Space pressed inside the expanded body (e.g. the image | |
| // preview button) must not reach the row's toggle handler — its | |
| // preventDefault would also cancel the button's click activation. | |
| // Other keys keep bubbling so window-level shortcuts still fire. | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") e.stopPropagation(); | |
| }} |
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cb07d87. Configure here.
| const rawPath = parsed.pathname; | ||
| const rawPath = parsed.hostname | ||
| ? `\\\\${parsed.hostname}${parsed.pathname.replaceAll("/", "\\")}` | ||
| : parsed.pathname; |
There was a problem hiding this comment.
Drive file URLs become UNC
Medium Severity
file://C:/... style Windows drive URLs are treated as UNC hosts. WHATWG parses the drive letter as hostname, and both the web parseFileUrlHref UNC branch and mobile markdownImageWorkspacePath authority branch rewrite it to \\C\.... Workspace image loads then request the wrong path and fail, while file:///C:/... (three slashes) works.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cb07d87. Configure here.
1 participant
Stacked on #6433 — only the commits after the parent PR are new; it builds on the signed workspace-file asset URLs introduced there.
Problem
Expanding a "Viewed (image)" / image-read tool call in the timeline only showed the file path as text — the image the agent looked at was never visible (raised by maria on Discord as a follow-up to #6433).
Fix
Read-classified tool entries (
image_view,file-read, Cursor's "Read File") whose detail is a single workspace image path now render the image itself above the text detail when the row is expanded:ToolCallExpandedImageinMessagesTimelineloads the image through the signed workspace-file asset URL, only while the row is expanded (collapsed rows never fetch). Clicking opens the existing image lightbox. On load failure it renders nothing — the path stays visible in the text body.ThreadFeedActivityrows carry aviewedImagePath, and the expanded work-log detail reusesThreadMarkdownImage(loading/unavailable states, fullscreen viewer on tap) via a render prop fromThreadFeed.The image-path predicate reuses
isWorkspaceImagePreviewPathfrom@t3tools/shared, so the client only attempts extensions the asset route will actually serve. Unit tests cover the web predicate; the mobile activity fixture gained the new field.Web
Before
After
Mobile (iOS Simulator)
Before
After
Built with Claude Fable 5 on Claude Code.
Note
Medium Risk
Touches markdown image rendering and rehype-sanitize (adds
fileonsrc) plus signed workspace-file fetches. Remote images still load as normal<img>; only workspace paths go through the asset route.Overview
Workspace-file images now show in chat markdown and in expanded read/view tool rows, loaded through signed asset URLs instead of raw paths.
Tool entries:
workEntryViewedImagePathpicks a single-line image path from read/view work (image_view,file-read, “Read File”). Web’s expanded timeline row rendersToolCallExpandedImage(lightbox on click; fetch only while expanded). Mobile work-log rows carryviewedImagePathand reuseThreadMarkdownImage.Markdown: Web
ChatMarkdownresolves relative, absolute, andfile://srcs (including Windows drive and UNC) viaChatMarkdownWorkspaceImage, with loading/unavailable UI. Sanitizer now allowsfileonsrcand rewrites drive-absolute HTMLsrcbefore sanitize. Mobile native markdown gains an optionalrenderImagecontext so the host can swap in signed-URL images; remotehttp(s)still uses the defaultImage.useAssetUrlStateon mobile exposes Loading/Failure/Success so those UIs can distinguish spinner vs unavailable.Reviewed by Cursor Bugbot for commit cb07d87. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Render workspace images in expanded tool entries and markdown
workEntryViewedImagePathin threadActivity.ts and MessagesTimeline.logic.ts to extract a workspace image path from read/view work log entries when the detail is a single-line image path.SelectableMarkdownTextgains an optionalrenderImageprop propagated viaMarkdownImageRendererContext;NativeMarkdownImageuses it to render custom nodes.<img>rendering to resolve workspace paths (relative, absolute,file://) throughChatMarkdownWorkspaceImage. AddsrehypeNormalizeWindowsImageSrcto rewrite drive-absolutesrcvalues tofile:///URLs before sanitization, and permits thefileprotocol inCHAT_MARKDOWN_SANITIZE_SCHEMA.useAssetUrlStatein assets.ts (mobile) returning aLoading | Failure | Successunion;useAssetUrlis refactored to delegate to it.markdownComponentsimg renderer in ChatMarkdown.tsx changes default image rendering for all markdown; non-workspacesrcvalues still render as normal<img>, but Windows drive-absolute paths are now rewritten tofile:///URLs, andfileprotocol is newly allowed byCHAT_MARKDOWN_SANITIZE_SCHEMA.Macroscope summarized cb07d87.