Skip to content

Fix terminal viewport width reporting in cmux #2926

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.

Sign up for GitHub

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

Open
austinywang wants to merge 1 commit into
base: main
Choose a base branch
from
+64 −20
Open

Fix terminal viewport width reporting in cmux #2926

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 64 additions & 20 deletions Sources/GhosttyTerminalView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4493,7 +4493,8 @@ final class TerminalSurface: Identifiable, ObservableObject {
}

ghostty_surface_set_content_scale(createdSurface, scaleFactors.x, scaleFactors.y)
let backingSize = view.convertToBacking(NSRect(origin: .zero, size: view.bounds.size)).size
let surfaceSize = view.resolvedSurfaceSize(preferred: view.bounds.size)
let backingSize = view.convertToBacking(NSRect(origin: .zero, size: surfaceSize)).size
let wpx = pixelDimension(from: backingSize.width)
let hpx = pixelDimension(from: backingSize.height)
if wpx > 0, hpx > 0 {
Expand Down Expand Up @@ -5212,6 +5213,7 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations {
}

weak var terminalSurface: TerminalSurface?
weak var surfaceHostView: GhosttySurfaceScrollView?
var scrollbar: GhosttyScrollbar?
/// Pending scrollbar value written from the action callback thread;
/// read and cleared on the main thread by `flushPendingScrollbar()`.
Expand Down Expand Up @@ -5618,7 +5620,13 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations {

override var isOpaque: Bool { false }

private func resolvedSurfaceSize(preferred size: CGSize?) -> CGSize {
fileprivate func resolvedSurfaceSize(preferred size: CGSize?) -> CGSize {
if let hostedViewportSize = surfaceHostView?.renderedSurfaceViewportRect(preferredSize: size).size,
hostedViewportSize.width > 0,
hostedViewportSize.height > 0 {
return hostedViewportSize
}

if let size,
size.width > 0,
size.height > 0 {
Expand Down Expand Up @@ -8993,6 +9001,7 @@ final class GhosttySurfaceScrollView: NSView {
documentView.addSubview(surfaceView)

super.init(frame: .zero)
surfaceView.surfaceHostView = self
wantsLayer = true
layer?.masksToBounds = true

Expand Down Expand Up @@ -9393,17 +9402,6 @@ final class GhosttySurfaceScrollView: NSView {
let previousSurfaceSize = surfaceView.frame.size
_ = setFrameIfNeeded(backgroundView, to: bounds)
_ = setFrameIfNeeded(scrollView, to: bounds)
let targetSize = scrollView.bounds.size
#if DEBUG
logLayoutDuringActiveDrag(targetSize: targetSize)
#endif
let targetSurfaceFrame = CGRect(origin: surfaceView.frame.origin, size: targetSize)
_ = setFrameIfNeeded(surfaceView, to: targetSurfaceFrame)
let targetDocumentFrame = CGRect(
origin: documentView.frame.origin,
size: CGSize(width: scrollView.bounds.width, height: documentView.frame.height)
)
_ = setFrameIfNeeded(documentView, to: targetDocumentFrame)
_ = setFrameIfNeeded(inactiveOverlayView, to: bounds)
if let zone = activeDropZone {
attachDropZoneOverlayIfNeeded()
Expand Down Expand Up @@ -9435,6 +9433,18 @@ final class GhosttySurfaceScrollView: NSView {
scrollView.tile()
}
scrollView.layoutSubtreeIfNeeded()
let targetViewportRect = renderedSurfaceViewportRect(preferredSize: scrollView.bounds.size)
let targetSize = targetViewportRect.size
#if DEBUG
logLayoutDuringActiveDrag(targetSize: targetSize)
#endif
let targetSurfaceFrame = CGRect(origin: surfaceView.frame.origin, size: targetSize)
_ = setFrameIfNeeded(surfaceView, to: targetSurfaceFrame)
Copy link
Copy Markdown

@cursor cursor bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surface view frame set redundantly in geometry sync

Low Severity

In synchronizeGeometryAndContent, surfaceView.frame is set twice: first via setFrameIfNeeded (with the stale current origin and viewport size), then immediately overwritten by synchronizeSurfaceView() which now sets the entire frame from renderedSurfaceViewportRect. Before this PR, synchronizeSurfaceView only set the origin, so both calls were needed—one for size, one for origin. Now that synchronizeSurfaceView sets origin and size, the first setFrameIfNeeded(surfaceView, ...) is fully redundant. The double-set also means renderedSurfaceViewportRect is evaluated three times per sync pass instead of the two that are actually needed.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1df203a. Configure here.

let targetDocumentFrame = CGRect(
origin: documentView.frame.origin,
size: CGSize(width: targetSize.width, height: documentView.frame.height)
)
_ = setFrameIfNeeded(documentView, to: targetDocumentFrame)
updateNotificationRingPath()
updateFlashPath(style: lastFlashStyle)
updateFlashAppearance(style: lastFlashStyle)
Expand Down Expand Up @@ -9610,13 +9620,17 @@ final class GhosttySurfaceScrollView: NSView {
}

func attachSurface(_ terminalSurface: TerminalSurface) {
let hasUsableBounds = bounds.width > 1 && bounds.height > 1
if hasUsableBounds {
_ = synchronizeGeometryAndContent()
}
surfaceView.attachSurface(terminalSurface)
let workspace = terminalSurface.owningWorkspace()
cachedOwningWorkspace = workspace
updateWorkspaceTerminalScrollBarObserver(workspace)
// Preserve the bootstrap 800x600 surface until portal reattach churn
// has produced a real host size instead of a transient 1x1 placeholder.
guard bounds.width > 1, bounds.height > 1 else { return }
guard hasUsableBounds else { return }
_ = synchronizeGeometryAndContent()
}
Comment on lines 9622 to 9635
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Double synchronizeGeometryAndContent runs synchronizeCoreSurface on the old surface

When hasUsableBounds is true, synchronizeGeometryAndContent() fires twice — Call 1 (pre-attach, lines 9624–9626) and Call 2 (post-attach, line 9634). Call 1 reaches synchronizeCoreSurface → surfaceView.pushTargetSurfaceSize(...) while surfaceView.terminalSurface is still the previous surface (or nil), sending a redundant resize event to that old surface before it is replaced. The intent of Call 1 is only to seed surfaceView.frame so resolvedSurfaceSize returns the correct viewport during the imminent TerminalSurface.create() call. Extracting just the frame/document layout portion — or at minimum adding a comment explaining the desired pre-attach side effects of the full sync — would prevent the unintended push to the outgoing surface.


Expand Down Expand Up @@ -11356,21 +11370,51 @@ final class GhosttySurfaceScrollView: NSView {
// Intentionally no-op (no retry loops).
}

private func synchronizeSurfaceView() {
// The clip view's visible rect is the authoritative terminal viewport. All
// Ghostty/PTTY size reporting should flow through this geometry so the
// drawn cell grid and the reported cols/rows never diverge.
fileprivate func renderedSurfaceViewportRect(preferredSize: CGSize? = nil) -> CGRect {
let visibleRect = scrollView.contentView.documentVisibleRect
guard !pointApproximatelyEqual(surfaceView.frame.origin, visibleRect.origin) else { return }
if visibleRect.width > 0, visibleRect.height > 0 {
return visibleRect
}

let clipBounds = scrollView.contentView.bounds
if clipBounds.width > 0, clipBounds.height > 0 {
return CGRect(origin: clipBounds.origin, size: clipBounds.size)
}

if let preferredSize,
preferredSize.width > 0,
preferredSize.height > 0 {
return CGRect(origin: surfaceView.frame.origin, size: preferredSize)
}

if surfaceView.frame.width > 0, surfaceView.frame.height > 0 {
return surfaceView.frame
}

return CGRect(origin: .zero, size: scrollView.bounds.size)
}

private func synchronizeSurfaceView() {
let visibleRect = renderedSurfaceViewportRect(preferredSize: surfaceView.frame.size)
guard !Self.rectApproximatelyEqual(surfaceView.frame, visibleRect) else { return }
#if DEBUG
logDragGeometryChange(event: "surfaceOrigin", old: surfaceView.frame.origin, new: visibleRect.origin)
if !pointApproximatelyEqual(surfaceView.frame.origin, visibleRect.origin) {
logDragGeometryChange(event: "surfaceOrigin", old: surfaceView.frame.origin, new: visibleRect.origin)
}
#endif
surfaceView.frame.origin = visibleRect.origin
surfaceView.frame = visibleRect
}

/// Match upstream Ghostty behavior: use content area width (excluding non-content
/// regions such as scrollbar space) when telling libghostty the terminal size.
@discardableResult
private func synchronizeCoreSurface() -> Bool {
let width = max(0, surfaceView.frame.width)
let height = surfaceView.frame.height
let viewportSize = renderedSurfaceViewportRect(preferredSize: surfaceView.frame.size).size
let width = max(0, viewportSize.width)
let height = viewportSize.height
guard width > 0, height > 0 else { return false }
return surfaceView.pushTargetSurfaceSize(CGSize(width: width, height: height))
}
Expand Down
Loading