Repository navigation
Fix Cloud terminal garble during pane resize #12918
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
31 changes: 31 additions & 0 deletions
31
...CmuxTerminalCore/Sources/CmuxTerminalCore/Scrollbar/TerminalScrollBarPresencePolicy.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /// Owns the rule that decides whether a terminal scrollbar is present. | ||
| /// | ||
| /// Presence is a layout input for legacy scrollbars. Keeping that presence | ||
| /// independent of the terminal's own scrollback prevents the grid width from | ||
| /// changing when a replay temporarily empties history. | ||
| public struct TerminalScrollBarPresencePolicy: Sendable { | ||
| /// Creates a stateless scrollbar presence policy. | ||
| public init() {} | ||
|
|
||
| /// Returns whether the terminal scrollbar should remain present. | ||
| /// | ||
| /// - Parameters: | ||
| /// - allowedBySettings: Whether terminal scrollbar settings allow a scrollbar. | ||
| /// - scrollerStyle: The style that lays out the terminal scroll view. | ||
| /// - hasScrollback: Whether the terminal has scrollback, or `nil` before | ||
| /// Ghostty publishes its first scrollbar state. | ||
| /// - Returns: `true` when the scroll view should keep its scrollbar present. | ||
| public func isPresent( | ||
| allowedBySettings: Bool, | ||
| scrollerStyle: TerminalScrollerStyle, | ||
| hasScrollback: Bool? | ||
| ) -> Bool { | ||
| guard allowedBySettings else { return false } | ||
| // A legacy scroller reserves layout space, so its presence must not | ||
| // follow scrollback or the terminal grid will change width. | ||
| if scrollerStyle == .legacy { return true } | ||
| // Ghostty reports scrollback asynchronously. Keep the overlay present | ||
| // until the first packet so restored surfaces do not appear broken. | ||
| return hasScrollback ?? true | ||
| } | ||
| } |
8 changes: 8 additions & 0 deletions
8
...ges/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Scrollbar/TerminalScrollerStyle.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /// The two AppKit scrollbar styles that differ in whether they reserve layout space. | ||
| public enum TerminalScrollerStyle: Equatable, Sendable { | ||
| /// The classic scrollbar reserves a fixed trailing gutter in the terminal grid. | ||
| case legacy | ||
|
|
||
| /// The overlay scrollbar draws over content and reserves no layout space. | ||
| case overlay | ||
| } |
31 changes: 31 additions & 0 deletions
31
...S/CmuxTerminalCore/Tests/CmuxTerminalCoreTests/TerminalScrollBarPresencePolicyTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import Testing | ||
| @testable import CmuxTerminalCore | ||
|
|
||
| @Suite("Terminal scroll bar presence policy") | ||
| struct TerminalScrollBarPresencePolicyTests { | ||
| @Test("legacy scrollers stay present regardless of scrollback") | ||
| func legacyStyleReservesStableGutter() { | ||
| let policy = TerminalScrollBarPresencePolicy() | ||
|
|
||
| #expect(policy.isPresent(allowedBySettings: true, scrollerStyle: .legacy, hasScrollback: false)) | ||
| #expect(policy.isPresent(allowedBySettings: true, scrollerStyle: .legacy, hasScrollback: nil)) | ||
| #expect(policy.isPresent(allowedBySettings: true, scrollerStyle: .legacy, hasScrollback: true)) | ||
| } | ||
|
|
||
| @Test("overlay scrollers follow scrollback") | ||
| func overlayStyleDoesNotReserveGutter() { | ||
| let policy = TerminalScrollBarPresencePolicy() | ||
|
|
||
| #expect(!policy.isPresent(allowedBySettings: true, scrollerStyle: .overlay, hasScrollback: false)) | ||
| #expect(policy.isPresent(allowedBySettings: true, scrollerStyle: .overlay, hasScrollback: nil)) | ||
| #expect(policy.isPresent(allowedBySettings: true, scrollerStyle: .overlay, hasScrollback: true)) | ||
| } | ||
|
|
||
| @Test("disabled settings always hide the scrollbar") | ||
| func settingsOverrideStyle() { | ||
| let policy = TerminalScrollBarPresencePolicy() | ||
|
|
||
| #expect(!policy.isPresent(allowedBySettings: false, scrollerStyle: .legacy, hasScrollback: true)) | ||
| #expect(!policy.isPresent(allowedBySettings: false, scrollerStyle: .overlay, hasScrollback: true)) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import AppKit | ||
| import CmuxTerminalCore | ||
| import Foundation | ||
| import Testing | ||
|
|
||
| #if canImport(cmux_DEV) | ||
| @testable import cmux_DEV | ||
| #elseif canImport(cmux) | ||
| @testable import cmux | ||
| #endif | ||
|
|
||
| /// The terminal grid must not depend on the terminal's own content. | ||
| /// | ||
| /// A legacy scroller reserves a gutter. When its presence followed scrollback, | ||
| /// a Cloud mirror whose replay reset empties history reported a new grid after | ||
| /// every remote `resized` replay, the remote PTY resized again and replayed | ||
| /// again, and Codex received a SIGWINCH storm that garbled and flickered its | ||
| /// frame (https://github.com/manaflow-ai/cmux/issues/12885). The same | ||
| /// dependency reflowed a local pane when its first row scrolled off | ||
| /// (https://github.com/manaflow-ai/cmux/issues/3051). | ||
| @MainActor | ||
| @Suite("Terminal scroll bar gutter stability", .serialized) | ||
| struct TerminalScrollBarGutterStabilityTests { | ||
| /// A pane hosted in an offscreen window so the scroll view tiles for real. | ||
| @MainActor | ||
| private final class Harness { | ||
| let window: NSWindow | ||
| let hostedView: GhosttySurfaceScrollView | ||
| let paneWidth: CGFloat = 640 | ||
|
|
||
| init(scrollerStyle: NSScroller.Style) { | ||
| let surfaceView = GhosttyNSView(frame: .zero) | ||
| hostedView = GhosttySurfaceScrollView(surfaceView: surfaceView) | ||
| window = NSWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: paneWidth, height: 400), | ||
| styleMask: [.borderless], | ||
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
| window.contentView?.addSubview(hostedView) | ||
| hostedView.frame = window.contentView?.bounds ?? .zero | ||
| // The hosted view leaves the style to AppKit, which derives it from | ||
| // the system preference; pin it on the scroll view itself, the | ||
| // object AppKit tiles by, so the test is the same on every Mac. | ||
| let scrollView = hostedView.subviews.compactMap { $0 as? NSScrollView }.first | ||
| scrollView?.scrollerStyle = scrollerStyle | ||
| hostedView.needsLayout = true | ||
| hostedView.layoutSubtreeIfNeeded() | ||
| } | ||
|
|
||
| /// Publishes one Ghostty scrollbar packet the way the runtime does and | ||
| /// returns the width the terminal surface is laid out with afterwards. | ||
| func contentWidth(after scrollbar: GhosttyScrollbar) -> CGFloat { | ||
| hostedView.surfaceView.scrollbar = scrollbar | ||
| NotificationCenter.default.post( | ||
| name: .ghosttyDidUpdateScrollbar, | ||
| object: hostedView.surfaceView, | ||
| userInfo: [GhosttyNotificationKey.scrollbar: scrollbar] | ||
| ) | ||
| hostedView.layoutSubtreeIfNeeded() | ||
| return hostedView.surfaceView.frame.width | ||
| } | ||
| } | ||
|
|
||
| private static let emptyHistory = GhosttyScrollbar(total: 40, offset: 0, len: 40) | ||
| private static let withHistory = GhosttyScrollbar(total: 400, offset: 360, len: 40) | ||
|
|
||
| @Test("A legacy scroller keeps the same content width with and without scrollback") | ||
| func legacyScrollerGutterDoesNotFollowScrollback() { | ||
| let harness = Harness(scrollerStyle: .legacy) | ||
| let gutter = NSScroller.scrollerWidth(for: .regular, scrollerStyle: .legacy) | ||
|
|
||
| // Attach replay: history exists. Resize replay: the reset (RIS + CSI 3 J) | ||
| // empties it, then the replay refills it. The grid must not move. | ||
| let withHistory = harness.contentWidth(after: Self.withHistory) | ||
| let afterReset = harness.contentWidth(after: Self.emptyHistory) | ||
| let afterReplay = harness.contentWidth(after: Self.withHistory) | ||
|
|
||
| #expect(withHistory == harness.paneWidth - gutter) | ||
| #expect(afterReset == withHistory, "the reset released the legacy gutter and widened the grid") | ||
| #expect(afterReplay == withHistory, "the replay reclaimed the legacy gutter and narrowed the grid") | ||
| } | ||
|
|
||
| @Test("An overlay scroller never changes the content width") | ||
| func overlayScrollerReservesNoGutter() { | ||
| let harness = Harness(scrollerStyle: .overlay) | ||
|
|
||
| let withHistory = harness.contentWidth(after: Self.withHistory) | ||
| let afterReset = harness.contentWidth(after: Self.emptyHistory) | ||
|
|
||
| #expect(withHistory == harness.paneWidth) | ||
| #expect(afterReset == harness.paneWidth) | ||
| } | ||
|
|
||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: manaflow-ai/cmux
Length of output: 50372
🤖 get_repo_knowledge executed:
get_repo_knowledge manaflow-ai/cmux /tmp/coderabbit-repo-knowledge/manaflow-ai-cmux-b0f68d40/learnings /tmp/coderabbit-repo-knowledge/manaflow-ai-cmux-b0f68d40/conventionsLength of output: 47727
🏁 Script executed:
Repository: manaflow-ai/cmux
Length of output: 50375
Reuse one capability snapshot for the full browser-proxy attempt.
browserProxyprobesclientCapabilitiesfor thebrowser-proxyguard, then probes it again beforeopenCmuxRemote. If the second probe fails or returns a different list,VMClient.openCmuxRemotereceives[]and omitsclientCapabilitiesfrom/api/vm/{id}/attach-endpoint. The request can therefore carry different capability state from the preflight that allowed the attempt to proceed. Bind the first successful probe result and reuse it foropenCmuxRemoteinstead of falling back to[]. This keeps one capability snapshot as the source of truth for the operation.🤖 Prompt for AI Agents
Source: Path instructions