Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions Sources/Panels/BrowserPanel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,36 @@ struct BrowserRemoteWorkspaceStatus: Equatable {
let lastHeartbeatAt: Date?
}

enum BrowserDeveloperToolsAttachmentPolicy {
private static let firstStableAttachedInspectorMajorVersion = 26
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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 = 16

Also 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.


#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
}
}
Comment on lines +42 to +70
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.


enum GhosttyBackgroundTheme {
static func clampedOpacity(_ opacity: Double) -> CGFloat {
WindowAppearanceSnapshot.clampedOpacity(opacity)
Expand Down Expand Up @@ -6300,6 +6330,10 @@ extension BrowserPanel {
}

private func prepareDeveloperToolsForRevealIfNeeded(_ inspector: NSObject) {
guard BrowserDeveloperToolsAttachmentPolicy.allowsAttachedInspector() else {
setPreferredDeveloperToolsPresentation(.detached)
return
}
if preferredDeveloperToolsPresentation != .unknown {
guard preferredDeveloperToolsPresentation == .attached else { return }
guard webView.superview != nil, webView.window != nil else { return }
Expand Down
55 changes: 38 additions & 17 deletions Sources/PostHogAnalytics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,46 @@ final class PostHogAnalytics {
private let workQueueSpecificKey = DispatchSpecificKey<Void>()
private let utcHourFormatter: DateFormatter
private let utcDayFormatter: DateFormatter
private let flushImplementation: () -> Void

private var didStart = false
private var activeCheckTimer: Timer?

private init() {
workQueue = DispatchQueue(label: "com.cmux.posthog.analytics", qos: .utility)
utcHourFormatter = Self.makeUTCFormatter("yyyy-MM-dd'T'HH")
utcDayFormatter = Self.makeUTCFormatter("yyyy-MM-dd")
self.workQueue = DispatchQueue(label: "com.cmux.posthog.analytics", qos: .utility)
self.utcHourFormatter = Self.makeUTCFormatter("yyyy-MM-dd'T'HH")
self.utcDayFormatter = Self.makeUTCFormatter("yyyy-MM-dd")
self.flushImplementation = { PostHogSDK.shared.flush() }
workQueue.setSpecific(key: workQueueSpecificKey, value: ())
}

private init(
workQueue: DispatchQueue,
didStart: Bool,
flushImplementation: @escaping () -> Void
) {
self.workQueue = workQueue
self.utcHourFormatter = Self.makeUTCFormatter("yyyy-MM-dd'T'HH")
self.utcDayFormatter = Self.makeUTCFormatter("yyyy-MM-dd")
self.flushImplementation = flushImplementation
self.didStart = didStart
workQueue.setSpecific(key: workQueueSpecificKey, value: ())
}

#if DEBUG
static func makeForTesting(
workQueue: DispatchQueue,
didStart: Bool,
flushImplementation: @escaping () -> Void
) -> PostHogAnalytics {
PostHogAnalytics(
workQueue: workQueue,
didStart: didStart,
flushImplementation: flushImplementation
)
}
#endif

private var isEnabled: Bool {
guard TelemetrySettings.enabledForCurrentLaunch else { return false }
#if DEBUG
Expand All @@ -56,7 +85,7 @@ final class PostHogAnalytics {
let didCaptureHourly = self.trackHourlyActiveOnWorkQueue(reason: reason, flush: false)
if didCaptureDaily || didCaptureHourly {
// On app focus we can capture both events; flush once to reduce extra work.
PostHogSDK.shared.flush()
self.flushImplementation()
}
}
}
Expand All @@ -74,9 +103,9 @@ final class PostHogAnalytics {
}

func flush() {
dispatchSyncOnWorkQueue {
guard didStart else { return }
PostHogSDK.shared.flush()
dispatchAsyncOnWorkQueue { [weak self] in
guard let self, self.didStart else { return }
self.flushImplementation()
}
Comment on lines 105 to 109
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

}
Comment on lines 105 to 110
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Async flush no longer guarantees delivery before process exit

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.


Expand Down Expand Up @@ -144,7 +173,7 @@ final class PostHogAnalytics {

if flush && Self.shouldFlushAfterCapture(event: event) {
// For active metrics we care more about delivery than batching.
PostHogSDK.shared.flush()
flushImplementation()
}

return true
Expand Down Expand Up @@ -176,7 +205,7 @@ final class PostHogAnalytics {

if flush && Self.shouldFlushAfterCapture(event: event) {
// Keep hourly freshness and avoid losing a deduped hour on abrupt exits.
PostHogSDK.shared.flush()
flushImplementation()
}

return true
Expand All @@ -190,14 +219,6 @@ final class PostHogAnalytics {
workQueue.async(execute: block)
}

private func dispatchSyncOnWorkQueue(_ block: () -> Void) {
if DispatchQueue.getSpecific(key: workQueueSpecificKey) != nil {
block()
return
}
workQueue.sync(execute: block)
}

private func utcHourString(_ date: Date) -> String {
utcHourFormatter.string(from: date)
}
Expand Down
34 changes: 34 additions & 0 deletions cmuxTests/BrowserConfigTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1833,6 +1833,23 @@ final class BrowserDeveloperToolsConfigurationTests: XCTestCase {
}
}

func testAttachedWebInspectorIsDisabledOnMacOS15() {
XCTAssertFalse(
BrowserDeveloperToolsAttachmentPolicy.allowsAttachedInspector(
osVersion: OperatingSystemVersion(majorVersion: 15, minorVersion: 7, patchVersion: 5)
),
"The crash report is WebKit::WebInspectorUIProxy::platformAttach on macOS 15.7.5; cmux should not force side-attached Web Inspector there."
)
}

func testAttachedWebInspectorRemainsEnabledOnMacOS26() {
XCTAssertTrue(
BrowserDeveloperToolsAttachmentPolicy.allowsAttachedInspector(
osVersion: OperatingSystemVersion(majorVersion: 26, minorVersion: 0, patchVersion: 0)
)
)
}

func testBrowserPanelRefreshesUnderPageBackgroundColorWhenGhosttyBackgroundChanges() {
let panel = BrowserPanel(workspaceId: UUID())
let updatedColor = NSColor(srgbRed: 0.18, green: 0.29, blue: 0.44, alpha: 1.0)
Expand Down Expand Up @@ -3623,6 +3640,23 @@ final class BrowserDeveloperToolsVisibilityPersistenceTests: XCTestCase {
XCTAssertEqual(inspector.attachCount, 2)
}

func testShowDeveloperToolsSkipsAttachWhenAttachmentPolicyDisallowsIt() throws {
let (panel, inspector) = makePanelWithInspector()
defer { closeBrowserPanel(panel) }

try BrowserDeveloperToolsAttachmentPolicy.withAttachedInspectorAllowedForTesting(false) {
XCTAssertTrue(panel.showDeveloperTools())
}

XCTAssertTrue(panel.isDeveloperToolsVisible())
XCTAssertEqual(
inspector.attachCount,
0,
"macOS versions with unstable WebKit side attach should open DevTools without calling the private attach selector."
)
XCTAssertEqual(inspector.showCount, 1)
}

func testSyncRespectsManualCloseAndPreventsUnexpectedRestore() {
let (panel, inspector) = makePanelWithInspector()
defer { closeBrowserPanel(panel) }
Expand Down
30 changes: 30 additions & 0 deletions cmuxTests/GhosttyConfigTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3362,6 +3362,36 @@ final class UITestLaunchManifestTests: XCTestCase {
}

final class PostHogAnalyticsPropertiesTests: XCTestCase {
func testFlushReturnsWithoutWaitingForBusyAnalyticsQueue() {
let queue = DispatchQueue(label: "com.cmux.posthog.analytics.test")
let queuedWorkStarted = DispatchSemaphore(value: 0)
let releaseQueuedWork = DispatchSemaphore(value: 0)
queue.async {
queuedWorkStarted.signal()
_ = releaseQueuedWork.wait(timeout: .now() + 5)
}
XCTAssertEqual(queuedWorkStarted.wait(timeout: .now() + 1), .success)

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."
)
Comment on lines +3375 to +3392
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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().

}

func testDailyActivePropertiesIncludeVersionAndBuild() {
let properties = PostHogAnalytics.dailyActiveProperties(
dayUTC: "2026-02-21",
Expand Down
Loading