Repository navigation
Avoid MainActor executor crashes in tab hit testing - #190
austinywang wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR updates tab-bar geometry observers to dispatch through the main actor and aligns hit-region protocols, registry entry points, and view implementations with main-actor isolation. ChangesTab-bar concurrency alignment
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a macOS 26.4.x crash where
Confidence Score: 4/5Safe to merge after dogfood stress validation; the async dispatch introduces a one-run-loop delay that is benign for geometry updates and the FIFO MainActor queue preserves notification ordering in almost all cases. The core fix (swapping assumeIsolated for Task { @mainactor }) is idiomatic and removes a real crash on macOS 26.4.x. The protocol-level @mainactor annotations are a clean long-term improvement. The leftover nonisolated(unsafe) on tabIds is a minor inconsistency, and there is a narrow theoretical ordering window around willStartLiveScrollNotification and boundsDidChangeNotification that deserves a clarifying comment but is unlikely to cause visible regressions in practice. TabBarItemGeometryRegistry.swift — the four async notification callbacks near the willStartLiveScroll observer are the most sensitive part of the change and would benefit from a brief inline comment explaining why FIFO MainActor task ordering is sufficient. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant NC as NotificationCenter
participant CB as Closure (main queue)
participant MA as MainActor (Task)
participant Reg as TabBarItemGeometryRegistry
participant HT as containsBonsplitTabItemHit (@MainActor)
Note over NC,Reg: Old path (crashy on macOS 26.4.x)
NC->>CB: boundsDidChangeNotification (queue: .main)
CB->>MA: MainActor.assumeIsolated
Note over MA: Runtime fault if executor state is inconsistent
MA->>Reg: scrollBoundsDidChange()
Note over NC,Reg: New path (this PR)
NC->>CB: boundsDidChangeNotification (queue: .main)
CB->>MA: "Task { @MainActor [weak self] in }"
Note over MA: Async enqueue — no runtime executor check
MA->>Reg: scrollBoundsDidChange()
Note over HT: Hit-test path (this PR)
HT->>HT: containsBonsplitTabItemHit(localPoint:)
Note over HT: Now @MainActor on protocol, hitBounds is plain stored property
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant NC as NotificationCenter
participant CB as Closure (main queue)
participant MA as MainActor (Task)
participant Reg as TabBarItemGeometryRegistry
participant HT as containsBonsplitTabItemHit (@MainActor)
Note over NC,Reg: Old path (crashy on macOS 26.4.x)
NC->>CB: boundsDidChangeNotification (queue: .main)
CB->>MA: MainActor.assumeIsolated
Note over MA: Runtime fault if executor state is inconsistent
MA->>Reg: scrollBoundsDidChange()
Note over NC,Reg: New path (this PR)
NC->>CB: boundsDidChangeNotification (queue: .main)
CB->>MA: "Task { @MainActor [weak self] in }"
Note over MA: Async enqueue — no runtime executor check
MA->>Reg: scrollBoundsDidChange()
Note over HT: Hit-test path (this PR)
HT->>HT: containsBonsplitTabItemHit(localPoint:)
Note over HT: Now @MainActor on protocol, hitBounds is plain stored property
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -120,7 +120,7 @@ final class TabBarItemGeometryRegistry { | |||
| object: scrollView, | |||
| queue: .main | |||
| ) { [weak self] _ in | |||
| MainActor.assumeIsolated { | |||
| Task { @MainActor [weak self] in | |||
| self?.userWillScroll() | |||
| } | |||
| } | |||
There was a problem hiding this comment.
The willStartLiveScrollNotification is documented to fire synchronously before the first scroll delta is applied. Replacing assumeIsolated with Task { @MainActor } means userWillScroll() (which clears expectedProgrammaticOffset and pendingScrollIntent) now runs one run-loop turn after the notification, while boundsDidChangeNotification tasks from that same scroll gesture may already be enqueued. Although Task enqueues are FIFO on the MainActor and the willStartLiveScroll task should arrive first, any notification delivered before that task drains — e.g., a re-entrant layout triggered by AppKit — can create a task that observes stale expectedProgrammaticOffset and issues a spurious programmatic offset correction mid-gesture. The window is tiny in practice, but worth a comment explaining why ordering is still safe here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift (1)
2273-2273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unnecessary
nonisolated(unsafe)fromtabIds.Since
containsBonsplitTabItemHitis now explicitly@MainActor,tabIdsis exclusively accessed and mutated on the main actor. Removingnonisolated(unsafe)aligns with the PR objective to clean up unsafe nonisolated hit-testing storage.♻️ Proposed refactor
- nonisolated(unsafe) var tabIds: [UUID] = [] + var tabIds: [UUID] = []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Bonsplit/Internal/Views/TabBarView.swift` at line 2273, Remove the unnecessary nonisolated(unsafe) annotation from tabIds, leaving it as a regular variable because containsBonsplitTabItemHit now accesses and mutates it exclusively on the MainActor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Line 2273: Remove the unnecessary nonisolated(unsafe) annotation from tabIds,
leaving it as a regular variable because containsBonsplitTabItemHit now accesses
and mutates it exclusively on the MainActor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1321fb49-3f09-4677-8715-56fa5c9972ff
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarItemGeometryRegistry.swiftSources/Bonsplit/Internal/Views/TabBarView.swift
1 participant
Summary
Why
On macOS 26.4.x, MainActor.assumeIsolated can fault inside the Swift executor runtime even when AppKit invokes the callback on the main thread. The tab hit-test path runs for ordinary pointer events, making tagged cmux builds crash during normal use.
Verification
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes crashes in tab hit testing on macOS 26.4 by running hit-region queries and geometry updates on the main actor. This removes reliance on
MainActor.assumeIsolatedand prevents executor faults during pointer events.BonsplitTabItemHitRegionProviding.containsBonsplitTabItemHitandBonsplitTabItemHitRegionRegistry.containsWindowPointas@MainActor.MainActor.assumeIsolatedwithTask { @MainActor ... }for scroll and geometry notifications.nonisolated(unsafe)hit-bounds storage; access is now main-actor isolated.Written for commit bb022b4. Summary will update on new commits.
Summary by CodeRabbit