Fix an overlay crash and two dead settings (pre-release audit, wave 2)#535
Conversation
1. Crash on a non-finite caret rect. Some host AX implementations (Chromium, Electron, mid-scroll or under load) return NaN/Inf bounds. The existing guards (isNull, != .zero, isEmpty) do not reject non-finite rects, so the value flowed into NSWindow.setFrame, which raises NSInvalidArgumentException and crashes the app on the hottest path (every suggestion shown). Reject non-finite rects at the AX ingest boundary (cocoaRect / validatedCocoaTextRect now collapse them to .zero, which also removes a latent Int(.nan) trap in OverlayState.detail), plus a last-resort finite guard before each OverlayController.setFrame. 2. Two Settings controls were silently dead. AppDelegate built the dependency graph as a local `let environment` and copied the individual services out, but never retained the environment itself. The environment owns the only subscriptions wiring the focus-poll-interval setting (to updatePollInterval) and the global-toggle hotkey rebind (to refreshToggleTap); when the environment deallocated as init returned, both were cancelled, so changing the poll interval did nothing and rebinding the hotkey did nothing until the next relaunch. Retain the environment for the app lifetime. Deliberately deferred: PermissionManager / PermissionGuidanceController have isolated deinits (the macOS 14 double-free class), but they are app-lifetime singletons that never dealloc, so this is latent only and the fix is non-trivial (the deinits do real cleanup, unlike the empty nonisolated-deinit precedent).
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard rectHasFiniteComponents(rect) else { | ||
| return .zero | ||
| } | ||
| guard !rect.isNull, rect != .zero else { |
There was a problem hiding this comment.
CGRect.null silently collapses to .zero
CGRect.null has its origin at (+∞, +∞) in CoreGraphics, so rectHasFiniteComponents(CGRect.null) returns false and both cocoaRect and validatedCocoaTextRect now return .zero for a null rect. Previously the guard !rect.isNull branch caught it and returned the null rect itself. Any downstream caller that distinguishes null from zero (e.g. an isNull check) will now see .zero instead. Worth verifying that every caller only guards with != .zero (not .isNull) so no silent pass-through occurs after this change.
| func test_validatedCocoaTextRect_collapsesNonFiniteRectToZero() { | ||
| // A NaN/Inf AX rect must collapse to .zero, never propagate toward NSWindow.setFrame (a crash). | ||
| let nan = CGRect(x: CGFloat.nan, y: 10, width: 20, height: 14) | ||
| XCTAssertEqual(AXHelper.validatedCocoaTextRect(fromAccessibilityRect: nan, anchorFrame: nil), .zero) | ||
| XCTAssertEqual(AXHelper.cocoaRect(fromAccessibilityRect: nan), .zero) | ||
| } |
There was a problem hiding this comment.
CGFloat.infinity and CGRect.null not covered by collapse test
test_validatedCocoaTextRect_collapsesNonFiniteRectToZero only exercises a NaN input. Adding a CGFloat.infinity case and a CGRect.null case (which now also returns .zero due to the new finite guard) would round out coverage and document the newly observed behavior for CGRect.null.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Wave 2 of the pre-release audit. Two fixes: a crash and two silently-dead settings.
NaN/Infcaret bounds. The existing guards (isNull,!= .zero,isEmpty) do not reject non-finite values, so they flowed intoNSWindow.setFrame, which raisesNSInvalidArgumentExceptionand crashes the app on the hottest path (every suggestion shown). Now rejected at the AX ingest boundary:cocoaRect/validatedCocoaTextRectcollapse a non-finite rect to.zero(which also removes a latentInt(.nan)trap inOverlayState.detail), plus a last-resort finite guard before eachOverlayController.setFrame.AppDelegatebuilt the graph as a locallet environmentand copied the services out, but never retained the environment. The environment owns the only subscriptions for the focus-poll-interval setting (→updatePollInterval) and the global-toggle hotkey rebind (→refreshToggleTap); both were cancelled when the environment deallocated asinitreturned, so changing the poll interval did nothing and rebinding the hotkey did nothing until the next relaunch. Now the environment is retained for the app lifetime.Validation
The AppDelegate retain is verified by trace (app-lifecycle code is not unit-tested); the caret guard is unit-tested.
Linked issues
None. Pre-release hardening, wave 2.
Risk / rollout notes
[weak self]/[weak ...]).PermissionManager/PermissionGuidanceControllerhave isolateddeinits (the macOS 14 double-free class), but they are app-lifetime singletons that never dealloc, so it is latent only and the fix is non-trivial (those deinits do real cleanup, unlike the empty-nonisolated deinitprecedent).Greptile Summary
This PR fixes two independent bugs found during a pre-release audit: a crash caused by non-finite (NaN/Inf) AX caret rects flowing into
NSWindow.setFrame, and two Settings controls (focus poll interval, global-toggle hotkey rebind) that were silently non-functional because theCotabbyAppEnvironmentowning their Combine subscriptions was immediately deallocated.rectHasFiniteComponentsis added as an ingest-boundary guard incocoaRectandvalidatedCocoaTextRect, collapsing non-finite rects to.zerobefore they reach layout math orNSPanel.setFrame; last-resort guards are also added directly before bothsetFramecall sites inOverlayController.AppDelegatenow holdsprivate let environment: CotabbyAppEnvironment, keeping the environment (and itscancellables) alive for the app's full lifetime so the poll-interval and hotkey-rebind subscriptions are never prematurely cancelled.rectHasFiniteComponentsrejection andvalidatedCocoaTextRect/cocoaRectcollapse for NaN inputs; theCGFloat.infinityandCGRect.nullcases are not yet exercised in the collapse test.Confidence Score: 4/5
Safe to merge; both fixes are narrowly scoped defensive changes with no new logic paths for the happy-path.
The crash fix and settings-lifetime fix are both correct. The subtle behavior change around
CGRect.nullcollapsing to.zerois worth a quick caller audit but does not affect observed code paths.Callers of
cocoaRectandvalidatedCocoaTextRectthat may check.isNullon the returned rect, given the newCGRect.nullto.zerocollapse behavior.Important Files Changed
rectHasFiniteComponentsguard at the top ofcocoaRectandvalidatedCocoaTextRect; minor side-effect:CGRect.null(origin = +∞) now collapses to.zeroinstead of propagating as the null rectNSPanel.setFramein both inline and mirror paths; straightforward defensive guardenvironmentto aprivate letinstance property so theCotabbyAppEnvironment's Combine subscriptions are not prematurely cancelled; correct and low-riskrectHasFiniteComponents(NaN + infinity) andvalidatedCocoaTextRect/cocoaRectcollapse;CGFloat.infinitycase is not tested in the collapse test, andCGRect.nullcollapse to.zerois untestedFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD AX[Host AX: BoundsForRange / AXFrame] --> ingest["cocoaRect / validatedCocoaTextRect\n(AX ingest boundary)"] ingest -->|non-finite NaN/Inf| zero[Return .zero] ingest -->|finite| geom[Layout geometry math] geom --> frame["panelFrame (inline) / panelFrame (mirror)"] frame -->|non-finite last-resort| skip[Log warning, skip setFrame] frame -->|finite| setFrame["NSPanel.setFrame → orderFrontRegardless"] zero --> overlaySkip[Overlay suppressed — no crash] skip --> overlaySkipReviews (1): Last reviewed commit: "Fix an overlay crash and two dead settin..." | Re-trigger Greptile