Repository navigation
Add browser mute tab toggle for #4910 - #4911
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:
📝 WalkthroughWalkthroughThis PR implements per-browser-pane audio muting with WebKit private API, preserving mute state across navigation and app restart. BrowserPanel tracks mute state independently of web view replacement, applies mute via private selector on lifecycle events, persists state in session snapshots, and exposes toggle controls via workspace context menu and programmatic API. ChangesAudio Mute Feature for Browser Pane
Sequence DiagramssequenceDiagram
participant User
participant BrowserPanel
participant WKWebView
User->>BrowserPanel: navigate to page
BrowserPanel->>BrowserPanel: didStartProvisionalNavigation
BrowserPanel->>BrowserPanel: applyMuteState(isMuted)
BrowserPanel->>WKWebView: cmuxSetPageAudioMuted(isMuted)
BrowserPanel->>BrowserPanel: didCommit
BrowserPanel->>BrowserPanel: applyMuteState(isMuted)
BrowserPanel->>WKWebView: cmuxSetPageAudioMuted(isMuted)
BrowserPanel->>BrowserPanel: didFinish
BrowserPanel->>BrowserPanel: applyMuteState(isMuted)
BrowserPanel->>WKWebView: cmuxSetPageAudioMuted(isMuted)
sequenceDiagram
participant User
participant TabContextMenu
participant Workspace
participant BrowserPanel
participant BonsplitController
User->>TabContextMenu: click toggleAudioMute
TabContextMenu->>BrowserPanel: toggleMute()
BrowserPanel->>BrowserPanel: isMuted = !isMuted
BrowserPanel-->>Workspace: publish $isMuted change
Workspace->>Workspace: observe isMuted change
Workspace->>BonsplitController: updateTab(isAudioMuted: ...)
BonsplitController->>BonsplitController: update tab UI
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryAdds per-browser-pane audio muting owned by
Confidence Score: 5/5Safe to merge — mute intent is correctly gated on API availability, tab indicator and session state stay in sync with actual WebKit state, and backward-compatible session decode is covered by tests. All three issues raised in the prior review round (actor isolation on static constants, isMuted set before API confirmation, asymmetric unmute guard) have been addressed. The private SPI dispatch is guarded by responds(to:), isMuted is only published when the call succeeds, and the custom decoder defaults to false for old snapshots. No new logic or state defects were found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Bonsplit
participant Workspace
participant BrowserPanel
participant WKWebView
User->>Bonsplit: Tap Mute Tab in context menu
Bonsplit->>Workspace: delegate .toggleAudioMute(tab)
Workspace->>BrowserPanel: toggleMute()
BrowserPanel->>BrowserPanel: setMuted(!isMuted)
BrowserPanel->>WKWebView: cmuxSetPageAudioMuted(true)
WKWebView-->>BrowserPanel: "applied = true"
BrowserPanel->>BrowserPanel: "isMuted = true only if applied"
BrowserPanel-->>Workspace: "@Published isMuted fires"
Workspace->>Bonsplit: updateTab(isAudioMuted: true)
Bonsplit-->>User: Speaker-slash indicator on tab
Reviews (6): Last reviewed commit: "fix: keep browser mute state aligned wit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/CmuxWebView.swift (1)
8-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
unsafeBitCastABI for WKWebView private"_setPageMuted:"invocation (tie to_WKMediaMutedState)
- WebKit declares
- (void)_setPageMuted:(_WKMediaMutedState)mutedState(void return, single parameter).- The current implementation still casts/invokes with
UInt(@convention(c) (AnyObject, Selector, UInt) -> Void); confirm_WKMediaMutedState’s underlying integer type matches that ABI (or adjust the cast to the correct underlying type) so “selector exists” doesn’t mask potential UB/crash on parameter representation drift.🤖 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.swift` around lines 8 - 30, The unsafeBitCast is using UInt which may not match the underlying C enum type for _WKMediaMutedState; update the ABI to match that enum by changing the SetPageMutedFunction signature in cmuxSetPageAudioMuted to use Int (Objective-C NSInteger-sized type) instead of UInt, and change cmuxMediaMutedStateAudio to return an Int so the passed value uses the same underlying integer width; keep using Self.cmuxSetPageMutedSelector and the same call path in cmuxSetPageAudioMuted but cast the implementation to `@convention`(c) (AnyObject, Selector, Int) -> Void and pass muted ? Self.cmuxMediaMutedStateAudio : 0.
🤖 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/CmuxWebView.swift`:
- Around line 8-30: The unsafeBitCast is using UInt which may not match the
underlying C enum type for _WKMediaMutedState; update the ABI to match that enum
by changing the SetPageMutedFunction signature in cmuxSetPageAudioMuted to use
Int (Objective-C NSInteger-sized type) instead of UInt, and change
cmuxMediaMutedStateAudio to return an Int so the passed value uses the same
underlying integer width; keep using Self.cmuxSetPageMutedSelector and the same
call path in cmuxSetPageAudioMuted but cast the implementation to `@convention`(c)
(AnyObject, Selector, Int) -> Void and pass muted ?
Self.cmuxMediaMutedStateAudio : 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26ac5396-8328-4f10-b6a7-8ff3ff1d2d5f
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/Panels/CmuxWebView.swift
|
Greptile noted that the vendor/bonsplit submodule bump is opaque from this parent PR. The Bonsplit implementation is in manaflow-ai/bonsplit#137, and its tests/review/security checks are passing; the parent PR body also links that dependency. |
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 `@vendor/bonsplit`:
- Line 1: The vendor submodule is pinned to an unreachable commit that adds
isAudioMuted (seen in Sources/Bonsplit/Public/Types/Tab.swift and
BonsplitController.swift); update the submodule reference to a commit reachable
from upstream main (or push the branch containing
3427ca47c769558cc7f275e401bcf8f04a9fc745 to the remote) so the pin is stable,
then update any references to isAudioMuted in callers/tests if the chosen
upstream tip differs; ensure the submodule pointer in vendor/bonsplit is
advanced to the selected reachable commit and commit the updated gitlink.
🪄 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: 00c388fc-6cb3-465d-adef-6140004aec9d
📒 Files selected for processing (3)
Sources/SessionIndexView.swiftSources/Workspace.swiftvendor/bonsplit
|
Actionable comments posted: 0 |
![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 d29ca0e. Configure here.
|
Updated the Bonsplit dependency branch and parent gitlink to 9166c3639fabb80bca050d57d7cbf3fe66248c2e, which merges Bonsplit origin/main with the browser audio mute action. Also addressed the remaining mute feedback by keeping isMuted tied to successful mute application, removing the unused capability wrapper, switching the WebKit SPI argument to Int, and removing the no-longer-used cmux Localizable.xcstrings entries. |
This branch was successfully deployed
1 participant
Summary
Verification
_setPageMuted:exists and accepts the audio bit on local WebKit with a focused Swift selector probe.jq empty Resources/Localizable.xcstrings.plutil -lint vendor/bonsplit/Sources/Bonsplit/Resources/en.lproj/Localizable.strings vendor/bonsplit/Sources/Bonsplit/Resources/ja.lproj/Localizable.strings.git diff --checkandgit -C vendor/bonsplit diff --check.Dependency
Closes #4910
Note
Medium Risk
Uses private WebKit
_setPageMuted:via runtime selector invocation; behavior may vary by WebKit version, though failures are guarded and mute state still persists in the app.Overview
Adds per-pane browser tab audio mute controlled from the Bonsplit pane-tab context menu (not the address bar), with a muted speaker-slash on the tab via
isAudioMutedmetadata.BrowserPanelowns publishedisMuted, exposessetMuted/toggleMute, and reapplies mute whenever the web view is bound or navigation starts/commits/finishes/fails so intent survives WKWebView replacement.cmuxSetPageAudioMutedonWKWebViewcalls WebKit’s private_setPageMuted:when available.Mute is persisted in
SessionBrowserPanelSnapshot(backward-compatible decode), restored on session load, copied when duplicating a browser tab, and wired through workspace tab subscriptions and mirror tab drag payloads. Tests cover snapshot round-trip and missingisMutedkey.Reviewed by Cursor Bugbot for commit 2adc6cb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Tests
Chores