-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: import newremote feature flag controller with override functionality #23487
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?
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
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.
The engine ready checks might not be necessary assuming this context is used in the right place. We already have a ControllersGate that blocks the majority of components from rendering before the engine is initialized. Can we rely on that instead?
Yes, correct, I don't think it is necessary at all. I had that logic because I was having trouble getting it to start I thought the issue was that the controller wasn't loading but it was because the controller was broken and wasn't building |
8003948 to
06336f2
Compare
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsThis PR modifies the core feature flag infrastructure, specifically the Key changes:
The
Because this selector is foundational infrastructure that gates nearly every major feature, any subtle bugs in how overrides are merged could cause unexpected behavior across the entire app. The comprehensive tag selection ensures we catch any regressions in feature-flag-controlled functionality. |
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 some comments
| /> | ||
| ); | ||
| case 'abTest': { | ||
| interface AbTestType { |
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.
Move type outside of component
| ios_backgroundColor={theme.colors.border.muted} | ||
| /> | ||
| ); | ||
| case 'abTest': { |
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.
While extracting types, also cleaner to extract these into enums
| // Local state for overrides | ||
| const [overrides, setOverrides] = useState<FeatureFlagOverrides>({}); | ||
| // Subscribe to controller state changes to ensure we stay in sync | ||
| useEffect(() => { |
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 doesn't seem like it should do anything since it's triggering an empty function from inside of the useEffect. What's the intended purpose here - is it to get the latest state every time the feature flag changes?
| } | ||
|
|
||
| // Helper to safely access the RemoteFeatureFlagController with proper typing | ||
| const getRemoteFeatureFlagController = (): |
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 does not need to be a function since it's just returning a reference
| | undefined; | ||
|
|
||
| // Helper to safely execute controller methods with error handling | ||
| const withRemoteFeatureFlagController = ( |
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 with naming convention is usually used for higher order wrappers, which typically used to wrap classes or functional components. Following the logic, it looks like this is intended to handle catching error on method calls for the remote feature flag controller. A few suggestions to make this cleaner:
- Create a remote feature flag hook instead,
useRemoteFeatureFlagController - In the hook, return remote feature flag functions that gracefully handle method call failures
| export const selectRemoteFeatureFlags = createSelector( | ||
| selectRemoteFeatureFlagControllerState, | ||
| (remoteFeatureFlagControllerState) => { | ||
| (remoteFeatureFlagControllerState: unknown) => { |
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.
We should use the state type from RemoteFeatureFlagController
| | 'array' | ||
| | 'boolean with minimumVersion' | ||
| | 'boolean nested' | ||
| | 'abTest' |
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.
Cleaner to abstract these into an enum
Description
This PR adds comprehensive override functionality to the
RemoteFeatureFlagControllerthat was added in this PR, allowing developers and users to locally override remote feature flags for testing and debugging purposes. The implementation also includes enhanced A/B test visibility features that provide access to available test groups.What is the reason for the change?
The need to override remote feature flags locally for development, testing, and debugging scenarios without modifying the remote configuration.
What is the improvement/solution?
Changelog
CHANGELOG entry: Added override functionality to remote feature flags with methods to set, get, clear overrides and access A/B test groups
Related issues
Fixes:
Manual testing steps
Feature: Remote Feature Flag Override Functionality
Scenario: user sets a local override for a feature flag
Given the RemoteFeatureFlagController is initialized with remote flags
When user calls setFlagOverride('testFlag', true)
Then testFlag selector should return the override value true
And getFlagOverride('testFlag') should return true
Scenario: user clears a specific flag override
Given a feature flag has a local override set
When user calls clearFlagOverride('testFlag')
Then testFlag selector should return the original remote value
And getFlagOverride('testFlag') should return undefined
Scenario: user accesses A/B test groups
Given remote flags contain A/B test configurations
When user calls rawProcessedRemoteFeatureFlags selector
Then it should return an array of available test groups with names and values
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist