Fix macOS Ghostty display scale sync#150
Conversation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/con-app/src/ghostty_view.rs (1)
1724-1740:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftInvalidate the backing observer in
Dropbefore theArc<GhosttyTerminal>is freed.The current
Dropimplementation only nils the NSView fields but does not remove the backing observer. Sinceterminal(line 124) is declared beforensview(line 133+), it drops first—freeing the Ghostty surface while the observer still holds a raw_surfacepointer. The next screen/backing notification will call into freed memory.While all explicit code paths (window close, tab close,
prepare_window_close) callshutdown_surface()which removes the observer viadetach_host_view(), theDropimplementation serves as the safety net for implicit teardown (panic unwind, GPUI entity cleanup without explicit shutdown). It should invalidate the observer before the Arc releases.Suggested fix
impl Drop for GhosttyView { fn drop(&mut self) { #[cfg(target_os = "macos")] { if let Some(underlay_view) = self.native_underlay_view.take() { Self::set_transition_underlay_owner_visible( underlay_view, self.native_transition_underlay_owner_id, false, ); } + if let Some(nsview) = self.nsview { + unsafe { + con_ghostty_surface_remove_backing_observer(nsview as *mut c_void); + } + } self.document_view = None; self.nsview = None; } self.host_view = None; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/con-app/src/ghostty_view.rs` around lines 1724 - 1740, Drop currently clears NSView/host_view after fields that own the Arc<GhosttyTerminal> are dropped, leaving the backing observer attached while the surface is freed; update GhosttyView::drop to explicitly invalidate/detach the backing observer (call the existing detach_host_view() or the same logic that removes the observer/shutdown_surface()) before the Arc<GhosttyTerminal> (terminal) is dropped—use take() on the relevant fields (e.g., terminal, host_view, nsview, native_underlay_view) in the correct order so detach_host_view() runs first, then clear the remaining fields (document_view, nsview, host_view) to ensure the observer cannot call into freed memory.
🧹 Nitpick comments (1)
crates/con-app/src/ghostty_view.rs (1)
971-987: 💤 Low valueRedundant install + sync: each
sync_native_backing_propertiestriggers two backing syncs.
con_ghostty_surface_install_backing_observerre-runsinstallForView:in the trampoline; when the observer is already attached for the same(window, surface)it falls through to[self sync], which callscon_ghostty_surface_sync_backingitself. The very next line then callscon_ghostty_surface_sync_backingagain. That doubles the AppKitconvertSizeToBacking/convertRectToBackinground-trips and theghostty_surface_set_*calls on every frame update path (including live resize and cross-display moves).The observer is already installed once in
ensure_initialized(lines 677–682); a redundant defensive install here isn't necessary on the layout hot path.♻️ Proposed refactor: drop the per-layout install
let ok = unsafe { - con_ghostty_surface_install_backing_observer( - nsview as *mut c_void, - terminal.raw_surface() as *mut c_void, - ); con_ghostty_surface_sync_backing( nsview as *mut c_void, terminal.raw_surface() as *mut c_void, f64::from(bounds.size.width.as_f32().max(1.0)), f64::from(bounds.size.height.as_f32().max(1.0)), fallback_scale, &mut scale_x, &mut scale_y, &mut width_px, &mut height_px, ) };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/con-app/src/ghostty_view.rs` around lines 971 - 987, The code calls con_ghostty_surface_install_backing_observer immediately before con_ghostty_surface_sync_backing inside sync_native_backing_properties, causing a redundant install that triggers an extra sync; remove the per-layout install call so the observer is only installed once in ensure_initialized (which already runs con_ghostty_surface_install_backing_observer for the (window, surface) pair), leaving only the con_ghostty_surface_sync_backing invocation in sync_native_backing_properties to avoid duplicate convertSizeToBacking/convertRectToBacking and duplicate ghostty_surface_set_* calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/con-app/src/ghostty_view.rs`:
- Around line 1724-1740: Drop currently clears NSView/host_view after fields
that own the Arc<GhosttyTerminal> are dropped, leaving the backing observer
attached while the surface is freed; update GhosttyView::drop to explicitly
invalidate/detach the backing observer (call the existing detach_host_view() or
the same logic that removes the observer/shutdown_surface()) before the
Arc<GhosttyTerminal> (terminal) is dropped—use take() on the relevant fields
(e.g., terminal, host_view, nsview, native_underlay_view) in the correct order
so detach_host_view() runs first, then clear the remaining fields
(document_view, nsview, host_view) to ensure the observer cannot call into freed
memory.
---
Nitpick comments:
In `@crates/con-app/src/ghostty_view.rs`:
- Around line 971-987: The code calls
con_ghostty_surface_install_backing_observer immediately before
con_ghostty_surface_sync_backing inside sync_native_backing_properties, causing
a redundant install that triggers an extra sync; remove the per-layout install
call so the observer is only installed once in ensure_initialized (which already
runs con_ghostty_surface_install_backing_observer for the (window, surface)
pair), leaving only the con_ghostty_surface_sync_backing invocation in
sync_native_backing_properties to avoid duplicate
convertSizeToBacking/convertRectToBacking and duplicate ghostty_surface_set_*
calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77c2d4b7-685b-479e-bed5-b90ab5df83f9
📒 Files selected for processing (4)
crates/con-app/build.rscrates/con-app/src/ghostty_view.rscrates/con-app/src/objc/ghostty_surface_trampoline.mpostmortem/2026-05-06-macos-display-scale-sync.md
|
@codex review |
💡 Codex Reviewcon-terminal/crates/con-app/src/objc/ghostty_surface_trampoline.m Lines 66 to 68 in c48a4cf
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
c48a4cf to
54ae594
Compare
|
@codex review |
54ae594 to
30f5fc3
Compare
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Thanks a lot for the great work! |
Summary
Validation
Summary by CodeRabbit
Bug Fixes