-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat: add platform and install_type as MetaMetrics user traits #39606
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
Add platform (browser type) and install_type (installation source) as user traits sent via identify() instead of as properties on every event. This is the appropriate approach for static installation properties. - Add initInstallType() to cache install type from browser.management API - Add Platform and InstallType to MetaMetricsUserTrait enum - Update _buildUserTraitsObject to include these traits - Update unit test expectations
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [74448f0]
UI Startup Metrics (1388 ± 119 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
MajorLift
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.
Looks good! Thoughts on enforcing + documenting the range of platform and install types with types?
Add type-safe constants and types for platform and install_type user traits instead of using plain strings. This provides compile-time validation and better IDE support. - Add PLATFORM and INSTALL_TYPE const objects in shared/constants/app.ts - Export Platform and InstallType types derived from these objects - Update getPlatform() and getInstallType() to use proper return types - Update MetaMetricsUserTraits to use typed fields Co-authored-by: Cursor <[email protected]>
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Remove duplicated install type fetching logic from setupSentry.js and use the shared functions from util.ts instead. This ensures a single source of truth for install type caching and eliminates redundant API calls to browser.management.getSelf(). Co-authored-by: Cursor <[email protected]>
Move install type caching logic to a separate lightweight module (install-type.ts) that only imports webextension-polyfill and constants. This prevents the sentry bundle from pulling in heavy dependencies like reselect through util.ts, which was causing browserify parsing errors. - Create app/scripts/lib/install-type.ts with initInstallType/getInstallType - Update setupSentry.js to import from install-type instead of util - Re-export from util.ts for backward compatibility
Builds ready [f526ca5]
UI Startup Metrics (1378 ± 117 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
MajorLift
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.
Nice LGTM!
Builds ready [cbb9c64]
UI Startup Metrics (1355 ± 87 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Add platform (browser type) and install_type (installation source) as user traits sent via identify() instead of as properties on every event. This is the appropriate approach for static installation properties.
Changelog
CHANGELOG entry: null
Related issues
Fixes:
Manual testing steps
platformandinstall_typedefined in its user profile propertiesScreenshots/Recordings
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Medium risk because it touches analytics trait generation and Sentry error-tagging, plus adds an early background initialization side-effect via
browser.management.getSelf()(albeit fire-and-forget). Functionality is additive but could impact telemetry correctness and startup behavior across browsers.Overview
Adds
platformandinstall_typeas new MetaMetrics user traits (viaidentify()), and wiresMetaMetricsController._buildUserTraitsObjectto populate them usinggetPlatform()and a new cachedgetInstallType().Introduces a dedicated
install-typehelper (withinitInstallType()+ cached value) and initializes it early in bothbackground.jsstartup and Sentry setup; Sentry reports now tag/includeinstallTypevia the cache rather than an inline async fetch. Updates shared constants/types to formalizePlatform/InstallTypeand adjusts MetaMetrics controller tests accordingly.Written by Cursor Bugbot for commit cbb9c64. This will update automatically on new commits. Configure here.