feat: Add FlagOptions for consolidated feature flag configuration - #896
Merged
Conversation
Introduces FlagOptions with builder pattern to group feature flag settings (enabled, context, loadOnFirstForeground) into a single configuration object. Deprecates the flat featureFlagsEnabled/featureFlagsContext params on MixpanelOptions in favor of the new setFlagOptions() builder method. The new loadOnFirstForeground option allows callers to control whether flags are automatically fetched on the first app foreground event. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@dillon-mce here's the Android version of the option to not load flags on first foreground! |
tylerjroach
requested changes
Feb 27, 2026
| * or an empty JSONObject if the source is {@code null} or copying fails. | ||
| */ | ||
| @NonNull | ||
| public static JSONObject defensiveCopy(@Nullable JSONObject source) { |
Contributor
There was a problem hiding this comment.
This seems unnecessary with proper nullability annotations and being defensive elsewhere before attempting to convert. Just feels like a utility function that will be forgotten about, used in some places, but not others.
| * @param enabled {@code true} to enable feature flags, {@code false} to disable. | ||
| * @return This Builder instance for chaining. | ||
| */ | ||
| public Builder setEnabled(boolean enabled) { |
Contributor
There was a problem hiding this comment.
current builder pattern is to not use set at the start of these as setting should be obvious given that we are in builder pattern. We don't use set for the mixpanel options
| private FlagOptions(Builder builder) { | ||
| this.mEnabled = builder.mEnabled; | ||
| this.mLoadOnFirstForeground = builder.mLoadOnFirstForeground; | ||
| this.mContext = builder.mContext != null ? builder.mContext : new JSONObject(); |
Contributor
There was a problem hiding this comment.
See argument against defensive copy utility since this would be a good way to ensure not null.
Contributor
Author
|
K @tylerjroach, I think I've addressed your concerns by:
|
… prefetchFlags Improves naming clarity for the feature flag configuration class and its auto-load parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tylerjroach
approved these changes
Mar 4, 2026
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
2 participants
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.
Summary
FlagOptionsclass (builder pattern) that groups feature flag settings (enabled,context,loadOnFirstForeground) into a single configuration objectsetFlagOptions(FlagOptions)toMixpanelOptions.Builder, deprecating the flatfeatureFlagsEnabled()andfeatureFlagsContext()methods (backward compatible — flat params are auto-wrapped into aFlagOptionswhen the new API isn't used)loadOnFirstForegroundoption so callers can control whether flags are automatically fetched on the first app foreground event🤖 Generated with Claude Code