Skip to content

fix + reduce: focus divergence, portal retry drift, delete SurfacePool#108

Closed
arzafran wants to merge 17 commits into
mainfrom
nr/gv
Closed

fix + reduce: focus divergence, portal retry drift, delete SurfacePool#108
arzafran wants to merge 17 commits into
mainfrom
nr/gv

Conversation

@arzafran

@arzafran arzafran commented Jul 9, 2026

Copy link
Copy Markdown
Member

What this does

Fixes two real focus/portal bugs and deletes 264 lines of permanently disabled machinery. Partial start on the GhosttyTerminalView split.

Summary

  • SurfacePool.swift deleted with its pbxproj entries and 3 call sites collapsed to the only path that ever ran (verified the enabling flag has no writer anywhere)
  • focus bug: clearSuppressReparentFocus set desiredFocusState then setFocus's dedup guard skipped the real ghostty_surface_set_focus push; each branch now has exactly one writer per transition
  • synchronizeHostedView's tripled hide/retry block extracted once, with the production recovery fallback all three call sites now share
  • forceRefresh, hitTest, and keyDown untouched (verified against origin/main)

#97's larger split is partial; the issue stays open.

Fixes #85. Fixes #86. Fixes #89. Refs #97.

Test Plan

  • build + build-for-testing green locally
  • CI green

arzafran added 14 commits July 9, 2026 13:48
SurfacePool was fully wired but permanently disabled behind
programaSurfacePoolEnabled, a UserDefaults flag with no writer
anywhere in the repo (verified via repo-wide grep including
tests_v2/, CLI/, daemon/). The claim()/warmIfNeeded() calls
always no-op, so this collapses each call site to the only
path that ever ran:

- TabManager.swift: drop the canUsePool/claim branch, call
  makeWorkspaceForCreation directly (the only reachable path).
- AppDelegate.swift: drop the teardownAll() call at app
  termination.
- GhosttyTerminalView.swift: drop the dead pre-warm
  DispatchQueue.main.async block after app init.

Also removes the pbxproj entries for the deleted file.

Refs #89.
Move TabItemView and its exclusive helper-view cluster
(SidebarWorkspaceDescriptionText, SidebarMarkdownRenderer,
SidebarMetadataRows/EntryRow/MarkdownBlocks/BlockRow) out of
ContentView.swift into a new file, verbatim.

Access-level widening: TabItemView was file-private (private struct);
widened to internal (dropped the private modifier) because
VerticalTabsSidebar, which stays in ContentView.swift, constructs it
at its ForEach call site. No other behavior change. Equatable
conformance, precomputed let parameters, and the .equatable() call
site are untouched.

Refs #94.
…move)

Move the FileDropOverlayView NSView class out of ContentView.swift
into its own file, verbatim (no access-level changes needed — it was
already internal).

Refs #94.
…anical move)

Move the command-palette fuzzy-search engine (switcher search
metadata/indexer, fuzzy matcher, search corpus entry/result types,
search engine) out of ContentView.swift into its own file, verbatim.
Zero coupling to ContentView state.

Refs #94.
TerminalSurface.desiredFocusState and GhosttyNSView.desiredFocus both
track intended focus, and could diverge silently. The concrete bug:
clearSuppressReparentFocus() called recordExternalFocusState(true)
unconditionally, then (on the immediate-apply branch) called
reassertTerminalSurfaceFocus() -> setFocus(true). setFocus's dedup
guard (focused != desiredFocusState) saw no change, because
recordExternalFocusState had already set desiredFocusState = true
moments earlier, so the real ghostty_surface_set_focus push was
silently skipped. A surface could end up looking focused in the UI
(bonsplit selection, desiredFocus true) while the Ghostty renderer
never actually received focus, after a programmatic split/reparent.

Fix: recordExternalFocusState(true) is now only called on the
branches that don't reach reassertTerminalSurfaceFocus (deferred:
window not key, or hidden/tiny geometry). The immediate-apply branch
lets setFocus own desiredFocusState as its sole writer, so the dedup
guard sees the real transition and pushes ghostty_surface_set_focus.
Each branch now has exactly one writer of desiredFocusState for its
transition. No change to forceRefresh or keyDown paths.

Refs #86.
…eHostedView

