Fix split zoom visibility sync to prevent black browser pane - #929
lawrencecchen wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughSynchronizes terminal portal visibility with workspace zoom/unzoom, exposes small debug visibility accessors for terminal views, and tightens browser webview portal detach semantics to validate expected anchor identity and return success indicators. Tests and panel close/dismantle paths adjusted to match new lifecycle behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Workspace
participant LayoutEngine
participant VisibilityCompute
participant TerminalPortalRegistry
participant TerminalPanel
User->>Workspace: toggleSplitZoom(panelId)
activate Workspace
Workspace->>LayoutEngine: apply zoom/unzoom
LayoutEngine-->>Workspace: layout updated
Workspace->>VisibilityCompute: synchronizeTerminalPortalVisibilityForCurrentLayout()
activate VisibilityCompute
VisibilityCompute->>VisibilityCompute: visiblePanelIdsForCurrentLayout()
VisibilityCompute->>TerminalPortalRegistry: set visibility for panels (visible/hidden)
activate TerminalPortalRegistry
TerminalPortalRegistry->>TerminalPanel: apply visibility change
TerminalPanel-->>TerminalPortalRegistry: ack
TerminalPortalRegistry-->>VisibilityCompute: done
VisibilityCompute-->>Workspace: sync complete
deactivate VisibilityCompute
Workspace-->>User: zoom toggled (visibility updated)
deactivate Workspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a visibility-state drift issue in split zoom by introducing Confidence Score: 4/5
Last reviewed commit: ddd996a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func debugIsVisibleInUI() -> Bool { | ||
| visibleInUI | ||
| } |
There was a problem hiding this comment.
debugIsVisibleInUI() bypasses fileprivate access control by exposing visibleInUI at default internal visibility. Since this method only supports unit-test assertions, it should be absent from production builds.
Apply #if DEBUG guards to debugIsVisibleInUI() here and on GhosttySurfaceScrollView (line 5706), and to debugVisiblePanelIdsForCurrentLayout() in Workspace.swift (line 3253):
| func debugIsVisibleInUI() -> Bool { | |
| visibleInUI | |
| } | |
| #if DEBUG | |
| func debugIsVisibleInUI() -> Bool { | |
| visibleInUI | |
| } | |
| #endif |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/BrowserWindowPortal.swift (1)
1245-1250: AlignhasPortalEntrywith the stale-mapping fallback used byisWebView(boundTo:).
hasPortalEntry(for:)currently returns false whenwebViewToWindowIdis stale, even if another portal still has the entry. Matching the fallback behavior avoids false negatives during churn.Suggested refactor
static func hasPortalEntry(for webView: WKWebView) -> Bool { let webViewId = ObjectIdentifier(webView) - guard let windowId = webViewToWindowId[webViewId], - let portal = portalsByWindowId[windowId] else { return false } - return portal.hasWebViewEntry(withId: webViewId) + if let windowId = webViewToWindowId[webViewId], + let portal = portalsByWindowId[windowId], + portal.hasWebViewEntry(withId: webViewId) { + return true + } + for portal in portalsByWindowId.values { + if portal.hasWebViewEntry(withId: webViewId) { + return true + } + } + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 1245 - 1250, hasPortalEntry(for:) currently relies solely on webViewToWindowId and returns false if that mapping is stale; change it to mirror isWebView(boundTo:) by falling back to scanning all portals when the webViewToWindowId lookup fails. Specifically, in static func hasPortalEntry(for webView: WKWebView) compute let webViewId = ObjectIdentifier(webView), attempt the existing webViewToWindowId then portalsByWindowId lookup, and if that returns nil iterate over portalsByWindowId.values and return true if any portal.hasWebViewEntry(withId: webViewId) is true; otherwise return false. Ensure you reference webViewToWindowId, portalsByWindowId, portal.hasWebViewEntry(withId:), and hasPortalEntry(for:) in your change.
🤖 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/BrowserPanelView.swift`:
- Around line 3228-3249: The guard currently only rebinds when both
!isBoundToHost and !hasPortalEntry, which skips the case where a portal entry
exists but is bound to the wrong host; change the logic to rebind whenever
!isBoundToHost OR the existing portal entry is associated with a different host
(i.e., detect entryHost mismatch), then call
BrowserWindowPortalRegistry.bind(webView:to:visibleInUI:zPriority:) and update
coordinator.lastPortalHostId accordingly (and if necessary unbind or overwrite
the stale entry before binding). Ensure you reference isBoundToHost,
hasPortalEntry, BrowserWindowPortalRegistry.bind, coordinator.lastPortalHostId,
coordinator.desiredPortalVisibleInUI, and coordinator.desiredPortalZPriority
when implementing the fix.
---
Nitpick comments:
In `@Sources/BrowserWindowPortal.swift`:
- Around line 1245-1250: hasPortalEntry(for:) currently relies solely on
webViewToWindowId and returns false if that mapping is stale; change it to
mirror isWebView(boundTo:) by falling back to scanning all portals when the
webViewToWindowId lookup fails. Specifically, in static func hasPortalEntry(for
webView: WKWebView) compute let webViewId = ObjectIdentifier(webView), attempt
the existing webViewToWindowId then portalsByWindowId lookup, and if that
returns nil iterate over portalsByWindowId.values and return true if any
portal.hasWebViewEntry(withId: webViewId) is true; otherwise return false.
Ensure you reference webViewToWindowId, portalsByWindowId,
portal.hasWebViewEntry(withId:), and hasPortalEntry(for:) in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfa4c26e-14ab-484f-87af-84124943760f
📒 Files selected for processing (3)
Sources/BrowserWindowPortal.swiftSources/Panels/BrowserPanelView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/CmuxWebViewKeyEquivalentTests.swift
| if host.window != nil, | ||
| !isBoundToHost, | ||
| !hasPortalEntry { | ||
| #if DEBUG | ||
| if let panel = coordinator.panel { | ||
| Self.logDevToolsState( | ||
| panel, | ||
| event: "portal.rebindOnGeometry", | ||
| generation: coordinator.attachGeneration, | ||
| retryCount: 0, | ||
| details: Self.attachContext(webView: webView, host: host) + " reason=portalEntryMissing" | ||
| ) | ||
| } | ||
| #endif | ||
| BrowserWindowPortalRegistry.bind( | ||
| webView: webView, | ||
| to: host, | ||
| visibleInUI: coordinator.desiredPortalVisibleInUI, | ||
| zPriority: coordinator.desiredPortalZPriority | ||
| ) | ||
| coordinator.lastPortalHostId = ObjectIdentifier(host) | ||
| } |
There was a problem hiding this comment.
Rebind guard misses the “entry exists but bound to wrong host” case.
At Line 3228, rebind only happens when !isBoundToHost && !hasPortalEntry. If a stale entry exists on a different anchor, hasPortalEntry is true and this path skips rebind, which can leave this host unsynchronized.
Suggested fix
- if host.window != nil,
- !isBoundToHost,
- !hasPortalEntry {
+ if host.window != nil,
+ !isBoundToHost {
`#if` DEBUG
if let panel = coordinator.panel {
+ let rebindReason = hasPortalEntry ? "wrongAnchor" : "portalEntryMissing"
Self.logDevToolsState(
panel,
event: "portal.rebindOnGeometry",
generation: coordinator.attachGeneration,
retryCount: 0,
- details: Self.attachContext(webView: webView, host: host) + " reason=portalEntryMissing"
+ details: Self.attachContext(webView: webView, host: host) + " reason=\(rebindReason)"
)
}
`#endif`
BrowserWindowPortalRegistry.bind(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanelView.swift` around lines 3228 - 3249, The guard
currently only rebinds when both !isBoundToHost and !hasPortalEntry, which skips
the case where a portal entry exists but is bound to the wrong host; change the
logic to rebind whenever !isBoundToHost OR the existing portal entry is
associated with a different host (i.e., detect entryHost mismatch), then call
BrowserWindowPortalRegistry.bind(webView:to:visibleInUI:zPriority:) and update
coordinator.lastPortalHostId accordingly (and if necessary unbind or overwrite
the stale entry before binding). Ensure you reference isBoundToHost,
hasPortalEntry, BrowserWindowPortalRegistry.bind, coordinator.lastPortalHostId,
coordinator.desiredPortalVisibleInUI, and coordinator.desiredPortalZPriority
when implementing the fix.
![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: a14ee31ee3
ℹ️ 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".
| let didDetach = portal.detachWebView(withId: webViewId, expectedAnchorId: expectedAnchorId) | ||
| guard didDetach else { return false } | ||
| webViewToWindowId.removeValue(forKey: webViewId) |
There was a problem hiding this comment.
BrowserWindowPortalRegistry.detach now returns early when portal.detachWebView is false, but that can happen after WindowBrowserPortal.pruneDeadEntries has already removed the portal entry (it detaches internally without updating webViewToWindowId). In that case the mapping is never cleared, so closed/replaced web views leave stale webViewToWindowId entries behind, and later registry lookups (hasPortalEntry, repeated detach) keep targeting a non-existent entry instead of cleaning up.
Useful? React with 👍 / 👎.
…t-enter-black-view # Conflicts: # Sources/BrowserWindowPortal.swift # Sources/Panels/BrowserPanelView.swift # cmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_browser_transient_dismantle_lifecycle_regression.py`:
- Around line 16-24: The repo_root() function calls "git" via PATH lookup;
replace that with a resolved absolute git executable using shutil.which("git")
(call once at function start or module-scope) and pass that absolute path to
subprocess.run([...]) instead of the bare "git"; if shutil.which returns None,
fall back to the current behavior (i.e., run with "git" or return the parent
path as the existing code does) so tests still work when git isn't on PATH;
update references in repo_root() only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 844d0415-da35-4b56-9844-6e20534d942c
📒 Files selected for processing (6)
Sources/BrowserWindowPortal.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swifttests/test_browser_transient_dismantle_lifecycle_regression.py
| def repo_root() -> Path: | ||
| result = subprocess.run( | ||
| ["git", "rev-parse", "--show-toplevel"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if result.returncode == 0: | ||
| return Path(result.stdout.strip()) | ||
| return Path(__file__).resolve().parents[1] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "test_browser_transient_dismantle_lifecycle_regression.py" -type fRepository: manaflow-ai/cmux
Length of output: 115
🏁 Script executed:
head -30 tests/test_browser_transient_dismantle_lifecycle_regression.pyRepository: manaflow-ai/cmux
Length of output: 1009
🏁 Script executed:
# Check if there are any other calls to subprocess with git in the codebase
rg "subprocess.*git|git.*subprocess" --type py -A 2 -B 2Repository: manaflow-ai/cmux
Length of output: 368
🏁 Script executed:
# Look for any existing use of shutil.which in the codebase
rg "shutil.which|shutil\.which" --type pyRepository: manaflow-ai/cmux
Length of output: 571
Use an absolute git path in repo_root() to avoid PATH-hijack risk in runners.
Line 18 executes git via PATH lookup. This pattern is already established elsewhere in the codebase (see tests/test_browser_portal_lifecycle_architecture.py and tests/regression_helpers.py). Resolve once with shutil.which("git") and pass the absolute executable path to subprocess.run(). Handle the case where git is not found in PATH by falling back to the current logic.
🔧 Proposed fix
+import shutil
import subprocess
from pathlib import Path
@@
def repo_root() -> Path:
+ git = shutil.which("git")
+ if git is None:
+ return Path(__file__).resolve().parents[1]
result = subprocess.run(
- ["git", "rev-parse", "--show-toplevel"],
+ [git, "rev-parse", "--show-toplevel"],
capture_output=True,
text=True,
)🧰 Tools
🪛 Ruff (0.15.2)
[error] 18-18: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_browser_transient_dismantle_lifecycle_regression.py` around lines
16 - 24, The repo_root() function calls "git" via PATH lookup; replace that with
a resolved absolute git executable using shutil.which("git") (call once at
function start or module-scope) and pass that absolute path to
subprocess.run([...]) instead of the bare "git"; if shutil.which returns None,
fall back to the current behavior (i.e., run with "git" or return the parent
path as the existing code does) so tests still work when git isn't on PATH;
update references in repo_root() only.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
After toggling split zoom (Cmd+Shift+Enter), terminal portal visibility could drift from the active layout during selection churn, leaving the browser pane effectively covered and appearing black.
This change makes zoom and unzoom transitions reconcile terminal visibility from layout state instead of relying on incidental callback ordering.
What changed
Workspace.TerminalWindowPortalRegistryvisibility in lockstep withsetVisibleInUI.Verification
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' buildxcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -only-testing:cmuxTests/WorkspacePanelGitBranchTests/testToggleSplitZoomCollapsesVisiblePanelSnapshotToZoomedPaneAndRestoresOnUnzoom testcodex review --base origin/main(clean)Summary by cubic
Fixes split zoom (Cmd+Shift+Enter) so terminal visibility and browser portals stay aligned with the layout, preventing the browser pane from appearing black. Hardens the portal lifecycle to survive SwiftUI host churn, stale dismantle calls, and transient recovery windows.
Bug Fixes
Tests
Written for commit 98d1948. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests