Fix terminal viewport width reporting in cmux#2926
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidated Ghostty surface sizing logic by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR makes Confidence Score: 5/5Safe to merge; remaining findings are P2 style/cleanup suggestions with no correctness impact. No P0 or P1 issues found. The viewport-as-source-of-truth design is sound and all fallback paths in renderedSurfaceViewportRect handle zero-size and bootstrap cases correctly. The two P2 notes (double sync call touching the old surface, redundant intermediate frame assignment) are cleanups and do not affect runtime behavior. No files require special attention — both findings are in synchronizeGeometryAndContent / attachSurface within Sources/GhosttyTerminalView.swift and are style-level only. Important Files Changed
Sequence DiagramsequenceDiagram
participant Host as GhosttySurfaceScrollView
participant NSV as GhosttyNSView (surfaceView)
participant Ghostty as libghostty
Note over Host: attachSurface() called
alt hasUsableBounds (pre-attach seed)
Host->>Host: synchronizeGeometryAndContent() [Call 1]
Host->>Host: scrollView.layoutSubtreeIfNeeded()
Host->>Host: renderedSurfaceViewportRect() → documentVisibleRect
Host->>NSV: setFrame(origin: old, size: viewportSize)
Host->>Host: synchronizeSurfaceView() → surfaceView.frame = visibleRect
Host->>Host: synchronizeCoreSurface() → pushTargetSurfaceSize (old surface)
end
Host->>NSV: attachSurface(terminalSurface)
NSV->>NSV: resolvedSurfaceSize(preferred: bounds.size)
NSV->>Host: renderedSurfaceViewportRect(preferredSize: bounds.size)
Host-->>NSV: viewportSize (from documentVisibleRect)
NSV->>Ghostty: ghostty_surface_set_size(wpx, hpx)
alt hasUsableBounds (post-attach sync)
Host->>Host: synchronizeGeometryAndContent() [Call 2]
Host->>Host: renderedSurfaceViewportRect() → documentVisibleRect
Host->>Host: synchronizeCoreSurface() → pushTargetSurfaceSize (new surface)
Host->>Ghostty: ghostty_surface_set_size (correct viewport)
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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() | ||
| } |
There was a problem hiding this comment.
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1df203a. Configure here.
| logLayoutDuringActiveDrag(targetSize: targetSize) | ||
| #endif | ||
| let targetSurfaceFrame = CGRect(origin: surfaceView.frame.origin, size: targetSize) | ||
| _ = setFrameIfNeeded(surfaceView, to: targetSurfaceFrame) |
There was a problem hiding this comment.
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)
Reviewed by Cursor Bugbot for commit 1df203a. Configure here.
1 participant
Summary
Testing
Fixes #2925
Note
Medium Risk
Touches core terminal sizing/layout paths that affect PTY cols/rows and rendering during resize/scroll; bugs here could cause visual glitches or incorrect terminal dimensions, but no security- or data-sensitive logic is involved.
Overview
Fixes terminal size/viewport reporting by making the scroll host’s rendered viewport rect the single source of truth for Ghostty surface geometry.
Surface creation, host layout sync (
synchronizeGeometryAndContent), and core size pushes (synchronizeCoreSurface) now derive width/height fromrenderedSurfaceViewportRect, and surface attachment pre-seeds geometry when bounds are usable to avoid reusing the bootstrap size.Reviewed by Cursor Bugbot for commit 1df203a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes horizontal cutoff by using the scroll view’s visible rect as the terminal viewport for surface sizing and PTY geometry. Reported columns now match the drawn grid across create, resize, and sync; fixes #2925.
Written for commit 1df203a. Summary will update on new commits.
Summary by CodeRabbit