Skip to content

fix(account): wait for remote config before the first balances fetch - #3064

Merged
daniel-carroll-stellar merged 4 commits into
stellar:masterfrom
daniel-carroll-stellar:fix/gate-balances-on-remote-config
Nov 10, 2026
Merged

daniel-carroll-stellar merged 4 commits into
stellar:masterfrom
daniel-carroll-stellar:fix/gate-balances-on-remote-config

Conversation

@daniel-carroll-stellar

@daniel-carroll-stellar daniel-carroll-stellar commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #3032

What

The account screen now waits for remote config flags from Amplitude to load before fetching balances in order to prevent one errant v1 call for users with v2 enabled. Updated test wrapper to default remote config to loaded since the account screen now waits on it.

Why

This bug would skew v1 vs v2 rollout comparison, since every v2 user would also be sending a v1 call. I went with the first proposed fix (waiting for the flags) instead of the seeding approach because this is similar to how mobile handles the issue, so this ensures parity between the two versions.

How I verified

I locally tested a fake slow Amplitude call with v2 enabled to ensure that the screen would wait and a v1 call would not appear. Regression testing also asserts that balances are not to be called until the flags are initialized.

Screenshots / video

N/A. Not a UI change.

Checklist

  • I have read CONTRIBUTING.md and this PR meets its requirements.
  • This PR has narrow scope and does not mix refactoring with feature changes.
  • This PR adds tests for the new functionality or fixes.
  • These changes have been tested in both Chrome and Firefox.
  • This PR includes relevant before and after screenshots for UI changes.
  • I took the time to review my own PR.

The Account view fired its first balances fetch on mount, before the
Amplitude flags had loaded. useGetBalances reads use_balances_v2 at call
time, so that first request read the hardcoded default (false) and went
to the v1 endpoint even for users enrolled in v2. Later polls used v2,
which skewed the v1/v2 rollout comparison.

The mount effect now waits for isRemoteConfigInitialized. The reducer
sets it on the failure path too, so Amplitude being down costs one
round trip instead of a stuck screen. The view already renders IDLE as
loading, so the wait shows the existing spinner.

The test Wrapper now preloads a settled remoteConfig slice; without it
every Account test would sit on the loader. Adds a regression test that
asserts no balances request goes out until the flags settle.

Fixes stellar#3032

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@daniel-carroll-stellar
daniel-carroll-stellar marked this pull request as ready for review November 6, 2026 17:04
@piyalbasu

piyalbasu commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Gate the flag read inside the balances/prices hooks, not in the Account view

Medium — the race in #3032 is still live on every entry point other than the Account route. Merge blocker as-is; the fix is small.

TL;DR: The Account screen now waits for feature flags before its first balances fetch, but the flag is read at call time inside the shared balances and token-prices hooks, and those hooks are called from ~27 places. The sign-transaction popup, every fullscreen tab (history, send, swap, add asset…), and sidebar mode each open a fresh document with an uninitialized store and never pass through the Account effect, so their first request still goes to v1 for a v2 user. Moving the wait to where the flag is read fixes all entry points at once, lets the Account change and the shared test-helper change be reverted, and the e2e suite needs a default stub for the Experiment request either way. Separately, any wait on the flag fetch inherits the Experiment SDK's 10 second default timeout with errors swallowed, so a slow or blocked Amplitude host now means up to 10 seconds of spinner on every popup open; shorten that timeout to 1 to 2 seconds wherever the gate ends up living.

Steps to reproduce:

  1. Put the account in the v2 balances variant in Amplitude.
  2. Trigger a dApp signing request so the sign-transaction popup opens as a new document, or open any fullscreen tab from the account menu.
  3. Watch the network tab: the first balances request goes to the v1 endpoint, same as before this PR.

Detailed explanation (for agents)

Root cause: the gate is upstream of only one caller. The read sites are:

The new gate only covers the Account effect:

// The balances fetch reads feature flags, so wait until they've loaded.
useEffect(() => {
if (!isRemoteConfigInitialized) {
return;
}
const getData = async () => {
await fetchData({ useAppDataCache: false });
};
getData();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isRemoteConfigInitialized]);

fetchFeatureFlags is dispatched once per document from App via useRemoteConfig, so every openTab(newTabHref(...)) target, the SignTransaction popup, and the sidebar start with isInitialized: false and race the thunk exactly as Account did.

Suggested fix: add one helper to popup/ducks/remoteConfig.ts that resolves when the slice has settled, and await it at the read sites. Both hooks already hold store from useStore, so no new plumbing:

export const waitForRemoteConfig = (store: Store): Promise<void> =>
  new Promise((resolve) => {
    if (isRemoteConfigInitializedSelector(store.getState())) return resolve();
    const unsubscribe = store.subscribe(() => {
      if (isRemoteConfigInitializedSelector(store.getState())) {
        unsubscribe();
        resolve();
      }
    });
  });
// useGetBalances.fetchData, before getAccountBalances
await waitForRemoteConfig(store);
const useV2 = balancesV2Selector(store.getState());

