Skip to content

fix: ensure firmware version is tracked in Device Connected Mixpanel event#4758

Merged
mdmohsin7 merged 2 commits intomainfrom
feat/track-firmware-version
Mar 20, 2026
Merged

fix: ensure firmware version is tracked in Device Connected Mixpanel event#4758
mdmohsin7 merged 2 commits intomainfrom
feat/track-firmware-version

Conversation

@krushnarout
Copy link
Copy Markdown
Member

Summary

  • The Device Connected Mixpanel event already included btDevice.toJson() which contains firmwareRevision, but the value was always stale or 'Unknown' due to timing issues
  • setConnectedDevice was void async, so getDeviceInfo() (which fetches firmware over BLE) could never be awaited — the Mixpanel event fired before firmware was fetched
  • The early-return path in getDeviceInfo() (when firmware was already known) skipped saving to SharedPreferences, so the Mixpanel event read old persisted data

Changes

  • Changed setConnectedDevice from void to Future<void> so callers can await it
  • await setConnectedDevice() in both connection paths before firing MixpanelManager().deviceConnected()
  • Persist pairedDevice to SharedPreferences on the early-return path in getDeviceInfo()

Closes #4228

Test plan

  • Connect a device and verify the Device Connected Mixpanel event contains the correct firmwareRevision value (not 'Unknown')
  • Reconnect a device that already has cached firmware info and verify the event still has the correct firmware version
  • Verify no regression in device connection flow timing

🤖 Generated with Claude Code

…l event

The Device Connected event already included btDevice.toJson() which contains
firmwareRevision, but it was always stale or 'Unknown' due to timing issues:
- setConnectedDevice was void async so getDeviceInfo() couldn't be awaited
- The early return path in getDeviceInfo() skipped saving to SharedPreferences

Changes:
- Change setConnectedDevice to Future<void> so callers can await it
- Await setConnectedDevice before firing the Mixpanel event in both connection paths
- Persist pairedDevice to SharedPreferences on the early return path in getDeviceInfo()

Closes #4228

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to resolve a timing issue where the Device Connected Mixpanel event was being sent with stale firmware information. The changes correctly make setConnectedDevice awaitable and ensure it's awaited before the event is fired. This allows for the device information, including the firmware version, to be fetched and persisted. However, I've identified a critical race condition in how the device information is saved to SharedPreferences. The current implementation uses a synchronous setter for an asynchronous operation, which could result in the Mixpanel event still capturing stale data, thereby undermining the intended fix. My review includes a specific comment and code suggestion to address this, aligning with best practices for provider state management and the use of existing helper functions.

Comment thread app/lib/providers/device_provider.dart
@beastoin
Copy link
Copy Markdown
Collaborator

Hey @krushnarout, sorry about the delay on this — @beastoin doesn't have bandwidth to review it right now and doesn't have any plans to in the near future.

@aaravgarg, could you take a look and make the call on this one? Feel free to approve and merge if it looks good to you. Thanks both!

@beastoin
Copy link
Copy Markdown
Collaborator

Hey 👋 — thanks for putting this together! Before we can review, could you share a quick live demo (screenshot, screen recording, or terminal output) showing this working on your local or dev environment?

In the AI era, writing code is the easy part — what really makes a PR stand out is proof that it works end-to-end. A short video or even a screenshot goes a long way in helping reviewers feel confident about merging.

Feel free to update this PR whenever you have something to show. Thanks! 🙏

Copy link
Copy Markdown
Collaborator

@beastoin beastoin left a comment

Choose a reason for hiding this comment

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

Review by @ryo (community PR reviewer)

The approach is correct — making setConnectedDevice awaitable so firmware version is fetched before the Mixpanel event fires.

Code looks good, but two items remain before this can be approved:

  1. Demo evidence still needed@beastoin requested a screenshot/video showing the correct firmware version appearing in the Mixpanel event (Feb 19). Without evidence this works end-to-end, we can't approve.

  2. Gemini flagged a race condition with SharedPreferences in the early-return path of getDeviceInfo() — worth addressing or explaining why it's not an issue.

@krushnarout — are you still planning to add demo evidence? Happy to help if you need anything.

Future getDeviceInfo() async {
if (connectedDevice != null) {
if (pairedDevice?.firmwareRevision != null && pairedDevice?.firmwareRevision != 'Unknown') {
SharedPreferencesUtil().btDevice = pairedDevice!;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not sure why we need to do this here? Is this part of the fix?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, without it it would return immediately without saving to SharedPreferences. So deviceConnected() would still read the stale firmware version from there

@mdmohsin7 mdmohsin7 merged commit 7acff84 into main Mar 20, 2026
1 check passed
@mdmohsin7 mdmohsin7 deleted the feat/track-firmware-version branch March 20, 2026 07:25
Glucksberg pushed a commit to Glucksberg/omi-local that referenced this pull request Apr 28, 2026
…event (BasedHardware#4758)

## Summary
- The `Device Connected` Mixpanel event already included
`btDevice.toJson()` which contains `firmwareRevision`, but the value was
always stale or `'Unknown'` due to timing issues
- `setConnectedDevice` was `void async`, so `getDeviceInfo()` (which
fetches firmware over BLE) could never be awaited — the Mixpanel event
fired before firmware was fetched
- The early-return path in `getDeviceInfo()` (when firmware was already
known) skipped saving to SharedPreferences, so the Mixpanel event read
old persisted data

### Changes
- Changed `setConnectedDevice` from `void` to `Future<void>` so callers
can await it
- `await setConnectedDevice()` in both connection paths before firing
`MixpanelManager().deviceConnected()`
- Persist `pairedDevice` to SharedPreferences on the early-return path
in `getDeviceInfo()`

Closes BasedHardware#4228

## Test plan
- [ ] Connect a device and verify the `Device Connected` Mixpanel event
contains the correct `firmwareRevision` value (not `'Unknown'`)
- [ ] Reconnect a device that already has cached firmware info and
verify the event still has the correct firmware version
- [ ] Verify no regression in device connection flow timing

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track firmware version on device-app connect in Mixpanel

3 participants