Repository navigation
Fix bottom-docked DevTools page shift - #1212
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded helpers to detect and repair bottom-docked WKInspector layouts and updated portal synchronization to prefer a specialized bottom-docked page-frame repair before falling back to the previous outside-bounds fix; tests and WebView reparenting were adjusted to preserve slot-local frames during moves. Changes
Sequence DiagramsequenceDiagram
participant Portal as BrowserWindowPortal
participant Container as ContainerView/Slot
participant WebView as WKWebView
participant Inspector as WKInspector
Portal->>Portal: portalDidSync()
Portal->>Inspector: hasVisibleInspectorDescendant(in: container)?
Inspector-->>Portal: visible (yes/no)
alt inspector visible
Portal->>Portal: inferredBottomDockedInspectorFrame(in: container, primaryWebView)
Portal-->>Portal: bottom-docked rect?
alt bottom-docked detected
Portal->>Portal: repairedBottomDockedPageFrame(...)
Portal-->>Portal: corrected page frame
Portal->>WebView: apply repaired frame
Portal->>Portal: DEBUG log old/new frames & inspector metrics
Portal->>Portal: append "webFrameBottomDock" to refreshReasons
else not detected
Portal->>Portal: fall back to frameExtendsOutsideBounds repair
Portal->>WebView: apply original repair frame
end
else inspector not visible
Portal->>WebView: no inspector repair needed
end
Portal->>Container: ensure layout/reflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
Greptile SummaryThis PR fixes the bottom-docked DevTools panel causing the page viewport to overflow the portal slot by adding a dedicated geometry-repair path in Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[synchronizeWebViewForAnchor] --> B{containerOwnsWebView?}
B -- No --> Z[Skip web-frame repair]
B -- Yes --> C[repairedBottomDockedPageFrame]
C --> D{frameExtendsOutsideBounds?}
D -- No --> E[return nil]
D -- Yes --> F[inferredBottomDockedInspectorFrame]
F --> G{Any subview with\nhasVisibleInspectorDescendant\nat container bottom?}
G -- No --> E
G -- Yes --> H[Pick tallest candidate]
H --> I[Compute NSRect\nx=containerBounds.minX\ny=inspectorFrame.maxY\nw=containerBounds.width\nh=containerBounds.maxY−inspectorFrame.maxY]
I --> J[Apply repairedBottomDockFrame\nvia CATransaction\nappend 'webFrameBottomDock']
E --> K{frameExtendsOutsideBounds\nfallback check}
K -- Yes --> L[Clamp webView to containerBounds\nappend 'webFrame']
K -- No --> Z
Last reviewed commit: fbd04ca |
![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: fbd04ca09e
ℹ️ 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".
| if current !== root { | ||
| let className = String(describing: type(of: current)) | ||
| if className.contains("WKInspector"), | ||
| !current.isHidden, |
There was a problem hiding this comment.
The new bottom-dock repair can miss real inspector layouts because hasVisibleInspectorDescendant skips checking the root view itself (current !== root). If WebKit exposes the inspector as a direct sibling of the page view (a pattern already handled elsewhere via isInspectorView), this helper returns false when that inspector view has no WKInspector-named descendants, so repairedBottomDockedPageFrame never runs and sync falls back to full-frame normalization that reintroduces the page shift.
Useful? React with 👍 / 👎.
![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
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/BrowserWindowPortal.swift">
<violation number="1" location="Sources/BrowserWindowPortal.swift:2234">
P2: The traversal loop unconditionally enters `current.subviews` even for hidden nodes, and bypasses the `isHidden` check for the `root` node. Because AppKit child views report `isHidden == false` even when their parent is hidden, this causes closed but cached DevTools containers to falsely report as visible and trigger unnecessary geometry repairs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| while let current = stack.popLast() { | ||
| if current !== root { | ||
| let className = String(describing: type(of: current)) | ||
| if className.contains("WKInspector"), | ||
| !current.isHidden, | ||
| current.alphaValue > 0, | ||
| current.frame.width > 1, | ||
| current.frame.height > 1 { | ||
| return true | ||
| } | ||
| } | ||
| stack.append(contentsOf: current.subviews) | ||
| } |
There was a problem hiding this comment.
P2: The traversal loop unconditionally enters current.subviews even for hidden nodes, and bypasses the isHidden check for the root node. Because AppKit child views report isHidden == false even when their parent is hidden, this causes closed but cached DevTools containers to falsely report as visible and trigger unnecessary geometry repairs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/BrowserWindowPortal.swift, line 2234:
<comment>The traversal loop unconditionally enters `current.subviews` even for hidden nodes, and bypasses the `isHidden` check for the `root` node. Because AppKit child views report `isHidden == false` even when their parent is hidden, this causes closed but cached DevTools containers to falsely report as visible and trigger unnecessary geometry repairs.</comment>
<file context>
@@ -2229,6 +2229,71 @@ final class WindowBrowserPortal: NSObject {
+ private static func hasVisibleInspectorDescendant(in root: NSView) -> Bool {
+ var stack: [NSView] = [root]
+ while let current = stack.popLast() {
+ if current !== root {
+ let className = String(describing: type(of: current))
</file context>
| while let current = stack.popLast() { | |
| if current !== root { | |
| let className = String(describing: type(of: current)) | |
| if className.contains("WKInspector"), | |
| !current.isHidden, | |
| current.alphaValue > 0, | |
| current.frame.width > 1, | |
| current.frame.height > 1 { | |
| return true | |
| } | |
| } | |
| stack.append(contentsOf: current.subviews) | |
| } | |
| while let current = stack.popLast() {\n if current.isHidden || current.alphaValue <= 0 { continue }\n if current !== root {\n let className = String(describing: type(of: current))\n if className.contains("WKInspector"),\n current.frame.width > 1,\n current.frame.height > 1 {\n return true\n }\n }\n stack.append(contentsOf: current.subviews)\n } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
2496-2506: Consider extracting a generic recursive view lookup helper.This duplicates the DFS pattern already used by
findHostContainerView(in:), so a small genericfindSubview<T: NSView>(ofType:in:)helper would keep future test helpers from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 2496 - 2506, Extract the duplicated DFS into a generic helper like findSubview<T: NSView>(ofType: T.Type, in root: NSView) -> T? and replace both findWindowBrowserSlotView(in:) and the existing findHostContainerView(in:) call sites to use it: implement the helper to check if root is T then recurse through root.subviews, and update callers to call findSubview(ofType: WindowBrowserSlotView.self, in: root) and findSubview(ofType: HostContainerView.self, in: root) so the recursive logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 2496-2506: Extract the duplicated DFS into a generic helper like
findSubview<T: NSView>(ofType: T.Type, in root: NSView) -> T? and replace both
findWindowBrowserSlotView(in:) and the existing findHostContainerView(in:) call
sites to use it: implement the helper to check if root is T then recurse through
root.subviews, and update callers to call findSubview(ofType:
WindowBrowserSlotView.self, in: root) and findSubview(ofType:
HostContainerView.self, in: root) so the recursive logic is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50d18cda-28e7-4433-9a8d-9ddf68f6a582
📒 Files selected for processing (2)
Sources/Panels/BrowserPanelView.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d82a29f45
ℹ️ 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".
| if className.contains("WKInspector"), | ||
| !current.isHidden, | ||
| current.alphaValue > 0, | ||
| current.frame.width > 1, | ||
| current.frame.height > 1 { |
There was a problem hiding this comment.
hasVisibleInspectorDescendant only checks isHidden/alphaValue on the current node, so a hidden inspector container can still be treated as visible when its WKInspector child views are not individually hidden. In that case inferredBottomDockedInspectorFrame can keep selecting an actually invisible bottom-dock candidate and webFrameBottomDock repair continues to reserve inspector space, leaving the page permanently shrunk after DevTools is hidden via wrapper visibility changes.
Useful? React with 👍 / 👎.
…hed-up # Conflicts: # 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 `@Sources/BrowserWindowPortal.swift`:
- Around line 2262-2291: The detector currently skips the root view in
hasVisibleInspectorDescendant(in:) so a bottom-docked inspector that is a direct
sibling/subview (i.e. the candidate itself) is missed; update the logic so the
candidate root is also examined for the WKInspector* class and visibility checks
(or add an explicit check for the candidate before calling
hasVisibleInspectorDescendant) so
inferredBottomDockedInspectorFrame(in:containerView:primaryWebView:epsilon:) can
detect and return a repaired bottom-docked inspector frame instead of falling
back to the generic reset; reference the functions hasVisibleInspectorDescendant
and inferredBottomDockedInspectorFrame (and the candidate guard currently
filtering out primaryWebView) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20a1b152-fc27-40c5-adaf-d240ea75bf0e
📒 Files selected for processing (2)
Sources/BrowserWindowPortal.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
| private static func hasVisibleInspectorDescendant(in root: NSView) -> Bool { | ||
| var stack: [NSView] = [root] | ||
| while let current = stack.popLast() { | ||
| if current !== root { | ||
| let className = String(describing: type(of: current)) | ||
| if className.contains("WKInspector"), | ||
| !current.isHidden, | ||
| current.alphaValue > 0, | ||
| current.frame.width > 1, | ||
| current.frame.height > 1 { | ||
| return true | ||
| } | ||
| } | ||
| stack.append(contentsOf: current.subviews) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| private static func inferredBottomDockedInspectorFrame( | ||
| in containerView: NSView, | ||
| primaryWebView: WKWebView, | ||
| epsilon: CGFloat = 1 | ||
| ) -> NSRect? { | ||
| let pageFrame = primaryWebView.frame | ||
| let containerBounds = containerView.bounds | ||
|
|
||
| let candidates = containerView.subviews.compactMap { candidate -> NSRect? in | ||
| guard candidate !== primaryWebView else { return nil } | ||
| guard hasVisibleInspectorDescendant(in: candidate) else { return nil } | ||
|
|
There was a problem hiding this comment.
Handle direct WKInspector* siblings in the detector.
Line 2265 skips root itself, so a bottom-docked inspector that is reparented as a direct companion subview never matches this path. In that case repairedBottomDockedPageFrame(...) stays nil and we fall back to the generic full-bounds reset instead of parking the page above the inspector.
Suggested fix
private static func hasVisibleInspectorDescendant(in root: NSView) -> Bool {
var stack: [NSView] = [root]
while let current = stack.popLast() {
- if current !== root {
- let className = String(describing: type(of: current))
- if className.contains("WKInspector"),
- !current.isHidden,
- current.alphaValue > 0,
- current.frame.width > 1,
- current.frame.height > 1 {
- return true
- }
- }
+ let className = String(describing: type(of: current))
+ if className.contains("WKInspector"),
+ !current.isHidden,
+ current.alphaValue > 0,
+ current.frame.width > 1,
+ current.frame.height > 1 {
+ return true
+ }
stack.append(contentsOf: current.subviews)
}
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 2262 - 2291, The detector
currently skips the root view in hasVisibleInspectorDescendant(in:) so a
bottom-docked inspector that is a direct sibling/subview (i.e. the candidate
itself) is missed; update the logic so the candidate root is also examined for
the WKInspector* class and visibility checks (or add an explicit check for the
candidate before calling hasVisibleInspectorDescendant) so
inferredBottomDockedInspectorFrame(in:containerView:primaryWebView:epsilon:) can
detect and return a repaired bottom-docked inspector frame instead of falling
back to the generic reset; reference the functions hasVisibleInspectorDescendant
and inferredBottomDockedInspectorFrame (and the candidate guard currently
filtering out primaryWebView) when making the change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e35e1cd94c
ℹ️ 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".
| guard frame.minY <= containerBounds.minY + epsilon else { return nil } | ||
| guard frame.maxY <= pageFrame.minY + epsilon else { return nil } |
There was a problem hiding this comment.
The bottom-dock inference currently accepts candidates solely by minY <= container minY and maxY <= page minY, which also matches inspector containers that are entirely below the slot (for example, during hide/transition states where a WKInspector view is still visible but has a negative y). In that case repairedBottomDockedPageFrame uses a negative inspectorFrame.maxY, producing another out-of-bounds page frame and repeatedly applying the wrong repair. Add a bounds-intersection check (or at least require frame.maxY >= containerBounds.minY) before selecting a candidate.
Useful? React with 👍 / 👎.
…e-pushed-up Fix bottom-docked DevTools page shift
This branch was successfully deployed
1 participant
Summary
Testing
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-task-browser-page-pushed-up-tests -only-testing:cmuxTests/BrowserWindowPortalLifecycleTests/testPortalSyncRepairsBottomDockedInspectorOverflowedPageFrame test(fails on commit94c5d621e, passes on this commit)xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-task-browser-page-pushed-up-tests -only-testing:cmuxTests/BrowserWindowPortalLifecycleTests/testPortalResizePreservesSideDockedInspectorManagedWebViewFrame -only-testing:cmuxTests/BrowserWindowPortalLifecycleTests/testPortalSyncRepairsBottomDockedInspectorOverflowedPageFrame testIssues
Summary by cubic
Fixes the bottom-docked DevTools bug that pushed the page upward and clipped headers. Also fixes geometry when replacing a visible local host so the inspector and page stay aligned.
WKInspectorfrom visible subviews and repairs theWKWebViewframe during portal sync to keep the page below the inspector and flush with the slot.Written for commit e35e1cd. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests