Repository navigation
Keep actively-playing media browser panes alive in the background (issue 5409) - #5412
Conversation
…sue 5409) Hidden browser panes are discarded for memory after the hidden delay (BrowserHiddenWebViewDiscardPolicy.defaultHiddenDelay = 300s), which stops playback and reloads the page on return. The media_capture blocker added in #4909 only covers camera/mic capture, not <video>/<audio> playback, so a backgrounded YouTube tab is discarded. This commit adds the failing test plus inert plumbing only: an isPlayingMedia field on BlockerSnapshot, the BrowserPanel stored property and injected media-playback hook, and the message handler. The discard decision itself is not yet wired, so the new test goes red while idle panes still discard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e 5409) Append a media_playback blocker when the pane has actively-playing media, so a hidden browser pane playing a YouTube video keeps its WebView alive instead of being discarded and reloaded on workspace switch. isPlayingMedia is driven by an injected main-frame media-session hook over a WKScriptMessageHandler (public DOM APIs, stable across WebKit versions, unlike private _isPlayingAudio KVO), and re-evaluates discard scheduling on change. Idle hidden panes with no playing media still discard, preserving the memory bound from #4539. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR prevents hidden WebView discard when media (video/audio) is actively playing. A JavaScript bootstrap script detects playback state per frame and reports changes to a native message handler, which updates BrowserPanel's playback-tracking state. This state populates a new ChangesMedia Playback Tracking for Discard Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
🤖 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 4203-4206: Resetting playback state currently only on provisional
navigations misses cases where a fresh WKWebView is swapped in; ensure you also
clear self.isPlayingMedia (and hiddenWebViewDiscardSnapshot.isPlayingMedia if
present) in the code paths that perform a web-view replacement/installation of a
blank replacement (the fresh-WKWebView replacement paths that create/assign a
new WKWebView), so any pane that had been playing cannot remain stuck as exempt
from discard after the view is replaced.
🪄 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: 38294f71-961e-44e6-bb12-6d47e6b6be42
📒 Files selected for processing (5)
Sources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserMediaPlayback.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserPanelTests.swift
Greptile SummaryThis PR fixes issue #5409 by adding a
Confidence Score: 5/5Safe to merge. The change is well-scoped: only panes with actively-playing media are exempted from discard, idle panes are unaffected, and two tests guard both invariants. The implementation carefully mirrors the existing media-capture blocker pattern, the content-world isolation is the correct mechanism to prevent spoofing, and the webViewInstanceID guard handles webview replacement. The only notable limitation — stale frameIDs on a renderer crash — is a theoretical over-retention scenario bounded by the next navigation, not a regression relative to the baseline. No files require special attention. BrowserPanel+MediaPlayback.swift is the most consequential file but its logic is sound and well-documented. Important Files Changed
Sequence DiagramsequenceDiagram
participant JS as Injected JS (all frames)
participant WK as WebKit / Main Thread
participant MH as BrowserMediaPlaybackMessageHandler
participant BP as BrowserPanel (MainActor)
participant DM as BrowserHiddenWebViewDiscardManager
JS->>JS: document-start: install play/pause/ended listeners + MutationObserver
JS->>WK: "postMessage({ frameID, playing })"
WK->>MH: userContentController(_:didReceive:)
MH->>BP: "MainActor.assumeIsolated { onReport(report) }"
BP->>BP: applyMediaPlaybackReport(frameID:isPlaying:)
BP->>BP: "isPlayingMedia = !playingMediaFrameIDs.isEmpty"
BP->>DM: reevaluateHiddenWebViewDiscardScheduling
DM->>DM: blockers(for: snapshot) → [media_playback] → no discard
Note over JS,DM: On main-frame navigation commit
WK->>BP: navigationDelegate.didCommit
BP->>BP: "resetMediaPlaybackTracking() → isPlayingMedia = false"
BP->>DM: reevaluateHiddenWebViewDiscardScheduling
DM->>DM: "schedule timer (or fire if >300s elapsed)"
Note over JS,DM: On pagehide (frame unload)
JS->>WK: "postMessage({ frameID, playing: false })"
WK->>MH: userContentController(_:didReceive:)
MH->>BP: applyMediaPlaybackReport(frameID:isPlaying:false)
BP->>BP: playingMediaFrameIDs.remove(frameID)
BP->>DM: reevaluateHiddenWebViewDiscardScheduling (if last frame)
Reviews (6): Last reviewed commit: "Apply media reports synchronously to pre..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func setupMediaPlaybackMessageHandler(for webView: WKWebView) { | ||
| let handler = BrowserMediaPlaybackMessageHandler { [weak self] playing in | ||
| self?.handleMediaPlaybackChange(isPlaying: playing) | ||
| } | ||
| mediaPlaybackMessageHandler = handler | ||
| webView.configuration.userContentController.add(handler, name: mediaPlaybackMessageHandlerName) | ||
| } |
There was a problem hiding this comment.
private(set) protection is nominal due to internal setter accessor
isPlayingMedia is declared private(set) in BrowserPanel.swift, signalling intent that only the owning file should mutate it. But setIsPlayingMedia (called here) and handleMediaPlaybackChange both have implicit internal access, so any code in the app module can call them to freely overwrite isPlayingMedia. The private(set) annotation therefore provides no real encapsulation.
The existing peer, isReactGrabActive, avoids this mismatch by using a plain @Published var with no access restriction. Either align isPlayingMedia to the same pattern (drop private(set), add @Published if Combine observation is eventually wanted) or move the BrowserMediaPlayback extension body into BrowserPanel.swift so the Swift private scope actually covers the setter.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
There was a problem hiding this comment.
Intentional: mutation funnels through applyMediaPlaybackReport (renamed from setIsPlayingMedia) so the private(set) keeps the per-frame Set aggregation as the single write path. Matches the repo guidance to funnel state writes through a method rather than expose a public setter.
— Claude Code
| window.addEventListener("pagehide", () => { | ||
| lastReported = false; | ||
| post(false); | ||
| }, true); |
There was a problem hiding this comment.
pagehide always posts false, bypassing dedup; minor but inconsistent with documented behaviour
The comment on mediaPlaybackTrackingBootstrapScriptSource says the hook "Reports only on change (debounced via lastReported)," but the pagehide handler sets lastReported = false and calls post(false) directly — so if the page was already not playing (lastReported === false), a duplicate {playing: false} message is sent to Swift on every pagehide. The Swift side absorbs duplicates cleanly via the didSet guard, so there is no behavioural bug. The intent (guarantee a false is sent on unload regardless of prior state) is sound, but it's worth either updating the comment to note this special case explicitly, or routing through report() after resetting lastReported so the dedup path is consistent.
There was a problem hiding this comment.
Addressed in the all-frames commit (0517cd8): the pagehide handler now early-returns when lastReported is already false, so it no longer posts a duplicate {playing:false}.
— Claude Code
…issue 5409) Inject the media-playback hook into every frame (forMainFrameOnly: false) so embedded players in cross-origin iframes (a news site embedding a YouTube, Vimeo, or Twitch iframe, etc.) also keep a hidden pane alive, not just main-frame video. Each frame tags its report with a stable per-document id; the panel keeps a Set of playing frame ids and stays exempt from discard while any frame is playing. The hook keys off <video>/<audio> paused/ended (mute-independent, so a user-muted playing video still counts, matching Chrome), and is purely passive (capture-phase listeners only, no console/global tampering) so it does not trip CAPTCHA fingerprint checks that live in cross-origin iframes. Reset tracking on main-frame navigation and webview rebind. Known limitation: Web-Audio-only playback with no media element is not detected, since a running AudioContext is not a reliable audible signal and would over-retain idle panes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dia-playback-hibernate
… 5409) Resetting isPlayingMedia in didStartProvisionalNavigation dropped a still-playing page's frame ids before the navigation committed. If the provisional navigation then failed or was canceled, WebKit keeps the old document alive and playing, but no media event re-fires, so the pane could be hidden and discarded while audio/video was still playing. Reset on didCommit instead, when the new top-level document has actually replaced the old one. A failed/canceled navigation never commits, so the surviving playing document stays protected; same-document (pushState) navigations do not fire didCommit, so a persisting SPA video keeps its frame id. Fresh-WKWebView swaps are still reset via setupMediaPlaybackMessageHandler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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)
Sources/Panels/BrowserPanel.swift (1)
4217-4225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't clear playback state before the new document commits.
didStartProvisionalNavigationfires while the old page can still be the live document. If that load is canceled or fails, the page keeps playing but never re-emitsplay, soisPlayingMediastays false and the hidden-pane discard timer can arm against active media. Clear this on commit instead, or restore the prior state on cancel/fail.Suggested fix
navigationDelegate.didStartProvisionalNavigation = { [weak self] webView in MainActor.assumeIsolated { guard let self, self.isCurrentWebView(webView, instanceID: boundWebViewInstanceID) else { return } self.isMainFrameProvisionalNavigationActive = true - self.resetMediaPlaybackTracking() self.refreshBackgroundAppearance() self.applyMuteState(to: webView, reason: "navigationStart") } } navigationDelegate.didCommit = { [weak self] webView in MainActor.assumeIsolated { guard let self, self.isCurrentWebView(webView, instanceID: boundWebViewInstanceID) else { return } + self.resetMediaPlaybackTracking() self.isMainFrameProvisionalNavigationActive = false self.publishCommittedURL(from: webView) self.applyMuteState(to: webView, reason: "navigationCommit") } }🤖 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 4217 - 4225, The reset of playback state is happening too early in navigationDelegate.didStartProvisionalNavigation (resetMediaPlaybackTracking and isMainFrameProvisionalNavigationActive = true) so a cancelled/failed provisional load can leave isPlayingMedia false; move the reset logic to the navigation commit path (e.g. navigationDelegate.didCommitNavigation) so the old document's playing state persists until the new document actually commits, and in addition handle provisional load failures/cancellations (e.g. navigationDelegate.didFailProvisionalNavigation / didFailNavigation or the cancellation callback) to restore the prior playback state or clear the isMainFrameProvisionalNavigationActive flag accordingly; update references to resetMediaPlaybackTracking(), isMainFrameProvisionalNavigationActive, and isCurrentWebView(webView, instanceID: boundWebViewInstanceID) to ensure the checks still run in the commit/fail handlers.
🤖 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/BrowserMediaPlayback.swift`:
- Around line 9-23: This file currently declares multiple top-level types; split
BrowserMediaPlaybackReport and BrowserMediaPlaybackMessageHandler into their own
files named after each type and keep only the BrowserPanel extension in
BrowserPanel+MediaPlayback.swift; move the struct BrowserMediaPlaybackReport
into BrowserMediaPlaybackReport.swift, the class
BrowserMediaPlaybackMessageHandler into
BrowserMediaPlaybackMessageHandler.swift, update any imports/visibility as
needed, and leave the existing BrowserPanel extension implementation in the new
BrowserPanel+MediaPlayback.swift so each major type lives in its own file.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 4217-4225: The reset of playback state is happening too early in
navigationDelegate.didStartProvisionalNavigation (resetMediaPlaybackTracking and
isMainFrameProvisionalNavigationActive = true) so a cancelled/failed provisional
load can leave isPlayingMedia false; move the reset logic to the navigation
commit path (e.g. navigationDelegate.didCommitNavigation) so the old document's
playing state persists until the new document actually commits, and in addition
handle provisional load failures/cancellations (e.g.
navigationDelegate.didFailProvisionalNavigation / didFailNavigation or the
cancellation callback) to restore the prior playback state or clear the
isMainFrameProvisionalNavigationActive flag accordingly; update references to
resetMediaPlaybackTracking(), isMainFrameProvisionalNavigationActive, and
isCurrentWebView(webView, instanceID: boundWebViewInstanceID) to ensure the
checks still run in the commit/fail handlers.
🪄 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: 26b51986-05ad-4e33-9ea0-e57239bfacd3
📒 Files selected for processing (4)
Sources/Panels/BrowserMediaPlayback.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserPanelTests.swift
| /// A per-frame media-playback report from the injected hook. | ||
| struct BrowserMediaPlaybackReport { | ||
| /// Stable id for the reporting frame's document, so the native side can | ||
| /// aggregate playback across the main frame and any (cross-origin) iframes. | ||
| let frameID: String | ||
| /// Whether that frame currently has any actively-playing media. | ||
| let isPlaying: Bool | ||
| } | ||
|
|
||
| /// Receives `{ frameID, playing }` from the injected media-playback hook and | ||
| /// forwards it to the owning ``BrowserPanel`` on the main actor. | ||
| /// | ||
| /// Mirrors ``ReactGrabMessageHandler``: a thin `NSObject` adapter so the panel | ||
| /// itself never has to conform to `WKScriptMessageHandler`. | ||
| final class BrowserMediaPlaybackMessageHandler: NSObject, WKScriptMessageHandler { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Split the new top-level declarations into type-named files.
This new file now contains BrowserMediaPlaybackReport, BrowserMediaPlaybackMessageHandler, and a BrowserPanel feature extension, which breaks the repo’s one-major-type-per-file rule and makes these symbols harder to locate. Move the helper types into their own files and keep the panel-specific code in a BrowserPanel+MediaPlayback.swift extension file.
As per coding guidelines, “One major type per file. Each struct, class, enum, actor, or protocol that is part of a public API (or has any meaningful body) lives in its own file named after the type.”
🧰 Tools
🪛 SwiftLint (0.63.3)
[Warning] 23-23: Classes should have an explicit deinit method
(required_deinit)
🤖 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/BrowserMediaPlayback.swift` around lines 9 - 23, This file
currently declares multiple top-level types; split BrowserMediaPlaybackReport
and BrowserMediaPlaybackMessageHandler into their own files named after each
type and keep only the BrowserPanel extension in
BrowserPanel+MediaPlayback.swift; move the struct BrowserMediaPlaybackReport
into BrowserMediaPlaybackReport.swift, the class
BrowserMediaPlaybackMessageHandler into
BrowserMediaPlaybackMessageHandler.swift, update any imports/visibility as
needed, and leave the existing BrowserPanel extension implementation in the new
BrowserPanel+MediaPlayback.swift so each major type lives in its own file.
… (issue 5409) Two over-retention fixes that keep the 4539 memory bound honest: - Removing a playing <video>/<audio> from the DOM fires no media event, so the frame stayed marked playing until a full navigation. While anything is playing, attach a MutationObserver and recheck on DOM changes; it is the one signal not throttled while the page is hidden, so a torn-down player now clears the blocker and the pane becomes discardable. - A report emitted by a replaced webview's still-registered content controller could arrive after resetMediaPlaybackTracking() and repopulate the frame set for a gone page, blocking discard forever. Bind the handler to the webViewInstanceID captured at setup and drop reports from a superseded generation, mirroring the existing KVO-observer guards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Panels/BrowserPanel.swift (1)
4229-4235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlso clear playback tracking on WebView replacement paths.
This reset only runs after a main-frame commit. The replacement flows at Line 4859 and Line 5447 still install a fresh
WKWebViewwithout clearingplayingMediaFrameIDs, so a hidden pane that was playing can stay stuck withisPlayingMedia == truewhile the replacement view is blank or waiting for manual recovery.Suggested fix
private func resetWebViewLifecycleMetadata(resetVisibility: Bool = true) { cancelHiddenWebViewDiscard() + resetMediaPlaybackTracking() webViewLifecycleState = .newTab if resetVisibility { webViewLastVisibleAt = nil webViewLastHiddenAt = nil🤖 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 4229 - 4235, When installing or replacing the WKWebView (the code paths that create and attach a fresh WKWebView instance), also clear playback tracking so a previously playing pane cannot remain marked as playing; call resetMediaPlaybackTracking() and explicitly clear the playingMediaFrameIDs collection (and any isPlayingMedia flags/state tied to those IDs) as part of the WKWebView replacement flow where a new view is created and installed.
🤖 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.
Duplicate comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 4229-4235: When installing or replacing the WKWebView (the code
paths that create and attach a fresh WKWebView instance), also clear playback
tracking so a previously playing pane cannot remain marked as playing; call
resetMediaPlaybackTracking() and explicitly clear the playingMediaFrameIDs
collection (and any isPlayingMedia flags/state tied to those IDs) as part of the
WKWebView replacement flow where a new view is created and installed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdf68f6d-080e-4d43-8fe0-17cbc658fd05
📒 Files selected for processing (1)
Sources/Panels/BrowserPanel.swift
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)
Sources/Panels/BrowserMediaPlayback.swift (1)
30-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAuthenticate
cmuxMediaPlaybackreports before mutating keep-alive state.Any JS running in a
WKWebViewcan callwindow.webkit.messageHandlers.cmuxMediaPlayback.postMessage(...).BrowserMediaPlaybackMessageHandlercurrently accepts{ frameID, playing }from the page as-is; the only native gating is dropping reports from supersededWKWebViewgenerations (fromWebViewInstanceIDcheck). A malicious script in the same live page can therefore send arbitrary{frameID, playing: true}to growplayingMediaFrameIDs, keepisPlayingMediatrue, and pin the"media_playback"blocker indefinitely. Bind a per-webview nonce/token into the injected bootstrap script (sealed so page JS can’t forge it) and require the matching token in the native payload before updatingplayingMediaFrameIDs/isPlayingMedia(mirroring howBrowserPickerMessageHandlergates untrustedWKScriptMessageinput). [Sources/Panels/BrowserMediaPlayback.swift:30-41, 104-110]
</review_comment>🤖 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/BrowserMediaPlayback.swift` around lines 30 - 40, The JS message handler userContentController(_:didReceive:) in BrowserMediaPlaybackMessageHandler currently trusts incoming {frameID, playing} and must be authenticated: add a per-webview sealed nonce/token (inject it via the bootstrap script) and require the token field in the WKScriptMessage payload before mutating playingMediaFrameIDs or isPlayingMedia; specifically, update userContentController(_:didReceive:) to validate body["token"] matches the stored per-webview token (same approach as BrowserPickerMessageHandler) and drop messages with missing/invalid tokens so only the genuine injected script can toggle media playback state.
🤖 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/BrowserMediaPlayback.swift`:
- Around line 119-142: The current MutationObserver in syncObserver creates a
full-document subtree watch and calls report() on every mutation, which causes
querySelectorAll in report() to rescan on high-churn pages; change the observer
callback to only trigger report when mutations touch media elements (inspect
mutation.removedNodes/mutation.addedNodes for tags "VIDEO"/"AUDIO" or elements
matching "video, audio") or maintain a live Set/count of known playing elements
(updated by anyPlaying/event listeners) and update that Set on mutations instead
of running querySelectorAll; modify the observer creation in syncObserver (and
the anonymous callback) to filter mutations before calling report and/or update
anyPlaying to read from the live Set, leaving mediaObserver, report, anyPlaying,
and disconnectObserver as the reference points to locate the changes.
---
Outside diff comments:
In `@Sources/Panels/BrowserMediaPlayback.swift`:
- Around line 30-40: The JS message handler userContentController(_:didReceive:)
in BrowserMediaPlaybackMessageHandler currently trusts incoming {frameID,
playing} and must be authenticated: add a per-webview sealed nonce/token (inject
it via the bootstrap script) and require the token field in the WKScriptMessage
payload before mutating playingMediaFrameIDs or isPlayingMedia; specifically,
update userContentController(_:didReceive:) to validate body["token"] matches
the stored per-webview token (same approach as BrowserPickerMessageHandler) and
drop messages with missing/invalid tokens so only the genuine injected script
can toggle media playback state.
🪄 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: f58bbf24-ebca-4885-8499-413699e8f6dd
📒 Files selected for processing (1)
Sources/Panels/BrowserMediaPlayback.swift
Honor the repo's one-major-type-per-file rule for new app-target files: split BrowserMediaPlayback.swift into BrowserMediaPlaybackReport.swift (struct), BrowserMediaPlaybackMessageHandler.swift (handler), and the BrowserPanel+MediaPlayback.swift extension. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e 5409) The merge with origin/main left project.pbxproj unparseable, and the manual wiring of the split files left the `+` path unquoted. xcodebuild rejected the project with "damaged ... parse error" before the tests/build jobs could run. In the OpenStep pbxproj format a `+` is not a legal unquoted-string character, so the path must be quoted exactly like BrowserPanel+MoveTabToNewWorkspace.swift. Rebuilt the media-playback entries onto origin/main's clean pbxproj and quoted the path. Verified with `xcodebuild -list` (reads cleanly, cmux-unit scheme present), normalize-pbxproj.py, and check-pbxproj.sh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The MutationObserver previously called report() (a full-document querySelectorAll) on every DOM mutation while playing, which is costly on mutation-heavy SPAs (live chat, infinite scroll) during playback. Recheck only when a mutation actually removes a media element; additions already fire a play event. Keeps the hidden-page removal-detection guarantee while removing the per-mutation rescan. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Page JavaScript could call window.webkit.messageHandlers.cmuxMediaPlayback
directly and post a fake {playing:true} report that is never cleared, pinning
isPlayingMedia true and keeping the pane exempt from hidden-webview discard
forever. Move both the injected script and the message handler into an isolated
WKContentWorld so the handler is invisible to page scripts. A content world
shares the DOM, so the media event listeners and MutationObserver still observe
real playback; only the privileged postMessage channel is hidden.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5246d23. Configure here.
…sue 5409)
The handler deferred each report through Task { @mainactor }, so a stale
playing:true emitted by a document just before it navigated away could run
after didCommit's resetMediaPlaybackTracking() and re-add a dead frame id that
never gets a matching false report, pinning the pane against hidden-webview
discard. WebKit delivers script messages on the main thread, so apply the
report inline via MainActor.assumeIsolated (the same pattern the navigation
delegate callbacks use). This keeps reports in WebKit's delivery order relative
to navigation callbacks, so a pre-navigation report is applied before the
reset.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Problem
#5409
Play a YouTube video in a browser pane, switch to another workspace, come back, and the pane has refreshed and the video stopped. Browser panes should persist in the background like Chrome: switching workspaces must not stop a playing video or reload the page.
Root cause (confirmed)
Hidden browser WebViews are discarded to bound memory after
BrowserHiddenWebViewDiscardPolicy.defaultHiddenDelay(300s), see #4539. The discard manager (Sources/Panels/BrowserHiddenWebViewDiscardManager.swift) exempts a set of blocker conditions. #4909 added amedia_captureblocker, but it is derived only fromcameraCaptureState/microphoneCaptureState(camera/mic capture), not media playback. A<video>playing audio/video uses neither, so a hidden pane has an empty blocker set, the 300s timer arms, anddiscardHiddenWebViewForMemorytears down and reloads on return.Class of bug: any background media (video, audio players, WebAudio) silently killed after the hidden delay.
On the secondary question (does WebKit also pause background
<video>the instant the pane is hidden, independent of discard): WebKit holds a media-playback process assertion that keeps audible playback alive for non-visible/out-of-window pages (the same mechanism that keeps Safari background-tab audio playing); it suspends rendering, not audio. So the discard is the actionable mechanism. Confirmed empirically in dogfood (see below).Fix
Mirror the existing
media_captureplumbing with amedia_playbackblocker:isPlayingMediatoBlockerSnapshot; append amedia_playbackblocker when set.BrowserPanel.isPlayingMedia(plainprivate(set)+didSet->reevaluateHiddenWebViewDiscardScheduling, mirroringisReactGrabActive) cancels a pending discard when playback starts and reschedules when it stops.WKScriptMessageHandler(Sources/Panels/BrowserMediaPlayback.swift), watching<video>/<audio>play/pause/ended/...and posting{playing}only on change.Why injected JS over the private
_isPlayingAudioKVO:_isPlayingAudiowould require string-keypath KVO plus anNSObject observeValue(both banned by the repo concurrency rules) and is a WebKit internal whose semantics drift between macOS versions. The injected hook uses only public DOM APIs, fits the existing message-handler infra (ReactGrab), and is version-stable. Trade-off: main-frame only (same CAPTCHA-tampering concern as the existing telemetry hooks), so media playing solely inside a cross-origin iframe degrades gracefully to current behavior.Memory bound from 4539 is intact: only panes with actively-playing media are exempt; idle hidden panes still discard.
Tests
Two-commit red/green (
cmuxTests/BrowserPanelTests.swift):testActiveMediaPlaybackBlocksHiddenWebViewDiscardScheduling) plus inert plumbing, with no discard decision wired -> red.media_playbackblocker -> green.A second test (
testIdlePaneWithoutMediaPlaybackStillSchedulesHiddenWebViewDiscard) asserts idle panes still schedule discard, guarding the memory bound. Behavior-level: asserts the discard-blocker decision and scheduling, not source shape.Localization
No new user-facing strings:
media_playbackis an internal blocker identifier and the debug log is#if DEBUGonly.Dogfood
To be filled after the approved cloud build: play a YouTube video, switch workspaces, wait past the hidden delay, return, confirm playback continued and no reload.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes when hidden WKWebViews are released (memory vs background playback) and adds injected page scripts; mitigations limit spoofing and stale state but behavior is user-visible if detection misses edge cases (e.g. Web Audio only).
Overview
Hidden browser panes with active video/audio playback are no longer torn down after the hidden-memory delay. The discard path gains a
media_playbackblocker driven byisPlayingMediaonBrowserPanel, which re-triggers hidden-webview discard scheduling when playback starts or stops (same pattern as React Grab).Playback is detected via a document-start injected hook in every frame (main and cross-origin iframes), posting per-frame
{ frameID, playing }through aWKScriptMessageHandlerin an isolated content world so page JS cannot spoof “always playing.” Native code aggregates frame IDs, ignores reports from superseded webviews, applies reports synchronously on the main actor (ordering vs navigation), and resets tracking on webview bind and on main-framedidCommit(not provisional start). The script uses public DOM media events plus a targeted MutationObserver when elements are removed while playing.Tests assert
media_playbackblocks discard scheduling and idle panes still schedule discard; new sources are wired in the Xcode project.Reviewed by Cursor Bugbot for commit f017273. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Keep actively playing media in browser panes alive across workspace switches so videos and audio don’t stop or reload. Fixes #5409 while preserving discard for idle panes, including playback inside cross-origin iframes.
Bug Fixes
media_playbackdiscard blocker driven by aggregated per-frameisPlayingMedia.<video>/<audio>and post{ frameID, playing }, running in an isolated content world so page JS can’t spoof playback; while playing, a MutationObserver rechecks only on media-element removal to clear the blocker without scanning on every DOM change.didCommitreset.webViewInstanceIDto drop reports from superseded webviews.Panels/BrowserPanel+MediaPlayback.swiftinproject.pbxprojsoxcodebuildparses cleanly.Refactors
BrowserMediaPlaybackReport.swift,BrowserMediaPlaybackMessageHandler.swift, andBrowserPanel+MediaPlayback.swift(no behavior change).Written for commit f017273. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests