Repository navigation
Fix browser image download filenames - #5938
Merged
austinywang merged 9 commits intoJun 12, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds BrowserDownloadFilenameResolver to validate HTTP responses, detect image UTType from bytes or files, sanitize candidate filenames, and ensure image extensions match detected content; integrates into WKDownload and context-menu download flows, and adds tests, localization, and project wiring. ChangesDownload Filename Resolver
sequenceDiagram
participant Client as CmuxWebView / BrowserPanel
participant URLSession as NSURLSession
participant Resolver as BrowserDownloadFilenameResolver
participant SavePanel
Client->>URLSession: fetch URL / network download
URLSession-->>Client: (response, data)
Client->>Resolver: httpStatusDecision(for: response)
alt allow (2xx or non-HTTP)
Resolver-->>Client: allow
Client->>Resolver: suggestedFilename(..., imageData or imageFileURL)
Resolver-->>Client: sanitized filename (correct image ext)
Client-->>SavePanel: present save with filename
else reject (non-2xx)
Resolver-->>Client: reject(statusCode)
Client-->>Client: fallback/cancel download action
end
🎯 3 (Moderate) | ⏱️ ~18 minutes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…wnload-image-wrong-extension
Contributor
Greptile SummaryIntroduces
Confidence Score: 5/5Safe to merge — the core filename resolution logic is correct and both download paths behave properly; the single note is a minor edge-case in the extension-stripping loop that only surfaces for unusual double-extension filenames. The resolver's sanitisation and image-type detection are correct for all common web image formats. The WKDownload path properly moves disk I/O off the main thread, and the context-menu path correctly gates on HTTP status before showing the save panel. The only gap is that strippingNonImageExtensions falls back to stripping only the outermost extension when an inner wrong-image extension blocks the loop, producing e.g. avatar.webp.png instead of avatar.png for an unusual double-suffix filename — a cosmetic output issue that does not affect file integrity or data safety. Sources/Panels/BrowserDownloadFilenameResolver.swift — the strippingNonImageExtensions fallback path in imageFilename. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[URL session / WKDownload response] --> B{HTTPStatus 2xx?}
B -- No --> C[reject / cancel download\nno save panel]
B -- Yes --> D[BrowserDownloadFilenameResolver\nsuggestedFilename]
D --> E{imageType available?}
E -- No --> F[sanitizedFilename only\ncolon replaced, fallback localized]
E -- Yes --> G[imageFilename]
G --> H{filename already has\nmatching image ext?}
H -- Yes --> I[return as-is]
H -- No --> J[strippingNonImageExtensions\nstrip until matching ext found]
J --> K{matching ext found\nafter stripping?}
K -- Yes --> I
K -- No --> L[strip final ext from original\nappend correct ext]
F --> M[NSSavePanel.nameFieldStringValue]
I --> M
L --> M
Reviews (4): Last reviewed commit: "fix: replace browser downloads atomicall..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| completionHandler(nil) | ||
| return | ||
| } | ||
| let sourceURL = response.url ?? URL(fileURLWithPath: suggestedFilename) |
Contributor
There was a problem hiding this comment.
When response.url is nil, URL(fileURLWithPath: suggestedFilename) creates a file:/// URL from what is actually a bare filename string (e.g. "photo.jpg"). The resolver only uses this for lastPathComponent, so it works today, but the file:// scheme means any future caller that inspects .scheme or .host on sourceURL will get unexpected values. A URL(string:) or a direct URL(fileURLWithPath:) on FileManager.default.temporaryDirectory.appendingPathComponent(_:) would be more semantically accurate.
Suggested change
| let sourceURL = response.url ?? URL(fileURLWithPath: suggestedFilename) | |
| let sourceURL = response.url ?? URL(string: suggestedFilename.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? suggestedFilename) ?? URL(fileURLWithPath: suggestedFilename) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/BrowserDownloadFilenameResolverTests.swift (1)
11-83: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueOptional: Consider additional test coverage for edge cases.
The current tests validate the key regression scenarios from the PR objectives. For more comprehensive coverage, consider adding tests for:
.allowcase for 2xx HTTP status codes.allowcase for non-HTTP responses (per upstream contract in Context snippet 1)- JPEG type detection and the special
"jpg"extension mapping- Multiple-layer extension stripping (e.g.,
"logo.png.txt.xml"→"logo.png")- The
"download"fallback filename when all candidates are emptyThese are not required for the regression fix but would strengthen the test suite against future changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/BrowserDownloadFilenameResolverTests.swift` around lines 11 - 83, Add unit tests to BrowserDownloadFilenameResolverTests to cover the suggested edge cases: add a test that asserts resolver.httpStatusDecision(for:) returns .allow for a 2xx HTTPURLResponse, a test that passes a non-HTTP URLResponse and asserts .allow behavior per upstream contract, tests that validate resolver.imageType(forImageData:) detects JPEG bytes and that suggestedFilename(...) maps JPEG imageType to "jpg" extension, a test that verifies multi-layer extension stripping (e.g., suggestedFilename "logo.png.txt.xml" → "logo.png"), and a test that when all filename candidates are empty the fallback filename is "download"; use the existing resolver instance and the functions httpStatusDecision(for:), imageType(forImageData:), and suggestedFilename(suggestedFilename:response:sourceURL:imageType:) to implement these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserDownloadFilenameResolver.swift`:
- Around line 93-101: The hardcoded fallback filename "download" in
sanitizedFilename(_:, fallbackURL:) must be localized; replace the literal with
a localized lookup (e.g. use NSLocalizedString with the key
"browser.download.defaultFilename" or your project's localization helper) in the
return path(s) where the function currently returns "download", and add matching
entries for browser.download.defaultFilename to Resources/Localizable.xcstrings
for every supported locale so the UI displays the translated default filename.
---
Outside diff comments:
In `@cmuxTests/BrowserDownloadFilenameResolverTests.swift`:
- Around line 11-83: Add unit tests to BrowserDownloadFilenameResolverTests to
cover the suggested edge cases: add a test that asserts
resolver.httpStatusDecision(for:) returns .allow for a 2xx HTTPURLResponse, a
test that passes a non-HTTP URLResponse and asserts .allow behavior per upstream
contract, tests that validate resolver.imageType(forImageData:) detects JPEG
bytes and that suggestedFilename(...) maps JPEG imageType to "jpg" extension, a
test that verifies multi-layer extension stripping (e.g., suggestedFilename
"logo.png.txt.xml" → "logo.png"), and a test that when all filename candidates
are empty the fallback filename is "download"; use the existing resolver
instance and the functions httpStatusDecision(for:), imageType(forImageData:),
and suggestedFilename(suggestedFilename:response:sourceURL:imageType:) to
implement these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4611d20-8a22-4cbc-add9-635c5ca240d2
📒 Files selected for processing (5)
Sources/Panels/BrowserDownloadFilenameResolver.swiftSources/Panels/BrowserPanel.swiftSources/Panels/CmuxWebView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserDownloadFilenameResolverTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 8863-8868: The code currently synthesizes sourceURL from
suggestedFilename when response.url is nil, which reintroduces wrong-extension
behavior; instead, leave sourceURL nil when response.url is nil and pass that
nil into filenameResolver.suggestedFilename (i.e. use response.url directly
rather than URL(fileURLWithPath: suggestedFilename)), and store the
DownloadState with the actual optional sourceURL (ensure DownloadState and
storeState accept an optional URL if needed) so URL-path fallback is skipped
when no real response URL was provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a56d3463-d1bb-4db0-9762-a035f211e54f
📒 Files selected for processing (4)
Sources/Panels/BrowserDownloadFilenameResolver.swiftSources/Panels/BrowserPanel.swiftSources/Panels/CmuxWebView.swiftcmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 8906-8912: The current flow deletes destURL then moves
info.tempURL into place, risking data loss if the move fails; instead, check
FileManager.default.fileExists(atPath: destURL.path) and when it exists use
FileManager.default.replaceItemAt(destURL, withItemAt: info.tempURL) to
atomically replace the file, or alternatively move/copy info.tempURL to a
sibling temp destination and only replace/rename to destURL after the write
succeeds; update the code around the removeItem/moveItem calls (references:
FileManager.default.removeItem, FileManager.default.moveItem,
FileManager.default.replaceItemAt, destURL, info.tempURL) to follow this safer
replacement pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: df59165c-b135-491c-a29f-5a25c8e49fbc
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/Panels/BrowserDownloadFilenameResolver.swiftSources/Panels/BrowserPanel.swift
austinywang
added a commit
that referenced
this pull request
Jun 12, 2026
Aziz concurrency policy (autoreview): main-thread hops in new files should use @mainactor structured concurrency, not DispatchQueue.main .async — same modernization as the BrowserPanel download path (#5938). The Task { @mainactor } hop preserves the load-bearing ordering: it is enqueued synchronously and runs after the current runloop callout, i.e. after AppKit's own per-scroll-view scroller-style reset. The test drain switches from a main-queue round trip to a bounded main-actor yield loop so it makes no cross-mechanism FIFO assumptions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
added a commit
that referenced
this pull request
Jun 12, 2026
… changes (#3241 sidebar scope) (#5955) * Add failing test: sidebar scroller config must re-assert after preferred-style change AppKit resets every NSScrollView's scrollerStyle to the new system preference when NSScroller.preferredScrollerStyleDidChangeNotification fires (mouse connect/disconnect, System Settings "Show scroll bars"). That clobbers the sidebar's forced overlay configuration with a legacy, space-reserving scrollbar until some unrelated SwiftUI re-render happens to re-run the resolver (#3241 reopen, sidebar scope; browser-pane scope is PR #5847). Moves SidebarScrollViewResolverView from ContentView.swift into SidebarScrollViewConfigurator.swift unchanged (internal visibility) so the test can exercise it; no behavior change in this commit, so the new test fails: posting the style-change notification does not re-resolve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Re-assert sidebar overlay scrollers when the preferred scroller style changes SidebarScrollViewResolverView now observes NSScroller.preferredScrollerStyleDidChangeNotification and re-resolves, re-applying SidebarScrollViewConfigurator's overlay configuration. The async main hop in resolveScrollView() guarantees the re-apply runs after AppKit's own synchronous per-scroll-view style reset, regardless of observer registration order — mirroring the terminal's existing handlePreferredScrollerStyleChange treatment of the same notification. Both sidebar resolver call sites share this path, and the configurator's guarded writes keep the re-apply a no-op when nothing changed, so the knob-fade contract from the #3241 follow-up is preserved. Fixes the sidebar scope of the #3241 reopen (browser panes: PR #5847). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Move SidebarScrollViewResolverView to its own file Autoreview: the extraction from ContentView.swift should follow the one-major-type-per-file architecture rule rather than sharing SidebarScrollViewConfigurator.swift with the configurator enum. No behavior change; wires the new file into the app target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Observe scroller-style changes on the main queue Greptile: queue: nil runs the observer block on the posting thread, so a background-thread post would call the NSView method off-main. Use .main — a main-thread post (AppKit's case) still executes the block synchronously in place, so the re-apply ordering after AppKit's own per-scroll-view reset is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Express the resolver's deferred hop with structured concurrency Aziz concurrency policy (autoreview): main-thread hops in new files should use @mainactor structured concurrency, not DispatchQueue.main .async — same modernization as the BrowserPanel download path (#5938). The Task { @mainactor } hop preserves the load-bearing ordering: it is enqueued synchronously and runs after the current runloop callout, i.e. after AppKit's own per-scroll-view scroller-style reset. The test drain switches from a main-queue round trip to a bounded main-actor yield loop so it makes no cross-mechanism FIFO assumptions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
Jun 12, 2026
* test: cover browser download image filenames * fix: validate browser image downloads * fix: keep browser download adapters within budget * fix: sniff native download images off main * fix: localize browser download fallback name * fix: preserve explicit browser download names * fix: use Swift concurrency for download save prompt * fix: replace browser downloads atomically
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5924
Summary
Verification
Note: local xcodebuild was not run per the task instruction to never run bare xcodebuild.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes wrong image filenames when downloading from the in‑app browser. Adds byte/file type sniffing, preserves explicit names, rejects non‑2xx before the save panel, localizes the fallback name, uses Swift concurrency for a responsive save prompt, and replaces existing files atomically.
WKDownload.browser.download.defaultFilename.Written for commit ca592d0. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Localization