🧹 [code health] Implement device disconnected analytics tracking - #12486
🧹 [code health] Implement device disconnected analytics tracking#12486undivisible wants to merge 5 commits into
Conversation
Failure-Class: none Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Failure-Class: none Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bebb31f3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // TODO: Track analytics when AnalyticsManager supports device events | ||
| // AnalyticsManager.shared.deviceDisconnected() | ||
| AnalyticsManager.shared.deviceDisconnected() |
There was a problem hiding this comment.
When a macOS-only user connects and later disconnects, this emits Device Disconnected, but the successful connection path in DeviceProvider.connect still leaves deviceConnected commented out. The shared PostHog connection-balance query counts unique users for Device Connected divided by Device Disconnected, so desktop users now contribute only to the denominator and can falsely lower the ratio or trigger its alert; enable the matching connection event in the same change.
Useful? React with 👍 / 👎.
|
|
||
| // TODO: Track analytics when AnalyticsManager supports device events | ||
| // AnalyticsManager.shared.deviceDisconnected() | ||
| AnalyticsManager.shared.deviceDisconnected() |
There was a problem hiding this comment.
When unpair() is called with an active connection, DeviceSessionCoordinator.unpair() invokes onSessionEnded, whose callback already calls resetSessionPresentation(), and then DeviceProvider.unpair() calls the same helper again with scheduleReconnectNotification: false. Because telemetry is now inside that helper, one user action records two Device Disconnected events; emit from a single authoritative lifecycle transition or suppress tracking during the second presentation reset.
Useful? React with 👍 / 👎.
| func deviceDisconnected() { | ||
| PostHogManager.shared.track("Device Disconnected") | ||
| } |
There was a problem hiding this comment.
This commit changes production files under Desktop/Sources without adding an unreleased changelog fragment. Running python3 .github/scripts/check-desktop-changelog.py --base b594fe5a^ --head b594fe5a fails for exactly these two files, so the PR contract and post-merge Release Eligibility lane remain red; add a user-facing fragment or {"kind":"none"} if this is internal-only.
AGENTS.md reference: desktop/macos/AGENTS.md:L504-L512
Useful? React with 👍 / 👎.
|
|
||
| // TODO: Track analytics when AnalyticsManager supports device events | ||
| // AnalyticsManager.shared.deviceDisconnected() | ||
| AnalyticsManager.shared.deviceDisconnected() |
There was a problem hiding this comment.
This changes production behavior without changing any test, and there is no disconnect-telemetry capture seam analogous to setDevicePairingTelemetryCaptureForTests. Add a behavioral DeviceProvider test that drives the production disconnect and unpair paths and asserts the exact event counts; that coverage would expose both duplicate active-unpair emission and false emission when unpairing an already-disconnected persisted device.
AGENTS.md reference: AGENTS.md:L30-L35
Useful? React with 👍 / 👎.
| func deviceDisconnected() { | ||
| PostHogManager.shared.track("Device Disconnected") | ||
| } |
There was a problem hiding this comment.
After adding this emitter, web/admin/docs/posthog-events.md still lists Device Disconnected as owned only by Flutter mobile and defines its authoritative emitter as the mobile BLE callback. Alert investigation and platform-level interpretation will therefore miss that macOS now contributes these events—including intentional teardown paths—so add a macOS row describing the actual emitter and lifecycle contract, as the dictionary already does for macOS Device Paired.
Useful? React with 👍 / 👎.
Line-Count-Exception: desktop/macos/Desktop/Sources/AnalyticsManager.swift | 1655 -> 1659 | Added missing Analytics tracking method Failure-Class: none Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks @undivisible — the intent here is right and this closes a real TODO; both desktop Swift CI lanes pass, and resetSessionPresentation (DeviceProvider.swift:378) is the correct single teardown point for this call. I verified a few concrete issues before merge, though:
1. Hygiene CI failure — the Line-Count-Exception is in the wrong place (one-line fix). check_product_file_line_count_ratchet.py reads exceptions only from the PR body (requires_pr_body: true), not from the commit message, so the trailer on commit 9aa1793 is invisible to it. I reproduced the failure locally against base e1eca38: pasting this single line into the PR description turns the check green:
Line-Count-Exception: desktop/macos/Desktop/Sources/AnalyticsManager.swift | 1655 -> 1659 | Added missing device-disconnect analytics tracking for the device lifecycle
2. Unpair with an active connection emits Device Disconnected twice. DeviceProvider.unpair() (DeviceProvider.swift:361) awaits sessionCoordinator.unpair(), which fires onSessionEnded (DeviceSessionCoordinator.swift:295) → resetSessionPresentation() → first emission at DeviceProvider.swift:388. The provider then calls resetSessionPresentation(scheduleReconnectNotification: false) again at DeviceProvider.swift:366 → second emission. One user action, two events.
3. Unpairing an already-disconnected persisted device emits a phantom event. With no active connection, DeviceSessionCoordinator.unpair() skips the if let connection branch and never fires onSessionEnded, but the provider's direct call at DeviceProvider.swift:366 still emits even though nothing disconnected.
Emitting from one authoritative transition (e.g. only the coordinator's onSessionEnded, or suppressing the intentional-unpair path) resolves 2 and 3 together.
4. Missing changelog fragment (verified failing). check-desktop-changelog.py --base e1eca38 --head 9aa1793 exits 1: desktop production edits need a fragment under desktop/macos/changelog/unreleased/ — {"kind":"none"} is fine for internal-only telemetry.
5. Denominator-only emission into a shared metric. deviceConnected is still commented out at DeviceProvider.swift:336 while Device Disconnected goes live. Per web/admin/docs/posthog-events.md, Device Connected and Device Disconnected feed one connection-balance contract — desktop would now contribute disconnects but never connects. Either enable the connect side here or flag the skew to the alert owner. The doc also still lists Device Disconnected as Flutter-mobile-only, so a macOS row is due once this lands.
6. No behavioral coverage. There is no test for the emission path and no disconnect capture seam analogous to setDevicePairingTelemetryCaptureForTests (AnalyticsManager.swift:44-58). A DeviceProvider test driving disconnect plus both unpair paths and asserting exact event counts would have caught 2 and 3.
Beyond the mechanical fixes, one judgment call for a maintainer: whether desktop should join the shared Device Disconnected stream before deviceConnected parity lands, given the shared connection-balance contract reads both sides.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Git-on-my-level
added
needs-tests
Move disconnect telemetry to onSessionEnded so unpair cannot double-fire or emit a phantom event, enable matching Device Connected, and add a changelog fragment plus a DeviceProvider test for exact event counts.
|
Thanks @undivisible — the PR-body
The behavioral items from my earlier review still stand on 9aa1793: the double Leaving as changes-requested on the existing review. For the maintainer judgment call — whether desktop should join the shared by AI on behalf of David — if you need David's attention urgently, please @Git-on-my-level and escalate with |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6a62ed9a-69d3-44bc-90f0-22831d185d57) |
|
format drift on DeviceProviderTests.swift, Device Disconnected once per session is in d8eebb5 + this format commit. |
Dismissing as resolved on 140cbfe: disconnect telemetry now emits once per ended session from onSessionEnded only (no double emission on unpair, no phantom emission on unpair of an already-disconnected device), deviceConnected is enabled, the Line-Count-Exception trailer matches the real 1655 -> 1679 growth, a valid kind:none changelog fragment exists, and testDisconnectAndUnpairEmitDeviceDisconnectedOnce pins the exact event counts. A fresh review covers new findings on this head.
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks @undivisible — this head resolves everything raised in the earlier reviews, and the emission design is now right. Verified on 140cbfe:
DeviceProvider.swift: thedeviceDisconnected()call now lives in thecoordinator.onSessionEndedclosure (~line 152) andresetSessionPresentation()no longer emits, so unpair of an active connection fires exactly once, and unpair of an already-disconnected device fires nothing (DeviceSessionCoordinator.unpair()only invokesonSessionEndedinside itsif let connectionbranch, DeviceSessionCoordinator.swift:285-296).deviceConnectedis enabled at line 339. The PR-bodyLine-Count-Exceptiontrailer now matches the real growth (1655 -> 1679, verified), and the{"kind":"none"}fragment satisfiescheck-desktop-changelog.py.DeviceProviderTests.swift:testDisconnectAndUnpairEmitDeviceDisconnectedOncepins the exact event sequence for connect -> disconnect, connect -> unpair, and unpair-of-disconnected (expects no event), with the new capture seam reset intearDown. This is precisely the coverage the earlier reviews asked for.
Two things before merge, both small:
1. The macOS Device Connected payload doesn't match the contract in the docs row this PR edits. web/admin/docs/posthog-events.md now lists macOS as an owning surface for Device Connected with "Event: closed device_vendor, enum-name type; … Person: device_vendor". But AnalyticsManager.deviceConnected (AnalyticsManager.swift:566-575) sends device_type + device_name, no device_vendor, and no person property. Concretely:
- the vendor-sliced weekly analysis the row links to cannot segment macOS events by vendor;
device_nameis free text (user-renamable BLE name), while that file's maintenance contract keeps dimensions bounded/content-free and excludes device names from the properties documented there;- the sibling
devicePairingReady(AnalyticsManager.swift:531-537) already shows the intended shape:device.type.analyticsVendorSlug+device.type.rawValue.
Mirroring that bounded set (device_vendor, device_type) and dropping device_name would make the code match the row this PR just edited; alternatively, if macOS deliberately needs different properties, the row should describe the macOS payload explicitly instead of inheriting the mobile contract.
2. No CI has run on the last two commits. Desktop Swift CI was last green on 9aa1793, which predates the emission move and the new test; d8eebb5 and 140cbfe have no workflow runs at all. The PR description mentions a local test.sh run — please make sure the Desktop Swift CI lane runs on this head before merge.
Non-blocking observation: deviceConnected fires at the end of the happy path (after checkFirmwareUpdates), so a session that becomes active at the coordinator level but fails post-connect setup will still emit Device Disconnected on teardown without a preceding Device Connected — a small downward skew on the connected/disconnected ratio in that edge. Fine to leave as is; worth knowing when calibrating the ratio alert.
The payload-shape choice in (1) is the one piece that needs a maintainer decision, since it defines the shared connection-funnel contract; everything else is mechanical.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Git-on-my-level
removed
the
needs-tests
2 participants
🎯 What: Implemented the
deviceDisconnected()method inAnalyticsManagerand uncommented its usage inDeviceProvider.💡 Why: To accurately track when a device disconnects in PostHog analytics, which was previously marked as a TODO. This improves visibility into device lifecycle events.
✅ Verification: Added the tracking call, verified its syntax, and ran the desktop macOS tests (
test.sh).✨ Result: Disconnect events are now correctly recorded via PostHog.
Line-Count-Exception: desktop/macos/Desktop/Sources/AnalyticsManager.swift | 1655 -> 1679 | Device Connected/Disconnected telemetry plus test capture seam
Failure-Class: none
PR created automatically by Jules for task 1012325855911202136 started by @undivisible
Note
Low Risk
Analytics-only changes on the macOS device lifecycle path, with unit tests guarding duplicate disconnect events; no auth, data handling, or connection logic changes beyond where events fire.
Overview
macOS desktop now emits Device Connected and Device Disconnected through
AnalyticsManager, aligning with the mobile BLE funnel documented inposthog-events.md.Device Connected fires after a successful
DeviceProvider.connect(withdevice_typeanddevice_name). Device Disconnected is emitted once per ended session fromDeviceSessionCoordinator.onSessionEnded—including explicit disconnect, unpair while connected, and unexpected drops—not fromresetSessionPresentation, so unpairing an already-disconnected device does not produce a phantom disconnect.A test-only capture seam on
AnalyticsManagerbacks a new unit test that asserts connect/disconnect/unpair ordering and single-disconnect semantics.Reviewed by Cursor Bugbot for commit 140cbfe. Configure here.