feat(analytics-controller): resolve geolocation only after opt-in - #9728
Merged
Conversation
Gate geolocation resolution behind user consent so a user's location is never requested before they opt in to analytics (for example, during onboarding). - `optIn` is now async: it resolves and awaits geolocation before replaying queued pre-consent events, so those events are enriched with the resolved location on replay (anonymous payloads excluded). - Resolution runs at most once per session: during `init` for an already opted-in user, otherwise on `optIn`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates @metamask/analytics-controller to ensure geolocation is only resolved after a user has opted in to analytics, preventing any location request prior to consent while still enriching queued pre-consent events upon replay.
Changes:
- Gate geolocation resolution behind opt-in via an idempotent per-session resolver (
#maybeResolveLocation) and defer resolution untiloptIn()when the user was previously undecided/opted out. - Make
AnalyticsController.optInasynchronous and await geolocation resolution before replaying queued pre-consent events (excluding anonymous payloads). - Update tests and changelog to reflect the new consent-gated behavior and the async
optIn()breaking change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/analytics-controller/src/AnalyticsController.ts | Defers geolocation resolution until the user is opted in; makes optIn() async and enriches replayed pre-consent events after resolution. |
| packages/analytics-controller/src/AnalyticsController.test.ts | Updates/extends tests to validate opt-in-gated geolocation resolution and replay enrichment, and awaits optIn(). |
| packages/analytics-controller/src/AnalyticsController-method-action-types.ts | Updates action documentation to reflect async opt-in and geolocation gating behavior. |
| packages/analytics-controller/CHANGELOG.md | Documents the breaking async optIn() change and revised geolocation resolution timing/behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Match the conditional-spread pattern used elsewhere so a replayed pre-consent event does not gain an explicit `context: undefined` key when geolocation is unresolved and the event carried no context, keeping queued/persisted payloads JSON-shaped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gauthierpetetin
marked this pull request as ready for review
July 31, 2026 04:17
gauthierpetetin
temporarily deployed
to
default-branch
July 31, 2026 04:17 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 95d85f1. Configure here.
optIn now awaits geolocation before draining the pre-consent queue, which opens a window where the consent decision can change (e.g. resetConsentDecision) while resolution is in flight. Replaying blindly after the await could deliver preserved pre-consent events once the user is undecided again. Reconcile against the current state after the await (as #performInit already does) instead of calling #replayPreConsentEvents directly, so preserved events are only delivered while the user is still opted in. #replayPreConsentEvents now receives the validated queue from its sole caller, dropping its redundant guards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gauthierpetetin
enabled auto-merge
July 31, 2026 04:36
NicolasMassart
approved these changes
Jul 31, 2026
| async #resolveLocationContext(): Promise<void> { | ||
| if (!this.#isGeolocationEnabled) { | ||
| return; | ||
| #maybeResolveLocation(): Promise<void> | undefined { |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): Could we explicitly document in JSDoc that a failed geolocation resolution is cached and will not be retried for this controller session?
The implementation is intentional, but the shared settled promise makes that behavior easy to overlook.
Contributor
Author
There was a problem hiding this comment.
Yes, good call, comment added in this PR: #9730
4 tasks
pull Bot
pushed a commit
to Reality2byte/core
that referenced
this pull request
Jul 31, 2026
…r session (MetaMask#9730) ## Explanation Follow-up to MetaMask#9728. A reviewer [noted](MetaMask#9728 (comment)) that the at-most-once resolution behavior of `#maybeResolveLocation` is easy to overlook, since `#resolveLocationContext` swallows its own errors (the shared promise resolves rather than rejects, and the `#locationResolvePromise !== undefined` guard then blocks any retry). This adds a JSDoc paragraph to `#maybeResolveLocation` making the behavior explicit: a failed resolution is not retried, and events are delivered without location for the rest of the session. ## References - Follow-up to MetaMask#9728 - Addresses [review comment](MetaMask#9728 (comment)) by @NicolasMassart ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > JSDoc-only change with no behavioral impact. > > **Overview** > Adds a JSDoc paragraph on **`#maybeResolveLocation`** so reviewers and maintainers can see behavior that was easy to miss: geolocation resolution runs **at most once per controller session**, the settled promise is kept (including after **`#resolveLocationContext`** swallows errors), failures are **not retried**, and analytics events go out **without location** for the rest of the session. > > No runtime or test changes—documentation only, following review on MetaMask#9728. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 46c2c80. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
BenraouaneSoufiane
pushed a commit
to BenraouaneSoufiane/metamask-extension
that referenced
this pull request
Jul 31, 2026
## **Description** Enables geolocation enrichment of analytics events in the extension, wiring it up to the core changes in [MetaMask/core#9691](MetaMask/core#9691) and [MetaMask/core#9728](MetaMask/core#9728) (implements [ADR-0008](MetaMask/decisions#217)). **Why:** Extension metric events currently reach Segment without geolocation, unlike Mobile. This attaches the user's country, region, and timezone so downstream destinations (Segment, BigQuery, Mixpanel) receive the same fields on both clients. **What changed:** - Enable `isGeolocationEnabled` on `AnalyticsController`, so it resolves the user's location and attaches `country_code` / `region` / `timezone` to `context.location` on non-anonymous track, identify, and view payloads. - Delegate `GeolocationController:getGeolocationData` to the analytics controller messenger. - Delegate `GeolocationApiService:fetchGeolocationData` (the v2 action, renamed from `fetchGeolocation`) on the geolocation controller messenger. - Initialize `GeolocationApiService` + `GeolocationController` **before** `AnalyticsController` in the controller init list, since `AnalyticsController.init()` resolves geolocation during its own init via the messenger. - Update the `GeolocationController` init test fixture for the new `country` / `region` / `timezone` state fields. **Dependency / status:** This is tested against **core preview builds** (`previewBuilds` manifest in `package.json`), since the core PR is not yet released. Keep as **draft** until [MetaMask/core#9691](MetaMask/core#9691) merges and publishes, then swap the preview versions for the released package versions. ## **Changelog** CHANGELOG entry: null <!-- Analytics enrichment plumbing; no user-facing UI change. Recommend the `no-changelog` label. --> ## **Related issues** Related to: [MetaMask/core#9691](MetaMask/core#9691), [ADR-0008](MetaMask/decisions#217) Fixes: MetaMask/MetaMask-planning#7001 ## **Manual testing steps** 1. Build and load the extension with analytics enabled (opted in). 2. Trigger some events (e.g. open the wallet, view a screen). 3. Inspect the outgoing Segment payloads and confirm non-anonymous events carry `context.location` with `country_code`, `region`, and `timezone`; anonymous payloads carry no location. ## **Screenshots/Recordings** N/A — no UI changes. ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes analytics payload shape and startup ordering for opted-in users, and adds network calls to geolocation hosts; behavior is gated on analytics consent in core but touches metrics and privacy-sensitive location fields. > > **Overview** > Turns on geolocation enrichment for extension analytics by upgrading `@metamask/analytics-controller` to v2 and `@metamask/geolocation-controller` to v1, then wiring them together at init time. > > `AnalyticsController` is constructed with **`isGeolocationEnabled: true`**, and its messenger is allowed to call **`GeolocationController:getGeolocationData`**. The geolocation stack delegates **`GeolocationApiService:fetchGeolocationData`** (v2 API action rename). **`GeolocationApiService` and `GeolocationController` now initialize immediately before `AnalyticsController`** so location can be resolved during analytics `init()`. > > E2E and privacy fixtures add mocks/allowlist entries for `geolocation.api.cx.metamask.io` (and dev host), plus state snapshots for `country` / `region` / `timezone`. Lavamoat policies drop a nested geolocation `controller-utils` entry in favor of the shared package. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 071aa2b. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Bigshmow
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 5, 2026
## **Description** Enables geolocation enrichment of analytics events in the extension, wiring it up to the core changes in [MetaMask/core#9691](MetaMask/core#9691) and [MetaMask/core#9728](MetaMask/core#9728) (implements [ADR-0008](MetaMask/decisions#217)). **Why:** Extension metric events currently reach Segment without geolocation, unlike Mobile. This attaches the user's country, region, and timezone so downstream destinations (Segment, BigQuery, Mixpanel) receive the same fields on both clients. **What changed:** - Enable `isGeolocationEnabled` on `AnalyticsController`, so it resolves the user's location and attaches `country_code` / `region` / `timezone` to `context.location` on non-anonymous track, identify, and view payloads. - Delegate `GeolocationController:getGeolocationData` to the analytics controller messenger. - Delegate `GeolocationApiService:fetchGeolocationData` (the v2 action, renamed from `fetchGeolocation`) on the geolocation controller messenger. - Initialize `GeolocationApiService` + `GeolocationController` **before** `AnalyticsController` in the controller init list, since `AnalyticsController.init()` resolves geolocation during its own init via the messenger. - Update the `GeolocationController` init test fixture for the new `country` / `region` / `timezone` state fields. **Dependency / status:** This is tested against **core preview builds** (`previewBuilds` manifest in `package.json`), since the core PR is not yet released. Keep as **draft** until [MetaMask/core#9691](MetaMask/core#9691) merges and publishes, then swap the preview versions for the released package versions. ## **Changelog** CHANGELOG entry: null <!-- Analytics enrichment plumbing; no user-facing UI change. Recommend the `no-changelog` label. --> ## **Related issues** Related to: [MetaMask/core#9691](MetaMask/core#9691), [ADR-0008](MetaMask/decisions#217) Fixes: MetaMask/MetaMask-planning#7001 ## **Manual testing steps** 1. Build and load the extension with analytics enabled (opted in). 2. Trigger some events (e.g. open the wallet, view a screen). 3. Inspect the outgoing Segment payloads and confirm non-anonymous events carry `context.location` with `country_code`, `region`, and `timezone`; anonymous payloads carry no location. ## **Screenshots/Recordings** N/A — no UI changes. ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes analytics payload shape and startup ordering for opted-in users, and adds network calls to geolocation hosts; behavior is gated on analytics consent in core but touches metrics and privacy-sensitive location fields. > > **Overview** > Turns on geolocation enrichment for extension analytics by upgrading `@metamask/analytics-controller` to v2 and `@metamask/geolocation-controller` to v1, then wiring them together at init time. > > `AnalyticsController` is constructed with **`isGeolocationEnabled: true`**, and its messenger is allowed to call **`GeolocationController:getGeolocationData`**. The geolocation stack delegates **`GeolocationApiService:fetchGeolocationData`** (v2 API action rename). **`GeolocationApiService` and `GeolocationController` now initialize immediately before `AnalyticsController`** so location can be resolved during analytics `init()`. > > E2E and privacy fixtures add mocks/allowlist entries for `geolocation.api.cx.metamask.io` (and dev host), plus state snapshots for `country` / `region` / `timezone`. Lavamoat policies drop a nested geolocation `controller-utils` entry in favor of the shared package. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 071aa2b. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
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
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
Follow-up to #9691, which added geo-enrichment of analytics events. As originally merged, the
AnalyticsControllerresolved the user's geolocation duringinitregardless of consent state — so a user's location could be requested before they had opted in to analytics (for example, during onboarding).This PR gates geolocation resolution behind user consent:
initfor an already-opted-in user, otherwise deferred tooptIn. A user's location is never requested before they consent to analytics.AnalyticsController.optInbecomes async (returnsPromise<void>). It resolves and awaits geolocation before replaying queued pre-consent events, so those events are enriched with the resolved location as they replay. Anonymous payloads are excluded, per ADR-0008.#maybeResolveLocationis idempotent).initwas already async (from #9691);optInbecoming async is the one new breaking change for consumers, who must nowawaitit.References
Checklist
Note
Medium Risk
Breaking API change (
optInasync) plus consent-sensitive geolocation and pre-consent replay timing; behavior is well-tested but integrators must awaitoptInand register GeolocationController before consent.Overview
Follow-up to geo-enrichment in AnalyticsController: geolocation is no longer fetched during
initfor undecided or opted-out users. WithisGeolocationEnabled, resolution runs only after the user has opted in—oninitif already opted in, otherwise onoptIn—via a one-shot#maybeResolveLocationpath.optInis now async (Promise<void>): it awaits geolocation, then calls#reconcilePreConsentEventsinstead of replaying the pre-consent queue immediately, so events are not sent if consent was reset while location was resolving. Queued pre-consent events getcontext.locationon replay through#enrichPreConsentEvent; anonymous track payloads stay without location.The changelog documents the breaking
await optIn()requirement for consumers.Reviewed by Cursor Bugbot for commit a8c6462. Bugbot is set up for automated code reviews on this repo. Configure here.