Repository navigation
Fix browser panes stuck black after a failed discard-restore - #7533
Conversation
…try (#7504) A discarded browser webview whose restore navigation never commits (connection refused, WebKit content-process death, dead localhost dev server) permanently consumes its discard state, so every later reveal, reload, or automation touch no-ops and the pane stays black forever. Red tests only, per the two-commit regression policy: - R1: manager-level — a restore whose navigation never starts/commits must leave the pane discarded and retryable. - R2: panel end-to-end — connection-refused restore must leave the next restore touch able to retry. CI on this commit is expected to fail these tests; the fix lands in the next commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keep the discard state armed until a restore navigation actually commits, so failed restores retry on the next touch instead of leaving the pane permanently black: - BrowserHiddenWebViewDiscardManager: restoreIfNeeded no longer clears the discard state before navigating; new isRestoreNavigationPending state machine (noteRestoreNavigationStarted / Committed / DidNotCommit) driven by real navigation-delegate signals; in-flight restores dedupe instead of double-navigating; reactivateWithoutNavigation no longer consumes state without a commit. - BrowserPanel: didCommit / didFailNavigation / didCancelProvisionalNavigation hooks drive the state machine; error-page commits do not clear the state; stall detection retries silently-dead restores on the next reveal/automation touch; blank-shell heal re-navigates a never-committed shell that still has a URL intent on reveal transitions (never on visibility heartbeats, and never while an insecure-HTTP consent alert is pending); restore_pending / has_committed_document diagnostics. - BrowserDiscardRestoreHeal (new): pure, unit-testable predicates for heal and stall eligibility, plus relocated lifecycle diagnostics helpers to stay inside the BrowserPanel.swift length budget. - Green tests for the new state machine and heal predicates. Fixes #7504 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds discard/restore healing helpers, tracks restore-navigation state through discard and navigation lifecycles, refactors external and insecure-HTTP navigation resolution, and adds project wiring plus tests for restore retry and blank-shell behavior. ChangesDiscard/Restore Heal State Machine
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserPanel
participant BrowserHiddenWebViewDiscardManager
participant BrowserNavigationDelegate
participant BrowserDiscardRestoreHeal
BrowserPanel->>BrowserHiddenWebViewDiscardManager: noteRestoreNavigationStarted(reason)
BrowserNavigationDelegate->>BrowserPanel: didBecomeDownload(...)
BrowserPanel->>BrowserHiddenWebViewDiscardManager: noteRestoreNavigationCommitted(reason)
BrowserPanel->>BrowserDiscardRestoreHeal: isRestoreStalled(...)
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 SummaryFixes #7504 by keeping
Confidence Score: 5/5Safe to merge — the restore state machine is well-reasoned, all previous review issues are addressed, and the change is thoroughly covered by focused unit tests. All edge cases are guarded by attempt-scoped UUIDs and WKNavigation identity checks so stale completions cannot corrupt newer restore state. Test seam migration follows the canonical pattern. No correctness gaps identified. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Rev as Reveal/Reload
participant BP as BrowserPanel
participant Mgr as DiscardManager
participant WK as WebKit
participant ND as NavigationDelegate
Note over Mgr: isDiscardedForMemory = true
Rev->>BP: restoreDiscardedWebViewIfNeeded
BP->>Mgr: restoreIfNeeded (isDiscardedForMemory stays true)
BP->>WK: navigateWithoutInsecureHTTPPrompt
Note over Mgr: isRestoreNavigationPending = true
alt Commits
WK-->>ND: didCommit
ND->>BP: noteRestoreNavigationCommitted
Note over Mgr: isDiscardedForMemory = false
else Fails or cancelled
WK-->>ND: didFail / didCancel (WKNavigation?)
ND->>BP: noteRestoreNavigationDidNotCommit
Note over Mgr: isDiscardedForMemory = true (retryable)
else Terminal policy cancel
ND->>BP: didCancelNavigationPolicy(.terminal)
Note over Mgr: isDiscardedForMemory = false
else Main-frame download
WK-->>ND: didBecomeDownload
ND->>BP: noteRestoreNavigationCommitted(download)
Note over Mgr: isDiscardedForMemory = false
end
%%{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 Rev as Reveal/Reload
participant BP as BrowserPanel
participant Mgr as DiscardManager
participant WK as WebKit
participant ND as NavigationDelegate
Note over Mgr: isDiscardedForMemory = true
Rev->>BP: restoreDiscardedWebViewIfNeeded
BP->>Mgr: restoreIfNeeded (isDiscardedForMemory stays true)
BP->>WK: navigateWithoutInsecureHTTPPrompt
Note over Mgr: isRestoreNavigationPending = true
alt Commits
WK-->>ND: didCommit
ND->>BP: noteRestoreNavigationCommitted
Note over Mgr: isDiscardedForMemory = false
else Fails or cancelled
WK-->>ND: didFail / didCancel (WKNavigation?)
ND->>BP: noteRestoreNavigationDidNotCommit
Note over Mgr: isDiscardedForMemory = true (retryable)
else Terminal policy cancel
ND->>BP: didCancelNavigationPolicy(.terminal)
Note over Mgr: isDiscardedForMemory = false
else Main-frame download
WK-->>ND: didBecomeDownload
ND->>BP: noteRestoreNavigationCommitted(download)
Note over Mgr: isDiscardedForMemory = false
end
Reviews (36): Last reviewed commit: "Fix PR comment CI guard regressions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…scard-restore-black
![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
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)
3872-3889: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle restore commits that land on an error page (Sources/Panels/BrowserPanel.swift:3872-3889)
If
activeErrorPageDisplayURLis set here, the restore never reaches a terminal state:hasCommittedDocumentSinceWebViewReplacementflips totrue, butisRestoreNavigationPendingstays stuck andisDiscardedForMemorynever clears. AddnoteDiscardedWebViewRestoreNavigationDidNotCommit(reason: "error_page")in theelsebranch so this webview can recover normally.🤖 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 3872 - 3889, The restore commit path in navigationDelegate.didCommit leaves error-page restores stuck because it only calls noteDiscardedWebViewRestoreNavigationCommitted when activeErrorPageDisplayURL is nil. Update the didCommit closure in BrowserPanel so the error-page case calls noteDiscardedWebViewRestoreNavigationDidNotCommit(reason: "error_page") in the else branch, allowing isRestoreNavigationPending and isDiscardedForMemory to clear correctly while preserving the existing commit handling for normal navigations.
🤖 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/BrowserDiscardRestoreHeal.swift`:
- Around line 3-65: The issue is that BrowserDiscardRestoreHeal is just a
stateless namespace, but it is unnecessarily declared as a caseless enum and
marked `@MainActor`. Move the pure boolean helpers into the existing BrowserPanel
extension as nonisolated static funcs, matching the pattern already used there,
so they no longer depend on main-actor isolation. Keep the formatter-backed
helpers (webViewLifecycleTimestamp and webViewHiddenDurationMilliseconds)
actor-isolated or otherwise protected, since they share the mutable
ISO8601DateFormatter instance.
In `@Sources/Panels/BrowserHiddenWebViewDiscardManager.swift`:
- Around line 260-274: The restore-navigation lifecycle methods in
BrowserHiddenWebViewDiscardManager are ignoring their reason parameter, so add
DEBUG-only cmuxDebugLog calls in noteRestoreNavigationStarted(reason:) and
noteRestoreNavigationDidNotCommit(reason:) to mirror the existing
noteSystemWillSleep/noteSystemDidWake logging. Include the passed reason in the
log message, and keep the current state changes in place so the transitions
remain diagnosable without changing behavior.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3872-3889: The restore commit path in navigationDelegate.didCommit
leaves error-page restores stuck because it only calls
noteDiscardedWebViewRestoreNavigationCommitted when activeErrorPageDisplayURL is
nil. Update the didCommit closure in BrowserPanel so the error-page case calls
noteDiscardedWebViewRestoreNavigationDidNotCommit(reason: "error_page") in the
else branch, allowing isRestoreNavigationPending and isDiscardedForMemory to
clear correctly while preserving the existing commit handling for normal
navigations.
🪄 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: a1f5242f-fe53-4de7-8c5a-59138e1b99ff
📒 Files selected for processing (5)
Sources/Panels/BrowserDiscardRestoreHeal.swiftSources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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)
cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift (1)
87-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest connects to a fixed loopback port instead of using a local fake server.
browserPanelRetriesDiscardedRestoreAfterConnectionRefusednavigates tohttp://127.0.0.1:1/...to force a connection-refused error. This relies on real OS-level socket behavior on a hardcoded port rather than a controlled local fixture, which can behave inconsistently across sandboxed CI/dev environments (firewall rules, IPv6/IPv4 resolution order, or restricted socket entitlements).As per path instructions for
{cmuxTests/**,...}: "Do not bind a fixed non-zero port or hit a live network host in tests; use a local fake or an ephemeral server instead." Consider binding an ephemeral local listener that immediately closes (guaranteeingECONNREFUSEDdeterministically) instead of hardcoding port1.🤖 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 `@cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift` around lines 87 - 110, The test in BrowserDiscardedWebViewRestoreRetryTests should not rely on the hardcoded loopback port to trigger connection refusal. Replace the fixed http://127.0.0.1:1 URL in browserPanelRetriesDiscardedRestoreAfterConnectionRefused with a controlled local fake or ephemeral server fixture that deterministically produces ECONNREFUSED, and keep the rest of the BrowserPanel discard/restore retry flow unchanged. Use the existing test helper/setup patterns in cmuxTests to locate the right place to introduce the ephemeral listener or fake server.Source: Path instructions
🤖 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 `@cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift`:
- Around line 87-110: The test in BrowserDiscardedWebViewRestoreRetryTests
should not rely on the hardcoded loopback port to trigger connection refusal.
Replace the fixed http://127.0.0.1:1 URL in
browserPanelRetriesDiscardedRestoreAfterConnectionRefused with a controlled
local fake or ephemeral server fixture that deterministically produces
ECONNREFUSED, and keep the rest of the BrowserPanel discard/restore retry flow
unchanged. Use the existing test helper/setup patterns in cmuxTests to locate
the right place to introduce the ephemeral listener or fake server.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b41d8c94-effb-47dc-907b-2fd57f3309cd
📒 Files selected for processing (6)
Sources/Panels/BrowserDiscardRestoreHeal.swiftSources/Panels/BrowserHiddenWebViewDiscardManager.swiftSources/Panels/BrowserNavigationDelegate.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift
A navigation commit to about:blank (e.g. the placeholder document) must not count as a successful discarded-webview restore; gate the restore-commit bookkeeping on a real committed URL. Harden the retry test to wait for the restore-pending flag to clear instead of only waiting for loading to settle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…scard-restore-black # Conflicts: # cmux.xcodeproj/project.pbxproj
…ad callback The discarded-restore fix adds a didBecomeDownload callback (property plus two delegate call sites, +3 lines) to BrowserNavigationDelegate.swift. Accept the growth in the checked-in budget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
webViewLifecycleTimestamp and webViewHiddenDurationMilliseconds are pure formatters and do not need MainActor isolation; align them with the sibling nonisolated helpers in BrowserDiscardRestoreHeal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 (2)
cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift (1)
104-127: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid fixed localhost port assumptions in this regression test.
Line 106 relies on
127.0.0.1:1being connection-refused. That is environment-dependent; use a test-owned ephemeral local server/port and close it before the restore attempt, or inject a deterministic navigation failure path.As per coding guidelines, tests must not hit a live network host or rely on a fixed non-zero port.
Source: Coding guidelines
Sources/Panels/BrowserPanel.swift (1)
3826-3842: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not mark ignored restore commits as committed documents.
Line 3830 flips
hasCommittedDocumentSinceWebViewReplacementbefore theabout:blank/ error-page gate. If a pending discarded restore commitsabout:blank,restore_pendingremains true, but laterisRestoreStalled(... hasCommittedDocument: true)returns false, so the stale pending restore is never cleared/retried.Proposed fix
- self.hasCommittedDocumentSinceWebViewReplacement = true + let isRecoverableDocumentCommit = self.shouldTreatCommitAsDiscardedRestoreCommit(from: webView) + if isRecoverableDocumentCommit { + self.hasCommittedDocumentSinceWebViewReplacement = true + } // Reset playback tracking only once the new top-level document has @@ - if self.shouldTreatCommitAsDiscardedRestoreCommit(from: webView) { + if isRecoverableDocumentCommit { self.noteDiscardedWebViewRestoreNavigationCommitted() }As per path instructions, correctness-critical restore lifecycle state should have one reliable state transition rather than disagreeing fallback flags.
🤖 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 3826 - 3842, The restore-commit path in navigationDelegate.didCommit is marking every commit as a committed document too early, which breaks ignored about:blank/error-page restore handling. Move the hasCommittedDocumentSinceWebViewReplacement update so it only happens after the restore gate in didCommit, or explicitly avoid setting it for discarded restore commits before calling shouldTreatCommitAsDiscardedRestoreCommit, and keep the state transition consistent with noteDiscardedWebViewRestoreNavigationCommitted and isRestoreStalled.Source: Path instructions
🤖 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/BrowserPanel.swift`:
- Around line 3826-3842: The restore-commit path in navigationDelegate.didCommit
is marking every commit as a committed document too early, which breaks ignored
about:blank/error-page restore handling. Move the
hasCommittedDocumentSinceWebViewReplacement update so it only happens after the
restore gate in didCommit, or explicitly avoid setting it for discarded restore
commits before calling shouldTreatCommitAsDiscardedRestoreCommit, and keep the
state transition consistent with noteDiscardedWebViewRestoreNavigationCommitted
and isRestoreStalled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 906720b8-3c7d-4a74-bbe2-17e137e98b23
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift
💤 Files with no reviewable changes (1)
- cmux.xcodeproj/project.pbxproj
Two review findings on the restore retry state machine: - A main-frame download cleared discard state but never committed a document, so blank-shell healing re-navigated to the download URL on every reveal, restarting the download. Treat a main-frame download as a committed terminal outcome for the replaced web view. - A discarded pane whose restore URL is nil or about:blank navigated (or skipped navigating) into a state whose commit is intentionally ignored, leaving the manager marked discarded (or restore-pending) forever and blocking future discards. Reactivate such panes in place through the existing reactivateWithoutNavigation path. Widen navigationDelegate to internal so the download regression test can drive the didBecomeDownload callback via @testable import, and raise the test settle timeout for loaded CI hosts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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/BrowserDiscardedWebViewRestoreRetryTests.swift`:
- Line 423: The assertion in BrowserDiscardedWebViewRestoreRetryTests should use
a direct Swift type check instead of a cast check to satisfy SwiftLint’s
prefer_type_checking rule. Update the expectation around
payload["discard_blockers"] to verify the value with an is-type check rather
than as? [String], keeping the same test intent while aligning with the lint
rule.
In `@Sources/Panels/BrowserPanel.swift`:
- Line 2994: The BrowserPanel navigation delegate is currently publicly
writable, which lets unrelated code replace or clear it and disrupt
restore/navigation wiring. Update BrowserPanel’s navigationDelegate to keep the
setter private while still allowing tests to read it and invoke callbacks, and
preserve the existing delegate lifecycle behavior without introducing any new
state or test-only seams.
🪄 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: 930d938d-5c42-480d-ab85-eac66a11b6bd
📒 Files selected for processing (3)
Sources/Panels/BrowserDiscardRestoreHeal.swiftSources/Panels/BrowserPanel.swiftcmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift
…cate A prewarmed webview is only claimable after its load finished, but the commit happened under the pool's delegate, so the panel's hasCommittedDocumentSinceWebViewReplacement stayed false and blank-shell healing reloaded the adopted page on first reveal. Seed the flag at adoption. Move shouldTreatCommitAsDiscardedRestoreCommit next to its sibling restore-heal predicates in BrowserDiscardRestoreHeal.swift and refresh the BrowserPanel.swift length budget for the net restore-retry growth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…scard-restore-black
A webview replaced after WebContent process termination waits for the user's explicit Reload (hasRecoverableWebContentTermination). The blank-shell heal predicate did not know about that gate, so a hidden crashed pane would auto-navigate on the next reveal, clear the recovery overlay, and could re-enter the crash loop. Add the recovery flag to shouldHealBlankShell and cover it in the predicate tests. Also move the no-restorable-URL restore fallback into BrowserDiscardRestoreHeal so BrowserPanel.swift stays below its pre-PR length (the guard job's hard cap forbids any growth of files over 900 lines), and drop the now-unneeded budget bump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
webViewLifecycleTopPayload runs on the polled debug-socket/top path for every browser panel; allocate the documented-thread-safe formatter once instead of per timestamp field, matching CmuxEventBus and Workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 23 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
1 similar comment
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 23 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort 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 3042ee2. Configure here.
…scard-restore-black # Conflicts: # Sources/Panels/BrowserNavigationDelegate.swift
|
Review comments addressed and branch updated at 74d3507. @coderabbitai resume |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 23 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReviews resumed. Review finished.
|
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)
cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift (1)
364-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead conditional block after assertion.
Line 361 asserts
terminalCancellationCount == 0, so theif terminalCancellationCount > 0block at lines 364-366 is unreachable in the passing case. Since Swift Testing's#expectcontinues execution after failure, a failed assertion here would cause theifblock to execute and fire a terminal cancellation simulation, producing confusing downstream assertion failures that mask the root cause.🧹 Remove dead conditional
`#expect`(terminalCancellationCount == 0) `#expect`(fallbackRequest?.url == fallbackURL) - if terminalCancellationCount > 0 { - panel.navigationDelegate?.didCancelNavigationPolicy?(panel.webView, .terminal(restoreAttemptID: panel.currentDiscardRestoreAttemptID)) - } panel.navigationDelegate?.didCancelProvisionalNavigation?(panel.webView, 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 `@cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift` around lines 364 - 366, Remove the dead conditional in BrowserDiscardedWebViewRestoreRetryTests around the terminal cancellation simulation: the preceding assertion on terminalCancellationCount already makes the `if terminalCancellationCount > 0` branch unreachable in the passing case, and with `#expect` a failing assertion should not trigger `panel.navigationDelegate?.didCancelNavigationPolicy` using `panel.webView` and `panel.currentDiscardRestoreAttemptID`. Delete that block so the test stops emitting misleading follow-on failures and keeps the focus on the original assertion.
🤖 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 `@cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift`:
- Around line 364-366: Remove the dead conditional in
BrowserDiscardedWebViewRestoreRetryTests around the terminal cancellation
simulation: the preceding assertion on terminalCancellationCount already makes
the `if terminalCancellationCount > 0` branch unreachable in the passing case,
and with `#expect` a failing assertion should not trigger
`panel.navigationDelegate?.didCancelNavigationPolicy` using `panel.webView` and
`panel.currentDiscardRestoreAttemptID`. Delete that block so the test stops
emitting misleading follow-on failures and keeps the focus on the original
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03fb95bd-88e4-4f1a-9959-4b468803c407
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Sources/Panels/BrowserNavigationDelegate.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserDiscardRestoreHealPredicateTests.swiftcmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift
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 (2)
Sources/Panels/BrowserPanel.swift (1)
3801-3808: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the unused cancellation hook.
BrowserNavigationDelegate.didCancelNavigationPolicyis never called, and terminal cancellation already flows throughterminalPolicyCancellationReporter. DropdidCancelNavigationPolicyandPolicyCancellationKindso this state has one path.🤖 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 3801 - 3808, Remove the unused navigation cancellation hook by deleting BrowserNavigationDelegate.didCancelNavigationPolicy and the related PolicyCancellationKind handling from BrowserPanel and its delegate setup. Keep the terminal cancellation behavior routed through terminalPolicyCancellationReporter and noteDiscardedWebViewRestoreNavigationTerminallyCancelled, so BrowserNavigationDelegate has a single cancellation path and no dead state remains.Source: Path instructions
cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift (1)
442-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest hits a live loopback host/port instead of a local fake or ephemeral server.
mainFrameDownloadCompletesRestoreAndSuppressesBlankShellHealcreates a real (isRemoteWorkspace: false)BrowserPanelwithinitialURLset to a fixedhttp://127.0.0.1:1/...address and waits for it to settle. As per path instructions, cmuxTests must "not bind a fixed non-zero port or hit a live network host in tests; use a local fake or an ephemeral server instead."In practice this is likely reliable (port 1 is essentially never bound), but it's a direct match for the forbidden pattern and could become flaky if the sandbox/CI environment restricts loopback connections differently.
🤖 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 `@cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift` around lines 442 - 452, `mainFrameDownloadCompletesRestoreAndSuppressesBlankShellHeal` is using a fixed loopback URL with a non-ephemeral port, which violates the test networking rule. Update the test to avoid `URL(string: "http://127.0.0.1:1/...")` and instead drive `BrowserPanel` through a local fake server or an ephemeral server fixture, keeping the existing `BrowserPanel`/`waitForDiscardRestoreRetryWebViewToSettle` flow intact.Source: Path instructions
🤖 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 `@cmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift`:
- Around line 442-452:
`mainFrameDownloadCompletesRestoreAndSuppressesBlankShellHeal` is using a fixed
loopback URL with a non-ephemeral port, which violates the test networking rule.
Update the test to avoid `URL(string: "http://127.0.0.1:1/...")` and instead
drive `BrowserPanel` through a local fake server or an ephemeral server fixture,
keeping the existing `BrowserPanel`/`waitForDiscardRestoreRetryWebViewToSettle`
flow intact.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3801-3808: Remove the unused navigation cancellation hook by
deleting BrowserNavigationDelegate.didCancelNavigationPolicy and the related
PolicyCancellationKind handling from BrowserPanel and its delegate setup. Keep
the terminal cancellation behavior routed through
terminalPolicyCancellationReporter and
noteDiscardedWebViewRestoreNavigationTerminallyCancelled, so
BrowserNavigationDelegate has a single cancellation path and no dead state
remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03fb95bd-88e4-4f1a-9959-4b468803c407
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Sources/Panels/BrowserNavigationDelegate.swiftSources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserDiscardRestoreHealPredicateTests.swiftcmuxTests/BrowserDiscardedWebViewRestoreRetryTests.swift
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 `@cmuxTests/BrowserInsecureHTTPAlertTestSupport.swift`:
- Around line 20-28: `resetInsecureHTTPAlertHooksForTesting()` is duplicating
the default `insecureHTTPAlertWindowProvider` fallback chain, so update it to
reuse the same production default source instead of hardcoding
`browserInteractiveModalHostWindow` and
`browserFallbackInteractiveModalHostWindow` again. Expose the default alert-hook
closures from `BrowserPanel` as a shared internal factory/constant (or
equivalent), then have `resetInsecureHTTPAlertHooksForTesting()` restore those
defaults directly so test resets always stay in sync with the production
initializer.
🪄 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: 9ec07293-44d2-4e84-91fc-7d912e6ead9d
📒 Files selected for processing (4)
Sources/Panels/BrowserPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserDiscardRestoreHealPredicateTests.swiftcmuxTests/BrowserInsecureHTTPAlertTestSupport.swift
💤 Files with no reviewable changes (1)
- cmuxTests/BrowserDiscardRestoreHealPredicateTests.swift
| func resetInsecureHTTPAlertHooksForTesting() { | ||
| insecureHTTPAlertFactory = { NSAlert() } | ||
| insecureHTTPAlertWindowProvider = { [weak self] in | ||
| if let self, let window = browserInteractiveModalHostWindow(for: self.webView) { | ||
| return window | ||
| } | ||
| return browserFallbackInteractiveModalHostWindow() | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Minor: default-provider logic duplicated between production and test reset.
resetInsecureHTTPAlertHooksForTesting() re-implements the same window-provider fallback chain (browserInteractiveModalHostWindow → browserFallbackInteractiveModalHostWindow) that presumably already exists as insecureHTTPAlertWindowProvider's production default initializer. If that default changes in BrowserPanel.swift, this copy can silently drift, letting tests reset to a stale "default."
Consider having this function pull the default closures from a small internal factory/constant on BrowserPanel so both sites stay in sync.
🤖 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 `@cmuxTests/BrowserInsecureHTTPAlertTestSupport.swift` around lines 20 - 28,
`resetInsecureHTTPAlertHooksForTesting()` is duplicating the default
`insecureHTTPAlertWindowProvider` fallback chain, so update it to reuse the same
production default source instead of hardcoding
`browserInteractiveModalHostWindow` and
`browserFallbackInteractiveModalHostWindow` again. Expose the default alert-hook
closures from `BrowserPanel` as a shared internal factory/constant (or
equivalent), then have `resetInsecureHTTPAlertHooksForTesting()` restore those
defaults directly so test resets always stay in sync with the production
initializer.
This branch was successfully deployed
1 participant
Fixes #7504
Summary
restoreDiscardedWebViewIfNeededpath, with queued-remote dedupe, stale-cancel protection, stall recovery, and reveal-only blank-shell healing..openedExternally, and delayed policy-prompt terminal completions still honor the restore attempt after WebKit's immediate provisional cancel.Testing
git diff --check./scripts/lint-pbxproj-test-wiring.shpython3 scripts/swift_file_length_budget.py(expected existing unrelated debt remains inSources/DockSplitStore.swiftandPackages/Shared/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swift; touched files stay within budget/hard-cap limits)python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv --base-ref "$BASE_REF" --merge-ref origin/main --merge-head HEAD(local git falls back from speculative merge-tree mode, budget respected)xcodebuild -quiet -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-7504-pr-comments-unit -only-testing:cmuxTests/BrowserDiscardRestorePolicyCancelTests/cancelledInsecureHTTPPromptKeepsDiscardRestoreRetryable -only-testing:cmuxTests/BrowserDiscardRestorePolicyCancelTests/failedInsecureHTTPExternalOpenDoesNotReportTerminalRestore -only-testing:cmuxTests/BrowserDiscardRestorePolicyCancelTests/terminalPolicyCompletionAfterProvisionalCancelCompletesDiscardRestore -only-testing:cmuxTests/BrowserDiscardRestorePolicyCancelTests/staleInsecureHTTPPromptDoesNotCompleteNewerRestore -only-testing:cmuxTests/BrowserDiscardRestorePolicyCancelTests/staleRestoreCancelDoesNotClearCurrentAttemptedRequest testxcodebuild -quiet -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-7504-pr-comments-unit -only-testing:cmuxTests/BrowserDiscardedWebViewRestoreRetryGreenTests/policyCancelledRestoreClearsDiscardStateInsteadOfReplaying -only-testing:cmuxTests/BrowserDiscardedWebViewRestoreRetryGreenTests/intentBrowserFallbackPolicyCancelStaysRetryableUntilFallbackCommits -only-testing:cmuxTests/BrowserDiscardedWebViewRestoreRetryGreenTests/unknownCancellationAfterClearedAttemptedURLKeepsRestoreRetryable test./scripts/reload.sh --tag issue-7504-pr-commentsLocalization audit: no new user-facing strings were added in the latest review-fix commits. The insecure-HTTP open-failure path does not add copy; no
Resources/Localizable.xcstringschanges were required.Demo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches core WebKit navigation and memory-discard lifecycle for browser panels; behavior is heavily tested but regressions could affect tab restore, downloads, and external/insecure-HTTP flows.
Overview
Fixes black browser panes after memory discard when a restore navigation fails or never commits (#7504).
Discard manager no longer clears discard state when
restoreIfNeededruns the restore closure; it stays discarded untilnoteRestoreNavigationCommitted,reactivateWithoutNavigation, ornoteRestoreNavigationDidNotCommit. AddsisRestoreNavigationPending,forcereload to restart in-flight restores, and start/commit/did-not-commit hooks.New
BrowserDiscardRestoreHealcentralizesrestoreDiscardedWebViewIfNeeded: stall detection, sticky Stop, queued remote dedupe,WKNavigationidentity for stale cancel/fail callbacks, restore-attempt UUIDs, reveal-only blank-shell heal (skips error page, crash recovery, insecure HTTP), andabout:blankreactivation without navigation.BrowserNavigationDelegatepassesWKNavigation?on fail/cancel, addsdidCancelNavigationPolicyanddidBecomeDownloadwith restore-attempt IDs; terminal policy cancels (external open, new tab, insecure HTTP) complete restore bookkeeping correctly.BrowserPanelwires commits (non-about:blank), downloads, and policy outcomes into that path;stopLoadingsets a flag so reveal cannot auto-reload; lifecycle payload addsrestore_pending/has_committed_document. External navigation returns a typed result so failed opens are not treated as terminal.Adds focused unit/integration tests and moves insecure-HTTP test hooks to
BrowserInsecureHTTPAlertTestSupport.Reviewed by Cursor Bugbot for commit 348e3ee. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Prevents black browser panes after a failed discard‑restore by keeping discard state until a real document commit, a main‑frame download, or a terminal, tokenized policy cancel, with stall detection and a reveal‑only blank‑shell heal that honors Stop and never heals over the browser error page (fixes #7504). Download callbacks are now scoped to the active restore attempt to avoid stale completions.
WKNavigationand ignores stale callbacks; stalled restores clear tracked navigation on next touch; reveal‑only blank‑shell heal gated by user Stop, crash‑recovery, insecure‑HTTP consent, and active error page;about:blank/no‑URL restores reactivate in place; prewarmed adoption seeds the committed flag; queued remote restores treated as in‑flight until explicit reload; nil‑target tabs now complete terminal restore cleanly; addsrestore_pendingandhas_committed_documentdiagnostics; restore/heal flow moved intoBrowserDiscardRestoreHeal.WKNavigation?; newdidCancelNavigationPolicyreports typed, tokenized terminal cancels keyed to the restore attempt;didBecomeDownloadreports main‑frame flag and restore‑attempt ID so download outcomes complete the correct attempt; preserves restore tokens across policy prompts;didBecomeDownloadtreats main‑frame downloads as committed outcomes; external navigation returns a typed result so browser‑fallback is not marked terminal; insecure‑HTTP prompts remain retryable and then complete or continue restores based on user choice.Written for commit 348e3ee. Summary will update on new commits.
Summary by CodeRabbit
about:blankrestore/heal behavior and updated blank/background rendering decisions to avoid unintended healing.