Repository navigation
Glue terminal frames to live window resize ticks #11323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -158,6 +158,65 @@ extension TerminalWindowPortalLifecycleTests { | |
| withExtendedLifetime((leftSurface, rightSurface)) {} | ||
| } | ||
|
|
||
| /// Regression: during a live window resize, each `didResize` tick must | ||
| /// synchronize hosted terminal frames INSIDE the tick — in the same | ||
| /// transaction that commits the window's new size. The portal's queued | ||
| /// sync (one main-queue hop) paints every tick with the PREVIOUS tick's | ||
| /// hosted frames, so the terminal visibly trails the window edge during | ||
| /// the whole drag (Ghostty hosts surfaces directly in the hierarchy and | ||
| /// has no such gap). | ||
| @MainActor | ||
| func testLiveResizeTickSynchronizesHostedFrameWithinTheSameTick() throws { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: This new non-UI regression test in cmuxTests is added to an XCTestCase suite, but the repo's test policy prefers Swift Testing (@Suite/@test) for new/touched non-UI tests, even inside files that already host XCTest suites. Move this test to a Swift Testing suite (the shared fixtures would need to be reachable from it) rather than growing the XCTest extension. Prompt for AI agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept as XCTest deliberately: the test depends on the tracked-window/portal/surface fixtures and leak-checked teardown that live on the XCTestCase base class (TerminalWindowPortalLifecycleTests), and every sibling regression in this suite is an extension of it. Porting those shared fixtures to Swift Testing is a suite-wide migration, not something to fork inside this fix PR. |
||
| let window = makeTestWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: 520, height: 340), | ||
| styleMask: [.titled, .closable, .resizable] | ||
| ) | ||
| defer { | ||
| NotificationCenter.default.post(name: NSWindow.willCloseNotification, object: window) | ||
| window.orderOut(nil) | ||
| } | ||
| realizeWindowLayout(window) | ||
| guard let contentView = window.contentView else { | ||
| XCTFail("Expected content view") | ||
| return | ||
| } | ||
|
|
||
| let portal = makeTrackedPortal(window: window) | ||
| let anchor = NSView(frame: NSRect(x: 8, y: 8, width: 240, height: 160)) | ||
| anchor.autoresizingMask = [.width, .height] | ||
| contentView.addSubview(anchor) | ||
|
|
||
| let surface = makeTrackedTerminalSurface() | ||
| portal.bind(hostedView: surface.hostedView, to: anchor, visibleInUI: true) | ||
| portal.synchronizeHostedViewForAnchor(anchor) | ||
| drainMainQueue() | ||
| realizeWindowLayout(window) | ||
| XCTAssertEqual( | ||
| surface.hostedView.frame.size, | ||
| NSSize(width: 240, height: 160), | ||
| "Precondition: the hosted view tracks the anchor at rest" | ||
| ) | ||
|
|
||
| portal.isWindowLiveResizeActiveOverrideForTesting = true | ||
|
|
||
| // One live-resize tick: setFrame posts didResize synchronously and | ||
| // the anchor grows with the content view through its autoresizing | ||
| // mask before the notification fires. | ||
| var frame = window.frame | ||
| frame.size.width += 100 | ||
| frame.size.height += 60 | ||
| window.setFrame(frame, display: false) | ||
|
|
||
| // No queue drain on purpose: the assertion runs before any queued | ||
| // portal pass can fire, exactly like the tick's own CA commit does. | ||
| XCTAssertEqual( | ||
| surface.hostedView.frame.size, | ||
| NSSize(width: 340, height: 220), | ||
| "A live-resize tick must glue hosted frames within the same tick, not a runloop turn later" | ||
| ) | ||
| withExtendedLifetime(surface) {} | ||
| } | ||
|
|
||
| /// Regression: switching a pane's tab from a terminal to a browser hides | ||
| /// the terminal only through its registry entry — the SwiftUI update that | ||
| /// carries visible=false is dropped by the portal-host ownership gate | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
Repository: manaflow-ai/cmux
Length of output: 22533
🏁 Script executed:
Repository: manaflow-ai/cmux
Length of output: 37339
🌐 Web query:
Apple Core Animation implicit transaction commits end of run loop DispatchQueue.main.async ordering before transaction commit💡 Result:
Core Animation implicit transactions are automatically committed by the system at the end of a run loop iteration [1][2][3]. This occurs because Core Animation registers a run loop observer for the kCFRunLoopBeforeWaiting activity [4][3][5]. When you modify the layer tree (e.g., updating a UIView property) on the main thread, Core Animation detects the change and implicitly creates a transaction if one does not already exist [4][6][7]. Because the Core Animation observer is triggered during the kCFRunLoopBeforeWaiting phase—which happens after standard source processing (including the execution of blocks on the main dispatch queue)—the implicit transaction typically commits after tasks submitted via DispatchQueue.main.async have finished executing in the current run loop pass [4][3][5]. In summary, the ordering is as follows: 1. Tasks queued on DispatchQueue.main.async are executed by the run loop [5]. 2. If those tasks modify the layer tree, they participate in the implicit transaction [7]. 3. Once the run loop has processed sources and blocks, it reaches the kCFRunLoopBeforeWaiting state [4][3]. 4. The Core Animation observer triggers, committing the implicit transaction and rendering the changes [4][3][5]. This mechanism ensures that multiple property changes made within the same run loop iteration are batched together into a single atomic update [1][2][7].
Citations:
🏁 Script executed:
Repository: manaflow-ai/cmux
Length of output: 25612
Use a transaction-complete signal before the reveal refresh.
deferSurfaceRefreshcallsrefreshSurfaceNow, which invokesdisplayIfNeededon the hosted view and surface.DispatchQueue.main.asynccan run before Core Animation commits thedidResizetransaction, so the reveal refresh can still execute during that transaction.Use an explicit post-transaction or renderer-safe completion signal.
🤖 Prompt for AI Agents
Source: Coding guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one-turn main-queue hop is the mechanism this file already uses for exactly this hazard:
deferSurfaceRefreshwas introduced (see the comment onpendingDeferredSurfaceRefreshes) so refreshes requested inside someone else's layout/update pass drain on the next main-queue turn, and the divider-drag paths have shipped on it. The main dispatch queue is serviced by the run loop before the BeforeWaiting observer that commits the implicit transaction only in the next iteration; a block enqueued while the didResize tick is on the stack runs after that tick's commit. This change routes the (rare) mid-live-resize reveal through that same existing mechanism rather than inventing a second transaction-complete signal beside it. If we ever see the wedge fire from a deferred refresh, the right fix is to replace the shared mechanism, not just this call site.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🤖 web_search executed:
Apple documentation NSWindow didResizeNotification Core Animation transaction completion DispatchQueue.main.async guarantee transaction commitLength of output: 11606
@azooz2003-bit, I agree that this call site should reuse the shared mechanism.However, the documented contracts do not guarantee the required ordering.
CATransactionstates that an implicit transaction commits when the run loop next iterates.DispatchQueue.main.asynconly schedules asynchronous main-queue work. It does not guarantee that the work runs after the current AppKit/Core Animation transaction commits.Therefore,
deferSurfaceRefreshcan remain the single shared mechanism, but it needs a transaction-safe completion boundary before it callsrefreshSurfaceNow. That change protects both the live-resize reveal path and the existing layout-update callers.You are interacting with an AI system.