feat(analytics-controller): update controller and adapter interface for mobile integration [Phase 1.4] #7080
Conversation
- Add onSetupCompleted lifecycle hook to AnalyticsPlatformAdapter interface - Remove add method from interface (breaking change) - Change AnalyticsPlatformAdapter from type alias to interface - Add UUIDv4 validation for analyticsId with error handling - Add AnalyticsUserTraits type for better semantic separation - Rename trackEvent to track and trackPage to view in adapter interface - Update controller to call onSetupCompleted after initialization - Add comprehensive tests for lifecycle hook and validation - Update documentation with lifecycle hook usage examples
NicolasMassart
changed the title
…y method - Add isOptedIn method and corresponding action type - Refactor identify method to use analyticsId from state instead of userId parameter - Update tests to use realistic traits (ENABLE_OPENSEA_API, NFT_AUTODETECTION) instead of email - Update UUID test example from v1 to v5 - Remove obsolete tests - Fix formatting issues
…TrackingEvent object - Add AnalyticsJsonValue and AnalyticsJsonMap types similar to legacy types for easier migration - Add AnalyticsTrackingEvent type matching legacy ITrackingEvent structure - Update trackEvent method to accept AnalyticsTrackingEvent instead of separate eventName and properties - Implement same logic as legacy trackEvent (handles hasProperties, isAnonymous, sensitiveProperties) - Remove redundant saveDataRecording parameter (now part of event object) - Convert interfaces to type aliases for consistency - Update tests
…m:MetaMask/core into refactor/7079_update_analytics_controller
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Hi @NicolasMassart! Thanks for the ping. I had some comments and suggestions below.
| * Represents values that can be passed as properties to the event tracking function. | ||
| * Similar to JsonValue from Segment SDK but decoupled for platform agnosticism. | ||
| */ | ||
| type AnalyticsJsonValue = |
There was a problem hiding this comment.
Is it possible to simplify this type and use Json? I see that there are slight differences between this type and Json, namely that objects are allowed to have number keys. However if this data really gets serialized to JSON, wouldn't the keys get converted to strings anyway?
There was a problem hiding this comment.
Here are the resons why I used this explicit type:
Using Record<string, Json> (current):
✅ Explicit: must be an object whith named props of Json type value
✅ Type-safe: rejects arrays, primitives, null
✅ Consistent with BaseController.StateConstraint and ApprovalController
Using Json directly:
✅ Simpler
✅ Matches runtime JSON serialization behavior
❌ Less type-safe: allows arrays, primitives, null
❌ Less explicit intent as naming the AnalyticsEventProperties and AnalyticsUserTraits types allows to clearly identify the usage in controller.
My recommendation:
Keep Record<string, Json> for type safety and consistency. If you really prefers Json, we can switch, but it weakens compile-time guarantees.
packages/analytics-controller/src/AnalyticsPlatformAdapter.types.ts
Outdated
Show resolved
Hide resolved
…and extract validation - Remove isAnonymous field, derive sensitivity from sensitiveProperties presence - Extract state validation to analyticsStateValidator module (called before super) - Add selectors module for state access - Add comprehensive unit tests for validator and selectors - Update documentation for clarity
…m:MetaMask/core into refactor/7079_update_analytics_controller # Conflicts: # packages/analytics-controller/src/selectors.test.ts
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ...event.properties, | ||
| ...event.sensitiveProperties, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: saveDataRecording property in event object is ignored
The AnalyticsTrackingEvent type includes a saveDataRecording property, but trackEvent ignores it and does not pass it to the platform adapter. This results in the property being silently discarded, potentially breaking intended functionality like session recording controls.
| ...event.properties, | ||
| ...event.sensitiveProperties, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: saveDataRecording property is ignored
The AnalyticsTrackingEvent type includes a saveDataRecording property, but the trackEvent method ignores it and does not pass it to the platform adapter or use it in any logic. This functionality appears to be unimplemented or lost during refactoring.
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
More suggestions, but all of them are super minor, so I will approve.
| * @param state - The analytics controller state to validate | ||
| * @throws {Error} If analyticsId is missing, invalid, or not a UUIDv4 | ||
| */ | ||
| export function validateAnalyticsState(state: AnalyticsControllerState): void { |
There was a problem hiding this comment.
Nit: Perhaps we should call this validateAnalyticsControllerState?
| * @param state - The current controller state | ||
| * @returns `true` if analytics tracking should be enabled, `false` otherwise | ||
| */ | ||
| export function computeEnabledState(state: AnalyticsControllerState): boolean { |
There was a problem hiding this comment.
Nit: Since we have a selector do we need this file? My thought is that the selector can be used not only in Redux but also on its own.
There was a problem hiding this comment.
Nit: Thoughts on calling this file validateAnalyticsState.ts to match the function being exported? The current name of the file makes it sounds like this exports a class or something.
There was a problem hiding this comment.
Nit: If we want to keep this file, thoughts on renaming it computeEnabledState to match the function being exported? The current name makes it sound like this exports a class or something.
| @@ -0,0 +1,44 @@ | |||
| import type { AnalyticsControllerState } from './AnalyticsController'; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Nit: If we do keep this function it is necessary to have so much documentation for it? I'm all for explaining how things work, but the code seems pretty simple to me, and there is a lot of redundant information here that can be easily gleaned already. (For instance, the fact that we are accessing state implies that whenever state changes, the return value of this function may change. And the code already says state.optedInForRegularAccount || state.optedInForSocialAccount, we don't need to repeat that in the documentation. Finally, we should not need to list all of the possible return values for this function; maintainers should be able to look at tests to discover this.)
|
|
||
| it('uses default analyticsId when not provided in partial state', () => { | ||
| describe('trackEvent', () => { | ||
| it('tracks event when enabled via regular opt-in', () => { |
There was a problem hiding this comment.
| it('tracks event when enabled via regular opt-in', () => { | |
| it('calls platform adapter to track event when enabled via regular opt-in', () => { |
|
|
||
| it('uses provided analyticsId when passed in state at initialization', () => { | ||
| const customId = '550e8400-e29b-41d4-a716-446655440000'; | ||
| it('tracks event when enabled via social opt-in', () => { |
There was a problem hiding this comment.
| it('tracks event when enabled via social opt-in', () => { | |
| it('tracks event via platform adapter when enabled via social opt-in', () => { |
| }); | ||
|
|
||
| it('tracks event with default empty properties when properties not provided', () => { | ||
| it('does not track event when disabled', () => { |
There was a problem hiding this comment.
| it('does not track event when disabled', () => { | |
| it('does not call platform adapter when disabled', () => { |
| }); | ||
|
|
||
| it('does not track page when disabled', () => { | ||
| it('does not track view when disabled', () => { |
There was a problem hiding this comment.
Looks like we are missing a test calls platform adapter to track view when enabled via social opt-in or something equivalent?
| }); | ||
|
|
||
| it('initializes with custom enabled/optedIn', () => { | ||
| it('returns controller when onSetupCompleted throws error', () => { |
There was a problem hiding this comment.
Nit: This is technically true, but what do you think about framing this in terms of not causing any issues?
| it('returns controller when onSetupCompleted throws error', () => { | |
| it('discards errors thrown by onSetupCompleted', () => { |
Important
This is an unreleased package which will continue to be improved until ready for release.
analytics-controllerupdateonSetupCompletedlifecycle hook for post-initialization setupgetAnalyticsId(),isEnabled(),isOptedIn(),isSocialOptedIn()socialOptIn(),socialOptOut()AnalyticsPlatformAdapterSetupErrorfor setup failuresenabled→ computed viacomputeEnabledState(),optedIn→user_optedIn,analyticsId→user_analyticsId, addeduser_socialOptedInanalyticsStateComputermodule withcomputeEnabledState()function to compute enabled state from user opt-in statusenable()anddisable()methods (replaced by computed enabled state)AnalyticsTrackingEventtype withproperties,sensitiveProperties,isAnonymous,hasPropertiesfor anonymous/non-anonymous trackingtrackEventto acceptAnalyticsTrackingEventinstead of separateeventNameandpropertiesparametersAnalyticsPlatformAdaptertrackPage→trackViewfor platform agnostic usagetrackEvent→trackto better match Segment SDKsidentify: removeduserIdparameter, usesanalyticsIdfrom state, addedAnalyticsUserTraitstypeReferences
fixes #7079
Checklist
Note
Refactors controller and adapter APIs, introduces opt-in based enabled state, adds onSetupCompleted lifecycle hook and UUIDv4 validation, and updates actions, selectors, tests, and docs.
trackEvent(event: AnalyticsTrackingEvent)(replacestrackEvent(name, props)),trackView(renamed fromtrackPage),identify(traits?)(uses stateanalyticsId).track(renamed fromtrackEvent),view(replacestrackPage),identifyrequired.optedInForRegularAccount,optedInForSocialAccount,analyticsId; enabled is derived viacomputeEnabledState.selectAnalyticsId,selectOptedIn,selectSocialOptedIn,selectEnabled.validateAnalyticsState.onSetupCompleted(analyticsId)hook on adapter; non-fatal errors wrapped byAnalyticsPlatformAdapterSetupError.AnalyticsTrackingEventwithproperties,sensitiveProperties,hasProperties; dual-track sensitive payloads withisSensitiveflag; omit props when none.trackViewand per-account opt-in/out; removedenable/disableand oldoptIn/optOut.AnalyticsPlatformAdapterSetupErrorinindex.ts.Written by Cursor Bugbot for commit cc7e39b. This will update automatically on new commits. Configure here.