fix(vscode): open sent-message images in an editor tab - #13683
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous Review Summary (commit d5cdf64)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit d5cdf64)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by grok-4.6 · Input: 104.6K · Output: 8.6K · Cached: 244K Review guidance: REVIEW.md from base branch |
| onImageClick={(dataUrl, filename) => { | ||
| if (!dataUrl.startsWith("data:")) return false | ||
| vscode.postMessage({ type: "previewImage", dataUrl, filename: filename ?? "image" }) | ||
| return true |
There was a problem hiding this comment.
This may cause a regression with certain data: URLs: we suppress the modal, but the host can't decode the image, so neither preview opens. Here we should keep the modal fallback for URLs the host doesn't support.
There was a problem hiding this comment.
Good catch — fixed in 7aff213. The guard was too broad: startsWith("data:") claimed URLs that the host's parseImage rejects (non-base64 data URLs, non-image mime types), so the modal was suppressed and nothing opened at all.
I pulled the host's accept rule into src/shared/image-data-url.ts and now use it on both sides: parseImage matches against it, and the webview only claims the click when imageMime(dataUrl) is truthy. Everything else falls through to the modal as before.
Sharing one matcher instead of duplicating the check keeps the two sides from drifting apart later. tests/unit/image-preview.test.ts now asserts that imageMime accepts exactly the URLs parseImage can decode, so a future change to one without the other fails the test.
| onRevert={props.onRevert} | ||
| onImageClick={(dataUrl, filename) => { | ||
| if (!dataUrl.startsWith("data:")) return false | ||
| vscode.postMessage({ type: "previewImage", dataUrl, filename: filename ?? "image" }) |
There was a problem hiding this comment.
An empty filename isn't handled by filename ?? "image". The host will reject the empty string, and the modal fallback has already been skipped at this point. I'd suggest using filename || "image".
There was a problem hiding this comment.
Fixed in 7aff213 — switched to filename || "image".
You're right that ?? left a real gap: the host's previewImage handler guards with if (message.dataUrl && message.filename), so an empty string was dropped there while the webview had already returned true and skipped the modal.
|
@johnnyeric I have updated the PR as per your feedback. Feel free to go through this again whenever you have time :) |
|
Thanks @sylwester-liljegren for the contribution! I was able to reproduce the fix. Merged. |
2 participants
Issue
No linked issue — small behavior inconsistency found while using the sidebar chat.
Context
Clicking an image attached in the prompt input opens it in a VS Code image preview editor tab, but clicking the same image after the message is sent opens it in a webview modal instead. The modal is small, cannot be zoomed or resized like the editor preview, and blocks the chat while it is open. This change makes both click targets behave the same way.
Implementation
UserMessageDisplayin@kilocode/kilo-uiopened image attachments throughdialog.show(<ImagePreview />)unconditionally. That component is shared with non-VS Code clients (TUI/session UI) where an editor tab does not exist, so the modal cannot simply be removed.Instead the component now accepts an optional
onImageClick(url, filename)handler. Returningtruemeans the host handled the click; returningfalse(or not passing the prop at all) keeps the existing modal, so every other consumer is unchanged.VscodeUserMessagepasses that handler and posts the already-existingpreviewImagewebview message — the same message the prompt input's attachment thumbnails send. The extension host path (handleEditorAction→previewImageinsrc/kilo-provider/editor-actions.ts) is reused as-is; no new message type, host handler, or extension command was added.The handler only claims
data:URLs, matching what the host-sideparseImagecan decode; anything else falls back to the modal rather than silently doing nothing.Screenshots / Video
How to Test
Manual/local verification
Performed by the agent:
bun run typecheckinpackages/kilo-vscode/— passed (bothcheck-typesandcheck-types:webview)bun run lintinpackages/kilo-vscode/— passedbun run formatinpackages/kilo-vscode/— no reformatting of the touched filesbun test tests/unit/image-preview.test.tsinpackages/kilo-vscode/— 9 pass, covering the host-sidepreviewImagepath this change now reusesReviewer test steps
Blocked checks and substitute verification
pre-pushhook's fullbun turbo typecheckcould not complete locally:@kilocode/kilo-jetbrains#typecheckfails because Gradle cannot find a JDK 21 toolchain on this machine (only Java 24 is installed). All 29 JS/TS typecheck tasks in that same run passed, includingkilo-code, and this change does not touch the JetBrains plugin. Substitute verification was the package-leveltypecheck,lint, and unit test run listed above.Checklist
Get in Touch