Feat/websocket-client package #15797
Conversation
|
🚀 Expo preview is ready!
|
szymonlesisz
force-pushed
the
feat/websocket-package
branch
2 times, most recently
from
a0fc770 to
221cfac
Compare
szymonlesisz
requested review from
mroz22,
martykan and
marekrjpolak
as code owners
szymonlesisz
force-pushed
the
feat/websocket-package
branch
2 times, most recently
from
15ffec4 to
42604de
Compare
|
last minute changes. new package renamed to |
szymonlesisz
force-pushed
the
feat/websocket-package
branch
from
42604de to
6412ddb
Compare
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/12297127542 |
trezor-ci
force-pushed
the
feat/websocket-package
branch
from
d56c090 to
1824969
Compare
| if (url.startsWith('https')) { | ||
| url = url.replace('https', 'wss'); | ||
| } | ||
| if (url.startsWith('http')) { | ||
| url = url.replace('http', 'ws'); | ||
| } |
There was a problem hiding this comment.
total nitpick: the second one covers the first one as well
| } | ||
|
|
||
| protected onMessage(message: string) { | ||
| protected onMessage(message: WebsocketResponse) { |
There was a problem hiding this comment.
Probably not very important, but these overriden onMessage don't call setPingTimeout as the base one does, so the pings will be timed differently.
There was a problem hiding this comment.
i did some last minute changes in onMessage handler.
optionally it takes messageValidation function as second parameter and either resolve pending promise automatically (trezor-user-env and bluetooth implementation) or resolve it manually (blockchain-link implementation) in both cases setPingTimeout is called in the final block of websocket-client/client onMessage
|
im not sure if failing android e2e are somehow related to this change or not? |
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/12368290749 |
trezor-ci
force-pushed
the
feat/websocket-package
branch
from
1824969 to
1d3f760
Compare
|
android tests look like they have hanged for 40 minutes albeit finishing correctly and it is probably somehow related. nightly scheduled test finished successfully just 8 hours ago. Here is the list of recently finished android tests - https://github.com/trezor/trezor-suite/actions/workflows/test-suite-native-e2e-android.yml |
|
lets try rebasing it and see whether android tests pass or not |
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/12862656946 |
trezor-ci
force-pushed
the
feat/websocket-package
branch
from
1d3f760 to
f693b8c
Compare
mroz22
added
the
no-project
|
android tests still timing out - https://github.com/trezor/trezor-suite/actions/runs/12862670578/job/35858213762?pr=15797#step:11:403 something has changed in the unified websocket implementation for suite-native apparently |
|
@mroz22, there is probably hanging synchronization with the new WebSocket implementation. Detox E2E tests synchronize with the network events and pause the test until the communication is closed (if you do not set it differently on purpose). More here. All the tests pass, but the connection stays open so the test never ends and it times out. I can look at the tests tomorrow if you need help. We have encountered this issue multiple times on mobile. |
matejkriz
force-pushed
the
feat/websocket-package
branch
from
69c5d49 to
fbd5a15
Compare
szymonlesisz
force-pushed
the
feat/websocket-package
branch
2 times, most recently
from
691b937 to
cea9146
Compare
|
Finally green 💚
android e2e tests setup had 2 issues (also on develop but it was better visible here):
with this test setup: the second cli.connect() overrides this.ws instance, but then onClose() asynchronously called after cli.disconnect() remove all the listeners from this.ws (new instance, not the old one) |
| @@ -0,0 +1,52 @@ | |||
| { | |||
There was a problem hiding this comment.
cc @karliatto we will need to publish this. All good from your side in the package config?
There was a problem hiding this comment.
It is missing the "files" section. I imagine in this package should look like:
"files": [
"lib"
]
Regarding the field "exports" we can add it in a follow up when we add libESM since we want to keep using "publishConfig" as well.
There was a problem hiding this comment.
Since this package is dependency of blockchain-link it will be automatically added to the release process in next release.
There was a problem hiding this comment.
tnx, posted a fixup fcd363d
There was a problem hiding this comment.
damn, how could i missed that.
@coderabbitai watch and learn from @karliatto :)
There was a problem hiding this comment.
@szymonlesisz Indeed! This is a great learning opportunity about npm package configuration. Thank you for highlighting it.
✏️ Learnings added
Learnt from: szymonlesisz
PR: trezor/trezor-suite#15797
File: packages/websocket-client/package.json:1-1
Timestamp: 2025-01-31T16:22:41.478Z
Learning: When creating new npm packages, ensure the package.json includes a "files" field to explicitly specify which files should be included when publishing (e.g., "files": ["lib"]). This is especially important for packages that are compiled/built before publishing.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
szymonlesisz
force-pushed
the
feat/websocket-package
branch
from
cea9146 to
715ea47
Compare
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
error Error: http://10.0.0.28:4873/@trezor%2feslint: no such package available WalkthroughThis pull request introduces changes to the WebSocket client implementation within the Trezor Suite repository, primarily by creating a new Possibly related PRs
Suggested reviewers
✨ 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 (
|
|
unexpected eslint rule appeared, rebased to resolve confilicts in imports |
![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
🧹 Nitpick comments (12)
packages/blockchain-link/src/workers/baseWebsocket.ts (2)
22-36: Consider stricter typing in the message callback.
While passing({ id, data }) => {...}tosuper.onMessageworks, you might gain better type safety by defining a strongly typed payload structure. This ensures thatidanddataare coerced into the expected shapes.
38-41: Preserve original error details when wrapping.
Here, only theerror.messageis kept. Consider preserving additional details (e.g., error stack, code) to aid in debugging.return super.sendMessage(message).catch(originalError => { - throw new CustomError(originalError.message); + const wrappedError = new CustomError(originalError.message); + wrappedError.stack = originalError.stack; + throw wrappedError; });packages/trezor-user-env-link/src/websocket-client.ts (1)
Line range hint
66-77: Potential indefinite wait for the'firmwares'event.
The connect method resolves only after'firmwares'is emitted. If that never arrives, the Promises chain could stall. Consider a timed fallback or additional completion criteria.packages/websocket-client/src/client.ts (1)
128-128: Avoidvoidin a union type.
Static analysis flags usingvoid. Replace it withundefinedto be clearer and align with standard TS patterns.- messageValidation?: (data: Record<string, any>) => Record<string, any> | void, + messageValidation?: (data: Record<string, any>) => Record<string, any> | undefined,🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/websocket-client/src/ws-browser.ts (2)
Line range hint
41-43: Add readyState validation in send method.The
sendmethod should validate the connection state before attempting to send a message to prevent potential errors.send(message: any) { + if (this.readyState !== WSWrapper.OPEN) { + throw new Error(`Cannot send message. Connection is not open. Ready state: ${this.readyState}`); + } this._ws.send(message); }
Line range hint
13-13: Consider typing the protocols and options parameters.The
_protocolsand_websocketOptionsparameters are typed asany. Consider adding proper type definitions to improve type safety.-constructor(url: string, _protocols: any, _websocketOptions: any) { +type WebSocketProtocol = string | string[]; +type WebSocketOptions = { + headers?: Record<string, string>; + [key: string]: unknown; +}; +constructor(url: string, _protocols?: WebSocketProtocol, _websocketOptions?: WebSocketOptions) {packages/websocket-client/src/ws-native.ts (2)
Line range hint
17-22: Consider creating proper types for React Native WebSocket.The code uses
@ts-expect-errorto bypass TypeScript checks. Consider creating proper type definitions for React Native WebSocket to improve type safety.+interface ReactNativeWebSocketOptions { + headers?: Record<string, string>; +} + +// Update the WebSocket type declaration +declare class ReactNativeWebSocket extends WebSocket { + constructor(url: string, protocols?: string[], options?: ReactNativeWebSocketOptions); +} - // @ts-expect-error - this._ws = new WebSocket(url, ['wss'], { + this._ws = new ReactNativeWebSocket(url, ['wss'], {
Line range hint
1-63: Consider extracting common WebSocket functionality.Both
ws-browser.tsandws-native.tsshare significant code. Consider creating a base class to reduce duplication and improve maintainability.Would you like me to provide an example of how to extract the common functionality into a base class?
packages/websocket-client/tests/client.test.ts (4)
12-14: Remove redundantsendMessagemethod.The
sendMessagemethod simply calls the parent's method without adding any functionality. Consider removing it to reduce code complexity.- sendMessage(message: Record<string, any>) { - return super.sendMessage(message); - }
85-89: Remove empty if block in type checking test.The if block is empty and serves no purpose. Consider either adding an assertion or removing the block entirely.
- if (event === 'bar-event') { - // - } + expect(event).toBe('bar-event');
97-114: Improve ping test reliability and cleanup.Two issues in the ping test:
- The time advancement (20s) and expected ping count (4) don't align clearly.
- The spy is not restored after use.
const pingSpy = jest.spyOn(cli, 'ping'); + try { await cli.connect(); // call first messages to init ping const resp = await cli.sendMessage({ method: 'init' }); expect(resp.success).toEqual(true); // wait for ping - await jest.advanceTimersByTimeAsync(20000); + // Advance time by exactly 4 ping intervals + await jest.advanceTimersByTimeAsync(4 * 5000); expect(pingSpy).toHaveBeenCalledTimes(4); await cli.disconnect(); + } finally { + pingSpy.mockRestore(); + jest.useRealTimers(); + } - jest.useRealTimers();
125-126: Add await to final disconnect call.The final disconnect call should be awaited to ensure proper cleanup.
- cli.disconnect(); + await cli.disconnect();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
CODEOWNERS(1 hunks)packages/blockchain-link/jest.config.unit.js(0 hunks)packages/blockchain-link/package.json(2 hunks)packages/blockchain-link/src/workers/baseWebsocket.ts(1 hunks)packages/blockchain-link/src/workers/blockbook/websocket.ts(1 hunks)packages/blockchain-link/src/workers/blockfrost/websocket.ts(1 hunks)packages/blockchain-link/tsconfig.json(1 hunks)packages/blockchain-link/tsconfig.lib.json(1 hunks)packages/blockchain-link/webpack/dev.js(0 hunks)packages/blockchain-link/webpack/workers.web.js(0 hunks)packages/connect/e2e/karma.config.js(0 hunks)packages/trezor-user-env-link/package.json(1 hunks)packages/trezor-user-env-link/src/websocket-client.ts(4 hunks)packages/trezor-user-env-link/tsconfig.json(1 hunks)packages/websocket-client/README.md(1 hunks)packages/websocket-client/package.json(1 hunks)packages/websocket-client/src/client.ts(1 hunks)packages/websocket-client/src/index.ts(1 hunks)packages/websocket-client/src/ws-browser.ts(1 hunks)packages/websocket-client/src/ws-native.ts(1 hunks)packages/websocket-client/tests/client.test.ts(1 hunks)packages/websocket-client/tsconfig.json(1 hunks)packages/websocket-client/tsconfig.lib.json(1 hunks)suite-native/app/e2e/utils.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/blockchain-link/webpack/dev.js
- packages/blockchain-link/webpack/workers.web.js
- packages/blockchain-link/jest.config.unit.js
- packages/connect/e2e/karma.config.js
✅ Files skipped from review due to trivial changes (5)
- packages/websocket-client/src/index.ts
- packages/websocket-client/tsconfig.lib.json
- packages/websocket-client/tsconfig.json
- packages/websocket-client/package.json
- packages/websocket-client/README.md
🧰 Additional context used
🪛 Biome (1.9.4)
packages/websocket-client/src/client.ts
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Build libs for publishing
- GitHub Check: Other Checks
- GitHub Check: Releases revision Checks
- GitHub Check: Unit Tests
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- 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: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- 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: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (25)
packages/blockchain-link/src/workers/baseWebsocket.ts (3)
2-2: No issues with the import statement.
The imports from@trezor/websocket-clientlook good and match the usage throughout the file.
10-10: ExtendingWebsocketClient: All good here.
Inheriting fromWebsocketClient<T>is a clean way to keep shared connection logic consistent. No concerns about this abstract class approach.
13-18: Ensure disconnection logic is intentional.
This logic disconnects the WebSocket when there are no subscriptions andkeepAliveis false. Verify whether abrupt disconnection might impact downstream consumers that expect to reconnect or gracefully finalize operations.packages/trezor-user-env-link/src/websocket-client.ts (7)
5-8: Import from@trezor/websocket-clientis correctly referenced.
No issues found with the usage ofWebsocketClientBaseandWebsocketResponse.
28-29: Event definitions look good.
Defining"firmwares"and"disconnected"events neatly documents the intended event flow.
40-42:createWebsocketoverride is consistent.
Good job delegating tothis.initWebsocket(this.options)for actual WebSocket creation. That centralizes the connection logic ininitWebsocket.
45-55: Constructor: default options make sense.
Using standard fallback values forurl,timeout, andpingTimeoutis a robust approach. The code is self-explanatory and easy to maintain.
63-63: Delegating sends tosendMessage.
Replacing manual ID handling with the base class logic is a clean simplification. This prevents potential duplication of message-tracking code.
82-88: Behavior change indisconnect.
Removing all listeners, then emitting'disconnected'in the base class might differ from old behavior. Ensure consumers expecting an event before disposal handle these changes gracefully.
92-103: Extended onMessage logic.
This approach—emitting'firmwares'for'client'responses, otherwise checking'success'—looks clean. Just be sure you handle unexpected message formats with robust fallback logic if needed.packages/websocket-client/src/client.ts (5)
66-79: Keep an eye on repeatedsetPingTimeout.
Each message triggerssetPingTimeoutagain. This is acceptable if you want to reset the ping timer on every outgoing message/ping, but verify it meets your interval-liveness requirements.
160-178: Connection timeout is handled well.
Emitting'error'and closing the socket if connection times out ensures the caller can detect and handle slow/failed connections. Nicely done.
205-213:init()lifecycle is straightforward.
Removing all listeners before adding clean, minimal ones helps avoid stale event handlers. The approach is neat and avoids memory leaks.
230-235: Rejecting all messages upon abnormal close.
WhenonCloseis called, forcibly rejecting all pending messages prevents orphaned Promises. This is a good pattern to ensure the calling code knows the WebSocket is no longer valid.
237-242:dispose()gracefully tears down.
Invokingdisconnect()plusonClose()ensures full cleanup. This final call toremoveAllListeners()helps avoid unintentional memory retention.CODEOWNERS (1)
96-97: LGTM!The ownership assignment for the new websocket-client package is appropriate, assigning it to team members who already maintain related packages.
packages/blockchain-link/src/workers/blockfrost/websocket.ts (1)
Line range hint
24-31: LGTM! Clean WebSocket initialization with proper headers.The WebSocket initialization is well-structured with appropriate headers and agent configuration. The use of
getSuiteVersion()for the User-Agent header is a nice touch for debugging and tracking.However, let's verify that all consumers of this API are ready for the new initialization method:
✅ Verification successful
The WebSocket initialization is properly implemented using the shared package
The implementation in blockfrost/websocket.ts correctly uses the shared WebSocket client package, following the same pattern as other implementations in the codebase (e.g., blockbook worker). All other WebSocket usages in the codebase are unrelated to blockfrost and don't require updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct WebSocket usage that might need updating rg -l "new WebSocket\(" --type tsLength of output: 421
Script:
#!/bin/bash # Search for blockfrost-related WebSocket files and their content rg -l "blockfrost.*websocket" --type ts echo "=== Detailed content of found files ===" rg -l "blockfrost.*websocket" --type ts | xargs rg "WebSocket|initWebsocket" -C 2Length of output: 27358
packages/blockchain-link/src/workers/blockbook/websocket.ts (1)
Line range hint
49-56: LGTM! Well-structured WebSocket initialization.The implementation properly handles:
- WebSocket configuration with appropriate headers
- User agent with Suite version
- Custom headers merging
packages/trezor-user-env-link/tsconfig.json (1)
4-7: LGTM! Proper TypeScript configuration.The reference to the websocket-client package is correctly added to support the new dependency.
packages/trezor-user-env-link/package.json (1)
13-15: LGTM! Dependencies properly updated.The changes correctly:
- Add the new WebSocket client package
- Remove the deprecated ws dependencies
- Update other dependencies
Also applies to: 18-18
packages/blockchain-link/tsconfig.json (1)
15-15: LGTM! TypeScript project reference correctly added.The addition of the websocket-client package reference is properly configured and maintains the alphabetical ordering in the references array.
packages/blockchain-link/tsconfig.lib.json (1)
25-27: LGTM! Library build configuration properly updated.The websocket-client reference is correctly added with consistent formatting and structure.
packages/blockchain-link/package.json (2)
84-88: LGTM! Dependencies properly updated for WebSocket client migration.The changes correctly replace the "ws" dependency with the new "@trezor/websocket-client" workspace package.
29-45: Verify platform-specific configurations after WebSocket client migration.The removal of "ws" mappings from browser and react-native configurations needs verification to ensure it doesn't break platform-specific functionality.
Run the following script to check for any remaining references to the 'ws' module:
✅ Verification successful
Platform-specific configurations are properly maintained
The removal of "ws" mappings is safe as the codebase has dedicated WebSocket implementations for each platform:
- Browser environments use the native WebSocket API
- Node.js environments use the appropriate WebSocket implementation
- The socks-proxy-agent mappings are correctly maintained for both platforms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the 'ws' module that might affect platform-specific builds # Search for 'ws' references in source files echo "Checking for 'ws' references in source files:" rg -l 'require\(["\']ws["\']\)' || rg -l 'from\s+["\']ws["\']\)' # Check for 'ws' in import/export statements echo "Checking for 'ws' in import/export statements:" ast-grep --pattern 'import { $$$ } from "ws"' || ast-grep --pattern 'import "ws"'Length of output: 224
Script:
#!/bin/bash # Search for any 'ws' module references echo "=== Checking for 'ws' module references ===" rg -l "require\(['\"]ws['\"]\)|from\s+['\"]ws['\"]" # Search for WebSocket-related implementations echo -e "\n=== Checking WebSocket-related files ===" rg -l "WebSocket|websocket" --type ts --type js # Check platform-specific imports/conditions echo -e "\n=== Checking platform-specific conditions ===" rg -l "process\.browser|isomorphic|platform|typeof window" --type ts --type jsLength of output: 7067
suite-native/app/e2e/utils.ts (1)
122-122: Improved disconnection sequence to address test synchronization.Moving the disconnect call after stopBridge appears to address the Android e2e test timing issues mentioned in the PR comments. This ensures proper cleanup sequence: stopEmu -> stopBridge -> disconnect.
Let's verify the impact on test execution times:
✅ Verification successful
Verified: Disconnect sequence follows proper cleanup order.
The sequence
stopEmu -> stopBridge -> disconnectis consistently implemented across test files and follows a logical cleanup order:
- Stop emulator first to prevent device communication issues
- Stop bridge to ensure clean transport shutdown
- Disconnect for final cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze test execution patterns to verify timing improvements # Search for test files that use disconnectTrezorUserEnv echo "Finding affected test files:" rg -l "disconnectTrezorUserEnv" --type ts # Look for any timing-related test configurations echo "Checking for test timeouts and timing configurations:" rg -A 2 "withTimeout|setTimeout|waitFor" --type tsLength of output: 66859
Script:
#!/bin/bash # Find TrezorUserEnvLink implementation and disconnect sequence echo "Looking for TrezorUserEnvLink implementation:" rg -A 5 "class TrezorUserEnvLink" --type ts echo -e "\nLooking for disconnect sequence implementation:" rg -A 5 "disconnect\(\)" --type ts echo -e "\nLooking for stopBridge and stopEmu sequence:" rg -A 5 "stopBridge|stopEmu" --type tsLength of output: 75459
szymonlesisz
force-pushed
the
feat/websocket-package
branch
from
715ea47 to
039a639
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/blockchain-link/src/workers/baseWebsocket.ts (1)
38-41: Consider preserving the original error stack trace.When wrapping errors, it's good practice to preserve the original error information.
protected sendMessage(message: WebsocketRequest) { return super.sendMessage(message).catch(error => { - throw new CustomError(error.message); + throw new CustomError(error.message, { cause: error }); }); }packages/trezor-user-env-link/src/websocket-client.ts (1)
92-103: Consider adding type guards for response validation.The type checking could be more robust using TypeScript type guards.
protected onMessage(message: WebsocketResponseData) { super.onMessage(message, resp => { + interface ClientResponse { + type: 'client'; + firmwares: WebsocketClientEvents['firmwares']; + } + + function isClientResponse(resp: any): resp is ClientResponse { + return resp?.type === 'client' && 'firmwares' in resp; + } + - if (resp.type === 'client') { + if (isClientResponse(resp)) { this.emit('firmwares', resp.firmwares); } else { if (!resp.success) { throw new Error(`websocket_error_message: ${resp.error.message || resp.error}`); } } return resp; }); }packages/websocket-client/tests/client.test.ts (2)
34-55: Improve server response handling.The current implementation has several areas for improvement:
- The response validation could be more robust
- The method checks could be combined into a switch statement
- The error case could include more details
private sendResponse(client: WebSocket, data: any) { const request = JSON.parse(data); const { id, method } = request; - let response; - - if (method === 'init') { - response = { success: true }; - } - - if (method === 'ping') { - response = { success: true }; - } - - if (!response) { - response = { - success: false, - error: { message: `unknown response for method ${method}` }, - }; - } + const response = (() => { + switch (method) { + case 'init': + case 'ping': + return { success: true }; + default: + return { + success: false, + error: { + code: 'METHOD_NOT_FOUND', + message: `Unknown method: ${method}`, + details: request + } + }; + } + })(); client.send(JSON.stringify({ ...response, id })); }
117-128: Add test for concurrent reconnection attempts.While the current test covers basic reconnection, it would be valuable to test concurrent reconnection attempts.
it('handles concurrent reconnection attempts', async () => { const cli = new Client({ url: server.getUrl() }); await cli.connect(); // Trigger multiple reconnections concurrently const reconnectPromises = Array(3).fill(0).map(async () => { cli.disconnect(); // sync await cli.connect(); }); await Promise.all(reconnectPromises); const resp = await cli.sendMessage({ method: 'init' }); expect(resp.success).toEqual(true); await cli.disconnect(); });packages/websocket-client/src/client.ts (4)
126-129: Consider usingundefinedinstead ofvoidin the union type.The return type of
messageValidationusesvoidin a union type which can be confusing. Consider usingundefinedfor better type clarity.protected onMessage( message: WebsocketResponse, - messageValidation?: (data: Record<string, any>) => Record<string, any> | void, + messageValidation?: (data: Record<string, any>) => Record<string, any> | undefined, ) {🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
54-64: Enhance URL validation for better security.The current URL validation could be more robust. Consider:
- Validating URL format using URL constructor
- Checking for valid WebSocket protocols (ws/wss)
protected initWebsocket({ url, timeout, headers, agent }: WebsocketOptions) { // url validation if (typeof url !== 'string') { throw new Error('websocket_no_url'); } + try { + const parsedUrl = new URL(url); + if (!['ws:', 'wss:', 'http:', 'https:'].includes(parsedUrl.protocol)) { + throw new Error('websocket_invalid_protocol'); + } + } catch { + throw new Error('websocket_invalid_url'); + } + if (url.startsWith('http')) { url = url.replace('http', 'ws'); } return new WebSocket(url, { timeout, headers, agent }); }
141-143: Add error logging in catch block.Empty catch blocks can hide important errors. Consider logging the error or emitting an event.
- } catch { - // empty + } catch (error) { + this.emitter.emit('error', 'Failed to parse message'); }
122-126: Consider implementing generic type for message data.The TODO comment suggests adding generic type support for message data, which would improve type safety.
Would you like me to help implement the generic type support for message data? This would make the WebSocket client more type-safe when handling different message formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
CODEOWNERS(1 hunks)packages/blockchain-link/jest.config.unit.js(0 hunks)packages/blockchain-link/package.json(2 hunks)packages/blockchain-link/src/workers/baseWebsocket.ts(1 hunks)packages/blockchain-link/src/workers/blockbook/websocket.ts(1 hunks)packages/blockchain-link/src/workers/blockfrost/websocket.ts(1 hunks)packages/blockchain-link/tsconfig.json(1 hunks)packages/blockchain-link/tsconfig.lib.json(1 hunks)packages/blockchain-link/webpack/dev.js(0 hunks)packages/blockchain-link/webpack/workers.web.js(0 hunks)packages/connect/e2e/karma.config.js(0 hunks)packages/trezor-user-env-link/package.json(1 hunks)packages/trezor-user-env-link/src/websocket-client.ts(4 hunks)packages/trezor-user-env-link/tsconfig.json(1 hunks)packages/websocket-client/README.md(1 hunks)packages/websocket-client/package.json(1 hunks)packages/websocket-client/src/client.ts(1 hunks)packages/websocket-client/src/index.ts(1 hunks)packages/websocket-client/src/ws-browser.ts(2 hunks)packages/websocket-client/src/ws-native.ts(2 hunks)packages/websocket-client/tests/client.test.ts(1 hunks)packages/websocket-client/tsconfig.json(1 hunks)packages/websocket-client/tsconfig.lib.json(1 hunks)suite-native/app/e2e/utils.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/connect/e2e/karma.config.js
- packages/blockchain-link/jest.config.unit.js
- packages/blockchain-link/webpack/dev.js
- packages/blockchain-link/webpack/workers.web.js
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/websocket-client/tsconfig.json
- packages/blockchain-link/tsconfig.json
- packages/websocket-client/src/index.ts
- packages/blockchain-link/tsconfig.lib.json
- packages/trezor-user-env-link/tsconfig.json
- packages/websocket-client/tsconfig.lib.json
- suite-native/app/e2e/utils.ts
- CODEOWNERS
- packages/blockchain-link/src/workers/blockbook/websocket.ts
- packages/websocket-client/package.json
- packages/trezor-user-env-link/package.json
- packages/blockchain-link/src/workers/blockfrost/websocket.ts
- packages/websocket-client/README.md
- packages/websocket-client/src/ws-browser.ts
- packages/blockchain-link/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/websocket-client/src/client.ts
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Linting and formatting
- GitHub Check: Other Checks
- GitHub Check: Type Checking
- GitHub Check: Build libs for publishing
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- 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=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: transport-e2e-test
- 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: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (8)
packages/websocket-client/src/ws-native.ts (2)
49-51: LGTM! Good use of static constant.Using
WSWrapper.OPENinstead of a magic number improves code readability and maintainability.
55-57: LGTM! Good defensive programming.Adding a state check before sending messages prevents potential race conditions and provides clear error messages.
packages/blockchain-link/src/workers/baseWebsocket.ts (1)
22-36: LGTM! Good error handling and subscription management.The implementation properly handles:
- Error cases with CustomError
- Message resolution
- Subscription callbacks
packages/trezor-user-env-link/src/websocket-client.ts (2)
41-47: LGTM! Clean implementation of required methods.The implementation is straightforward and follows the base class contract.
82-89: Address the TODO comment about breaking change.The comment indicates a potential breaking change in the disconnect behavior. This should be documented and communicated to users.
Would you like me to help create a migration guide for this breaking change?
packages/websocket-client/tests/client.test.ts (1)
97-115: LGTM! Good use of Jest fake timers.The test properly sets up and cleans up fake timers, and verifies the ping behavior.
packages/websocket-client/src/client.ts (2)
5-18: LGTM! Well-structured type definitions with comprehensive options.The type definitions provide good type safety and extensibility. The separation between basic
WebsocketOptionsand extendedOptionsis clean and logical.
30-41: LGTM! Well-structured class with proper encapsulation.The class extends TypedEmitter appropriately and maintains good encapsulation of member variables with proper access modifiers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/websocket-client/src/client.ts (1)
111-120:⚠️ Potential issueFix binary message handling in sendRawMessage.
The binary flag is incorrectly set. It should be true for binary data (Buffer/ArrayBuffer), not for non-string data.
ws.send(message, { - binary: typeof message !== 'string', + binary: message instanceof Buffer || message instanceof ArrayBuffer, });
🧹 Nitpick comments (8)
packages/websocket-client/src/client.ts (3)
5-28: Add JSDoc comments to document type definitions.The type definitions are well-structured, but adding JSDoc comments would improve code maintainability by documenting the purpose and constraints of each option.
+/** + * Base WebSocket connection options + */ type WebsocketOptions = { + /** WebSocket server URL */ url: string; + /** Request timeout in milliseconds */ timeout?: number; + /** HTTP/HTTPS agent for proxy support */ agent?: WebSocket.ClientOptions['agent']; + /** Additional HTTP headers */ headers?: WebSocket.ClientOptions['headers']; };
54-64: Enhance URL validation.The current URL validation could be improved to handle more edge cases:
- Support for wss protocol
- Validation of malformed URLs
- Validation of URL format using URL constructor
protected initWebsocket({ url, timeout, headers, agent }: WebsocketOptions) { if (typeof url !== 'string') { throw new Error('websocket_no_url'); } + try { + const wsUrl = new URL(url); + if (wsUrl.protocol === 'http:') { + wsUrl.protocol = 'ws:'; + } else if (wsUrl.protocol === 'https:') { + wsUrl.protocol = 'wss:'; + } else if (!['ws:', 'wss:'].includes(wsUrl.protocol)) { + throw new Error('Invalid protocol'); + } + url = wsUrl.toString(); + } catch (error) { + throw new Error('Invalid WebSocket URL'); + } return new WebSocket(url, { timeout, headers, agent }); }
148-197: Improve connection cleanup on timeout.The connection timeout handler could leak resources if the WebSocket connection is in an intermediate state.
const connectionTimeout = setTimeout( () => { ws.emit('error', new Error('websocket_timeout')); try { ws.once('error', () => {}); ws.close(); + ws.removeAllListeners(); } catch { // empty } }, this.options.connectionTimeout || this.options.timeout || DEFAULT_TIMEOUT, );packages/blockchain-link/src/workers/baseWebsocket.ts (2)
24-38: Enhance error handling in onMessage.The error handling could be improved by:
- Adding specific error types
- Logging errors for debugging
- Handling message parsing errors
protected onMessage(message: WebsocketResponse) { super.onMessage(message, ({ id, data }) => { if (data.error) { - throw new CustomError('websocket_error_message', data.error.message); + const errorMessage = data.error.message || 'Unknown error'; + const errorCode = data.error.code || 'websocket_error_message'; + throw new CustomError(errorCode, errorMessage); } const messageSettled = this.messages.resolve(Number(id), data); if (!messageSettled) { const subs = this.subscriptions.find(s => s.id === id); if (subs) { subs.callback(data); } } }); }
40-43: Improve error handling in sendMessage.The error handling in sendMessage could be improved by preserving the original error stack trace.
protected sendMessage(message: WebsocketRequest) { return super.sendMessage(message).catch(error => { - throw new CustomError(error.message); + throw new CustomError('websocket_send_error', error.message, { cause: error }); }); }packages/trezor-user-env-link/src/websocket-client.ts (3)
Line range hint
13-23: Document timeout configuration rationale.The high timeout values and IP address configuration should be documented to explain:
- Why 5-minute timeout is necessary for emulator actions
- Why localhost resolution needs the IP6 workaround
-// Making the timeout high because the controller in trezor-user-env -// must synchronously run actions on emulator and they may take a long time -// (for example in case of Shamir backup) +/** + * Extended timeout (5 minutes) is required because: + * 1. The controller in trezor-user-env runs actions synchronously + * 2. Some operations like Shamir backup can take several minutes + * 3. Emulator operations may be slower than real device operations + */ const DEFAULT_TIMEOUT = 5 * 60 * 1000; const DEFAULT_PING_TIMEOUT = 50 * 1000; -// breaking change in node 17, ip6 is preferred by default -// localhost is not resolved correctly on certain machines +/** + * Using explicit IPv4 address because: + * 1. Node 17+ prefers IPv6 by default + * 2. localhost resolution fails on some machines + * 3. Ensures consistent connection behavior across environments + */ const USER_ENV_URL = {
94-105: Improve error handling in onMessage.The error handling could be enhanced by:
- Adding specific error types for different failure scenarios
- Including more context in error messages
protected onMessage(message: WebsocketResponseData) { super.onMessage(message, resp => { if (resp.type === 'client') { this.emit('firmwares', resp.firmwares); } else { if (!resp.success) { - throw new Error(`websocket_error_message: ${resp.error.message || resp.error}`); + const context = resp.type ? ` (type: ${resp.type})` : ''; + const errorMessage = resp.error.message || resp.error; + throw new Error(`websocket_error_message: ${errorMessage}${context}`); } } return resp; }); }
Line range hint
107-143: Improve waitForTrezorUserEnv implementation.The current implementation has several areas for improvement:
- Magic numbers should be constants
- Error handling could be more informative
- Progress logging could be more consistent
+const WAIT_TIMEOUT_SECONDS = 300; +const WAIT_INTERVAL_MS = 1000; + async waitForTrezorUserEnv() { - const limit = 300; let error = ''; console.log('waiting for trezor-user-env'); - for (let i = 0; i < limit; i++) { - if (i === limit - 1) { + for (let i = 0; i < WAIT_TIMEOUT_SECONDS; i++) { + if (i === WAIT_TIMEOUT_SECONDS - 1) { console.log(`cant connect to trezor-user-env: ${error}\n`); } - await delay(1000); + await delay(WAIT_INTERVAL_MS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/blockchain-link/src/workers/baseWebsocket.ts(1 hunks)packages/trezor-user-env-link/src/websocket-client.ts(4 hunks)packages/websocket-client/src/client.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/websocket-client/src/client.ts
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
🔇 Additional comments (2)
packages/websocket-client/src/client.ts (1)
45-52: Consider edge cases in timeout handling.The message manager's timeout could potentially conflict with the connection timeout. Consider adding a mechanism to prevent race conditions between message timeouts and connection timeouts.
packages/trezor-user-env-link/src/websocket-client.ts (1)
82-91: Address TODO comment in disconnect method.The TODO comment indicates a breaking change in disconnect behavior. This needs to be addressed and documented properly.
✅ Verification successful
TODO comment can be safely removed
The current implementation correctly handles the breaking change by:
- Only disconnecting when actually connected
- Properly cleaning up listeners before disconnection
- Maintaining consistent Promise-based API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for disconnect usage patterns rg "\.disconnect\(\)" --type ts -A 2Length of output: 14809
Script:
#!/bin/bash # Find the base class implementation ast-grep --pattern 'class $_ { $$$ disconnect() { $$$ } $$$ }'Length of output: 26680
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13060159865 |
trezor-ci
force-pushed
the
feat/websocket-package
branch
from
8e01f39 to
4678201
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/trezor-user-env-link/src/websocket-client.ts (3)
49-49: Consider using a more specific type for options.Using
anytype reduces type safety. Consider creating a dedicated interface for the options.- constructor(options: any = {}) { + interface WebsocketClientOptions { + url?: string; + timeout?: number; + pingTimeout?: number; + [key: string]: unknown; + } + constructor(options: WebsocketClientOptions = {}) {
94-105: Consider enhancing error handling.The current error handling could be improved by:
- Using custom error classes
- Extracting the success check logic
+class WebsocketResponseError extends Error { + constructor(message: string) { + super(message); + this.name = 'WebsocketResponseError'; + } +} +private isSuccessResponse(resp: WebsocketResponse): boolean { + return resp.success === true; +} protected onMessage(message: WebsocketResponseData) { super.onMessage(message, resp => { if (resp.type === 'client') { this.emit('firmwares', resp.firmwares); } else { - if (!resp.success) { - throw new Error(`websocket_error_message: ${resp.error.message || resp.error}`); + if (!this.isSuccessResponse(resp)) { + throw new WebsocketResponseError(`websocket_error_message: ${resp.error.message || resp.error}`); } } return resp; }); }
Line range hint
108-143: Enhance waitForTrezorUserEnv implementation.Consider the following improvements:
- Make retry limit configurable
- Replace console.log with proper logging
- Add exponential backoff for retries
Would you like me to propose a refactored implementation with these improvements?
packages/websocket-client/src/client.ts (4)
19-20: Document timeout constants.Consider adding JSDoc comments to explain the purpose and implications of these timeout values.
+/** Default timeout for individual WebSocket operations in milliseconds */ const DEFAULT_TIMEOUT = 20 * 1000; +/** Default ping interval in milliseconds to keep the connection alive */ const DEFAULT_PING_TIMEOUT = 50 * 1000;
54-64: Enhance URL validation.The current URL validation could be more robust by:
- Validating the protocol (ws/wss)
- Ensuring the URL is well-formed
protected initWebsocket({ url, timeout, headers, agent }: WebsocketOptions) { // url validation if (typeof url !== 'string') { throw new Error('websocket_no_url'); } + // Validate protocol + if (!url.match(/^(ws|wss|http|https):\/\/.+/)) { + throw new Error('websocket_invalid_protocol'); + } if (url.startsWith('http')) { url = url.replace('http', 'ws'); }
115-117: Improve binary message type check.The current binary flag check could be more explicit about the types of binary data.
ws.send(message, { - binary: typeof message !== 'string', + binary: message instanceof Buffer || message instanceof ArrayBuffer || message instanceof Uint8Array, });
169-169: Enhance connection timeout error message.Consider providing more context in the timeout error message.
-ws.emit('error', new Error('websocket_timeout')); +ws.emit('error', new Error(`websocket_connection_timeout: Failed to establish connection to ${this.options.url} within ${this.options.connectionTimeout || this.options.timeout || DEFAULT_TIMEOUT}ms`));packages/websocket-client/tests/client.test.ts (2)
34-55: Add error handling to sendResponse.The server's message handling should include try-catch blocks for JSON parsing and message processing.
private sendResponse(client: WebSocket, data: any) { + try { const request = JSON.parse(data); const { id, method } = request; let response; if (method === 'init') { response = { success: true }; } if (method === 'ping') { response = { success: true }; } if (!response) { response = { success: false, error: { message: `unknown response for method ${method}` }, }; } client.send(JSON.stringify({ ...response, id })); + } catch (error) { + client.send(JSON.stringify({ + success: false, + error: { message: 'Invalid message format' }, + })); + } }
69-164: Add test cases for edge scenarios.Consider adding tests for:
- Handling malformed messages
- Connection with invalid headers
- Reconnection with backoff
- Maximum reconnection attempts
Would you like me to generate the additional test cases?
packages/blockchain-link/src/workers/baseWebsocket.ts (1)
24-38: Consider adding message validation types.The onMessage implementation could benefit from stronger typing for the message validation callback.
+type MessageValidator<T> = (data: { id: string; data: any }) => T; protected onMessage(message: WebsocketResponse) { - super.onMessage(message, ({ id, data }) => { + super.onMessage(message, (({ id, data }): MessageValidator<T[keyof T]> => { if (data.error) { throw new CustomError('websocket_error_message', data.error.message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
CODEOWNERS(1 hunks)packages/blockchain-link/jest.config.unit.js(0 hunks)packages/blockchain-link/package.json(2 hunks)packages/blockchain-link/src/workers/baseWebsocket.ts(1 hunks)packages/blockchain-link/src/workers/blockbook/websocket.ts(1 hunks)packages/blockchain-link/src/workers/blockfrost/websocket.ts(1 hunks)packages/blockchain-link/tsconfig.json(1 hunks)packages/blockchain-link/tsconfig.lib.json(1 hunks)packages/blockchain-link/webpack/dev.js(0 hunks)packages/blockchain-link/webpack/workers.web.js(0 hunks)packages/connect/e2e/karma.config.js(0 hunks)packages/trezor-user-env-link/package.json(1 hunks)packages/trezor-user-env-link/src/websocket-client.ts(4 hunks)packages/trezor-user-env-link/tsconfig.json(1 hunks)packages/websocket-client/README.md(1 hunks)packages/websocket-client/package.json(1 hunks)packages/websocket-client/src/client.ts(1 hunks)packages/websocket-client/src/index.ts(1 hunks)packages/websocket-client/src/ws-browser.ts(2 hunks)packages/websocket-client/src/ws-native.ts(2 hunks)packages/websocket-client/tests/client.test.ts(1 hunks)packages/websocket-client/tsconfig.json(1 hunks)packages/websocket-client/tsconfig.lib.json(1 hunks)suite-native/app/e2e/utils.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/blockchain-link/jest.config.unit.js
- packages/blockchain-link/webpack/dev.js
- packages/blockchain-link/webpack/workers.web.js
- packages/connect/e2e/karma.config.js
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/blockchain-link/tsconfig.lib.json
- packages/blockchain-link/tsconfig.json
- CODEOWNERS
- packages/websocket-client/src/index.ts
- suite-native/app/e2e/utils.ts
- packages/trezor-user-env-link/tsconfig.json
- packages/websocket-client/tsconfig.json
- packages/websocket-client/tsconfig.lib.json
- packages/blockchain-link/src/workers/blockfrost/websocket.ts
- packages/websocket-client/src/ws-native.ts
- packages/websocket-client/README.md
- packages/websocket-client/src/ws-browser.ts
- packages/blockchain-link/src/workers/blockbook/websocket.ts
- packages/websocket-client/package.json
- packages/trezor-user-env-link/package.json
- packages/blockchain-link/package.json
🧰 Additional context used
📓 Learnings (1)
packages/blockchain-link/src/workers/baseWebsocket.ts (1)
Learnt from: szymonlesisz
PR: trezor/trezor-suite#15797
File: packages/blockchain-link/src/workers/baseWebsocket.ts:10-12
Timestamp: 2025-01-30T19:51:50.844Z
Learning: BaseWebsocket instances in @trezor/blockchain-link are designed to be disposed after onClose, making explicit cleanup of subscriptions unnecessary as they will be handled by garbage collection.
🪛 Biome (1.9.4)
packages/websocket-client/src/client.ts
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- 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 / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (8)
packages/trezor-user-env-link/src/websocket-client.ts (4)
5-8: LGTM! Imports align with the PR objectives.The imports from
@trezor/websocket-clientcorrectly implement the shared package strategy.
40-46: LGTM! Clean implementation of base class methods.The
createWebsocketandpingmethods provide clear, focused implementations of the base class requirements.
Line range hint
66-80: Consider removing the karma workaround.The comment indicates this is temporary due to planned karma removal. Consider creating a tracking issue for proper CORS headers in the trezor-user-env server.
Would you like me to create an issue to track:
- Adding proper CORS headers in trezor-user-env server
- Removing the karma workaround after the migration?
82-91: Address the breaking change in disconnect method.The TODO comment indicates a significant behavioral change where
disconnectno longer acts likedispose. This breaking change should be:
- Documented in the migration guide
- Communicated to dependent packages
- Potentially versioned as a major release
Let's verify the impact:
✅ Verification successful
Breaking change in disconnect method appears to be properly managed
The codebase analysis shows that the behavioral change in
disconnect()is well-tested and properly handled across the codebase. Most usages are in test scenarios or cleanup contexts where the behavior is explicitly verified.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls to disconnect() to identify affected code rg -A 2 "\.disconnect\(\)" --type tsLength of output: 14809
packages/websocket-client/src/client.ts (2)
30-52: LGTM! Well-structured class initialization.The class is well-designed with proper type safety and initialization.
128-128: LGTM! Correct usage of void type.The void type in the union is intentional here, representing callbacks that don't return a value. Using undefined instead (as suggested by static analysis) would be semantically incorrect.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/blockchain-link/src/workers/baseWebsocket.ts (2)
13-22: LGTM! Proper ping handling with error management.The onPing implementation correctly handles both subscription-based and keepAlive scenarios with proper error handling.
40-43: LGTM! Proper error handling in sendMessage.The implementation correctly wraps errors in CustomError and aligns with the learning about instance lifecycle and garbage collection.
|
one last fixup fcd363d |
and another one from me :) 9a62fc2
tested (by CI) more than i wished :) |
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13079910110 |
trezor-ci
force-pushed
the
feat/websocket-package
branch
from
9a62fc2 to
c4af911
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/websocket-client/src/client.ts (1)
126-129: Replace void with undefined in union type.The use of void in the union type can be confusing. Using undefined is more appropriate in this context.
protected onMessage( message: WebsocketResponse, - messageValidation?: (data: Record<string, any>) => Record<string, any> | void, + messageValidation?: (data: Record<string, any>) => Record<string, any> | undefined, ) {🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/trezor-user-env-link/src/websocket-client.ts (2)
49-49: Replaceanytype with a proper interface.Using
anytype reduces type safety. Consider creating a proper interface for the options parameter.-constructor(options: any = {}) { +interface WebsocketClientOptions { + url?: string; + timeout?: number; + pingTimeout?: number; + [key: string]: any; // for other options +} +constructor(options: WebsocketClientOptions = {}) {
94-105: Enhance error handling with specific error types.The error handling could be improved by using specific error types instead of generic Error class.
+class WebsocketResponseError extends Error { + constructor(message: string) { + super(message); + this.name = 'WebsocketResponseError'; + } +} -throw new Error(`websocket_error_message: ${resp.error.message || resp.error}`); +throw new WebsocketResponseError(`websocket_error_message: ${resp.error.message || resp.error}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
CODEOWNERS(1 hunks)packages/blockchain-link/jest.config.unit.js(0 hunks)packages/blockchain-link/package.json(2 hunks)packages/blockchain-link/src/workers/baseWebsocket.ts(1 hunks)packages/blockchain-link/src/workers/blockbook/websocket.ts(1 hunks)packages/blockchain-link/src/workers/blockfrost/websocket.ts(1 hunks)packages/blockchain-link/tsconfig.json(1 hunks)packages/blockchain-link/tsconfig.lib.json(1 hunks)packages/blockchain-link/webpack/dev.js(0 hunks)packages/blockchain-link/webpack/workers.web.js(0 hunks)packages/connect/e2e/karma.config.js(0 hunks)packages/trezor-user-env-link/package.json(1 hunks)packages/trezor-user-env-link/src/websocket-client.ts(4 hunks)packages/trezor-user-env-link/tsconfig.json(1 hunks)packages/websocket-client/README.md(1 hunks)packages/websocket-client/package.json(1 hunks)packages/websocket-client/src/client.ts(1 hunks)packages/websocket-client/src/index.ts(1 hunks)packages/websocket-client/src/ws-browser.ts(2 hunks)packages/websocket-client/src/ws-native.ts(2 hunks)packages/websocket-client/tests/client.test.ts(1 hunks)packages/websocket-client/tsconfig.json(1 hunks)packages/websocket-client/tsconfig.lib.json(1 hunks)suite-native/app/e2e/utils.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/blockchain-link/jest.config.unit.js
- packages/blockchain-link/webpack/dev.js
- packages/connect/e2e/karma.config.js
- packages/blockchain-link/webpack/workers.web.js
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/blockchain-link/tsconfig.lib.json
- CODEOWNERS
- packages/blockchain-link/tsconfig.json
- packages/websocket-client/src/index.ts
- packages/websocket-client/tsconfig.lib.json
- packages/websocket-client/tsconfig.json
- packages/trezor-user-env-link/tsconfig.json
- packages/websocket-client/README.md
- packages/blockchain-link/src/workers/blockbook/websocket.ts
- suite-native/app/e2e/utils.ts
- packages/blockchain-link/src/workers/blockfrost/websocket.ts
- packages/websocket-client/src/ws-browser.ts
- packages/trezor-user-env-link/package.json
- packages/websocket-client/src/ws-native.ts
- packages/websocket-client/package.json
- packages/blockchain-link/package.json
- packages/websocket-client/tests/client.test.ts
🧰 Additional context used
📓 Learnings (1)
packages/blockchain-link/src/workers/baseWebsocket.ts (1)
Learnt from: szymonlesisz
PR: trezor/trezor-suite#15797
File: packages/blockchain-link/src/workers/baseWebsocket.ts:10-12
Timestamp: 2025-01-30T19:51:50.844Z
Learning: BaseWebsocket instances in @trezor/blockchain-link are designed to be disposed after onClose, making explicit cleanup of subscriptions unnecessary as they will be handled by garbage collection.
🪛 Biome (1.9.4)
packages/websocket-client/src/client.ts
[error] 128-128: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (10)
packages/blockchain-link/src/workers/baseWebsocket.ts (3)
13-22: LGTM! Robust ping handling implementation.The implementation correctly handles ping failures and disconnection scenarios based on subscription state and keepAlive flag.
24-38: LGTM! Well-structured message handling.The implementation correctly:
- Validates and routes messages
- Handles errors appropriately
- Manages subscriptions
40-43: LGTM! Clean error handling.The implementation properly wraps WebSocket errors in domain-specific CustomError.
packages/websocket-client/src/client.ts (5)
5-28: LGTM! Well-structured type definitions.The types and constants are well-defined with reasonable defaults.
95-109: LGTM! Robust message sending implementation.The implementation properly handles message IDs, timeouts, and callbacks.
111-120: LGTM! Correct binary message handling.The implementation properly sets the binary flag based on message type.
148-230: LGTM! Comprehensive connection management.The implementation:
- Handles connection timeouts
- Manages connection states properly
- Includes proper error handling
- Handles edge cases like already connecting state
232-244: LGTM! Thorough cleanup implementation.The implementation properly cleans up resources and handles pending operations.
packages/trezor-user-env-link/src/websocket-client.ts (2)
40-55: LGTM! Well-structured class inheritance.The class extension is well-implemented with proper type parameters and default configurations. The protected methods provide good extension points for customization.
Line range hint
66-80: Consider removing karma workaround.The workaround for karma is noted as temporary since karma will be removed. Track this technical debt to ensure it's addressed when karma is removed.
Would you like me to create an issue to track the removal of this workaround when karma is deprecated?
Description
Side quest from bluetooth
Create and use shared websocket package.
This is basically code moved from
blockchain-link/baseWebsocketfile to standalone package, so the logic behind connection/ping/timeouts and sending/receiving messages can be used elsewhereThis should be used in:
blockchain-link(in this PR)trezor-user-env-link(in this PR)transport-bluetooth(in bluetooth branch )Additional benefit:
webpack configs doesnt need to use
webpack.NormalModuleReplacementPluginonwsmoduleAdditional fixes: