Bluetooth common reducer #17008
Bluetooth common reducer #17008
Conversation
|
🚀 Expo preview is ready!
|
peter-sanderson
added
the
no-project
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
from
11546ec to
5dbe6c7
Compare
peter-sanderson
requested review from
mroz22,
szymonlesisz,
martykan and
marekrjpolak
as code owners
WalkthroughThe changes integrate Bluetooth functionality across several modules. In the device package, a private Bluetooth property is added to the ✨ 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: 1
🧹 Nitpick comments (3)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (2)
46-52: Initialize adapter state carefully.Setting
isAdapterEnabledtotrueandscanStatusto'running'by default could lead to UI confusion if the real adapter state differs. Consider initializing them tofalseor a neutral state until a real event updates them.
57-59: Avoid silencing type errors with@ts-expect-error.Using
@ts-expect-errormay hide legitimate type issues. Investigate whether a properly typed action or an explicit type guard can eliminate this error while preserving type safety.packages/transport/src/types/index.ts (1)
31-32: Confirm naming consistency for the new UUID property.Adding
uuidto theDescriptortype is sensible. However, confirm it aligns with other device identifiers (e.g.,path,session) and consider adopting a consistent naming convention if additional Bluetooth-specific fields are added later.
📜 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 (8)
packages/connect/src/device/Device.ts(4 hunks)packages/connect/src/types/device.ts(4 hunks)packages/transport/src/api/abstract.ts(1 hunks)packages/transport/src/types/index.ts(1 hunks)suite-common/bluetooth/package.json(1 hunks)suite-common/bluetooth/src/bluetoothActions.ts(1 hunks)suite-common/bluetooth/src/bluetoothReducerCreator.ts(1 hunks)suite-common/bluetooth/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- suite-common/bluetooth/package.json
- suite-common/bluetooth/tsconfig.json
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Type Checking
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Unit Tests
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: test
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
🔇 Additional comments (7)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
67-82: Ensure no concurrency issues when merging devices.When merging known devices and scanned devices, ensure no concurrency issues arise if multiple updates arrive in quick succession. The
Map+sortapproach is solid, but consider whether stale device data from parallel scans or re-connection cycles could overwrite newer data.suite-common/bluetooth/src/bluetoothActions.ts (1)
1-45: Actions are well-structured and consistent.The action creators and payload definitions are clear and type-safe. Aggregating them into
allBluetoothActionsis a convenient approach for maintainability.packages/transport/src/api/abstract.ts (1)
26-26: LGTM! The new Bluetooth device type is properly added.The addition follows the existing pattern and is well-documented with a reference to the trezord-go implementation.
packages/connect/src/types/device.ts (2)
84-86: LGTM! The Bluetooth properties type is well-defined.The type is properly exported and uses a UUID for unique device identification, which is a standard practice for Bluetooth devices.
112-112: LGTM! Bluetooth properties are consistently added across device types.The optional
bluetoothPropsproperty is uniformly applied to all device types (KnownDevice, UnknownDevice, and UnreadableDevice), maintaining type consistency.Also applies to: 134-134, 156-156
packages/connect/src/device/Device.ts (2)
132-132: LGTM! The Bluetooth property is properly initialized.The
bluetoothPropsproperty is correctly marked as readonly and initialized using a clean conditional pattern in the constructor.Also applies to: 224-224
1222-1222: LGTM! The Bluetooth properties are consistently included in device messages.The
bluetoothPropsis properly included in both unacquired and acquired device message objects, maintaining consistency in the device's public interface.Also applies to: 1248-1248
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
2 times, most recently
from
44f7c48 to
3192dde
Compare
| }, | ||
| }, | ||
| ) => { | ||
| if (bluetoothProps && bluetoothProps.uuid in state.devices) { |
There was a problem hiding this comment.
lets make sure that suite-native also provides this value, also change uuid => id
There was a problem hiding this comment.
This will be provided by connect right? I suppose this will be same on mobile and desktop as it uses same connect, right?
There was a problem hiding this comment.
but connect needs to know it from the transport, and mobile uses different transport
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
108-108:⚠️ Potential issueUse array methods instead of the
inoperator.Using
bluetoothProps.id in state.devicesfor an array check is incorrect. Theinoperator checks object properties, not array elements.- if (bluetoothProps && bluetoothProps.id in state.devices) { + if (bluetoothProps && state.devices.some(it => it.id === bluetoothProps.id)) {
🧹 Nitpick comments (2)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (2)
48-53: Reconsider initial scanStatus value.Setting
scanStatusto 'running' initially might be misleading as no scan is actually in progress when the reducer is initialized. Consider setting it to 'done' or adding a new status like 'initial' to better represent the true state.
76-78: Consider reversing the sort order for better UX.The current sort order puts oldest devices first. Consider reversing the order to show recently updated devices at the beginning of the list.
- (a, b) => a.lastUpdate - b.lastUpdate, + (a, b) => b.lastUpdate - a.lastUpdate,
📜 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 (6)
packages/connect/src/device/Device.ts(4 hunks)packages/connect/src/types/device.ts(4 hunks)packages/transport/src/types/index.ts(1 hunks)suite-common/bluetooth/package.json(1 hunks)suite-common/bluetooth/src/bluetoothReducerCreator.ts(1 hunks)suite-common/bluetooth/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/transport/src/types/index.ts
- suite-common/bluetooth/package.json
- suite-common/bluetooth/tsconfig.json
- packages/connect/src/types/device.ts
- packages/connect/src/device/Device.ts
⏰ 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: build-deploy
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=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: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (1)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
15-21: Document the purpose of thedatafield.The
datafield's type (number[]) and purpose are not immediately clear. Consider adding a JSDoc comment explaining what this array represents and its expected format.
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
from
4d84d5f to
7b8e6b2
Compare
|
do not merge until tested in |
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
from
7b8e6b2 to
dc44955
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
108-108:⚠️ Potential issueUse array methods instead of the 'in' operator.
The 'in' operator is not correct for array checks. Use array methods like
.find()or.some()instead.Apply this fix:
- if (bluetoothProps && bluetoothProps.id in state.devices) { + if (bluetoothProps && state.devices.some(device => device.id === bluetoothProps.id)) {
🧹 Nitpick comments (1)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
76-78: Consider optimizing device sorting.The current implementation sorts devices on every update. For better performance with large device lists, consider:
- Moving the sort operation to the selector level
- Using a memoized selector to prevent unnecessary re-sorting
Example implementation using reselect:
import { createSelector } from '@reduxjs/toolkit'; export const selectSortedDevices = createSelector( (state: BluetoothState<T>) => state.devices, (devices) => [...devices].sort((a, b) => a.lastUpdate - b.lastUpdate) );
📜 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 (8)
packages/connect/src/device/Device.ts(4 hunks)packages/connect/src/types/device.ts(4 hunks)packages/transport/src/api/abstract.ts(1 hunks)packages/transport/src/types/index.ts(1 hunks)suite-common/bluetooth/package.json(1 hunks)suite-common/bluetooth/src/bluetoothActions.ts(1 hunks)suite-common/bluetooth/src/bluetoothReducerCreator.ts(1 hunks)suite-common/bluetooth/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/transport/src/types/index.ts
- packages/transport/src/api/abstract.ts
- suite-common/bluetooth/package.json
- packages/connect/src/types/device.ts
- suite-common/bluetooth/tsconfig.json
- packages/connect/src/device/Device.ts
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- 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: test
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
suite-common/bluetooth/src/bluetoothActions.ts (2)
35-38: Consider renaming 'done' status to 'idle'.Based on previous feedback, the term 'done' in
BluetoothScanStatusmight be confusing as it implies a final state. Consider using 'idle' instead to better represent a state that can transition back to 'running'.
1-46: Well-structured Redux actions implementation!The implementation follows Redux best practices with:
- Clear action naming
- Type-safe payloads
- Good separation of concerns
- Consistent use of action creators
suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
47-133: Solid reducer implementation with good state management!The implementation:
- Handles all Bluetooth-related actions properly
- Maintains clear state transitions
- Includes proper error handling
- Has good TypeScript type safety
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
from
dc44955 to
1348459
Compare
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
from
1348459 to
9e47312
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/bluetooth/src/bluetoothActions.ts (1)
28-33: Consider consistent parameter ordering in the payload.The parameters in the type definition are ordered as
id, connectionStatusbut in the payload they're ordered asconnectionStatus, id. Consider maintaining consistent ordering for better maintainability.Apply this diff to maintain consistent parameter ordering:
export const bluetoothConnectDeviceEventAction = createAction( `${BLUETOOTH_PREFIX}/device-connection-status`, ({ connectionStatus, id }: { id: string; connectionStatus: DeviceBluetoothStatus }) => ({ - payload: { id, connectionStatus }, + payload: { connectionStatus, id }, }), );
📜 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)
packages/connect/src/device/Device.ts(4 hunks)packages/connect/src/types/device.ts(4 hunks)packages/transport/src/types/index.ts(1 hunks)suite-common/bluetooth/package.json(1 hunks)suite-common/bluetooth/src/bluetoothActions.ts(1 hunks)suite-common/bluetooth/src/bluetoothReducerCreator.ts(1 hunks)suite-common/bluetooth/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/transport/src/types/index.ts
- suite-common/bluetooth/package.json
- packages/connect/src/device/Device.ts
- suite-common/bluetooth/tsconfig.json
- packages/connect/src/types/device.ts
- suite-common/bluetooth/src/bluetoothReducerCreator.ts
⏰ 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-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=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: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (5)
suite-common/bluetooth/src/bluetoothActions.ts (5)
1-10: LGTM!The imports are well-structured, and the prefix follows Redux naming conventions.
11-14: LGTM!The action creator is well-implemented with proper typing and follows Redux Toolkit best practices.
16-26: Consider renamingdevicestonearbyDevicesfor better clarity.As suggested in a previous review, renaming
devicestonearbyDeviceswould make the distinction between discovered and known devices more clear.Apply this diff to improve naming:
type BluetoothDeviceListUpdatePayload = { - devices: BluetoothDevice[]; + nearbyDevices: BluetoothDevice[]; knownDevices: BluetoothDevice[]; }; export const bluetoothDeviceListUpdate = createAction( `${BLUETOOTH_PREFIX}/device-list-update`, - ({ devices, knownDevices }: BluetoothDeviceListUpdatePayload) => ({ - payload: { devices, knownDevices }, + ({ nearbyDevices, knownDevices }: BluetoothDeviceListUpdatePayload) => ({ + payload: { nearbyDevices, knownDevices }, }), );
35-38: LGTM!The action creator is well-implemented with proper typing and follows Redux Toolkit best practices.
40-45: LGTM!The export is well-structured and follows the common pattern of exporting all actions as a single object.
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
2 times, most recently
from
76327ce to
1c56f4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
97-115:⚠️ Potential issueFix array membership check.
The
inoperator is not appropriate for checking array membership:-if (bluetoothProps && bluetoothProps.id in state.nearbyDevices) { +if (bluetoothProps && state.nearbyDevices.some(device => device.id === bluetoothProps.id)) {
🧹 Nitpick comments (2)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (2)
28-28: Consider defining a structured type for device data.The TODO comment suggests a need for a more specific type for the
datafield.Would you like me to help define a typed data structure for the device data?
62-66: Consider memoizing the sorted device list.The current implementation sorts the list on every update. Consider memoizing the sorted list or sorting only when necessary.
+const sortDevicesByTimestamp = <T extends BluetoothDevice>(devices: T[]) => + devices.sort((a, b) => a.lastUpdatedTimestamp - b.lastUpdatedTimestamp); .addCase(bluetoothDeviceListUpdateAction, (state, { payload: { devices } }) => { - state.nearbyDevices = devices.sort( - (a, b) => a.lastUpdatedTimestamp - b.lastUpdatedTimestamp, - ) as Draft<T>[]; + state.nearbyDevices = sortDevicesByTimestamp(devices) as Draft<T>[]; })
📜 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 (4)
suite-common/bluetooth/package.json(1 hunks)suite-common/bluetooth/src/bluetoothActions.ts(1 hunks)suite-common/bluetooth/src/bluetoothReducerCreator.ts(1 hunks)suite-common/bluetooth/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- suite-common/bluetooth/package.json
- suite-common/bluetooth/tsconfig.json
⏰ 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-override init-api-flaky
- 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: 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: 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: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (8)
suite-common/bluetooth/src/bluetoothActions.ts (4)
1-10: LGTM!The Redux action prefix and imports are well-structured and follow best practices.
11-14: LGTM!The action is well-defined with proper typing for the power status.
16-25: Consider renaming for consistency with state structure.The payload type uses
deviceswhile the state in reducer usesnearbyDevices. For better consistency:-type BluetoothDeviceListUpdatePayload = { - devices: BluetoothDevice[]; -}; +type BluetoothDeviceListUpdatePayload = { + nearbyDevices: BluetoothDevice[]; +}; export const bluetoothDeviceListUpdateAction = createAction( `${BLUETOOTH_PREFIX}/device-list-update`, - ({ devices }: BluetoothDeviceListUpdatePayload) => ({ - payload: { devices }, + ({ nearbyDevices }: BluetoothDeviceListUpdatePayload) => ({ + payload: { nearbyDevices }, }), );
27-37: LGTM!Both actions are well-structured with proper typing for their respective payloads.
suite-common/bluetooth/src/bluetoothReducerCreator.ts (4)
13-43: LGTM!The type definitions are comprehensive and well-structured, providing good type safety for the Bluetooth functionality.
45-52: LGTM!The reducer creator is well-structured with proper generic type support and initial state definition.
55-61: LGTM!The adapter event handling includes proper cleanup of device list and scan status when powered off.
117-122: LGTM!The storage load handling includes proper fallback for undefined values.
| export const allBluetoothActions = { | ||
| bluetoothAdapterEventAction, | ||
| bluetoothDeviceListUpdate: bluetoothDeviceListUpdateAction, | ||
| bluetoothConnectDeviceEventAction, | ||
| bluetoothScanStatusAction, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix naming inconsistency in exported actions.
The bluetoothDeviceListUpdate key doesn't match the action creator name:
export const allBluetoothActions = {
bluetoothAdapterEventAction,
- bluetoothDeviceListUpdate: bluetoothDeviceListUpdateAction,
+ bluetoothDeviceListUpdateAction,
bluetoothConnectDeviceEventAction,
bluetoothScanStatusAction,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const allBluetoothActions = { | |
| bluetoothAdapterEventAction, | |
| bluetoothDeviceListUpdate: bluetoothDeviceListUpdateAction, | |
| bluetoothConnectDeviceEventAction, | |
| bluetoothScanStatusAction, | |
| }; | |
| export const allBluetoothActions = { | |
| bluetoothAdapterEventAction, | |
| bluetoothDeviceListUpdateAction, | |
| bluetoothConnectDeviceEventAction, | |
| bluetoothScanStatusAction, | |
| }; |
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
from
1c56f4c to
3cbf279
Compare
peter-sanderson
force-pushed
the
bluetooth-common-reducer
branch
from
3cbf279 to
d0a484d
Compare
| }, | ||
| }, | ||
| ) => { | ||
| if (bluetoothProps && bluetoothProps.id in state.nearbyDevices) { |
There was a problem hiding this comment.
This condition seems to always evaluate to false since nearbyDevices is no longer an object but an array.
| if (bluetoothProps && bluetoothProps.id in state.nearbyDevices) { | |
| if (!bluetoothProps || !bluetoothProps.id) { | |
| return; | |
| } |
|
|
||
| // Once device is fully connected, we save it to the list of paired devices | ||
| // so next time user opens suite | ||
| const foundPairedDevice = state.knownDevices.find( |
There was a problem hiding this comment.
| const foundPairedDevice = state.knownDevices.find( | |
| const foundKnownDevice = state.knownDevices.find( |
| if (deviceState !== undefined) { | ||
| deviceState.status = null; | ||
|
|
||
| // Once device is fully connected, we save it to the list of paired devices |
| ); | ||
|
|
||
| if (deviceState !== undefined) { | ||
| deviceState.status = null; |
There was a problem hiding this comment.
Why is status set to null here? 🤔
There was a problem hiding this comment.
i've added and initialized this reducer in suite (in rust-bluetooth branch) and the AppState types shows bluetooth: never
import { BluetoothDevice } from '@trezor/transport-bluetooth';
import { bluetoothReducerCreator } from '@suite-common/bluetooth';
const rootReducer = combineReducers({
....,
bluetooth: bluetoothReducerCreator<BluetoothDevice>(),
});
| name: string; | ||
| data: number[]; // Todo: consider typed data-structure for this | ||
| lastUpdatedTimestamp: number; | ||
| status: DeviceBluetoothStatus | null; |
There was a problem hiding this comment.
status should not be persistent, its related only to connection/pairing phase
| "private": true, | ||
| "license": "See LICENSE.md in repo root", | ||
| "sideEffects": false, | ||
| "main": "src/index", |
There was a problem hiding this comment.
missing src/index file
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
suite-common/bluetooth/src/bluetoothSelectors.ts (2)
3-5: Add documentation for the generic type parameter.Consider adding JSDoc comments to explain the purpose of the generic type parameter
Tand its constraints.+/** + * Represents the Redux state shape containing Bluetooth state. + * @template T Type of Bluetooth device, must extend BluetoothDevice + */ type State<T extends BluetoothDevice> = { bluetooth: BluetoothState<T>; };
7-17: Consider using createSelector for memoization.These selectors might benefit from memoization using
createSelectorfrom Redux Toolkit to prevent unnecessary re-renders when the state hasn't changed.+import { createSelector } from '@reduxjs/toolkit'; -export const selectAdapterStatus = <T extends BluetoothDevice>(state: State<T>) => - state.bluetooth.adapterStatus; +export const selectAdapterStatus = createSelector( + <T extends BluetoothDevice>(state: State<T>) => state.bluetooth, + bluetooth => bluetooth.adapterStatus +); -export const selectKnownDevices = <T extends BluetoothDevice>(state: State<T>) => - state.bluetooth.knownDevices; +export const selectKnownDevices = createSelector( + <T extends BluetoothDevice>(state: State<T>) => state.bluetooth, + bluetooth => bluetooth.knownDevices +);suite-common/bluetooth/src/bluetoothActions.ts (2)
16-18: Add documentation for the action payload type.Consider adding JSDoc comments to explain the purpose and structure of the payload type.
+/** + * Payload type for the bluetoothNearbyDevicesUpdateAction. + * Contains an array of nearby Bluetooth devices discovered during scanning. + */ type BluetoothNearbyDevicesUpdateActionPayload = { nearbyDevices: BluetoothDevice[]; };
27-32: Consider using a dedicated type for the action payload.Extract the payload type to improve type safety and reusability.
+type BluetoothConnectDeviceEventActionPayload = { + id: string; + connectionStatus: DeviceBluetoothStatus; +}; export const bluetoothConnectDeviceEventAction = createAction( `${BLUETOOTH_PREFIX}/connect-device-event`, - ({ connectionStatus, id }: { id: string; connectionStatus: DeviceBluetoothStatus }) => ({ + ({ connectionStatus, id }: BluetoothConnectDeviceEventActionPayload) => ({ payload: { id, connectionStatus }, }), );suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
65-67: Consider sorting by signal strength or name.The current sorting by
lastUpdatedTimestampmight not provide the best user experience. Consider sorting by signal strength (if available) or device name for better usability.state.nearbyDevices = nearbyDevices.sort( - (a, b) => a.lastUpdatedTimestamp - b.lastUpdatedTimestamp, + (a, b) => a.name.localeCompare(b.name), ) as Draft<T>[];
📜 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 (6)
suite-common/bluetooth/package.json(1 hunks)suite-common/bluetooth/src/bluetoothActions.ts(1 hunks)suite-common/bluetooth/src/bluetoothReducerCreator.ts(1 hunks)suite-common/bluetooth/src/bluetoothSelectors.ts(1 hunks)suite-common/bluetooth/src/index.ts(1 hunks)suite-common/bluetooth/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- suite-common/bluetooth/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- suite-common/bluetooth/package.json
- suite-common/bluetooth/tsconfig.json
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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: 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: transport-e2e-test
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (1)
suite-common/bluetooth/src/bluetoothReducerCreator.ts (1)
25-31: Consider using a typed data structure for the data array.The TODO comment suggests that the
dataproperty needs a more specific type. Consider defining a proper type or interface for the data structure.Could you provide more context about what kind of data is stored in this array? This would help in suggesting an appropriate type structure.
| if (bluetoothProps && bluetoothProps.id in state.nearbyDevices) { | ||
| const deviceState = state.nearbyDevices.find( | ||
| it => it.id === bluetoothProps.id, | ||
| ); | ||
|
|
||
| if (deviceState !== undefined) { | ||
| deviceState.status = null; | ||
|
|
||
| // Once device is fully connected, we save it to the list of paired devices | ||
| // so next time user opens suite | ||
| const foundPairedDevice = state.knownDevices.find( | ||
| it => it.id === bluetoothProps.id, | ||
| ); | ||
| if (foundPairedDevice === undefined) { | ||
| state.knownDevices.push(deviceState); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix array membership check and potential race condition.
The code has two issues:
- Using
inoperator for array membership check is incorrect - Setting device status to
nullbefore checking known devices could lead to a race condition
-if (bluetoothProps && bluetoothProps.id in state.nearbyDevices) {
+if (bluetoothProps?.id) {
const deviceState = state.nearbyDevices.find(
it => it.id === bluetoothProps.id,
);
if (deviceState !== undefined) {
- deviceState.status = null;
+ const foundKnownDevice = state.knownDevices.find(
+ it => it.id === bluetoothProps.id,
+ );
+ if (foundKnownDevice === undefined) {
+ state.knownDevices.push(deviceState);
+ }
- // Once device is fully connected, we save it to the list of paired devices
- // so next time user opens suite
- const foundPairedDevice = state.knownDevices.find(
- it => it.id === bluetoothProps.id,
- );
- if (foundPairedDevice === undefined) {
- state.knownDevices.push(deviceState);
- }
}
}📝 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.
| if (bluetoothProps && bluetoothProps.id in state.nearbyDevices) { | |
| const deviceState = state.nearbyDevices.find( | |
| it => it.id === bluetoothProps.id, | |
| ); | |
| if (deviceState !== undefined) { | |
| deviceState.status = null; | |
| // Once device is fully connected, we save it to the list of paired devices | |
| // so next time user opens suite | |
| const foundPairedDevice = state.knownDevices.find( | |
| it => it.id === bluetoothProps.id, | |
| ); | |
| if (foundPairedDevice === undefined) { | |
| state.knownDevices.push(deviceState); | |
| } | |
| } | |
| } | |
| if (bluetoothProps?.id) { | |
| const deviceState = state.nearbyDevices.find( | |
| it => it.id === bluetoothProps.id, | |
| ); | |
| if (deviceState !== undefined) { | |
| const foundKnownDevice = state.knownDevices.find( | |
| it => it.id === bluetoothProps.id, | |
| ); | |
| if (foundKnownDevice === undefined) { | |
| state.knownDevices.push(deviceState); | |
| } | |
| } | |
| } |
|
Closing in favor of: #17090 |
This PR introduces first version of the shared reducers and state (for desktop & mobile) for connecting a Bluetooth device.