Repository navigation
Protect visible browser web views from discard - #6191
lawrencecchen wants to merge 3 commits into
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 ChangesWorkspace-visibility discard protection for hidden browser WebViews
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 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 |
![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/Workspace.swift`:
- Around line 10663-10669: The workspace-visibility discard-protection flag is
set to true when browser portals become visible through the
`setWorkspaceVisibilityProtectsHiddenWebViewDiscard` call, but this flag is not
being cleared when portals are hidden in `hideAllBrowserPortalViews()` or
`hideBrowserPortalView(source:)`. This causes inactive workspaces to retain the
"workspace_visible" blocker and prevents proper hidden-webview cleanup. Add
calls to `setWorkspaceVisibilityProtectsHiddenWebViewDiscard` with `false` in
both the `hideAllBrowserPortalViews()` method and the
`hideBrowserPortalView(source:)` method to clear the discard-protection flag
when hiding portals, ensuring the flag state matches the actual visibility state
of the browser portals.
🪄 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: 5d9725b9-b3f8-4dad-a3b8-ad27f477ea37
📒 Files selected for processing (4)
Sources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserPanel.swiftSources/Workspace.swiftcmuxTests/BrowserPanelTests.swift
Greptile SummaryAdds a
Confidence Score: 5/5Safe to merge; the new protection mechanism is well-tested and the existing workspace retire path correctly clears per-panel flags through The core fix (blocking discard when The interaction between Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant WCV as WorkspaceContentView
participant WS as Workspace
participant BP as BrowserPanel
participant DM as DiscardManager
WCV->>WS: setWorkspacePresentationVisible(true)
WS->>WS: reconcileBrowserPortalVisibilityForCurrentRenderedLayout
WS->>BP: setWorkspaceVisibilityProtectsHiddenWebViewDiscard(true)
BP->>DM: reevaluateHiddenWebViewDiscardScheduling
Note over DM: workspace_visible blocker → discard cancelled
Note over BP: SwiftUI transiently hides WebView
BP->>DM: noteWebViewVisibility(false)
DM-->>DM: blockers present (workspace_visible) → no discard
WCV->>WS: setWorkspacePresentationVisible(false)
WS->>WS: reconcileBrowserPortalVisibilityForCurrentRenderedLayout
WS->>BP: setWorkspaceVisibilityProtectsHiddenWebViewDiscard(false)
BP->>BP: "webViewLastHiddenAt = now (clock reset)"
BP->>DM: reevaluateHiddenWebViewDiscardScheduling
Note over DM: No blockers → discard timer scheduled
%%{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 WCV as WorkspaceContentView
participant WS as Workspace
participant BP as BrowserPanel
participant DM as DiscardManager
WCV->>WS: setWorkspacePresentationVisible(true)
WS->>WS: reconcileBrowserPortalVisibilityForCurrentRenderedLayout
WS->>BP: setWorkspaceVisibilityProtectsHiddenWebViewDiscard(true)
BP->>DM: reevaluateHiddenWebViewDiscardScheduling
Note over DM: workspace_visible blocker → discard cancelled
Note over BP: SwiftUI transiently hides WebView
BP->>DM: noteWebViewVisibility(false)
DM-->>DM: blockers present (workspace_visible) → no discard
WCV->>WS: setWorkspacePresentationVisible(false)
WS->>WS: reconcileBrowserPortalVisibilityForCurrentRenderedLayout
WS->>BP: setWorkspaceVisibilityProtectsHiddenWebViewDiscard(false)
BP->>BP: "webViewLastHiddenAt = now (clock reset)"
BP->>DM: reevaluateHiddenWebViewDiscardScheduling
Note over DM: No blockers → discard timer scheduled
Reviews (7): 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}
| private var pendingInteractiveBrowserPrompts: [PendingInteractiveBrowserPrompt] = [] | ||
| private var isPresentingPendingInteractiveBrowserPrompt = false | ||
| private var isWebViewVisibleInUI: Bool = false | ||
| private var isProtectedByWorkspaceVisibilityForResourceCleanup: Bool = false |
There was a problem hiding this comment.
isProtectedByWorkspaceVisibilityForResourceCleanup is a cached copy of workspace-level state kept on BrowserPanel. Its correctness depends entirely on every layout transition eventually calling reconcileBrowserPortalVisibilityForCurrentRenderedLayout. There are currently three call sites (layout change, toggleSplitZoom, canvas layout), but if a future transition is added and the reconcile call is missed — or if a panel is moved between windows in a multi-window path that lacks its own reconcile call — the flag remains true and the panel is silently exempt from discard indefinitely. The source of truth (renderedVisiblePanelIdsForCurrentLayout) lives in Workspace, not in BrowserPanel, so the panel can represent the wrong visibility without any compile-time or runtime signal.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
8c8de46 to
573ca6a
Compare
573ca6a to
4ff16f5
Compare
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/Workspace.swift`:
- Around line 10667-10673: The workspace-visibility blocker "workspace_visible"
is not being cleared when browser tabs are deselected, preventing hidden-WebView
discard for inactive tabs. In the `hideBrowserPortalsForDeselectedTabs` function
(and the related location at lines 11541-11547), when hiding deselected browser
portals, call `setWorkspaceVisibilityProtectsHiddenWebViewDiscard` with
`shouldBeVisible` set to false and an appropriate reason to clear the blocker,
ensuring that previously visible tabs don't retain the blocker after being
hidden.
🪄 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: 8509c4bb-e45f-42f0-ae27-f5dc49b00e3b
📒 Files selected for processing (4)
Sources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserPanel.swiftSources/Workspace.swiftcmuxTests/BrowserPanelTests.swift
| let shouldBeVisible = visiblePanelIds.contains(browserPanel.id) | ||
| if browserPanel.setWorkspaceVisibilityProtectsHiddenWebViewDiscard( | ||
| shouldBeVisible, | ||
| reason: reason | ||
| ) { | ||
| didChange = true | ||
| } |
There was a problem hiding this comment.
Clear the workspace-visibility blocker when a tab is deselected.
reconcileBrowserPortalVisibilityForCurrentRenderedLayout updates the new blocker, but normal tab selection hides deselected browser portals through hideBrowserPortalsForDeselectedTabs without clearing it. A previously visible browser tab can therefore keep the "workspace_visible" blocker after it is hidden, suppressing hidden-WebView discard for inactive tabs.
Proposed fix
private func hideBrowserPortalsForDeselectedTabs(inPane pane: PaneID, selectedTabId: TabID) {
for tab in bonsplitController.tabs(inPane: pane) {
guard tab.id != selectedTabId else { continue }
guard let panelId = panelIdFromSurfaceId(tab.id),
let browserPanel = panels[panelId] as? BrowserPanel else { continue }
+ _ = browserPanel.setWorkspaceVisibilityProtectsHiddenWebViewDiscard(
+ false,
+ reason: "tabDeselected"
+ )
browserPanel.hideBrowserPortalView(source: "tabDeselected")
}
}Also applies to: 11541-11547
🤖 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/Workspace.swift` around lines 10667 - 10673, The workspace-visibility
blocker "workspace_visible" is not being cleared when browser tabs are
deselected, preventing hidden-WebView discard for inactive tabs. In the
`hideBrowserPortalsForDeselectedTabs` function (and the related location at
lines 11541-11547), when hiding deselected browser portals, call
`setWorkspaceVisibilityProtectsHiddenWebViewDiscard` with `shouldBeVisible` set
to false and an appropriate reason to clear the blocker, ensuring that
previously visible tabs don't retain the blocker after being hidden.
4ff16f5 to
4a56cfa
Compare
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/BrowserPanel.swift`:
- Around line 8442-8445: The call to
setWorkspaceVisibilityProtectsHiddenWebViewDiscard in the generic portal-hide
path at line 8442 is prematurely clearing the workspace protection during
transient hides. Remove this call from the transient portal-hide path in
BrowserPanel.swift, and instead add a call to
setWorkspaceVisibilityProtectsHiddenWebViewDiscard with
clearWorkspaceVisibilityProtection: true in the authoritative
workspace-retirement or workspace-removal code path to ensure protection is only
cleared when the workspace is actually being retired or removed, not during
temporary portal visibility changes.
In `@Sources/Workspace.swift`:
- Around line 10665-10672: The code at line 10665 intentionally bypasses portal
reconciliation for canvas-inline browsers, but the follow-up predicate
browserPortalVisibilityNeedsFollowUp() (around lines 10737-10750) still expects
those visible panels to be portal-bound, creating a mismatch. Add a check in the
browserPortalVisibilityNeedsFollowUp() predicate to skip canvas-inline browsers
(checking canvasInlineHostingActive) the same way they are skipped during the
main visibility update, so that layout follow-up converges properly instead of
timing out.
🪄 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: dfbb8121-0a46-4dfa-b2c7-1c8371926d0b
📒 Files selected for processing (5)
Sources/Canvas/CanvasPaneContent.swiftSources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserPanel.swiftSources/Workspace.swiftcmuxTests/BrowserPanelTests.swift
| _ = setWorkspaceVisibilityProtectsHiddenWebViewDiscard( | ||
| false, | ||
| reason: "portal.\(source)" | ||
| ) |
There was a problem hiding this comment.
Don’t clear workspace protection from the generic portal-hide path.
Line 8442 removes the new blocker before recording the webview as hidden, so a workspace-visible browser can still become discard-eligible during the transient hide this PR is meant to tolerate. Clear this only from the authoritative workspace-retirement/removal path, not every portal hide.
🐛 Proposed fix
- func hideBrowserPortalView(source: String) {
- _ = setWorkspaceVisibilityProtectsHiddenWebViewDiscard(
- false,
- reason: "portal.\(source)"
- )
+ func hideBrowserPortalView(
+ source: String,
+ clearWorkspaceVisibilityProtection: Bool = false
+ ) {
+ if clearWorkspaceVisibilityProtection {
+ _ = setWorkspaceVisibilityProtectsHiddenWebViewDiscard(
+ false,
+ reason: "portal.\(source)"
+ )
+ }
noteWebViewVisibility(
false,
reason: "portal.\(source)",
recordIfUnchanged: true
)Update the workspace-retirement caller to pass clearWorkspaceVisibilityProtection: true; leave transient portal hides at the default false.
🤖 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 8442 - 8445, The call to
setWorkspaceVisibilityProtectsHiddenWebViewDiscard in the generic portal-hide
path at line 8442 is prematurely clearing the workspace protection during
transient hides. Remove this call from the transient portal-hide path in
BrowserPanel.swift, and instead add a call to
setWorkspaceVisibilityProtectsHiddenWebViewDiscard with
clearWorkspaceVisibilityProtection: true in the authoritative
workspace-retirement or workspace-removal code path to ensure protection is only
cleared when the workspace is actually being retired or removed, not during
temporary portal visibility changes.
| if browserPanel.canvasInlineHostingActive { | ||
| if browserPanel.setWorkspaceVisibilityProtectsHiddenWebViewDiscard( | ||
| false, | ||
| reason: "canvasInline.\(reason)" | ||
| ) { | ||
| didChange = true | ||
| } | ||
| continue |
There was a problem hiding this comment.
Skip canvas-inline browsers in the follow-up predicate too.
Line 10665 now intentionally bypasses portal reconciliation for canvasInlineHostingActive, but browserPortalVisibilityNeedsFollowUp() still expects those visible panels to be portal-bound. That mismatch can leave layout follow-up non-converging until timeout.
Proposed fix
private func browserPortalVisibilityNeedsFollowUp() -> Bool {
let visiblePanelIds = renderedVisiblePanelIdsForCurrentLayout()
for panel in panels.values {
guard let browserPanel = panel as? BrowserPanel else { continue }
+ if browserPanel.canvasInlineHostingActive { continue }
guard visiblePanelIds.contains(browserPanel.id) else { continue }
let anchorView = browserPanel.portalAnchorViewAlso applies to: 10737-10750
🤖 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/Workspace.swift` around lines 10665 - 10672, The code at line 10665
intentionally bypasses portal reconciliation for canvas-inline browsers, but the
follow-up predicate browserPortalVisibilityNeedsFollowUp() (around lines
10737-10750) still expects those visible panels to be portal-bound, creating a
mismatch. Add a check in the browserPortalVisibilityNeedsFollowUp() predicate to
skip canvas-inline browsers (checking canvasInlineHostingActive) the same way
they are skipped during the main visibility update, so that layout follow-up
converges properly instead of timing out.
4a56cfa to
79c26e6
Compare
b0123ca to
eeef54c
Compare
08c1252 to
93ce68b
Compare
93ce68b to
1d12181
Compare
…bernation # Conflicts: # .github/swift-file-length-budget.tsv
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
xcodebuild test -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-bhib-test -only-testing:cmuxTests/BrowserHiddenWebViewDiscardManagerTests/testWorkspaceVisibleBrowserPanelBlocksHiddenWebViewDiscard\n-./scripts/reload-cloud.sh --tag bhib\n\n## Issues\n- Closes Prevent cmux browser web views from resource cleanup while active #6189Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches browser memory lifecycle and workspace/portal visibility reconciliation; wrong gating could leave WebViews undiscarded or discard too aggressively, but behavior is scoped with explicit blockers and tests.
Overview
Adds a
workspace_visiblehidden-webview discard blocker so browser panels in the rendered layout are not memory-discarded when SwiftUI or the portal briefly reports the WebView hidden.Workspace tracks presentation visibility (
setWorkspacePresentationVisiblefromWorkspaceContentView) and drivessetWorkspaceVisibilityProtectsHiddenWebViewDiscardduring browser portal reconciliation, using the same rendered-panel set as layout visibility but only while the workspace is presented. Canvas inline hosting clears that protection on mount/render/occlude/unmount so discard follows canvas visibility instead.Clearing protection while the WebView is still hidden resets the hidden-at timestamp so the discard timer does not fire immediately after a stale hide. Portal hide and workspace retire clear the blocker as well.
Regression coverage is in
BrowserWorkspaceVisibleDiscardTests(visible panel + transient hide, clock restart, canvas-inline exclusion, background-mounted workspace).Reviewed by Cursor Bugbot for commit d43d404. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Prevents discarding browser WebViews that are still visible by adding a
workspace_visibleblocker keyed off the presentation-visible rendered layout. Canvas-inline hosts are excluded, and clearing protection while hidden restarts the discard timer.workspace_visibleto discard blockers/lifecycle payload; toggling re-schedules; clearing while hidden resets the hidden-at timestamp.Workspace.setWorkspacePresentationVisible; portal hide, canvas mount/render/unmount, and workspace retire clear it. Tests cover the blocker, clock restart, canvas-inline exclusion, workspace retire clearing, and background-mounted workspace exclusion.Written for commit d43d404. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests