-
Notifications
You must be signed in to change notification settings - Fork 275
Ensure onboarding feature flag is loaded from remote source #4882
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
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.
Pull request overview
This PR addresses an edge case where the onboarding feature flag is queried before Firebase Remote Config finishes loading, causing new app installs to see the old onboarding flow despite remote configuration. The solution introduces an awaitInitialization() method to all feature providers and a new isEnabledWithRemote() function that suspends until remote config is fetched.
Key changes:
- Added
awaitInitialization()method to theFeatureProviderinterface, implemented across all providers - Created
isEnabledWithRemote()function that waits for remote config initialization with a configurable timeout - Updated onboarding flow to use the new async feature flag check to ensure remote values are loaded before rendering
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/featureflag/FeatureProvider.kt | Adds awaitInitialization() method to the interface |
| modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/featureflag/providers/FirebaseRemoteFeatureProvider.kt | Implements awaitInitialization() using CompletableDeferred that completes when Firebase fetch succeeds or fails |
| modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/featureflag/providers/PreferencesFeatureProvider.kt | Implements awaitInitialization() returning true immediately (no async work needed) |
| modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/featureflag/providers/DefaultReleaseFeatureProvider.kt | Implements awaitInitialization() returning true immediately (no async work needed) |
| modules/services/sharedtest/src/main/java/au/com/shiftyjelly/pocketcasts/sharedtest/InMemoryFeatureProvider.kt | Implements awaitInitialization() returning true immediately for test provider |
| modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/featureflag/FeatureFlag.kt | Adds isEnabledWithRemote() function that awaits all provider initialization with timeout before returning feature value |
| modules/features/account/src/main/java/au/com/shiftyjelly/pocketcasts/account/onboarding/OnboardingFlowComposable.kt | Uses produceState with isEnabledWithRemote() to await remote config before building navigation graph |
| app/src/main/java/au/com/shiftyjelly/pocketcasts/ui/MainActivity.kt | Wraps notification permission check in coroutine to await remote feature flag value |
| if (FeatureFlag.isEnabled(Feature.NEW_ONBOARDING_ACCOUNT_CREATION)) { | ||
| navController.popBackStack() | ||
| } else { | ||
| exitOnboarding(OnboardingExitInfo.Simple) | ||
| } | ||
| }, | ||
| onComplete = { | ||
| val route = if (FeatureFlag.isEnabled(Feature.NEW_ONBOARDING_ACCOUNT_CREATION)) { |
Copilot
AI
Jan 6, 2026
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.
After waiting for the remote config to load with isEnabledWithRemote(), these subsequent calls to isEnabled() for the same feature flag could potentially return inconsistent values. While the remote config should now be loaded, it's safer and clearer to use the already-loaded useNewOnboardingFlow value instead of making repeated synchronous checks that bypass the await logic.
Consider using the useNewOnboardingFlow variable (which comes from the awaited remote value) instead of calling FeatureFlag.isEnabled() again.
| if (FeatureFlag.isEnabled(Feature.NEW_ONBOARDING_ACCOUNT_CREATION)) { | |
| navController.popBackStack() | |
| } else { | |
| exitOnboarding(OnboardingExitInfo.Simple) | |
| } | |
| }, | |
| onComplete = { | |
| val route = if (FeatureFlag.isEnabled(Feature.NEW_ONBOARDING_ACCOUNT_CREATION)) { | |
| if (useNewOnboardingFlow) { | |
| navController.popBackStack() | |
| } else { | |
| exitOnboarding(OnboardingExitInfo.Simple) | |
| } | |
| }, | |
| onComplete = { | |
| val route = if (useNewOnboardingFlow) { |
...services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/featureflag/FeatureFlag.kt
Show resolved
Hide resolved
|
@sztomek I would like to ask Michał to review this once he is back from AFK. |
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 do you think about adjusting this slightly? Instead of exposing FeatureFlag.isEnabledWithRemote(), we could expose FeatureFlag.awaitRemoteProviders().
The main issue I see with isEnabledWithRemote() is that it does not compose well with the rest of the API surface, such as isEnabledFlow(), isEnabledForUser(), and isExclusiveToPatron(). Introducing an explicit awaitRemoteProviders() call would allow all feature-flag accessors to share the same initialization mechanism.
With this approach, usage would allow more flexibility.
produceState<Boolean?>(null) {
FeatureFlag.awaitRemoteProviders()
FeatureFlag.isEnabled(Feature.Foo) // Or any other getter
}
val enabledFlow = flow {
FeatureFlag.awaitRemoteProviders()
emitAll(FeatureFlag.isEnabledFlow(Feature.Foo))
}
Description
This PR aims to cover an edge case we've identified last year.
Feature initialization happens in an async manner (when we're talking about prod builds) inside the ctor of
FireabaseRemoteFeatureProvider. If a flag is queried while the remote config is being fetched, we simply return the default value.That resulted the very awkward scenario with the onboarding feature launch -- people who just installed the app saw the old onboarding despite we had the flags properly configured on Firebase. The default value of
Feature.NEW_ONBOARDING_ACCOUNT_CREATIONwas false at this time, we changed it later to true so the issue was seemingly solved.This is my last attempt to fix the issue ultimately. My initial attemtps were:
androidx.startup.Initializerto init firebase provider. that was a dead end beacuse the initializer is not blocking the app launch flowMainActivityto init the feature providers inonCreatebefore doing anything else. that did not work fine either, i had many issues with how we launch the onboarding activity. multiple onboarding activities could be created or none of them were launched at all.So i finally went with this approach and created a suspend function that will wait until all the providers are inited. Yeah,
FeatureProvider.awaitInitializationhas been also introduced for this purpose.This isn't ideal either, because you'll need to 'know' that a feature flag is expected to be called early in the app launch flow...
The bulletproof path would be to rework our whole app launch flow and stay of splash until everything gets initialized.
Testing Instructions
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml