Repository navigation
Fix spinner hang after display resolution changes - #1549
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughHostContainerView now defers and coalesces portal-geometry synchronization via a new private synchronization helper, replacing immediate per-host registry calls. TerminalWindowPortal synchronization also gains an extra asynchronous hop. A unit test verifies synchronization waits for queued layout shifts. Changes
Sequence Diagram(s)sequenceDiagram
participant AppKit as AppKit Layout
participant HCV as HostContainerView
participant DQ as DispatchQueue.main
participant Portal as TerminalWindowPortal
rect rgba(220, 20, 60, 0.5)
Note over AppKit,Portal: Previous (Synchronous Re-entry)
AppKit->>HCV: onGeometryChanged()
HCV->>Portal: synchronizeForAnchor(host) [immediate]
Portal->>Portal: reconcileGeometryNow()
Portal->>Portal: refreshSurfaceNow()
Note over HCV,Portal: ⚠️ Can re-enter AppKit layout / block
end
rect rgba(34, 139, 34, 0.5)
Note over AppKit,Portal: New (Deferred, Coalesced)
AppKit->>HCV: onGeometryChanged()
HCV->>DQ: synchronizePortalGeometry(for:host, coordinator:)
DQ->>Portal: scheduleExternalGeometrySynchronize() [coalesced]
DQ->>DQ: nested async hop (delay to next run loop tick)
DQ->>Portal: synchronizeAllEntriesFromExternalGeometryChange()
Note over HCV,Portal: ✓ Non-blocking, uses settled layout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 8d1d4722f6
ℹ️ 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".
| coordinator.lastSynchronizedHostGeometryRevision = geometryRevision | ||
| // Avoid synchronizing the terminal portal while AppKit is still inside | ||
| // the current layout turn. Re-entrant syncs here can wedge window resize | ||
| // handling and leave the app spinning on the wait cursor. | ||
| TerminalWindowPortalRegistry.scheduleExternalGeometrySynchronizeForAllWindows() |
There was a problem hiding this comment.
scheduleDeferredPortalGeometrySynchronize now always queues scheduleExternalGeometrySynchronizeForAllWindows() after a geometry revision bump, but unlike the previous synchronizeForAnchor(host) path it no longer short-circuits when host.window == nil. During SwiftUI reparent/detach churn (viewDidMoveToWindow/viewDidMoveToSuperview), this can trigger unnecessary full-portal syncs across every window, including reconcileGeometryNow()/refreshSurfaceNow() on unrelated terminals, which is a user-visible performance regression under window/workspace churn.
Useful? React with 👍 / 👎.
![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)
13124-13125: Use deterministic queue draining instead of fixed sleep.Line 13124 uses a timed
RunLoopwait, which can be flaky for nestedDispatchQueue.main.asyncchains on busy CI machines.♻️ Suggested stabilization
- RunLoop.current.run(until: Date().addingTimeInterval(0.05)) + let firstDrain = expectation(description: "drain main queue (1)") + DispatchQueue.main.async { firstDrain.fulfill() } + wait(for: [firstDrain], timeout: 1.0) + + let secondDrain = expectation(description: "drain main queue (2)") + DispatchQueue.main.async { secondDrain.fulfill() } + wait(for: [secondDrain], timeout: 1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 13124 - 13125, The test currently uses a fixed RunLoop.sleep call (RunLoop.current.run(until: Date().addingTimeInterval(0.05))) which is flaky; replace it with a deterministic queue drain using an XCTestExpectation: schedule a DispatchQueue.main.async that fulfills an expectation and then call wait(for: [expectation], timeout: X) so the test only proceeds after the enqueued main-queue work completes (this avoids relying on timed sleeps for nested DispatchQueue.main.async chains). Use the existing RunLoop/current dispatching point as the location to swap in the expectation-based drain.
🤖 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 13124-13125: The test currently uses a fixed RunLoop.sleep call
(RunLoop.current.run(until: Date().addingTimeInterval(0.05))) which is flaky;
replace it with a deterministic queue drain using an XCTestExpectation: schedule
a DispatchQueue.main.async that fulfills an expectation and then call wait(for:
[expectation], timeout: X) so the test only proceeds after the enqueued
main-queue work completes (this avoids relying on timed sleeps for nested
DispatchQueue.main.async chains). Use the existing RunLoop/current dispatching
point as the location to swap in the expectation-based drain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3ff42d1-7177-4c96-8973-0b094ff912e7
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/TerminalWindowPortal.swiftcmuxTests/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/GhosttyTerminalView.swift`:
- Around line 8583-8585: The code currently calls
TerminalWindowPortalRegistry.synchronizeForAnchor(host) immediately when
host.inLiveResize or host.window?.inLiveResize == true, which can re-enter
AppKit during live-resize; change this to defer synchronization instead of
calling synchronizeForAnchor synchronously — e.g., skip the immediate call when
host.inLiveResize is true and schedule a deferred synchronization for that host
(using a main-queue async/next-runloop dispatch or a window live-resize end
observer) so TerminalWindowPortalRegistry.synchronizeForAnchor(host) runs after
live-resize finishes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d943118-d77f-414e-8dca-f79eccaf5e70
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftSources/TerminalWindowPortal.swift
| if host.inLiveResize || host.window?.inLiveResize == true { | ||
| TerminalWindowPortalRegistry.synchronizeForAnchor(host) | ||
| return |
There was a problem hiding this comment.
Avoid immediate portal synchronization during live-resize churn
Line 8584 still calls the immediate synchronizeForAnchor path during live resize. That can re-enter AppKit layout/resize work and reintroduce the spinner-hang path this PR is targeting.
💡 Proposed fix
- if host.inLiveResize || host.window?.inLiveResize == true {
- TerminalWindowPortalRegistry.synchronizeForAnchor(host)
- return
- }
- // Avoid synchronizing the terminal portal while AppKit is still inside
- // the current layout turn. Re-entrant syncs here can wedge window resize
- // handling and leave the app spinning on the wait cursor.
- TerminalWindowPortalRegistry.scheduleExternalGeometrySynchronizeForAllWindows()
+ // Avoid re-entrant portal sync during AppKit layout/live-resize churn.
+ // Keep this deferred so geometry reconciliation happens after the current turn.
+ TerminalWindowPortalRegistry.scheduleExternalGeometrySynchronizeForAllWindows()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 8583 - 8585, The code
currently calls TerminalWindowPortalRegistry.synchronizeForAnchor(host)
immediately when host.inLiveResize or host.window?.inLiveResize == true, which
can re-enter AppKit during live-resize; change this to defer synchronization
instead of calling synchronizeForAnchor synchronously — e.g., skip the immediate
call when host.inLiveResize is true and schedule a deferred synchronization for
that host (using a main-queue async/next-runloop dispatch or a window
live-resize end observer) so
TerminalWindowPortalRegistry.synchronizeForAnchor(host) runs after live-resize
finishes.
…ution-spinner Fix spinner hang after display resolution changes
This branch was successfully deployed
1 participant
Summary
Testing
xcodebuild -quiet -project /Users/lawrence/fun/cmuxterm-hq/worktrees/issue-1541-resolution-spinner/GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-1541-red-final -only-testing:cmuxTests/TerminalWindowPortalLifecycleTests/testScheduledExternalGeometrySyncRefreshesAncestorLayoutShift -only-testing:cmuxTests/TerminalWindowPortalLifecycleTests/testScheduledExternalGeometrySyncWaitsForQueuedLayoutShift test(fails on94f7529a, new regression test fails)xcodebuild -quiet -project /Users/lawrence/fun/cmuxterm-hq/worktrees/issue-1541-resolution-spinner/GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-1541-green3 -only-testing:cmuxTests/TerminalWindowPortalLifecycleTests/testScheduledExternalGeometrySyncRefreshesAncestorLayoutShift -only-testing:cmuxTests/TerminalWindowPortalLifecycleTests/testScheduledExternalGeometrySyncWaitsForQueuedLayoutShift test(passes)./scripts/reload.sh --tag issue-1541-resolution-spinner(build succeeds)Issues
Summary by cubic
Fixes a spinner hang after display resolution changes by deferring terminal portal geometry sync until AppKit finishes the current layout pass while keeping syncs immediate during live resize; closes #1541. Adds a regression test that verifies the sync waits for queued layout shifts and updates the portal position correctly.
GhosttyTerminalViewwith a revision-keyed scheduler to dedupe updates.Written for commit 95ef1c8. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Behavior
Tests