chore(monorepo): remove dependency on connect in components and produ… #17153
Conversation
mroz22
requested review from
szymonlesisz,
martykan,
marekrjpolak and
a team
as code owners
WalkthroughThe changes update multiple packages by shifting dependencies and type imports from the old ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/product-components/src/components/TokenIconSet/TokenIconSet.tsx (2)
23-23: Simplify the grid layout configuration.The ternary operator in
minmax(${$length > 1 ? '1px' : '6px'}, 6px)is redundant since this CSS block only executes when$length > 1.- grid-template-columns: repeat(auto-fit, minmax(${$length > 1 ? '1px' : '6px'}, 6px)); + grid-template-columns: repeat(auto-fit, minmax(1px, 6px));
55-63: Add error handling and type narrowing.Consider adding error handling for invalid inputs and type narrowing for optional fields:
{visibleTokens.map(token => ( + // Ensure contract address exists + token.contract ? ( <AssetLogo key={token.contract} size={20} coingeckoId={coingeckoId ?? ''} contractAddress={getContractAddressForNetworkSymbol(symbol, token.contract)} - placeholder={token.symbol?.toUpperCase() ?? ''} + placeholder={token.symbol ? token.symbol.toUpperCase() : ''} placeholderWithTooltip={false} /> + ) : null ))}packages/device-utils/src/types.ts (2)
3-11: Consider adding JSDoc comments for device models.While the enum values are self-explanatory for those familiar with Trezor devices, adding documentation would help new developers understand what each model represents.
export enum DeviceModelInternal { + /** Trezor One with Bitcoin-only firmware */ T1B1 = 'T1B1', + /** Trezor T with regular firmware */ T2T1 = 'T2T1', + /** Trezor T with Bitcoin-only firmware */ T2B1 = 'T2B1', + /** Trezor Safe 3 with Bitcoin-only firmware */ T3B1 = 'T3B1', + /** Trezor Safe 3 with regular firmware */ T3T1 = 'T3T1', + /** Trezor Safe 3 wireless variant */ T3W1 = 'T3W1', + /** Unknown device model */ UNKNOWN = 'UNKNOWN', }
20-41: Consider grouping related version fields.The features object has multiple version-related fields that could be grouped for better organization.
export type PartialDevice = { features?: { bootloader_hash?: string; - major_version?: number; - minor_version?: number; - patch_version?: number; - - fw_major?: number; - fw_minor?: number; - fw_patch?: number; + version?: { + major?: number; + minor?: number; + patch?: number; + }; + firmware?: { + major?: number; + minor?: number; + patch?: number; + }; firmwareType?: FirmwareType; revision?: string; bootloader_mode?: boolean; initialize?: boolean; no_backup?: boolean; unit_btconly?: boolean; }; };packages/product-components/src/components/PassphraseTypeCard/PassphraseTypeCard.tsx (1)
67-67: Consider using a more specific type for deviceBackup.The type change from
Features['backup_type'] | nulltostring | nullmakes the type less specific. Since this prop is used to check for specific values (e.g.,deviceBackup === 'Bip39'), consider using a union type to ensure type safety:- deviceBackup?: string | null; + deviceBackup?: 'Bip39' | null;
📜 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 (27)
packages/components/package.json(1 hunks)packages/components/src/components/animations/DeviceAnimation.tsx(1 hunks)packages/components/src/components/animations/LottieAnimation.tsx(1 hunks)packages/components/tsconfig.json(1 hunks)packages/connect/package.json(1 hunks)packages/connect/src/types/device.ts(1 hunks)packages/connect/src/types/firmware.ts(1 hunks)packages/connect/tsconfig.json(1 hunks)packages/connect/tsconfig.lib.json(1 hunks)packages/device-utils/package.json(0 hunks)packages/device-utils/src/firmwareUtils.ts(1 hunks)packages/device-utils/src/index.ts(1 hunks)packages/device-utils/src/types.ts(1 hunks)packages/device-utils/tsconfig.json(1 hunks)packages/product-components/package.json(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.stories.tsx(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.tsx(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDeviceContent.tsx(1 hunks)packages/product-components/src/components/PassphraseTypeCard/EnterOnTrezorButton.tsx(1 hunks)packages/product-components/src/components/PassphraseTypeCard/PassphraseTypeCard.tsx(2 hunks)packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.stories.tsx(1 hunks)packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.tsx(1 hunks)packages/product-components/src/components/TokenIconSet/TokenIconSet.tsx(1 hunks)packages/product-components/src/utils/mapTrezorModelToIcon.ts(1 hunks)packages/product-components/tsconfig.json(1 hunks)packages/protobuf/src/messages-schema.ts(1 hunks)packages/suite-desktop-core/tsconfig.lib.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/device-utils/package.json
✅ Files skipped from review due to trivial changes (10)
- packages/product-components/src/components/PassphraseTypeCard/EnterOnTrezorButton.tsx
- packages/components/src/components/animations/LottieAnimation.tsx
- packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.stories.tsx
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDeviceContent.tsx
- packages/components/src/components/animations/DeviceAnimation.tsx
- packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.tsx
- packages/product-components/src/utils/mapTrezorModelToIcon.ts
- packages/device-utils/src/firmwareUtils.ts
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.stories.tsx
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.tsx
⏰ Context from checks skipped due to timeout of 90000ms (16)
- 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=passphrase, trezor-user-env-unix)
- GitHub Check: build
- GitHub Check: transport-e2e-test
- 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: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (17)
packages/product-components/src/components/TokenIconSet/TokenIconSet.tsx (1)
5-5: LGTM! Import change aligns with PR objectives.The change successfully removes the dependency on
@trezor/connectby importing theTokenInfotype from@trezor/blockchain-link-types. The explicittypeimport is a good TypeScript practice.packages/device-utils/src/index.ts (1)
5-5: LGTM! Export statement aligns with PR objectives.The addition of
export * from './types';supports the centralization of device-related types in the device-utils package, which is a key part of removing the connect dependency.packages/device-utils/src/types.ts (2)
1-1: LGTM! Well-defined version string type.The template literal type ensures version strings follow the correct format of three dot-separated numbers.
13-16: LGTM! Clear firmware type enum.The enum values accurately represent the two firmware variants.
packages/connect/src/types/firmware.ts (1)
1-1: LGTM! Import aligns with type centralization.Importing
VersionArrayfrom device-utils instead of defining it locally reduces duplication and centralizes type definitions.packages/connect/src/types/device.ts (1)
1-1: LGTM! Import aligns with type centralization.Importing
FirmwareTypefrom device-utils instead of defining it locally reduces duplication and centralizes type definitions.packages/product-components/src/components/PassphraseTypeCard/PassphraseTypeCard.tsx (1)
8-8: LGTM!The import of
DeviceModelInternalfrom@trezor/device-utilsaligns with the PR objective of removing dependency on@trezor/connect.packages/protobuf/src/messages-schema.ts (1)
1-1: LGTM!The import of
DeviceModelInternalfrom@trezor/device-utilsand removal of the local enum aligns with the PR objective of centralizing device-related types.packages/device-utils/tsconfig.json (1)
4-4: LGTM!Removing the reference to
@trezor/connectis correct as it prevents circular dependencies and aligns with the architectural goal of makingdevice-utilsindependent.packages/connect/tsconfig.json (1)
11-11: LGTM!Adding a reference to
@trezor/device-utilscorrectly establishes the new dependency relationship and maintains proper dependency direction.packages/product-components/tsconfig.json (1)
15-34: Updated References Array Aligns with PR ObjectivesThe references array has been updated to include the new dependency reference
"../blockchain-link-types"(line 25) and no longer includes the"../connect"reference. This change correctly aligns the tsconfig with the intended removal of the obsolete connect dependency in product-components.packages/components/tsconfig.json (1)
29-30: TSConfig Path Reference UpdatedThe reference for
"device-utils"(line 30) has been added as a replacement for the removed"connect"dependency. This update improves the modularity of the components package and is in line with the PR objectives.packages/connect/tsconfig.lib.json (1)
27-29: Added Device-Utils Reference in ConnectA new reference to
"../device-utils"(lines 27–29) has been introduced. This ensures that the connect package now leverages device-related utilities from the updated module. Please confirm that all device-related types and functionality now originate from@trezor/device-utils.packages/suite-desktop-core/tsconfig.lib.json (1)
8-74: Extensive References Array Update – Verify Connect DependencyThe references array has been significantly extended to include many new modules (e.g. message-system, sentry, suite-types, etc.), which should enhance cross-package type-checking and integration. One point to note: the reference to
"../connect"(present around lines 29–32) remains in the configuration. Given that the PR’s main focus was on removing the connect dependency in components and product-components, please verify that its retention in suite-desktop-core is intentional and that no unintended dependencies persist.packages/product-components/package.json (1)
22-22: Dependencies Updated CorrectlyThe dependency on
@trezor/blockchain-link-typeshas been added (line 22) and the obsolete@trezor/connectdependency has been removed. This update is consistent with the broader effort to eliminate the connect dependency and streamline the module imports. Ensure all affected import statements in the codebase have been updated accordingly.packages/components/package.json (1)
29-29: New Device Utils Dependency Added
The new dependency on@trezor/device-utilshas been correctly added to replace the removed@trezor/connectdependency. This update aligns with the PR objectives by reducing unnecessary coupling and increasing modularity.packages/connect/package.json (1)
89-89: Added Device Utils Dependency in Connect Package
The addition of@trezor/device-utilsin the dependencies ensures that device-related functionalities and type definitions are centralized. This change supports the broader effort to shift from@trezor/connectand enhance consistency across the codebase.
|
🚀 Expo preview is ready!
|
| import { CloneType, Static, Type } from '@trezor/schema-utils'; | ||
|
|
||
| export enum DeviceModelInternal { |
There was a problem hiding this comment.
This is an auto-generated file, to do this the scripts would need updating
There was a problem hiding this comment.
and I can't say I like it very much making this types generation script even more complex :(
| @@ -19,8 +19,8 @@ | |||
| "@suite-common/validators": "workspace:*", | |||
| "@suite-common/wallet-config": "workspace:*", | |||
| "@suite-common/wallet-utils": "workspace:*", | |||
| "@trezor/blockchain-link-types": "workspace:*", | |||
There was a problem hiding this comment.
@trezor/device-utils should be here too right?
mroz22
force-pushed
the
components-should-not-depend-on-connect
branch
from
692bf4b to
f6b52f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/device-utils/src/customTypes.ts (1)
1-9: LGTM! Consider adding JSDoc comments.The
DeviceModelInternalenum is well-structured with clear naming. Consider adding JSDoc comments to document:
- The purpose of this enum
- What each device model represents (T1B1, T2T1, etc.)
- When UNKNOWN is used
packages/device-utils/package.json (1)
9-9: New "build:lib" Script AdditionThe new
"build:lib"script successfully chains the commands to clean thelibdirectory, build the TypeScript project withtsconfig.lib.json, and update import statements using the customreplace-imports.shscript. Verify that the relative path (../../scripts/replace-imports.sh) is correct within the monorepo structure and that the script has the necessary execution permissions.
📜 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 (45)
packages/components/package.json(1 hunks)packages/components/src/components/animations/DeviceAnimation.tsx(1 hunks)packages/components/src/components/animations/LottieAnimation.tsx(1 hunks)packages/components/tsconfig.json(2 hunks)packages/connect/package.json(1 hunks)packages/connect/src/api/firmware/getBinaryForFirmwareUpgrade.ts(1 hunks)packages/connect/src/api/firmware/parseFirmwareHeaders.ts(1 hunks)packages/connect/src/data/firmwareInfo.ts(1 hunks)packages/connect/src/data/models.ts(1 hunks)packages/connect/src/device/__tests__/checkFirmwareRevision.test.ts(1 hunks)packages/connect/src/device/calculateRevisionForDevice.ts(1 hunks)packages/connect/src/events/device.ts(1 hunks)packages/connect/src/types/api/firmwareUpdate.ts(1 hunks)packages/connect/src/types/device.ts(2 hunks)packages/connect/src/types/firmware.ts(1 hunks)packages/connect/src/types/index.ts(1 hunks)packages/connect/tsconfig.json(1 hunks)packages/connect/tsconfig.lib.json(1 hunks)packages/device-utils/package.json(1 hunks)packages/device-utils/src/customTypes.ts(1 hunks)packages/device-utils/src/firmwareUtils.ts(2 hunks)packages/device-utils/src/index.ts(1 hunks)packages/device-utils/src/modelUtils.ts(1 hunks)packages/device-utils/src/types.ts(1 hunks)packages/device-utils/tests/__fixtures__/modelUtils.ts(1 hunks)packages/device-utils/tsconfig.json(1 hunks)packages/device-utils/tsconfig.lib.json(1 hunks)packages/product-components/package.json(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.stories.tsx(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.tsx(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDeviceContent.tsx(1 hunks)packages/product-components/src/components/PassphraseTypeCard/EnterOnTrezorButton.tsx(1 hunks)packages/product-components/src/components/PassphraseTypeCard/PassphraseTypeCard.tsx(2 hunks)packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.stories.tsx(1 hunks)packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.tsx(1 hunks)packages/product-components/src/components/TokenIconSet/TokenIconSet.tsx(1 hunks)packages/product-components/src/utils/mapTrezorModelToIcon.ts(1 hunks)packages/product-components/tsconfig.json(1 hunks)packages/protobuf/messages.json(2 hunks)packages/protobuf/scripts/protobuf-types.ts(1 hunks)packages/protobuf/src/index.ts(0 hunks)packages/protobuf/src/messages-schema.ts(1 hunks)packages/protobuf/src/messages.ts(5 hunks)packages/suite-desktop-core/tsconfig.lib.json(1 hunks)submodules/trezor-common(1 hunks)
💤 Files with no reviewable changes (1)
- packages/protobuf/src/index.ts
✅ Files skipped from review due to trivial changes (9)
- packages/connect/src/data/models.ts
- packages/connect/src/device/tests/checkFirmwareRevision.test.ts
- packages/device-utils/tests/fixtures/modelUtils.ts
- packages/connect/src/api/firmware/parseFirmwareHeaders.ts
- packages/device-utils/tsconfig.lib.json
- packages/connect/src/events/device.ts
- packages/device-utils/src/modelUtils.ts
- submodules/trezor-common
- packages/connect/src/data/firmwareInfo.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/device-utils/tsconfig.json
- packages/connect/tsconfig.lib.json
- packages/connect/tsconfig.json
- packages/components/package.json
- packages/product-components/src/components/PassphraseTypeCard/EnterOnTrezorButton.tsx
- packages/connect/package.json
- packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.stories.tsx
- packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.tsx
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.tsx
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.stories.tsx
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDeviceContent.tsx
- packages/components/src/components/animations/LottieAnimation.tsx
- packages/device-utils/src/firmwareUtils.ts
- packages/components/src/components/animations/DeviceAnimation.tsx
- packages/components/tsconfig.json
- packages/product-components/src/components/PassphraseTypeCard/PassphraseTypeCard.tsx
- packages/connect/src/types/firmware.ts
- packages/product-components/tsconfig.json
- packages/product-components/package.json
- packages/product-components/src/components/TokenIconSet/TokenIconSet.tsx
- packages/product-components/src/utils/mapTrezorModelToIcon.ts
- packages/protobuf/src/messages-schema.ts
- packages/suite-desktop-core/tsconfig.lib.json
⏰ Context from checks skipped due to timeout of 90000ms (21)
- 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: EAS Update
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- 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: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (13)
packages/protobuf/src/messages.ts (1)
1781-1783:❓ Verification inconclusive
Approve the type change but verify impact.
The change makes the
macproperty required inUnlockedPathRequestresponses, which improves type safety by ensuring the property is always present. However, this is a breaking change that could affect code expectingmacto be optional.Run the following script to find any code that might be affected by this change:
🏁 Script executed:
#!/bin/bash # Description: Find code that might be affected by the UnlockedPathRequest.mac change # Search for UnlockedPathRequest usage rg -A 5 "UnlockedPathRequest" # Search for potential destructuring of the mac property rg "(?:\{[^}]*mac[\s,}]|\.mac\b)"Length of output: 4112
Type Change Approval – Please Verify Downstream Usage
The change to make the
macproperty inUnlockedPathRequestrequired improves type safety since responses will now always include this field. However, since some parts of the code (e.g., inpackages/connect/src/device/DeviceCommands.tsand related usage inpackages/protobuf/messages.jsonandpackages/protobuf/src/messages-schema.ts) may have been written with the assumption thatmaccould be optional, this constitutes a breaking change. Please verify that all consumers ofUnlockedPathRequestcorrectly handle the now-mandatorymacfield.
- File Affected:
packages/protobuf/src/messages.ts(line 1782)- Impact: The response type now enforces the presence of
mac; ensure that downstream code (e.g., inDeviceCommands.tsandunlockPath.ts) does not assume its absence.- Action: Double-check how
UnlockedPathRequestis used across the codebase to prevent any runtime issues due to the required property.packages/protobuf/messages.json (2)
2964-2971: New Field Added: "return_empty_state" in DebugLinkGetStateThis change introduces a new boolean field
"return_empty_state"with ID 4 and a default value offalse. It provides additional control for state retrieval. Please verify that the documentation and corresponding TypeScript schema (in messages-schema.ts) have been updated accordingly so that downstream consumers can correctly interpret and use this field.
5127-5135: Updated Field Requirement: "mac" Field in UnlockedPathRequestThe
"mac"field in theUnlockedPathRequestmessage now explicitly includes"rule": "required", enforcing that a MAC value must be provided. Ensure that all clients sending this message have been updated to provide a valid MAC. It’s also recommended to double-check that the change is consistently reflected in the TypeScript schema and that any related tests are updated.packages/device-utils/src/index.ts (1)
5-6: LGTM! The new exports align with the PR objective.The addition of these exports from
typesandcustomTypesmodules supports the goal of moving type definitions from@trezor/connectto@trezor/device-utils.packages/connect/src/device/calculateRevisionForDevice.ts (1)
1-1: LGTM! Import change aligns with PR objective.The change to import
VersionArrayfrom@trezor/device-utilssupports the goal of centralizing type definitions while preserving the existing functionality.packages/connect/src/types/api/firmwareUpdate.ts (1)
1-1: LGTM! Import change aligns with PR objective.The change to import
VersionArrayfrom@trezor/device-utilssupports the goal of centralizing type definitions while maintaining the existing type structure.packages/connect/src/types/index.ts (1)
46-46: LGTM! Export statement aligns with PR objectives.The export of
FirmwareTypeandVersionArrayfrom@trezor/device-utilsaligns with the PR's goal of reducing dependency on connect by centralizing these types in the device-utils package.packages/device-utils/src/types.ts (3)
1-8: LGTM! Well-structured type definitions.The template literal type
FirmwareVersionString, enumFirmwareType, and tuple typeVersionArrayprovide strong type safety and clear semantics for version handling.
10-60: LGTM! Comprehensive version and bootloader state handling.The
FeaturesNarrowingtype union provides thorough type safety for different device states and version combinations.
62-62:❓ Verification inconclusive
Address the TODO comment regarding code duplication.
The comment indicates potential code duplication with the protobuf package. Consider either:
- Moving these types to the protobuf package
- Creating a shared package for common types
- Documenting why the duplication is necessary
🏁 Script executed:
#!/bin/bash # Check if these types exist in the protobuf package rg -A 5 "type PartialDevice" packages/protobuf/src/messagesLength of output: 132
Attention: Code Duplication in Type Definitions
The TODO comment notes that these types were copy-pasted from
packages/protobuf/src/messages, but our search suggests that the expected file path doesn’t exist. Please verify whether duplicate types (such asPartialDevice) actually reside in the protobuf package—or if the reference is outdated. Based on your findings, consider the following actions:
- If the duplicate types are maintained in an updated location within the protobuf package, refactor by moving these types there.
- Alternatively, create a shared package for common types.
- If the duplication is intentional, document the rationale clearly.
packages/connect/src/api/firmware/getBinaryForFirmwareUpgrade.ts (1)
1-1: LGTM! Import statement updated correctly.The import of
VersionArrayfrom@trezor/device-utilsaligns with the PR's goal of centralizing types while maintaining the function's robust version comparison and error handling.packages/connect/src/types/device.ts (1)
1-1: LGTM! Type imports and exports updated correctly.The imports of
FeaturesNarrowingandFirmwareType, and the export ofDeviceModelInternalfrom@trezor/device-utilsalign with the PR's goal of centralizing device-related types.Also applies to: 151-151
packages/device-utils/package.json (1)
7-8: Dependency Section Clean-up ValidatedThe absence of a
"dependencies"section indicates that the dependency on@trezor/connecthas been removed as per the PR objectives. Please double-check that no module in the package still relies on it.
| @@ -184,7 +184,7 @@ const createCustomTypes = () => { | |||
| lines.push(`export type ${UINT_TYPE} = string | number;`, ''); | |||
| lines.push('// custom type sint32/64'); | |||
| lines.push(`export type ${SINT_TYPE} = string | number;`, ''); | |||
| lines.push(readPatch('customTypes.ts'), ''); | |||
| lines.push(readPatch('../../../device-utils/src/customTypes.ts'), ''); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider using package imports instead of relative paths.
The relative path ../../../device-utils/src/customTypes.ts traverses outside the package boundary, which can be fragile if the directory structure changes. Consider importing from the package directly using @trezor/device-utils instead.
- lines.push(readPatch('../../../device-utils/src/customTypes.ts'), '');
+ lines.push(readPatch('@trezor/device-utils/lib/customTypes'), '');Additionally, verify that this dependency is declared in your package.json:
🏁 Script executed:
#!/bin/bash
# Description: Check if @trezor/device-utils is properly declared as a dependency
# Check package.json for the dependency
jq '.dependencies["@trezor/device-utils"] // .devDependencies["@trezor/device-utils"]' packages/protobuf/package.jsonLength of output: 124
Switch to package import and declare dependency
The relative path ../../../device-utils/src/customTypes.ts is fragile and may break if the directory structure changes. Instead, import custom types directly from the @trezor/device-utils package. However, the dependency check indicates that @trezor/device-utils is currently not declared in packages/protobuf/package.json.
- Action Items:
- Update the import in
packages/protobuf/scripts/protobuf-types.tsto:- lines.push(readPatch('../../../device-utils/src/customTypes.ts'), ''); + lines.push(readPatch('@trezor/device-utils/lib/customTypes'), '');
- Add
@trezor/device-utilsas a dependency inpackages/protobuf/package.json(or confirm it is provided via the monorepo's dependency management).
- Update the import in
📝 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.
| lines.push(readPatch('../../../device-utils/src/customTypes.ts'), ''); | |
| lines.push(readPatch('@trezor/device-utils/lib/customTypes'), ''); |
mroz22
added
the
no-project
mroz22
force-pushed
the
components-should-not-depend-on-connect
branch
2 times, most recently
from
0159c6f to
224f1da
Compare
mroz22
force-pushed
the
components-should-not-depend-on-connect
branch
from
224f1da to
ad1dbc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite-desktop-core/tsconfig.scripts.json (1)
27-34: Revisit 'connect' DependenciesThe references for
"path": "../connect"and"path": "../connect-common"are still present. Given the PR objective of removing the dependency on the "connect" module from components, please confirm whether these references should be removed or updated (for example, replacing them with references todevice-utils) to align with the updated dependency policy.packages/device-utils/src/types.ts (1)
62-62: Address TODO comment in future work.The TODO indicates this code was copied from the protobuf package. Consider creating a follow-up task to address this duplication in a future PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
packages/components/package.json(1 hunks)packages/components/src/components/animations/DeviceAnimation.tsx(1 hunks)packages/components/src/components/animations/LottieAnimation.tsx(1 hunks)packages/components/tsconfig.json(1 hunks)packages/connect/package.json(1 hunks)packages/connect/src/api/firmware/getBinaryForFirmwareUpgrade.ts(1 hunks)packages/connect/src/api/firmware/parseFirmwareHeaders.ts(1 hunks)packages/connect/src/data/firmwareInfo.ts(1 hunks)packages/connect/src/data/models.ts(1 hunks)packages/connect/src/device/__tests__/checkFirmwareRevision.test.ts(1 hunks)packages/connect/src/device/calculateRevisionForDevice.ts(1 hunks)packages/connect/src/events/device.ts(1 hunks)packages/connect/src/types/api/firmwareUpdate.ts(1 hunks)packages/connect/src/types/device.ts(2 hunks)packages/connect/src/types/firmware.ts(1 hunks)packages/connect/src/types/index.ts(1 hunks)packages/connect/tsconfig.json(1 hunks)packages/connect/tsconfig.lib.json(1 hunks)packages/device-utils/package.json(1 hunks)packages/device-utils/src/customTypes.ts(1 hunks)packages/device-utils/src/firmwareUtils.ts(2 hunks)packages/device-utils/src/index.ts(1 hunks)packages/device-utils/src/modelUtils.ts(1 hunks)packages/device-utils/src/types.ts(1 hunks)packages/device-utils/tests/__fixtures__/modelUtils.ts(1 hunks)packages/device-utils/tsconfig.json(1 hunks)packages/device-utils/tsconfig.lib.json(1 hunks)packages/product-components/package.json(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.stories.tsx(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.tsx(1 hunks)packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDeviceContent.tsx(1 hunks)packages/product-components/src/components/PassphraseTypeCard/EnterOnTrezorButton.tsx(1 hunks)packages/product-components/src/components/PassphraseTypeCard/PassphraseTypeCard.tsx(2 hunks)packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.stories.tsx(1 hunks)packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.tsx(1 hunks)packages/product-components/src/components/TokenIconSet/TokenIconSet.tsx(1 hunks)packages/product-components/src/utils/mapTrezorModelToIcon.ts(1 hunks)packages/product-components/tsconfig.json(1 hunks)packages/protobuf/scripts/protobuf-types.ts(1 hunks)packages/protobuf/src/index.ts(0 hunks)packages/suite-desktop-core/tsconfig.scripts.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/protobuf/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (37)
- packages/components/src/components/animations/DeviceAnimation.tsx
- packages/device-utils/tsconfig.lib.json
- packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.stories.tsx
- packages/connect/src/data/firmwareInfo.ts
- packages/connect/tsconfig.json
- packages/connect/src/device/tests/checkFirmwareRevision.test.ts
- packages/connect/tsconfig.lib.json
- packages/device-utils/tsconfig.json
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.stories.tsx
- packages/connect/package.json
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDevice.tsx
- packages/product-components/src/utils/mapTrezorModelToIcon.ts
- packages/product-components/src/components/RotateDeviceImage/RotateDeviceImage.tsx
- packages/device-utils/src/customTypes.ts
- packages/connect/src/data/models.ts
- packages/components/src/components/animations/LottieAnimation.tsx
- packages/components/tsconfig.json
- packages/connect/src/events/device.ts
- packages/product-components/tsconfig.json
- packages/connect/src/types/index.ts
- packages/protobuf/scripts/protobuf-types.ts
- packages/product-components/src/components/TokenIconSet/TokenIconSet.tsx
- packages/connect/src/types/api/firmwareUpdate.ts
- packages/product-components/src/components/ConfirmOnDevice/ConfirmOnDeviceContent.tsx
- packages/connect/src/types/firmware.ts
- packages/device-utils/src/modelUtils.ts
- packages/device-utils/tests/fixtures/modelUtils.ts
- packages/connect/src/device/calculateRevisionForDevice.ts
- packages/device-utils/src/firmwareUtils.ts
- packages/product-components/package.json
- packages/device-utils/package.json
- packages/components/package.json
- packages/product-components/src/components/PassphraseTypeCard/EnterOnTrezorButton.tsx
- packages/product-components/src/components/PassphraseTypeCard/PassphraseTypeCard.tsx
- packages/connect/src/api/firmware/getBinaryForFirmwareUpgrade.ts
- packages/device-utils/src/index.ts
- packages/connect/src/api/firmware/parseFirmwareHeaders.ts
⏰ Context from checks skipped due to timeout of 90000ms (20)
- 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: 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: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (8)
packages/suite-desktop-core/tsconfig.scripts.json (2)
8-25: New Suite-Common References AddedThe newly added references to various suite-common modules (message-system, sentry, suite-constants, suite-types, suite-utils, and wallet-config) enhance module resolution for the scripts. Verify that each added module is indeed needed by the scripts in this directory.
35-73: Overall References VerificationThe remaining references (env-utils, eslint, ipc-proxy, node-utils, request-manager, suite, suite-desktop-api, transport, transport-bridge, urls, utils, trezor-user-env-link, and type-utils) look consistent with the intended modular architecture. Please ensure that each of these modules is correctly configured in your monorepo and that no obsolete or redundant dependency remains.
packages/connect/src/types/device.ts (3)
1-1: Well-organized dependency migration.Nice job importing
FeaturesNarrowingandFirmwareTypefrom the dedicated@trezor/device-utilspackage instead of defining them locally. This aligns with the PR objective of removing dependencies on connect.
158-158: Good package reference update.Updating the import source for
DeviceModelInternalfrom '@trezor/protobuf' to '@trezor/device-utils' maintains consistency with the other device-related type imports.
162-162:✅ Verification successful
Verify type compatibility.
Line 162 uses the imported
FeaturesNarrowingtype. Make sure the implementation indevice-utilsis fully compatible with how it's being used here.
🏁 Script executed:
#!/bin/bash # Find all usages of StrictFeatures to ensure compatibility rg "StrictFeatures" --type tsLength of output: 1555
Type Consistency Verified
The type alias in
packages/connect/src/types/device.ts—declared asexport type StrictFeatures = Features & FeaturesNarrowing;—is used throughout the codebase (e.g., infirmwareUtils.tsvia theisStrictFeaturestype guard and several tests). The shell script output confirms that the usages ofStrictFeaturesalign with the expected pattern, and there’s no evidence of incompatibility with theFeaturesNarrowingimplementation. The integration looks sound, and no further adjustments are needed.packages/device-utils/src/types.ts (3)
1-8: Good type definitions with proper constraints.The new type definitions for
FirmwareVersionString,FirmwareTypeenum, andVersionArrayare well-structured and provide strong typing:
- The template literal type for version strings ensures proper format
- The enum provides type safety for firmware types
- The tuple type for version arrays ensures fixed length
10-60: Comprehensive type definition for device features.The
FeaturesNarrowingtype is well-structured with union types that handle different device states and versions. This provides strong type safety when working with device features across various states.
63-80: Enhanced PartialDevice type with better structured features.The modifications to
PartialDeviceimprove type safety by:
- Using the
FirmwareTypeenum instead of string literals- Expanding the
featuresproperty with comprehensive type informationThis provides better IntelliSense support and compile-time checking.
…ct-components
I'd say that components and probably also product-components should not depend on connect.