fix: feature staying enabled despite its feature flag disabling [LW-12195] #1684
Conversation
| @@ -60,71 +62,67 @@ export class PostHogClient<Action extends string = string> { | |||
| private postHogHost: string = POSTHOG_HOST | |||
| ) { | |||
| if (!this.postHogHost) throw new Error('POSTHOG_HOST url has not been provided'); | |||
|
|
|||
| // eslint-disable-next-line promise/catch-or-return | |||
| this.initializePosthogClient().then(() => { | |||
There was a problem hiding this comment.
Two improvements with initializePosthogClient initializeFromBackgroundStorage:
- they are not chained - previously posthog initialization was delayed by accessing BG storage
- extracting to methods improves the readibility of the constructor so it is clear what happens here.
| const payload = payloadsByFeature[feature as FeatureFlag]; | ||
| const allowedNetworks = payload ? payload.allowedNetworks : []; | ||
| flags[feature as FeatureFlag] = isFeatureEnabled && allowedNetworks.includes(networkName); | ||
| const handleFeatureFlagsUpdate: Parameters<typeof posthog.onFeatureFlags>[0] = async (_, loadedFeatureFlags) => { |
There was a problem hiding this comment.
That callback extracted to improve visibility of sending to hasPostHogInitialized$ stream which happens bellow
| if (NETWORK_NAME_TO_NETWORK_MAGIC.hasOwnProperty(networkName)) { | ||
| const networkMagic = NETWORK_NAME_TO_NETWORK_MAGIC[networkName]; | ||
|
|
||
| for (const feature in featureFlags) { |
There was a problem hiding this comment.
Iterating over received feature flags seems the reason of the issue.
- posthog does not send FFs which are disabled
- recently we introduced initialization of feature flags with a data from BG storage (last known feature flag values)
- dapp-explorer was set enabled during initialization
- it was never overridden as we didn;t receive it from posthog
Allure Report
processReports: ✅ test report for cb26170b
|
| } | ||
| } | ||
|
|
||
| const backgroundStorage = await this.backgroundServiceUtils.getBackgroundStorage(); | ||
|
|
||
| if (!PostHogClient.areAllFeatureFlagsEmpty(this.featureFlags)) { |
There was a problem hiding this comment.
I don't see a value in checking if flags are empty (the check was looking if there is no single FF key). They cannot be empty as we now iterate over defined list of flags.
If there is any change it is intentional - saving in storage always
| // if we were not able to retrieve posthog experiment config, use local config | ||
| if (PostHogClient.areAllFeatureFlagsEmpty(this.featureFlags)) { | ||
| // override posthog experiment config with local | ||
| posthog.featureFlags.override( |
There was a problem hiding this comment.
We don't need to override as we don't access FFs via posthog client directly. We keep them in the this.featureFlagsByNetwork
| import { useEffect, useState } from 'react'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types | ||
| export const useIsPosthogClientInitialized = () => { |
There was a problem hiding this comment.
This hook replaced entire experiments context that was exposing Provider, hook and was providing down the context some unused properties.
szymonmaslowski
force-pushed
the
fix/LW-12195-fix-feature-flags
branch
2 times, most recently
from
ad3171c to
3c9a451
Compare
| @@ -203,17 +203,17 @@ export const BrowserViewRoutes = ({ routesMap = defaultRoutes }: { routesMap?: R | |||
| }); | |||
|
|
|||
| const isLoaded = useMemo( | |||
| () => !areExperimentsLoading && !isLoadingWalletInfo && walletInfo && walletState && initialHdDiscoveryCompleted, | |||
| [areExperimentsLoading, isLoadingWalletInfo, walletInfo, walletState, initialHdDiscoveryCompleted] | |||
| () => posthogClientInitialized && !isLoadingWalletInfo && walletInfo && walletState && initialHdDiscoveryCompleted, | |||
There was a problem hiding this comment.
what happens to the application if a request to posthog can't be made?
There was a problem hiding this comment.
- if there is last value stored in background storage then client will use it
- if no value in background storage then every FF disabled.
szymonmaslowski
force-pushed
the
fix/LW-12195-fix-feature-flags
branch
from
3c9a451 to
f5fbcab
Compare
- removed a check for existance of particular ff in the posthog response from the iteration over all defined feature flags. Posthog does not respond with feature flags that are disabled. Therefore those been switched off didn't change their current value as they were skipped. - dead code removal - naming improvements - removed experiments context (mostly dead code) and implemented a simple useIsPosthogClientInitialized() hook in its place
szymonmaslowski
force-pushed
the
fix/LW-12195-fix-feature-flags
branch
from
f5fbcab to
cb26170
Compare
|
Checklist
Proposed solution
Testing
allowedNetworksproperty and ensure the feature does not load for given networkScreenshots
Attach screenshots here if implementation involves some UI changes