refactor: fetches turnstile sitekey from api#1318
Conversation
WalkthroughThis update introduces a dynamic configuration system for the Turnstile site key, enabling secure test token injection for UI tests. It adds a new API endpoint for configuration, updates the dependency injection and service instantiation patterns, and enhances Playwright test fixtures to support authenticated test requests. Workflow and environment variable configurations are also adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TurnstileComponent
participant useAppConfig
participant ConfigService
participant API_Config_Endpoint
Client->>TurnstileComponent: Render
TurnstileComponent->>useAppConfig: Fetch config (enabled)
useAppConfig->>ConfigService: getConfig()
ConfigService->>API_Config_Endpoint: GET /api/config (with optional UI test token)
API_Config_Endpoint-->>ConfigService: { TURNSTILE_SITE_KEY }
ConfigService-->>useAppConfig: Config data
useAppConfig-->>TurnstileComponent: Config data
TurnstileComponent-->>Client: Render Turnstile with site key
sequenceDiagram
participant PlaywrightTest
participant base-test.ts
participant API_Mainnet/Sandbox
PlaywrightTest->>base-test.ts: beforeEach
base-test.ts->>API_Mainnet/Sandbox: Intercept request, add x-ui-tests-token header
API_Mainnet/Sandbox-->>PlaywrightTest: Response (with test context)
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
apps/deploy-web/src/services/config/config.service.ts (1)
1-11: Well-designed config service for dynamic Turnstile site key fetchingThe new
ConfigServicefollows good software design principles:
- It extends the base
HttpServicefor HTTP functionality- Defines a clear interface for the remote configuration
- Implements a simple method for fetching configuration
This approach enables dynamic configuration of the Turnstile site key based on whether the request comes from a test environment.
However, I recommend enhancing the error handling:
export class ConfigService extends HttpService { async getConfig(): Promise<RemoteConfig> { - return this.extractData(await this.get<RemoteConfig>("/api/config")); + try { + return this.extractData(await this.get<RemoteConfig>("/api/config")); + } catch (error) { + console.error("Failed to fetch remote config:", error); + // Fallback to default configuration + return { TURNSTILE_SITE_KEY: process.env.NEXT_PUBLIC_TURNSTILE_SITE_KEY || "" }; + } } }apps/deploy-web/tests/fixture/base-test.ts (1)
15-17: Consider more specific route patterns for token injectionThe current route patterns will add the token to a broad set of requests. While the mainnet and sandbox API routes make sense, the localhost pattern will intercept all requests to the local server.
If not all routes need the token, consider using more specific patterns:
await page.route("**/api-mainnet/**", addUITestsToken); await page.route("**/api-sandbox/**", addUITestsToken); - await page.route("http://localhost:3000/**", addUITestsToken); + await page.route("http://localhost:3000/api/**", addUITestsToken);Or be explicit about which specific endpoints need the token if you know them.
apps/deploy-web/src/pages/api/config.ts (1)
8-10: Improve warning message clarityThe warning message about missing UI_TESTS_TOKEN is good defensive programming, but could be more actionable.
Consider improving the warning message to be more actionable:
- logger.warn(`UI_TESTS_TOKEN is not set, but turnstile protection is enabled.` + `It means UI tests will not be able to pass the turnstile protection.`); + logger.warn(`UI_TESTS_TOKEN is not set, but turnstile protection is enabled. UI tests will fail unless you set UI_TESTS_TOKEN in your environment. See documentation for setup instructions.`);apps/deploy-web/src/queries/useAppConfig.ts (1)
8-14: Consider retry strategy for config fetchingThe current implementation doesn't specify a retry strategy for fetching configuration. Since this is critical app configuration, you might want to ensure it reliably loads.
Add a retry strategy to the query configuration:
const { data } = useQuery({ queryKey: ["app-config"], queryFn: () => di.config.getConfig(), gcTime: Infinity, + retry: 3, + retryDelay: (attemptIndex) => Math.min(1000 * 2 ** attemptIndex, 30000), ...options });.github/actions/console-web-ui-testing/action.yml (1)
18-20: Document behavior with empty token valueThe ui-tests-token input has an empty default value, but it's not clear how the system behaves with this value.
Improve the documentation to clarify the behavior with an empty token:
ui-tests-token: - description: UI tests token used to bypass turnstile check - default: "" + description: UI tests token used to bypass turnstile check. If empty, tests will use the production Turnstile site key. + default: ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/actions/console-web-ui-testing/action.yml(3 hunks).github/workflows/console-web-release.yml(1 hunks)apps/deploy-web/src/components/turnstile/Turnstile.tsx(4 hunks)apps/deploy-web/src/config/env-config.schema.ts(1 hunks)apps/deploy-web/src/pages/_app.tsx(1 hunks)apps/deploy-web/src/pages/api/config.ts(1 hunks)apps/deploy-web/src/queries/useAppConfig.ts(1 hunks)apps/deploy-web/src/queries/useBalancesQuery.ts(2 hunks)apps/deploy-web/src/services/config/config.service.ts(1 hunks)apps/deploy-web/src/services/container/createContainer.ts(1 hunks)apps/deploy-web/src/services/http-factory/http-factory.service.ts(1 hunks)apps/deploy-web/tests/build-template.spec.ts(1 hunks)apps/deploy-web/tests/deploy-custom-template.spec.ts(1 hunks)apps/deploy-web/tests/deploy-linux.spec.ts(1 hunks)apps/deploy-web/tests/fixture/base-test.ts(1 hunks)apps/deploy-web/tests/fixture/context-with-extension.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/deploy-web/src/pages/_app.tsx (1)
apps/deploy-web/src/components/turnstile/Turnstile.tsx (1)
ClientOnlyTurnstile(181-181)
apps/deploy-web/src/services/config/config.service.ts (1)
packages/http-sdk/src/http/http.service.ts (1)
HttpService(3-15)
apps/deploy-web/src/queries/useAppConfig.ts (2)
apps/deploy-web/src/services/config/config.service.ts (1)
RemoteConfig(9-11)apps/deploy-web/src/context/ServicesProvider/index.tsx (1)
useServices(26-28)
apps/deploy-web/src/queries/useBalancesQuery.ts (2)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
getBalances(48-70)apps/deploy-web/src/types/address.ts (1)
Balances(41-50)
apps/deploy-web/src/components/turnstile/Turnstile.tsx (2)
apps/deploy-web/src/config/browser-env.config.ts (1)
browserEnvConfig(3-40)apps/deploy-web/src/queries/useAppConfig.ts (1)
useAppConfig(6-16)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (21)
apps/deploy-web/tests/build-template.spec.ts (1)
1-1: Properly imported test utilities from the local fixture.The import source for
expectandtesthas been changed from@playwright/testto the local module./fixture/base-test. This change is appropriate as it leverages a custom test fixture that automatically adds thex-ui-tests-tokenheader to network requests, allowing Turnstile to be bypassed during e2e tests.apps/deploy-web/tests/deploy-custom-template.spec.ts (1)
2-2: Properly imported test utilities from the local fixture.The import source for
expectandtesthas been changed from@playwright/testto the local module./fixture/base-test. This change is appropriate as it leverages a custom test fixture that automatically adds thex-ui-tests-tokenheader to network requests, allowing Turnstile to be bypassed during e2e tests.apps/deploy-web/tests/deploy-linux.spec.ts (1)
1-1: Properly imported test utilities from the local fixture.The import source for
expectandtesthas been changed from@playwright/testto the local module./fixture/base-test. This change is appropriate as it leverages a custom test fixture that automatically adds thex-ui-tests-tokenheader to network requests, allowing Turnstile to be bypassed during e2e tests.apps/deploy-web/src/pages/_app.tsx (1)
64-64:Details
✅ Verification successful
ClientOnlyTurnstile now properly positioned within QueryClientProvider.
The
ClientOnlyTurnstilecomponent has been repositioned inside theQueryClientProviderwhich is appropriate since it now needs access to React Query's context to dynamically fetch the Turnstile site key via theuseAppConfighook. This change supports the dynamic configuration approach described in the PR objectives.To ensure the component is properly retrieving the Turnstile site key, we can verify if the
useAppConfighook is being used correctly:
🏁 Script executed:
#!/bin/bash # Check if Turnstile component is using the useAppConfig hook rg -A 3 "const ClientOnlyTurnstile|function Turnstile" apps/deploy-web/src/components/turnstile/Turnstile.tsxLength of output: 205
🏁 Script executed:
#!/bin/bash # Locate the Turnstile component definition rg -n "function Turnstile" -A 5 apps/deploy-web/src/components/turnstile/Turnstile.tsx # Check for useAppConfig hook usage within the Turnstile component rg -n "useAppConfig" -A 5 apps/deploy-web/src/components/turnstile/Turnstile.tsxLength of output: 719
🏁 Script executed:
#!/bin/bash # Verify that ClientOnlyTurnstile is nested within QueryClientProvider in _app.tsx rg -n "QueryClientProvider" -A 10 -B 10 apps/deploy-web/src/pages/_app.tsxLength of output: 2821
ClientOnlyTurnstile correctly accesses React Query context
The
ClientOnlyTurnstilecomponent is now properly wrapped by<QueryClientProvider>inapps/deploy-web/src/pages/_app.tsx(lines 63–64), and the underlyingTurnstileimplementation imports and callsuseAppConfig(line 52 inapps/deploy-web/src/components/turnstile/Turnstile.tsx) to fetch the site key dynamically. No further changes are needed here..github/workflows/console-web-release.yml (1)
53-53: Properly integrated UI tests token for Turnstile bypassThe addition of the
ui-tests-tokenparameter to the UI testing action ensures that e2e tests can bypass the Turnstile verification as intended. This is a clean implementation that follows security best practices by using GitHub secrets.apps/deploy-web/tests/fixture/context-with-extension.ts (1)
2-3: Good refactoring of test imports for token injectionThe import changes correctly separate the chromium import and switch to using the local base-test module. This change enables the global token injection for API requests during e2e tests, making Turnstile bypass possible while keeping the original test functionality intact.
Also applies to: 8-9
apps/deploy-web/src/config/env-config.schema.ts (1)
63-69: Well-implemented environment variables for Turnstile testingThe addition of
TURNSTILE_TEST_SITE_KEYwith the Cloudflare dummy key default and the optionalUI_TESTS_TOKENvariable provides a clean way to handle the Turnstile bypass mechanism. The code comments documenting the source of the dummy key are particularly helpful.apps/deploy-web/src/queries/useBalancesQuery.ts (2)
17-18: Type consistency improvement.Changed return type from
undefinedtonullfor the absence of data. This creates more consistent typing throughout the application and makes the behavior more explicit.
64-64: Type signature updated to match implementation.The generic type parameter for
UseQueryOptionshas been updated to reflect the change fromundefinedtonullin the underlying function, maintaining type consistency throughout the component.apps/deploy-web/src/components/turnstile/Turnstile.tsx (5)
17-17: Added import for dynamic configuration.The new import enables the component to fetch the Turnstile site key from the app configuration service instead of using a hardcoded value.
47-47: Removed hardcoded site key in favor of dynamic configuration.This change aligns with the PR's objective to dynamically fetch the Turnstile site key, making it possible to modify it for e2e tests without changing code.
52-52: Fetch configuration dynamically based on enabled state.The
useAppConfighook is passed theenabledparameter to control when configuration is loaded, which is an efficient approach that prevents unnecessary API calls when Turnstile is disabled.
113-113: Check for both enabled state and site key availability.This conditional correctly handles cases where the component should not render - either when explicitly disabled or when the site key is not available in the configuration.
135-135: Use dynamically loaded site key for Turnstile widget.The component now uses the site key from the app configuration rather than a hardcoded value, supporting the PR's objective of making the site key configurable for testing.
apps/deploy-web/src/services/container/createContainer.ts (2)
1-28: Well-implemented dependency injection container.This utility creates a container with lazily evaluated properties, ensuring efficient resource usage and proper dependency management. The circular dependency detection is a good safeguard against potential runtime issues.
The implementation correctly:
- Creates getters for lazy initialization
- Tracks resolution path to detect cycles
- Caches instances to ensure singletons
- Provides helpful error messages for circular dependencies
This pattern supports the PR's objective by enabling modular and testable service configuration.
30-32: Type-safe container type definition.This type alias ensures that the container's properties match the return types of the factory functions, providing compile-time type safety for accessing resolved instances.
apps/deploy-web/src/services/http-factory/http-factory.service.ts (5)
9-10: Added imports for new configuration service and container utility.These imports support the PR's objective by introducing the configuration service needed for dynamic Turnstile site key fetching and the container utility for modular service management.
15-64: Refactored service creation to use dependency injection container.The implementation has been improved to use a modular container-based approach that:
- Lazy-loads services only when needed
- Applies interceptors consistently
- Simplifies service creation and management
- Adds a new configuration service for dynamic app settings
This supports the PR's objective of fetching the Turnstile site key dynamically from an API endpoint.
60-63: Added configuration service with appropriate interceptors.The new config service will handle fetching remote configuration including the Turnstile site key. It's properly configured with the global request middleware for consistent request handling.
73-77: Helper function for applying interceptors to axios instances.This utility function cleanly encapsulates the logic for adding request and response interceptors to axios instances, improving code readability and maintainability.
79-83: Type definitions for interceptors.These types provide proper typing for the interceptor configuration, ensuring type safety when configuring services with interceptors.
stalniy
force-pushed
the
refactor/turnstile-sitekey
branch
from
cabcabd to
0856fe1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/deploy-web/src/services/http-factory/http-factory.service.ts (2)
79-83: Consider filtering undefined interceptors.The interceptors array may contain undefined values (as indicated by the type), but there's no filtering of these values before applying them.
function withInterceptors<T extends Axios>(axios: T, interceptors: Interceptors) { - interceptors.request?.forEach(interceptor => axios.interceptors.request.use(interceptor)); - interceptors.response?.forEach(interceptor => axios.interceptors.response.use(interceptor)); + interceptors.request?.filter(Boolean).forEach(interceptor => axios.interceptors.request.use(interceptor)); + interceptors.response?.filter(Boolean).forEach(interceptor => axios.interceptors.response.use(interceptor)); return axios; }
15-64: Consider adding error handling for service initialization.The container pattern is excellent, but there's no explicit error handling for service initialization failures. Consider adding try/catch blocks or a global error handler for services that might fail during initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/actions/console-web-ui-testing/action.yml(3 hunks).github/workflows/console-web-release.yml(1 hunks)apps/deploy-web/src/components/turnstile/Turnstile.tsx(4 hunks)apps/deploy-web/src/config/env-config.schema.ts(1 hunks)apps/deploy-web/src/pages/_app.tsx(1 hunks)apps/deploy-web/src/pages/api/config.ts(1 hunks)apps/deploy-web/src/queries/useAppConfig.ts(1 hunks)apps/deploy-web/src/queries/useBalancesQuery.ts(2 hunks)apps/deploy-web/src/services/config/config.service.ts(1 hunks)apps/deploy-web/src/services/container/createContainer.ts(1 hunks)apps/deploy-web/src/services/http-factory/http-factory.service.ts(1 hunks)apps/deploy-web/tests/build-template.spec.ts(1 hunks)apps/deploy-web/tests/deploy-custom-template.spec.ts(1 hunks)apps/deploy-web/tests/deploy-linux.spec.ts(1 hunks)apps/deploy-web/tests/fixture/base-test.ts(1 hunks)apps/deploy-web/tests/fixture/context-with-extension.ts(1 hunks)apps/deploy-web/tests/fixture/test-env.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/deploy-web/tests/deploy-custom-template.spec.ts
- apps/deploy-web/tests/fixture/test-env.config.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/deploy-web/tests/build-template.spec.ts
- apps/deploy-web/src/pages/_app.tsx
- apps/deploy-web/tests/fixture/context-with-extension.ts
- apps/deploy-web/src/services/config/config.service.ts
- apps/deploy-web/tests/deploy-linux.spec.ts
- apps/deploy-web/src/queries/useAppConfig.ts
- .github/actions/console-web-ui-testing/action.yml
- apps/deploy-web/src/config/env-config.schema.ts
- .github/workflows/console-web-release.yml
- apps/deploy-web/src/services/container/createContainer.ts
- apps/deploy-web/src/queries/useBalancesQuery.ts
- apps/deploy-web/src/components/turnstile/Turnstile.tsx
- apps/deploy-web/tests/fixture/base-test.ts
- apps/deploy-web/src/pages/api/config.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (5)
apps/deploy-web/src/services/http-factory/http-factory.service.ts (5)
3-3: Good use of explicit type imports.The change from importing all of axios to importing specific types improves code clarity and potentially reduces bundle size.
9-10: Good additions for dependency management.Importing the ConfigService and createContainer supports the goal of fetching the Turnstile sitekey dynamically and implementing a DI container pattern.
15-64: Excellent refactoring to use container pattern.The refactoring to use a container pattern for service instantiation greatly improves code organization and maintainability by:
- Centralizing service creation
- Enabling dependency injection
- Allowing for service instance caching
- Making service configuration more consistent
This approach aligns well with the PR objective of making the Turnstile sitekey configurable.
60-63: Good addition of ConfigService.Adding the ConfigService as a dependency enables the fetching of dynamic configuration including the Turnstile sitekey, which directly supports the PR objectives.
73-77: Well-designed helper function for interceptor management.The
withInterceptorsfunction elegantly modularizes the process of attaching request and response interceptors to Axios instances, reducing code duplication and improving readability.
stalniy
force-pushed
the
refactor/turnstile-sitekey
branch
from
0856fe1 to
e1de86d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
stalniy
force-pushed
the
refactor/turnstile-sitekey
branch
2 times, most recently
from
624eb09 to
c765087
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1318 +/- ##
==========================================
+ Coverage 33.50% 33.56% +0.05%
==========================================
Files 770 774 +4
Lines 19009 19052 +43
Branches 3537 3551 +14
==========================================
+ Hits 6369 6394 +25
- Misses 11891 12308 +417
+ Partials 749 350 -399
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
refactor/turnstile-sitekey
branch
5 times, most recently
from
9d000f7 to
8b631cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/deploy-web/src/services/http-factory/http-factory.service.ts (1)
79-83: Well-defined types for interceptorsThese type definitions provide good type safety for the interceptor pattern. The optional arrays allow flexibility in applying different interceptors to different services.
However, consider making these types more reusable by moving them to a separate types file if they might be used in other modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (19)
.github/actions/console-web-ui-testing/action.yml(3 hunks).github/workflows/console-web-docker-build.yml(0 hunks).github/workflows/console-web-release.yml(1 hunks)apps/deploy-web/env/.env.sample(1 hunks)apps/deploy-web/src/components/turnstile/Turnstile.tsx(4 hunks)apps/deploy-web/src/config/env-config.schema.ts(1 hunks)apps/deploy-web/src/pages/_app.tsx(1 hunks)apps/deploy-web/src/pages/api/config.ts(1 hunks)apps/deploy-web/src/queries/useAppConfig.ts(1 hunks)apps/deploy-web/src/queries/useBalancesQuery.ts(2 hunks)apps/deploy-web/src/services/config/config.service.ts(1 hunks)apps/deploy-web/src/services/container/createContainer.ts(1 hunks)apps/deploy-web/src/services/http-factory/http-factory.service.ts(1 hunks)apps/deploy-web/tests/build-template.spec.ts(1 hunks)apps/deploy-web/tests/deploy-custom-template.spec.ts(1 hunks)apps/deploy-web/tests/deploy-linux.spec.ts(1 hunks)apps/deploy-web/tests/fixture/base-test.ts(1 hunks)apps/deploy-web/tests/fixture/context-with-extension.ts(1 hunks)apps/deploy-web/tests/fixture/test-env.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/console-web-docker-build.yml
🚧 Files skipped from review as they are similar to previous changes (17)
- apps/deploy-web/env/.env.sample
- apps/deploy-web/tests/build-template.spec.ts
- apps/deploy-web/tests/deploy-linux.spec.ts
- apps/deploy-web/src/pages/_app.tsx
- apps/deploy-web/tests/deploy-custom-template.spec.ts
- apps/deploy-web/tests/fixture/test-env.config.ts
- apps/deploy-web/src/pages/api/config.ts
- apps/deploy-web/tests/fixture/context-with-extension.ts
- apps/deploy-web/src/services/config/config.service.ts
- .github/workflows/console-web-release.yml
- apps/deploy-web/src/config/env-config.schema.ts
- .github/actions/console-web-ui-testing/action.yml
- apps/deploy-web/tests/fixture/base-test.ts
- apps/deploy-web/src/services/container/createContainer.ts
- apps/deploy-web/src/components/turnstile/Turnstile.tsx
- apps/deploy-web/src/queries/useBalancesQuery.ts
- apps/deploy-web/src/queries/useAppConfig.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/deploy-web/src/services/http-factory/http-factory.service.ts (11)
apps/deploy-web/src/services/container/createContainer.ts (1)
createContainer(1-28)apps/deploy-web/src/services/auth/auth.service.ts (1)
authService(23-23)apps/deploy-web/src/services/analytics/analytics.service.ts (1)
analyticsService(254-271)packages/http-sdk/src/stripe/stripe.service.ts (1)
StripeService(12-20)packages/http-sdk/src/tx-http/tx-http.service.ts (1)
TxHttpService(15-34)apps/provider-console/src/utils/customRegistry.ts (1)
customRegistry(2-2)packages/http-sdk/src/template/template-http.service.ts (1)
TemplateHttpService(34-46)packages/http-sdk/src/auth/auth-http.service.ts (1)
AuthHttpService(5-13)apps/deploy-web/src/services/provider-proxy/provider-proxy.service.ts (1)
ProviderProxyService(4-23)packages/http-sdk/src/api-key/api-key-http.service.ts (1)
ApiKeyHttpService(4-16)apps/deploy-web/src/services/config/config.service.ts (1)
ConfigService(3-7)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (5)
apps/deploy-web/src/services/http-factory/http-factory.service.ts (5)
3-3: Type imports added to support new interceptor implementationThe explicit import of Axios types is necessary for the new interceptor pattern introduced in this refactoring.
9-10: Added imports for the new dependency injection patternThese imports support the refactoring to a lazy dependency injection container pattern, which is a good improvement that delays service instantiation until needed.
15-64: Well-structured refactoring to a dependency injection containerThe refactoring from eagerly instantiated services to a lazy DI container is well-implemented. Each service is now created through a factory function and properly configured with interceptors. This approach:
- Improves performance by only instantiating services when needed
- Prevents circular dependencies (as handled in createContainer)
- Makes the code more maintainable with consistent initialization patterns
The addition of the config service (lines 60-63) aligns with the PR objective of fetching Turnstile sitekey from an API.
73-77: Clean implementation of interceptor applicationThe
withInterceptorshelper function provides a consistent way to apply request and response interceptors to Axios instances. This improves code maintainability by centralizing interceptor application logic.
56-59:Details
✅ Verification successful
Verify axios instance usage
You're creating a generic axios instance without specifying a baseURL. Make sure this is intentional and that consumers of this instance are aware they need to provide full URLs.
🏁 Script executed:
#!/bin/bash # Look for usages of the axios service to ensure they provide full URLs rg -A 5 "services\.(axios|httpFactory\.axios)" --type tsLength of output: 3032
All axios calls supply full URLs
I’ve checked all uses of
services.axios—each call is passed a complete URL (either viaApiUrlService.*helpers orserverEnvConfig.BASE_API_MAINNET_URL), so the omission of a defaultbaseURLis intentional and safe. No changes are needed here.
stalniy
force-pushed
the
refactor/turnstile-sitekey
branch
from
8b631cc to
61d4f93
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/deploy-web/src/services/http-factory/http-factory.service.ts (1)
15-64: Consider adding error handling to interceptors.While the implementation is solid, there's no explicit error handling for interceptors. If any interceptor throws an exception, it might affect the entire request/response chain.
Consider adding error handling to protect against interceptor failures:
function withInterceptors<T extends Axios>(axios: T, interceptors: Interceptors) { - interceptors.request?.forEach(interceptor => axios.interceptors.request.use(interceptor)); - interceptors.response?.forEach(interceptor => axios.interceptors.response.use(interceptor)); + interceptors.request?.forEach(interceptor => + axios.interceptors.request.use( + interceptor, + error => Promise.reject(error) + ) + ); + interceptors.response?.forEach(interceptor => + axios.interceptors.response.use( + interceptor, + error => Promise.reject(error) + ) + ); return axios; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (19)
.github/actions/console-web-ui-testing/action.yml(3 hunks).github/workflows/console-web-docker-build.yml(0 hunks).github/workflows/console-web-release.yml(1 hunks)apps/deploy-web/env/.env.sample(1 hunks)apps/deploy-web/src/components/turnstile/Turnstile.tsx(4 hunks)apps/deploy-web/src/config/env-config.schema.ts(1 hunks)apps/deploy-web/src/pages/_app.tsx(1 hunks)apps/deploy-web/src/pages/api/config.ts(1 hunks)apps/deploy-web/src/queries/useAppConfig.ts(1 hunks)apps/deploy-web/src/queries/useBalancesQuery.ts(2 hunks)apps/deploy-web/src/services/config/config.service.ts(1 hunks)apps/deploy-web/src/services/container/createContainer.ts(1 hunks)apps/deploy-web/src/services/http-factory/http-factory.service.ts(1 hunks)apps/deploy-web/tests/build-template.spec.ts(1 hunks)apps/deploy-web/tests/deploy-custom-template.spec.ts(1 hunks)apps/deploy-web/tests/deploy-linux.spec.ts(1 hunks)apps/deploy-web/tests/fixture/base-test.ts(1 hunks)apps/deploy-web/tests/fixture/context-with-extension.ts(1 hunks)apps/deploy-web/tests/fixture/test-env.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/console-web-docker-build.yml
✅ Files skipped from review due to trivial changes (1)
- apps/deploy-web/tests/build-template.spec.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- apps/deploy-web/env/.env.sample
- apps/deploy-web/src/pages/_app.tsx
- apps/deploy-web/tests/deploy-custom-template.spec.ts
- .github/workflows/console-web-release.yml
- apps/deploy-web/tests/fixture/context-with-extension.ts
- apps/deploy-web/tests/deploy-linux.spec.ts
- apps/deploy-web/src/services/config/config.service.ts
- apps/deploy-web/src/pages/api/config.ts
- apps/deploy-web/src/config/env-config.schema.ts
- apps/deploy-web/tests/fixture/base-test.ts
- apps/deploy-web/src/queries/useAppConfig.ts
- apps/deploy-web/src/queries/useBalancesQuery.ts
- apps/deploy-web/tests/fixture/test-env.config.ts
- apps/deploy-web/src/components/turnstile/Turnstile.tsx
- .github/actions/console-web-ui-testing/action.yml
- apps/deploy-web/src/services/container/createContainer.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (4)
apps/deploy-web/src/services/http-factory/http-factory.service.ts (4)
3-3: Good additions for the new dependency injection pattern.The imports of Axios types and the new
ConfigServiceandcreateContainerutilities properly support the refactoring to a container-based approach with lazy initialization.Also applies to: 9-10
15-64: Well-structured service container implementation.The refactoring to use a container-based approach with factory functions is a good architectural improvement that:
- Enables lazy initialization (services are only created when needed)
- Makes the code more modular and maintainable
- Provides a consistent pattern for adding interceptors to each service
- Properly preserves the existing behavior (auth headers, analytics tracking)
The addition of the
configservice aligns perfectly with the PR objective to fetch the Turnstile sitekey dynamically.
73-77: Clean utility function for interceptor management.The
withInterceptorshelper function is a good abstraction that centralizes the logic for attaching interceptors to Axios instances. The implementation is concise and reusable.
79-83: Well-designed type definitions for interceptors.The type definitions for interceptors provide good type safety and help document the expected shape of interceptor functions. The use of optional arrays in the
Interceptorsinterface allows for flexibility in configuration.
3 participants
Why
Our e2e tests cannot bypass turnstile check
What
Summary by CodeRabbit
New Features
Improvements
Testing
Chores