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
|
Warning Rate limit exceeded@mroz22 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
WalkthroughThis pull request systematically replaces references to the 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
from
f6b52f1 to
0159c6f
Compare
mroz22
force-pushed
the
components-should-not-depend-on-connect
branch
from
0159c6f to
224f1da
Compare
…ct-components
I'd say that components and probably also product-components should not depend on connect.