Avoid macOS 15 DevTools attach crash and analytics quit hang#4981
Avoid macOS 15 DevTools attach crash and analytics quit hang#4981lawrencecchen wants to merge 2 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds OS-version-based gating for WebKit's attached Web Inspector in BrowserPanel to work around a crash on macOS 15.x, and refactors PostHogAnalytics to support testable flush behavior via dependency injection while converting the public flush API from synchronous to asynchronous dispatch. ChangesBrowser Developer Tools Attachment Policy
PostHog Analytics Flush Dependency Injection
Sequence Diagram(s)Not applicable. The changes introduce dependency-injection infrastructure and a policy-gating mechanism without introducing multi-component sequential flows that would benefit from visualization. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (14 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 3375-3392: The test currently only asserts flush() returns quickly
but doesn't verify the queued task actually ran; update the test by adding an
expectation inside the injected flushImplementation passed to
PostHogAnalytics.makeForTesting (e.g., create an XCTestExpectation in the test
and have flushImplementation fulfill it when executed), then after calling
releaseQueuedWork.signal() wait for that expectation to ensure the enqueued
flush work ran; locate the test that constructs PostHogAnalytics via
PostHogAnalytics.makeForTesting, modify the flushImplementation closure to call
expectation.fulfill(), and add an XCTWaiter/XCTestExpectation wait after
releaseQueuedWork.signal().
In `@Sources/Panels/BrowserPanel.swift`:
- Line 43: The constant firstStableAttachedInspectorMajorVersion is set to 26
which incorrectly disables the attached inspector for all current macOS
releases; change its value to the intended macOS major-version threshold (e.g.,
15) so the attached inspector is only disabled for versions older than the macOS
15 workaround, and update both occurrences of
firstStableAttachedInspectorMajorVersion in BrowserPanel.swift to the corrected
value.
In `@Sources/PostHogAnalytics.swift`:
- Around line 105-109: The flush() method currently calls
dispatchAsyncOnWorkQueue(_:) which may execute inline if the caller is already
on workQueue, making flush synchronous for re-entrant callers; change flush() to
always schedule asynchronously by dispatching directly to workQueue (e.g., use
workQueue.async { [weak self] in ... }) so that the body (guard let self,
self.didStart else { return }; self.flushImplementation()) never runs
synchronously; keep the weak self capture and the same
guard/didStart/flushImplementation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8aa9b702-3309-472b-939c-6eb5fce2a3fc
📒 Files selected for processing (4)
Sources/Panels/BrowserPanel.swiftSources/PostHogAnalytics.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/GhosttyConfigTests.swift
| let analytics = PostHogAnalytics.makeForTesting( | ||
| workQueue: queue, | ||
| didStart: true, | ||
| flushImplementation: {} | ||
| ) | ||
| let flushReturned = expectation(description: "flush returned") | ||
| DispatchQueue.global(qos: .userInitiated).async { | ||
| analytics.flush() | ||
| flushReturned.fulfill() | ||
| } | ||
|
|
||
| let result = XCTWaiter().wait(for: [flushReturned], timeout: 0.2) | ||
| releaseQueuedWork.signal() | ||
| XCTAssertEqual( | ||
| result, | ||
| .completed, | ||
| "PostHogAnalytics.flush must not synchronously wait for the analytics queue while the main thread is terminating." | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Assert that the queued flush eventually runs.
This test proves flush() returns quickly, but it would also pass if flush() stopped enqueuing any work at all. Add an expectation in flushImplementation and wait for it after releaseQueuedWork.signal() so the test also covers the injected execution path.
Suggested strengthening
- let analytics = PostHogAnalytics.makeForTesting(
+ let didExecuteFlush = expectation(description: "flush executed")
+ let analytics = PostHogAnalytics.makeForTesting(
workQueue: queue,
didStart: true,
- flushImplementation: {}
+ flushImplementation: {
+ didExecuteFlush.fulfill()
+ }
)
let flushReturned = expectation(description: "flush returned")
DispatchQueue.global(qos: .userInitiated).async {
analytics.flush()
flushReturned.fulfill()
@@
releaseQueuedWork.signal()
XCTAssertEqual(
result,
.completed,
"PostHogAnalytics.flush must not synchronously wait for the analytics queue while the main thread is terminating."
)
+ wait(for: [didExecuteFlush], timeout: 1.0)🤖 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 `@cmuxTests/GhosttyConfigTests.swift` around lines 3375 - 3392, The test
currently only asserts flush() returns quickly but doesn't verify the queued
task actually ran; update the test by adding an expectation inside the injected
flushImplementation passed to PostHogAnalytics.makeForTesting (e.g., create an
XCTestExpectation in the test and have flushImplementation fulfill it when
executed), then after calling releaseQueuedWork.signal() wait for that
expectation to ensure the enqueued flush work ran; locate the test that
constructs PostHogAnalytics via PostHogAnalytics.makeForTesting, modify the
flushImplementation closure to call expectation.fulfill(), and add an
XCTWaiter/XCTestExpectation wait after releaseQueuedWork.signal().
| } | ||
|
|
||
| enum BrowserDeveloperToolsAttachmentPolicy { | ||
| private static let firstStableAttachedInspectorMajorVersion = 26 |
There was a problem hiding this comment.
Fix the OS major-version threshold; 26 effectively disables attached inspector for all current macOS releases.
ProcessInfo.processInfo.operatingSystemVersion.majorVersion returns macOS major versions (15/16/…), so >= 26 keeps attached inspector disabled far beyond the intended macOS 15 workaround. This broadens the fallback behavior and likely regresses attach support on unaffected versions.
Suggested fix
enum BrowserDeveloperToolsAttachmentPolicy {
- private static let firstStableAttachedInspectorMajorVersion = 26
+ private static let firstStableAttachedInspectorMajorVersion = 16Also applies to: 68-68
🤖 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/Panels/BrowserPanel.swift` at line 43, The constant
firstStableAttachedInspectorMajorVersion is set to 26 which incorrectly disables
the attached inspector for all current macOS releases; change its value to the
intended macOS major-version threshold (e.g., 15) so the attached inspector is
only disabled for versions older than the macOS 15 workaround, and update both
occurrences of firstStableAttachedInspectorMajorVersion in BrowserPanel.swift to
the corrected value.
| func flush() { | ||
| dispatchSyncOnWorkQueue { | ||
| guard didStart else { return } | ||
| PostHogSDK.shared.flush() | ||
| dispatchAsyncOnWorkQueue { [weak self] in | ||
| guard let self, self.didStart else { return } | ||
| self.flushImplementation() | ||
| } |
There was a problem hiding this comment.
Make flush() unconditionally async.
Line 106 still goes through dispatchAsyncOnWorkQueue(_:), and that helper runs inline when the caller is already on workQueue. That means flush() is still synchronous for re-entrant callers, which weakens the new “return promptly” contract this PR is introducing.
Suggested fix
func flush() {
- dispatchAsyncOnWorkQueue { [weak self] in
+ workQueue.async { [weak self] in
guard let self, self.didStart else { return }
self.flushImplementation()
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func flush() { | |
| dispatchSyncOnWorkQueue { | |
| guard didStart else { return } | |
| PostHogSDK.shared.flush() | |
| dispatchAsyncOnWorkQueue { [weak self] in | |
| guard let self, self.didStart else { return } | |
| self.flushImplementation() | |
| } | |
| func flush() { | |
| workQueue.async { [weak self] in | |
| guard let self, self.didStart else { return } | |
| self.flushImplementation() | |
| } | |
| } |
🤖 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/PostHogAnalytics.swift` around lines 105 - 109, The flush() method
currently calls dispatchAsyncOnWorkQueue(_:) which may execute inline if the
caller is already on workQueue, making flush synchronous for re-entrant callers;
change flush() to always schedule asynchronously by dispatching directly to
workQueue (e.g., use workQueue.async { [weak self] in ... }) so that the body
(guard let self, self.didStart else { return }; self.flushImplementation())
never runs synchronously; keep the weak self capture and the same
guard/didStart/flushImplementation logic.
Greptile SummaryThis PR fixes two independent macOS regressions: a crash when the Web Inspector attach API is called on macOS 15 WebKit builds, and a quit-time hang caused by
Confidence Score: 4/5The changes are narrowly scoped, well-tested, and address concrete crash/hang reports. The async flush trade-off and the DEBUG-global race are minor concerns that don't block shipping. Both changes are backed by reproducing unit tests. The async flush() intentionally trades guaranteed quit-time delivery for non-blocking behaviour — acceptable for telemetry, but the delivery semantics shift is undocumented at the call-site. The nonisolated(unsafe) test global is DEBUG-only and low-risk in practice. No production logic path is left in an incorrect state. Sources/PostHogAnalytics.swift — the quit-time delivery semantics of flush() changed; Sources/Panels/BrowserPanel.swift — the nonisolated(unsafe) test override global. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["showDeveloperTools()"] --> B["prepareDeveloperToolsForRevealIfNeeded(inspector)"]
B --> C{"allowsAttachedInspector()\nosVersion.major >= 26?"}
C -- "No (macOS 15–25)" --> D["setPreferredPresentation(.detached)\nreturn — no attach call"]
C -- "Yes (macOS 26+)" --> E{"preferredPresentation\n== .unknown?"}
E -- "Yes" --> H["inspector.attach()"]
E -- "No" --> F{"preferredPresentation\n== .attached?"}
F -- "No (.detached)" --> G["return — skip attach"]
F -- "Yes" --> I{"webView in window?\ninspector not already attached?"}
I -- "No" --> G
I -- "Yes" --> H
H --> J["inspector.show()"]
D --> J
Reviews (1): Last reviewed commit: "fix: prevent crash report diagnostics" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func flush() { | ||
| dispatchSyncOnWorkQueue { | ||
| guard didStart else { return } | ||
| PostHogSDK.shared.flush() | ||
| dispatchAsyncOnWorkQueue { [weak self] in | ||
| guard let self, self.didStart else { return } | ||
| self.flushImplementation() | ||
| } | ||
| } |
There was a problem hiding this comment.
The quit-time caller in applicationWillTerminate now dispatches flushImplementation() asynchronously and returns immediately. If the run loop drains before the workQueue block executes, events captured in the final moments of the session will not be sent in this launch. Whether PostHog's SDK persists those events to disk for the next launch depends on whether the SDK's own flush cycle fires before exit — there's no guarantee. The fix is correct for eliminating the hang, but it's worth noting explicitly (e.g. in a comment near the applicationWillTerminate call-site or here) that quit-time delivery is now best-effort.
| enum BrowserDeveloperToolsAttachmentPolicy { | ||
| private static let firstStableAttachedInspectorMajorVersion = 26 | ||
|
|
||
| #if DEBUG | ||
| private nonisolated(unsafe) static var attachedInspectorAllowedOverrideForTesting: Bool? | ||
|
|
||
| static func withAttachedInspectorAllowedForTesting<T>( | ||
| _ allowed: Bool?, | ||
| _ body: () throws -> T | ||
| ) rethrows -> T { | ||
| let previous = attachedInspectorAllowedOverrideForTesting | ||
| attachedInspectorAllowedOverrideForTesting = allowed | ||
| defer { attachedInspectorAllowedOverrideForTesting = previous } | ||
| return try body() | ||
| } | ||
| #endif | ||
|
|
||
| static func allowsAttachedInspector( | ||
| osVersion: OperatingSystemVersion = ProcessInfo.processInfo.operatingSystemVersion | ||
| ) -> Bool { | ||
| #if DEBUG | ||
| if let attachedInspectorAllowedOverrideForTesting { | ||
| return attachedInspectorAllowedOverrideForTesting | ||
| } | ||
| #endif | ||
| // macOS 15 WebKit 20621 crashes inside WebInspectorUIProxy::platformAttach. | ||
| return osVersion.majorVersion >= firstStableAttachedInspectorMajorVersion | ||
| } | ||
| } |
There was a problem hiding this comment.
nonisolated(unsafe) test-override global is not concurrency-safe across parallel test suites
attachedInspectorAllowedOverrideForTesting is a mutable static written and read in withAttachedInspectorAllowedForTesting without any lock. If Xcode runs test classes in parallel (e.g. BrowserDeveloperToolsConfigurationTests alongside BrowserDeveloperToolsVisibilityPersistenceTests), concurrent mutation of this global is a data race under Swift 6's memory model. Since the scope is DEBUG-only and the existing test runner is likely serial, this is low risk in practice, but OSAllocatedUnfairLock or @MainActor isolation would eliminate the hazard.
1 participant
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Targeted workarounds for a known WebKit crash and analytics shutdown blocking; behavior changes are version-gated and limited to DevTools presentation and flush timing.
Overview
Adds a macOS version gate so Web Inspector is not side-attached on macOS 15 (WebKit
platformAttachcrash); those systems prefer detached DevTools and skip the privateattachcall when revealing tools.PostHog
flush()is now async on the analytics queue (nosyncwait), with an injectable flush hook and DEBUG test factory so shutdown does not block on a busy queue.Tests cover the attach policy (15 vs 26), DevTools show without attach when disallowed, and non-blocking flush.
Reviewed by Cursor Bugbot for commit 1ccac78. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Prevents a crash when opening DevTools on macOS 15 by avoiding side-attached Web Inspector, and fixes a quit hang by making analytics flushing non-blocking. Adds tests to cover both behaviors.
Written for commit 1ccac78. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests