Repository navigation
Fix browser panel lifecycle after WebContent process termination - #892
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes introduce web view lifecycle management to handle content process terminations. A new monotonic identifier tracks web view replacements, and a factory/binder pattern replaces direct initialization. The navigation delegate notifies about process terminations, triggering state preservation and web view replacement. SwiftUI view wiring is simplified by removing retry logic and consolidating portal updates. Tests verify the new behavior and enforce architectural constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant Panel as BrowserPanel
participant NavDelegate as BrowserNavigationDelegate
participant Portal as BrowserWindowPortalRegistry
participant WebView as CmuxWebView
NavDelegate->>NavDelegate: Detect content process termination
NavDelegate->>Panel: didTerminateWebContentProcess callback
Panel->>Panel: replaceWebViewAfterContentProcessTermination()
Panel->>Panel: Preserve state (URL, zoom, DevTools, history)
Panel->>Panel: Create fresh webView via makeWebView()
Panel->>Panel: bindWebView() to attach delegates
Panel->>Portal: Update portal with new webView
Panel->>WebView: Restore preserved state
Panel->>Panel: Increment webViewInstanceID
Note over Panel,WebView: SwiftUI detects instanceID change, remounts view
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a non-functional browser panel after a WebContent process crash by replacing the stale Key changes:
Issue found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant WK as WebKit
participant NavDelegate as BrowserNavigationDelegate
participant Panel as BrowserPanel
participant Portal as BrowserWindowPortalRegistry
participant SwiftUI as BrowserPanelView / SwiftUI
WK->>NavDelegate: webView(_:webContentProcessDidTerminate:)
NavDelegate->>Panel: didTerminateWebContentProcess(webView)
Panel->>Panel: replaceWebViewAfterContentProcessTermination(for: terminatedWebView)
Panel->>Panel: webViewObservers.removeAll()
Panel->>Portal: detach(webView: terminatedWebView)
Panel->>Panel: terminatedWebView delegates = nil
Panel->>Panel: let replacement = makeWebView()
Panel->>Panel: self.webView = replacement
Panel->>Panel: webViewInstanceID = UUID() [triggers @Published]
Panel->>Panel: bindWebView(replacement)
Panel->>Panel: restoreSessionHistory / navigateToRestoreURL
Panel-->>SwiftUI: objectWillChange (webViewInstanceID changed)
SwiftUI->>SwiftUI: .id(panel.webViewInstanceID) changed → remount WebViewRepresentable
SwiftUI->>SwiftUI: dismantleNSView(old coordinator)
SwiftUI->>Portal: detach(webView: terminatedWebView) [idempotent]
SwiftUI->>SwiftUI: makeNSView + makeCoordinator
SwiftUI->>SwiftUI: updateNSView → updateUsingWindowPortal
SwiftUI->>Portal: bind(webView: replacement, to: host, ...)
Last reviewed commit: 1ee8283 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self.insecureHTTPAlertFactory = { NSAlert() } | ||
| self.insecureHTTPAlertWindowProvider = { [weak webView] in | ||
| webView?.window ?? NSApp.keyWindow ?? NSApp.mainWindow | ||
| } | ||
| self.insecureHTTPAlertWindowProvider = { NSApp.keyWindow ?? NSApp.mainWindow } |
There was a problem hiding this comment.
Dead assignment to insecureHTTPAlertWindowProvider
This initial assignment to { NSApp.keyWindow ?? NSApp.mainWindow } on line 1548 is immediately overwritten on line 1611–1613 with the closure that captures [weak self] and reads self?.webView.window. The first assignment is never observable by any caller and can be removed to avoid confusion.
| self.insecureHTTPAlertFactory = { NSAlert() } | |
| self.insecureHTTPAlertWindowProvider = { [weak webView] in | |
| webView?.window ?? NSApp.keyWindow ?? NSApp.mainWindow | |
| } | |
| self.insecureHTTPAlertWindowProvider = { NSApp.keyWindow ?? NSApp.mainWindow } | |
| self.insecureHTTPAlertFactory = { NSAlert() } |
Additional Comments (1)
After N crashes there will be N+1 subscriptions all firing for the same notification, each executing The fix is to store this subscription alongside the KVO observers so it is torn down consistently:
|
![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)
1691-1755:⚠️ Potential issue | 🟠 MajorObserver subscriptions can accumulate after repeated WebView replacement.
setupObservers(for:)adds a NotificationCenter sink intocancellables, and replacement rebinding callssetupObservers(for:)again without clearing/replacing that sink. Over multiple process terminations, this grows listeners indefinitely.♻️ Suggested fix (single replaceable cancellable)
- private var cancellables = Set<AnyCancellable>() + private var cancellables = Set<AnyCancellable>() + private var ghosttyBackgroundChangeCancellable: AnyCancellable? @@ private func setupObservers(for webView: WKWebView) { @@ - NotificationCenter.default.publisher(for: .ghosttyDefaultBackgroundDidChange) - .sink { [weak self] notification in - guard let self else { return } - self.webView.underPageBackgroundColor = GhosttyBackgroundTheme.color(from: notification) - } - .store(in: &cancellables) + ghosttyBackgroundChangeCancellable?.cancel() + ghosttyBackgroundChangeCancellable = NotificationCenter.default + .publisher(for: .ghosttyDefaultBackgroundDidChange) + .sink { [weak self] notification in + guard let self else { return } + self.webView.underPageBackgroundColor = GhosttyBackgroundTheme.color(from: notification) + } }Also applies to: 1757-1794
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanel.swift` around lines 1691 - 1755, setupObservers(for:) currently appends a NotificationCenter sink into the shared cancellables collection each time a WebView is rebound, causing listeners to accumulate; change this to use a single replaceable cancellable (e.g., add a property like backgroundThemeCancellable: AnyCancellable?) and before creating the new NotificationCenter.default.publisher sink cancel/replace the existing backgroundThemeCancellable, then assign the new sink to that property instead of storing it into &cancellables; update references in setupObservers(for:) and the analogous block at 1757-1794 so the sink is not appended repeatedly to cancellables.
🧹 Nitpick comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
2368-2419: Consider extracting duplicated window/anchor portal setup into a helper.Line 2368–2383 and Line 2404–2419 repeat the same setup. A helper would reduce drift and simplify future portal lifecycle test updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 2368 - 2419, The duplicated test setup that creates NSWindow, anchor NSView, calls window.makeKeyAndOrderFront/displayIfNeeded/layoutSubtreeIfNeeded, runs the RunLoop, then calls BrowserWindowPortalRegistry.bind(...) and BrowserWindowPortalRegistry.synchronizeForAnchor(...) should be extracted into a single helper (e.g., setupPortalAnchorAndBind(webView: NSView, anchorFrame: NSRect, zPriority: Int) or makeWindowWithAnchorAndBind(panel:webView:anchorFrame:zPriority:)), and both testWebViewDismantleDetachesPortalHostedWebView and testWebViewDismantleDetachesPortalHostedWebViewWhenDeveloperToolsIntentIsHidden should call that helper instead of repeating the block; ensure the helper returns the created window and anchor so tests can continue asserting panel.webView.superview and later call window.orderOut(nil).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3173-3176: The delegate method
webView(_:webContentProcessDidTerminate:) is using the wrong signature and never
gets called; change the method to the correct WebKit delegate signature func
webViewWebContentProcessDidTerminate(_ webView: WKWebView) and keep the existing
call to didTerminateWebContentProcess?(webView). Also replace the NSLog call
with a dlog call wrapped inside `#if` DEBUG / `#endif` per coding guidelines so the
debug message is only emitted in debug builds.
In `@tests/test_browser_portal_lifecycle_architecture.py`:
- Around line 17-25: The repo_root() function calls subprocess.run with the bare
"git" executable which triggers Ruff S607; modify repo_root to resolve the full
git executable path first using shutil.which("git") (import shutil), then call
subprocess.run with that returned path (git_path) instead of the literal "git";
if shutil.which returns None, either raise a clear error or fall back to the
existing behavior (e.g., attempt "git" or return the __file__ parent), ensuring
the function references repo_root and the subprocess.run call are updated
accordingly.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 1691-1755: setupObservers(for:) currently appends a
NotificationCenter sink into the shared cancellables collection each time a
WebView is rebound, causing listeners to accumulate; change this to use a single
replaceable cancellable (e.g., add a property like backgroundThemeCancellable:
AnyCancellable?) and before creating the new
NotificationCenter.default.publisher sink cancel/replace the existing
backgroundThemeCancellable, then assign the new sink to that property instead of
storing it into &cancellables; update references in setupObservers(for:) and the
analogous block at 1757-1794 so the sink is not appended repeatedly to
cancellables.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 2368-2419: The duplicated test setup that creates NSWindow, anchor
NSView, calls window.makeKeyAndOrderFront/displayIfNeeded/layoutSubtreeIfNeeded,
runs the RunLoop, then calls BrowserWindowPortalRegistry.bind(...) and
BrowserWindowPortalRegistry.synchronizeForAnchor(...) should be extracted into a
single helper (e.g., setupPortalAnchorAndBind(webView: NSView, anchorFrame:
NSRect, zPriority: Int) or
makeWindowWithAnchorAndBind(panel:webView:anchorFrame:zPriority:)), and both
testWebViewDismantleDetachesPortalHostedWebView and
testWebViewDismantleDetachesPortalHostedWebViewWhenDeveloperToolsIntentIsHidden
should call that helper instead of repeating the block; ensure the helper
returns the created window and anchor so tests can continue asserting
panel.webView.superview and later call window.orderOut(nil).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39382228-b015-4e83-85ac-8f05d18a2ed5
📒 Files selected for processing (4)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swifttests/test_browser_portal_lifecycle_architecture.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ee82839be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| webViewInstanceID = UUID() | ||
| shouldRenderWebView = wasRenderable | ||
|
|
||
| bindWebView(replacement) |
There was a problem hiding this comment.
When WebContent terminates, replaceWebViewAfterContentProcessTermination rebinds via bindWebView(replacement), which calls setupObservers(for:) and adds another .ghosttyDefaultBackgroundDidChange subscription to cancellables. This path clears webViewObservers but never clears/reuses cancellables, so repeated process crashes accumulate stale subscriptions and make each background-theme notification run multiple times, causing unbounded memory/work growth over time.
Useful? React with 👍 / 👎.
|
Addressed review feedback in follow-up commits:
Validation rerun:
Resolve Package Graph Resolved source packages: ComputePackagePrebuildTargetDependencyGraph Prepare packages CreateBuildRequest SendProjectDescription CreateBuildOperation ComputeTargetDependencyGraph GatherProvisioningInputs CreateBuildDescription ExecuteExternalTool /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/swiftc --version ExecuteExternalTool /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -v -E -dM -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX26.2.sdk -x c -c /dev/null ExecuteExternalTool /Applications/Xcode.app/Contents/Developer/usr/bin/actool --version --output-format xml1 ExecuteExternalTool /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -v -E -dM -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX26.2.sdk -x c -c /dev/null ExecuteExternalTool /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ld -version_details Build description signature: d4582144c20a085744e6e99b6b8eae39 ProcessProductPackaging "" /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Intermediates.noindex/GhosttyTabs.build/Debug/cmux-cli.build/cmux.xcent (in target 'cmux-cli' from project 'GhosttyTabs') } ProcessProductPackagingDER /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Intermediates.noindex/GhosttyTabs.build/Debug/cmux-cli.build/cmux.xcent /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Intermediates.noindex/GhosttyTabs.build/Debug/cmux-cli.build/cmux.xcent.der (in target 'cmux-cli' from project 'GhosttyTabs') warning: Run script build phase 'Run Script' will be run during every build because it does not specify any outputs. To address this issue, either add output dependencies to the script phase, or configure it to run in every build by unchecking "Based on dependency analysis" in the script phase. (in target 'GhosttyTabs' from project 'GhosttyTabs') } ProcessProductPackagingDER /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Intermediates.noindex/GhosttyTabs.build/Debug/GhosttyTabs.build/cmux\ DEV.app.xcent /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Intermediates.noindex/GhosttyTabs.build/Debug/GhosttyTabs.build/cmux\ DEV.app.xcent.der (in target 'GhosttyTabs' from project 'GhosttyTabs') ProcessInfoPlistFile /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/PostHog_PostHog.bundle/Contents/Info.plist /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Intermediates.noindex/PostHog.build/Debug/PostHog_PostHog.build/empty-PostHog_PostHog.plist (in target 'PostHog_PostHog' from project 'PostHog') CodeSign /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux (in target 'cmux-cli' from project 'GhosttyTabs') /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux: replacing existing signature PhaseScriptExecution Run\ Script /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Intermediates.noindex/GhosttyTabs.build/Debug/GhosttyTabs.build/Script-A5001300A1B2C3D4E5F60719.sh (in target 'GhosttyTabs' from project 'GhosttyTabs') CopySwiftLibs /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app (in target 'GhosttyTabs' from project 'GhosttyTabs') ProcessInfoPlistFile /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app/Contents/Info.plist /Users/lawrencechen/fun/cmuxterm-hq/worktrees/task-nightly-browser-not-working/Resources/Info.plist (in target 'GhosttyTabs' from project 'GhosttyTabs') ProcessInfoPlistFile /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app/Contents/PlugIns/cmuxTests.xctest/Contents/Info.plist /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Intermediates.noindex/GhosttyTabs.build/Debug/cmuxTests.build/empty-cmuxTests.plist (in target 'cmuxTests' from project 'GhosttyTabs') CopySwiftLibs /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app/Contents/PlugIns/cmuxTests.xctest (in target 'cmuxTests' from project 'GhosttyTabs') CodeSign /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app/Contents/MacOS/cmux\ DEV.debug.dylib (in target 'GhosttyTabs' from project 'GhosttyTabs') /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux DEV.app/Contents/MacOS/cmux DEV.debug.dylib: replacing existing signature CodeSign /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app/Contents/MacOS/__preview.dylib (in target 'GhosttyTabs' from project 'GhosttyTabs') /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux DEV.app/Contents/MacOS/__preview.dylib: replacing existing signature CodeSign /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app (in target 'GhosttyTabs' from project 'GhosttyTabs') /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux DEV.app: replacing existing signature Validate /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app (in target 'GhosttyTabs' from project 'GhosttyTabs') RegisterWithLaunchServices /Users/lawrencechen/Library/Developer/Xcode/DerivedData/GhosttyTabs-cvrmjsvnlttdlaazhosofqqadjdd/Build/Products/Debug/cmux\ DEV.app (in target 'GhosttyTabs' from project 'GhosttyTabs') 2026-03-04 16:09:02.807228-0800 cmux DEV[15542:16436313] [SwiftUI] Accessing StateObject's object without being installed on a View. This will create a new instance each time. [Sentry] [debug] [1772669344.081593] [SentryFileIOTrackerHelper:196] No transaction bound to scope. Won't track file IO operation. Test session results, code coverage, and logs: ** TEST SUCCEEDED ** Testing started |
|
Addressed review feedback in follow-up commits:
Validation rerun:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_browser_portal_lifecycle_architecture.py">
<violation number="1" location="tests/test_browser_portal_lifecycle_architecture.py:17">
P3: This test duplicates shared helper logic (`repo_root` and `extract_block`) that already exists in another regression test, which increases maintenance burden and drift risk.</violation>
</file>
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:1691">
P2: Move the `NotificationCenter` Combine subscription out of `setupObservers` and into `init()`. Calling `setupObservers` on every WebContent process crash causes duplicated Combine subscriptions to leak into `cancellables`.</violation>
<violation number="2" location="Sources/Panels/BrowserPanel.swift:3174">
P0: The delegate method has an incorrect signature and will never be called by WebKit. Change the method signature to `func webViewWebContentProcessDidTerminate(_ webView: WKWebView)`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| from pathlib import Path | ||
|
|
||
|
|
||
| def repo_root() -> Path: |
There was a problem hiding this comment.
P3: This test duplicates shared helper logic (repo_root and extract_block) that already exists in another regression test, which increases maintenance burden and drift risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_browser_portal_lifecycle_architecture.py, line 17:
<comment>This test duplicates shared helper logic (`repo_root` and `extract_block`) that already exists in another regression test, which increases maintenance burden and drift risk.</comment>
<file context>
@@ -0,0 +1,91 @@
+from pathlib import Path
+
+
+def repo_root() -> Path:
+ result = subprocess.run(
+ ["git", "rev-parse", "--show-toplevel"],
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23419583fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
|
||
| webViewObservers.removeAll() | ||
| webViewCancellables.removeAll() | ||
| BrowserWindowPortalRegistry.detach(webView: terminatedWebView) |
There was a problem hiding this comment.
In replaceWebViewAfterContentProcessTermination, the old terminatedWebView is detached immediately, but this recovery path never clears first responder first. If the crash happens while the browser pane is focused (common during typing/clicking), detaching a responder-chain WKWebView can leave a stale responder or trigger the same WebKit reparenting crash that other teardown paths already guard against in WebViewRepresentable.dismantleNSView via window.makeFirstResponder(nil). Please resign first responder for the terminating view/window before BrowserWindowPortalRegistry.detach(...).
Useful? React with 👍 / 👎.
|
Follow-up on cubic/coderabbit feedback:
I left the test-helper deduplication suggestion as-is for now since it is non-blocking and would be a broader cleanup beyond this bugfix scope. |
2341958 to
8e01acf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e01acfa6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| webViewInstanceID = UUID() | ||
| shouldRenderWebView = wasRenderable | ||
|
|
||
| bindWebView(replacement) |
There was a problem hiding this comment.
The crash-recovery path builds a new WKWebView and binds delegates/observers, but it never reapplies the user-selected browser theme mode. makeWebView() leaves appearance at system defaults, so after a WebContent termination a forced light/dark setting is lost until a later successful navigation triggers didFinish (and in some failure flows it may stay wrong longer). Apply applyBrowserThemeModeIfNeeded() immediately after rebinding the replacement view to preserve theme consistency through recovery.
Useful? React with 👍 / 👎.
…aflow-ai#892) * Fix browser panel webview lifecycle after web content crashes * Fix BrowserPanel observer lifecycle during webview replacement * Fix WebKit termination delegate and harden lifecycle regression check
This branch was successfully deployed
1 participant
Summary
Root cause
WebViewRepresentable had two independent mount paths (portal and direct attach/retry). After WebContent termination, the existing instance could be left in a stale attachment state and the retry branch introduced nondeterministic behavior.
How to reproduce (before fix)
Validation
Summary by cubic
Fixes browser panel recovery after WebContent crashes by replacing WKWebView and routing all mounts through the window portal. Preserves history, URL, zoom, DevTools, and forces clean SwiftUI remounts via a per-instance ID.
Bug Fixes
Refactors
Written for commit 8e01acf. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests