Feat: new analytics data regulation controller#7643
Merged
NicolasMassart merged 40 commits intomainfrom Jan 28, 2026
Merged
Conversation
…ad of dates - Changed `deleteRegulationDate` to `deleteRegulationTimestamp` in the state and related methods to store timestamps in milliseconds since epoch. - Updated relevant methods and tests to reflect the new timestamp format. - Removed date formatting logic and adjusted selectors accordingly. - Added new dependencies for testing and updated the test suite to ensure proper functionality with the new timestamp format. This change enhances consistency in handling date-related data within the analytics privacy controller.
NicolasMassart
added
enhancement
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
NicolasMassart
commented
Jan 15, 2026
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…e PascalCase - Refactored enum values in `DataDeleteResponseStatus` and `DataDeleteStatus` to follow PascalCase naming convention. - Updated all references in the codebase and tests to ensure consistency with the new enum values. - This change enhances code readability and aligns with common TypeScript practices.
…st title - Rename Error to Failure (and Ok to Success) for clearer naming - Fix duplicate test title in AnalyticsPrivacyController.test.ts - Update all references across the codebase
…tead of type-only
packages/analytics-data-regulation-controller/tsconfig.build.json
Outdated
Show resolved
Hide resolved
…nd improve state handling - Removed references to the `analytics-controller` in `tsconfig` files to streamline project structure. - Enhanced `checkDataDeleteStatus` method by capturing state values before async calls to ensure consistency during concurrent operations. - Simplified `IDeleteRegulationResponse` type to only represent the success case, removing unnecessary complexity. - Cleaned up unused imports in `logger.ts` and removed the `IDeleteRegulationStatusResponse` type from `types.ts` to reduce clutter. These changes improve code clarity and maintainability while ensuring the functionality remains intact.
packages/analytics-data-regulation-controller/src/AnalyticsDataRegulationController.ts
Outdated
Show resolved
Hide resolved
packages/analytics-data-regulation-controller/src/AnalyticsDataRegulationService.ts
Show resolved
Hide resolved
…ns for data deletion responses - Replaced `IDeleteRegulationResponse` and `IDeleteRegulationStatus` with `DeleteRegulationResponse` and `DeleteRegulationStatus` for improved clarity and consistency. - Updated the `checkDataDeleteStatus` method to utilize the new type definitions, ensuring type safety and better alignment with the API response structure. - Cleaned up imports in `index.ts` to reflect the new type names. These changes enhance code readability and maintainability while ensuring accurate type representation.
packages/analytics-data-regulation-controller/src/AnalyticsDataRegulationController.ts
Outdated
Show resolved
Hide resolved
… status handling - Updated `deletionRequestTimestamp` in `DeleteRegulationStatus` to remove the nullish coalescing operator, ensuring it directly reflects the provided timestamp. - Added `AnalyticsDataRegulationServiceMessenger` type export in `index.ts` for better type accessibility. - Removed the `DeleteRegulationStatusResponse` type from `types.ts` to streamline type definitions and reduce redundancy. These changes enhance clarity and maintainability of the data regulation controller's type definitions and state management.
…etaMask/core into feat/7618_analytics_privacy_controller
NicolasMassart
changed the title
Contributor
Author
|
@metamaskbot publish-preview |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| * Type union for data deletion response status values. | ||
| */ | ||
| export type DataDeleteResponseStatus = | ||
| (typeof DATA_DELETE_RESPONSE_STATUSES)[keyof typeof DATA_DELETE_RESPONSE_STATUSES]; |
There was a problem hiding this comment.
Unused DataDeleteResponseStatus type is never referenced
Low Severity
The DataDeleteResponseStatus type is defined and exported but never used as a type annotation anywhere in the codebase. The service methods explicitly return typeof DATA_DELETE_RESPONSE_STATUSES.Success in their return types rather than using this union type. This type exists only because it's derived from the status constants, but it provides no value.
Additional Locations (1)
mcmire
approved these changes
Jan 28, 2026
Contributor
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Had some more comments, but they are fairly minor at this point, implementation looks pretty good to me. I'll go ahead and approve.
| actions: ['AnalyticsDataRegulationService:createDataDeletionTask'], | ||
| }); | ||
|
|
||
| const controller = new AnalyticsDataRegulationController({ |
Contributor
There was a problem hiding this comment.
Nit: Can we use setupController in this test to remove some of the boilerplate? There are some other tests in this file that are also creating the controller and messengers manually?
| }); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const _controller = new AnalyticsDataRegulationController({ |
Contributor
There was a problem hiding this comment.
Same here. It seems if we used setupController we could only destructure the messenger and avoid from needing an eslint-disable comment.
Comment on lines
+788
to
+831
|
|
||
| describe('stateChange event', () => { | ||
| it('emits stateChange event with new state when hasCollectedDataSinceDeletionRequest is updated', () => { | ||
| const { rootMessenger, messenger } = setupController({ | ||
| state: { | ||
| hasCollectedDataSinceDeletionRequest: false, | ||
| }, | ||
| }); | ||
|
|
||
| const eventListener = jest.fn(); | ||
| messenger.subscribe( | ||
| 'AnalyticsDataRegulationController:stateChange', | ||
| eventListener, | ||
| ); | ||
|
|
||
| rootMessenger.call( | ||
| 'AnalyticsDataRegulationController:updateDataRecordingFlag', | ||
| ); | ||
|
|
||
| expect(eventListener).toHaveBeenCalled(); | ||
| const [newState] = eventListener.mock.calls[0]; | ||
| expect(newState.hasCollectedDataSinceDeletionRequest).toBe(true); | ||
| }); | ||
|
|
||
| it('does not emit stateChange event when hasCollectedDataSinceDeletionRequest is already true', () => { | ||
| const { rootMessenger, messenger } = setupController({ | ||
| state: { | ||
| hasCollectedDataSinceDeletionRequest: true, | ||
| }, | ||
| }); | ||
|
|
||
| const eventListener = jest.fn(); | ||
| messenger.subscribe( | ||
| 'AnalyticsDataRegulationController:stateChange', | ||
| eventListener, | ||
| ); | ||
|
|
||
| rootMessenger.call( | ||
| 'AnalyticsDataRegulationController:updateDataRecordingFlag', | ||
| ); | ||
|
|
||
| expect(eventListener).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Do we need this describe? Typically we don't need to test that the stateChange event is triggered for controllers, as this behavior is already tested in the BaseController tests.
Suggested change
| describe('stateChange event', () => { | |
| it('emits stateChange event with new state when hasCollectedDataSinceDeletionRequest is updated', () => { | |
| const { rootMessenger, messenger } = setupController({ | |
| state: { | |
| hasCollectedDataSinceDeletionRequest: false, | |
| }, | |
| }); | |
| const eventListener = jest.fn(); | |
| messenger.subscribe( | |
| 'AnalyticsDataRegulationController:stateChange', | |
| eventListener, | |
| ); | |
| rootMessenger.call( | |
| 'AnalyticsDataRegulationController:updateDataRecordingFlag', | |
| ); | |
| expect(eventListener).toHaveBeenCalled(); | |
| const [newState] = eventListener.mock.calls[0]; | |
| expect(newState.hasCollectedDataSinceDeletionRequest).toBe(true); | |
| }); | |
| it('does not emit stateChange event when hasCollectedDataSinceDeletionRequest is already true', () => { | |
| const { rootMessenger, messenger } = setupController({ | |
| state: { | |
| hasCollectedDataSinceDeletionRequest: true, | |
| }, | |
| }); | |
| const eventListener = jest.fn(); | |
| messenger.subscribe( | |
| 'AnalyticsDataRegulationController:stateChange', | |
| eventListener, | |
| ); | |
| rootMessenger.call( | |
| 'AnalyticsDataRegulationController:updateDataRecordingFlag', | |
| ); | |
| expect(eventListener).not.toHaveBeenCalled(); | |
| }); | |
| }); |
Comment on lines
+22
to
+23
| cleanAll(); | ||
| disableNetConnect(); |
Contributor
There was a problem hiding this comment.
Do you need to do this? It should already be happening for all tests in tests/setupAfterEnv/nock.ts.
Suggested change
| cleanAll(); | |
| disableNetConnect(); |
Comment on lines
+28
to
+29
| cleanAll(); | ||
| enableNetConnect(); |
Contributor
There was a problem hiding this comment.
Do you need to do this? It should already be happening for all tests in tests/setupAfterEnv/nock.ts.
Suggested change
| cleanAll(); | |
| enableNetConnect(); |
| }); | ||
|
|
||
| describe('onBreak', () => { | ||
| it('calls break listener when circuit breaker opens after multiple failures', async () => { |
Contributor
There was a problem hiding this comment.
Nit: The "it" here is the onBreak listener, so maybe this would be more accurate?
Suggested change
| it('calls break listener when circuit breaker opens after multiple failures', async () => { | |
| it('is called when circuit breaker opens after multiple failures', async () => { |
(Similar for the other onBreak and onDegraded tests)
Comment on lines
+248
to
+255
| if (!this.#analyticsId || this.#analyticsId.trim() === '') { | ||
| const error = new Error( | ||
| 'Analytics ID not found. You need to provide a valid analytics ID when initializing the AnalyticsDataRegulationController.', | ||
| ); | ||
| log('Analytics Deletion Task Error', error); | ||
| throw error; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Is this runtime check necessary? There are other instances within core where we perform runtime checks, but I think a lot of those instances are left over from when the code used to be written in JavaScript. I believe we have been largely relying on TypeScript to verify that required inputs have been provided. Of course if you think it's necessary, then that's fine, but I wanted to double-check first.
Suggested change
| if (!this.#analyticsId || this.#analyticsId.trim() === '') { | |
| const error = new Error( | |
| 'Analytics ID not found. You need to provide a valid analytics ID when initializing the AnalyticsDataRegulationController.', | |
| ); | |
| log('Analytics Deletion Task Error', error); | |
| throw error; | |
| } |
Comment on lines
+286
to
+288
| const { deleteRegulationId } = this.state; | ||
| const { deleteRegulationTimestamp } = this.state; | ||
| const { hasCollectedDataSinceDeletionRequest } = this.state; |
Contributor
There was a problem hiding this comment.
Can these be grouped together?
Suggested change
| const { deleteRegulationId } = this.state; | |
| const { deleteRegulationTimestamp } = this.state; | |
| const { hasCollectedDataSinceDeletionRequest } = this.state; | |
| const { | |
| deleteRegulationId, | |
| deleteRegulationTimestamp, | |
| hasCollectedDataSinceDeletionRequest | |
| } = this.state; |
packages/analytics-data-regulation-controller/src/AnalyticsDataRegulationController.ts
Show resolved
Hide resolved
Comment on lines
+300
to
+307
| // Service validates and throws on all errors, so if we reach here, the response | ||
| // is guaranteed to be a success response with dataDeleteStatus present | ||
| const dataDeletionTaskStatus = await this.messenger.call( | ||
| 'AnalyticsDataRegulationService:checkDataDeleteStatus', | ||
| deleteRegulationId, | ||
| ); | ||
|
|
||
| status.dataDeletionRequestStatus = dataDeletionTaskStatus.dataDeleteStatus; |
Contributor
There was a problem hiding this comment.
Nit: I think this comment might be misplaced? Maybe it was meant to go after the call.
Suggested change
| // Service validates and throws on all errors, so if we reach here, the response | |
| // is guaranteed to be a success response with dataDeleteStatus present | |
| const dataDeletionTaskStatus = await this.messenger.call( | |
| 'AnalyticsDataRegulationService:checkDataDeleteStatus', | |
| deleteRegulationId, | |
| ); | |
| status.dataDeletionRequestStatus = dataDeletionTaskStatus.dataDeleteStatus; | |
| const dataDeletionTaskStatus = await this.messenger.call( | |
| 'AnalyticsDataRegulationService:checkDataDeleteStatus', | |
| deleteRegulationId, | |
| ); | |
| // Service validates and throws on all errors, so if we reach here, the response | |
| // is guaranteed to be a success response with dataDeleteStatus present | |
| status.dataDeletionRequestStatus = dataDeletionTaskStatus.dataDeleteStatus; |
github-project-automation
bot
moved this from Needs dev review
to Review finalised - Ready to be merged
in PR review queue
github-project-automation
bot
moved this from Review finalised - Ready to be merged
to Merged, Closed or Archived
in PR review queue
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
This PR introduces a new
@metamask/analytics-data-regulation-controllerpackage that provides GDPR/CCPA data deletion functionality for analytics data. The package allows to extract the logic from the mobile app (and will be compatible with extension too)Current state: MetaMask mobile app currently has a dedicated mechanism to handle user data deletion requests for analytics data in compliance with GDPR and CCPA regulations.
Solution: This package introduces:
hasCollectedDataSinceDeletionRequestflag,deleteRegulationId, anddeleteRegulationTimestampto support compliance workflowsImplementation details:
analyticsIdas a constructor parameter (provided by the consumer)AnalyticsDataRegulationServicevia messenger actions to make HTTP requests to Segment's Regulations APIcreateServicePolicyfrom@metamask/controller-utilsfor retry logic and error handlingReferences
see also MetaMask/metamask-mobile#22016
Fixes #7618
Checklist
Note
Cursor Bugbot is generating a summary for commit 52f4a2c. Configure here.