Repository navigation
browser: expose hidden webview discard settings - #4283
austinywang wants to merge 19 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:
📝 WalkthroughWalkthroughThis PR adds a configurable hidden-WebView memory-discard feature: a policy resolves enabled/delay settings, a manager schedules/cancels delayed discards using blocker snapshots, BrowserPanel snapshots/replaces/restores WKWebView instances, settings/schema/localization/search and parsing are wired, and tests exercise policy behavior. ChangesHidden WebView Discard for Memory Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings, 3 inconclusive)
✅ Passed checks (9 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 |
Greptile SummaryThis PR exposes a user-facing Browser Memory Saver that discards hidden WebView tabs after a configurable delay and restores them on show. It supersedes #4245 and incorporates all of the follow-up fixes requested in that round of review.
Confidence Score: 5/5Safe to merge; all previous iteration findings are resolved and the new discard/restore paths are well-guarded. The manager extraction, @mainactor isolation, DispatchSourceTimer with generation guard, nonisolated policy type, and delegate wiring are all correctly implemented. The only remaining note is the deferred portal visibility Task in BrowserPanelView which is not stored in Coordinator for cancellation — it relies entirely on the generation guard to discard stale runs rather than cancelling them up-front. That is a minor structural concern with no observed failure path given the guards in place. Sources/Panels/BrowserPanelView.swift — the new schedulePortalLifecycleVisibilityUpdate helper creates Tasks that cannot be proactively cancelled between rapid updateNSView calls. Important Files Changed
Sequence DiagramsequenceDiagram
participant BPV as BrowserPanelView
participant BP as BrowserPanel
participant MGR as BrowserHiddenWebViewDiscardManager
participant POL as BrowserHiddenWebViewDiscardPolicy
participant UD as UserDefaults
BPV->>BP: noteWebViewVisibility(false)
BP->>MGR: scheduleIfNeeded(reason)
MGR->>POL: isEnabled / hiddenDelay
POL->>UD: reads UserDefaults.standard
POL-->>MGR: "enabled=true, delay=300s"
MGR->>MGR: arm DispatchSourceTimer(remaining)
Note over MGR: 300s later, no blockers
MGR-->>BP: didRequestDiscard
BP->>MGR: markDiscarded(reason, now)
Note over BP: Tab is .discarded
BPV->>BP: noteWebViewVisibility(true)
BP->>MGR: restoreIfNeeded(reason, performRestore)
MGR->>MGR: cancel + clearDiscardState
MGR-->>BP: performRestore closure
BP->>BP: "shouldRenderWebView=true, navigate"
Note over BP: Tab restored to .active
UD-->>MGR: didChangeNotification
MGR->>POL: resolved() compare policyState
alt policy changed
MGR-->>BP: policyDidChange
BP->>MGR: scheduleIfNeeded / cancel
end
Reviews (10): Last reviewed commit: "fix: preserve hidden portal lifecycle up..." | Re-trigger Greptile |
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)
2249-3220: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the discard policy/scheduler out of
BrowserPanel.swiftbefore merge.This adds another large chunk of lifecycle/policy code to a file that is already far past the repo’s size budget, and it pushes even more independent state-management logic into an already mixed-responsibility panel implementation. Please move the hidden-discard policy and scheduling/restore logic into dedicated types/files instead of growing this file further.
As per coding guidelines
**/{Sources,CLI,Packages,cmuxTests,cmuxUITests}/**/*.swift: “Do not add more than 250 lines to an existing production Swift file that is already over 800 lines” and “Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file.”🤖 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 2249 - 3220, Extract the hidden-webview discard policy, scheduler and restore logic into a new type (e.g. BrowserHiddenWebViewDiscardManager) in its own file and replace the in-file logic with a small interface from BrowserPanel that delegates to that manager; move BrowserHiddenWebViewDiscardPolicy (the nonisolated enum), the state properties hiddenWebViewDiscardTimer, hiddenWebViewDiscardPolicyCancellable, isWebViewDiscardedForMemory, webViewDiscardedAt, webViewLastDiscardReason, webViewLastRestoreReason, restoredSessionShouldRenderWebView and any related booleans/flags plus the methods hiddenWebViewDiscardBlockers(), scheduleHiddenWebViewDiscardIfNeeded(reason:), cancelHiddenWebViewDiscard(), reevaluateHiddenWebViewDiscardScheduling(reason:), installHiddenWebViewDiscardPolicyObserver(), discardHiddenWebViewForMemory(reason:now:), restoreDiscardedWebViewIfNeeded(reason:), clearWebViewDiscardState(reason:) and reactivateDiscardedWebViewWithoutNavigation(reason:) into that manager; expose a minimal API on the manager for BrowserPanel to call (e.g. start/stop/schedule/forceDiscard/restore/queryState) and update BrowserPanel to call those APIs (preserving behavior such as using webViewInstanceID, performing session snapshot/restore via existing BrowserPanel helpers like sessionNavigationHistorySnapshot(), bindWebView(_:) and restoreSessionNavigationHistory(...), and calling refreshWebViewLifecycleState()/refreshNavigationAvailability() where needed). Ensure all moved logic compiles by updating references to private members to use manager APIs and keep only UI/state orchestration in BrowserPanel.
🤖 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 3067-3093: The timer handler must be guarded by a
schedule-generation token so stale queued handlers can't act after a reschedule;
add a property like hiddenWebViewDiscardScheduleGeneration (increment it each
time scheduleHiddenWebViewDiscardIfNeeded runs), capture the current generation
in the timer's closure and verify it matches before cancelling/clearing
hiddenWebViewDiscardTimer and calling discardHiddenWebViewForMemory(reason:), in
addition to the existing webViewInstanceID check; ensure you increment the
generation before creating the new DispatchSource and clear/increment it again
when cancelling timers to fully invalidate older handlers.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2249-3220: Extract the hidden-webview discard policy, scheduler
and restore logic into a new type (e.g. BrowserHiddenWebViewDiscardManager) in
its own file and replace the in-file logic with a small interface from
BrowserPanel that delegates to that manager; move
BrowserHiddenWebViewDiscardPolicy (the nonisolated enum), the state properties
hiddenWebViewDiscardTimer, hiddenWebViewDiscardPolicyCancellable,
isWebViewDiscardedForMemory, webViewDiscardedAt, webViewLastDiscardReason,
webViewLastRestoreReason, restoredSessionShouldRenderWebView and any related
booleans/flags plus the methods hiddenWebViewDiscardBlockers(),
scheduleHiddenWebViewDiscardIfNeeded(reason:), cancelHiddenWebViewDiscard(),
reevaluateHiddenWebViewDiscardScheduling(reason:),
installHiddenWebViewDiscardPolicyObserver(),
discardHiddenWebViewForMemory(reason:now:),
restoreDiscardedWebViewIfNeeded(reason:), clearWebViewDiscardState(reason:) and
reactivateDiscardedWebViewWithoutNavigation(reason:) into that manager; expose a
minimal API on the manager for BrowserPanel to call (e.g.
start/stop/schedule/forceDiscard/restore/queryState) and update BrowserPanel to
call those APIs (preserving behavior such as using webViewInstanceID, performing
session snapshot/restore via existing BrowserPanel helpers like
sessionNavigationHistorySnapshot(), bindWebView(_:) and
restoreSessionNavigationHistory(...), and calling
refreshWebViewLifecycleState()/refreshNavigationAvailability() where needed).
Ensure all moved logic compiles by updating references to private members to use
manager APIs and keep only UI/state orchestration in BrowserPanel.
🪄 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: 2f77df8d-f4a8-4c07-b6c3-72305774d273
📒 Files selected for processing (11)
Resources/Localizable.xcstringsSources/CmuxSettingsJSONPathSupport.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/GhosttyConfigTests.swiftweb/data/cmux.schema.json
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)
5496-5532:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReevaluate hidden-discard scheduling when DevTools becomes visible.
Lines 5498-5532 only reevaluate the discard scheduler on the hide path. That leaves any timer armed before DevTools opened still running through the DevTools-visible interval, so the next discard can happen earlier than the configured delay after DevTools is closed again.
Suggested fix
let visible = inspector.cmuxCallBool(selector: isVisibleSelector) ?? false setPreferredDeveloperToolsVisible(targetVisible) developerToolsTransitionTargetVisible = targetVisible + if targetVisible { + reevaluateHiddenWebViewDiscardScheduling(reason: "developer_tools_visibility_changed") + } if targetVisible { if !visible { _ = revealDeveloperTools(inspector) } else {🤖 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 5496 - 5532, The code only calls reevaluateHiddenWebViewDiscardAfterDeveloperToolsHidden() when DevTools are hidden; to prevent an armed discard timer from firing while DevTools are visible, call reevaluateHiddenWebViewDiscardAfterDeveloperToolsHidden() when DevTools become visible as well. Specifically, in the targetVisible true path after you detect visibleAfterTransition (the block after inspector.cmuxCallBool(selector: isVisibleSelector) where you call syncDeveloperToolsPresentationPreferenceFromUI(), cancelDeveloperToolsRestoreRetry(), and scheduleDetachedDeveloperToolsWindowDismissal()), add a call to reevaluateHiddenWebViewDiscardAfterDeveloperToolsHidden() so the hidden-webview discard scheduler is rechecked/cancelled when DevTools open.
3623-3641:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPause the discard countdown when a download starts.
Line 3624 only flips the download state; it never invalidates or reevaluates an already-armed hidden-discard timer. If a hidden tab becomes ineligible because a download starts, the old timer can keep counting and discard too soon after the download finishes.
Suggested fix
private func beginDownloadActivity() { let apply = { + let wasDownloading = self.isDownloading self.activeDownloadCount += 1 self.isDownloading = self.activeDownloadCount > 0 + if !wasDownloading && self.isDownloading { + self.reevaluateHiddenWebViewDiscardScheduling(reason: "download.started") + } } if Thread.isMainThread { apply() } else { DispatchQueue.main.async(execute: apply)🤖 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 3623 - 3641, When a download starts beginDownloadActivity currently flips isDownloading but doesn't stop an already-armed hidden-discard timer, so add logic that, when activeDownloadCount transitions from 0→1, cancels/invalidates any pending hidden-webview-discard timer; implement or call a helper like cancelHiddenWebViewDiscard() or invalidateHiddenWebViewDiscardTimer() from inside the apply closure of beginDownloadActivity (keeping the DispatchQueue.main behavior) so the discard countdown is paused while downloading, and leave the existing scheduleHiddenWebViewDiscardIfNeeded(reason: "download.finished") in endDownloadActivity to re-arm/re-evaluate after downloads complete.
🤖 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/BrowserHiddenWebViewDiscardManager.swift`:
- Around line 18-51: The class BrowserHiddenWebViewDiscardManager mutates
several properties (discardTask, scheduleGeneration, isDiscardedForMemory,
discardedAt, lastDiscardReason, lastRestoreReason,
restoredSessionShouldRenderWebView, etc.) but isn't annotated with `@MainActor`,
so mark the class declaration with `@MainActor` to ensure all its instance methods
and property accessors are executed on the main actor; update the declaration
"final class BrowserHiddenWebViewDiscardManager" to "`@MainActor` final class
BrowserHiddenWebViewDiscardManager" and ensure any external callers or delegate
interactions (BrowserHiddenWebViewDiscardManagerDelegate usages) are invoked
from the main actor to avoid compiler warnings or data races.
In `@Sources/Panels/BrowserHiddenWebViewDiscardPolicy.swift`:
- Around line 31-37: The isEnabled(defaults:) function currently treats any
non-false env token as enabled; change it so the environment override only
enables the feature when the token explicitly matches a true set (e.g.
"1","true","yes","on") and otherwise falls through to honor UserDefaults. In
BrowserHiddenWebViewDiscardPolicy.isEnabled(defaults:) read
ProcessInfo.processInfo.environment["CMUX_BROWSER_HIDDEN_WEBVIEW_DISCARD_ENABLED"],
trim and lowercased, and if present return true only when it is in the
accepted-true list; if absent or any other value, return the existing
defaults.bool(forKey:) behavior (i.e. treat unknown tokens as no override).
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 5496-5532: The code only calls
reevaluateHiddenWebViewDiscardAfterDeveloperToolsHidden() when DevTools are
hidden; to prevent an armed discard timer from firing while DevTools are
visible, call reevaluateHiddenWebViewDiscardAfterDeveloperToolsHidden() when
DevTools become visible as well. Specifically, in the targetVisible true path
after you detect visibleAfterTransition (the block after
inspector.cmuxCallBool(selector: isVisibleSelector) where you call
syncDeveloperToolsPresentationPreferenceFromUI(),
cancelDeveloperToolsRestoreRetry(), and
scheduleDetachedDeveloperToolsWindowDismissal()), add a call to
reevaluateHiddenWebViewDiscardAfterDeveloperToolsHidden() so the hidden-webview
discard scheduler is rechecked/cancelled when DevTools open.
- Around line 3623-3641: When a download starts beginDownloadActivity currently
flips isDownloading but doesn't stop an already-armed hidden-discard timer, so
add logic that, when activeDownloadCount transitions from 0→1,
cancels/invalidates any pending hidden-webview-discard timer; implement or call
a helper like cancelHiddenWebViewDiscard() or
invalidateHiddenWebViewDiscardTimer() from inside the apply closure of
beginDownloadActivity (keeping the DispatchQueue.main behavior) so the discard
countdown is paused while downloading, and leave the existing
scheduleHiddenWebViewDiscardIfNeeded(reason: "download.finished") in
endDownloadActivity to re-arm/re-evaluate after downloads complete.
🪄 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: 0c22dd8d-4dc3-4e2d-b855-3fa67ace9a2d
📒 Files selected for processing (4)
Sources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserHiddenWebViewDiscardPolicy.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxproj
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/BrowserHiddenWebViewDiscardManager.swift`:
- Around line 127-133: The current nonisolated stop() uses
MainActor.assumeIsolated which is unsafe from deinit; replace that with an
explicit asynchronous hop to the main actor so cleanup runs on Main safely:
remove MainActor.assumeIsolated and instead spawn Task { `@MainActor` in cancel();
policyObservationTask?.cancel(); policyObservationTask = nil } (or MainActor.run
{ ... } inside an async Task) so stop() remains nonisolated but performs
main-thread cleanup without invoking assumeIsolated; update any callers/tests
expecting synchronous teardown accordingly.
In `@Sources/Panels/BrowserPanel.swift`:
- Line 3561: BrowserPanel is directly setting
hiddenWebViewDiscardManager.restoredSessionShouldRenderWebView in multiple
lifecycle paths; add an encapsulating API on HiddenWebViewDiscardManager such as
setRestoredSessionShouldRenderWebView(Bool) and
clearRestoredSessionShouldRenderWebView() (or a single
updateRestoredSessionRenderIntent(_:)) and replace all direct writes to
restoredSessionShouldRenderWebView in BrowserPanel with those manager methods so
the discard/session-snapshot invariant is maintained in one place; update
HiddenWebViewDiscardManager to perform any related side effects/validation when
the flag changes and replace every occurrence where BrowserPanel writes the
property (previously using
hiddenWebViewDiscardManager.restoredSessionShouldRenderWebView) to call the new
manager API.
🪄 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: c91919d5-3457-4bf3-840a-6545e3add447
📒 Files selected for processing (4)
Sources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserHiddenWebViewDiscardPolicy.swiftSources/Panels/BrowserPanel.swiftcmuxTests/GhosttyConfigTests.swift
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Stale automated review after follow-up commits. The requested extraction/scheduler changes have been implemented, the associated inline threads are resolved, and the current CodeRabbit check is green.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 3d15bc7. Configure here.

Supersedes #4245 with conflicts resolved against current main.\n\nChanges:\n- Adds Browser Memory Saver settings backed by UserDefaults, env overrides, and cmux.json.\n- Adds schema entries, Settings search wiring, reset handling, and English/Japanese localizations.\n- Clamps the hidden discard delay to 0...3600 seconds across runtime, UI, and cmux.json parsing.\n- Defers portal lifecycle visibility updates out of updateNSView with a generation guard and keeps discard timer work on MainActor.\n\nValidation:\n- Parsed Resources/Localizable.xcstrings and web/data/cmux.schema.json as JSON.\n- Ran git diff --check.\n- Attempted ./scripts/reload.sh --tag pr4245-browser-settings; local xcodebuild lock is currently held by another agent, so no tagged app path was produced yet.
Note
Medium Risk
Moderate risk because it changes
BrowserPanelwebview lifecycle/timer behavior and adds new UserDefaults/env/cmux.json inputs that can affect tab restoration and navigation timing.Overview
Adds a user-facing Browser Memory Saver that can discard hidden browser tabs after a configurable delay and restore them when shown again.
Introduces
BrowserHiddenWebViewDiscardPolicy(UserDefaults-backed with env overrides + 0–3600s clamping) and aBrowserHiddenWebViewDiscardManagerto centralize scheduling, blocker checks (downloads/devtools/popups/fullscreen/loading), and policy-change observation;BrowserPanelis refactored to delegate discard/restore state and telemetry fields to this manager.Wires the new toggle + delay controls into Settings UI (including reset behavior), settings search indexing/aliases, and
cmux.jsonsupport (schema, template, JSON path allowlist + parsing validation). Also defers portal visibility lifecycle updates out ofupdateNSViewto a generation-guarded MainActor task, and updates tests to cover policy/defaults and to assert boolean defaults viaobject(forKey:).Reviewed by Cursor Bugbot for commit 7669829. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Expose Browser Memory Saver to discard hidden tabs after a configurable delay and restore on show to reduce memory use. Adds settings, config, telemetry, and dedicated
BrowserHiddenWebViewDiscardPolicy+ MainActorBrowserHiddenWebViewDiscardManagerwith blocker-aware scheduling, policy observation, and strict 0–3600s delay clamping.New Features
browser.discardHiddenWebViews,browser.hiddenWebViewDiscardDelaySecondsincmux.json(schema + template),UserDefaultskeys, and envCMUX_BROWSER_HIDDEN_WEBVIEW_DISCARD_ENABLED/CMUX_BROWSER_HIDDEN_WEBVIEW_DISCARD_DELAY_SECONDSwith validation and clamping; JSON path allowlist + settings search anchors added.discarded, telemetry (eligibility, blockers, timestamps, last reasons), centralized scheduling viaBrowserHiddenWebViewDiscardManagerwith blocker checks and policy change observation; tests cover defaults and clamping.Bug Fixes
UserDefaultson MainActor and only reschedule when the resolved policy actually changes.stop().updateNSViewwith a generation-guarded MainActor task.Written for commit 7669829. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Settings
Localization
Tests
Chores