refactor: shared portal helpers with divergences as explicit policy - #255
Merged
Conversation
…une behavior Adds the shared TransientRecoveryRetryState/scheduleRetryIfNeeded pure abstraction (H5 of the portal-registry dedup) with unit tests covering both reset policies, plus a new test pinning WindowBrowserPortal's deliberate nil-anchor-preserves-entry behavior so a later shared pruneEntries migration can't accidentally harmonize it with Terminal's opposite (nil-anchor-is-dead) policy.
Adds tearDownEntries, observeGeometryAffectingNotifications (+ GeometryNotificationSpec), hitScanReversedSubviews, and the generic pruneEntries skeleton to HostedViewPortalRegistry. Nothing calls these yet — WindowTerminalPortal and WindowBrowserPortal are wired to them in follow-up commits, one helper at a time.
remaining==0 always grants a fresh budget under whenExhausted, regardless of reason (Terminal doesn't track a reason at all) — my initial assertion had this backwards.
Browser side of the H1 migration. Behavior-preserving: same removeGeometryObservers-then-detach-each-id order, just via the shared base-class loop shape.
… helper (H2) Browser side of the H2 migration: the 4-notification list (didResize, didEndLiveResize, NSSplitView.willResizeSubviews, didResizeSubviews gated through shouldTreatSplitResizeAsExternalGeometry) is passed as data to observeGeometryAffectingNotifications instead of repeating the addObserver/MainActor.assumeIsolated boilerplate per notification. Handler bodies are unchanged.
Terminal side of the H1 migration. Behavior-preserving: same removeGeometryObservers-then-detach-each-id order, plus Terminal's own NSLayoutConstraint cleanup and hostView removal unchanged around it.
…d helper (H2) Terminal side of the H2 migration: the 5-notification list (didResize, didEndLiveResize, NSSplitView.didResizeSubviews gated on window match, hostView frameDidChange, hostView boundsDidChange) is passed as data to observeGeometryAffectingNotifications instead of repeating the addObserver/MainActor.assumeIsolated boilerplate per notification. Handler bodies are unchanged.
…ton (H3) Browser side of the H3 migration. Same reversed-subviews scan; map casts to WindowBrowserSlotView and checks isHidden, resolve does the frame-containment check plus the containerView->webView lookup.
…n (H3) Terminal side of the H3 migration. Same reversed-subviews scan; map casts to GhosttySurfaceScrollView and checks the entries dict plus isHidden, resolve does the frame-containment check plus the existing hitTest fallback. terminalViewAtWindowPoint (no Browser counterpart) is left untouched.
…ton (H4) Browser side of the H4 migration. isDead closure preserves the deliberate nil-anchor-is-NOT-dead policy (keeps a hidden WKWebView alive across workspace switches instead of forcing a WebKit reload storm) plus the container-orphan signal (container.superview == nil || !isDescendant(of: hostView)). The reverse-map reconciliation pass after pruning is unchanged.
…eton (H4) Terminal side of the H4 migration. isDead closure preserves the existing policy: fully deallocated anchor is dead; anchor merely off-tree-for-this-host is dead only when the entry isn't currently visible. The reverse-map reconciliation pass after pruning is unchanged.
…yState (H5) Browser side of the H5 migration. scheduleTransientRecoveryRetryIfNeeded now delegates its counter/reason bookkeeping to the shared, pure scheduleRetryIfNeeded using the .whenReasonChanges policy (a new retry reason gets its own full budget) — preserving the exact original behavior, now backed by the unit-tested pure function. entry field writes and the deferred-full-sync side effect are unchanged.
…yState (H5) Terminal side of the H5 migration. scheduleTransientRecoveryRetryIfNeeded now delegates its counter bookkeeping to the shared, pure scheduleRetryIfNeeded using the .whenExhausted policy (Terminal has no reason field — reset only when the budget is fully spent) — preserving the exact original behavior, now backed by the unit-tested pure function.
…test anchor2 was allocated after anchor1 was dropped, so ARC could reuse anchor1's freed address for anchor2 — aliasing the stale webViewByAnchorId[anchor1] entry and tripping bind()'s unrelated "anchor replaced" detach path (which detaches webView1) instead of exercising pruneDeadEntries' nil-anchor policy. Allocating anchor2 up front avoids the collision.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Fourth of five PRs from today's maintainability audit (finding N4, executing the planner design). The browser and terminal window portals reimplemented the same machinery — teardown loops, geometry-notification registration, at-point hit scans, dead-entry pruning, transient-recovery retry — as five near-identical copies each. Those five helpers now live once on the shared HostedViewPortalRegistry base, and the two REAL behavioral divergences become explicit, tested policy instead of buried conditionals:
Deliberately NOT shared (majority-bespoke, out of scope per plan): ensureInstalled, bind, synchronizeHostedView/WebView, scheduleExternalGeometrySynchronize. The audit's 800-1200-line estimate counted superficial similarity; the honest yield is the helpers + the divergence-as-data clarity. The inverse z-order placement rules between the two portals were verified as the deliberate stacking contract, not drift.
14 commits, browser-first per helper (lower blast radius proves each helper before it reaches the terminal hot path), every commit builds green.
Review order
Test plan