Repository navigation
Conversation
|
@luoxi is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds local-file image support for markdown: URL classification, a LocalFileImageProvider with mtime-keyed NSImage caching and cancellation-safe async loading, integration into MarkdownPanelView, a localized missing-image string, Xcode project entries, and unit tests for loader and cache. Changes
Sequence DiagramsequenceDiagram
participant MarkdownView
participant LocalFileImageProvider
participant LocalFileImageLoader
participant LocalFileImageCache
participant FileSystem
MarkdownView->>LocalFileImageProvider: request image(url)
LocalFileImageProvider->>LocalFileImageLoader: classify(url)
LocalFileImageLoader-->>LocalFileImageProvider: Kind(.local | .remote | .unsupported)
alt remote
LocalFileImageProvider->>LocalFileImageProvider: delegate to DefaultImageProvider
LocalFileImageProvider-->>MarkdownView: rendered remote image
else local
LocalFileImageProvider->>LocalFileImageCache: key = key(for: fileURL)?
LocalFileImageCache-->>LocalFileImageProvider: cached image or nil
alt cache miss
LocalFileImageProvider->>FileSystem: load NSImage(contentsOf: fileURL)
FileSystem-->>LocalFileImageProvider: NSImage or error
LocalFileImageProvider->>LocalFileImageCache: store(image, key)
end
LocalFileImageProvider-->>MarkdownView: rendered local image or placeholder
else unsupported/missing
LocalFileImageProvider-->>MarkdownView: show placeholder + localized label
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/LocalFileImageProvider.swift">
<violation number="1" location="Sources/Panels/LocalFileImageProvider.swift:32">
P2: Local image state is reused across URL changes, allowing a stale previously loaded image to flash before the new load task resets state.</violation>
<violation number="2" location="Sources/Panels/LocalFileImageProvider.swift:64">
P2: Detached image-loading task is not cancellation-linked to the SwiftUI lifecycle task, so stale decode/I/O can continue after view task cancellation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR adds local image rendering to the Markdown viewer by introducing Confidence Score: 5/5Safe to merge; all remaining findings are non-blocking style suggestions. The core logic is correct: URL classification handles all cases (file/http/https/relative/schemeless/unsupported), percent-encoding and query/fragment stripping work correctly, the cache key uses path + mtime to avoid stale bitmap reuse, and state transitions in LocalFileImageView are sound. Both findings are P2: the Task.detached cancellation gap is a minor performance concern (not a correctness bug), and the pbxproj ID format is a stylistic deviation that doesn't affect build correctness. GhosttyTabs.xcodeproj/project.pbxproj (non-standard object IDs) and Sources/Panels/LocalFileImageProvider.swift (Task.detached cancellation). Important Files Changed
Sequence DiagramsequenceDiagram
participant MPV as MarkdownPanelView
participant MUI as MarkdownUI
participant LFIP as LocalFileImageProvider
participant LFIL as LocalFileImageLoader
participant LFIView as LocalFileImageView
participant Cache as LocalFileImageCache
participant FS as FileSystem
MPV->>MUI: Markdown(content, imageBaseURL)
MPV->>MUI: markdownImageProvider(LocalFileImageProvider)
MUI->>LFIP: makeImage(url)
LFIP->>LFIView: render LocalFileImageView
LFIView->>LFIL: classify(url)
alt local file
LFIL-->>LFIView: .local(fileURL)
LFIView->>Cache: key(for fileURL) - stat mtime
Cache->>FS: attributesOfItem
FS-->>Cache: mtime
alt cache hit
Cache-->>LFIView: NSImage
else cache miss
LFIView->>FS: NSImage contentsOf fileURL
FS-->>LFIView: NSImage
LFIView->>Cache: store image
end
LFIView-->>MPV: Image with ResizeToFit layout
else remote URL
LFIL-->>LFIView: .remote(url)
LFIView->>MUI: DefaultImageProvider makeImage
MUI-->>MPV: remote image
else unsupported or nil
LFIL-->>LFIView: .unsupported
LFIView-->>MPV: Image not found placeholder
end
Reviews (1): Last reviewed commit: "Render local images in the Markdown View..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| A5001421 /* MarkdownPanelView.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5001419 /* MarkdownPanelView.swift */; }; | ||
| A500LFI011 /* LocalFileImageLoader.swift in Sources */ = {isa = PBXBuildFile; fileRef = A500LFI010 /* LocalFileImageLoader.swift */; }; |
There was a problem hiding this comment.
The new object IDs (A500LFI010, A500LFI011, A500LFI020, A500LFI021, A500LFI030, A500LFI031) contain non-hex letters and are only 10 characters, whereas Xcode generates 24-character hex strings. While pbxproj accepts arbitrary string keys, manually crafted short IDs like these increase the chance of collisions as the project grows and may confuse some Xcode tooling (e.g. xcodebuild -list, merge conflict resolution). Consider regenerating these entries through Xcode's "Add Files…" flow to get canonical IDs.
| let loaded = await Task.detached(priority: .userInitiated) { () -> NSImage? in | ||
| let cacheKey = LocalFileImageCache.key(for: fileURL) | ||
| if let cacheKey, let cached = LocalFileImageCache.shared.object(forKey: cacheKey) { | ||
| return cached | ||
| } | ||
| guard let image = NSImage(contentsOf: fileURL) else { return nil } | ||
| if let cacheKey { | ||
| LocalFileImageCache.shared.setObject(image, forKey: cacheKey) | ||
| } | ||
| return image | ||
| }.value |
There was a problem hiding this comment.
Task.detached doesn't inherit cancellation
When SwiftUI's .task(id: fileURL) cancels the outer task (because fileURL changes or the view is removed), the detached task is not a child and continues to completion. Because Task<NSImage?, Never>.value has a Never error type it never throws CancellationError, so the outer task silently waits for the full decode before honouring the cancellation signal. Back-to-back file navigations can therefore queue up multiple simultaneous background decodes.
Adding a Task.isCancelled guard inside the detached closure — both before the cache stat and before the NSImage decode — would let the hot path abort early when the result is no longer needed.
The Markdown Viewer panel used swift-markdown-ui's default image provider, which only handles http(s) URLs, so every local image reference — relative paths, absolute paths, and file:// URLs — rendered as a broken placeholder. Introduce LocalFileImageProvider and a pure URL classifier that together resolve every local form against the .md file's parent directory, load NSImage off the main thread, and cache decoded bitmaps in a bounded NSCache. The cache key combines path and file mtime so a remounted image view (panel closed and reopened after an edit) picks up the new bytes instead of reusing a stale entry. Local images follow swift-markdown-ui's shrink-only sizing; remote URLs delegate straight to DefaultImageProvider; missing files render a localized placeholder. Percent-encoded, fragmented, and query-suffixed paths round-trip cleanly. The viewer is wired through imageBaseURL only, so regular markdown link resolution is unchanged. Addresses the local-image portion of #2069. Mermaid rendering is out of scope and is being tracked separately in #2463.
Gate LocalFileImageView's render on a URL match so a stale outcome from a previous fileURL cannot flash through during a URL transition before the new load task updates state. Forward the parent .task(id:) cancellation into the detached image-loading task via withTaskCancellationHandler so an in-flight stat + NSImage load short-circuits when the view moves on to a new URL. Renumber the six new pbxproj object IDs from the ad-hoc A500LFI0xx form to 24-character hex so they line up with the rest of the project file.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Panels/LocalFileImageProvider.swift (1)
113-127: Consider adding a memory-based limit alongside the count limit.The cache is bounded by entry count (
countLimit = 64), but image sizes can vary significantly. A single large image could consume substantial memory.Optional enhancement: Add totalCostLimit
static let shared: NSCache<NSString, NSImage> = { let cache = NSCache<NSString, NSImage>() cache.name = "cmux.markdown.localFileImage" cache.countLimit = 64 + // ~50 MB rough memory bound + cache.totalCostLimit = 50 * 1024 * 1024 return cache }()Then when storing, pass the image size as cost:
if let cacheKey { - LocalFileImageCache.shared.setObject(image, forKey: cacheKey) + let cost = Int(image.size.width * image.size.height * 4) // rough bytes estimate + LocalFileImageCache.shared.setObject(image, forKey: cacheKey, cost: cost) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/LocalFileImageProvider.swift` around lines 113 - 127, LocalFileImageCache currently only sets a countLimit (countLimit = 64) so a single large NSImage can still blow memory; add a memory-based bound by setting shared.totalCostLimit to a suitable byte limit (e.g., a few MBs to match app requirements) and when inserting into the cache use NSCache.setObject(_:forKey:cost:) with the image's approximate memory cost. Locate the LocalFileImageCache.shared initializer to add totalCostLimit and ensure callers that store images (places that call LocalFileImageCache.shared.setObject or similar) compute a cost for the NSImage (e.g., from tiffRepresentation?.count or using bitmapRepresentation bytesPerRow * pixelsHigh) and pass that value to setObject(_:forKey:cost:).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Panels/LocalFileImageProvider.swift`:
- Around line 113-127: LocalFileImageCache currently only sets a countLimit
(countLimit = 64) so a single large NSImage can still blow memory; add a
memory-based bound by setting shared.totalCostLimit to a suitable byte limit
(e.g., a few MBs to match app requirements) and when inserting into the cache
use NSCache.setObject(_:forKey:cost:) with the image's approximate memory cost.
Locate the LocalFileImageCache.shared initializer to add totalCostLimit and
ensure callers that store images (places that call
LocalFileImageCache.shared.setObject or similar) compute a cost for the NSImage
(e.g., from tiffRepresentation?.count or using bitmapRepresentation bytesPerRow
* pixelsHigh) and pass that value to setObject(_:forKey:cost:).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e30c45d-b3ba-4900-a6a9-91b75d52c47a
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/Panels/LocalFileImageLoader.swiftSources/Panels/LocalFileImageProvider.swiftSources/Panels/MarkdownPanelView.swiftcmuxTests/LocalFileImageLoaderTests.swift
✅ Files skipped from review due to trivial changes (2)
- Sources/Panels/MarkdownPanelView.swift
- Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (2)
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/Panels/LocalFileImageLoader.swift
|
Closing this PR — it's been superseded by #4288 ("Fix markdown viewer image rendering"), which already landed on While this PR was open, the Markdown Viewer was rewritten from the swift-markdown-ui (
This PR's Closing as superseded. Leaving #2069 open for the remaining mermaid work. |
1 participant
Summary
LocalFileImageProviderthat loads local images viaNSImageand delegatesremote URLs to swift-markdown-ui's
DefaultImageProviderso remote sizing stayson the upstream
ResizeToFitpathLocalFileImageLoader, a pure URL classifier that normalizes relative URLsvia
absoluteURL; for local file loads it ignores?query/#fragmentandhandles percent-encoded paths cleanly
imageBaseURL(the.mdfile's parent directory) intoMarkdown(...)sorelative image destinations resolve against it — regular markdown link
resolution is untouched,
baseURLis left at the defaultNSImages in anNSCachekeyed by path + file mtime, boundedat 64 entries, so a remounted image view (panel closed and reopened after an
edit) picks up the new bytes instead of reusing a stale entry
re-render path stays free of blocking file I/O
unsupported schemes
Addresses the local-image portion of #2069. Mermaid rendering is out of
scope and is being tracked separately in #2463.
Demo
Test plan
./images/foo.png,images/foo.png,../foo.png) renderfile://URLs render?query/#fragmentsuffixes are stripped before loadDefaultImageProvider(no regression)cmuxTests/LocalFileImageLoaderTests.swiftfor theURL classifier and the path+mtime cache key
Testing
./scripts/reload.sh --tag fix-md-local-images/tmp/md-test/edge.mdfixture covering every case in the plan above
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Tests