diff --git a/packages/remote-feature-flag-controller/CHANGELOG.md b/packages/remote-feature-flag-controller/CHANGELOG.md index 47f685a091..9848e47152 100644 --- a/packages/remote-feature-flag-controller/CHANGELOG.md +++ b/packages/remote-feature-flag-controller/CHANGELOG.md @@ -7,8 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add optional `featureFlagThresholdGroups` field to `RemoteFeatureFlagControllerState` to map feature flag names to their selected threshold group names ([#9289](https://github.com/MetaMask/core/pull/9289)) + ### Changed +- **BREAKING:** Threshold feature flags now return the selected `value` directly instead of a `{ name, value }` wrapper. The selected threshold group name is stored separately in `featureFlagThresholdGroups` on controller state when the selected threshold entry includes `thresholdName` ([#9289](https://github.com/MetaMask/core/pull/9289)) - Merge `localOverrides` into `remoteFeatureFlags` at the controller level so consumers receive effective flag values directly ([#9259](https://github.com/MetaMask/core/pull/9259)) - Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074)) - Bump `@metamask/controller-utils` from `^12.1.0` to `^12.3.0` ([#9058](https://github.com/MetaMask/core/pull/9058), [#9083](https://github.com/MetaMask/core/pull/9083), [#9218](https://github.com/MetaMask/core/pull/9218)) diff --git a/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.test.ts b/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.test.ts index f2009007f9..d6da8bf7f8 100644 --- a/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.test.ts +++ b/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.test.ts @@ -478,15 +478,13 @@ describe('RemoteFeatureFlagController', () => { // With MOCK_METRICS_ID + 'testFlagForThreshold' hashed: // Threshold = 0.380673, which falls in groupB range (0.3 < t <= 0.5) - expect( - controller.state.remoteFeatureFlags.testFlagForThreshold, - ).toStrictEqual({ - name: 'groupB', - value: 'valueB', - }); + expect(controller.state.remoteFeatureFlags.testFlagForThreshold).toBe( + 'valueB', + ); + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({}); }); - it('preserves selected legacy threshold object value wrappers', async () => { + it('returns selected threshold values directly without threshold group mapping when thresholdName is absent', async () => { const thresholdFlagValue = { enabled: true, minimumVersion: '13.10.0', @@ -515,13 +513,11 @@ describe('RemoteFeatureFlagController', () => { expect( controller.state.remoteFeatureFlags.thresholdObjectFlag, - ).toStrictEqual({ - name: 'enabled', - value: thresholdFlagValue, - }); + ).toStrictEqual(thresholdFlagValue); + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({}); }); - it('returns selected threshold version 2 values without wrapper metadata', async () => { + it('uses thresholdName when provided for threshold group mapping', async () => { const thresholdFlagValue = { enabled: true, minimumVersion: '13.10.0', @@ -552,9 +548,12 @@ describe('RemoteFeatureFlagController', () => { expect( controller.state.remoteFeatureFlags.thresholdObjectFlag, ).toStrictEqual(thresholdFlagValue); + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({ + thresholdObjectFlag: 'enabled', + }); }); - it('falls back to legacy threshold wrappers for unrecognized threshold versions', async () => { + it('returns selected threshold values for unrecognized threshold versions', async () => { const thresholdFlagValue = { enabled: true, }; @@ -582,10 +581,8 @@ describe('RemoteFeatureFlagController', () => { expect( controller.state.remoteFeatureFlags.thresholdObjectFlag, - ).toStrictEqual({ - name: 'enabled', - value: thresholdFlagValue, - }); + ).toStrictEqual(thresholdFlagValue); + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({}); }); it('preserves non-threshold feature flags unchanged', async () => { @@ -649,9 +646,10 @@ describe('RemoteFeatureFlagController', () => { // Assert - User gets different groups because each flag uses unique seed const { featureA, featureB } = controller.state.remoteFeatureFlags; // featureA: hash(MOCK_METRICS_ID + 'featureA') → threshold 0.966682 → groupA2 - expect(featureA).toStrictEqual({ name: 'groupA2', value: 'A2' }); + expect(featureA).toBe('A2'); // featureB: hash(MOCK_METRICS_ID + 'featureB') → threshold 0.398654 → groupB1 - expect(featureB).toStrictEqual({ name: 'groupB1', value: 'B1' }); + expect(featureB).toBe('B1'); + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({}); // Different groups proves independence! }); @@ -711,10 +709,10 @@ describe('RemoteFeatureFlagController', () => { ); // Assert - Invalid item skipped, valid item selected - expect(controller.state.remoteFeatureFlags.mixedArray).toStrictEqual({ - name: 'validGroup', - value: 'selectedValue', - }); + expect(controller.state.remoteFeatureFlags.mixedArray).toBe( + 'selectedValue', + ); + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({}); }); it('assigns users to same group for same feature flag on multiple calls', async () => { @@ -761,7 +759,8 @@ describe('RemoteFeatureFlagController', () => { // Assert - Same user always gets same group (deterministic) // testFlag: hash(MOCK_METRICS_ID + 'testFlag') → threshold 0.496587 → control expect(firstResult).toStrictEqual(secondResult); - expect(firstResult).toStrictEqual({ name: 'control', value: false }); + expect(firstResult).toBe(false); + expect(controller1.state.featureFlagThresholdGroups).toStrictEqual({}); }); }); @@ -1073,9 +1072,10 @@ describe('RemoteFeatureFlagController', () => { // With MOCK_METRICS_ID + 'multiVersionABFlag' hashed: // Threshold = 0.094878, which falls in groupA range (t <= 0.3) expect(multiVersionABFlag).toStrictEqual({ - name: 'groupA', - value: { feature: 'A', enabled: true }, + feature: 'A', + enabled: true, }); + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({}); expect(regularFlag).toBe(true); }); }); @@ -1406,6 +1406,72 @@ describe('RemoteFeatureFlagController', () => { jest.useRealTimers(); }); + it('removes stale featureFlagThresholdGroups entries when flags are removed from server', async () => { + jest.useRealTimers(); + const clientConfigApiService = buildClientConfigApiService({ + remoteFeatureFlags: { + flagA: [ + { + thresholdName: 'groupA', + thresholdVersion: ThresholdVersion.DirectValue, + scope: { type: 'threshold', value: 1.0 }, + value: true, + }, + ], + flagB: [ + { + thresholdName: 'groupB', + thresholdVersion: ThresholdVersion.DirectValue, + scope: { type: 'threshold', value: 1.0 }, + value: false, + }, + ], + }, + }); + const { controller, messenger } = createController({ + clientConfigApiService, + getMetaMetricsId: () => MOCK_METRICS_ID, + }); + + await messenger.call( + 'RemoteFeatureFlagController:updateRemoteFeatureFlags', + ); + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({ + flagA: 'groupA', + flagB: 'groupB', + }); + + jest + .spyOn(clientConfigApiService, 'fetchRemoteFeatureFlags') + .mockResolvedValue({ + remoteFeatureFlags: { + flagB: [ + { + thresholdName: 'groupB', + thresholdVersion: ThresholdVersion.DirectValue, + scope: { type: 'threshold', value: 1.0 }, + value: false, + }, + ], + }, + cacheTimestamp: Date.now(), + }); + + jest.useFakeTimers(); + jest.advanceTimersByTime(2 * DEFAULT_CACHE_DURATION); + + await messenger.call( + 'RemoteFeatureFlagController:updateRemoteFeatureFlags', + ); + await flushPromises(); + + expect(controller.state.featureFlagThresholdGroups).toStrictEqual({ + flagB: 'groupB', + }); + + jest.useRealTimers(); + }); + it('preserves threshold cache entries for flags still in server response', async () => { // Arrange const mockFlags = { diff --git a/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts b/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts index 453b4ed9e2..436451dd2b 100644 --- a/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts +++ b/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts @@ -9,7 +9,6 @@ import type { Json, SemVerVersion } from '@metamask/utils'; import type { AbstractClientConfigApiService } from './client-config-api-service/abstract-client-config-api-service'; import type { RemoteFeatureFlagControllerMethodActions } from './remote-feature-flag-controller-method-action-types'; -import { ThresholdVersion } from './remote-feature-flag-controller-types'; import type { FeatureFlags, ServiceResponse, @@ -34,6 +33,7 @@ export type RemoteFeatureFlagControllerState = { rawRemoteFeatureFlags?: FeatureFlags; cacheTimestamp: number; thresholdCache?: Record; + featureFlagThresholdGroups?: Record; }; const remoteFeatureFlagControllerMetadata = { @@ -67,6 +67,12 @@ const remoteFeatureFlagControllerMetadata = { includeInDebugSnapshot: false, usedInUi: false, }, + featureFlagThresholdGroups: { + includeInStateLogs: true, + persist: true, + includeInDebugSnapshot: true, + usedInUi: true, + }, }; // === MESSENGER === @@ -119,19 +125,6 @@ export function getDefaultRemoteFeatureFlagControllerState(): RemoteFeatureFlagC }; } -function normalizeThresholdValue(featureFlag: FeatureFlagScopeValue): Json { - if (featureFlag.thresholdVersion === ThresholdVersion.DirectValue) { - return featureFlag.value; - } - - // Unknown threshold versions fall back to the legacy wrapper shape for - // backwards compatibility with existing threshold feature flag configs. - return { - name: featureFlag.name, - value: featureFlag.value, - }; -} - /** * The RemoteFeatureFlagController manages the retrieval and caching of remote feature flags. * It fetches feature flags from a remote API, caches them, and provides methods to access @@ -288,8 +281,11 @@ export class RemoteFeatureFlagController extends BaseController< * @param remoteFeatureFlags - The new feature flags to cache. */ async #updateCache(remoteFeatureFlags: FeatureFlags): Promise { - const { processedFlags, thresholdCacheUpdates } = - await this.#processRemoteFeatureFlags(remoteFeatureFlags); + const { + processedFlags, + thresholdCacheUpdates, + featureFlagThresholdGroupUpdates, + } = await this.#processRemoteFeatureFlags(remoteFeatureFlags); const metaMetricsId = this.#getMetaMetricsId(); const currentFlagNames = Object.keys(remoteFeatureFlags); @@ -314,6 +310,17 @@ export class RemoteFeatureFlagController extends BaseController< } } + const updatedFeatureFlagThresholdGroups = { + ...(this.state.featureFlagThresholdGroups ?? {}), + ...featureFlagThresholdGroupUpdates, + }; + + for (const flagName of Object.keys(updatedFeatureFlagThresholdGroups)) { + if (!currentFlagNames.includes(flagName)) { + delete updatedFeatureFlagThresholdGroups[flagName]; + } + } + // Single state update with all changes batched together this.#processedRemoteFeatureFlags = processedFlags; @@ -327,6 +334,7 @@ export class RemoteFeatureFlagController extends BaseController< rawRemoteFeatureFlags: remoteFeatureFlags, cacheTimestamp: Date.now(), thresholdCache: updatedThresholdCache, + featureFlagThresholdGroups: updatedFeatureFlagThresholdGroups, }; }); } @@ -348,10 +356,12 @@ export class RemoteFeatureFlagController extends BaseController< async #processRemoteFeatureFlags(remoteFeatureFlags: FeatureFlags): Promise<{ processedFlags: FeatureFlags; thresholdCacheUpdates: Record; + featureFlagThresholdGroupUpdates: Record; }> { const processedFlags: FeatureFlags = {}; const metaMetricsId = this.#getMetaMetricsId(); const thresholdCacheUpdates: Record = {}; + const featureFlagThresholdGroupUpdates: Record = {}; for (const [ remoteFeatureFlagName, @@ -409,14 +419,22 @@ export class RemoteFeatureFlagController extends BaseController< ); if (selectedGroup) { - processedValue = normalizeThresholdValue(selectedGroup); + processedValue = selectedGroup.value; + if (selectedGroup.thresholdName) { + featureFlagThresholdGroupUpdates[remoteFeatureFlagName] = + selectedGroup.thresholdName; + } } } processedFlags[remoteFeatureFlagName] = processedValue; } - return { processedFlags, thresholdCacheUpdates }; + return { + processedFlags, + thresholdCacheUpdates, + featureFlagThresholdGroupUpdates, + }; } /**