-
Notifications
You must be signed in to change notification settings - Fork 3
feat(cli_tools): Enable IP geotracking in analytics events #93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughMixPanelAnalytics constructor was changed: Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MixPanelAnalytics
participant HTTP
Caller->>MixPanelAnalytics: new MixPanelAnalytics(..., endpoint?, disableIpTracking?)
Note right of MixPanelAnalytics: constructor
MixPanelAnalytics->>MixPanelAnalytics: _buildEndpoint(baseEndpoint, disableIpTracking)\n(parse, preserve queries, set ip=0/1)
MixPanelAnalytics->>MixPanelAnalytics: store `_endpoint` as Uri
Caller->>MixPanelAnalytics: trackEvent(...)
MixPanelAnalytics->>MixPanelAnalytics: _quietPost(payload)
MixPanelAnalytics->>HTTP: POST `_endpoint` with payload
HTTP-->>MixPanelAnalytics: response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli_tools/lib/src/analytics/analytics.dart (1)
34-34: Consider using positive boolean naming for clarity.The parameter
disableIpTrackinguses negative naming, which can lead to double-negative logic (e.g.,!disableIpTrackingto check if enabled). Consider renaming toenableIpTrackingwith a default oftruefor improved readability.If you prefer positive naming, apply this diff:
- final bool disableIpTracking = false, + final bool enableIpTracking = true, }) : _uniqueUserId = uniqueUserId, _projectToken = projectToken, _version = version, _endpoint = (endpoint ?? _defaultEndpoint) + - (disableIpTracking ? '?ip=0' : '?ip=1'), + (enableIpTracking ? '?ip=1' : '?ip=0'), _timeout = timeout;Note: This would need to be combined with the query string fix from the previous comment.
Also applies to: 38-39
SandPod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any custom parameters in Serverpod, so the CodeRabbit issue is not critical for us.
Although it would probably be good to use that approach (uri.replace) to support any additional parameters.
To enable IP-based geotracking of the client origin, MixPanel requires the URI query parameter
ipto be set to 1.Summary by CodeRabbit
New Features
Improvements