refactor: adds support for automatic cancellation of unauthorized requests#2491
refactor: adds support for automatic cancellation of unauthorized requests#2491
Conversation
📝 WalkthroughWalkthroughCentralizes a Cockatiel-based fetch adapter into packages/http-sdk, removes the local fetch adapter and axios-retry usage, updates createHttpClient/createAxios signatures and callers to use the shared adapter, adds abort-on-failure behavior, and short-circuits AbortError in the error reporter. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (consumer)
participant DI as DI / app-di-container
participant SDK as `@akashnetwork/http-sdk`
participant Policy as Cockatiel Policy Chain
participant Server as Remote API
App->>DI: request HttpClient / make request
DI->>SDK: call createHttpClient(options { adapter, abortPendingWhenOneFail })
SDK->>Policy: createFetchAdapter(retries, circuitBreaker?, abort predicate)
App->>SDK: invoke Axios HttpClient (adapter triggers)
SDK->>Policy: adapter executes (apply retry/circuit-breaker & abort logic)
Policy->>Server: perform network call (fetch/http adapter)
Server-->>Policy: response or error
Policy-->>SDK: onSuccess/onFailure (may abort pending requests)
SDK-->>App: return AxiosResponse or throw error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-19T15:15:07.283ZApplied to files:
⏰ 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). (12)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2491 +/- ##
==========================================
- Coverage 50.95% 50.88% -0.07%
==========================================
Files 1064 1063 -1
Lines 29457 29417 -40
Branches 6530 6494 -36
==========================================
- Hits 15010 14970 -40
+ Misses 14100 14037 -63
- Partials 347 410 +63
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
refactor/authentication-handling
branch
from
0357b64 to
1880868
Compare
stalniy
force-pushed
the
refactor/authentication-handling
branch
from
1880868 to
ffc28b2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts`:
- Around line 282-320: The test mixing jest.useFakeTimers() with
timers/promises.wait in the createFetchAdapter.spec (the adapter mock uses wait
with config.signal) is fragile because it never advances fake timers; update the
test that aborts pending requests ("aborts all pending requests when one request
fails with matching condition") to call await jest.runAllTimersAsync() at
appropriate points (for example after initiating the two fetch calls and before
awaiting Promise.allSettled) so the fake timers advance and the wait() resolves
or the abort is processed deterministically; alternatively, refactor the test to
avoid timers/promises.wait (e.g., use a controllable Promise or manual
resolve/reject and trigger aborts) while keeping the references to
createFetchAdapter, adapter, abortPendingWhenOneFail, and wait.
🧹 Nitpick comments (3)
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts (2)
313-319: Consider type-safe extraction instead ofas unknown ascasts.The double cast through
unknownworks but is a pattern that can mask type issues. Consider a type guard or assertion function for cleaner extraction.♻️ Suggested improvement
- expect(pendingResult.status).toBe("rejected"); - const abortError = (pendingResult as unknown as PromiseRejectedResult).reason; - expect(abortError.name).toBe("AbortError"); - - expect(failingResult.status).toBe("rejected"); - const error = (failingResult as unknown as PromiseRejectedResult).reason; - expect(error.response?.status).toBe(401); + expect(pendingResult.status).toBe("rejected"); + if (pendingResult.status === "rejected") { + expect(pendingResult.reason.name).toBe("AbortError"); + } + + expect(failingResult.status).toBe("rejected"); + if (failingResult.status === "rejected") { + expect(failingResult.reason.response?.status).toBe(401); + }
391-411: Test verifies signal wrapping but not external abort propagation.This test confirms the signal is replaced with a composed signal, but doesn't verify that aborting
externalAbortControllerwould still cancel the request. Consider adding a test case that aborts the external controller mid-request to ensure both signals are respected.packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts (1)
62-69: Potential issue withundefinedsignal cast.Line 64 casts
config.signal as AbortSignal, butconfig.signalmay beundefined. While cockatiel'sexecute()likely acceptsundefined, the explicit cast could mask type issues.♻️ Suggested improvement
- .execute(() => fetchAdapter(config), config.signal as AbortSignal) + .execute(() => fetchAdapter(config), config.signal ?? undefined)Or simply pass without casting if cockatiel accepts
AbortSignal | undefined:- .execute(() => fetchAdapter(config), config.signal as AbortSignal) + .execute(() => fetchAdapter(config), config.signal)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
apps/deploy-web/src/context/SettingsProvider/SettingsProviderContext.tsxapps/deploy-web/src/services/app-di-container/app-di-container.tsapps/deploy-web/src/services/createFallbackableHttpClient/createFallbackableHttpClient.tsapps/deploy-web/src/services/createFetchAdapter/createFetchAdapter.tsapps/deploy-web/src/services/error-handler/error-handler.service.tspackages/http-sdk/package.jsonpackages/http-sdk/src/index.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.tspackages/http-sdk/src/utils/httpClient.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/src/services/createFetchAdapter/createFetchAdapter.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/deploy-web/src/services/error-handler/error-handler.service.ts
- apps/deploy-web/src/services/app-di-container/app-di-container.ts
- packages/http-sdk/package.json
- apps/deploy-web/src/context/SettingsProvider/SettingsProviderContext.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/services/createFallbackableHttpClient/createFallbackableHttpClient.tspackages/http-sdk/src/index.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.tspackages/http-sdk/src/utils/httpClient.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under testUse
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
**/*.spec.{ts,tsx}: Use<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references
Use either a method name or a condition starting with 'when' for nested suite descriptions in tests
Use present simple, 3rd person singular for test descriptions without prepending 'should'
Files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
setupfunction instead ofbeforeEachin unit and service level tests
Files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Learnt from: ygrishajev
Repo: akash-network/console PR: 2254
File: apps/api/test/functional/sign-and-broadcast-tx.spec.ts:4-4
Timestamp: 2025-11-19T15:15:07.283Z
Learning: In the Akash Network Console project, when tests use native Node.js fetch (available in Node 18+), fetch-mock should be used for HTTP mocking instead of nock, as nock does not support intercepting native fetch calls. This applies to apps/api/test/functional/sign-and-broadcast-tx.spec.ts and any other tests using native fetch.
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1372
File: apps/api/src/dashboard/services/stats/stats.service.ts:0-0
Timestamp: 2025-05-28T20:42:58.200Z
Learning: The user successfully implemented CosmosHttpService with retry logic using axiosRetry, exponential backoff, and proper error handling for Cosmos API calls, replacing direct axios usage in StatsService.
📚 Learning: 2025-11-19T15:15:07.283Z
Learnt from: ygrishajev
Repo: akash-network/console PR: 2254
File: apps/api/test/functional/sign-and-broadcast-tx.spec.ts:4-4
Timestamp: 2025-11-19T15:15:07.283Z
Learning: In the Akash Network Console project, when tests use native Node.js fetch (available in Node 18+), fetch-mock should be used for HTTP mocking instead of nock, as nock does not support intercepting native fetch calls. This applies to apps/api/test/functional/sign-and-broadcast-tx.spec.ts and any other tests using native fetch.
Applied to files:
apps/deploy-web/src/services/createFallbackableHttpClient/createFallbackableHttpClient.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
📚 Learning: 2025-05-28T20:42:58.200Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1372
File: apps/api/src/dashboard/services/stats/stats.service.ts:0-0
Timestamp: 2025-05-28T20:42:58.200Z
Learning: The user successfully implemented CosmosHttpService with retry logic using axiosRetry, exponential backoff, and proper error handling for Cosmos API calls, replacing direct axios usage in StatsService.
Applied to files:
apps/deploy-web/src/services/createFallbackableHttpClient/createFallbackableHttpClient.tspackages/http-sdk/src/utils/httpClient.ts
⏰ 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). (13)
- GitHub Check: validate (apps/stats-web) / validate-unsafe
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/deploy-web/src/services/createFallbackableHttpClient/createFallbackableHttpClient.ts (2)
1-3: LGTM - Import centralization is clean.Moving imports from local path to
@akashnetwork/http-sdkaligns with the PR's goal of centralizing the fetch adapter.
17-23: No changes needed. The customIBreakerimplementation is type-safe and correctly compatible with cockatiel's interface. TypeScript validation passes without errors, and the code is already unit tested.packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts (4)
6-17: LGTM - Well-typed options interface.The
FetchAdapterOptionsinterface provides flexible configuration for retries, circuit breaker, and the new abort functionality.
74-78: LGTM - Clear error classification logic.The function correctly distinguishes between network-level errors (connection refused, reset, etc.) and HTTP-level errors, applying appropriate retry logic to each.
86-94: LGTM - Correct idempotent request error handling.The logic correctly identifies retriable errors for idempotent HTTP methods, excluding user-initiated aborts and only retrying on transient failures (rate limits, server errors, network issues).
96-113: No action needed. The project explicitly requires Node 24.11.1+ (viaenginesin rootpackage.json), which fully supportsAbortSignal.any()(available in Node 20.3+). Since http-sdk is a Node.js library targeting the server environment, browser compatibility is not a concern. A polyfill is unnecessary.Likely an incorrect or invalid review comment.
packages/http-sdk/src/index.ts (1)
28-28: LGTM - Clean public API expansion.The new exports properly expose the fetch adapter factory and its associated types, enabling consumers to configure and use the adapter directly.
packages/http-sdk/src/utils/httpClient.ts (2)
6-12: LGTM - Clean adapter integration.The destructuring pattern cleanly separates adapter-specific options from axios config, and the adapter is properly created with the abort functionality.
25-26: LGTM - Type extension is correct.The
HttpClientOptionstype properly extends axios defaults with the new abort predicate option.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/authentication-handling
branch
from
ffc28b2 to
8a0f4c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/core/providers/http-sdk.provider.ts (1)
32-37: Inconsistency:SERVICESbypass the standardized adapter.
BalanceHttpService,BidHttpService, andProviderHttpServiceare instantiated directly with only{ baseURL }, bypassingcreateHttpClientand the new adapter configuration with retry/circuit-breaker logic.Unlike
NON_AXIOS_SERVICES(which receiveCHAIN_API_HTTP_CLIENTcreated viacreateHttpClient), these services are plain Axios instances without the fetch adapter. Consider refactoring them to accept anHttpClientdependency like theNON_AXIOS_SERVICESpattern, or add a comment explaining why they intentionally skip the standardized adapter.
🤖 Fix all issues with AI agents
In `@packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts`:
- Line 36: The assignment for maxAttempts incorrectly uses the || operator which
treats 0 as falsy and overrides an explicit retries: 0; update the expression in
createFetchAdapter so maxAttempts: options.retries ?? 3 (use the nullish
coalescing operator) so that undefined/null fall back to 3 but 0 is respected;
locate the maxAttempts setting where options.retries is referenced and replace
|| with ??.
- Line 28: The breaker initialization uses a fallthrough operator that treats
explicit 0 as falsy; update the maxAttempts fallback to use nullish coalescing
so an explicit 0 is honored: in the breaker property (the object key that
constructs a new ConsecutiveBreaker) replace the use of || for
options.circuitBreaker?.maxAttempts with ?? so it passes
options.circuitBreaker?.maxAttempts ?? 1 into the ConsecutiveBreaker
constructor; leave the rest (the breaker property and ConsecutiveBreaker usage)
unchanged.
🧹 Nitpick comments (3)
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts (1)
391-411: Consider strengthening the signal preservation test.The test verifies that the passed signal is replaced with an internal one, but it doesn't verify that aborting the external controller still propagates correctly. Consider adding an assertion that the external abort signal's abort event still triggers the request cancellation.
💡 Suggested enhancement
it("aborts when external signal is aborted", async () => { const externalAbortController = new AbortController(); const adapter = jest.fn().mockImplementation(async (config: InternalAxiosRequestConfig) => { await wait(100, undefined, { signal: config.signal as AbortSignal }); return { status: 200, data: "success" }; }); const fetch = createFetchAdapter({ adapter, abortPendingWhenOneFail: response => response.status === 401, retries: 1 }); const requestPromise = fetch({ method: "GET", url: "/test", headers: new AxiosHeaders(), signal: externalAbortController.signal }); externalAbortController.abort(); await expect(requestPromise).rejects.toThrow(); });packages/http-sdk/src/utils/httpClient.ts (1)
8-9: Consider makingretriesconfigurable.The retry count is hardcoded to
3. While this is a reasonable default, exposing it viaHttpClientOptionswould provide flexibility for consumers with different retry requirements.💡 Suggested enhancement
export type HttpClientOptions = Omit<CreateAxiosDefaults, "adapter"> & { /** `@default` 'fetch' */ adapter?: "fetch" | "xhr" | "http" | AxiosAdapter; abortPendingWhenOneFail?: (response: AxiosResponse) => boolean; + /** `@default` 3 */ + retries?: number; };export function createHttpClient(fullConfig: HttpClientOptions = {}): HttpClient { - const { abortPendingWhenOneFail, adapter, ...config } = fullConfig; + const { abortPendingWhenOneFail, adapter, retries = 3, ...config } = fullConfig; const customAdapter = createFetchAdapter({ - retries: 3, + retries, adapter: typeof adapter === "function" ? adapter : axios.getAdapter(adapter || "fetch"), abortPendingWhenOneFail });packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts (1)
64-64: Unnecessary cast whenconfig.signalmay beundefined.Cockatiel's
executeaccepts an optionalAbortSignal, so the cast is misleading. Passingundefinedis valid.Suggested fix
- .execute(() => fetchAdapter(config), config.signal as AbortSignal) + .execute(() => fetchAdapter(config), config.signal)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
apps/api/src/core/providers/http-sdk.provider.tsapps/deploy-web/src/context/SettingsProvider/SettingsProviderContext.tsxapps/deploy-web/src/services/app-di-container/app-di-container.tsapps/deploy-web/src/services/createFallbackableHttpClient/createFallbackableHttpClient.tsapps/deploy-web/src/services/createFetchAdapter/createFetchAdapter.tsapps/deploy-web/src/services/error-handler/error-handler.service.tsapps/notifications/src/modules/alert/providers/http-sdk.provider.tspackages/http-sdk/package.jsonpackages/http-sdk/src/index.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.tspackages/http-sdk/src/utils/httpClient.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/src/services/createFetchAdapter/createFetchAdapter.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/deploy-web/src/services/app-di-container/app-di-container.ts
- apps/deploy-web/src/services/createFallbackableHttpClient/createFallbackableHttpClient.ts
- packages/http-sdk/package.json
- apps/deploy-web/src/services/error-handler/error-handler.service.ts
- packages/http-sdk/src/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
packages/http-sdk/src/utils/httpClient.tspackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.tsapps/api/src/core/providers/http-sdk.provider.tsapps/notifications/src/modules/alert/providers/http-sdk.provider.tsapps/deploy-web/src/context/SettingsProvider/SettingsProviderContext.tsxpackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under testUse
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
**/*.spec.{ts,tsx}: Use<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references
Use either a method name or a condition starting with 'when' for nested suite descriptions in tests
Use present simple, 3rd person singular for test descriptions without prepending 'should'
Files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
setupfunction instead ofbeforeEachin unit and service level tests
Files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1372
File: apps/api/src/dashboard/services/stats/stats.service.ts:0-0
Timestamp: 2025-05-28T20:42:58.200Z
Learning: The user successfully implemented CosmosHttpService with retry logic using axiosRetry, exponential backoff, and proper error handling for Cosmos API calls, replacing direct axios usage in StatsService.
📚 Learning: 2025-05-28T20:42:58.200Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1372
File: apps/api/src/dashboard/services/stats/stats.service.ts:0-0
Timestamp: 2025-05-28T20:42:58.200Z
Learning: The user successfully implemented CosmosHttpService with retry logic using axiosRetry, exponential backoff, and proper error handling for Cosmos API calls, replacing direct axios usage in StatsService.
Applied to files:
packages/http-sdk/src/utils/httpClient.ts
📚 Learning: 2025-11-19T15:15:07.283Z
Learnt from: ygrishajev
Repo: akash-network/console PR: 2254
File: apps/api/test/functional/sign-and-broadcast-tx.spec.ts:4-4
Timestamp: 2025-11-19T15:15:07.283Z
Learning: In the Akash Network Console project, when tests use native Node.js fetch (available in Node 18+), fetch-mock should be used for HTTP mocking instead of nock, as nock does not support intercepting native fetch calls. This applies to apps/api/test/functional/sign-and-broadcast-tx.spec.ts and any other tests using native fetch.
Applied to files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.tsapps/deploy-web/src/context/SettingsProvider/SettingsProviderContext.tsxpackages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts
📚 Learning: 2025-11-25T17:45:52.965Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-11-25T17:45:52.965Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
Applied to files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
📚 Learning: 2025-11-25T17:45:44.790Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-11-25T17:45:44.790Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test
Applied to files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
📚 Learning: 2026-01-13T17:40:26.436Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T17:40:26.436Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Use `setup` function instead of `beforeEach` in unit and service level tests
Applied to files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
📚 Learning: 2025-10-31T11:26:42.138Z
Learnt from: stalniy
Repo: akash-network/console PR: 2138
File: apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts:379-382
Timestamp: 2025-10-31T11:26:42.138Z
Learning: In TypeScript/JavaScript, the pattern of checking a cached value and then performing an async operation to fetch it without proper synchronization is race condition unsafe:
```typescript
private async getAddress() {
if (!this.address) {
this.address = await this.wallet.getFirstAddress();
}
return this.address;
}
```
Multiple concurrent calls can all pass the `if (!this.address)` check before any of them sets the value, leading to duplicate async operations. This should be flagged as a race condition. Proper synchronization (mutex, atomic promise caching, or guaranteed single-threaded execution) is required.
Applied to files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts
🧬 Code graph analysis (2)
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts (2)
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts (1)
createFetchAdapter(20-72)packages/http-sdk/src/index.ts (1)
createFetchAdapter(28-28)
apps/api/src/core/providers/http-sdk.provider.ts (4)
packages/http-sdk/src/coin-gecko/coin-gecko-http.service.ts (1)
CoinGeckoHttpService(4-11)packages/http-sdk/src/utils/httpClient.ts (1)
createHttpClient(6-24)packages/http-sdk/src/index.ts (1)
createHttpClient(27-27)packages/http-sdk/src/node/node-http.service.ts (1)
NodeHttpService(16-31)
⏰ 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). (18)
- GitHub Check: validate (apps/provider-console) / validate-unsafe
- GitHub Check: validate (apps/stats-web) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (21)
apps/notifications/src/modules/alert/providers/http-sdk.provider.ts (2)
14-18: LGTM!The factory correctly passes
adapter: "http"tocreateHttpClient, aligning with the standardized HTTP adapter configuration across the codebase.
30-34: Inconsistency:BalanceHttpServicebypasses the standardized adapter.Unlike the other services that use
CHAIN_API_HTTP_CLIENT_TOKEN(which now includesadapter: "http"),BalanceHttpServiceis instantiated directly with onlybaseURL. This means it won't use the centralized fetch adapter with retry/circuit-breaker logic.Is this intentional? If
BalanceHttpServiceshould also use the standardized adapter, consider refactoring it to accept anHttpClientlike the other services.apps/deploy-web/src/context/SettingsProvider/SettingsProviderContext.tsx (3)
3-3: LGTM!Import correctly consolidated from a local path to the centralized
@akashnetwork/http-sdkpackage.
55-59: LGTM!The fetch adapter is correctly instantiated at module level with a 5-second
halfOpenAftercircuit breaker configuration, which is appropriate for node status polling operations.
192-195: LGTM!The
fetchAdapteris correctly passed to the HTTP client for status requests, enabling the circuit breaker behavior for unreliable node endpoints.packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.spec.ts (5)
8-15: Timer setup usingbeforeEach/afterEachis acceptable here.While coding guidelines prefer
setupfunctions overbeforeEach, this usage is appropriate since it's configuring Jest's timer environment rather than creating test subjects. Thesetuppattern is meant for object instantiation and dependency injection.
191-196: LGTM!The circuit breaker test now explicitly configures
maxAttempts: 1, making the test behavior more deterministic and self-documenting.
282-320: Well-structured abort test.The test correctly validates the abort behavior by:
- Using staggered delays (
i * 100) to ensure predictable ordering- Verifying both the aborted request (
AbortError) and the triggering request (401 status)- Using
Promise.allSettledto capture both outcomesThe
wait()call withconfig.signalensures the abort signal properly cancels pending waits, which is the intended behavior.
322-354: LGTM!The test correctly verifies that the adapter allows subsequent requests after an abort scenario, confirming the abort controller is reset appropriately.
356-389: LGTM!Good negative test case verifying that requests are not aborted when the failure condition (401) doesn't match (400 in this case).
apps/api/src/core/providers/http-sdk.provider.ts (3)
23-30: LGTM!The
CHAIN_API_HTTP_CLIENTfactory correctly passesadapter: "http"tocreateHttpClient, enabling the Node.js HTTP adapter for server-side requests.
51-53: LGTM!
CoinGeckoHttpServicenow usescreateHttpClientwithadapter: "http", ensuring consistent HTTP handling for external API calls.
54-58: LGTM!
NodeHttpServicenow usescreateHttpClientwithadapter: "http", aligning with the standardized adapter configuration.packages/http-sdk/src/utils/httpClient.ts (3)
1-4: LGTM!Clean import restructuring, replacing axios-retry dependency with the local
createFetchAdapterthat provides equivalent retry functionality via Cockatiel.
6-12: LGTM!The adapter selection logic correctly handles both custom
AxiosAdapterfunctions and string-based adapter names, with"fetch"as the sensible default.
27-31: LGTM!Well-defined type with proper JSDoc default annotation. The adapter union type covers all standard axios adapters plus custom implementations.
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts (5)
74-78: LGTM!The function correctly distinguishes between network errors (with retriable codes) and Axios errors (checking idempotency). The logic handles both error categories appropriately.
80-84: LGTM!Covers the standard Node.js network error codes for retriable scenarios.
86-94: LGTM!Correctly identifies idempotent HTTP methods and appropriate retry conditions (429 rate-limit, 5xx server errors, network errors), while excluding explicitly aborted connections.
96-113: LGTM!The abort propagation logic correctly combines request-level and adapter-level abort signals. The pattern of resetting the controller after abort ensures subsequent requests aren't affected.
100-100: No action needed.AbortSignal.any()is fully supported in this project. The codebase targets Node 24.11.1 minimum (per rootpackage.jsonengines), which includes the API introduced in Node 20.0.0. The type assertion toAbortSignal(notany) is appropriate and follows the coding guidelines.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts
Outdated
Show resolved
Hide resolved
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts
Outdated
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/authentication-handling
branch
2 times, most recently
from
c7cdadf to
43592ee
Compare
stalniy
force-pushed
the
refactor/authentication-handling
branch
from
43592ee to
41f61cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts`:
- Around line 49-52: The code computes a potentially negative delay from the
Retry-After header and passes it into ConstantBackoff; update the logic in
createFetchAdapter to clamp the computed delay to zero or positive before
constructing ConstantBackoff (e.g., compute delay = Math.max(0,
retryAfterDate.getTime() - Date.now() + EXTRA_RETRY_AFTER_DELAY)) and use that
clamped delay when creating new ConstantBackoff(...).next(); reference
retryAfterDate, EXTRA_RETRY_AFTER_DELAY, and ConstantBackoff to locate the
change.
♻️ Duplicate comments (2)
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts (2)
28-29: Same||issue withhalfOpenAfteron line 29.The
||operator on line 29 treats0as falsy. WhilehalfOpenAfter: 0may be an edge case, using??maintains consistency with the fix needed formaxAttemptson the same line.Note: The
maxAttemptsissue on line 28 was already flagged in a previous review.Suggested fix
- breaker: options.circuitBreaker?.breaker ?? new ConsecutiveBreaker(options.circuitBreaker?.maxAttempts ?? 1), - halfOpenAfter: options.circuitBreaker?.halfOpenAfter || 15 * 1000 + breaker: options.circuitBreaker?.breaker ?? new ConsecutiveBreaker(options.circuitBreaker?.maxAttempts ?? 1), + halfOpenAfter: options.circuitBreaker?.halfOpenAfter ?? 15 * 1000
38-38: Already flagged:||treats explicit0as falsy.This was previously identified in an earlier review. Use
??to respect explicitretries: 0.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.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:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Learnt from: ygrishajev
Repo: akash-network/console PR: 2254
File: apps/api/test/functional/sign-and-broadcast-tx.spec.ts:4-4
Timestamp: 2025-11-19T15:15:07.283Z
Learning: In the Akash Network Console project, when tests use native Node.js fetch (available in Node 18+), fetch-mock should be used for HTTP mocking instead of nock, as nock does not support intercepting native fetch calls. This applies to apps/api/test/functional/sign-and-broadcast-tx.spec.ts and any other tests using native fetch.
📚 Learning: 2025-11-19T15:15:07.283Z
Learnt from: ygrishajev
Repo: akash-network/console PR: 2254
File: apps/api/test/functional/sign-and-broadcast-tx.spec.ts:4-4
Timestamp: 2025-11-19T15:15:07.283Z
Learning: In the Akash Network Console project, when tests use native Node.js fetch (available in Node 18+), fetch-mock should be used for HTTP mocking instead of nock, as nock does not support intercepting native fetch calls. This applies to apps/api/test/functional/sign-and-broadcast-tx.spec.ts and any other tests using native fetch.
Applied to files:
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts
⏰ 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). (15)
- GitHub Check: codecov/patch/deploy-web
- GitHub Check: validate (apps/api) / validate-unsafe
- GitHub Check: validate (packages) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/http-sdk/src/utils/createFetchAdapter/createFetchAdapter.ts (3)
100-117: LGTM on abort propagation logic.The
abortableAdaptercorrectly combines request-level and adapter-level abort signals, aborts all in-flight requests when the predicate triggers, and resets the controller for subsequent requests.
78-98: LGTM on error classification helpers.The
isNetworkOrIdempotentRequestError,isRetriableError, andisIdempotentRequestErrorfunctions correctly classify retriable errors based on network conditions and idempotent HTTP methods.
103-107: Verify minimum browser version requirements forAbortSignal.any()usage.
AbortSignal.any()was added in Node.js 20+ (already satisfied by ^24.11.1), but browser support requires Chrome 116+, Firefox 124+, and Safari 17.4+. The project has no.browserslistrcor explicit browser version documentation. While the modern tech stack suggests modern browser targets, confirm that the minimum supported browser versions align withAbortSignal.any()availability, or add a fallback/polyfill if older browser support is required.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
3 participants
Why
If one request to authenticated API fails with 401, we must cancel all in flight requests to the same API, since they will fail with 401 as well and will produce noise in error reporting
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.