fix: Normalize remote feature flag value wrappers by pedronfigueiredo · Pull Request #8908 · MetaMask/core

pedronfigueiredo · 2026-05-27T13:24:39Z

There is an issue with the current remote feature flag implementation.

As you can see when the feature flags are configured for threshold based config, we define an array of objects that contains name, scope, and value. The problem with this is that there's lots of feature flags that are not defined in this format, and the selectors are unfortunately coupled to this format.

getIsNotificationEnabledByDefaultFeatureFlag inside ui/selectors/metamask-notifications/metamask-notifications.ts expects a .value property for example. But confirmations_pay feature flag, we don't expect such .value property to house the configuration JSON, and instead it's available directly at the root level.

Summary

Normalize processed remote feature flag wrapper objects that include a top-level value field.
Copy object-valued value contents onto the root flag object while preserving the original .value field.
Preserve direct feature flag config objects that do not use wrapper metadata.

Root Cause

Some consumers read feature flag config directly from the flag object, while threshold or version wrapper shapes expose config under .value. This made selectors depend on the rollout format instead of the feature flag config itself.

Impact

Feature flags can move between direct, threshold, and versioned configurations without requiring selectors to switch between direct config access and .value access. Existing consumers that still read .value remain compatible.

Note

Medium Risk
Changes processed flag shapes for v2 threshold configs, which can affect UI selectors that expect .value; legacy and unversioned thresholds stay wrapped for compatibility.

Overview
Remote feature flag threshold processing now supports a v2 direct-value shape via ThresholdVersion.DirectValue (thresholdVersion: 2): after threshold selection, the controller exposes the chosen config as the flag value itself instead of { name, value }.

Legacy behavior is unchanged for entries without that version (and for unknown thresholdVersion values), which still emit the { name, value } wrapper. Types add optional thresholdName / thresholdVersion on threshold scope entries, and ThresholdVersion is exported from the package. Tests cover direct configs, legacy wrappers, v2 output, and fallback for unrecognized versions.

^{Reviewed by Cursor Bugbot for commit 1de7517. Bugbot is set up for automated code reviews on this repo. Configure here.}

matthewwalsh0 · 2026-05-28T08:12:14Z

