Skip to content

Commit e282e27

Browse files
Archit-lalclaude
andcommitted
fix(macos): keep recognizing other tabs, and stop ambient suppression events
Eight findings from a fourth review pass. Each is real: - **One settled integration ended recognition for the whole browser session.** `shouldKeepWatching` only continued on "nothing matched", so a user whose Gmail is already connected settled at 3s on a Gmail tab and was never offered ChatGPT after switching tabs — with `alreadyConnected` being the most common outcome, this quietly disabled browser nudges for exactly the set-up users the export integrations target. Suppressions are now split into per-integration and global; only a global answer ends the session. - **The browser-chrome strip list named the wrong browsers.** On macOS the Chromium browsers do not append their product name; Firefox, Zen and Vivaldi do, and none of them were stripped — so suffix matching failed on the browsers that need it most. - **Signed-out and mid-onboarding emitted an event per activation.** Finder is a Local Files trigger and is activated constantly, so a signed-out user produced a suppressed event every few seconds for a state that cannot change until they sign in. Both are now part of the pre-inspection gate, so no work happens at all, and both are classified as ambient. - **A queued card that was later dropped was invisible.** `showNotification` offers an `onDropped` callback and the presenter passed none, so the "owed but never drawn" class the code comments care about was silent. It now reports `bar_unavailable`. - **"Already connected" paid a full MCP config scan on every activation.** The policy front-loads its cheap gates so an opted-out user never reaches the scan, but the most common terminal state can only be decided after it. Export statuses are cached for a minute and invalidated on connect. - **The owner guard compared a raw default against a resolved runtime owner**, which differ by trimming, the non-production automation override, and the nil returned mid-transition — so it misfired on exactly the builds it is exercised on. Both sides now resolve the same way. - **`stop()` and `setFeatureEnabled` had no callers**, reading as live control surfaces for a lifecycle nothing drives. Accepted trade-off, not fixed: the card keeps the bar's standard 6s dismissal, so a user who glances away can lose one of an integration's three lifetime offers. Making this card persistent means either bypassing the owner-bound delivery path (as the reach-error card does) or editing a shared timeout whose SwiftLint baseline is down-only. Both are worse than the 6s. Verification: xcrun swift test --package-path Desktop — 5509 tests, 0 failures; SwiftLint 0 violations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 2278384 commit e282e27

7 files changed

Lines changed: 183 additions & 59 deletions

desktop/macos/Desktop/Sources/IntegrationNudges/IntegrationConnectionInspector.swift

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,45 @@ enum IntegrationConnectionInspector {
1818

1919
case .exportDestination(let destinationID):
2020
guard let destination = MemoryExportDestination(rawValue: destinationID) else { return false }
21-
let statuses = await MemoryExportService.shared.allStatuses()
22-
return statuses[destination]?.hasConnection ?? false
21+
return await exportStatuses()[destination]?.hasConnection ?? false
2322
}
2423
}
2524

25+
/// Export statuses, cached briefly.
26+
///
27+
/// `allStatuses()` scans and parses every local MCP config file. The policy
28+
/// front-loads its cheap gates so an opted-out user never reaches this, but
29+
/// "already connected" — the most common terminal state, and the one every
30+
/// set-up user hits — can only be decided *after* the scan. Without a cache,
31+
/// opening ChatGPT.app, Claude.app and Obsidian in a minute pays for three
32+
/// full scans to reach the same answer each time.
33+
///
34+
/// The window is short because the cost of being stale is bounded either way:
35+
/// a just-connected integration is cleared through `noteConnected`, and a
36+
/// just-disconnected one waits at most this long for its next offer.
37+
private static let exportStatusCacheTTL: TimeInterval = 60
38+
private static var cachedExportStatuses: [MemoryExportDestination: MemoryExportStatus]?
39+
private static var cachedExportStatusesAt: Date?
40+
41+
private static func exportStatuses() async -> [MemoryExportDestination: MemoryExportStatus] {
42+
if let cachedExportStatuses, let cachedExportStatusesAt,
43+
Date().timeIntervalSince(cachedExportStatusesAt) < exportStatusCacheTTL
44+
{
45+
return cachedExportStatuses
46+
}
47+
let statuses = await MemoryExportService.shared.allStatuses()
48+
cachedExportStatuses = statuses
49+
cachedExportStatusesAt = Date()
50+
return statuses
51+
}
52+
53+
/// Drops the export cache. Called when a connection changes so the next offer
54+
/// sees the new state rather than waiting out the window.
55+
static func invalidateExportStatuses() {
56+
cachedExportStatuses = nil
57+
cachedExportStatusesAt = nil
58+
}
59+
2660
private static var cachedStore: ImportConnectorStatusStore?
2761
private static var cachedStoreOwnerID: String?
2862

