TON-1531: abort controllers in api client#479
Open
heyllog wants to merge 5 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces ChangesPer-request AbortSignal support and error hierarchy
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ApiClientTonApi
participant BaseApiClient
participant combineSignals
participant HTTPFetch
rect rgba(100, 149, 237, 0.5)
note over Caller,HTTPFetch: Per-request abort signal flow
Caller->>ApiClientTonApi: getAccountState(address, seqno, {signal})
ApiClientTonApi->>BaseApiClient: getJson(path, query, {signal})
BaseApiClient->>BaseApiClient: doRequest(url, init, signal)
alt signal already aborted
BaseApiClient-->>Caller: throw abort reason (no fetch)
else timeout configured
BaseApiClient->>combineSignals: combineSignals(signal, timeoutController.signal)
combineSignals-->>BaseApiClient: combinedSignal
BaseApiClient->>HTTPFetch: fetch(url, {signal: combinedSignal})
alt timeout fires first
BaseApiClient-->>Caller: throw ApiClientTimeoutError
else caller signal fires first
BaseApiClient-->>Caller: propagate abort reason
else success
HTTPFetch-->>BaseApiClient: Response
BaseApiClient-->>ApiClientTonApi: parsed JSON
ApiClientTonApi-->>Caller: AccountState
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/walletkit/src/clients/tonapi/ApiClientTonApi.ts (1)
395-417:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t swallow caller aborts in DNS helpers.
With
opts.signalnow supported, these broadcatchblocks convert aborts intoundefined, making cancellation indistinguishable from a normal “not found” result.💡 Suggested fix
- } catch (_e) { + } catch (error) { + if (opts?.signal?.aborted) { + throw error; + } return undefined; } @@ - } catch (_e) { + } catch (error) { + if (opts?.signal?.aborted) { + throw error; + } return undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/walletkit/src/clients/tonapi/ApiClientTonApi.ts` around lines 395 - 417, The catch blocks in both resolveDnsWallet and backResolveDnsWallet methods are too broad and swallow all errors including AbortError from request cancellation, making it impossible to distinguish between a DNS lookup failure and a cancelled request. In both methods, check if the caught error is an AbortError and rethrow it to allow cancellation to propagate to the caller, while returning undefined only for actual DNS resolution failures. This allows the caller to properly handle request cancellations via opts.signal.
🧹 Nitpick comments (2)
packages/walletkit/src/clients/tonapi/ApiClientTonApi.spec.ts (1)
352-361: ⚡ Quick winAssert the explicit abort outcome to avoid false positives.
rejects.not.toBeInstanceOf(ApiClientTimeoutError)can pass for unrelated errors. Assert the exact abort reason to prove caller cancellation is what ended the request.💡 Suggested test tightening
it('forwards a caller abort through getMasterchainInfo down to fetch', async () => { const controller = new AbortController(); + const abortReason = new DOMException('Aborted', 'AbortError'); const client = new ApiClientTonApi({ fetchApi: hangingFetch(), timeout: 1000 }); const promise = client.getMasterchainInfo({ signal: controller.signal }); - controller.abort(); + controller.abort(abortReason); // If the forward were dropped, the request would hang until the 1000ms timeout // and reject as ApiClientTimeoutError instead. - await expect(promise).rejects.not.toBeInstanceOf(ApiClientTimeoutError); + await expect(promise).rejects.toBe(abortReason); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/walletkit/src/clients/tonapi/ApiClientTonApi.spec.ts` around lines 352 - 361, The test assertion in the getMasterchainInfo abort signal test uses a negative check that can pass for any error other than ApiClientTimeoutError, masking unrelated failures and not confirming the abort actually occurred. Replace the rejects.not.toBeInstanceOf assertion with a positive assertion that explicitly checks the promise rejects with an AbortError, confirming the caller abort signal is properly threaded through to termination rather than just relying on timeout behavior not occurring.packages/walletkit/src/clients/toncenter/ApiClientToncenter.spec.ts (1)
186-195: ⚡ Quick winStrengthen abort test to assert the exact cancellation reason.
Like the TON API spec, this currently allows false positives from any non-timeout rejection. Assert the specific abort reason to verify true signal propagation.
💡 Suggested test tightening
it('forwards a caller abort through getAccountState down to fetch', async () => { const controller = new AbortController(); + const abortReason = new DOMException('Aborted', 'AbortError'); const client = new ApiClientToncenter({ fetchApi: hangingFetch(), timeout: 1000 }); const promise = client.getAccountState(TEST_ADDRESS, undefined, { signal: controller.signal }); - controller.abort(); + controller.abort(abortReason); // If the forward were dropped, the request would hang until the 1000ms timeout // and reject as ApiClientTimeoutError instead. - await expect(promise).rejects.not.toBeInstanceOf(ApiClientTimeoutError); + await expect(promise).rejects.toBe(abortReason); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/walletkit/src/clients/toncenter/ApiClientToncenter.spec.ts` around lines 186 - 195, The test assertion for abort signal threading in the getAccountState method currently only checks that the promise does not reject with ApiClientTimeoutError, which allows false positives from other rejection types. Strengthen the test by replacing the negative assertion with a positive assertion that checks for the specific abort error type. When an AbortController is aborted, it typically rejects with an AbortError or DOMException with the name property set to "AbortError". Update the await expect statement to explicitly assert that the promise rejects with this specific abort error type instead of just verifying it's not a timeout error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/walletkit/src/api/interfaces/ApiClient.ts`:
- Around line 139-142: The getPendingTransactions method signature in the
ApiClientTonApi concrete implementation does not match the updated signature in
the ApiClient interface. Update the getPendingTransactions method in
ApiClientTonApi (located in
packages/walletkit/src/clients/tonapi/ApiClientTonApi.ts) to include the
optional opts?: RequestOptions parameter to align with the interface definition.
Ensure the implementation passes this options parameter through to any
underlying API calls so that consumers can provide per-request options
consistently across both the interface and concrete implementation.
In `@packages/walletkit/src/clients/BaseApiClient.ts`:
- Line 113: Replace the `externalSignal?.throwIfAborted()` call with a check
using the `.aborted` property instead, which has broader runtime compatibility
across older Safari versions, iOS, Android WebView, and Node.js versions. Check
if `externalSignal?.aborted` is true and throw an appropriate abort error when
needed, ensuring the abort check works correctly in all supported runtime
environments rather than silently becoming a no-op when throwIfAborted() is
unavailable.
In `@packages/walletkit/src/clients/toncenter/ApiClientToncenter.ts`:
- Around line 304-311: The retry predicate in the tryGetTrace function treats
all non-HTTP errors as retryable, which incorrectly includes abort errors from
canceled requests. Update the retry predicate to detect abort errors (check for
AbortError or similar cancellation indicators) and return false to prevent
retries when an abort occurs, ensuring abort semantics are preserved rather than
being retried. Additionally, ensure getPendingTrace does not wrap abort errors
into a generic error but instead re-throws them directly to maintain proper
error semantics for the caller. Apply the same abort-aware changes to the
similar retry logic in the range around lines 341-367.
---
Outside diff comments:
In `@packages/walletkit/src/clients/tonapi/ApiClientTonApi.ts`:
- Around line 395-417: The catch blocks in both resolveDnsWallet and
backResolveDnsWallet methods are too broad and swallow all errors including
AbortError from request cancellation, making it impossible to distinguish
between a DNS lookup failure and a cancelled request. In both methods, check if
the caught error is an AbortError and rethrow it to allow cancellation to
propagate to the caller, while returning undefined only for actual DNS
resolution failures. This allows the caller to properly handle request
cancellations via opts.signal.
---
Nitpick comments:
In `@packages/walletkit/src/clients/tonapi/ApiClientTonApi.spec.ts`:
- Around line 352-361: The test assertion in the getMasterchainInfo abort signal
test uses a negative check that can pass for any error other than
ApiClientTimeoutError, masking unrelated failures and not confirming the abort
actually occurred. Replace the rejects.not.toBeInstanceOf assertion with a
positive assertion that explicitly checks the promise rejects with an
AbortError, confirming the caller abort signal is properly threaded through to
termination rather than just relying on timeout behavior not occurring.
In `@packages/walletkit/src/clients/toncenter/ApiClientToncenter.spec.ts`:
- Around line 186-195: The test assertion for abort signal threading in the
getAccountState method currently only checks that the promise does not reject
with ApiClientTimeoutError, which allows false positives from other rejection
types. Strengthen the test by replacing the negative assertion with a positive
assertion that checks for the specific abort error type. When an AbortController
is aborted, it typically rejects with an AbortError or DOMException with the
name property set to "AbortError". Update the await expect statement to
explicitly assert that the promise rejects with this specific abort error type
instead of just verifying it's not a timeout error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3beb4c4-2958-46b0-973b-2e298662049f
📒 Files selected for processing (14)
packages/walletkit/src/api/interfaces/ApiClient.tspackages/walletkit/src/clients/BaseApiClient.spec.tspackages/walletkit/src/clients/BaseApiClient.tspackages/walletkit/src/clients/TonClientError.tspackages/walletkit/src/clients/combine-signals.tspackages/walletkit/src/clients/errors.tspackages/walletkit/src/clients/tonapi/ApiClientTonApi.spec.tspackages/walletkit/src/clients/tonapi/ApiClientTonApi.tspackages/walletkit/src/clients/toncenter/ApiClientToncenter.spec.tspackages/walletkit/src/clients/toncenter/ApiClientToncenter.tspackages/walletkit/src/clients/types.tspackages/walletkit/src/defi/gasless/tonapi/mappers/map-gasless-error.tspackages/walletkit/src/defi/gasless/tonapi/utils.tspackages/walletkit/src/index.ts
💤 Files with no reviewable changes (1)
- packages/walletkit/src/clients/TonClientError.ts
Comment on lines
+139
to
+142
| getPendingTransactions( | ||
| request: GetPendingTransactionsRequest, | ||
| opts?: RequestOptions, | ||
| ): Promise<TransactionsResponse>; |
There was a problem hiding this comment.
Align getPendingTransactions signature with concrete TON API client.
ApiClient now declares getPendingTransactions(request, opts?), but ApiClientTonApi still exposes a single-argument signature. Consumers typed as ApiClientTonApi can’t pass RequestOptions on this method, which breaks the new per-request contract consistency.
💡 Suggested fix (in packages/walletkit/src/clients/tonapi/ApiClientTonApi.ts)
- async getPendingTransactions(_request: GetPendingTransactionsRequest): Promise<TransactionsResponse> {
+ async getPendingTransactions(
+ _request: GetPendingTransactionsRequest,
+ _opts?: RequestOptions,
+ ): Promise<TransactionsResponse> {
// TonAPI doesn't expose Toncenter-like pending transaction list.
// Returning an empty list keeps compatibility with existing consumers.
return {
transactions: [],
addressBook: {},
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/walletkit/src/api/interfaces/ApiClient.ts` around lines 139 - 142,
The getPendingTransactions method signature in the ApiClientTonApi concrete
implementation does not match the updated signature in the ApiClient interface.
Update the getPendingTransactions method in ApiClientTonApi (located in
packages/walletkit/src/clients/tonapi/ApiClientTonApi.ts) to include the
optional opts?: RequestOptions parameter to align with the interface definition.
Ensure the implementation passes this options parameter through to any
underlying API calls so that consumers can provide per-request options
consistently across both the interface and concrete implementation.
| if (!this.timeout || this.timeout <= 0) { | ||
| return fetchFn(url, init); | ||
| // Bail out before touching the network if the caller already aborted. | ||
| externalSignal?.throwIfAborted(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AbortSignal.throwIfAborted compatibility matrix for Safari, iOS WebView, Android WebView, and Node.js (minimum versions that support it).
💡 Result:
The AbortSignal.throwIfAborted method is supported in the following environments: Browser Support: - Safari: Supported since version 15.4 [1]. - iOS WebView (WKWebView): Supported since version 15.4 [1]. - Android WebView: Supported since version 149 [1]. Node.js Support: - Node.js: Supported since v16.17.0 and v17.3.0 [2][3][4][5]. Summary Table: Environment | Minimum Version --- | --- Safari | 15.4 [1] iOS WebView | 15.4 [1] Android WebView | 149 [1] Node.js | 16.17.0 / 17.3.0 [2][3][4][5] Note: While AbortController and the basic AbortSignal interface have broader support in older versions of these platforms (e.g., Android WebView 66+ for AbortController) [6][7], the specific throwIfAborted convenience method was introduced later [1][8].
Citations:
- 1: https://caniuse.com/mdn-api_abortsignal_throwifaborted
- 2: https://nodejs.org/api/globals.html
- 3: https://nodejs.org/docs/latest-v26.x/api/globals.html
- 4: https://nodejs.org/dist/latest/docs/api/globals.html
- 5: https://r2.nodejs.org/docs/v20.6.0/api/globals.html
- 6: https://caniuse.com/abortcontroller
- 7: https://caniwebview.com/features/mdn-abortcontroller/
- 8: https://www.chromestatus.com/feature/5029737100476416
🏁 Script executed:
#!/bin/bash
# Check for runtime configuration in project files
for file in "tsconfig.json" "package.json" ".browserslistrc" "webpack.config.js" "babel.config.js"; do
if [ -f "$file" ]; then
echo "=== $file ==="
cat "$file" | head -50
fi
doneRepository: ton-org/kit
Length of output: 1944
🏁 Script executed:
#!/bin/bash
# Examine the BaseApiClient to understand signal handling context
wc -l packages/walletkit/src/clients/BaseApiClient.ts
cat -n packages/walletkit/src/clients/BaseApiClient.ts | head -150Repository: ton-org/kit
Length of output: 1944
🌐 Web query:
AbortSignal.aborted property availability and compatibility across runtime versions (Safari, iOS, Android WebView, Node.js)
💡 Result:
The AbortSignal.aborted property is widely supported across modern web browsers and server-side environments [1][2]. It has been a stable, established feature since early 2018 [1]. Runtime Compatibility: - Safari and iOS Safari: Supported since version 11.1 [3][2]. - Android WebView: Supported since version 66 [2]. - Chrome and Chrome for Android: Supported since version 66 [3][2]. - Node.js: Supported since version 15.0.0 and 14.17.0 [4][5]. The property is a read-only boolean that returns true if the signal has been aborted and false otherwise [1][6]. It is commonly used alongside AbortController to manage and cancel asynchronous operations [1][4]. Because it is a well-established standard, it is widely available in all modern browsers and Node.js environments [1][4][2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/aborted
- 2: https://docs.w3cub.com/dom/abortsignal/aborted
- 3: https://caniuse.com/mdn-api_abortsignal_aborted
- 4: https://nodejs.org/api/globals.html
- 5: https://nodejs.org/docs/latest-v26.x/api/globals.html
- 6: https://github.com/mdn/content/blob/main/files/en-us/web/api/abortsignal/aborted/index.md
Use .aborted property instead of throwIfAborted() for broader runtime compatibility.
Line 113 uses externalSignal?.throwIfAborted(), which is unavailable in many environments. While the optional chaining prevents a TypeError, it causes the abort check to silently become a no-op in runtimes that don't support throwIfAborted() (Safari <15.4, iOS <15.4, Android WebView <149, Node.js <16.17.0). The .aborted property has much broader compatibility (Safari 11.1+, iOS 11.1+, Android WebView 66+, Node.js 14.17.0+), making it a more reliable choice.
🐛 Proposed fix
- externalSignal?.throwIfAborted();
+ if (externalSignal?.aborted) {
+ throw externalSignal.reason ?? new Error('Aborted');
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/walletkit/src/clients/BaseApiClient.ts` at line 113, Replace the
`externalSignal?.throwIfAborted()` call with a check using the `.aborted`
property instead, which has broader runtime compatibility across older Safari
versions, iOS, Android WebView, and Node.js versions. Check if
`externalSignal?.aborted` is true and throw an appropriate abort error when
needed, ensuring the abort check works correctly in all supported runtime
environments rather than silently becoming a no-op when throwIfAborted() is
unavailable.
Comment on lines
304
to
311
| const tryGetTrace = async (field: 'tx_hash' | 'trace_id' | 'msg_hash') => { | ||
| const response = await CallForSuccess( | ||
| () => this.getJson<ToncenterTracesResponse>('/api/v3/traces', { [field]: traceId }), | ||
| () => this.getJson<ToncenterTracesResponse>('/api/v3/traces', { [field]: traceId }, opts), | ||
| undefined, | ||
| undefined, | ||
| // 422: toncenter failed to decode field value | ||
| (err) => (err instanceof TonClientError ? err.status !== 422 : true), | ||
| (err) => (err instanceof ApiClientHttpError ? err.status !== 422 : true), | ||
| ); |
There was a problem hiding this comment.
Make trace retries abort-aware and preserve caller abort errors.
Current retry predicates treat non-HTTP errors (including aborts) as retryable, and getPendingTrace then wraps failures into a generic error. This causes canceled requests to be retried and lose abort semantics.
💡 Suggested fix
- (err) => (err instanceof ApiClientHttpError ? err.status !== 422 : true),
+ (err) => {
+ if (opts?.signal?.aborted) return false;
+ return err instanceof ApiClientHttpError ? err.status !== 422 : true;
+ },
@@
- (err) => (err instanceof ApiClientHttpError ? err.status !== 422 : true),
+ (err) => {
+ if (opts?.signal?.aborted) return false;
+ return err instanceof ApiClientHttpError ? err.status !== 422 : true;
+ },
@@
- } catch (error) {
+ } catch (error) {
+ if (opts?.signal?.aborted) {
+ throw error;
+ }
log.error('Error fetching pending trace', { error });
}Also applies to: 341-367
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/walletkit/src/clients/toncenter/ApiClientToncenter.ts` around lines
304 - 311, The retry predicate in the tryGetTrace function treats all non-HTTP
errors as retryable, which incorrectly includes abort errors from canceled
requests. Update the retry predicate to detect abort errors (check for
AbortError or similar cancellation indicators) and return false to prevent
retries when an abort occurs, ensuring abort semantics are preserved rather than
being retried. Additionally, ensure getPendingTrace does not wrap abort errors
into a generic error but instead re-throws them directly to maintain proper
error semantics for the caller. Apply the same abort-aware changes to the
similar retry logic in the range around lines 341-367.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements