Repository navigation
Fix browser pane portal anchor sizing - #1094
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-pane portal-host ownership and gating across BrowserPanel/BrowserPanelView, threads a new Changes
Sequence Diagram(s)sequenceDiagram
participant Panel as BrowserPanel
participant View as BrowserPanelView
participant Host as PortalHost(NSView)
participant Registry as BrowserWindowPortalRegistry
Panel->>View: notify host/geometry/window update
View->>Panel: claimPortalHostFromUpdate(host)
Panel->>Panel: isCurrentPortalHost(host)?
alt host is current
Panel->>View: request installPortalAnchorView(host)
View->>Host: detach old anchor (if any) / reparent anchor
View->>Host: set translatesAutoresizingMaskIntoConstraints = false
View->>Host: create & add edge constraints (with identifiers)
View->>Host: activate constraints / layout subtree
View->>Registry: sync portal state & webViewSurfaceIdentity
Registry->>Host: update overlay/pane chrome height / visibility
else not current
View->>Registry: update webview visibility / gating only
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 issue #1093 where the browser pane's Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[installPortalAnchorView called] --> B{host.window != nil?}
B -- No --> Z[return — guard host off-window]
B -- Yes --> C{anchorView.superview !== host AND oldHost exists?}
C -- Yes --> D[Deactivate old edge constraints on oldHost]
D --> E[anchorView.removeFromSuperview]
E --> F{anchorView.superview !== host?}
C -- No --> F
F -- Yes --> G[anchorView.frame = host.bounds\nhost.addSubview anchorView]
F -- No --> H
G --> H[translatesAutoresizingMaskIntoConstraints = false]
H --> I[portalAnchorConstraints for anchorView in host]
I --> J{Constraints count or identifiers mismatch?}
J -- Yes --> K[Deactivate existing constraints\nCreate & activate top/bottom/leading/trailing]
J -- No --> L
K --> L[host.layoutSubtreeIfNeeded]
L --> M[Portal sync proceeds against\nfully-resolved anchor frame]
Last reviewed commit: 86fdfae |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| if anchorView.superview !== host { | ||
| anchorView.frame = host.bounds |
There was a problem hiding this comment.
Redundant frame assignment
anchorView.frame = host.bounds is set here, but translatesAutoresizingMaskIntoConstraints is immediately set to false on line 4300 and explicit constraints are activated shortly after. Auto Layout completely ignores the frame when TAMIC is false, so this line has no effect on the final layout. It appears to be a leftover from the old autoresizing-mask approach and could be removed to avoid confusion.
| anchorView.frame = host.bounds | |
| if anchorView.superview !== host { | |
| host.addSubview(anchorView) | |
| } |
| NSLayoutConstraint.activate([top, bottom, leading, trailing]) | ||
| } | ||
|
|
||
| host.layoutSubtreeIfNeeded() |
There was a problem hiding this comment.
Unconditional layout pass on every call
host.layoutSubtreeIfNeeded() is called at the end of every invocation of installPortalAnchorView, including the common steady-state path where the anchor is already in the correct host with all four constraints already active (the if block above is skipped). This triggers a synchronous full-subtree layout pass even when nothing has changed.
installPortalAnchorView is called from at least three sites — including onGeometryChanged, which fires on every resize event. On a host with many subviews, or during animated resizing, this could pile up unnecessary synchronous layout work.
Consider guarding the call so the layout pass only happens when a structural change actually occurred (new subview added, or constraints re-created):
var layoutNeeded = false
if anchorView.superview !== host {
anchorView.frame = host.bounds
host.addSubview(anchorView)
layoutNeeded = true
}
anchorView.translatesAutoresizingMaskIntoConstraints = false
let existingConstraints = portalAnchorConstraints(for: anchorView, in: host)
let existingConstraintIdentifiers = Set(existingConstraints.compactMap(\.identifier))
if existingConstraints.count != portalAnchorConstraintIdentifiers.count ||
existingConstraintIdentifiers != portalAnchorConstraintIdentifiers {
// ... activate constraints ...
layoutNeeded = true
}
if layoutNeeded {
host.layoutSubtreeIfNeeded()
}![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
🤖 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 397-399: When shouldDisplayPaneInstance flips to false we must
clear any shared omnibar focus suppression left by this instance so the
canonical pane can restore WKWebView focus; detect the ownership transition
(shouldDisplayPaneInstance changing from true to false) and if addressBarFocused
is true call the counterpart cleanup
(panel.endSuppressWebViewFocusForAddressBar()) and update addressBarFocused
state as needed; integrate this cleanup alongside the existing visibility
changes and the shouldProcessViewEvents guards (also apply same fix in the other
affected block around the 501-537 region) so suppression is always unwound when
an instance stops being the canonical pane.
- Around line 4309-4324: The helper deactivatePortalHostIfNeeded currently calls
BrowserWindowPortalRegistry.updateEntryVisibility(for: webView, visibleInUI:
false, zPriority: 0) unconditionally, which hides the shared WKWebView even if
the web view has already been reattached to a new host; change the logic so that
you only call BrowserWindowPortalRegistry.updateEntryVisibility(...) and
BrowserWindowPortalRegistry.hide(webView: source:) when the panel's
portalAnchorView.superview === host (i.e., the host being deactivated still owns
the anchor). In other words, move the visibility update and hide calls so they
run inside the existing guard that checks panel.portalAnchorView.superview ===
host in deactivatePortalHostIfNeeded (and apply the same change in the other
occurrence around lines 4697-4704).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42968e5e-cef0-4ad9-9181-2129aad7b3ff
📒 Files selected for processing (1)
Sources/Panels/BrowserPanelView.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:7016">
P2: Releasing the portal host when `host.window == nil` makes the deferred visibility-update branch effectively unreachable, so portal entry visibility is not refreshed during deferred bind transitions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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="Sources/Panels/BrowserPanelView.swift">
<violation number="1" location="Sources/Panels/BrowserPanelView.swift:867">
P2: Remove the duplicate accessibility identifier `browserContentAccessibilityIdentifier` from `WebViewRepresentable` to avoid XCUITest "Multiple matches found" errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .accessibilityIdentifier("BrowserWebViewSurface") | ||
| // Keep the host stable for normal pane churn, but force a remount when | ||
| // BrowserPanel replaces its underlying WKWebView after process termination. | ||
| .id(webViewSurfaceIdentity) |
There was a problem hiding this comment.
P2: Remove the duplicate accessibility identifier browserContentAccessibilityIdentifier from WebViewRepresentable to avoid XCUITest "Multiple matches found" errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanelView.swift, line 867:
<comment>Remove the duplicate accessibility identifier `browserContentAccessibilityIdentifier` from `WebViewRepresentable` to avoid XCUITest "Multiple matches found" errors.</comment>
<file context>
@@ -839,40 +840,46 @@ struct BrowserPanelView: View {
+ .accessibilityIdentifier("BrowserWebViewSurface")
+ // Keep the host stable for normal pane churn, but force a remount when
+ // BrowserPanel replaces its underlying WKWebView after process termination.
+ .id(webViewSurfaceIdentity)
+ .contentShape(Rectangle())
+ .accessibilityIdentifier(browserContentAccessibilityIdentifier)
</file context>
d60c685 to
d959d40
Compare
* Fix browser pane webview sizing * Guard browser portal by pane ownership * Keep browser portal frame during transient zero geometry * Guard portal surfaces against duplicate hosts * Defer terminal surface reflow during tab drags * Fix browser panel pane ID unit tests
This branch was successfully deployed
1 participant
Summary
Testing
Fixes #1093
Summary by cubic
Switches the browser pane to constraint-based layout so the portal anchor and hosted
WKWebViewalways fill the pane, and preserves the portal frame during transient zero geometry to prevent flicker. Restricts portal ownership to the active pane to avoid stale hosts during splits and tab moves, improves drag/overlay behavior, and reduces jank during tab drags. Fixes #1093.WKWebViewviapinHostedWebView; force layout on attach/update; ensure fill with.frame(.infinity),.layoutPriority(1), and address bar.fixedSize; preserve the visible frame across transient zero-geometry (clears overlays/context when preserving).paneId; compose the web view host.idwithpaneIdto remount on pane moves; install the anchor only for the owner; release leases on dismantle.WKWebView(only sidebar tab reorders bypass); keep drop-zone overlays and top-chrome height in sync; remove the moved‑browser refresh path; fix browser panel pane ID unit tests.Written for commit 4d86924. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes