Unify timeout utils #16851
Unify timeout utils #16851
Conversation
WalkthroughThe changes update the codebase to standardize the handling of asynchronous delays by replacing the use of Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
marekrjpolak
force-pushed
the
refactor/timeout-util
branch
from
5cc78a6 to
9f32633
Compare
marekrjpolak
force-pushed
the
refactor/timeout-util
branch
from
9f32633 to
f4d872e
Compare
marekrjpolak
requested review from
karliatto,
mroz22,
szymonlesisz and
martykan
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/utils/tests/resolveAfter.test.ts (1)
7-7: Simplify function call by omitting unnecessaryundefinedparameterPassing
undefinedexplicitly for thesignalparameter is unnecessary and can be omitted for cleaner code. The function will treat the absence of thesignalparameter asundefinedby default.Apply this diff to simplify the function call:
-const promise = resolveAfter(200, undefined, 'foo'); +const promise = resolveAfter(200, 'foo');packages/transport/src/api/udp.ts (1)
39-39: Consider passing the abort signal toresolveAfter.Since
resolveAfternow supports an abort signal and the class already has alistenAbortController, consider passing the signal to make the delay abortable:-await resolveAfter(500); +await resolveAfter(500, this.listenAbortController.signal);This would allow the delay to be aborted when the UDP transport is disposed.
packages/connect/src/device/DeviceCommands.ts (1)
564-564: Consider adding version information in the comment.The 1ms delay is a workaround for bridge versions <= 2.0.28. Consider adding this version information in the code comment for better maintainability.
- // UI_EVENT is send right before ButtonAck, make sure that ButtonAck is sent + // UI_EVENT is sent right before ButtonAck. For bridge versions <= 2.0.28, + // ensure ButtonAck is sent before acquiring the devicepackages/transport-bridge/tests/http.test.ts (1)
706-706: Consider improving the test setup delay.The delay is currently used as a workaround for server initialization timing. As noted in the comment, this should be solved later with a more robust solution.
Consider replacing the fixed delay with a proper server readiness check, such as:
- Implementing a health check endpoint
- Using a server "ready" event
- Implementing a polling mechanism with timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/connect-popup/e2e/tests/popup-close.test.ts(2 hunks)packages/connect/src/api/composeTransaction.ts(2 hunks)packages/connect/src/api/getAccountInfo.ts(1 hunks)packages/connect/src/core/onCallFirmwareUpdate.ts(3 hunks)packages/connect/src/device/Device.ts(3 hunks)packages/connect/src/device/DeviceCommands.ts(2 hunks)packages/connect/src/device/DeviceList.ts(2 hunks)packages/connect/src/utils/abortablePromise.ts(0 hunks)packages/suite-desktop-core/src/app.ts(2 hunks)packages/suite/src/components/suite/WordInput.tsx(2 hunks)packages/suite/src/components/suite/WordInputAdvanced.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AddAccountModal/AddAccountButton/AddCoinjoinAccountButton.tsx(2 hunks)packages/transport-bridge/tests/http.test.ts(2 hunks)packages/transport/src/api/udp.ts(2 hunks)packages/transport/src/api/usb.ts(2 hunks)packages/transport/src/transports/bridge.ts(2 hunks)packages/utils/src/createTimeoutPromise.ts(0 hunks)packages/utils/src/index.ts(0 hunks)packages/utils/src/resolveAfter.ts(1 hunks)packages/utils/tests/resolveAfter.test.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/utils/src/index.ts
- packages/utils/src/createTimeoutPromise.ts
- packages/connect/src/utils/abortablePromise.ts
✅ Files skipped from review due to trivial changes (1)
- packages/connect/src/api/getAccountInfo.ts
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: e2e-test-suite-web (@group=other, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata2, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata1, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=settings, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=suite, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group_wallet, trezor-user-env-unix bitcoin-regtest, 1)
- GitHub Check: e2e-test-suite-web (@group_other, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_passphrase, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_device-management, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_suite, trezor-user-env-unix, 1)
- GitHub Check: Type Checking
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Unit Tests
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (17)
packages/utils/src/resolveAfter.ts (1)
11-14: Ensure proper cleanup of event listenersReturning
promise.finallyto clear the timeout and remove the abort event listener is good practice. This ensures that resources are properly cleaned up regardless of whether the promise is resolved or rejected.packages/utils/tests/resolveAfter.test.ts (1)
15-19:⚠️ Potential issueNon-standard usage of
abort.abort(new Error('bar'))The
AbortController.abort()method does not accept arguments in the standard API; passing an error toabort()is non-standard and may not work consistently across environments. This could lead to the rejection reason beingundefinedor not as expected.Consider modifying the test to handle the rejection appropriately:
-// Reject the promise after 100ms -setTimeout(() => abort.abort(new Error('bar')), 100); +// Abort the signal after 100ms +setTimeout(() => abort.abort(), 100); + +// Ensure the promise rejects with a specific error +promise.catch(error => { + expect(error.name).toBe('AbortError'); + expect(error.message).toBe('The operation was aborted.'); +});Please verify that the rejection reason is as expected and compatible with your environment.
packages/suite/src/components/suite/WordInput.tsx (1)
83-83: LGTM!Replacing
createTimeoutPromisewithresolveAftercorrectly maintains the intended asynchronous delay without impacting functionality.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AddAccountModal/AddAccountButton/AddCoinjoinAccountButton.tsx (1)
114-114: Verify the TODO comment and consider removing the delay.The timeout appears to be a temporary solution. Please verify if issue #6902 has been resolved and consider removing the delay if possible.
packages/suite/src/components/suite/WordInputAdvanced.tsx (1)
43-43: LGTM!The delay before sending the UI response appears intentional and the change to use
resolveAfteris appropriate.packages/suite-desktop-core/src/app.ts (1)
303-303: LGTM!The change to use
resolveAfteris appropriate here. The timeout is used in a Promise.race to ensure modules quit within 5 seconds, and no abort signal is needed as the race will handle the cancellation.packages/transport/src/transports/bridge.ts (1)
149-149: LGTM!The change to use
resolveAfter(1000)maintains the same delay functionality while simplifying the code.packages/connect/src/api/composeTransaction.ts (2)
304-304: Consider revisiting the temporary solution.The 501ms delay is marked as a temporary solution for the "device call in progress" issue. Consider investigating and implementing a more robust solution.
339-339: LGTM!The 2000ms delay before returning to discovery after insufficient funds provides a good user experience by allowing time to read the error message.
packages/connect-popup/e2e/tests/popup-close.test.ts (1)
149-149: LGTM!The change to use
resolveAfter(WAIT_AFTER_TEST)maintains the same test cleanup functionality while aligning with the new timeout utility standard.packages/transport/src/api/usb.ts (1)
280-280: LGTM! Consistent with the PR objectives.The change correctly replaces the timeout utility while maintaining the existing retry delay behavior.
packages/connect/src/core/onCallFirmwareUpdate.ts (2)
93-93: LGTM! Critical delay for device reconnection.The 2-second delay is essential for proper device reconnection during firmware updates.
463-463: LGTM! Required delay for T1 devices.The 2-second delay is crucial for T1 devices after reboot to bootloader, as documented in the comment referencing issue #1983.
packages/connect/src/device/DeviceList.ts (1)
399-399: LGTM! Proper abort signal handling.The change correctly utilizes the new
resolveAfterutility with abort signal support for transport reconnection delays.packages/connect/src/device/Device.ts (3)
12-12: LGTM!The import of
resolveAfteris correctly added to the existing destructured import statement from@trezor/utils.
346-346: LGTM!The replacement of
createTimeoutPromisewithresolveAftermaintains the same timing logic while aligning with the PR's goal to unify timeout utilities.
387-387: LGTM!The replacement of
createTimeoutPromisewithresolveAftermaintains the same timing logic while aligning with the PR's goal to unify timeout utilities.
| const onAbort = () => reject(signal?.reason); | ||
| signal?.addEventListener('abort', onAbort); | ||
| if (signal?.aborted) onAbort(); |
There was a problem hiding this comment.
Potential issue: AbortSignal may not have a reason property
In the onAbort function, you are accessing signal?.reason to reject the promise. However, the standard AbortSignal does not have a reason property in all environments. This could result in undefined being passed as the rejection reason, potentially causing unexpected behavior.
Consider modifying the onAbort function to reject with a standard error message:
-const onAbort = () => reject(signal?.reason);
+const onAbort = () => reject(new DOMException('The operation was aborted.', 'AbortError'));Alternatively, ensure that your environment supports AbortSignal.reason or handle cases where signal.reason may be undefined.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onAbort = () => reject(signal?.reason); | |
| signal?.addEventListener('abort', onAbort); | |
| if (signal?.aborted) onAbort(); | |
| const onAbort = () => reject(new DOMException('The operation was aborted.', 'AbortError')); | |
| signal?.addEventListener('abort', onAbort); | |
| if (signal?.aborted) onAbort(); |
| @@ -1,11 +1,15 @@ | |||
| import { createDeferred } from './createDeferred'; | |||
|
|
|||
| export const resolveAfter = <T = void>(msec: number, value?: T) => { | |||
| export const resolveAfter = <T = void>(msec: number, signal?: AbortSignal, value?: T) => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reordering parameters to maintain backward compatibility
The addition of the optional signal parameter as the second argument changes the function signature and may break existing calls that provide value as the second parameter. To maintain backward compatibility and enhance usability, consider placing signal as the last parameter.
Apply this diff to adjust the parameter order:
-export const resolveAfter = <T = void>(msec: number, signal?: AbortSignal, value?: T) => {
+export const resolveAfter = <T = void>(msec: number, value?: T, signal?: AbortSignal) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const resolveAfter = <T = void>(msec: number, signal?: AbortSignal, value?: T) => { | |
| export const resolveAfter = <T = void>(msec: number, value?: T, signal?: AbortSignal) => { |
Description
This PR makes
resolveAfterutil abortable by signal, therefore it can replacecreateTimeoutPromiseandabortablePromiseutils, which can be removed.