UI preview image#339
Conversation
Use a PowerShell encoded invocation that passes the selected file path as a single explorer argument, preventing cmd metacharacter injection while preserving /select behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
…erMCP into pr/file-preview-core-tracking
Store absolute file paths in structured content so the "Open in folder" button works correctly with relative paths (~, ./, etc). Previously, paths like ~/file.txt or ./file.txt would fail because: - Tilde is not expanded inside shell single quotes - Relative paths depend on the MCP server's current working directory Co-authored-by: Cursor <cursoragent@cursor.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughAdds image preview support and absolute local-path resolution: filesystem handler expands home and resolves absolute paths, types gain imageData/mimeType, UI app and toolbar render image previews via data URLs, new image MIME utilities added, styles and tests updated for SVG/image handling. Changes
Sequence DiagramsequenceDiagram
participant User
participant Handler as Filesystem Handler
participant App as File Preview App
participant UI as Renderer/DOM
User->>Handler: Request file preview
Handler->>Handler: expandHome() & resolveAbsolutePath()
Handler->>Handler: detect file type (image/pdf/text/html)
alt Local image
Handler->>Handler: read bytes, normalize mime, encode imageData
Handler->>App: return FilePreviewStructuredContent (fileType: image, imageData, mimeType, resolved filePath)
else Remote URL or other
Handler->>App: return structuredContent (url or text/html/unsupported)
end
App->>App: branch on payload.fileType
alt image
App->>App: validate mime with isAllowedImageMimeType()
App->>UI: render <img> with data URL from imageData
else other
App->>UI: render markdown/text/html or unsupported view
end
UI->>User: display preview and toolbar (copy/open actions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
codeant-ai
Bot
added
the
size:L
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/handlers/filesystem-handlers.ts`:
- Around line 38-43: The expandHome function erroneously expands paths like
"~otheruser/file"; restrict expansion to only the standalone "~" and the
current-user prefix by checking filePath === '~' or that it starts with "~/" or
"~" + path.sep, then replace only that leading "~" with os.homedir(); update the
logic inside expandHome (refer to function name expandHome and use path and os
imports) to perform this stricter check and return the original filePath
unchanged for "~user/..." forms.
In `@test/test-file-handlers.js`:
- Around line 344-354: The SVG test block is missing assertions for
structuredContent.filePath and the nested _meta.ui.resourceUri like the image
test; update the SVG assertions after calling handleReadFile (svgResult) to
assert svgResult.structuredContent.filePath equals the expected
FILE_PREVIEW_RESOURCE_URI/path value and assert svgResult._meta.ui &&
svgResult._meta.ui.resourceUri (or svgResult._meta['ui/resourceUri']) equals
FILE_PREVIEW_RESOURCE_URI, using the same assertion style (assert.strictEqual /
assert.ok) as the image test to keep parity with handleReadFile output
validation.
🧹 Nitpick comments (3)
src/ui/file-preview/src/components/toolbar.ts (1)
8-24:inferFilePilldoesn't have an explicit case forfileType === 'image'— it falls through to extension-based labeling.This works fine in practice (e.g.,
.png→ "PNG",.svg→ "SVG"), but if an image file has no extension or a long one, the fallbackextension.slice(0, 4).toUpperCase()might produce an unclear label. Consider whether an explicit'image'case (e.g., label: "IMG") would be more consistent with the markdown/html branches.src/ui/file-preview/src/app.ts (2)
253-258: Minor:cleanedContentis computed but unused for image payloads.
stripReadStatusLineon line 254 runs for image file types even though the result is never used (image branch returns early on line 257). Consider moving the computation below the image check.♻️ Suggested reorder
function renderBody(payload: FilePreviewStructuredContent, htmlMode: HtmlPreviewMode, startLine = 1): { html: string; notice?: string } { - const cleanedContent = stripReadStatusLine(payload.content); - if (payload.fileType === 'image') { return renderImageBody(payload); } + const cleanedContent = stripReadStatusLine(payload.content); + if (payload.fileType === 'unsupported') {
642-654: Footer label shows line count for image files, which may be confusing.For image payloads,
lineCountis computed frompayload.content(the text description), so the footer reads something like "IMAGE • 1 LINE". This might confuse users who expect image metadata (e.g., dimensions) rather than a description line count. Consider suppressing the line count for image file types.♻️ Suggested change
const footerLabel = range?.isPartial ? `${escapeHtml(fileTypeLabel)} • LINES ${range.fromLine}–${range.toLine} OF ${range.totalLines}` - : `${escapeHtml(fileTypeLabel)} • ${lineCount} LINE${lineCount !== 1 ? 'S' : ''}`; + : payload.fileType === 'image' + ? escapeHtml(fileTypeLabel) + : `${escapeHtml(fileTypeLabel)} • ${lineCount} LINE${lineCount !== 1 ? 'S' : ''}`;
| const svgResult = await handleReadFile({ path: SVG_FILE }); | ||
| assert.ok(Array.isArray(svgResult.content), 'SVG result should include content array'); | ||
| assert.ok(!svgResult.content.some((item) => item.type === 'image'), 'SVG result should avoid image content item for host compatibility'); | ||
| assert.ok(svgResult.structuredContent, 'SVG should include structuredContent'); | ||
| assert.strictEqual(svgResult.structuredContent.fileType, 'image', 'SVG should map to image preview state'); | ||
| assert.strictEqual(svgResult.structuredContent.mimeType, 'image/svg+xml', 'SVG structured payload should include SVG mimeType'); | ||
| assert.strictEqual(typeof svgResult.structuredContent.imageData, 'string', 'SVG structured payload should include imageData'); | ||
| assert.ok(svgResult.structuredContent.imageData.length > 0, 'SVG structured payload should include non-empty imageData'); | ||
| assert.strictEqual(svgResult._meta['ui/resourceUri'], FILE_PREVIEW_RESOURCE_URI, 'SVG should include ui/resourceUri'); | ||
| assert.strictEqual(svgResult._meta['openai/widgetAccessible'], true, 'SVG should enable widget accessibility'); | ||
|
|
There was a problem hiding this comment.
SVG test block is missing filePath and nested ui.resourceUri assertions that the image block includes.
The image test (lines 339–342) validates structuredContent.filePath and _meta.ui.resourceUri, but the SVG block omits both. This creates an inconsistency and could mask regressions in the SVG path.
Suggested additions
assert.ok(svgResult.structuredContent.imageData.length > 0, 'SVG structured payload should include non-empty imageData');
+ assert.strictEqual(svgResult.structuredContent.filePath, SVG_FILE, 'SVG file path should be present');
assert.strictEqual(svgResult._meta['ui/resourceUri'], FILE_PREVIEW_RESOURCE_URI, 'SVG should include ui/resourceUri');
+ assert.strictEqual(svgResult._meta.ui.resourceUri, FILE_PREVIEW_RESOURCE_URI, 'SVG should include preview resource URI');
assert.strictEqual(svgResult._meta['openai/widgetAccessible'], true, 'SVG should enable widget accessibility');🤖 Prompt for AI Agents
In `@test/test-file-handlers.js` around lines 344 - 354, The SVG test block is
missing assertions for structuredContent.filePath and the nested
_meta.ui.resourceUri like the image test; update the SVG assertions after
calling handleReadFile (svgResult) to assert
svgResult.structuredContent.filePath equals the expected
FILE_PREVIEW_RESOURCE_URI/path value and assert svgResult._meta.ui &&
svgResult._meta.ui.resourceUri (or svgResult._meta['ui/resourceUri']) equals
FILE_PREVIEW_RESOURCE_URI, using the same assertion style (assert.strictEqual /
assert.ok) as the image test to keep parity with handleReadFile output
validation.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| if (import.meta.url === `file://${process.argv[1]}`) { |
There was a problem hiding this comment.
Suggestion: The equality check between import.meta.url and file://${process.argv[1]} will almost never be true because import.meta.url is an absolute file:///... URL while process.argv[1] is typically a plain path (often relative), so the tests will not auto-run when the file is executed directly with Node. You should normalize process.argv[1] to a file:// URL in the same way Node does (e.g., resolving it against the current working directory) before comparing it to import.meta.url. [logic error]
Severity Level: Major ⚠️
- ⚠️ Direct CLI execution of runtime tests does nothing.
- ⚠️ Developers may think tests ran when they did not.
- ⚠️ Automated scripts invoking this file directly skip tests.| if (import.meta.url === `file://${process.argv[1]}`) { | |
| if (process.argv[1] && import.meta.url === new URL(process.argv[1], `file://${process.cwd()}/`).href) { |
Steps of Reproduction ✅
1. From the project root, run the test file directly with Node: `node
test/test-file-preview-image-runtime.js` (this file is
`test/test-file-preview-image-runtime.js:1-66`).
2. At module evaluation, Node sets `import.meta.url` to an absolute file URL like
`file:///ABS_PATH/test/test-file-preview-image-runtime.js` (see bottom guard at
`test/test-file-preview-image-runtime.js:59-66`).
3. In the same process, `process.argv[1]` is the path argument passed to Node, typically a
plain (often relative) path such as `test/test-file-preview-image-runtime.js`, so the
template literal produces `file://test/test-file-preview-image-runtime.js`.
4. Because `import.meta.url` (`file:///ABS_PATH/...`) and ``file://${process.argv[1]}``
(`file://test/...`) differ, the condition `import.meta.url ===
\`file://${process.argv[1]}\`` at line 59 is false, the `runTests()` block is never
executed, and the runtime tests do not auto-run when the script is invoked directly.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** test/test-file-preview-image-runtime.js
**Line:** 59:59
**Comment:**
*Logic Error: The equality check between `import.meta.url` and ``file://${process.argv[1]}`` will almost never be true because `import.meta.url` is an absolute `file:///...` URL while `process.argv[1]` is typically a plain path (often relative), so the tests will not auto-run when the file is executed directly with Node. You should normalize `process.argv[1]` to a `file://` URL in the same way Node does (e.g., resolving it against the current working directory) before comparing it to `import.meta.url`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/handlers/filesystem-handlers.ts (1)
154-154: Minor path inconsistency:imageSummaryuses rawparsed.pathwhile structuredContent usesresolvedFilePath.For consistency, consider using
resolvedFilePathin the summary string as well (same applies to the PDF summary on line 133). Currently a user could see~/photo.pngin the text content but/home/user/photo.pngin the structured metadata.Suggested fix
- const imageSummary = `Image file: ${parsed.path} (${fileResult.mimeType})\n`; + const imageSummary = `Image file: ${resolvedFilePath} (${fileResult.mimeType})\n`;And for the PDF case (line 133):
- text: `PDF file: ${parsed.path}${author}${title} (${meta?.totalPages} pages) \n` + text: `PDF file: ${resolvedFilePath}${author}${title} (${meta?.totalPages} pages) \n`
2 participants
User description
read_fileusingstructuredContent(fileType: image,imageData,mimeType).contenttext-only for all images (better host compatibility).CodeAnt-AI Description
Add image previews and absolute file paths for file preview UI
What Changed
Impact
✅ Preview images render in the widget from structured payloads✅ Open in folder works with ~/ and relative paths✅ Clearer behavior for unsupported/missing image previews💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Improvements
Tests