Harden terminal tab re-parenting against rapid switches

[dhilgaertner]

Summary

• Issue Crash when switching between shell terminal and Claude Code tabs in a workspace #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 PR tightens the path so the race surface is gone, and adds an uncaught NSException handler so the next reproduction 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 50/100 ms DispatchQueue.main.asyncAfter first-responder closures with a single Task @MainActor guarded 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

Test plan

• make build — clean compile, no warnings.
• make test — 46/46 tests in main app suite pass on a clean re-run (tmuxBackendDefaultIsOff is a pre-existing flake from concurrent @Test writes to FeatureFlags.tmuxBackendConfigOverride; passes on retry, unrelated to this PR).
• Repro of issue Crash when switching between shell terminal and Claude Code tabs in a workspace #240 with CROW_TMUX_BACKEND=1:
  • Launch Crow.app, open a session with a Claude Code tab.
  • Click "+" to add a shell terminal tab.
  • Type something in the shell tab so focus moves there.
  • Rapidly click between Claude Code and shell tabs 20+ times.
  • App should not crash; both tabs remain interactive; focus follows the visible tab.
• Sanity: same flow without tmux backend (default .ghostty), to confirm the per-tab surface path isn't regressed.
• Sanity: verify the uncaught-exception handler fires by temporarily injecting NSException(name: .genericException, reason: "test", userInfo: nil).raise() somewhere reachable, launching, observing [CrowCrash] in Console.app, then removing the test.

🤖 Generated with Claude Code

[dgershman]

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

• The uncaught-exception handler in AppDelegate only logs to NSLog (Console.app / unified log) — no external network calls, no user data leakage.
• No new inputs, IPC surface, or permission changes — the attack surface is unchanged.

Concerns:

• None identified.

Code Quality

TerminalSurfaceView — well-structured simplification:

1. attach(surface:to:) extraction — DRYs up the 4-constraint block that was duplicated between makeNSView and updateNSView. The surface.superview !== nsView guard in updateNSView:58 prevents duplicate constraint stacking on repeated calls. Sound.

2. Dropping explicit removeFromSuperview() before addSubview — correct per AppKit semantics: addSubview atomically re-parents, so the explicit removeFromSuperview was triggering a spurious viewDidMoveToWindow(nil) → ghostty_surface_set_focus(false) round-trip on the shared tmux surface. This was likely the primary culprit for the intermittent crash on rapid tab toggling.

3. Replacing DispatchQueue.main.asyncAfter with Task { @MainActor } — the old 50/100 ms closures could stack on rapid tab switches because each closure captured surface strongly and had no staleness guard. The new Task version uses [weak surface] and validates surface.superview === nsView && surface.window === window before acting, so stale closures from prior tab switches are correctly discarded. Good fix for the race.

4. Dropping manual setFrameSize calls — autolayout edge-pinning already drives frame sizing. The manual call was redundant and racing with the layout pass. Correct removal.

5. Removing duplicate TmuxBackend.shared.makeActive(id:) from makeNSView — updateNSView fires immediately after makeNSView per Apple's NSViewRepresentable contract, so the call in makeNSView was always a double-fire.

TerminalManager — removeFromSuperview() before destroy():

Adding removeFromSuperview() in surface(for:) (nil-surface discard path), destroy(id:), and retry(id:) ensures stale views aren't left dangling in offscreenWindow.contentView.subviews after the underlying ghostty surface is freed. This is a necessary complement to the TerminalSurfaceView changes — without it, the offscreen window could hold a reference to a destroyed view.

Minor observations (non-blocking):

• The Task { @MainActor [weak surface] } in updateNSView fires on every updateNSView call, not just on re-parenting. This means makeFirstResponder is attempted even when nothing changed. It's harmless (the guards ensure correctness and makeFirstResponder on the already-first-responder is a no-op), but you could move it inside the if surface.superview !== nsView block if you wanted to reduce no-op work. Low priority — leave as-is if you want focus recovery on non-re-parenting updates.

• The NSSetUncaughtExceptionHandler closure doesn't call abort() or fatalError() after logging. This means the app continues running after the ObjC exception is logged. In practice, AppKit's default behavior re-raises the exception anyway, so the process still terminates — but if you ever want to guarantee termination after logging, you'd need an explicit abort(). Current behavior is fine for diagnostic purposes.

Summary Table

Priority	Issue
Green	makeFirstResponder Task fires on every updateNSView, not just re-parenting — harmless, consider scoping if desired
Green	Exception handler doesn't explicitly terminate — acceptable for crash diagnostics

Recommendation: Approve — the changes correctly eliminate the race condition in terminal tab re-parenting, the constraint-duplication guard is sound, and the exception handler adds useful crash diagnostics without introducing risk.

[dgershman]

Code & Security Review

Critical Issues

None found.

Security Review

Strengths:

• The uncaught exception handler in AppDelegate only logs to NSLog (Console.app / unified log), which is an appropriate destination — no sensitive data is sent externally.
• No new external inputs, network calls, or untrusted data processing introduced.
• removeFromSuperview() before destroy() in TerminalManager is a defensive pattern that prevents use-after-free scenarios with the underlying ghostty surface.

Concerns:

• None. The changes are purely internal AppKit view-lifecycle hardening.

Code Quality

Positive:

• Extracting attach(surface:to:) eliminates constraint-setup duplication between makeNSView and updateNSView — good DRY improvement.
• Replacing DispatchQueue.main.asyncAfter (50ms/100ms) with Task { @MainActor } guarded by staleness checks (surface.superview === nsView, surface.window === window) is a well-reasoned fix for the stale-closure stacking hypothesis. The guard conditions ensure that if the surface was re-parented between scheduling and execution, the task becomes a no-op.
• Dropping the redundant TmuxBackend.shared.makeActive(id:) from makeNSView (since updateNSView fires immediately after per Apple's contract) removes a double-fire of tmux select-window.
• removeFromSuperview() before destroy() in all three TerminalManager paths prevents a dangling subview reference in the offscreen window.

Minor observation (not blocking):

• attach(surface:to:) is called from updateNSView when surface.superview !== nsView, meaning the surface was in a different container. While addSubview atomically re-parents, the old container (now empty) retains orphaned constraints targeting its own anchors. These are harmless (deactivated when the surface leaves) but could be explicitly removed for hygiene. This is a nit — not actionable since autolayout handles it correctly.
• The Task { @MainActor } in updateNSView fires on every updateNSView call, not just re-parenting. In practice this means redundant makeFirstResponder calls when SwiftUI re-evaluates the view body without a tab switch. The guards prevent actual damage, but if profiling later shows excessive responder-chain churn, consider moving the Task inside the if surface.superview !== nsView block. Low priority.

Summary Table

Priority	Issue
Green	Consider gating the first-responder Task on the re-parent condition to avoid redundant calls
Green	Orphaned constraints on old container are harmless but could be cleaned up

Recommendation: Approve — the changes correctly eliminate the race surface identified in #240 (stale asyncAfter closures stacking on a shared surface during rapid tab switches), add proper cleanup ordering in TerminalManager, and install diagnostic logging for future crash investigations. The approach is sound and the net effect is a reduction in code complexity.