+  };
+}
+
+function normalizeFeatureFlagValue(featureFlagValue: Json): Json {


For clarity, should we call this normalizeThresholdValue?

matthewwalsh0 · 2026-05-28T08:13:49Z


-      processedFlags[remoteFeatureFlagName] = processedValue;
+      processedFlags[remoteFeatureFlagName] =
+        normalizeFeatureFlagValue(processedValue);


As we also normalize for versioning, would it be simpler to normalize on line 412 above, so a function specific for getting threshold data?

matthewwalsh0 · 2026-05-28T08:19:31Z

+
+function normalizeFeatureFlagValue(featureFlagValue: Json): Json {
+  return isFeatureFlagValueWrapper(featureFlagValue)
+    ? spreadFeatureFlagValueWrapper(featureFlagValue)


Am I misreading or is this saying if the flag is an object with a value property, then return the value property directly also?

But won't that mean we duplicate the data in every threshold flag?

Could we check (just for the threshold flow) if a thresholdVersion property exists and if set to 2, then we just return value directly? And maybe with a thresholdName property instead of name for clarity?

Yes we do duplicate because although we know in this function what is the shape of the flag defined on launch darkly, we have no way of knowing at this point what is the shape of the data that the selector needs or expects. In other words, we need this change to be backwards compatible.

We have to support three cases:

If a threshold hasn't been defined, then just add the whole flag

If a threshold is defined, and the selectors are currently defined without .value, we want to spread the .value at the root level so no selector change is required (to support our confirmations_pay with thresholds use case)

If a threshold is defined, and the selectors already have .value, we also need to keep the .value prop that the selector expects.

To support 2 and 3 simultaneously we need to duplicate the data.

I am not sure what you mean by the thresholdVersion prop with value 2, I can't see it in the client config api response. Can you clarify?

I was suggesting we make the distinction explicit with a new property such as thresholdVersion: 2 that we put in one of the array entries, so the code knows to only spread the value, rather than including it as it does currently.

Both should solve the problem, but didn't know if we wanted to future-proof and make the RemoteFeatureFlagController state more readable?

matthewwalsh0 · 2026-05-28T13:11:23Z

+      return null;
+    }
+
+    return isFeatureFlagValueWrapper(versionData)


Doesn't the chosen version data get assigned to processedValue and then hit the standard threshold flow so we don't need to touch this?

matthewwalsh0 · 2026-05-28T13:12:35Z

+}
+
+function normalizeThresholdValue(featureFlag: FeatureFlagScopeValue): Json {
+  if (featureFlag.thresholdVersion === THRESHOLD_VALUE_VERSION) {


Minor, if we add future versions, this constant might get confusing. Maybe an enum?

matthewwalsh0 · 2026-05-28T13:13:29Z

+
+function normalizeThresholdValue(featureFlag: FeatureFlagScopeValue): Json {
+  if (featureFlag.thresholdVersion === THRESHOLD_VALUE_VERSION) {
+    return featureFlag.value;


Minor, could still be useful to also include thresholdName property only if an object, for debug in the client?

Not sure how this will impact the new AB testing hooks.

good points let's add that if and when it becomes necessary then

matthewwalsh0 · 2026-05-28T13:15:48Z

+  const name = featureFlag.thresholdName ?? featureFlag.name;
+
+  return {
+    ...(isJsonObject(featureFlag.value) ? featureFlag.value : {}),


If this is the fallback for the existing support, can we just return the same?

{ name: featureFlag.name, value: featureFlag.value }

good point, I forgot to revert this part

matthewwalsh0 · 2026-05-28T13:17:18Z

+  );
+}
+
+function spreadFeatureFlagValueWrapper(


Not needed?

matthewwalsh0 · 2026-05-28T13:17:23Z

+  return typeof value === 'object' && value !== null && !Array.isArray(value);
+}
+
+function isFeatureFlagValueWrapper(


Not needed?

matthewwalsh0 · 2026-05-28T13:17:28Z

  };
 }

+function isJsonObject(value: Json): value is JsonObject {


Not needed?

cursor

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 9dc313d. Configure here.}

DDDDDanica · 2026-05-29T13:13:02Z

LGTM !

NicolasMassart

Nice work overall — the normalizeThresholdValue approach is clean and the legacy wrapper fallback is well-preserved. One MEDIUM item to address before merge (type safety), two low-priority notes.

feedback addressed

NicolasMassart

Looks good to me.

## Description Updates the remote feature flag documentation to explain direct-value threshold entries using `thresholdVersion: 2`. ## Changes - Documents the legacy threshold output shape as `{ name, value }`. - Adds a direct-value threshold example for selectors that read root-level object fields. - Clarifies that non-threshold object flags and version-based object flags do not need `thresholdVersion`. - Updates the composed version + threshold example to place `thresholdVersion: 2` inside each threshold entry. Related to this core PR MetaMask/core#8908  --- > [!NOTE] > **Low Risk** > Documentation-only changes with no runtime, auth, or data-handling impact. > > **Overview** > Updates **`docs/remote-feature-flags.md`** to document **direct-value threshold entries** (`thresholdVersion: 2`) and how they differ from the legacy threshold output. > > The doc now states that entries **without** `thresholdVersion` resolve to the legacy **`{ name, value }`** wrapper, and that selectors should use **`.value`** in that case. A new section explains **`thresholdVersion: 2`**, which returns the selected entry’s **`value` at the root** (like a plain object flag), and recommends **`thresholdName`** instead of **`name`** for entry labels so config stays clear without colliding with fields inside the value object. > > It also notes that **version-only object flags** do not use `thresholdVersion`, and refreshes the **version + threshold** composed example to use **`thresholdName`** / **`thresholdVersion: 2`** on each threshold entry, with a note that omitting v2 keeps the legacy wrapper shape. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6131eb6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>

pedronfigueiredo changed the title ~~[codex] Normalize remote feature flag value wrappers~~ fix: Normalize remote feature flag value wrappers May 27, 2026

pedronfigueiredo marked this pull request as ready for review May 27, 2026 16:37

pedronfigueiredo requested review from a team as code owners May 27, 2026 16:37

pedronfigueiredo temporarily deployed to default-branch May 27, 2026 16:38 — with GitHub Actions Inactive

matthewwalsh0 requested changes May 28, 2026

View reviewed changes

pedronfigueiredo force-pushed the pnf/remote-feature-flag-renormalization branch from 37347e8 to 8576910 Compare May 28, 2026 09:59

pedronfigueiredo requested a review from matthewwalsh0 May 28, 2026 09:59

pedronfigueiredo force-pushed the pnf/remote-feature-flag-renormalization branch from 66f42ad to 0f896ee Compare May 28, 2026 11:31

cursor Bot reviewed May 28, 2026

View reviewed changes

Comment thread packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts Outdated

cursor Bot reviewed May 28, 2026

View reviewed changes

Comment thread packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts

fix: Normalize remote feature flag value wrappers

761c717

pedronfigueiredo force-pushed the pnf/remote-feature-flag-renormalization branch from a525b59 to 761c717 Compare May 28, 2026 13:03

matthewwalsh0 previously requested changes May 28, 2026

View reviewed changes

pedronfigueiredo added 3 commits May 28, 2026 14:28

Remove unsupported version flag wrapper normalization

4b6e1cc

Use enum for threshold versions

b0a1f11

Preserve legacy threshold wrapper fallback

9dc313d

cursor Bot reviewed May 28, 2026

View reviewed changes

Comment thread packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts Outdated

Restore direct version data return

363c9fd

DDDDDanica approved these changes May 29, 2026

View reviewed changes

NicolasMassart reviewed Jun 2, 2026

View reviewed changes

Address threshold version review notes

1de7517

pedronfigueiredo requested a review from NicolasMassart June 3, 2026 09:07

NicolasMassart approved these changes Jun 3, 2026

View reviewed changes

pedronfigueiredo added this pull request to the merge queue Jun 3, 2026

Merged via the queue into main with commit 5ac3102 Jun 3, 2026
370 checks passed

pedronfigueiredo deleted the pnf/remote-feature-flag-renormalization branch June 3, 2026 09:17

pedronfigueiredo mentioned this pull request Jun 3, 2026

Document direct-value threshold feature flags MetaMask/contributor-docs#172

Merged

Uh oh!

Conversation

pedronfigueiredo commented May 27, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Impact

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronfigueiredo May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DDDDDanica commented May 29, 2026

Uh oh!

NicolasMassart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicolasMassart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pedronfigueiredo commented May 27, 2026 •

edited by cursor Bot

Loading

pedronfigueiredo May 28, 2026 •

edited

Loading