From 3f414df30775be4225a3ab52ba7761356ab0767e Mon Sep 17 00:00:00 2001 From: Dustin Hilgaertner Date: Mon, 4 May 2026 17:42:31 -0500 Subject: [PATCH 1/2] Harden terminal tab re-parenting against rapid switches (#240) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #240 reports an intermittent app crash when switching between a Claude Code tab and a shell tab with `CROW_TMUX_BACKEND=1` enabled. No stack trace was captured. In tmux mode every visible tab re-parents the same shared `GhosttySurfaceView` into its own SwiftUI container, and the old re-parenting path stacked deferred closures against that shared surface on each switch — the most plausible culprit for an intermittent crash on rapid tab toggling. This change tightens the path so the race surface is gone, and adds an uncaught NSException handler so the next reproduction (here or elsewhere) lands a usable stack trace in Console.app instead of an abort. `TerminalSurfaceView`: - Drop the explicit `removeFromSuperview()` before `addSubview` — `addSubview` re-parents atomically; the explicit call only added a spurious `viewDidMoveToWindow(nil)` and `set_focus(false)` per switch. - Drop the deferred `setFrameSize` calls in both `makeNSView` and `updateNSView`. Autolayout already drives `setFrameSize` via the edge constraints, so the manual call was redundant and racing against autolayout. - Replace the `DispatchQueue.main.asyncAfter(deadline: .now() + 0.05/0.1)` first-responder closures with a single `Task @MainActor` that guards on `nsView.window != nil && surface.superview === nsView` — so stale closures from prior switches no longer steal focus from the currently-visible tab. - Drop the duplicate `TmuxBackend.shared.makeActive(id:)` call from `makeNSView` — `updateNSView` fires immediately after make per Apple's NSViewRepresentable contract. - Extract `attach(surface:to:)` so make and update share one constraint setup and can't drift. `TerminalManager`: - `surface(for:)`, `destroy(id:)`, and `retry(id:)` now call `removeFromSuperview()` before `destroy()` so a stale view isn't left dangling in `offscreenWindow.contentView.subviews` after its underlying ghostty surface is freed. `AppDelegate`: - Install `NSSetUncaughtExceptionHandler` in `applicationDidFinishLaunching` to NSLog name + reason + `callStackSymbols` for any uncaught NSException. Mach exceptions (SIGSEGV) still go to the system crash reporter; this just covers the ObjC-exception gap that currently lets AppKit / libghostty wrappers abort silently. Closes #240 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../CrowTerminal/TerminalManager.swift | 11 ++- .../CrowTerminal/TerminalSurfaceView.swift | 82 ++++++++++--------- Sources/Crow/App/AppDelegate.swift | 19 +++++ 3 files changed, 70 insertions(+), 42 deletions(-) diff --git a/Packages/CrowTerminal/Sources/CrowTerminal/TerminalManager.swift b/Packages/CrowTerminal/Sources/CrowTerminal/TerminalManager.swift index 6a423df..b1f543f 100644 --- a/Packages/CrowTerminal/Sources/CrowTerminal/TerminalManager.swift +++ b/Packages/CrowTerminal/Sources/CrowTerminal/TerminalManager.swift @@ -82,6 +82,7 @@ public final class TerminalManager { return existing } NSLog("[TerminalManager] surface(for: \(id)) — cached view has nil surface, discarding") + existing.removeFromSuperview() existing.destroy() surfaces.removeValue(forKey: id) } @@ -102,14 +103,20 @@ public final class TerminalManager { public func existingSurface(for id: UUID) -> GhosttySurfaceView? { surfaces[id] } public func destroy(id: UUID) { - if let view = surfaces.removeValue(forKey: id) { view.destroy() } + if let view = surfaces.removeValue(forKey: id) { + view.removeFromSuperview() + view.destroy() + } } /// Discard the cached view for `id` (if any) and re-run preInitialize. /// Used by the UI's "Retry" affordance after a permanent surface-creation failure. public func retry(id: UUID, workingDirectory: String, command: String? = nil) { NSLog("[TerminalManager] retry(\(id)) — destroying broken surface and re-preInitializing") - if let view = surfaces.removeValue(forKey: id) { view.destroy() } + if let view = surfaces.removeValue(forKey: id) { + view.removeFromSuperview() + view.destroy() + } // Re-arm readiness tracking; surfaceDidFail removed the id from the set. monitoredTerminals.insert(id) preInitialize(id: id, workingDirectory: workingDirectory, command: command) diff --git a/Packages/CrowTerminal/Sources/CrowTerminal/TerminalSurfaceView.swift b/Packages/CrowTerminal/Sources/CrowTerminal/TerminalSurfaceView.swift index 02e18b2..3ba470f 100644 --- a/Packages/CrowTerminal/Sources/CrowTerminal/TerminalSurfaceView.swift +++ b/Packages/CrowTerminal/Sources/CrowTerminal/TerminalSurfaceView.swift @@ -36,33 +36,17 @@ public struct TerminalSurfaceView: NSViewRepresentable { public func makeNSView(context: Context) -> NSView { let surface = surfaceForBackend() let container = NSView() - container.addSubview(surface) - surface.translatesAutoresizingMaskIntoConstraints = false - NSLayoutConstraint.activate([ - surface.leadingAnchor.constraint(equalTo: container.leadingAnchor), - surface.trailingAnchor.constraint(equalTo: container.trailingAnchor), - surface.topAnchor.constraint(equalTo: container.topAnchor), - surface.bottomAnchor.constraint(equalTo: container.bottomAnchor), - ]) - // Resize after layout settles - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { - let size = container.bounds.size - if size.width > 0 && size.height > 0 { - surface.setFrameSize(size) - } - surface.window?.makeFirstResponder(surface) - } - // For tmux backends, switch to this terminal's window now that it's - // about to be visible. - if backend == .tmux { - try? TmuxBackend.shared.makeActive(id: terminalID) - } + attach(surface: surface, to: container) + // makeActive is fired from updateNSView — issuing it here too can + // double-fire `tmux select-window` on the same tab activation when + // SwiftUI calls update right after make on .id() recreation. return container } - /// Re-parent the surface if SwiftUI replaced the container, and resize. - /// For tmux backends, also fire makeActive — this is the "tab switched - /// to a different tmux terminal" hook in the shared-surface model. + /// Re-parent the surface if SwiftUI replaced the container, and acquire + /// first responder once the view is in a window. For tmux backends, also + /// fire makeActive — this is the "tab switched to a different tmux + /// terminal" hook in the shared-surface model. @MainActor public func updateNSView(_ nsView: NSView, context: Context) { guard let surface = existingSurfaceForBackend() else { return } @@ -72,23 +56,41 @@ public struct TerminalSurfaceView: NSViewRepresentable { } if surface.superview !== nsView { - surface.removeFromSuperview() - nsView.addSubview(surface) - surface.translatesAutoresizingMaskIntoConstraints = false - NSLayoutConstraint.activate([ - surface.leadingAnchor.constraint(equalTo: nsView.leadingAnchor), - surface.trailingAnchor.constraint(equalTo: nsView.trailingAnchor), - surface.topAnchor.constraint(equalTo: nsView.topAnchor), - surface.bottomAnchor.constraint(equalTo: nsView.bottomAnchor), - ]) - DispatchQueue.main.asyncAfter(deadline: .now() + 0.05) { - let size = nsView.bounds.size - if size.width > 0 && size.height > 0 { - surface.setFrameSize(size) - } - surface.window?.makeFirstResponder(surface) - } + // addSubview re-parents atomically — no need for an explicit + // removeFromSuperview, which would trigger an extra + // viewDidMoveToWindow(nil) round-trip and a redundant + // ghostty_surface_set_focus(false) on the shared surface. + attach(surface: surface, to: nsView) } + + // Acquire first responder after AppKit has had a chance to add the + // container to the window hierarchy. Scheduling via Task @MainActor + // hops to the next main-actor execution without an arbitrary delay, + // so rapid tab switches don't stack stale closures against the + // (shared, in tmux mode) surface. + Task { @MainActor [weak surface] in + guard let surface, + let window = nsView.window, + surface.superview === nsView, + surface.window === window else { return } + window.makeFirstResponder(surface) + } + } + + /// Re-parent `surface` into `container` and pin to its edges. Idempotent + /// constraint setup: relies on `addSubview` to atomically re-parent and + /// on autolayout to drive subsequent `setFrameSize` calls — manual + /// `setFrameSize` here races with autolayout and is unnecessary. + @MainActor + private func attach(surface: GhosttySurfaceView, to container: NSView) { + container.addSubview(surface) + surface.translatesAutoresizingMaskIntoConstraints = false + NSLayoutConstraint.activate([ + surface.leadingAnchor.constraint(equalTo: container.leadingAnchor), + surface.trailingAnchor.constraint(equalTo: container.trailingAnchor), + surface.topAnchor.constraint(equalTo: container.topAnchor), + surface.bottomAnchor.constraint(equalTo: container.bottomAnchor), + ]) } @MainActor diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 1988fa2..09636cc 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -26,6 +26,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate { private var appConfig: AppConfig? func applicationDidFinishLaunching(_ notification: Notification) { + installUncaughtExceptionHandler() + // Check for devRoot pointer if let root = ConfigStore.loadDevRoot() { devRoot = root @@ -35,6 +37,23 @@ final class AppDelegate: NSObject, NSApplicationDelegate { } } + /// Capture ObjC exceptions that would otherwise tear the app down + /// silently. The macOS crash reporter handles Mach exceptions / pure + /// SIGSEGV on its own, but ObjC exceptions thrown out of AppKit or + /// libghostty wrappers can `abort()` without producing a useful .ips + /// file. Logging name + reason + symbolicated stack to NSLog routes + /// them into Console.app and the unified log so the next reproduction + /// of issue #240 (and similar) is debuggable. + private func installUncaughtExceptionHandler() { + NSSetUncaughtExceptionHandler { exception in + let symbols = exception.callStackSymbols.joined(separator: "\n") + NSLog( + "[CrowCrash] uncaught NSException name=\(exception.name.rawValue) " + + "reason=\(exception.reason ?? "")\n\(symbols)" + ) + } + } + // MARK: - tmux watchdog alert /// Suppress repeated alerts while one is already on screen. Each alert From b7abe82bcabce1a3757cf8797f9b37038f31de2b Mon Sep 17 00:00:00 2001 From: Dustin Hilgaertner Date: Mon, 4 May 2026 18:56:08 -0500 Subject: [PATCH 2/2] Serialize FeatureFlags suite to fix CI flake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tmuxBackendDefaultIsOff` and `tmuxBackendOnWhenConfigOverrideOn` both mutate `FeatureFlags.tmuxBackendConfigOverride` (a `nonisolated(unsafe) static var`). Swift Testing runs `@Test` functions in parallel by default, so the two interleave and one test's `= false` reset can land between the other's `= true` set and its read of `FeatureFlags.tmuxBackend`. The flake hit this PR's CI run as a `tmuxBackendOnWhenConfigOverrideOn` failure ("FeatureFlags.tmuxBackend → false"), and previously appeared locally as `tmuxBackendDefaultIsOff`. Adding `.serialized` to the suite forces those two writers to run sequentially while leaving the rest of the test run parallel. Co-Authored-By: Claude Opus 4.7 (1M context) --- Tests/CrowTests/FeatureFlagsTests.swift | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Tests/CrowTests/FeatureFlagsTests.swift b/Tests/CrowTests/FeatureFlagsTests.swift index 028f14b..9f58273 100644 --- a/Tests/CrowTests/FeatureFlagsTests.swift +++ b/Tests/CrowTests/FeatureFlagsTests.swift @@ -6,7 +6,13 @@ import Testing /// flip the live environment from tests, so this exercises the same shape /// directly via a free function. The intent is to lock in which spellings /// of "true" we accept so the rollout's flag flips are predictable. -@Suite("FeatureFlags env parsing") +/// +/// `.serialized` because `tmuxBackendDefaultIsOff` and +/// `tmuxBackendOnWhenConfigOverrideOn` both mutate the global static +/// `FeatureFlags.tmuxBackendConfigOverride`. Without serialization the two +/// tests race on that shared state and the suite fails intermittently +/// (one test's `= false` reset lands between the other's set + read). +@Suite("FeatureFlags env parsing", .serialized) struct FeatureFlagsTests { /// Mirror of the parsing rule in `FeatureFlags.boolFlag`. If the