Skip to content

Conversation

@krushnarout
Copy link
Member

When connecting device to app we already track that in Mixpanel—add a parameter for firmware version on that event so >we can see if connection issues correlate with people not updating firmware vs other problems.

we can't track firmware version at connect time because that time firmware version is not guaranteed to be available

@krushnarout krushnarout linked an issue Jan 19, 2026 that may be closed by this pull request
Copy link
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 adds firmware version tracking to the 'Device Disconnected' analytics event. The changes in mixpanel.dart are correct. However, there is a critical issue in device_provider.dart where the logic to retrieve the firmware version is flawed, as it incorrectly inlines fallback logic for a complex case. It doesn't correctly fall back to other sources if the primary source provides an invalid but non-null value like 'Unknown'. I've provided a suggestion to fix this to ensure accurate data tracking, aligning with best practices for handling complex fallbacks.

Comment on lines 284 to 286
final String trackFirmware = connectedDevice?.firmwareRevision ??
pairedDevice?.firmwareRevision ??
SharedPreferencesUtil().btDevice.firmwareRevision;
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The current logic for retrieving the firmware version might not work as intended. The null-coalescing operator (??) only checks for null, but the firmwareRevision getter can return non-null but invalid values like 'Unknown' or an empty string. If connectedDevice.firmwareRevision is 'Unknown', the subsequent fallbacks to pairedDevice and SharedPreferencesUtil will not be triggered, which could lead to incorrect data being tracked. To ensure you get the first valid firmware version available, you should check for these invalid string values as well. This aligns with the principle of preferring existing helper functions for complex fallback logic to avoid subtle bugs from incorrect inlining.

Suggested change
final String trackFirmware = connectedDevice?.firmwareRevision ??
pairedDevice?.firmwareRevision ??
SharedPreferencesUtil().btDevice.firmwareRevision;
final String trackFirmware = [connectedDevice?.firmwareRevision, pairedDevice?.firmwareRevision, SharedPreferencesUtil().btDevice.firmwareRevision].firstWhere((v) => v != null && v.isNotEmpty && v != 'Unknown', orElse: () => 'Unknown');
References
  1. Prefer using existing helper functions over inlining their logic, especially when they handle complex cases like fallbacks or error handling. Inlining can introduce subtle bugs by missing parts of the original logic.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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

2 participants