Repository navigation
Reapply media-playback hibernation fix (issue 5409) - #5441
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR injects per-frame JS to detect playing ChangesMedia Playback Detection and Discard Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (18 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}
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 e17dadc. Configure here.
Greptile SummaryReapplies the media-playback hibernation fix (originally #5412, reverted in #5436 for unrelated shortcut/loading regressions). Hidden browser panes that are actively playing
Confidence Score: 5/5The change is safe to merge for the media-playback discard fix itself; the remaining risk is the shortcut/loading regression that triggered the prior revert, which is runtime behaviour not visible in the diff and requires macOS 15.7.4 dogfood as noted in the PR. The implementation is well-scoped: per-frame state is tracked in an isolated content world, reports are applied synchronously on the main actor preserving delivery order relative to didCommit resets, stale reports from superseded webview generations are dropped, and the JS hook is purely passive. No blocking primitives, no legacy concurrency, no logging leaks, and the new tests cover the key regression and the happy path. No files require special attention from a code-correctness standpoint. The prior revert was for shortcut/loading behaviour under real navigation, so end-to-end testing on macOS 15.7.4 before merging is the main remaining gate. Important Files Changed
Sequence DiagramsequenceDiagram
participant Page as Page JS (isolated world)
participant WK as WebKit IPC
participant MH as BrowserMediaPlaybackMessageHandler
participant BP as BrowserPanel (MainActor)
participant DM as BrowserHiddenWebViewDiscardManager
Page->>Page: play/pause/ended/pagehide event fires
Page->>Page: "report() → anyPlaying() → post({frameID, playing})"
Page->>WK: "postMessage({frameID, playing})"
WK->>MH: userContentController(_:didReceive:) [main thread]
MH->>MH: "MainActor.assumeIsolated { onReport(report) }"
MH->>BP: handleMediaPlaybackReport(_:fromWebViewInstanceID:)
BP->>BP: "guard instanceID == webViewInstanceID"
BP->>BP: applyMediaPlaybackReport(frameID:isPlaying:)
BP->>BP: isPlayingMedia.didSet → reevaluateHiddenWebViewDiscardScheduling
alt Media playing
BP->>DM: scheduleIfNeeded → blockers includes media_playback → no timer
else Media stopped
BP->>DM: scheduleIfNeeded → no blockers → discard timer armed
end
Note over WK,BP: didCommit fires on main-frame navigation
WK->>BP: navigationDelegate.didCommit
BP->>BP: resetMediaPlaybackTracking() → playingMediaFrameIDs.removeAll()
BP->>DM: reevaluateHiddenWebViewDiscardScheduling (loading blocker still active)
Reviews (3): Last reviewed commit: "Address media playback review feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// A per-frame media-playback report from the injected media-playback hook. | ||
| struct BrowserMediaPlaybackReport { |
There was a problem hiding this comment.
BrowserMediaPlaybackReport passes through a @MainActor closure in BrowserMediaPlaybackMessageHandler but does not explicitly declare Sendable conformance. Both stored properties (String, Bool) are Sendable, so Swift 6 will infer it, but making the conformance explicit documents the concurrency contract and matches the preferred shape from the actor isolation rule for pure value model types.
| /// A per-frame media-playback report from the injected media-playback hook. | |
| struct BrowserMediaPlaybackReport { | |
| /// A per-frame media-playback report from the injected media-playback hook. | |
| struct BrowserMediaPlaybackReport: Sendable { |
Rule Used: Flag new or materially worsened Swift 6 actor isol... (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.
Fixed by making BrowserMediaPlaybackReport explicitly Sendable.
— Claude Code
| func testIdlePaneWithoutMediaPlaybackStillSchedulesHiddenWebViewDiscard() { | ||
| let snapshot = makeHiddenWebViewDiscardBlockerSnapshot(isPlayingMedia: false) | ||
| let manager = BrowserHiddenWebViewDiscardManager() | ||
| let delegate = BrowserHiddenWebViewDiscardTestDelegate(snapshot: snapshot, hiddenAt: Date()) | ||
| manager.delegate = delegate | ||
|
|
||
| XCTAssertEqual(manager.blockers(for: snapshot), []) | ||
|
|
||
| manager.scheduleIfNeeded(reason: "test.hidden") | ||
|
|
||
| XCTAssertTrue(manager.hasScheduledDiscard) | ||
| } |
There was a problem hiding this comment.
discardRequestCount assertion in the idle-pane test
The sibling test (testActiveMediaPlaybackBlocksHiddenWebViewDiscardScheduling) also asserts delegate.discardRequestCount == 0 after scheduleIfNeeded. This test uses hiddenAt: Date() (pane hidden just now) so no immediate discard fires and the assert would pass — but omitting it means a future regression where scheduleIfNeeded incorrectly calls the delegate immediately would go undetected here.
There was a problem hiding this comment.
Fixed by adding the missing delegate.discardRequestCount == 0 assertion to the idle-pane Swift Testing case.
— Claude Code
![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 `@cmuxTests/BrowserPanelTests.swift`:
- Around line 161-195: These two XCTest-style methods
(testActiveMediaPlaybackBlocksHiddenWebViewDiscardScheduling and
testIdlePaneWithoutMediaPlaybackStillSchedulesHiddenWebViewDiscard) must be
converted in-place to the repository's Swift Testing style used for other
migrated tests instead of remaining as XCTestCase methods; update each test to
the Swift Testing equivalent used elsewhere in this file (preserve the same
setup: makeHiddenWebViewDiscardBlockerSnapshot(isPlayingMedia:),
BrowserHiddenWebViewDiscardManager, and BrowserHiddenWebViewDiscardTestDelegate)
and register them following the existing Swift Testing pattern in this file so
they run under the new harness while keeping semantics and assertions identical.
Ensure you do not introduce a new XCTestCase class or restore XCTest-only APIs;
follow the surrounding migrated-tests' structure and naming so test
discovery/registration matches the repo convention.
In `@Sources/Panels/BrowserPanel`+MediaPlayback.swift:
- Line 5: Replace the hardcoded message handler string with the single
source-of-truth constant: use the existing mediaPlaybackMessageHandlerName
wherever "cmuxMediaPlayback" is currently hardcoded (e.g., in the message
registration/handler attachment in BrowserPanel+MediaPlayback, around the
register/observe calls near the playback setup code and any removeHandler
calls). Ensure all references (registration, callback lookup, and removal)
reference mediaPlaybackMessageHandlerName so the handler name cannot drift
between locations.
🪄 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: 8d5318cb-5a67-4147-81af-76e1038e4ef6
📒 Files selected for processing (7)
Sources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserMediaPlaybackMessageHandler.swiftSources/Panels/BrowserMediaPlaybackReport.swiftSources/Panels/BrowserPanel+MediaPlayback.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserPanelTests.swift
This branch was successfully deployed
1 participant
Summary
Verification
./scripts/normalize-pbxproj.py cmux.xcodeproj/project.pbxproj./scripts/check-pbxproj.sh./scripts/reload.sh --tag reapply-5409scripts/launch-tagged-automation.sh reapply-5409 --mode allowAll --wait-socket 12(socket came up just after the 12s wait)CMUX_TAG=reapply-5409 scripts/cmux-debug-cli.sh new-pane --type browser --url https://example.com --focus trueCMUX_TAG=reapply-5409 scripts/cmux-debug-cli.sh browser --surface surface:2 wait --load-state complete --timeout-ms 10000https://example.com/loaded with titleExample Domain.Cmd+Lfocused the browser address bar and was consumed byhandleCustomShortcut.Cmd+Shift+Lopened a new browser and focused the address bar after the tagged app was fully ready.Notes
Note
Medium Risk
Touches hidden-webview discard policy and injects JS into all frames; prior revert cited shortcut/loading regressions, so behavior under navigation and workspace switches still warrants careful dogfood.
Overview
Reapplies the media-playback hibernation fix (issue 5409): hidden browser panes with active
<video>/<audio>playback are no longer discarded after the hidden delay.Detection injects a document-start hook in every frame (main + cross-origin iframes) inside an isolated
WKContentWorld, posts{ frameID, playing }to a native handler, and aggregates per-frame state intoisPlayingMedia. Muted playback still counts; Web Audio–only pages are intentionally out of scope.Discard integration adds
isPlayingMediato the hidden-webview blocker snapshot and a newmedia_playbackblocker inBrowserHiddenWebViewDiscardManager. Playback changes re-arm discard scheduling like other blockers.Lifecycle hardening resets tracking on top-level
didCommit(not provisional start), ignores reports from superseded webview generations, applies script messages synchronously on the main actor for ordering vs navigation, and usespagehideplus a targetedMutationObserverwhen media nodes are removed.Tests add Swift Testing coverage that playing media blocks discard scheduling while idle panes still schedule it.
Reviewed by Cursor Bugbot for commit e797acf. Bugbot is set up for automated code reviews on this repo. Configure here.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Reapplies the media-playback hibernation fix so hidden browser panes with active audio/video aren’t discarded. Restores all-frame playback reporting and the
media_playbackdiscard blocker to keep background videos running (issue 5409).WKContentWorldwith synchronous main-actor handling; reset on top-leveldidCommitand drop stale reports from replaced webviews; send a final stop onpagehideand use aMutationObserverto handle removed media.isPlayingMediato the hidden-webview snapshot and amedia_playbackblocker; tests verify playing panes block discard and idle panes still schedule discard.Written for commit e797acf. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests