Repository navigation
fix: coalesce browser rendering and teardown - #8958
usr-bin-roygbiv wants to merge 49 commits into
Conversation
|
To use Codex here, create a Codex account and connect to 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:
📝 WalkthroughWalkthroughThe portal now coalesces geometry and presentation refreshes, relies on invalidation instead of synchronous WebKit display flushing, and avoids redundant synchronization. Browser-panel close teardown now shuts down lifecycle resources, detaches hosted views, and installs a replacement web view with expanded regression coverage. ChangesBrowser portal rendering and lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Geometry as External geometry
participant Portal as WindowBrowserPortal
participant Host as BrowserViewportHostView
participant WebView as WKWebView
Geometry->>Portal: synchronizeAllWebViews(source: externalGeometry)
Portal->>Portal: coalesce presentation refresh
Portal->>Host: invalidateBrowserPortalPresentation()
Host->>WebView: request layout and display invalidation
Portal->>WebView: refresh rendering state when required
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches🧪 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 |
|
@codex review |
@usr-bin-roygbiv 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 6 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:
|
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
Greptile SummaryCoalesces browser rendering updates and makes browser-pane teardown deterministic.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (14): Last reviewed commit: "test: cover delayed workspace rebind" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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/BrowserPanelTests.swift (1)
4336-4352: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale
displayIfNeededCountexpectation contradicts the sibling rebind test.Line 4348 still requires
displayIfNeededCountto increase after the rebind, butrunHostedWebViewRefreshPassno longer flushes display — and the equivalent workspace-rebind test at line 4562 now asserts the opposite ("must not synchronously flush WebKit display"). Any pass here comes from incidental AppKit flushing duringaddSubview/unhide, not from a contract, so this will flake.setNeedsDisplayCountis the counter that expresses the intended "refresh presentation" invariant, as already used at line 4547.💚 Align with the sibling rebind assertions
- let displayCountBeforeRebind = webView.displayIfNeededCount + let displayCountBeforeRebind = webView.displayIfNeededCount + let redrawCountBeforeRebind = webView.setNeedsDisplayCount let enterInWindowCountBeforeRebind = webView.enterInWindowCount let endDeferringCountBeforeRebind = webView.endDeferringViewInWindowChangesCount let anchor2 = NSView(frame: anchorFrame) contentView.addSubview(anchor2) portal.bind(webView: webView, to: anchor2, visibleInUI: true) let enterInWindowCountAfterRebind = webView.enterInWindowCount let endDeferringCountAfterRebind = webView.endDeferringViewInWindowChangesCount XCTAssertTrue(webView.superview === slot, "Rebinding after transient anchor removal should reuse the existing portal slot") XCTAssertFalse(slot.isHidden) XCTAssertEqual(portal.debugEntryCount(), 1) XCTAssertGreaterThan( - webView.displayIfNeededCount, - displayCountBeforeRebind, - "Anchor rebinds should refresh hosted browser presentation even when geometry is unchanged" + webView.setNeedsDisplayCount, + redrawCountBeforeRebind, + "Anchor rebinds should invalidate hosted browser presentation even when geometry is unchanged" + ) + XCTAssertEqual( + webView.displayIfNeededCount, + displayCountBeforeRebind, + "Anchor rebind must not synchronously flush WebKit display" )🤖 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/BrowserPanelTests.swift` around lines 4336 - 4352, Update the rebind assertions in the test around portal.bind and replace the displayIfNeededCount increase requirement with the intended setNeedsDisplayCount refresh invariant, matching the sibling workspace-rebind test. Keep the existing superview, visibility, and entry-count assertions unchanged, and do not require synchronous WebKit display flushing.
🤖 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/BrowserPanelTests.swift`:
- Around line 4336-4352: Update the rebind assertions in the test around
portal.bind and replace the displayIfNeededCount increase requirement with the
intended setNeedsDisplayCount refresh invariant, matching the sibling
workspace-rebind test. Keep the existing superview, visibility, and entry-count
assertions unchanged, and do not require synchronous WebKit display flushing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 266c0129-088c-4ebe-a769-d164ebede07f
📒 Files selected for processing (6)
Sources/BrowserWindowPortal.swiftSources/Panels/BrowserPanel+AutomationViewport.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/BrowserPortalFirstRevealScrollTests.swift
💤 Files with no reviewable changes (2)
- Sources/Panels/BrowserPanel+AutomationViewport.swift
- cmuxTests/BrowserPortalFirstRevealScrollTests.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 (1)
cmuxTests/BrowserPanelTests.swift (1)
3497-3503: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace fixed-duration run-loop drains with deterministic completion.
These tests use
advanceAnimations()to wait for deferred work; that helper waits a fixed 250 ms. In the display-count tests, the wait occurs before asserting that no synchronous flush happened, so normal deferred AppKit rendering can be misclassified as a synchronous portal flush. Capture synchronous counters immediately after the operation, then await a real completion signal or the existing one-main-turn helper for deferred assertions.As per coding guidelines and path instructions, tests must avoid fixed wall-clock waits and use deterministic completion signals.
Also applies to: 3541-3569, 3571-3630, 3683-3707
🤖 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/BrowserPanelTests.swift` around lines 3497 - 3503, Update the display-count tests around anchor resizing and the related ranges to capture conversion counters immediately after each operation, before any deferred-work wait. Replace advanceAnimations() fixed-duration drains with a deterministic completion signal or the existing one-main-turn helper, then perform deferred assertions only after that completion.Sources: Coding guidelines, 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/BrowserPanelTests.swift`:
- Around line 3497-3503: Update the display-count tests around anchor resizing
and the related ranges to capture conversion counters immediately after each
operation, before any deferred-work wait. Replace advanceAnimations()
fixed-duration drains with a deterministic completion signal or the existing
one-main-turn helper, then perform deferred assertions only after that
completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 49233c61-7cf1-4604-ad3f-ff8f32c83f3a
📒 Files selected for processing (1)
cmuxTests/BrowserPanelTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
@lawrencecchen, this browser rendering and teardown coalescing fix is ready for review when you have a chance. |
632e033 to
1f74228
Compare
4f6c06a to
adba692
Compare
|
This work has moved to the Roy-owned nightly integration repository: https://github.com/usr-bin-roygbiv/cmux/pull/1 Closing this upstream PR so the nightly repository is the sole integration target. |
1 participant
Summary
Purpose
Coalesce browser-portal geometry and presentation work without synchronous AppKit/WebKit display flushing, keep one explicit rendering-state repair at lifecycle transitions, skip redundant single-browser reconciliation, and synchronously release browser-pane discard and presentation resources on close. This preserves active viewport, rebind, inspector, reveal, recovery, and teardown behavior while reducing avoidable main-thread work.
Fixes #1114.
Related: #4188, #5305, #3085, #7895.
Tests
30210823264compiled the application and test targets at the first complete code-and-test checkpoint. The active-viewport, external-split, workspace-rebind, portal-anchor, and single-entry regressions passed on macOS 15. Repository-wide jobs later reached their time limits, unrelated suites also reported failures, and the Intel macOS 14 job could not start under the account spending limit; this is scoped rather than full-suite-green evidence.30225738150built reviewed source heade779b88e3csuccessfully on macOS 15. The installed tagged app passed activation, local-page loading, scrolling, browser-owned screenshot capture, deterministic resize, a 220-sample live-resize probe with zero browser RPC errors, and the bounded 12-surface close/reclaim probe.213f48628eadds causal refresh checks and delayed-workspace-rebind coverage. Its focused lifecycle, first-reveal, and teardown workflows are pending on macOS 15: run 30323970929, run 30323971382, and run 30323971416. They have no observed conclusions yet and are not claimed as passing. Two earlierdf5c4b9063focused runs ended with workflow exit code 4 instead of reporting an assertion result.Metrics
All measurements used local deterministic fixtures on an Apple Silicon macOS 15 system. They are bounded before/after observations, not universal speed claims.
The memory probe is synthetic and short, not a long-session soak. Whole-window SwiftUI/AppKit layout still produces long frames during aggressive resize. Screen-level capture could not score intermediate hosted-layer tearing, so the evidence does not claim that all long frames, historical memory growth, or compositor tearing are eliminated.
Safety
displayIfNeeded()or invoke WebKit recovery.docs/browser-rendering-performance-investigation.md.Testing
Demo Video
No video attached. Screen-level capture on the validation system omitted the hosted WebKit surface. Browser-owned PNGs plus DOM, animation-frame, RPC, process, and footprint instrumentation verify the exercised behavior; the capture limitation and claim boundary are documented in the investigation report.
Review Trigger (Copy/Paste as PR comment)
Checklist