-
-
Notifications
You must be signed in to change notification settings - Fork 256
feat: add override functionality to remote feature flags #7271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: add override functionality to remote feature flags #7271
Conversation
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.test.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/transaction-controller/src/TransactionControllerIntegration.test.ts
Show resolved
Hide resolved
f131cc8 to
11c0f59
Compare
11c0f59 to
7ae47a3
Compare
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
Cal-L
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment
| * @param value - The override value for the feature flag. | ||
| */ | ||
| setFlagOverride(flagName: string, value: Json): void { | ||
| this.update(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can spread ...this.state for unaffected states
| this.update(() => { | ||
| return { | ||
| remoteFeatureFlags: this.state.remoteFeatureFlags, | ||
| localOverrides: newOverrides, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spread ..this.state for unaffected states
| this.update(() => { | ||
| return { | ||
| remoteFeatureFlags: this.state.remoteFeatureFlags, | ||
| localOverrides: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spread state for unaffected states
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Show resolved
Hide resolved
mcmire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I had some comments on this PR. Most are minor.
I think the main thing I am trying to understand is what these overrides actually do. I started reviewing thinking that they affected how this controller returns feature flags — that perhaps the local overrides were mixed into the flags returned by the API. But that is not what seems to be happening. If we are expecting the consumer to really say state.localOverrides[flagName] ?? state.remoteFeatureFlags[flagName] when they want to access a feature flag, that seems inconvenient to me. Let me know your thoughts.
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
|
|
||
| // Access flag value with override taking precedence | ||
| const flagValue = | ||
| controller.state.localOverrides.testFlagForThreshold ?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using ?? here? Do we not expect controller.state.localOverrides.testFlagForThreshold to be set, and if not, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed test
| }); | ||
|
|
||
| describe('integration with remote flags', () => { | ||
| it('preserves overrides when remote flags are updated', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a large number of steps represented here for what I think ought to be a simple test (given my reading of the logic). Similar to other suggestions I've made, is it enough to set up the initial state instead of having to call a few methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
|
|
||
| await controller.updateRemoteFeatureFlags(); | ||
|
|
||
| // Mock different remote response for second fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mentions that we are mocking the request for the second time, but where are we mocking the request for the first time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first fetch would have been created the initial remote value. I will update the comment to be more clear
| }); | ||
| }); | ||
|
|
||
| it('overrides work with threshold-based feature flags', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this test testing? If it's merely testing that overrides do not change when updateRemoteFeatureFlags is called, isn't that what the previous test is for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had changed the logic for threshold based feature flag. so this test is not necessary anymore. removed!
…g-controller.ts Co-authored-by: Elliot Winkler <[email protected]>
Co-authored-by: Elliot Winkler <[email protected]>
…g-controller.ts Co-authored-by: Elliot Winkler <[email protected]>
…g-controller.ts Co-authored-by: Elliot Winkler <[email protected]>
…g-controller.ts Co-authored-by: Elliot Winkler <[email protected]>
…g-controller.ts Co-authored-by: Elliot Winkler <[email protected]>
Co-authored-by: Elliot Winkler <[email protected]>
vinnyhoward
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Cal-L
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Remote Feature Flag Controller - Override Support & A/B Test Visibility
Explanation
Current State & Problem
The RemoteFeatureFlagController currently only supports remote feature flags with no ability to locally override them. This creates several pain points for developers and QA teams:
Solution Overview
This PR adds local override functionality with A/B test visibility through direct state access:
Core Features:
localOverridesstate field allows manual flag overrides that take precedence over remote flagsrawRemoteFeatureFlagsfield stores raw A/B test arrays (preserves original flag data before processing)setFlagOverride,clearFlagOverride,clearAllOverrides)How It Works:
#updateCachemethod now stores raw flag data inrawRemoteFeatureFlagsbefore processing A/B tests into single valuesstate.localOverrides[flagName] ?? state.remoteFeatureFlags[flagName]for override-aware access#updateCachemethodImplementation Details
The implementation in metamask-mobile can be found in this PR
State Management: Three state fields work together:
remoteFeatureFlags: Processed flags (A/B tests resolved to single values)localOverrides: Manual overrides that take precedencerawRemoteFeatureFlags: Original flag data before processing (preserves A/B test arrays)Override Persistence: Remote flag updates preserve existing local overrides through state preservation in
#updateCacheMessenger Integration: All override methods are exposed as controller actions for external access
Metadata Configuration: All new state fields are properly configured for logging, persistence, and UI exposure
Access Patterns
Getting Flag Values (with override support):
Getting the effective Value can be retrieved from the remote feature flag selector by processing the overrides on it as such
Accessing A/B Test Groups:
View available A/B test options can be done through the rawRemoteFeatureFlags selector
Managing Overrides:
Dependencies & Imports
Jsontype import from@metamask/utilsfor type safetyReferences
This enhancement addresses the need for local flag testing and A/B test visibility that has been requested by development and QA teams for improved testing workflows.
Checklist
Note: No breaking changes were introduced - this is purely additive functionality that maintains full backward compatibility.
Note
Adds local override support and raw flag storage to the RemoteFeatureFlagController, updates state/metadata and exports, and updates tests/consumers accordingly.
localOverridesandrawRemoteFeatureFlags; update metadata and default state; include in logs/persistence and expose relevant fields to UI.setFlagOverride(flagName, value),removeFlagOverride(flagName),clearAllFlagOverrides().#updateCachenow preserveslocalOverridesand storesrawRemoteFeatureFlagsalongside processed flags.controllerName; extend actions withRemoteFeatureFlagControllerSetFlagOverrideAction,RemoveFlagOverrideAction,ClearAllFlagOverridesActionand re-export them insrc/index.ts.CHANGELOG.mdwith new features and metadata changes.Written by Cursor Bugbot for commit fb818a8. This will update automatically on new commits. Configure here.