desktop/macos/Desktop/Sources/IntegrationNudges/IntegrationNudgeCoordinator.swift

Lines changed: 54 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,12 @@ final class IntegrationNudgeCoordinator {
5151
@MainActor (
5252
_ ownerID: String,
5353
_ match: IntegrationNudgeMatcher.Match,
54-
_ onPresented: @escaping @MainActor () -> Void
54+
_ onPresented: @escaping @MainActor () -> Void,
55+
_ onDropped: @escaping @MainActor () -> Void
5556
) -> OwnerBoundNotificationPresentationResult
5657

5758
/// The default presenter: the real floating-bar card.
58-
static let floatingBarPresenter: Presenter = { ownerID, match, onPresented in
59+
static let floatingBarPresenter: Presenter = { ownerID, match, onPresented, onDropped in
5960
FloatingControlBarManager.shared.showNotification(
6061
ownerID: ownerID,
6162
title: "Connect \(match.entry.displayName) to Omi",
@@ -66,7 +67,8 @@ final class IntegrationNudgeCoordinator {
6667
telemetryID: match.entry.telemetryID,
6768
triggerID: match.trigger.id
6869
),
69-
onPresented: onPresented
70+
onPresented: onPresented,
71+
onDropped: onDropped
7072
)
7173
}
7274

@@ -232,11 +234,19 @@ final class IntegrationNudgeCoordinator {
232234
}
233235
}
234236

235-
/// Whether the user currently wants this feature at all. Read before any
236-
/// window inspection, not as part of the nudge decision.
237+
/// Whether a nudge is possible at all right now, independent of which window
238+
/// is in front. Read before any inspection, not as part of the nudge decision.
239+
///
240+
/// Signed-out and mid-onboarding are included deliberately: without them a
241+
/// signed-out user emits a suppressed event on every Finder activation — and
242+
/// Finder is activated constantly — for a state that cannot change until they
243+
/// sign in.
237244
private var isEnabledNow: Bool {
238245
let environment = environment()
239-
return environment.isFeatureEnabled && environment.notificationsEnabled
246+
return environment.isFeatureEnabled
247+
&& environment.notificationsEnabled
248+
&& environment.isOnboardingComplete
249+
&& ownerID() != nil
240250
}
241251

242252
/// Recognize the frontmost window and, if it earns one, offer its integration.
@@ -290,23 +300,33 @@ final class IntegrationNudgeCoordinator {
290300
/// Suppressed for a reason that cannot change while the user stays here.
291301
case settled(IntegrationNudgePolicy.Suppression)
292302

293-
/// Only an unrecognized window is worth looking at again. Once an answer
294-
/// exists, re-checking re-reads the window title and re-emits the funnel's
295-
/// denominator for a decision already made.
296-
var shouldKeepWatching: Bool { self == .noMatchYet }
303+
/// Whether re-checking this browser could still produce a different answer.
304+
///
305+
/// An unrecognized window obviously can — the user may not have opened the
306+
/// site yet. So can a settlement that was about *one integration*: someone
307+
/// whose Gmail is already connected should still be offered ChatGPT when
308+
/// they switch tabs. What ends the session is a global refusal or a card
309+
/// already delivered.
310+
var shouldKeepWatching: Bool {
311+
switch self {
312+
case .noMatchYet: return true
313+
case .settled(let reason): return IntegrationNudgePolicy.isPerIntegration(reason)
314+
case .delivered, .abandoned: return false
315+
}
316+
}
297317
}
298318

299-
/// Emit the suppression, unless it is one the user has permanently settled.
319+
/// Emit the suppression, unless it is an ambient state.
300320
///
301-
/// A permanent reason — connected, opted out, budget spent, feature off — is
302-
/// the same answer on every activation for the life of the install. Emitting
303-
/// it each time is unbounded volume, and it inflates the very denominator the
304-
/// event exists to provide.
321+
/// An ambient reason — connected, opted out, budget spent, signed out — is the
322+
/// same answer on every activation for as long as it holds. Emitting it each
323+
/// time is unbounded volume, and it inflates the very denominator the event
324+
/// exists to provide.
305325
private func report(
306326
_ reason: IntegrationNudgePolicy.Suppression,
307327
for match: IntegrationNudgeMatcher.Match
308328
) -> Outcome {
309-
if !IntegrationNudgePolicy.isPermanent(reason) {
329+
if !IntegrationNudgePolicy.isAmbient(reason) {
310330
AnalyticsManager.shared.integrationNudgeSuppressed(
311331
entry: match.entry,
312332
trigger: match.trigger,
@@ -360,17 +380,25 @@ final class IntegrationNudgeCoordinator {
360380
// as the screen-capture-reset defect (see
361381
// `NotificationService.screenCaptureResetShownKey`).
362382
var recorded = false
363-
let result = presenter(ownerID, match) { [weak self] in
383+
let onDropped: @MainActor () -> Void = { [weak self] in
384+
// Queue eviction or a stale owner: the budget correctly stays unspent, but
385+
// a nudge that was owed and never drawn still has to be visible.
364386
guard let self else { return }
365-
recorded = true
366-
let shownCountBefore = self.store.state(for: match.entry.telemetryID).shownCount
367-
self.store.recordDelivery(telemetryID: match.entry.telemetryID, now: self.now())
368-
AnalyticsManager.shared.integrationNudgeShown(
369-
entry: match.entry,
370-
trigger: match.trigger,
371-
shownCount: shownCountBefore + 1
372-
)
387+
_ = self.report(.barUnavailable, for: match)
373388
}
389+
let result = presenter(
390+
ownerID, match,
391+
{ [weak self] in
392+
guard let self else { return }
393+
recorded = true
394+
let shownCountBefore = self.store.state(for: match.entry.telemetryID).shownCount
395+
self.store.recordDelivery(telemetryID: match.entry.telemetryID, now: self.now())
396+
AnalyticsManager.shared.integrationNudgeShown(
397+
entry: match.entry,
398+
trigger: match.trigger,
399+
shownCount: shownCountBefore + 1
400+
)
401+
}, onDropped)
374402

375403
// `.presented` invokes the callback synchronously; `.queued` invokes it
376404
// later, if and only if the card reaches the screen. Either way the bar owns
@@ -482,6 +510,7 @@ final class IntegrationNudgeCoordinator {
482510
/// disconnect can start the pitch over instead of finding a spent budget.
483511
func noteConnected(route: IntegrationNudgeRoute) {
484512
store.recordConnected(telemetryID: route.telemetryID)
513+
IntegrationConnectionInspector.invalidateExportStatuses()
485514
}
486515

487516
// MARK: - Settings
@@ -492,7 +521,4 @@ final class IntegrationNudgeCoordinator {
492521
UserDefaults.standard.object(forKey: .integrationNudgesEnabled) as? Bool ?? true
493522
}
494523

495-
static func setFeatureEnabled(_ enabled: Bool) {
496-
UserDefaults.standard.set(enabled, forKey: .integrationNudgesEnabled)
497-
}
498524
}

desktop/macos/Desktop/Sources/IntegrationNudges/IntegrationNudgeMatcher.swift

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,18 +67,38 @@ enum IntegrationNudgeMatcher {
6767
}
6868

6969
/// Lowercased, trimmed, and stripped of the browser's own trailing chrome so
70-
/// a site name that a browser appended its product name after still reads as
71-
/// the end of the title.
70+
/// a site name the browser appended its product name after still reads as the
71+
/// end of the title.
72+
///
73+
/// On macOS the Chromium browsers do *not* append their name — a Chrome window
74+
/// showing Gmail is titled exactly "Inbox (12) - you@corp.com - Gmail". The
75+
/// ones that do are Firefox and its relatives, so those are what this strips.
76+
/// Chromium suffixes stay in the list only because they cost nothing and some
77+
/// builds and window managers do add them.
7278
static func normalizedTitle(_ title: String) -> String {
7379
var value = title.lowercased().trimmingCharacters(in: .whitespacesAndNewlines)
74-
for chrome in [" - google chrome", " — google chrome", " - brave", " - microsoft edge"] {
75-
if value.hasSuffix(chrome) {
80+
var didStrip = true
81+
while didStrip {
82+
didStrip = false
83+
for chrome in browserTitleChrome where value.hasSuffix(chrome) {
7684
value = String(value.dropLast(chrome.count)).trimmingCharacters(in: .whitespaces)
85+
didStrip = true
86+
break
7787
}
7888
}
7989
return value
8090
}
8191

92+
private static let browserTitleChrome = [
93+
" — mozilla firefox", " - mozilla firefox",
94+
" — firefox developer edition", " - firefox developer edition",
95+
" — zen browser", " - zen browser",
96+
" - vivaldi", " — vivaldi",
97+
" - google chrome", " — google chrome",
98+
" - brave", " — brave",
99+
" - microsoft edge", " — microsoft edge",
100+
]
101+
82102
static func isBrowser(bundleIdentifier: String?) -> Bool {
83103
guard let bundleIdentifier else { return false }
84104
return IntegrationNudgeCatalog.browserBundleIdentifiers.contains(bundleIdentifier)

desktop/macos/Desktop/Sources/IntegrationNudges/IntegrationNudgePolicy.swift

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -195,16 +195,33 @@ enum IntegrationNudgePolicy {
195195
return nil
196196
}
197197

198-
/// Suppressions that cannot change while the user stays on this Mac. Emitting
199-
/// a funnel event for these on every activation is unbounded volume for a
200-
/// decision that was made once, and it inflates the very denominator the
201-
/// event exists to provide.
202-
static func isPermanent(_ suppression: Suppression) -> Bool {
198+
/// Suppressions that persist across activations, so emitting a funnel event
199+
/// for them every time is unbounded volume for a decision made once — and it
200+
/// inflates the very denominator the event exists to provide. Finder alone is
201+
/// activated dozens of times an hour.
202+
static func isAmbient(_ suppression: Suppression) -> Bool {
203203
switch suppression {
204-
case .alreadyConnected, .featureDisabled, .optedOut, .connectorLifetimeCap:
204+
case .alreadyConnected, .featureDisabled, .optedOut, .connectorLifetimeCap,
205+
.notSignedIn, .onboardingIncomplete:
205206
return true
206-
case .notSignedIn, .onboardingIncomplete, .snoozed, .connectorCooldown,
207-
.globalCooldown, .dailyCap, .barUnavailable:
207+
case .snoozed, .connectorCooldown, .globalCooldown, .dailyCap, .barUnavailable:
208+
return false
209+
}
210+
}
211+
212+
/// Whether this suppression is about *one integration* rather than about the
213+
/// user or the moment.
214+
///
215+
/// It decides whether a browser is still worth watching: a user whose Gmail is
216+
/// already connected should still be offered ChatGPT when they switch tabs, so
217+
/// "this integration is settled" must not end recognition for the session. A
218+
/// global refusal — signed out, budget spent for the day — genuinely does.
219+
static func isPerIntegration(_ suppression: Suppression) -> Bool {
220+
switch suppression {
221+
case .alreadyConnected, .optedOut, .connectorLifetimeCap, .snoozed, .connectorCooldown:
222+
return true
223+
case .featureDisabled, .notSignedIn, .onboardingIncomplete, .globalCooldown, .dailyCap,
224+
.barUnavailable:
208225
return false
209226
}
210227
}

desktop/macos/Desktop/Sources/MemoryExportService.swift

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,9 +1056,14 @@ actor MemoryExportService {
10561056
/// there: an account switch racing this callback would otherwise clear the
10571057
/// wrong person's history.
10581058
private func clearIntegrationNudgeHistory(for destination: MemoryExportDestination) {
1059-
let connectionOwnerID = defaults.string(forKey: DefaultsKey.authUserId.rawValue)
10601059
Task { @MainActor in
1061-
guard RuntimeOwnerIdentity.currentOwnerId() == connectionOwnerID else { return }
1060+
// Both sides resolve through `RuntimeOwnerIdentity`, so the comparison is
1061+
// like-for-like: reading the raw `authUserId` default here would miss the
1062+
// trimming, the non-production automation override, and the nil returned
1063+
// during an owner transition, and would then clear the wrong account's
1064+
// history on exactly the builds this is tested on.
1065+
let connectionOwnerID = RuntimeOwnerIdentity.currentOwnerId()
1066+
guard connectionOwnerID != nil else { return }
10621067
IntegrationNudgeCoordinator.shared.noteConnected(route: .exportDestination(destination.rawValue))
10631068
}
10641069
}

desktop/macos/Desktop/Tests/IntegrationNudgeCoordinatorTests.swift

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ final class IntegrationNudgeCoordinatorTests: XCTestCase {
4242
IntegrationNudgeCoordinator(
4343
store: IntegrationNudgeStore(defaults: defaults, ownerID: ownerID),
4444
now: { self.now },
45-
presenter: { _, _, onPresented in
45+
presenter: { _, _, onPresented, _ in
4646
presentedCount.value += 1
4747
onPresented()
4848
return result
@@ -89,7 +89,7 @@ final class IntegrationNudgeCoordinatorTests: XCTestCase {
8989
let coordinator = IntegrationNudgeCoordinator(
9090
store: IntegrationNudgeStore(defaults: defaults, ownerID: "user-a"),
9191
now: { self.now },
92-
presenter: { _, _, _ in .rejectedOwnerChange },
92+
presenter: { _, _, _, _ in .rejectedOwnerChange },
9393
ownerID: { "user-a" },
9494
environment: { .init(isFeatureEnabled: true, notificationsEnabled: true, isOnboardingComplete: true) }
9595
)
@@ -111,7 +111,7 @@ final class IntegrationNudgeCoordinatorTests: XCTestCase {
111111
let coordinator = IntegrationNudgeCoordinator(
112112
store: IntegrationNudgeStore(defaults: defaults, ownerID: "user-a"),
113113
now: { self.now },
114-
presenter: { _, _, _ in .queued },
114+
presenter: { _, _, _, _ in .queued },
115115
ownerID: { "user-a" },
116116
environment: { .init(isFeatureEnabled: true, notificationsEnabled: true, isOnboardingComplete: true) }
117117
)
@@ -132,7 +132,7 @@ final class IntegrationNudgeCoordinatorTests: XCTestCase {
132132
let coordinator = IntegrationNudgeCoordinator(
133133
store: IntegrationNudgeStore(defaults: defaults, ownerID: "user-a"),
134134
now: { self.now },
135-
presenter: { _, _, onPresented in
135+
presenter: { _, _, onPresented, _ in
136136
present.value = onPresented
137137
return .queued
138138
},
@@ -234,7 +234,7 @@ final class IntegrationNudgeCoordinatorTests: XCTestCase {
234234
let coordinator = IntegrationNudgeCoordinator(
235235
store: IntegrationNudgeStore(defaults: makeDefaults(), ownerID: "user-a"),
236236
now: { self.now },
237-
presenter: { _, _, _ in
237+
presenter: { _, _, _, _ in
238238
presented.value += 1
239239
return .presented
240240
},
@@ -264,20 +264,24 @@ final class IntegrationNudgeCoordinatorTests: XCTestCase {
264264

265265
/// Once a window has an answer, re-checking re-reads the title and re-emits
266266
/// the funnel's denominator for a decision already made.
267-
func testASettledOutcomeStopsTheReCheckLoop() throws {
267+
func testOnlyAGlobalAnswerEndsTheReCheckLoop() throws {
268268
XCTAssertFalse(IntegrationNudgeCoordinator.Outcome.delivered.shouldKeepWatching)
269269
XCTAssertFalse(IntegrationNudgeCoordinator.Outcome.abandoned.shouldKeepWatching)
270-
XCTAssertFalse(
271-
IntegrationNudgeCoordinator.Outcome.settled(.connectorCooldown).shouldKeepWatching)
270+
XCTAssertFalse(IntegrationNudgeCoordinator.Outcome.settled(.dailyCap).shouldKeepWatching)
272271
XCTAssertTrue(IntegrationNudgeCoordinator.Outcome.noMatchYet.shouldKeepWatching)
272+
// The case that matters: Gmail already connected must not stop the session,
273+
// or switching to a ChatGPT tab in the same browser never gets an offer.
274+
XCTAssertTrue(
275+
IntegrationNudgeCoordinator.Outcome.settled(.alreadyConnected).shouldKeepWatching)
276+
XCTAssertTrue(IntegrationNudgeCoordinator.Outcome.settled(.optedOut).shouldKeepWatching)
273277
}
274278

275279
/// A browser tab the user has not opened yet is the one case worth watching.
276280
func testAnUnrecognizedWindowKeepsTheLoopAlive() async {
277281
let coordinator = IntegrationNudgeCoordinator(
278282
store: IntegrationNudgeStore(defaults: makeDefaults(), ownerID: "user-a"),
279283
now: { self.now },
280-
presenter: { _, _, _ in .presented },
284+
presenter: { _, _, _, _ in .presented },
281285
ownerID: { "user-a" },
282286
environment: { .init(isFeatureEnabled: true, notificationsEnabled: true, isOnboardingComplete: true) },
283287
frontmostBundleID: { "com.google.Chrome" },
@@ -301,7 +305,7 @@ final class IntegrationNudgeCoordinatorTests: XCTestCase {
301305
IntegrationNudgeCoordinator(
302306
store: IntegrationNudgeStore(defaults: makeDefaults(), ownerID: "user-a"),
303307
now: { self.now },
304-
presenter: { _, _, onPresented in
308+
presenter: { _, _, onPresented, _ in
305309
presented.value += 1
306310
onPresented()
307311
return .presented

0 commit comments

Comments
 (0)