chore(transport-bridge): simplify dist structure, inline html #17465
Conversation
WalkthroughThis pull request introduces several configuration and code adjustments. The webpack configuration for the desktop build has been modified by removing a file-copying pattern for UI assets from the transport bridge, thereby changing how these assets are handled during the build. Updates in the transport-bridge package's build scripts now include the processing of HTML files as text and adjust the build order by invoking the UI build prior to the Node.js components, followed by the deletion of a specific directory. In the transport-bridge HTTP endpoint, file system operations have been replaced with a direct import of HTML content, simplifying the response handling. Additionally, tests have been updated to change the import source of a node component and introduce a type declaration with error suppression for asynchronous bridge operations. Finally, the CI workflow has been revised to include the installation and build steps for the transport-bridge package before executing end-to-end tests. ✨ 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 (
|
b3da4b4 to
4704116
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/suite-build/configs/desktop.webpack.config.ts(0 hunks)packages/transport-bridge/package.json(1 hunks)packages/transport-bridge/src/http.ts(2 hunks)packages/transport-test/e2e/bridge/controller.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/suite-build/configs/desktop.webpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/transport-bridge/package.json
- packages/transport-bridge/src/http.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/transport-test/e2e/bridge/controller.ts
[error] 14-14: Shouldn't redeclare 'TrezordNode'. Consider to delete it or rename it.
'TrezordNode' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: build-web
- 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: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- 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: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (1)
packages/transport-test/e2e/bridge/controller.ts (1)
5-5: Update import path to use built output.This change correctly updates the import path to use the compiled output from
dist/index.jsinstead of directly importing from the source, which aligns with the PR objective to simplify the dist structure.
| @@ -11,6 +11,8 @@ export const env = { | |||
| USE_NODE_BRIDGE: process.env.USE_NODE_BRIDGE === 'true', | |||
| }; | |||
|
|
|||
| type TrezordNode = any; | |||
There was a problem hiding this comment.
Remove redundant type redeclaration.
The TrezordNode type is already imported on line 5, and redeclaring it as any causes a conflict. This redeclaration can lead to type checking issues and should be removed.
-type TrezordNode = any;📝 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.
| type TrezordNode = any; | |
| // (Assuming the beginning of the file includes the import on line 5) | |
| // ... other content above ... | |
| // Removed the redundant TrezordNode type declaration | |
| // ... rest of the file remains unchanged ... |
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Shouldn't redeclare 'TrezordNode'. Consider to delete it or rename it.
'TrezordNode' is defined here:
(lint/suspicious/noRedeclare)
| @@ -55,6 +57,7 @@ | |||
| this.originalApi.startBridge(version) | |||
| : env.USE_NODE_BRIDGE | |||
| ? async () => { | |||
| // @ts-expect-error | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid TypeScript error suppression.
Using // @ts-expect-error suggests there might be compatibility issues with the imported TrezordNode type. Consider properly typing the interface instead of suppressing the error.
🏁 Script executed:
#!/bin/bash
# Check the TrezordNode type definition in the imported file
cat packages/transport-bridge/dist/index.js | grep -A 10 "TrezordNode"
# Check how TrezordNode is used elsewhere in the codebase
rg "TrezordNode" --type ts -A 2 -B 2Length of output: 13590
Action Required: Remove TS Error Suppression and Fix TrezordNode Typing
The use of // @ts-expect-error in packages/transport-test/e2e/bridge/controller.ts indicates that the current typing of TrezordNode is being bypassed (in part due to the in-file declaration type TrezordNode = any). This workaround not only reduces type safety but also deviates from how TrezordNode is used in other parts of the repository. Please address the following:
- Remove the error suppression: Eliminate the
// @ts-expect-errordirective above the instantiation ofTrezordNode. - Align type definitions: Import and use the properly declared
TrezordNodefrom@trezor/transport-bridge(likely from its source module, e.g.packages/transport-bridge/src/http.ts) instead of redefining it asanyin the test file. - Ensure consistency: Update the interface and usage of
TrezordNodeso that it reflects the correct types without needing to bypass TypeScript’s checks.
625a189 to
80af2ef
Compare
80af2ef to
bc5cbb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/transport-test/e2e/bridge/controller.ts (2)
14-14:⚠️ Potential issueRemove redundant type redeclaration.
The
TrezordNodetype is already imported on line 5, and redeclaring it asanycauses a conflict. This redeclaration can lead to type checking issues and should be removed.-type TrezordNode = any;🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Shouldn't redeclare 'TrezordNode'. Consider to delete it or rename it.
'TrezordNode' is defined here:
(lint/suspicious/noRedeclare)
60-60:⚠️ Potential issueRemove TS error suppression and fix type compatibility.
Using
// @ts-expect-errorsuppresses important type checking which can lead to runtime errors. Instead of bypassing TypeScript's type system, properly address the underlying type compatibility issue.- // @ts-expect-error this.nodeBridge = new TrezordNode({Consider importing
TrezordNodeproperly and ensuring type compatibility with how it's being used in this file.
📜 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 (5)
.github/workflows/test-transport.yml(1 hunks)packages/suite-build/configs/desktop.webpack.config.ts(0 hunks)packages/transport-bridge/package.json(2 hunks)packages/transport-bridge/src/http.ts(2 hunks)packages/transport-test/e2e/bridge/controller.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/suite-build/configs/desktop.webpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/transport-bridge/package.json
- packages/transport-bridge/src/http.ts
- .github/workflows/test-transport.yml
🧰 Additional context used
🪛 Biome (1.9.4)
packages/transport-test/e2e/bridge/controller.ts
[error] 14-14: Shouldn't redeclare 'TrezordNode'. Consider to delete it or rename it.
'TrezordNode' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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-web
- GitHub Check: Analyze with CodeQL (javascript)
- 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: run-desktop-tests (@group=passphrase, 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: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| import { WebUSB } from 'usb'; | |||
|
|
|||
| import { TrezordNode } from '@trezor/transport-bridge/src'; | |||
| import { TrezordNode } from '@trezor/transport-bridge/dist/index.js'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid importing directly from the dist folder.
Importing directly from the dist folder creates a tight coupling with the build output structure, which can lead to issues if the build process changes. Instead, import from the package's main entry point.
-import { TrezordNode } from '@trezor/transport-bridge/dist/index.js';
+import { TrezordNode } from '@trezor/transport-bridge';This approach provides better encapsulation and follows the principle of importing through the package's public API.
📝 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.
| import { TrezordNode } from '@trezor/transport-bridge/dist/index.js'; | |
| -import { TrezordNode } from '@trezor/transport-bridge/dist/index.js'; | |
| +import { TrezordNode } from '@trezor/transport-bridge'; |
|
🚀 Expo preview is ready!
|
bc5cbb3 to
4f7ca29
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/transport-bridge/src/http.ts (1)
411-415: Much cleaner implementation of the status endpoint.The endpoint handler is now more straightforward, using the pre-imported HTML content instead of reading from the filesystem at runtime. This eliminates potential file I/O errors and improves response time.
Consider adding a content encoding header if the HTML contains non-ASCII characters:
- res.writeHead(200, { 'Content-Type': 'text/html' }); + res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/suite-desktop-core/webpack/core.webpack.config.ts(1 hunks)packages/transport-bridge/package.json(1 hunks)packages/transport-bridge/src/http.ts(2 hunks)packages/transport-bridge/tsconfig.lib.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/transport-bridge/tsconfig.lib.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/transport-bridge/package.json
⏰ Context from checks skipped due to timeout of 90000ms (18)
- 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: 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: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (2)
packages/suite-desktop-core/webpack/core.webpack.config.ts (1)
106-113: Nice addition of HTML loader configuration.Adding the raw-loader for HTML files allows direct importing of HTML content as strings in JavaScript/TypeScript modules, which simplifies handling of HTML templates in the codebase. This change complements the modifications in the transport-bridge package.
packages/transport-bridge/src/http.ts (1)
21-23:✅ Verification successful
Good simplification with inline HTML.
The approach of raw-loading HTML as a string is efficient for this use case. The TODO comment accurately describes the current build process and suggests a potential future improvement.
However, let's make sure the TypeScript configuration properly supports this pattern:
🏁 Script executed:
#!/bin/bash # Check if there are any TypeScript configurations that might need adjustment for HTML imports find . -name "tsconfig*.json" | xargs grep -l "allowJs\|resolveJsonModule\|moduleResolution" | xargs catLength of output: 5114
Simplification with inline HTML is well executed.
The code efficiently loads HTML as a string, and the TODO comment clearly explains the current two-phase build process and hints at a future consolidation. After reviewing the relevant tsconfig files, no adjustments are needed for HTML imports—the current use of
@ts-expect-erroris acceptable. That said, if you plan to remove the error suppression down the road, consider adding a module declaration (e.g., acustom.d.tswithdeclare module '*.html' { const content: string; export default content; }) to provide proper type support.
- File:
packages/transport-bridge/src/http.ts(lines 21-23)- Suggestion: Optionally add a declaration file for HTML imports if this approach becomes permanent.
7147672 to
0b219df
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/transport-test/e2e/bridge/controller.ts (1)
62-63: 🛠️ Refactor suggestionEliminate TypeScript error suppression.
Using
// @ts-ignorebypasses TypeScript's type checking, which reduces code quality and can hide potential issues. Instead, fix the underlying type compatibility problem.- // @ts-ignore - this.nodeBridge = new TrezordNode({ + this.nodeBridge = new TrezordNode({Consider creating proper type definitions for TrezordNode if they're missing or incomplete.
🧹 Nitpick comments (1)
packages/transport-bridge/src/http.ts (1)
20-22: Consider improving the HTML content import approach.The comment identifies that the build process has two phases (UI with webpack, JS with esbuild) and suggests it could be streamlined. The use of
// @ts-expect-errorto suppress type checking for HTML imports indicates a potential opportunity for improvement.Consider one of these approaches:
- Use a more TypeScript-friendly way to import HTML content, like creating a proper type declaration file for HTML imports
- Create a unified build process that handles both UI and JS in a single step
-// todo: the build process now has 2 phases, build:ui (using webpack) and build:js (using esbuild). Ui is then raw-loaded as a string. It would be probably better to streamline the process into a single step -// @ts-expect-error +// Import HTML content with proper type declaration +// This requires a .d.ts file with: declare module '*.html' { const content: string; export default content; }
📜 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 (7)
.github/workflows/test-transport.yml(1 hunks)packages/suite-build/configs/desktop.webpack.config.ts(0 hunks)packages/suite-desktop-core/webpack/core.webpack.config.ts(1 hunks)packages/transport-bridge/package.json(1 hunks)packages/transport-bridge/src/http.ts(2 hunks)packages/transport-bridge/tsconfig.lib.json(1 hunks)packages/transport-test/e2e/bridge/controller.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/suite-build/configs/desktop.webpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/transport-bridge/tsconfig.lib.json
- packages/transport-bridge/package.json
- .github/workflows/test-transport.yml
- packages/suite-desktop-core/webpack/core.webpack.config.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/transport-test/e2e/bridge/controller.ts
[error] 16-16: Shouldn't redeclare 'TrezordNode'. Consider to delete it or rename it.
'TrezordNode' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- 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 / node-override init-api-flaky
- GitHub Check: PR-check / web-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: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
🔇 Additional comments (3)
packages/transport-bridge/src/http.ts (1)
410-414: LGTM! Simplified endpoint handler for /status.The code has been simplified by using a direct import of HTML content rather than file system operations, making the endpoint handler more straightforward and removing potential error paths.
packages/transport-test/e2e/bridge/controller.ts (2)
6-7: Avoid importing directly from the dist folder.Importing directly from the
distfolder creates a tight coupling with the build output structure, which can lead to issues if the build process changes. Instead, import from the package's main entry point.-// @ts-ignore -import { TrezordNode } from '@trezor/transport-bridge/dist/index.js'; +import { TrezordNode } from '@trezor/transport-bridge';
16-16: Remove redundant type redeclaration.The
TrezordNodetype is already imported on line 7, and redeclaring it asanycauses a conflict. This redeclaration can lead to type checking issues and should be removed.-type TrezordNode = any;🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Shouldn't redeclare 'TrezordNode'. Consider to delete it or rename it.
'TrezordNode' is defined here:
(lint/suspicious/noRedeclare)
| @@ -1,8 +1,10 @@ | |||
| /* eslint no-console: 0 */ | |||
| /* eslint @typescript-eslint/ban-ts-comment: 0 */ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid disabling ESLint rules globally.
Disabling the @typescript-eslint/ban-ts-comment rule for the entire file is a broad approach that can lead to unintended TypeScript issues being overlooked. Consider addressing the underlying type issues instead.
-/* eslint @typescript-eslint/ban-ts-comment: 0 */📝 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.
| /* eslint @typescript-eslint/ban-ts-comment: 0 */ | |
| // (The ESLint rule disabling comment has been removed) |
0b219df to
c332cd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/transport-bridge/src/http.ts (1)
411-415: Simplified response handling - approvedThe endpoint handler has been simplified by directly using the imported HTML content instead of reading from the filesystem. This is more efficient, eliminates potential I/O errors, and simplifies the code.
Consider adding a comment explaining that the HTML content is pre-loaded during build time to help other developers understand this approach.
app.get('/status', [ (_req, res) => { + // HTML content is imported at build time rather than read from the filesystem res.writeHead(200, { 'Content-Type': 'text/html' }); res.end(htmlContent); }, ]);
📜 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 (7)
.github/workflows/test-transport.yml(1 hunks)packages/suite-build/configs/desktop.webpack.config.ts(0 hunks)packages/suite-desktop-core/webpack/core.webpack.config.ts(1 hunks)packages/transport-bridge/package.json(1 hunks)packages/transport-bridge/src/http.ts(2 hunks)packages/transport-bridge/tsconfig.lib.json(1 hunks)packages/transport-test/e2e/bridge/controller.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/suite-build/configs/desktop.webpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/suite-desktop-core/webpack/core.webpack.config.ts
- .github/workflows/test-transport.yml
- packages/transport-bridge/package.json
- packages/transport-test/e2e/bridge/controller.ts
- packages/transport-bridge/tsconfig.lib.json
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- 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-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: 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: run-desktop-tests (@group=passphrase, 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: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (1)
packages/transport-bridge/src/http.ts (1)
21-23:❓ Verification inconclusive
Good approach to inline HTML, but consider addressing build process later
The approach to import HTML content directly as a module eliminates file system dependencies and simplifies the runtime code. The TODO comment correctly identifies that having two separate build phases might not be optimal.
This change impacts how the UI is loaded. Let's verify that no other parts of the codebase are referencing the original file path:
🏁 Script executed:
#!/bin/bash # Search for code that might be expecting the UI files in their original location rg -A 5 "ui/index\.html" --glob="!packages/transport-bridge/src/http.ts"Length of output: 952
Inline HTML module integration verified – ensure build config consistency
The inline import of HTML in
packages/transport-bridge/src/http.tsis a good move to eliminate runtime file system dependencies. I verified that no other part of the codebase unexpectedly refers to the built file (../dist/ui/index.html). Notably, build configuration files—
packages/transport-bridge/webpack/ui.webpack.config.ts(using../src/ui/index.html)packages/blockchain-link/webpack/dev.js(using${SRC}ui/index.html)—still reference the source HTML. This split is intentional given the current dual-phase build process. As noted in the TODO, consider streamlining the build steps into a single phase down the line.
c332cd6 to
a078ae6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/transport-test/e2e/bridge/controller.ts (2)
6-8: 🛠️ Refactor suggestionImprove import structure for TrezordNode.
The current import setup is overly complex:
- Using
// @ts-ignoreto suppress errors- Importing the type from one location and the class from another
- Directly importing from the dist folder
This creates maintenance challenges and tight coupling with the build output structure.
-// @ts-ignore -import { type TrezordNode } from '@trezor/transport-bridge'; -import { TrezordNode as TrezordNodeClass } from '@trezor/transport-bridge/dist/index.js'; +import { TrezordNode } from '@trezor/transport-bridge';Consider properly aligning the types and imports to avoid needing error suppression.
61-69: 💡 Verification agent🧩 Analysis chain
Remove TypeScript error suppressions.
Multiple TypeScript error suppressions (
@ts-expect-errorand@ts-ignore) indicate underlying type compatibility issues. These should be fixed properly rather than suppressed.The assignment pattern is also unusual - creating the instance and then immediately reassigning it with the result of
.start(). This pattern may lead to confusion about the actual state of the object.Consider refactoring to:
- Fix the type definitions so suppressions aren't needed
- Simplify the initialization pattern
-// @ts-expect-error -this.nodeBridge = new TrezordNodeClass({ - port: 21325, - api: !env.USE_HW ? 'udp' : 'usb', - logger: new Log('test-bridge', false), -}); -// @ts-ignore -this.nodeBridge = await this.nodeBridge.start(); +this.nodeBridge = new TrezordNode({ + port: 21325, + api: !env.USE_HW ? 'udp' : 'usb', + logger: new Log('test-bridge', false), +}); +await this.nodeBridge.start();
🏁 Script executed:
#!/bin/bash # Check the return type of the start method in TrezordNode rg "start\(\)" --type ts -A 5 -B 5 packages/transport-bridge/src/http.tsLength of output: 1063
Refactor instantiation and initialization of nodeBridge
Please remove the TypeScript error suppression comments and refactor the initialization. Creating the instance and then immediately overwriting it with the result of calling its async method leads to subtle type inconsistencies—as confirmed by the inspected implementation of
start(). Instead, instantiate the bridge with the correct type definitions and then simply await its start without reassigning the instance.
- Remove both
@ts-expect-errorand@ts-ignorecomments.- Adjust the code so that you first create the instance and then call
await this.nodeBridge.start();on the already created object.-// @ts-expect-error -this.nodeBridge = new TrezordNodeClass({ - port: 21325, - api: !env.USE_HW ? 'udp' : 'usb', - logger: new Log('test-bridge', false), -}); -// @ts-ignore -this.nodeBridge = await this.nodeBridge.start(); +this.nodeBridge = new TrezordNode({ + port: 21325, + api: !env.USE_HW ? 'udp' : 'usb', + logger: new Log('test-bridge', false), +}); +await this.nodeBridge.start();
🧹 Nitpick comments (1)
packages/transport-test/e2e/bridge/controller.ts (1)
30-30: Property lacks initialization.The
nodeBridgeproperty is now declared without an initial value, which could lead to "undefined" errors if accessed before initialization. Consider retaining the explicit initialization toundefined.-private nodeBridge: TrezordNode | undefined; +private nodeBridge: TrezordNode | undefined = undefined;
📜 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 (7)
.github/workflows/test-transport.yml(1 hunks)packages/suite-build/configs/desktop.webpack.config.ts(0 hunks)packages/suite-desktop-core/webpack/core.webpack.config.ts(1 hunks)packages/transport-bridge/package.json(1 hunks)packages/transport-bridge/src/http.ts(3 hunks)packages/transport-bridge/tsconfig.lib.json(1 hunks)packages/transport-test/e2e/bridge/controller.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/suite-build/configs/desktop.webpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/transport-bridge/tsconfig.lib.json
- packages/suite-desktop-core/webpack/core.webpack.config.ts
- packages/transport-bridge/package.json
- .github/workflows/test-transport.yml
⏰ Context from checks skipped due to timeout of 90000ms (18)
- 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: 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=passphrase, 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: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
packages/transport-bridge/src/http.ts (2)
21-23: Comment on build process improvement.Good documentation of the current two-phase build process. The comment accurately explains the current approach of building UI with webpack and JS with esbuild, then raw-loading the HTML as a string. I agree that streamlining into a single step would be better in the future.
The TypeScript error suppression is necessary here since importing HTML content directly isn't natively supported by TypeScript. This is a reasonable workaround given the build process changes.
411-415: Simplified status endpoint handling.The new implementation directly serves the pre-loaded HTML content, eliminating file system operations at runtime. This is a good improvement that:
- Reduces potential I/O errors and latency
- Simplifies the code flow
- Aligns with the PR objective to simplify the dist structure
The Content-Type header is properly set to 'text/html'.
packages/transport-test/e2e/bridge/controller.ts (1)
2-2: Avoid disabling ESLint rules globally.Disabling the
@typescript-eslint/ban-ts-commentrule for the entire file is a broad approach that can lead to unintended TypeScript issues being overlooked. Consider addressing the underlying type issues instead.-/* eslint @typescript-eslint/ban-ts-comment: 0 */
No description provided.