fix(network): pin deploy-web to the managed-wallet network without a reload - #3537
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3537 +/- ##
==========================================
- Coverage 74.36% 73.43% -0.93%
==========================================
Files 1150 1060 -90
Lines 29883 27524 -2359
Branches 7475 7006 -469
==========================================
- Hits 22223 20213 -2010
+ Misses 6766 6449 -317
+ Partials 894 862 -32
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughChangesThe network store now supports pinned selection for the managed-wallet network. Wallet and onboarding flows no longer synchronize or reload network state, and related tests and deployment fixtures were updated. Managed network selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean fix for the beta onboarding reload bug, with a minor doc nit flagged inline.
Extended reasoning...
Overview
The PR removes a window.location.reload() in WalletProvider that fired whenever the selected network diverged from the managed-wallet network (only observable on beta, where default network is mainnet but managed-wallet network is sandbox). It replaces the reactive reconciliation with a new opt-in fixed option on NetworkStore (packages/network-store) that pins selectedNetworkId to a constant sourced from defaultNetworkId, with the setter throwing if ever called. deploy-web's store now sets fixed: true with defaultNetworkId pointed at NEXT_PUBLIC_MANAGED_WALLET_NETWORK_ID. The now-redundant network-setting logic in WalletProvider.loadWallet and the legacy OnboardingContainer is removed.
Security risks
None identified. No auth, crypto, or permission logic is touched. The change is limited to client-side network-selection state management.
Level of scrutiny
Low-to-medium. This is a focused bug fix confined to 6 files, with a clear root cause and a minimal, well-reasoned solution (opt-in flag rather than a broad rewrite). I verified there are no other call sites in deploy-web that invoke the network setter (which would now throw), and confirmed stats-web (the other consumer of NetworkStore) does not set fixed and is therefore unaffected — matching the PR's own claim of "no behavior change on prod" and stats-web being untouched.
Other factors
Tests were updated consistently with the removed dependencies (OnboardingContainer.spec.tsx drops the now-unused networkStore mock; DeploymentAlertsContainer.spec.tsx switches to the network-independent UACT_DENOM instead of relying on a mainnet default). One inline nit was found: the new JSDoc on NetworkStoreOptions.fixed overstates that the ?network= URL path throws on a fixed store, when in fact initiateNetworkFromUrl returns early and silently ignores it — only the direct setter throws. This is a doc-accuracy issue with no functional impact, not a blocker.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/network-store/src/network.store.ts`:
- Around line 38-45: Update the custom setter used by useSelectedNetworkIdStore
when reloadOnChange is enabled so fixed stores cannot update the network query
parameter or reload the page. Guard that reloadOnChange branch with the store’s
fixed option, or delegate fixed-store writes to selectedNetworkIdStore’s
throwing setter, while preserving the existing reload behavior for non-fixed
stores.
- Line 105: Update the fixed-network handling in the relevant network store
method so a provided ?network= parameter throws an error instead of being
silently ignored when this.options.fixed is enabled. Preserve the existing
early-return behavior when no network parameter is supplied, and align the error
with the documented URL-change contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba57f63e-42ad-425b-ac00-08fdc4f710aa
📒 Files selected for processing (6)
apps/deploy-web/src/components/alerts/DeploymentAlertsContainer/DeploymentAlertsContainer.spec.tsxapps/deploy-web/src/components/onboarding/OnboardingContainer/OnboardingContainer.spec.tsxapps/deploy-web/src/components/onboarding/OnboardingContainer/OnboardingContainer.tsxapps/deploy-web/src/context/WalletProvider/WalletProvider.tsxapps/deploy-web/src/store/networkStore.tspackages/network-store/src/network.store.ts
e028d75 to
d00014c
Compare
…reload WalletProvider forced a full page reload whenever the selected network differed from the managed-wallet network. On beta (default mainnet, managed sandbox) this fired mid-onboarding, aborting the trial-start managed-wallet creation and leaving auto-deploy waiting on a trial that never became ready. Add a `fixed` option to NetworkStore: the selected network is a constant sourced from config and any attempt to change it (setter or `?network=` URL) throws. deploy-web opts in, fixed to NEXT_PUBLIC_MANAGED_WALLET_NETWORK_ID, so the network is correct from first render with no storage and nothing to reconcile. Remove the reload effect and the now-redundant network-set calls in WalletProvider and OnboardingContainer.
d00014c to
4380d7a
Compare
There was a problem hiding this comment.
No bugs found in this run. The issues flagged earlier on this PR — the reloadOnChange setter bypassing the fixed-store guard, the stats-web TypeScript build failure from the union atom type, and the JSDoc wording mismatch — are all resolved in the current commit.
Given this changes the network-selection path used by every blockchain call in deploy-web, and the shared network-store package is also consumed by stats-web, I'd like a human to take a final look before merge.
What was reviewed:
- Ran
tsc --noEmitfor stats-web: clean, confirming the earlier CI build failure from the union atom type is gone. - Checked every other
networkStoreconsumer in deploy-web (ProviderList, BidGroup, apiUtils, urlUtils, deploymentData, providerUtils, priceUtils) — all are read-only (selectedNetworkId/selectedNetwork), none call the setter, so pinning tofixed: truedoesn't break them. - Confirmed stats-web doesn't pass
fixed, so its runtime network switching is unaffected. - Confirmed the
reloadOnChangebranch now throws for fixed stores, closing the reload bypass CodeRabbit flagged.
Extended reasoning...
Overview
This PR removes a forced window.location.reload() in WalletProvider that fired whenever the selected network differed from the managed-wallet network, which interrupted trial provisioning during onboarding on beta. It adds an opt-in fixed mode to the shared @akashnetwork/network-store package: when set, selectedNetworkId becomes a constant sourced from defaultNetworkId, the setter throws, and ?network= URL params are ignored. deploy-web opts into fixed: true pinned to NEXT_PUBLIC_MANAGED_WALLET_NETWORK_ID; stats-web is untouched since it never sets the option. Redundant network-sync calls in WalletProvider.loadWallet and OnboardingContainer are removed along with related test mocks.
Security risks
None identified. No auth, crypto, or permission logic is touched — this is purely client-side state management for which blockchain network endpoint is used.
Level of scrutiny
This warrants more than a rubber stamp: it touches the network-selection path that every blockchain-facing call in deploy-web depends on (API URLs, deployment SDL versions, provider proxy URLs), and the change lives in a shared package also consumed by stats-web. Notably, an earlier commit on this same PR had a real functional bug (the reloadOnChange setter could still bypass the fixed guard and trigger the very reload this PR aims to remove) that was only caught by automated review, not by the author — that's a signal the atom-typing logic here is subtle enough to warrant a second human look even though the current commit resolves it.
Other factors
The current commit already addresses all issues raised by CodeRabbit and my own earlier inline comment (verified above). Test changes are minor and mechanical (removing now-dead network-store mocks, swapping a denom fixture). The network-store package itself has no dedicated unit tests, which isn't a regression introduced by this PR but means the new fixed-mode branch (throwing setter, URL-param no-op, reloadOnChange guard) is only exercised indirectly through deploy-web's existing component tests.
There was a problem hiding this comment.
LGTM — a small, well-scoped fix for a real onboarding regression on beta, with the root cause clearly reasoned through and tests updated to match.
What was reviewed:
- The new
fixedmode onNetworkStore(throwing setter, ignored?network=param) and its wiring into deploy-web'snetworkStore.ts - Removal of the forced reload and now-redundant network-set calls in
WalletProviderandOnboardingContainer - Whether
reloadOnChangecould still force a reload on a fixed store — it throws before returning the reload setter, and no deploy-web caller passesreloadOnChange: true - The fixed-mode ternary's TS build compatibility (
Promise<NetworkId>union) — already investigated and ruled out this run
Extended reasoning...
Overview
The PR removes a full-page reload in WalletProvider that was interrupting onboarding trial provisioning on beta, by making deploy-web's selected network a pinned constant instead of a mutable, localStorage-backed value. This is implemented as a new opt-in fixed option on the shared @akashnetwork/network-store package (used by both deploy-web and stats-web), plus removal of now-redundant network-sync logic in WalletProvider and OnboardingContainer. Test fixtures were updated to use the network-independent UACT_DENOM instead of a mainnet-specific denom.
Security risks
None identified. This is client-side UI state management (which network id is selected for display/API routing) — no auth, crypto, or permission logic is touched. The fixed-mode setter throwing on write is a defensive guard against accidental network switching, not a security boundary.
Level of scrutiny
Moderate scrutiny is appropriate: the change is small and confined to a well-understood subsystem (network selection state), but it touches a shared package (network-store) consumed by two apps (deploy-web and stats-web), so a regression could silently affect the multi-network stats-web app if the fixed option's absence there weren't correctly preserved (it is — fixed is opt-in and stats-web's store construction is untouched). I traced the reloadOnChange path specifically because CodeRabbit flagged it as a way to bypass the fixed store's throwing setter; in the current code that path throws synchronously when fixed && reloadOnChange, before ever returning the reload-triggering setter, so the bypass CodeRabbit described is not reachable, and no deploy-web call site passes reloadOnChange: true today.
Other factors
A prior nit (already posted inline) points out the new JSDoc slightly overstates behavior — it says both the setter and a ?network= URL throw, but the URL path is actually a silent no-op via an early return. That's a documentation-only discrepancy with no functional impact and doesn't warrant blocking. The bug-hunting system's own pass this run found no functional issues, and the previously-raised concern about the fixed-mode ternary breaking the TS build for stats-web/deploy-web was investigated and ruled out. Test coverage was updated appropriately for the removed reload/network-sync behavior.
2 participants
Why
A brand-new user onboarding on beta could hit a full page reload right after landing on
/onboarding. The reload interrupted the background trial (managed-wallet) provisioning and bounced the onboarding gate (/onboarding → / → /onboarding), leaving the one-click template auto-deploy — and the "bring your own image" quote request — waiting on a trial that never became ready. It reproduced as theonboarding-journeye2e timing out (6 min / 3 min) againstconsole-beta.Root cause:
WalletProviderforcedwindow.location.reload()whenever the selected network differed from the managed-wallet network. This only ever fires on beta, where the default network (mainnet) differs from the managed-wallet network (sandbox) — on prod both aremainnet, so the reload never triggered and the bug stayed hidden.What
deploy-web has no network switcher — it always operates on the managed-wallet network — so the selected network is effectively a static, per-deployment config value. This change makes it exactly that and drops the reload:
@akashnetwork/network-store— add an opt-infixedoption. When set, the selected network is a constant sourced fromdefaultNetworkId(nolocalStorage), and any attempt to change it (the setter or a?network=URL) throws. Multi-network apps (stats-web) are unaffected.fixedtoNEXT_PUBLIC_MANAGED_WALLET_NETWORK_ID, so the network is correct from the first render on every page (including login), with nothing stored and nothing to reconcile.WalletProvider, plus the now-redundant network-set inWalletProvider.loadWalletand the legacyOnboardingContainer.No behavior change on prod (the network was already
mainneteverywhere). On beta, onboarding no longer reloads and the trial provisions uninterrupted. TheDeploymentAlertsContainerspec was updated to seed the network-independent ACT denom, since it previously relied on the app defaulting to mainnet in the staging test env.Summary by CodeRabbit