Same two lines at the three useGetTokenPrices reads and in AddToken. The rejected case already sets isInitialized, so an Amplitude failure releases the wait:

.addCase(fetchFeatureFlags.rejected, (state) => {
// Mark as initialized with safe defaults so we don't retry and block
// the user if the network or SDK call fails.
state.isInitialized = true;
});

With that in place the Account gate and the remoteConfig default added to the shared Wrapper in popup/__testHelpers__/index.tsx can be reverted. The Wrapper default matters beyond Account: setting isInitialized: true for every test file flips the fail-closed kill-switch selectors (e.g. the price-history chart) from off to on across the suite, so tests that never mocked those fetches now mount them.

Two things the gate does not cover on its own, wherever it lives:

  1. Hang cap. The thunk has no timeout of its own and client.fetch() defaults to a 10s fetchTimeoutMillis with errors swallowed, so a slow or black-holed api.lab.amplitude.com (VPN, captive portal) now shows a spinner for up to 10s on every open. Pass a shorter fetchTimeoutMillis (1–2s) in initExperimentClient, or race the helper against a short cap and fall through to the defaults:
    try {
    client = AMPLITUDE_EXPERIMENT_DEPLOYMENT_KEY
    ? Experiment.initializeWithAmplitudeAnalytics(deploymentKey)
    : Experiment.initialize(deploymentKey);
    } catch (e) {
  2. e2e stub. The vardata route is stubbed only in the maintenance-mode tests, so every other e2e test's login step now waits on a real request to Amplitude with the playwright deployment key. A default **/sdk/v2/vardata** stub in the shared login helper removes that dependency:
    const AMPLITUDE_EXPERIMENT_VARDATA_ROUTE = "**/sdk/v2/vardata**";

On the regression test: the negative assertion runs after one microtask tick, but the unfixed path needs several awaits (loadAccount → loadSettings → fetchBalances) before reaching getAccountBalances, so it is not established that it fails against the unfixed code. It also asserts only toHaveBeenCalled() after dispatching use_balances_v2: true; asserting the useV2 argument is what pins #3032. A hook-level test that calls fetchData on a store with isInitialized: false, then dispatches fetchFeatureFlags.fulfilled, and checks the argument is the discriminating version:

await Promise.resolve();
});
expect(balancesSpy).not.toHaveBeenCalled();
await act(async () => {
getTestStore()?.dispatch({
type: fetchFeatureFlags.fulfilled.type,

@piyalbasu

Copy link
Copy Markdown
Contributor

Every stubbed e2e test now waits on a real Amplitude Experiment request

Medium — likely to show up as slow or flaky e2e runs, especially on fork CI where egress is constrained. Merge blocker: this should land with the gate, since the gate is what puts the request on the critical path.

TL;DR: The e2e suite stubs every external API the popup calls during login, but the Amplitude Experiment flag request is stubbed only in the maintenance-mode spec. Before this PR that did not matter because nothing waited on it. Now the Account screen blocks its first balances fetch until the flag request settles, so every other spec's login step makes a real request to Amplitude with the fake playwright deployment key and waits for it to answer or time out. The fix is a default Experiment stub in the shared stub helper so the response is immediate and local.

Steps to reproduce:

  1. Run any e2e spec other than maintenance mode with outbound access to Amplitude blocked or throttled.
  2. Watch the Account screen after login: balances do not appear until the Experiment request fails over, up to the SDK's 10 second default timeout, where before this PR they loaded immediately.

Detailed explanation (for agents)

Root cause: stubAllExternalApis is the shared setup every stubbed spec runs through loginToTestAccount, and it has no route for the Experiment variants endpoint:

export const stubAllExternalApis = async (
page: Page,
context: BrowserContext,
) => {
// User notification
await stubUserNotification(context);
// Feature flags
await stubFeatureFlags(context);
// RPC health is fetched by the background service worker
await stubRpcHealth(context);
// Subscription account
await stubSubscriptionAccount(context);
// Stellar Asset List
await stubStellarAssetList(page);
await stubAssetSearch(page);
// Token metadata
await stubTokenDetails(page);
await stubTokenPrices(page);
await stubIsSac(page);

The only vardata route is the maintenance-mode helper, which is registered per test and only in maintenanceMode.test.ts:

const AMPLITUDE_EXPERIMENT_VARDATA_ROUTE = "**/sdk/v2/vardata**";
const MAINTENANCE_VARIANT_VALUE = "on";
const stubExperimentVariants = async (
page: Page,
variants: Record<string, { payload?: unknown }>,
) => {
await page.route(AMPLITUDE_EXPERIMENT_VARDATA_ROUTE, async (route) => {
const response = Object.fromEntries(
Object.entries(variants).map(([flagKey, variant]) => [
flagKey,
{
key: MAINTENANCE_VARIANT_VALUE,
value: MAINTENANCE_VARIANT_VALUE,
...(variant.payload !== undefined
? { payload: variant.payload }
: {}),
},
]),
);
await route.fulfill({ json: response });

Playwright sets IS_PLAYWRIGHT, so initExperimentClient initializes a real client with the test deployment key rather than skipping:

const deploymentKey =
AMPLITUDE_EXPERIMENT_DEPLOYMENT_KEY ||
(isRuntimeTestEnv() ? TEST_EXPERIMENT_DEPLOYMENT_KEY : "");
if (!deploymentKey) {
if (!isDev && !isRuntimeTestEnv()) {
console.error(
`${LOG_MESSAGES.EXPERIMENT_PREFIX} ${LOG_MESSAGES.MISSING_KEY}`,
);
}
return;
}
try {
client = AMPLITUDE_EXPERIMENT_DEPLOYMENT_KEY
? Experiment.initializeWithAmplitudeAnalytics(deploymentKey)
: Experiment.initialize(deploymentKey);
} catch (e) {

fetchFeatureFlags then calls client.fetch() against api.lab.amplitude.com. The SDK defaults are fetchTimeoutMillis: 10000 and throwOnError: false (verified in the installed @amplitude/[email protected] bundle), so an unreachable host resolves silently after 10s and the thunk marks the store initialized with defaults. With the new gate in Account, that 10s is now in front of the first balances fetch in every spec:

// The balances fetch reads feature flags, so wait until they've loaded.
useEffect(() => {
if (!isRemoteConfigInitialized) {
return;
}
const getData = async () => {
await fetchData({ useAppDataCache: false });
};
getData();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isRemoteConfigInitialized]);

Note the existing stubFeatureFlags routes the freighter-backend /feature-flags endpoint, not Amplitude, so it does not cover this.

Suggested fix: add a default Experiment stub to stubAllExternalApis that returns an empty variant map, so every spec gets the reducer defaults instantly. Register it on the context so it also covers any fullscreen tab or popup the spec opens:

export const stubExperimentDefaults = async (context: BrowserContext) => {
  await context.route(AMPLITUDE_EXPERIMENT_VARDATA_ROUTE, async (route) => {
    await route.fulfill({ json: {} });
  });
};

and call it next to stubFeatureFlags(context) in stubAllExternalApis. The maintenance-mode helpers register a more specific page.route afterwards and reload, so they keep winning for those specs. Integration-mode runs (isIntegrationMode: true) skip all stubbing and are unaffected either way.

This is needed wherever the gate ends up living. If the PR moves to seeding the store from the SDK's persisted variants instead of waiting, the request leaves the critical path and this becomes a hygiene fix rather than a blocker.

Move the remote config wait from the Account view into the five places
that read a flag to pick an endpoint (useGetBalances, useGetTokenPrices,
AddToken), so the popup, fullscreen tabs, sidebar, and sign-transaction
window all wait. Cap the Amplitude fetch at 1.5s, stub the Experiment
request by default in e2e, and keep the price-history kill switch off in
the test Wrapper.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@daniel-carroll-stellar

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Pushed 19edd8c with the changes:

Gate moved into the hooks

  • Added waitForRemoteConfig(store) to ducks/remoteConfig.ts and await it before each flag read. Now all the places that pick between v1 and v2 will wait for the flag.
  • Reverted the Account view change since this lives in the hooks now.
  • Set fetchTimeoutMillis: 1500 on the Experiment client, so a blocked Amplitude host holds the spinner for 1.5s instead of 10s.

e2e

  • Added stubExperimentDefaults to stubAllExternalApis. It returns {} for **/sdk/v2/vardata** so the tests don't have to wait for a real Amplitude call.

Tests

  • New hook-level tests in useGetBalances.test.tsx and useGetTokenPrices.test.tsx. Each starts a fetch with flags unsettled, checks nothing was called, settles the flags, then asserts the useV2 argument (instead of just a generic check like last time). Both fail with the wait disabled ("expected 0 calls, received 1") and pass with it.

Test helper

  • Kept isInitialized: true in the shared Wrapper rather than fully reverting. Unit tests never load the flags, so with the wait now in the hooks, reverting it makes 56 tests across 8 suites fail: the screens stay on the loading spinner and their assertions time out. I set use_token_price_history: false there so the price chart stays off in tests, same as on master.

@piyalbasu

@piyalbasu piyalbasu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - this is looking good to me now

@netlify

netlify Bot commented Nov 10, 2026

Copy link
Copy Markdown

👷 Deploy request for freighter-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit ebe81db

@daniel-carroll-stellar
daniel-carroll-stellar merged commit 63e32be into stellar:master Nov 10, 2026
7 checks passed
JakeUrban pushed a commit that referenced this pull request Nov 10, 2026
Resolve the conflict with #3064 in useGetTokenPrices. That change waits
for the remote config flags before it reads use_token_prices_v2. This
branch removes that flag, so the price hook does not wait for the flags.
The balances hook still waits, because use_balances_v2 picks its
endpoint.

- Replace the new "waits for the flags" price test with a test that
  prices are fetched without a wait.
- Remove use_token_prices_v2 from the new test fixtures.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01R8cp3zUn1aPP4zQrPvEFDQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Account screen calls v1 balances on popup open before use_balances_v2 flag loads

3 participants