-
Notifications
You must be signed in to change notification settings - Fork 76
refactor: injects browserEnvConfig from DI container #2453
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
📝 WalkthroughWalkthroughThis PR refactors the deploy-web application to source public configuration from a ServicesProvider context (via useServices()) instead of directly importing browserEnvConfig. The migration spans 14 files across components, hooks, and configuration, removes the NEXT_PUBLIC_BILLING_ENABLED property, and eliminates billing-based visibility gating from several wallet and payment-related components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
7d5695f to
35b76e7
Compare
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @apps/deploy-web/src/components/layout/WalletStatus.tsx:
- Line 84: WalletStatus.tsx renders ManagedWalletPopup and
ConnectManagedWalletButton based only on isManaged and !isSignedInWithTrial,
which is inconsistent with GetStartedStepper.tsx and UserProviders.tsx that gate
behavior on publicConfig.NEXT_PUBLIC_BILLING_ENABLED; either update
WalletStatus.tsx to include the billing flag in the render conditions (e.g.,
change both conditions to publicConfig.NEXT_PUBLIC_BILLING_ENABLED && isManaged
and publicConfig.NEXT_PUBLIC_BILLING_ENABLED && !isSignedInWithTrial for
ManagedWalletPopup and ConnectManagedWalletButton respectively) or add a clear
code comment above these renders documenting the intentional decision to ignore
the billing flag so behavior is explicit and consistent with
GetStartedStepper.tsx and UserProviders.tsx.
In @apps/deploy-web/src/components/wallet/CustodialWalletPopup.tsx:
- Around line 85-93: CustodialWalletPopup.tsx is inconsistent with
GetStartedStepper.tsx: restore the same NEXT_PUBLIC_BILLING_ENABLED gating
around the ConnectManagedWalletButton so behavior matches; specifically, when
rendering the else branch that currently returns <ConnectManagedWalletButton />,
wrap that rendering in the same environment flag check used in
GetStartedStepper.tsx (e.g., process.env.NEXT_PUBLIC_BILLING_ENABLED === 'true')
so ConnectManagedWalletButton is only shown when billing is enabled, keeping the
existing trial/user sign-in branch unchanged.
🧹 Nitpick comments (1)
apps/deploy-web/src/components/layout/TrackingScripts.tsx (1)
55-55: Consider addingpublicConfigto the dependency array.The
useEffecton line 11 referencespublicConfigvalues but only includesisProductionin its dependency array. IfpublicConfigcan change during runtime, this could lead to stale closures.However, if
publicConfigfrom the DI container is a stable reference that never changes (which is typical for DI patterns), the current implementation is correct.🔎 Proposed fix if publicConfig can change
- }, [isProduction]); + }, [isProduction, publicConfig]);Or if only specific config values matter:
- }, [isProduction]); + }, [isProduction, publicConfig.NEXT_PUBLIC_TRACKING_ENABLED, publicConfig.NEXT_PUBLIC_GROWTH_CHANNEL_TRACKING_ENABLED, publicConfig.NEXT_PUBLIC_GTM_ID]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/deploy-web/src/components/deployments/DeploymentDetailTopBar.tsxapps/deploy-web/src/components/get-started/GetStartedStepper.tsxapps/deploy-web/src/components/layout/CustomGoogleAnalytics.tsxapps/deploy-web/src/components/layout/TrackingScripts.tsxapps/deploy-web/src/components/layout/WalletStatus.tsxapps/deploy-web/src/components/remote-deploy/update/RemoteDeployUpdate.tsxapps/deploy-web/src/components/shared/TrialDeploymentBadge.tsxapps/deploy-web/src/components/wallet/CustodialWalletPopup.tsxapps/deploy-web/src/config/browser-env.config.tsapps/deploy-web/src/context/FlagProvider/FlagProvider.tsxapps/deploy-web/src/hooks/useAutoTopUpLimits.tsxapps/deploy-web/src/hooks/useFlag.tsxapps/deploy-web/src/hooks/useHasCreditCardBanner.tsapps/deploy-web/src/hooks/useManagedWallet.tsapps/deploy-web/src/hooks/useManagedWalletDenom.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/src/config/browser-env.config.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/hooks/useAutoTopUpLimits.tsxapps/deploy-web/src/components/remote-deploy/update/RemoteDeployUpdate.tsxapps/deploy-web/src/components/layout/WalletStatus.tsxapps/deploy-web/src/components/wallet/CustodialWalletPopup.tsxapps/deploy-web/src/components/layout/TrackingScripts.tsxapps/deploy-web/src/hooks/useManagedWallet.tsapps/deploy-web/src/components/shared/TrialDeploymentBadge.tsxapps/deploy-web/src/hooks/useFlag.tsxapps/deploy-web/src/components/layout/CustomGoogleAnalytics.tsxapps/deploy-web/src/components/get-started/GetStartedStepper.tsxapps/deploy-web/src/hooks/useManagedWalletDenom.tsapps/deploy-web/src/hooks/useHasCreditCardBanner.tsapps/deploy-web/src/context/FlagProvider/FlagProvider.tsxapps/deploy-web/src/components/deployments/DeploymentDetailTopBar.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/deploy-web/src/hooks/useManagedWallet.ts
📚 Learning: 2025-07-28T10:40:13.595Z
Learnt from: stalniy
Repo: akash-network/console PR: 1480
File: apps/deploy-web/src/hooks/useFlag.tsx:0-0
Timestamp: 2025-07-28T10:40:13.595Z
Learning: In the Akash Network Console project, backend-specific feature flags are intentional and acceptable. The frontend FeatureFlag union type should only include flags that are actually used by the frontend, not all flags defined in the backend. Backend can have internal feature flags for backend-only functionality without requiring frontend synchronization.
Applied to files:
apps/deploy-web/src/hooks/useFlag.tsxapps/deploy-web/src/context/FlagProvider/FlagProvider.tsx
📚 Learning: 2025-11-12T16:36:02.543Z
Learnt from: baktun14
Repo: akash-network/console PR: 2203
File: apps/deploy-web/src/components/onboarding/steps/PaymentMethodContainer/PaymentMethodContainer.tsx:161-168
Timestamp: 2025-11-12T16:36:02.543Z
Learning: In apps/deploy-web/src/components/onboarding/steps/PaymentMethodContainer/PaymentMethodContainer.tsx, the organization field captured during payment method setup is internal metadata. Errors from stripe.updateCustomerOrganization should be logged to Sentry but not surfaced to users, and the flow should continue even if the organization update fails, as it's non-critical and not something users can fix.
Applied to files:
apps/deploy-web/src/components/get-started/GetStartedStepper.tsx
🧬 Code graph analysis (12)
apps/deploy-web/src/hooks/useAutoTopUpLimits.tsx (3)
apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)apps/deploy-web/src/queries/useExactFeeAllowanceQuery.ts (1)
useExactFeeAllowanceQuery(6-13)apps/deploy-web/src/queries/useExactDeploymentGrantsQuery.ts (1)
useExactDeploymentGrantsQuery(6-16)
apps/deploy-web/src/components/remote-deploy/update/RemoteDeployUpdate.tsx (1)
apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)
apps/deploy-web/src/components/layout/WalletStatus.tsx (2)
apps/deploy-web/src/components/wallet/ManagedWalletPopup.tsx (1)
ManagedWalletPopup(19-90)apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.tsx (1)
ConnectManagedWalletButton(26-41)
apps/deploy-web/src/components/wallet/CustodialWalletPopup.tsx (2)
apps/deploy-web/src/utils/urlUtils.ts (1)
UrlService(16-96)apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.tsx (1)
ConnectManagedWalletButton(26-41)
apps/deploy-web/src/components/layout/TrackingScripts.tsx (1)
apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)
apps/deploy-web/src/hooks/useManagedWallet.ts (1)
apps/deploy-web/src/queries/useManagedWalletQuery.ts (1)
useManagedWalletQuery(7-20)
apps/deploy-web/src/components/shared/TrialDeploymentBadge.tsx (1)
apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)
apps/deploy-web/src/hooks/useFlag.tsx (1)
apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)
apps/deploy-web/src/components/layout/CustomGoogleAnalytics.tsx (2)
apps/deploy-web/src/services/analytics/analytics.service.ts (1)
GoogleAnalytics(142-142)apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)
apps/deploy-web/src/hooks/useManagedWalletDenom.ts (1)
apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)
apps/deploy-web/src/context/FlagProvider/FlagProvider.tsx (1)
apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)
apps/deploy-web/src/components/deployments/DeploymentDetailTopBar.tsx (1)
apps/deploy-web/src/context/ServicesProvider/ServicesProvider.tsx (1)
useServices(33-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (14)
apps/deploy-web/src/context/FlagProvider/FlagProvider.tsx (2)
8-8: LGTM! Clean DI integration.The import of
useServicescorrectly supports the refactoring to injectpublicConfigfrom the DI container instead of directly importingbrowserEnvConfig.
19-21: Successful DI migration for feature flag configuration.The refactoring correctly replaces direct
browserEnvConfigimport with context-basedpublicConfigfromuseServices(). This improves testability by allowing the configuration to be mocked throughServicesProvider, aligning with the PR's stated objective.apps/deploy-web/src/components/deployments/DeploymentDetailTopBar.tsx (1)
50-50: LGTM! Clean refactor to DI-based configuration.The migration from
browserEnvConfigtopublicConfigviauseServices()is consistent with the PR objective. The managed wallet denomination is now sourced from the DI container, improving testability without changing behavior.Also applies to: 234-234
apps/deploy-web/src/components/layout/CustomGoogleAnalytics.tsx (1)
5-5: LGTM! Configuration now sourced from DI container.The Google Analytics flag is now accessed through the
publicConfigfromuseServices(), maintaining the same conditional rendering logic while improving testability.Also applies to: 8-8, 17-17
apps/deploy-web/src/components/shared/TrialDeploymentBadge.tsx (1)
7-7: LGTM! Trial duration configuration migrated to DI.The trial duration fallback now reads from
publicConfigviauseServices(), consistent with the broader refactoring. The logic remains unchanged.Also applies to: 27-28
apps/deploy-web/src/hooks/useManagedWalletDenom.ts (1)
1-1: LGTM! Hook now uses context-based configuration.The managed wallet denomination is now sourced from
publicConfigviauseServices(). The conditional logic remains unchanged, and the hook maintains proper type safety.Also applies to: 6-6, 10-10
apps/deploy-web/src/hooks/useManagedWallet.ts (1)
17-17: The billing feature flag gating has been removed from the managed wallet query.The invocation at line 17 is now unconditional, depending only on
user?.idexistence. TheuseManagedWalletQueryimplementation properly handles the null case via itsenabled: !!userIdflag. TheNEXT_PUBLIC_BILLING_ENABLEDflag still exists in the codebase but is now only used inUserProviders.tsxandGetStartedStepper.tsxfor conditional UI rendering—it has been intentionally removed from the managed wallet hook.apps/deploy-web/src/components/remote-deploy/update/RemoteDeployUpdate.tsx (1)
11-11: LGTM! Clean refactoring to use DI container.The migration from
browserEnvConfigtopublicConfigviauseServices()is consistent and straightforward, improving testability as stated in the PR objectives.Also applies to: 30-30, 64-64
apps/deploy-web/src/hooks/useAutoTopUpLimits.tsx (1)
6-6: LGTM! Consistent migration to DI container.The wallet address configuration is now properly sourced from
publicConfig, maintaining all existing behavior while improving testability.Also applies to: 12-12, 14-17
apps/deploy-web/src/hooks/useHasCreditCardBanner.ts (2)
14-17: Inconsistent AI summary.The AI summary states "removed requirements for hasManagedWallet, isWalletLoading, and isSignedInWithTrial," but the code on line 15 still checks all these conditions. The actual change appears to be the removal of the
NEXT_PUBLIC_BILLING_ENABLEDflag check only.
14-17: LGTM! Billing flag removal simplifies banner logic.The removal of the billing flag check from
shouldShowBanneris consistent with the PR's objective to removeNEXT_PUBLIC_BILLING_ENABLED. The banner visibility now depends solely on initialization state, wallet status, and trial sign-in state.apps/deploy-web/src/components/layout/TrackingScripts.tsx (1)
4-4: LGTM! Comprehensive migration to DI container.All tracking configuration (GTM ID, feature flags, environment) is now properly sourced from
publicConfigvia theuseServices()hook, maintaining consistent behavior while improving testability.Also applies to: 8-13, 25-25, 32-32
apps/deploy-web/src/hooks/useFlag.tsx (1)
8-12: LGTM! Clean refactor to use DI container.The refactor successfully moves from static import to runtime configuration via
useServices(). The wrapper function pattern ensures a consistent API by always accepting aflagparameter and delegating to the selected hook implementation based onpublicConfig.NEXT_PUBLIC_UNLEASH_ENABLE_ALL.Note that this hook now requires React component context (since
useServices()usesuseContext), but this is appropriate for a React hook.apps/deploy-web/src/components/get-started/GetStartedStepper.tsx (1)
16-16: LGTM! Consistent migration to DI-based configuration.The refactor successfully replaces direct
browserEnvConfigusage withpublicConfigfrom the DI container viauseServices(). The logic remains unchanged—only the configuration source has been updated, which aligns with the PR's objective of enabling better testing through dependency injection.All three usages of
publicConfig.NEXT_PUBLIC_BILLING_ENABLED(lines 80, 84, 147) correctly gate billing-related UI elements.Also applies to: 32-32, 80-80, 84-84, 147-147
| <div> | ||
| {!isManaged && <CustodialWalletPopup walletBalance={walletBalance} />} | ||
| {withBilling && isManaged && <ManagedWalletPopup walletBalance={walletBalance} />} | ||
| {isManaged && <ManagedWalletPopup walletBalance={walletBalance} />} |
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for NEXT_PUBLIC_BILLING_ENABLED usage to understand the pattern
rg -n "NEXT_PUBLIC_BILLING_ENABLED" --type=ts --type=tsx -C2Repository: akash-network/console
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Examine WalletStatus.tsx around lines 84 and 93
wc -l apps/deploy-web/src/components/layout/WalletStatus.tsxRepository: akash-network/console
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Look for billing-related conditions and imports in WalletStatus.tsx
rg -n "withBilling|NEXT_PUBLIC_BILLING|isManaged|isSignedInWithTrial" apps/deploy-web/src/components/layout/WalletStatus.tsx -C1Repository: akash-network/console
Length of output: 1523
🏁 Script executed:
#!/bin/bash
# Check GetStartedStepper.tsx for billing flag usage
rg -n "NEXT_PUBLIC_BILLING_ENABLED" apps/deploy-web/src/components/GetStartedStepper.tsx -C2Repository: akash-network/console
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Search for NEXT_PUBLIC_BILLING_ENABLED with correct file type syntax
rg -n "NEXT_PUBLIC_BILLING_ENABLED" --type ts -C2Repository: akash-network/console
Length of output: 3626
🏁 Script executed:
#!/bin/bash
# Find GetStartedStepper file
fd "GetStartedStepper" --type fRepository: akash-network/console
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Read full WalletStatus.tsx to understand context
cat -n apps/deploy-web/src/components/layout/WalletStatus.tsxRepository: akash-network/console
Length of output: 5341
🏁 Script executed:
#!/bin/bash
# Search for billing-related patterns more broadly
rg -n "withBilling|billing" apps/deploy-web/src/components/layout/WalletStatus.tsx --type ts -iRepository: akash-network/console
Length of output: 47
Billing flag check inconsistency requires clarification.
Line 84 renders ManagedWalletPopup and line 93 renders ConnectManagedWalletButton based solely on isManaged and !isSignedInWithTrial without checking publicConfig.NEXT_PUBLIC_BILLING_ENABLED.
However, GetStartedStepper.tsx (line 147) conditionally renders the same ConnectManagedWalletButton component with the billing flag: publicConfig.NEXT_PUBLIC_BILLING_ENABLED && !isSignedInWithTrial. Additionally, UserProviders.tsx (line 15) gates the entire user provider setup on NEXT_PUBLIC_BILLING_ENABLED.
Confirm whether this inconsistency is intentional. If the billing feature can be disabled via configuration, both components should respect the same flag, or the design decision to always show these components in WalletStatus.tsx should be documented.
🤖 Prompt for AI Agents
In @apps/deploy-web/src/components/layout/WalletStatus.tsx around line 84,
WalletStatus.tsx renders ManagedWalletPopup and ConnectManagedWalletButton based
only on isManaged and !isSignedInWithTrial, which is inconsistent with
GetStartedStepper.tsx and UserProviders.tsx that gate behavior on
publicConfig.NEXT_PUBLIC_BILLING_ENABLED; either update WalletStatus.tsx to
include the billing flag in the render conditions (e.g., change both conditions
to publicConfig.NEXT_PUBLIC_BILLING_ENABLED && isManaged and
publicConfig.NEXT_PUBLIC_BILLING_ENABLED && !isSignedInWithTrial for
ManagedWalletPopup and ConnectManagedWalletButton respectively) or add a clear
code comment above these renders documenting the intentional decision to ignore
the billing flag so behavior is explicit and consistent with
GetStartedStepper.tsx and UserProviders.tsx.
| <Separator className="my-4" /> | ||
|
|
||
| {isSignedInWithTrial && !user ? ( | ||
| <Link className={cn(buttonVariants({ variant: "outline" }), "w-full space-x-2")} href={UrlService.newLogin()}> | ||
| Sign in for USD Payments | ||
| </Link> | ||
| ) : ( | ||
| <ConnectManagedWalletButton className="w-full" /> | ||
| )} | ||
| </> | ||
| {isSignedInWithTrial && !user ? ( | ||
| <Link className={cn(buttonVariants({ variant: "outline" }), "w-full space-x-2")} href={UrlService.newLogin()}> | ||
| Sign in for USD Payments | ||
| </Link> | ||
| ) : ( | ||
| <ConnectManagedWalletButton className="w-full" /> | ||
| )} |
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for other usages of NEXT_PUBLIC_BILLING_ENABLED or withBilling
rg -n "NEXT_PUBLIC_BILLING_ENABLED|withBilling" --type ts --type tsx -C 2Repository: akash-network/console
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Examine the CustodialWalletPopup.tsx file to understand context
find . -name "CustodialWalletPopup.tsx" -type fRepository: akash-network/console
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Look at the file content around lines 85-93 and surrounding context
find . -name "CustodialWalletPopup.tsx" -type f -exec head -150 {} \; -exec echo "---END OF FILE---" \;Repository: akash-network/console
Length of output: 3882
🏁 Script executed:
#!/bin/bash
# Search for billing flag usages without the problematic tsx type argument
rg -n "NEXT_PUBLIC_BILLING_ENABLED|withBilling" -C 2Repository: akash-network/console
Length of output: 3626
🏁 Script executed:
#!/bin/bash
# Also check for any references to this component or similar patterns
rg -n "isSignedInWithTrial" --type ts -C 2 | head -50Repository: akash-network/console
Length of output: 4544
Review the inconsistency: CustodialWalletPopup no longer gates ConnectManagedWalletButton on NEXT_PUBLIC_BILLING_ENABLED, but GetStartedStepper.tsx still does.
The conditional logic in CustodialWalletPopup.tsx (lines 85-93) correctly handles wallet states: trial users without accounts see "Sign in for USD Payments", and all others see ConnectManagedWalletButton. However, NEXT_PUBLIC_BILLING_ENABLED is still actively used in the codebase (UserProviders.tsx, GetStartedStepper.tsx) and has not been globally removed. GetStartedStepper.tsx line 147 still gates the same ConnectManagedWalletButton component on this flag, creating inconsistent behavior across the application. Either this component needs to restore the flag check to align with other usages, or the flag usage should be systematically removed across all affected components.
🤖 Prompt for AI Agents
In @apps/deploy-web/src/components/wallet/CustodialWalletPopup.tsx around lines
85-93, CustodialWalletPopup.tsx is inconsistent with GetStartedStepper.tsx:
restore the same NEXT_PUBLIC_BILLING_ENABLED gating around the
ConnectManagedWalletButton so behavior matches; specifically, when rendering the
else branch that currently returns <ConnectManagedWalletButton />, wrap that
rendering in the same environment flag check used in GetStartedStepper.tsx
(e.g., process.env.NEXT_PUBLIC_BILLING_ENABLED === 'true') so
ConnectManagedWalletButton is only shown when billing is enabled, keeping the
existing trial/user sign-in branch unchanged.
Why
better testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.