WindowTerminalPortal.synchronizeHostedView tripled a near-identical
hide-and-schedule-transient-recovery block across the missingAnchorOrWindow,
anchorWindowMismatch, and hostBoundsNotReady guards. Only the
hostBoundsNotReady copy had the production-build fallback (calling
scheduleDeferredFullSynchronizeAll() when Self.transientRecoveryEnabled is
false) — the other two copies would silently drop the hide with no
follow-up resync scheduled in that configuration, and a genuinely-transient
condition could leave a hosted view stuck hidden.

Extracted hideHostedViewSchedulingRecovery(hostedId:entry:hostedView:reason:)
with the fallback included, and call it from all three sites. Behavior is
now consistent across all three guards; each caller's own distinct
diagnostic dlog (anchorWindowMismatch's anchorWindow=, hostBoundsNotReady's
portal.sync.defer) is kept as a separate pre-log alongside the shared
generic 'portal.hidden value=1 reason=X' log the helper emits, so a couple
of call sites may emit one additional DEBUG-only log line per hide compared
to before; no functional/production behavior change beyond fixing the
missing fallback for the two previously-unprotected reasons.

Refs #85.
Mechanical eviction of ~2,950 zero-dependency-on-AppDelegate free-standing
types into their own files:

- MainWindowHostingView.swift: MainWindowHostingView (SwiftUI hosting view)
- TypingProfiler.swift: ProgramaTypingTiming, ProgramaMainRunLoopStallMonitor,
  ProgramaMainThreadTurnProfiler (all #if DEBUG)
- TerminalDirectoryOpener.swift: FinderServicePathResolver,
  TerminalDirectoryOpenTarget
- VSCodeIntegration.swift: VSCodeServeWebURLBuilder,
  VSCodeCLILaunchConfiguration(Builder), VSCodeServeWebController,
  ServeWebOutputCollector, ServeWebPortStore
- WorkspaceShortcutMapper.swift: WorkspaceShortcutMapper
- CLIInstaller.swift: ProgramaCLIPathInstaller
- MenuBarIconRenderer.swift: MenuBarExtraController, notification menu
  snapshot/formatting types, MenuBarIconRenderer and its settings
- WindowSwizzles.swift: the NSWindow/NSApplication method-swizzle
  implementations and their supporting file-scoped state

Access-level widening required for cross-file visibility (no other code
changes):
- AppDelegate.recordTypingActivity(): fileprivate -> internal. Called from
  NSWindow.programa_sendEvent(_:), now in WindowSwizzles.swift.
- extension NSWindow / extension NSApplication (swizzle methods): private
  -> internal. AppDelegate.installWindowResponderSwizzles() references
  programa_makeFirstResponder/programa_sendEvent/programa_performKeyEquivalent/
  programa_applicationSendEvent via #selector(...) from AppDelegate.swift.
- programaFirstResponderGuardCurrentEventOverride/HitViewOverride: private
  -> internal. AppDelegate.setWindowFirstResponderGuardTesting(...) and
  .clearWindowFirstResponderGuardTesting() write these directly.

private extension AppDelegate (handleThemesReloadNotification) stays in
AppDelegate.swift since it extends AppDelegate itself, not a zero-dependency
type.

Refs #95.
handleCustomShortcut(event:) was a single 966-line function with ~55
sequential if-checks. Documents the discovered precedence in a comment
(palette > browser/terminal pre-checks > app-shortcut) and extracts the
lowest-precedence phase -- the flat table of ~55 matchConfiguredShortcut /
numberedConfiguredShortcutDigit / matchConfiguredDirectionalShortcut /
matchTabShortcut checks -- into handleConfiguredAppShortcutActions(event:
commandPaletteTargetWindow:hasFocusedAddressBarInShortcutContext:).

This is pure tail-call code motion: the extracted function is byte-identical
to the original tail of handleCustomShortcut (same statements, same order,
same early returns), so evaluation order and behavior are unchanged. The
higher-precedence palette and browser/terminal pre-check phases are left
grouped in place per the conservative fallback (group into sub-functions
without changing evaluation order) rather than risking a full reorder of a
typing-latency-sensitive, tightly state-coupled dispatch chain.

Refs #95.
…moves

TabItemView.swift, FileDropOverlayView.swift, and CommandPaletteSearchEngine.swift
now live in their own files, so file-private symbols they depend on (or that
depend on them) that remain in ContentView.swift need at least internal
visibility:

- SidebarDragAutoScrollController, SidebarTabItemSettingsSnapshot,
  SidebarTabDragPayload, BonsplitTabDragPayload, SidebarBonsplitTabDropDelegate,
  SidebarTabDropDelegate, MiddleClickCapture, MiddleClickCaptureView — used by
  the extracted TabItemView.
- coloredCircleImage, debugCommandPaletteTextPreview, the Color(hex:)
  initializer — top-level helpers used by the extracted TabItemView.

FileDropOverlayView.swift needs `import Bonsplit` for dlog() (per CLAUDE.md's
dlog-needs-bonsplit-import note).

No behavior changes — access-level widening only.

Refs #94.
…nalView.swift

GhosttyTerminalView.swift (9,490 lines) hosts 6+ subsystems; apply the
existing +Extension convention (+Keyboard, +Mouse, +DragDrop,
+Accessibility) to two more, clearly self-contained ones:

- GhosttyTerminalView+IME.swift: the GhosttyNSView: NSTextInputClient
  conformance (marked-text/composition state, committed-text delivery,
  character-coordinate queries for system input methods). Moved verbatim.
- GhosttyTerminalView+SwiftUIWrapper.swift: the GhosttyTerminalView
  NSViewRepresentable struct, its Coordinator, and the private
  HostContainerView it manages — the SwiftUI/AppKit bridge. Moved verbatim.

Both are pure file moves; the only code change is widening
GhosttyNSView.imePointOverrideForTesting from private to private(set) so
the moved IME extension (a #if DEBUG testing hook, firstRect override) can
still read it — it is written only inside GhosttyNSView's own declaration,
which stays in GhosttyTerminalView.swift, so private(set) preserves the
single-writer invariant.

GhosttyTerminalView.swift drops from 9,490 to 8,592 lines. Registered both
new files in project.pbxproj (NRGV0001-NRGV0004).

Refs #97.
…onary

Replace 7 parallel [UUID: _] dictionaries (visibility, pendingOpen,
recentRequestAt, escapeSuppression set, escapeSuppressionStartedAt,
selection, snapshot) with a single CommandPaletteWindowState struct stored
in one commandPaletteStateByWindowId: [UUID: CommandPaletteWindowState]
dictionary.

Adds three small helpers (commandPaletteState(for:),
updateCommandPaletteState(for:_:), teardownCommandPaletteState(for:)) so the
two previously-duplicated 7-line teardown blocks (window context discard,
window close) collapse to a single teardownCommandPaletteState(for:) call
each. Every read/write site is a mechanical per-field translation with no
behavior change (removeValue(forKey:) on a field-specific dictionary maps to
setting that field back to its default; Set.contains maps to an Optional
Bool comparison, etc).

Refs #95.
…ls.swift

Consolidate the drag-and-drop domain (DragOverlayRoutingPolicy, drag payload
enums, drop delegates, drop edge/indicator/planner, auto-scroll planner, drag
lifecycle/failsafe types) and the sidebar visual chrome (footer/help-menu/
scrim/blur/material/tint/preset enums, visual-effect/backdrop views, NSColor
extension), both previously fragmented across several non-contiguous locations
in ContentView.swift, into two new files. Done together in one pass since both
extractions needed the same verification cycle.

Access-level widening (dropped `private`, no other behavior change):
- SidebarDragFailsafeMonitor, SidebarExternalDropOverlay — constructed from
  VerticalTabsSidebar / ContentView, which stay in ContentView.swift.
- SidebarFooter, SidebarTopScrim, SidebarScrollViewResolver,
  SidebarScrollViewResolverView, SidebarEmptyArea, ClearScrollBackground,
  DraggableFolderIcon, TitlebarLeadingInsetReader, SidebarTrailingBorder,
  SidebarBackdrop — same reason.

The fileDropOverlayKey/commandPaletteWindowOverlayKey/
tmuxWorkspacePaneWindowOverlayKey associated-object keys and their overlay
container identifiers stay in ContentView.swift (they back
WindowCommandPaletteOverlayController / WindowTmuxWorkspacePaneOverlayController,
not the drag-drop domain) — they only transited SidebarDragDrop.swift briefly
during the mechanical move and were moved back.

Refs #94.
…ttySurfaceScrollView

GhosttySurfaceScrollView (~2,780 lines) is one of the god-view subsystems
called out by Nuclear Review #97. Extract its debug-only render/frame
introspection (debugRenderStats, debugCopyIOSurfaceCGImage,
debugSampleIOSurface + their DebugRenderStats/DebugFrameSample structs;
entirely #if DEBUG) into GhosttyTerminalView+RenderStats.swift as a
same-type extension, matching the existing +Extension convention.

Pure move; three access-level widenings were required for cross-file
visibility from the moved extension (each noted inline at the
declaration):
- GhosttySurfaceScrollView.surfaceView: private let -> internal let
  (immutable, so this only grants read access).
- GhosttySurfaceScrollView.isActive: private var -> private(set) var
  (write stays confined to the class's own declaration in
  GhosttyTerminalView.swift; the moved extension only reads it).
- GhosttySurfaceScrollView.contentsKey(for:) and .updatePresentStats(...):
  private static func -> internal static func (called, not just read,
  from the moved extension).

Refs #97.
…tate ownership

Move all 34 command-palette-only @State properties plus the 2 @AppStorage
settings (commandPaletteQuery, commandPaletteMode, search corpus/results,
rename/workspace-description drafts, focus-restore targets, usage history,
etc.) off ContentView and onto a new CommandPaletteController: ObservableObject,
as @Published/@AppStorage properties. ContentView keeps exactly one
`@StateObject private var commandPaletteController`.

ContentView's existing (unqualified) call sites — the ~4000 lines of palette
orchestration (registerCommandPaletteHandlers, commandPaletteCommandContributions,
rename/workspace-description flows, focus-restore, usage-history persistence,
the view body) — are preserved unchanged: each former @State name becomes a
thin computed proxy on ContentView that forwards get/set to the controller
(`private var commandPaletteQuery: String { get { commandPaletteController.
commandPaletteQuery } nonmutating set { ... } }`). This is a real ownership
transfer (the controller is the only place these values are stored; ContentView
holds no @State backing them anymore), done without touching the orchestration
method bodies, which keeps this change mechanically verifiable.

5 properties that were bound via SwiftUI's $name projection at their call
sites (commandPaletteQuery, commandPaletteRenameDraft,
commandPaletteWorkspaceDescriptionDraft, commandPaletteWorkspaceDescriptionHeight,
commandPaletteShouldFocusWorkspaceDescriptionEditor) get a matching
`nameBinding: Binding<T>` proxy instead, since computed properties can't
project a $-binding; call sites were updated to use .

The 2 @focusstate properties (isCommandPaletteSearchFocused,
isCommandPaletteRenameFocused) stay on ContentView — @focusstate is a SwiftUI
View-only property wrapper and cannot be hosted on an ObservableObject. This
is the actual reason the file's previous "CV1" extraction attempt called a
full split impossible; it wasn't, once the @Published-storage-plus-proxy
shape is used instead of moving @State verbatim.

Access-level widening (dropped `private`, no other behavior change) for
nested types now referenced from CommandPaletteController.swift (a different
file): CommandPaletteMode, CommandPaletteRestoreFocusTarget,
CommandPaletteTextSelectionBehavior, CommandPaletteMultilineTextEditorRepresentable,
CommandPaletteRenameTarget, CommandPaletteWorkspaceDescriptionTarget (enum-case
associated types must be at least as visible as the enum), and their AppKit
backing views CommandPaletteMultilineTextEditorView / CommandPaletteMultilineTextView
(NSViewRepresentable makeNSView/updateNSView must be at least as visible as
the representable struct).

The ~4000-line orchestration block and the ContentView+CommandPalette.swift
pure helpers were NOT physically relocated onto the controller in this pass —
they continue to compile and run correctly against the controller through the
proxies, but remain textually in ContentView.swift/ContentView+CommandPalette.swift.
Physically moving that method block is left for a follow-up; the state
ownership change (the core ask) is complete and verified.

Refs #88.
@arzafran

arzafran commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Superseded by #116, which consolidates the seven remaining nuclear-review PRs into one CI run per maintainer request. Every commit from this branch is contained in #116 verbatim.

@arzafran arzafran closed this Jul 9, 2026
@arzafran arzafran deleted the nr/gv branch July 9, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant