Fix browser downloads from subframes - #6756
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 a browser save-location setting, exposes it across settings surfaces, and updates download handling for frame-aware classification, subframe intent tracking, WebKit download routing, and save-dialog or auto-save completion paths. ChangesBrowser download save prompt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (19 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 |
![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
🤖 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/CmuxWebView.swift`:
- Around line 1633-1635: Move the download write path off the main thread: the
synchronous Data write inside the writeData helper in CmuxWebView is being
invoked from main-dispatched download closures, which can block the UI for large
downloads. Keep only destination resolution on the main thread, then perform
directory creation and file writing in a background task before returning to the
main queue for any state updates or fallback callbacks. Apply the same pattern
to the related download call sites in the data, file, and response handlers so
they all use the background write path.
In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift:
- Around line 212-216: The scripted download handler in
CmuxWebView+ScriptedDownloads should not accept native file: downloads from
injected web content. Update the URL interception logic in the download path
that checks scheme handling so only http/https are allowed for general scripted
content, and keep file: support restricted to trusted file-origin cases with
strict scope checks before reaching downloadURLViaSession or the file branch.
Ensure the same fix is applied anywhere the same scheme allow-list appears in
this scripted download flow.
🪄 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: e25895dc-cb8b-4b11-8c17-b687ffb864c6
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (19)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/BrowserCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BrowserSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftResources/Localizable.xcstringsSources/CmuxSettingsJSONPathSupport.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/Panels/BrowserDownloadFilenameResolver.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftSources/Panels/CmuxWebView+ScriptedDownloads.swiftSources/Panels/CmuxWebView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftcmuxTests/BrowserDownloadFilenameResolverTests.swiftweb/data/cmux.schema.jsonweb/messages/en.jsonweb/messages/ja.json
Greptile SummaryThis PR fixes browser downloads initiated from subframes by injecting a tokenized, trusted-click intent script into all frames; adds a persistent downloads popover button with progress and file actions; unifies the WKDownload, popup, context-menu, and scripted-download paths through shared completion logic; and adds an opt-in
Confidence Score: 4/5Safe to merge modulo open prior-round comments on the terminal event queue and save-panel file I/O; the new subframe interception and unified completion paths are logically sound. The core subframe intent tracking, collision-safe auto-save, popover UI, and localization are all well-constructed. Two dead-code items were found in this round (the handledAnchors WeakSet that is never populated, and the allowsSubframeDownload parameter that is passed to navigationResponseDownloadReason but never read), both minor. The lower ceiling reflects that comments from prior review rounds about the ready_to_save stale-event queue in TerminalController and synchronous file I/O in the BrowserDownloadDelegate.presentSavePanel completion handler remain open without a resolution reply. Sources/TerminalController.swift — prior-round comment on stale terminal event after save-panel dismissal is unresolved. Sources/Panels/BrowserPanel.swift — prior-round comment on synchronous file I/O in the presentSavePanel NSSavePanel completion handler is unresolved. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant JS as Subframe JS
participant Bridge as WKScriptMessageHandler
participant Tracker as BrowserSubframeDownloadIntentTracker
participant NavDel as BrowserNavigationDelegate
participant DlDel as BrowserDownloadDelegate
participant BP as BrowserPanel
JS->>Bridge: "kind=subframeDownloadIntent {url}"
Bridge->>Tracker: record(url)
NavDel->>Tracker: updateIfNeeded(navigationAction)
NavDel->>Tracker: consume(for: url) [action policy]
NavDel->>NavDel: decisionHandler(.download)
DlDel->>DlDel: decideDestination to tempURL
DlDel->>BP: onDownloadStarted(filename, downloadID)
BP->>BP: applyBrowserDownloadEvent started
alt "askWhereToSave = false"
DlDel->>DlDel: moveTemporaryDownloadToDownloads Task.detached
DlDel->>BP: onDownloadSaved(filename, destURL, true, downloadID)
else "askWhereToSave = true"
DlDel->>DlDel: presentSavePanel MainActor
DlDel->>BP: onDownloadReadyToSave(filename, downloadID)
BP->>BP: endDownloadActivity
DlDel->>BP: onDownloadSaved(filename, destURL, false, downloadID)
end
BP->>BP: applyBrowserDownloadEvent saved to recentDownloads
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant JS as Subframe JS
participant Bridge as WKScriptMessageHandler
participant Tracker as BrowserSubframeDownloadIntentTracker
participant NavDel as BrowserNavigationDelegate
participant DlDel as BrowserDownloadDelegate
participant BP as BrowserPanel
JS->>Bridge: "kind=subframeDownloadIntent {url}"
Bridge->>Tracker: record(url)
NavDel->>Tracker: updateIfNeeded(navigationAction)
NavDel->>Tracker: consume(for: url) [action policy]
NavDel->>NavDel: decisionHandler(.download)
DlDel->>DlDel: decideDestination to tempURL
DlDel->>BP: onDownloadStarted(filename, downloadID)
BP->>BP: applyBrowserDownloadEvent started
alt "askWhereToSave = false"
DlDel->>DlDel: moveTemporaryDownloadToDownloads Task.detached
DlDel->>BP: onDownloadSaved(filename, destURL, true, downloadID)
else "askWhereToSave = true"
DlDel->>DlDel: presentSavePanel MainActor
DlDel->>BP: onDownloadReadyToSave(filename, downloadID)
BP->>BP: endDownloadActivity
DlDel->>BP: onDownloadSaved(filename, destURL, false, downloadID)
end
BP->>BP: applyBrowserDownloadEvent saved to recentDownloads
Reviews (45): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| do { | ||
| let directory = filenameResolver.downloadsDirectory() | ||
| try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true, attributes: nil) | ||
| let destinationURL = filenameResolver.uniqueDownloadDestination( | ||
| suggestedFilename: saveName, | ||
| in: directory | ||
| ) | ||
| debugContextDownload( | ||
| "browser.ctxdl.\(logCategory) trace=\(traceID) stage=autoSave path=\(destinationURL.path)" | ||
| ) | ||
| let didWrite = writeData(destinationURL) | ||
| notifyContextMenuDownloadState(false) | ||
| if !didWrite, failureFallbackReason == nil { | ||
| return | ||
| } | ||
| } catch { | ||
| notifyContextMenuDownloadState(false) | ||
| debugContextDownload( | ||
| "browser.ctxdl.\(logCategory) trace=\(traceID) stage=saveFailure error=\(error.localizedDescription)" | ||
| ) | ||
| if let failureFallbackReason { | ||
| runContextMenuFallback( | ||
| action: fallbackAction, | ||
| target: fallbackTarget, | ||
| sender: sender, | ||
| traceID: traceID, | ||
| reason: failureFallbackReason | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
finishSessionDownload's auto-save branch calls FileManager.createDirectory, the fileExists loop inside uniqueDownloadDestination, and data.write(to:options:.atomic) all synchronously on @MainActor. CmuxWebView inherits WKWebView's @MainActor isolation, so every call site that hops back to main via DispatchQueue.main.async before calling this (the http/https and file: URLSession paths) will block the run loop for the full duration of the disk write. A large file from the context-menu "Save Image As" or scripted-download path will freeze the UI until the write completes.
BrowserDownloadDelegate.downloadDidFinish handles the same auto-save operation correctly by wrapping it in Task.detached(priority: .utility) and awaiting the result before calling onDownloadSaved. The two paths need to be consistent. The createDirectory + uniqueDownloadDestination + data.write block should be lifted into a detached task with a main-actor hop for the notifyContextMenuDownloadState/fallback work, matching the delegate pattern at BrowserPanel.swift lines 8442–8460.
There was a problem hiding this comment.
Resolved in current head: non-WebKit session download writes now run through the detached save path before main-actor state updates; this thread is outdated against 209962a.
— Claude Code
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/Panels/BrowserPanel.swift (2)
8454-8456: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log raw download destination paths.
destinationURL.pathcan include usernames and private folder/file names. Keep the DEBUG trace to non-sensitive metadata such as byte counts or a trace id.Suggested change
- cmuxDebugLog("download.saved path=\(destinationURL.path)") + cmuxDebugLog("download.saved pathBytes=\(destinationURL.path.utf8.count)")🤖 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 `@Sources/Panels/BrowserPanel.swift` around lines 8454 - 8456, The DEBUG trace in the download save flow is logging a raw destination path, which can expose sensitive user and folder information. Update the logging in the download handling code around the cmuxDebugLog call in BrowserPanel to avoid destinationURL.path and instead record only non-sensitive metadata such as byte counts, a trace identifier, or other generic status details.Source: Coding guidelines
8370-8379: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the save-panel file operation off the main actor.
presentSavePanelis@MainActor, so replacing/moving a completed download here can block the UI for large files or cross-volume saves. Mirror the auto-save path and do theFileManagerwork in a detached task, then hop back to the main actor for callbacks.Suggested direction
- do { - if FileManager.default.fileExists(atPath: destURL.path) { - _ = try FileManager.default.replaceItemAt(destURL, withItemAt: tempURL) - } else { - try FileManager.default.moveItem(at: tempURL, to: destURL) - } - self.onDownloadSaved?(suggestedFilename, destURL) - } catch { - try? FileManager.default.removeItem(at: tempURL) - self.onDownloadFailed?(error) - } + Task { + let result = await Task.detached(priority: .utility) { + Result { + if FileManager.default.fileExists(atPath: destURL.path) { + _ = try FileManager.default.replaceItemAt(destURL, withItemAt: tempURL) + } else { + try FileManager.default.moveItem(at: tempURL, to: destURL) + } + } + }.value + + switch result { + case .success: + self.onDownloadSaved?(suggestedFilename, destURL) + case .failure(let error): + try? FileManager.default.removeItem(at: tempURL) + self.onDownloadFailed?(error) + } + }🤖 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 `@Sources/Panels/BrowserPanel.swift` around lines 8370 - 8379, The file move/replace work in presentSavePanel is still running on the MainActor and can block the UI for large or cross-volume saves. Move the FileManager operations in the save-panel completion path into a detached task, mirroring the auto-save flow, and only hop back to the MainActor for onDownloadSaved and onDownloadFailed callbacks; use the presentSavePanel and FileManager replaceItemAt/moveItem logic to locate the code.Source: Coding guidelines
Sources/Panels/CmuxWebView+ScriptedDownloads.swift (1)
133-135: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not materialize unknown blob URLs in JavaScript before size checks.
fetch(url).blob()buffers the whole blob beforereadBlobForDownloadcan reject payloads overmaxPayloadBytes, so large iframe downloads can still spike memory or kill the web content process. Post the blob URL directly and let the native/WebKit download path stream it.Suggested change
- fetch(url) - .then((response) => response.blob()) - .then((blob) => readBlobForDownload(blob, suggestedFilename, url)) - .catch(() => postURLDownload(url, suggestedFilename)); + postURLDownload(url, suggestedFilename); return true;🤖 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 `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift around lines 133 - 135, The scripted download flow in CmuxWebView+ScriptedDownloads is materializing blob URLs in JavaScript via fetch(url).blob() before size validation, which can spike memory for large unknown payloads. Update the download path in the blob handling code so the blob URL is passed directly into the native/WebKit download path and let readBlobForDownload or its caller avoid buffering unknown blob URLs in JS before maxPayloadBytes checks. Keep the fix localized around the download promise chain and the readBlobForDownload helper.
🤖 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.
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 8454-8456: The DEBUG trace in the download save flow is logging a
raw destination path, which can expose sensitive user and folder information.
Update the logging in the download handling code around the cmuxDebugLog call in
BrowserPanel to avoid destinationURL.path and instead record only non-sensitive
metadata such as byte counts, a trace identifier, or other generic status
details.
- Around line 8370-8379: The file move/replace work in presentSavePanel is still
running on the MainActor and can block the UI for large or cross-volume saves.
Move the FileManager operations in the save-panel completion path into a
detached task, mirroring the auto-save flow, and only hop back to the MainActor
for onDownloadSaved and onDownloadFailed callbacks; use the presentSavePanel and
FileManager replaceItemAt/moveItem logic to locate the code.
In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift:
- Around line 133-135: The scripted download flow in
CmuxWebView+ScriptedDownloads is materializing blob URLs in JavaScript via
fetch(url).blob() before size validation, which can spike memory for large
unknown payloads. Update the download path in the blob handling code so the blob
URL is passed directly into the native/WebKit download path and let
readBlobForDownload or its caller avoid buffering unknown blob URLs in JS before
maxPayloadBytes checks. Keep the fix localized around the download promise chain
and the readBlobForDownload helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d8a73c1-ef83-476f-bdb8-6a5bfa800b71
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftSources/Panels/CmuxWebView+ScriptedDownloads.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/BrowserPanel.swift (2)
9019-9034: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove production raw-URL logging from navigation response/download classification.
After subframe responses started flowing through this path, these
NSLogcalls can emit full browsing URLs for ordinary iframe/subresource traffic. Use DEBUG-only redacted diagnostics orLoggerwith private/redacted fields. As per coding guidelines, runtime Swift should not useNSLog, and secrets/customer content/personal data must not be logged without redaction.🤖 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 `@Sources/Panels/BrowserPanel.swift` around lines 9019 - 9034, Remove the production raw-URL logging from the BrowserPanel navigation/download path. Replace the two NSLog calls in BrowserPanel’s navigationResponse handling with DEBUG-only diagnostics or Logger-based messages that redact private values, especially responseURL and any URL-derived fields. Keep the classification logic in BrowserDownloadFilenameResolver.navigationResponseDownloadReason unchanged, but ensure ordinary subframe/subresource traffic cannot emit full browsing URLs in release builds.Source: Coding guidelines
8383-8389: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the prompted save file operation off the main actor.
The auto-save path detaches file work, but the save-panel OK path still does
replaceItemAt/moveItemon@MainActor. Large or cross-volume prompted downloads can freeze the UI; perform the move/replace in a detached task and hop back only foronDownloadSaved/onDownloadFailed.🤖 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 `@Sources/Panels/BrowserPanel.swift` around lines 8383 - 8389, The prompted save flow in BrowserPanel’s save handling still performs FileManager.replaceItemAt/moveItem on the MainActor, which can block the UI for large or cross-volume files. Move the file operation out of the `@MainActor` context by running the save-panel OK path work in a detached task, then switch back only to call onDownloadSaved or onDownloadFailed. Keep the fix centered around the save logic near onDownloadSaved, replaceItemAt, and moveItem so the UI thread only handles callbacks.
🤖 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/BrowserPopupWindowController.swift`:
- Around line 605-609: The subframe download intent check is relying on an
unreliable app event heuristic in browserNavigationHasSimpleUserActivation(),
which can be triggered by unrelated input. Update the intent gate in
BrowserPopupWindowController and the copied helper in BrowserNavigationDelegate
to use a scoped, authoritative signal such as navigationType == .linkActivated
or a structured scripted-download bridge token, and fail closed when that signal
is absent. Keep the existing recentSubframeDownloadIntentKeys tracking, but
ensure only the new trusted navigation/download intent source can authorize the
auto-save path.
In `@Sources/Panels/CmuxWebView.swift`:
- Around line 1637-1644: The diagnostics in CmuxWebView are logging sensitive
download details such as destinationURL.path and saveName, which can expose
private filenames or local paths. Update the affected logging in the
download/save flow (including the saveSuccess/saveFailure paths and the related
sites around the other noted locations) to emit only non-sensitive metadata,
such as byte counts, file extension, or a redacted/private identifier. Keep the
existing debugContextDownload call sites, but replace any raw path/name values
with sanitized fields from the relevant download handling methods and models.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 9019-9034: Remove the production raw-URL logging from the
BrowserPanel navigation/download path. Replace the two NSLog calls in
BrowserPanel’s navigationResponse handling with DEBUG-only diagnostics or
Logger-based messages that redact private values, especially responseURL and any
URL-derived fields. Keep the classification logic in
BrowserDownloadFilenameResolver.navigationResponseDownloadReason unchanged, but
ensure ordinary subframe/subresource traffic cannot emit full browsing URLs in
release builds.
- Around line 8383-8389: The prompted save flow in BrowserPanel’s save handling
still performs FileManager.replaceItemAt/moveItem on the MainActor, which can
block the UI for large or cross-volume files. Move the file operation out of the
`@MainActor` context by running the save-panel OK path work in a detached task,
then switch back only to call onDownloadSaved or onDownloadFailed. Keep the fix
centered around the save logic near onDownloadSaved, replaceItemAt, and moveItem
so the UI thread only handles callbacks.
🪄 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: d00c0a4d-1a79-4374-b02f-19ba28c4c2a4
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Sources/Panels/BrowserDownloadFilenameResolver.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftSources/Panels/CmuxWebView.swiftcmuxTests/BrowserDownloadFilenameResolverTests.swift
| webView.cmuxDownloadDelegate = downloadDelegate | ||
| configureMoveTabToNewWorkspaceContextMenu(for: webView); configureNavigationDelegateCallbacks() |
There was a problem hiding this comment.
cmuxDownloadDelegate set before delegate is created — blob downloads always fail
webView.cmuxDownloadDelegate = downloadDelegate (line 3744) executes before configureNavigationDelegateCallbacks() (line 3745), which is what actually creates the new BrowserDownloadDelegate and assigns it to self.downloadDelegate. On the first bind, self.downloadDelegate is nil, so webView.cmuxDownloadDelegate is set to nil. On a re-bind it is set to the previous stale delegate. Either way, the webview never holds the current delegate.
startScriptedWebKitDownload guards on cmuxDownloadDelegate being non-nil and logs stage=rejectMissingDelegate if it is nil, so blob: scripted downloads — the primary new path added in this PR — will silently fail in every BrowserPanel context.
Move the assignment to after configureNavigationDelegateCallbacks() returns, or assign inside that method just after self.downloadDelegate = dlDelegate.
There was a problem hiding this comment.
Resolved in current head: the current BrowserPanel binding assigns the web view download delegate after delegate creation, so scripted WebKit downloads have the active delegate; this thread is outdated against 209962a.
— Claude Code
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 8323-8336: `suggestedFilenameOverrides` can retain stale entries
when a `WKDownload` is rejected or fails before `decideDestination` is reached.
Update the shared cleanup path in `BrowserPanel` so the override is removed
whenever download state is cleared, not only via
`takeSuggestedFilenameOverride(for:)`; also clear it in the early HTTP-status
reject branch inside `download(_:decideDestinationUsing:...)` and the failure
path in `download(_:didFailWithError:)` or `removeState`. Ensure the cleanup
uses the same `ObjectIdentifier(download)` key so entries for dead downloads
cannot accumulate.
🪄 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: 8c1a465b-7912-465a-b022-0101ea57f895
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
Sources/Panels/BrowserDownloadFilenameResolver.swiftSources/Panels/BrowserPanel.swiftSources/Panels/CmuxWebView+ScriptedDownloads.swiftcmuxTests/BrowserDownloadFilenameResolverTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/BrowserPanel.swift (1)
9037-9052: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove production
NSLogcalls that include raw URLs.These lines log full navigation/download URLs, including query strings, outside
#if DEBUG. UsecmuxDebugLogunder DEBUG and scrub viabrowserNavigationDebugURL(...), or drop the URL entirely.Suggested fix
- NSLog("BrowserPanel navigationResponse: url=%@ mime=%@ canShow=%d isMainFrame=%d", - responseURL, mime, canShow ? 1 : 0, - navigationResponse.isForMainFrame ? 1 : 0) + `#if` DEBUG + let debugResponseURL = browserNavigationDebugURL(navigationResponse.response.url) + cmuxDebugLog( + "browser.nav.response url=\(debugResponseURL) mime=\(mime) " + + "canShow=\(canShow ? 1 : 0) isMainFrame=\(navigationResponse.isForMainFrame ? 1 : 0)" + ) + `#endif` ... - NSLog("BrowserPanel download: %@ mime=%@ url=%@", reason, mime, responseURL) `#if` DEBUG - cmuxDebugLog("download.policy=download reason=\(reason) mime=\(mime) mainFrame=\(navigationResponse.isForMainFrame ? 1 : 0)") + let debugResponseURL = browserNavigationDebugURL(navigationResponse.response.url) + cmuxDebugLog( + "download.policy=download reason=\(reason) mime=\(mime) " + + "mainFrame=\(navigationResponse.isForMainFrame ? 1 : 0) url=\(debugResponseURL)" + ) `#endif`🤖 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 `@Sources/Panels/BrowserPanel.swift` around lines 9037 - 9052, Remove the production NSLog calls in BrowserPanel navigation/download handling that print raw URLs, especially the logging around navigationResponse and the download reason. Update the logic in BrowserPanel’s navigation decision path to use cmuxDebugLog only under DEBUG, and when a URL is needed, pass it through browserNavigationDebugURL(...) instead of logging responseURL directly. If the URL is not essential, drop it from the log message entirely.Source: Coding guidelines
Sources/Panels/CmuxWebView+ScriptedDownloads.swift (1)
149-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not main-frame-gate the hooks needed for subframe scripted downloads.
The script now runs in subframes, but subframe
URL.createObjectURLtracking and programmaticanchor.click()interception are disabled here. That misses common iframe blob-download flows.Suggested fix
- if (isMainFrame && typeof originalCreateObjectURL === "function") { + if (typeof originalCreateObjectURL === "function") { ... - if (isMainFrame && typeof originalRevokeObjectURL === "function") { + if (typeof originalRevokeObjectURL === "function") { ... - if (isMainFrame && typeof originalAnchorClick === "function") { + if (typeof originalAnchorClick === "function") {Also applies to: 232-236
🤖 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 `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift around lines 149 - 161, The `CmuxWebView+ScriptedDownloads` hooks are still being gated by `isMainFrame`, which prevents subframe blob-download tracking and programmatic `anchor.click()` interception from working. Remove the main-frame-only condition around the `URLCtor.createObjectURL`/`URLCtor.revokeObjectURL` wrappers and the `anchor.click()` interception path so the scripted-download hooks are installed for subframes as well, while keeping the existing `originalCreateObjectURL`, `originalRevokeObjectURL`, and click-hook logic intact.
🤖 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.
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 9037-9052: Remove the production NSLog calls in BrowserPanel
navigation/download handling that print raw URLs, especially the logging around
navigationResponse and the download reason. Update the logic in BrowserPanel’s
navigation decision path to use cmuxDebugLog only under DEBUG, and when a URL is
needed, pass it through browserNavigationDebugURL(...) instead of logging
responseURL directly. If the URL is not essential, drop it from the log message
entirely.
In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift:
- Around line 149-161: The `CmuxWebView+ScriptedDownloads` hooks are still being
gated by `isMainFrame`, which prevents subframe blob-download tracking and
programmatic `anchor.click()` interception from working. Remove the
main-frame-only condition around the
`URLCtor.createObjectURL`/`URLCtor.revokeObjectURL` wrappers and the
`anchor.click()` interception path so the scripted-download hooks are installed
for subframes as well, while keeping the existing `originalCreateObjectURL`,
`originalRevokeObjectURL`, and click-hook logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67bbc0ca-1f10-4de2-bd76-689b6e00a71c
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftSources/Panels/CmuxWebView+ScriptedDownloads.swift
| guard !recentSubframeDownloadIntentKeys.isEmpty else { return false } | ||
| recentSubframeDownloadIntentKeys.removeFirst() | ||
| return true |
There was a problem hiding this comment.
When the response URL doesn't match any recorded intent key, this branch removes the oldest intent — regardless of which subframe it belongs to — and returns true. On a page with multiple active subframes, a user click in subframe A (URL X) records an intent; if subframe B then receives a response for URL Y with Content-Disposition: attachment, the URL-mismatch fallback fires, consumes subframe A's intent, and grants allowsSubframeDownload = true to subframe B's response. The user never clicked anything in subframe B, but a download is triggered.
The redirect-following scenario (URL A → redirect → URL B) is the presumed motivation, but an "unreliable but better than nothing" fallback that can produce a wrong authoritative value is not acceptable. The fallback should be dropped; redirects on subframes that carry explicit Content-Disposition: attachment headers are rare, and the user-activation check in recordSubframeDownloadIntentIfNeeded already requires a real gesture, so failing closed is correct here.
There was a problem hiding this comment.
Resolved in current head: the unrelated fallback consume path was removed; subframe response authorization now requires a recorded matching intent or redirect transfer and fails closed otherwise.
— Claude Code
- Surface Download/Print buttons in the omnibar when a PDF document is rendered (main frame or subframe), tracked via BrowserPanel .renderedPDFDocumentURL and navigation-delegate render callbacks. - Force-download subframe navigation responses that carry explicit download signals (Content-Disposition: attachment / force-download MIME), and apply the insecure-HTTP subframe block only once a download is actually chosen. Update resolver tests for the new classification. - Extract navigation popup policy, debug-URL helper, and the omnibar address button style out of BrowserPanel/BrowserPanelView into their own files; wire the new files into the Xcode project. - Localize the new PDF download/print strings (en + ja). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve conflicts in registry/list files: - Resources/Localizable.xcstrings: union of both branches' new keys (main's dock/paneMemoryGuardrail/automation strings + this branch's browser PDF/download strings); kept main's improved mobile.chat.error.transcriptNotReadable text. - cmux.xcodeproj/project.pbxproj: union of both branches' file/build entries (main's BrowserPaneSplitTarget + this branch's PDF toolbar / popup-policy extraction files), re-normalized. - .github/swift-file-length-budget.tsv: regenerated from the merged tree. vendor/bonsplit advanced to main's published pointer. Swift compiles clean (verified via tagged Debug build; the only build-phase failure is the pre-existing command-palette nucleo Rust FFI script, which needs the aarch64-apple-darwin target in Homebrew's cargo — an environment issue unrelated to this change). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve conflicts after main advanced (SSL "proceed anyway" #3711, etc.): - BrowserNavigationDelegate.swift (real code merge): * didStartProvisionalNavigation: keep main's lastAttemptedRequest URL fallback plus this branch's print-reset + didClearPDFDocument(). * shouldPerformDownload: keep this branch's subframe download gating (main-frame / user-activation / recorded-intent / insecure-HTTP block) and add main's clearAttemptedRequest() on the path that actually proceeds to .download. * keep this branch's WKWebView.cmuxRunPrintOperation() extension. - cmux.xcodeproj/project.pbxproj: union of both branches' new files (main's SSL/error-page files + this branch's PDF/popup-policy files), re-normalized; no duplicate entries. - .github/swift-file-length-budget.tsv: regenerated from merged tree. Localizable.xcstrings auto-merged. Swift compiles clean (tagged Debug build SUCCEEDED). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Auto-review caught that noteRenderedPDFDocument stored renderedPDFDocumentURL for subframe PDFs too, so an embedded <iframe> PDF (e.g. a Gmail/Drive-style preview) would show the omnibar Download/Print buttons even though the visible page is not a PDF — and Print runs on the main web view, printing the host page instead of the embedded document. Gate on isMainFrame so only the top-level PDF drives the toolbar. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| private func v2IsTerminalBrowserDownloadEvent(_ event: [String: Any]) -> Bool { | ||
| let type = event["type"] as? String | ||
| return type == "saved" || type == "cancelled" || type == "failed" | ||
| } |
There was a problem hiding this comment.
"ready_to_save" consumed key does not block the subsequent terminal event, poisoning the next automation call
When askWhereToSaveDownloads is true, v2WaitForDownloadEvent returns on "ready_to_save" and marks the consumed key as "ready_to_save\u{0}<downloadID>". Because v2IsTerminalBrowserDownloadEvent returns false for "ready_to_save", the bare downloadID is never stored in consumed. After the user dismisses the save panel, the resulting "saved", "cancelled", or "failed" event arrives at v2RecordBrowserDownloadEvent: v2ShouldStoreBrowserDownloadEvent checks consumed.contains(downloadID) (false) and consumed.contains("saved\u{0}<downloadID>") (false) — so the terminal event IS queued. The next browser.download call on the same surface pops it immediately as if a new download completed, with stale path/state from the previous interaction.
| private func v2IsTerminalBrowserDownloadEvent(_ event: [String: Any]) -> Bool { | |
| let type = event["type"] as? String | |
| return type == "saved" || type == "cancelled" || type == "failed" | |
| } | |
| private func v2IsTerminalBrowserDownloadEvent(_ event: [String: Any]) -> Bool { | |
| let type = event["type"] as? String | |
| return type == "saved" || type == "cancelled" || type == "failed" || type == "ready_to_save" | |
| } |
…wnloads # Conflicts: # .github/swift-file-length-budget.tsv
Downloads already saved correctly to ~/Downloads, but the only feedback was a transient toolbar spinner that vanished on completion, so finished downloads were invisible and felt like failures. Add a persistent downloads affordance: - BrowserDownloadRecord: immutable per-download snapshot (id/filename/ url/state/size) passed below the popover's ForEach boundary (no store crosses it, per the snapshot-boundary rule). - BrowserPanel.recentDownloads (+ applyBrowserDownloadEvent, open/reveal/ clear) fed from both download paths — the WKDownload navigation/response handlers and the session/context-menu path — via the existing event vocabulary (started/saved/failed/cancelled). Existing browserDownloadEventDidArrive posts are unchanged. - BrowserDownloadsToolbarButton: omnibar button (spinner while active) opening a popover that lists recent downloads with Open / Show in Finder, an empty state, and Clear. - Localized new strings (en + ja). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…wnloads # Conflicts: # .github/swift-file-length-budget.tsv
applyBrowserDownloadEvent originally dispatched its mutation to main with the same DispatchQueue.main.async(execute:) dance as begin/endDownloadActivity, which added a third instance of the budgeted non-sendable-closure concurrency warning in BrowserPanel.swift and tripped the Swift warning budget gate. All callers already run on the main thread (the WKDownload callbacks fire inside a @mainactor Task / notifyOnMain, and the session path hops to main before delivering), so fold the event synchronously with a main-thread assert instead of re-dispatching. No new warnings; begin/endDownloadActivity are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The button rendered a static icon with a tiny scaled spinner that was easy to miss, so an in-flight or finished download didn't read as anything happening. - Use a plain SF Symbol so `.symbolEffect` applies: a continuous bounce while a download is in flight, and a one-shot bounce each time one completes. - Tint the button with the accent color whenever it is downloading or has downloads. - Add a count badge (springs in) so completed downloads are visible at a glance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The accent-colored button lit up automatically and read as too loud. Make it match the rest of the omnibar instead: - Monochrome icon; motion carries the state — a spinner while a download is in flight (a repeating .bounce would require macOS 15; the deployment target is 14), and a discrete bounce each time one lands. - The count becomes a red notification bubble that clears when the popover is opened (tracks unseen download ids). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…wnloads # Conflicts: # .github/swift-file-length-budget.tsv
… error-page URL + subframe download blocking), delete stale inline copy + dup debugURL, add insecure-HTTP helpers, wire pbxproj (round 42-fix)
This branch was successfully deployed
1 participant
Summary
<a download>and blob flows can reach the native bridge~/Downloadsby default with collision-safe filenames, with an opt-in setting to ask where to save each fileCloses #6754
Verification
git diff --check.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes browser downloads started in subframes and unifies all download paths with trusted, user-activated intents, safer redirect/HTTP gating, and consistent completion/events. Adds a persistent downloads button with a popover and an opt‑in
browser.askWhereToSaveDownloadssetting.Bug Fixes
New Features
browser.askWhereToSaveDownloadswith settings UI, search anchors/aliases, command palette toggle, JSON schema, and localization. Off by default.Written for commit c6c6b6d. Summary will update on new commits.
Summary by CodeRabbit