Repository navigation
Revert liveSurfaceForGhosttyAccess: fix Cmd+N @Published nil crash - #2221
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refactors how TabManager and Workspace retrieve terminal surfaces for config inheritance. It replaces guarded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ 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 reverts #1915 to fix a Cmd+N crash where Confidence Score: 3/5Safe to merge on the happy path, but two deliberate guard removals risk re-introducing separate crashes: one on Intel Macs (font pointer) and one with teardown-phase surfaces (portalLifecycleState). The primary crash (#2212, Cmd+N nil @published) is correctly fixed by removing the side-effecting quarantine logic from config-inheritance paths. The TabManager change is clean. However, the Workspace loop and rememberTerminalConfigInheritanceSource now use only pointer-nullity guards — the documented safe access pattern requires checking portalLifecycleState too. The font-pointer liveness guard removal is also an unforced regression of a previous Intel Mac fix not required to close #2212. Sources/Workspace.swift — font pointer guard at line 48 and loop guard at lines 7318/7220 need targeted fixes before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Cmd+N / New Split triggered"] --> B["inheritedTerminalConfigForNewWorkspace\n(TabManager)"]
A --> C["inheritedTerminalConfig\n(Workspace)"]
B --> D["terminalPanelForWorkspaceConfigInheritanceSource\n(already filters hasLiveSurface)"]
D --> E{"panel.surface.surface != nil?"}
E -- yes --> F["cmuxInheritedSurfaceConfig(sourceSurface)"]
E -- no --> G["fallback: lastRememberedTerminalFontPoints"]
F --> H["ghostty_surface_inherited_config ✅"]
F --> I["cmuxCurrentSurfaceFontSizePoints"]
I --> J{"cmuxSurfacePointerAppearsLive? ✅"}
J -- yes --> K["ghostty_surface_quicklook_font"]
K --> L{"cmuxPointerAppearsLive(font)?\n❌ REMOVED — Intel Mac risk"}
L -- was guarded --> M["CTFont.fromOpaque → CTFontGetSize"]
C --> N["terminalPanelConfigInheritanceCandidates\n(all panels, no lifecycle filter)"]
N --> O{"surface.surface != nil?\n⚠️ missing hasLiveSurface check"}
O -- passes --> F
O -- all fail --> P["fallback: lastTerminalConfigInheritanceFontPoints"]
Reviews (1): Last reviewed commit: "Revert "Merge pull request #1915 from el..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return nil | ||
| } | ||
|
|
||
| let ctFont = Unmanaged<CTFont>.fromOpaque(quicklookFont).takeUnretainedValue() |
There was a problem hiding this comment.
The guard cmuxPointerAppearsLive(quicklookFont) was explicitly added to fix crashes on Intel Macs where ghostty_surface_quicklook_font returned a freed CTFont pointer even while the surface itself was still live (issues #1496, #1870). The comment that was removed stated:
"This does not prove the object is still a valid CTFont, but it filters out the common fully-freed/unmapped cases that previously crashed on Intel Macs"
The surface-level cmuxSurfacePointerAppearsLive(surface) check at the top of the function remains, but that only validates the ghostty_surface_t pointer — it does not guarantee that the CTFont * returned by ghostty_surface_quicklook_font is still backed by a live allocation. If font objects can be freed or reallocated independently of their parent surface (e.g., during a font-size change mid-flight), calling Unmanaged<CTFont>.fromOpaque(quicklookFont).takeUnretainedValue() on a freed pointer will still crash.
The PR description acknowledges this risk with "Verify on Intel Mac if possible (original #1870 fix target)", but the guard removal is a deliberate regression of a targeted crash fix. Consider keeping the font pointer check, or confirming via testing on Intel hardware that ghostty_surface_quicklook_font can never return a freed pointer while the surface passes cmuxSurfacePointerAppearsLive.
| } | ||
| continue | ||
| } | ||
| guard let sourceSurface = terminalPanel.surface.surface else { continue } |
There was a problem hiding this comment.
surface.surface != nil skips the portalLifecycleState teardown check
hasLiveSurface is defined as surface != nil && portalLifecycleState == .live. The new guard only tests the pointer nullity half of that invariant; it will pass for surfaces that are mid-teardown (portalLifecycleState != .live) but still have a non-nil pointer.
The documentation in GhosttyTerminalView.swift (lines 2844–2851) explicitly warns that a surface should be validated before passing it to Ghostty C APIs like ghostty_surface_inherited_config and ghostty_surface_quicklook_font, because "a Swift wrapper around ghostty_surface_t can remain non-nil after the backing native surface has already been freed."
The original crash from #2212 was caused by liveSurfaceForGhosttyAccess mutating self.surface = nil (a @Published property) as a side-effect during config inheritance. Simply using hasLiveSurface — which is a pure read with no side-effects — would be a safer middle ground:
| guard let sourceSurface = terminalPanel.surface.surface else { continue } | |
| guard terminalPanel.surface.hasLiveSurface, | |
| let sourceSurface = terminalPanel.surface.surface else { continue } |
This avoids the @Published mutation that caused #2212 while also not admitting surfaces that are in the process of being torn down.
| if let sourceSurface = terminalPanel.surface.liveSurfaceForGhosttyAccess( | ||
| reason: "workspace.rememberConfigInheritanceSource" | ||
| ), | ||
| if let sourceSurface = terminalPanel.surface.surface, |
There was a problem hiding this comment.
hasLiveSurface gap in rememberTerminalConfigInheritanceSource
Only pointer nullity is checked here, not portalLifecycleState. A surface mid-teardown with a non-nil pointer will be used to call cmuxCurrentSurfaceFontSizePoints, updating terminalInheritanceFontPointsByPanelId and lastTerminalConfigInheritanceFontPoints with whatever the (potentially stale) surface reports.
For consistency with the inheritedTerminalConfig loop, consider:
| if let sourceSurface = terminalPanel.surface.surface, | |
| if terminalPanel.surface.hasLiveSurface, | |
| let sourceSurface = terminalPanel.surface.surface, |
…lit-crash-intel-1870" (manaflow-ai#2221) This reverts commit df80486, reversing changes made to 37c7ccd.
This branch was successfully deployed
1 participant
Summary
liveSurfaceForGhosttyAccess/cmuxPointerAppearsLiveguard) which introduced a regression causing Cmd+N to crash with a nil@Publishedvalue.surfaceaccess for config inheritance inTabManagerandWorkspace, removing the quarantine/liveness-check layer that was rejecting valid surfaces during new-tab creationcmuxPointerAppearsLivepointer guard fromcmuxCurrentSurfaceFontSizePointsand simplifies the font fallback logicFixes #2212
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Summary by cubic
Fixes a crash when pressing Cmd+N caused by a nil @published value during new-tab creation. Reverts the
liveSurfaceForGhosttyAccessquarantine and restores direct.surfaceaccess for config inheritance. Fixes #2212.liveSurfaceForGhosttyAccess/cmuxliveness guard from PR Fix #1870: prevent split crash on Intel Macs caused by stale font pointer #1915 that blocked valid surfaces..surfaceusage inTabManagerandWorkspacefor inheritance.cmuxPointerAppearsLivefromcmuxCurrentSurfaceFontSizePointsand simplify font fallback to last known size.Written for commit a47c8af. Summary will update on new commits.