BT transport #11855
BT transport #11855
Conversation
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
from
b189c0e to
8b7b77a
Compare
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
2 times, most recently
from
450bb2a to
4f69c75
Compare
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is unstable ownership?A new collaborator has begun publishing package versions. Package stability and security risk may be elevated. Try to reduce the number of authors you depend on to reduce the risk to malicious actors gaining access to your supply chain. Packages should remove inactive collaborators with publishing rights from packages on npm. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
|
|
||
| export class BleApi extends AbstractApi { | ||
| chunkSize = 244; | ||
| devices: BleDevice[] = []; |
| } | ||
|
|
||
| private async enumerate() { | ||
| const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs); |
There was a problem hiding this comment.
Can not be this.bleManager accessed instead? Since the _bleManager should be initialized only once, it should be safe.
| const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs); | |
| const connectedDevices = this.bleManager.connectedDevices(devicesUUIDs); |
| this.bleManager.startDeviceScan(devicesUUIDs, scanOptions, (error, scannedDevice) => { | ||
| if (error) { | ||
| debugLog('Scan error'); | ||
| console.error(error); |
There was a problem hiding this comment.
Should not this be a debugLog?
| // await awaitsBleOn(bleManagerInstance()); | ||
|
|
||
| // Returns a list of known devices by their identifiers | ||
| const devices = await bleManagerInstance().devices([deviceOrId]); |
There was a problem hiding this comment.
| const devices = await bleManagerInstance().devices([deviceOrId]); | |
| const devices = await this.bleManager.devices([deviceOrId]); |
| // Returns a list of the peripherals currently connected to the system | ||
| // which have discovered services, connected to system doesn't mean | ||
| // connected to our app, we check that below. | ||
| const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs); |
There was a problem hiding this comment.
| const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs); | |
| const connectedDevices = await this.bleManager.connectedDevices(devicesUUIDs); |
|
|
||
| // Nb ConnectionOptions dropped since it's not used internally by ble-plx. | ||
| try { | ||
| device = await bleManagerInstance().connectToDevice(deviceOrId, connectOptions); |
| let writeCharacteristic: Characteristic | undefined; | ||
| let notifyCharacteristic: Characteristic | undefined; | ||
| for (const characteristic of characteristics) { | ||
| if (characteristic.isWritableWithoutResponse) { |
There was a problem hiding this comment.
are characteristic UUIDs static or dynamic? I'm wondering if this matching should not be done based on characteristic UUID. But it would be possible only if all the Trezors would have the write/notify characteristic with the same UUID. Does it work like that or is the UUID assigned dynamicaly?
Of course no need to change it in this PR since the prototype is still in development.
| return this.appConnectedDevices.map(d => d.bleDevice); | ||
| }; | ||
|
|
||
| public findDevice = (deviceId: string) => { |
There was a problem hiding this comment.
nit for better clarity:
| public findDevice = (deviceId: string) => { | |
| public findConnectedDevice = (deviceId: string) => { |
| @@ -76,6 +76,11 @@ export const selectDiscoverySupportedNetworks = memoizeWithArgs( | |||
| (state: DeviceRootState, areTestnetsEnabled: boolean) => | |||
| pipe( | |||
| selectDeviceSupportedNetworks(state), | |||
| symbols => { | |||
There was a problem hiding this comment.
leftover that should be removed?
| @@ -17,6 +18,10 @@ export const prepareDiscoveryMiddleware = createMiddlewareWithExtraDeps( | |||
| dispatch(discoveryActions.removeDiscovery(action.payload.state)); | |||
| } | |||
|
|
|||
| if (action.type.startsWith(DISCOVERY_MODULE_PREFIX)) { | |||
| @@ -474,6 +474,8 @@ export const createDescriptorPreloadedDiscoveryThunk = createThunk( | |||
| return; | |||
| } | |||
|
|
|||
| console.log('createDescriptorPreloadedDiscoveryThunk', deviceState, supportedNetworks); | |||
|
|
||
| export const NUS_SERVICE_UUID = '6e400001-b5a3-f393-e0a9-e50e24dcca9e'; | ||
| // const NUS_CHARACTERISTIC_NOTIFY = '6e400003-b5a3-f393-e0a9-e50e24dcca9e'; | ||
| // const NUS_CHARACTERISTIC_TX = '6e400002-b5a3-f393-e0a9-e50e24dcca9e'; |
| }; | ||
| } | ||
|
|
||
| public async enumerate() { |
|
|
||
| if (typeof deviceOrId === 'string') { | ||
| debugLog(`Trying to open device: ${deviceOrId}`); | ||
| // await awaitsBleOn(bleManagerInstance()); |
| let characteristics: Characteristic[] = | ||
| await device.characteristicsForService(NUS_SERVICE_UUID); | ||
|
|
||
| // debugLog('Characteristics: ', JSON.stringify(characteristics)); |
| } | ||
|
|
||
| if (bluetoothAdapterState === AdapterState.Unsupported) { | ||
| return <AlertBox title={'Bluetooth Unsupported on this device'} variant="error" />; |
There was a problem hiding this comment.
please, remove curly braces
| return ( | ||
| <VStack spacing="small"> | ||
| <AlertBox | ||
| title={'Bluetooth is turned off. Please turn of Bluetooth to continue.'} |
| import { AlertBox, Box, Button, HStack, Loader, Text, VStack } from '@suite-native/atoms'; | ||
| import { Translation } from '@suite-native/intl'; | ||
| import { prepareNativeStyle, useNativeStyles } from '@trezor/styles'; | ||
| import { BLEScannedDevice, nativeBleManager } from '@trezor/transport-native-ble'; |
There was a problem hiding this comment.
checks failing due to unused imports
| // TODO: implement proper device type | ||
| return devices.map(d => ({ path: d.id, type: DEVICE_TYPE.TypeT2 })); |
There was a problem hiding this comment.
yes, we probably don't know the type yet? and also we maybe don't know the unique values that are use to match the type yet?
There was a problem hiding this comment.
it might be useful to add return value DescriptorApiLevel to this function. There were some additions to this type, you probably will need to stuff some dummy values (for example product field). they are not that much important for higher layers. But who knows, maybe you have some value like this also available.
|
|
||
| const DEBUG_LOGS = true; | ||
|
|
||
| const debugLog = (...args: any[]) => { |
There was a problem hiding this comment.
now it should be possible to pass logger instance to BleApi from outside. So feel free to use it if you find it useful.
| return this.success(undefined); | ||
| } | ||
|
|
||
| public async openInternal(_path: string, _first: boolean) { |
There was a problem hiding this comment.
openInternal is private so it doesn't need to be here imho
| bleDevice: Device; | ||
| } | ||
|
|
||
| // TODO(possibly?): would be nice to have state machine to handle all possible states and transitions |
There was a problem hiding this comment.
also it would be nice to have this somehow unified with desktop bluetooth implementation. lets agree on some common grounds and ideally use some shared types?
| // This is for debug purposes it should be removed once BT goes to production | ||
| export const log = (message: string) => { | ||
| logs.push(message); | ||
| }; |
There was a problem hiding this comment.
one attempt to unify logs is this https://github.com/trezor/trezor-suite/blob/develop/packages/utils/src/logs.ts#L12
| // TODO: descriptors are somewhere mutated which causes problems on mobile BLE | ||
| descriptors: descriptors.map(d => ({ ...d })), |
There was a problem hiding this comment.
this should be now solved. the mutation happend in background.ts.
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
from
cf42223 to
d2199b1
Compare
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
from
b1224e8 to
8146dd4
Compare
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
from
859b656 to
34043ec
Compare
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
from
34043ec to
5d9f983
Compare
| NSBluetoothAlwaysUsageDescription: | ||
| '$(PRODUCT_NAME) needs access to Bluetooth to connect to your Trezor device.', | ||
| NSBluetoothPeripheralUsageDescription: | ||
| '$(PRODUCT_NAME) needs access to Bluetooth to connect to your Trezor device.', |
There was a problem hiding this comment.
I guess we need to exclude it from production builds before public release. So far it can be under process.env.EXPO_PUBLIC_BLUETOOTH_ENABLED flag.
| @@ -3,7 +3,7 @@ import { Blur, Canvas, useFont, Text as SkiaText } from '@shopify/react-native-s | |||
| import { Color } from '@trezor/theme'; | |||
| import { prepareNativeStyle, useNativeStyles } from '@trezor/styles'; | |||
|
|
|||
| const satoshiFont = require('../../../../packages/theme/fonts/TTSatoshi-Medium.otf'); | |||
| const satoshiFont = require('@trezor/theme/fonts/TTSatoshi-Medium.otf'); | |||
There was a problem hiding this comment.
Yea, it did not worked for me with relative path for a while but in the end it was caused by something different issue, but I would keep it anyway it's better than relative path.
| fontSize: 20, | ||
| lineWidth: 230, | ||
| lineHeight: 25, | ||
| pagerOffset: 60, | ||
| }, | ||
| [DeviceModelInternal.T2B1]: safe3Styles, | ||
| [DeviceModelInternal.T3B1]: safe3Styles, | ||
| ['T3W1']: safe3Styles, |
There was a problem hiding this comment.
T3W1 is already defined in DeviceModelInternal enum
There was a problem hiding this comment.
yea I need to do another rebase
| const bluetoothInfoCache: { [deviceUuid: string]: any } = {}; // Allows us to give more granulary error messages. | ||
|
|
||
| let connectOptions: Record<string, unknown> = { | ||
| requestMTU: 247, |
There was a problem hiding this comment.
Why 247? Missing some comment here.
| }; | ||
|
|
||
| export class BleApi extends AbstractApi { | ||
| chunkSize = 244; |
There was a problem hiding this comment.
I think this constants are taken from desktop implementation
There was a problem hiding this comment.
I will try to add some comments
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
from
7173e33 to
14ac1a5
Compare
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
from
14ac1a5 to
9137792
Compare
|
🚀 Expo preview is ready!
|
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
3 times, most recently
from
152f077 to
99bdf82
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 18
🔭 Outside diff range comments (1)
packages/connect/src/api/index.ts (1)
Line range hint
22-44: Clean up commented exports.Several exports are commented out rather than removed. This creates maintenance overhead and confusion about the API surface.
Options:
- Remove the commented exports if they're no longer needed
- If they're temporarily disabled, add a comment explaining why and when they'll be re-enabled
- If they're pending removal, create a separate PR to remove them properly
Example:
- // export { default as cancel } from './cancel'; - // export { default as disableWebUSB } from './disableWebUSB'; - // export { default as dispose } from './dispose'; + // TODO: These exports are temporarily disabled until the USB transport layer is implemented + // See issue #XXXX for details
🧹 Nitpick comments (21)
suite-native/bluetooth/src/index.ts (1)
2-2: Consider using named exports instead of wildcard export.Wildcard exports (
export *) can make it harder to track dependencies and may accidentally expose unintended exports. Consider explicitly listing the feature flag exports for better maintainability and clarity.-export * from './featureFlag'; +export { isBluetoothEnabled, toggleBluetooth } from './featureFlag';suite-native/bluetooth/src/components/ScannedDeviceItem.tsx (2)
27-36: Consider using relative dimensions for better responsiveness.The deviceItemStyle uses hardcoded dimensions (width: 50, height: 50) which might not scale well across different device sizes.
Consider using relative dimensions or design tokens:
const deviceItemStyle = prepareNativeStyle(utils => ({ flexDirection: 'row', alignItems: 'center', padding: utils.spacings.sp4, backgroundColor: utils.colors.backgroundSurfaceElevation2, - width: 50, - height: 50, + width: utils.dimensions.iconSize.large, + height: utils.dimensions.iconSize.large, borderRadius: 25, justifyContent: 'center', }));
72-75: Consider making maxRetries configurable.The maxRetries value is hardcoded to 2. Consider making this configurable through props or environment variables for better flexibility.
+interface ScannedDeviceItemProps { + device: BLEScannedDevice; + maxRetries?: number; +} + -export const ScannedDeviceItem = ({ device }: { device: BLEScannedDevice }) +export const ScannedDeviceItem = ({ device, maxRetries = 2 }: ScannedDeviceItemProps)Then update the connection logic:
await nativeBleManager.connectDeviceWithRetry({ deviceOrId: bleDevice, - maxRetries: 2, + maxRetries, });suite-native/bluetooth/src/hooks/useBluetoothState.ts (1)
5-16: Consider adding error boundaries and cleanup.While the implementation is solid, consider these improvements:
- Add error handling for status changes
- Ensure proper cleanup of BLE subscriptions
export const useBluetoothState = (): BLEStatus => { const bluetoothState = useSyncExternalStore<BLEStatus>( onStoreChange => { - return nativeBleManager.onStatusChange(onStoreChange); + try { + return nativeBleManager.onStatusChange(onStoreChange); + } catch (error) { + console.error('Failed to subscribe to BLE status:', error); + return () => {}; + } }, () => { - return nativeBleManager.getStatus(); + try { + return nativeBleManager.getStatus(); + } catch (error) { + console.error('Failed to get BLE status:', error); + return 'unknown'; + } }, ); return bluetoothState; };packages/transport-native-ble/src/nativeTransportBLE.ts (1)
9-19: Consider adding validation for BLE availability.The constructor should validate BLE availability before initializing the transport.
constructor(params?: ConstructorParameters<typeof AbstractTransport>[0]) { const { logger } = params || {}; + + if (!nativeBleManager.isBluetoothAvailable()) { + throw new Error('Bluetooth is not available on this device'); + } super({ id: 'native-ble', api: new BleApi({ logger, }), ...params, }); }suite-native/bluetooth/src/featureFlag.ts (2)
10-11: Document storage choice rationale.Using unencrypted storage for the Bluetooth enabled flag should be documented with the security implications.
Add a comment explaining why unencrypted storage is acceptable for this use case:
+// Using unencrypted storage as this is just a feature flag that doesn't contain sensitive data export const isBluetoothEnabled = isBluetoothBuild && (unecryptedJotaiStorage.getBoolean('bluetoothEnabled') ?? false);
13-16: Consider improving UX around app restart.Forcing an app restart on Bluetooth toggle might be jarring for users.
Consider adding a confirmation dialog and handling the restart more gracefully:
export const setBluetoothEnabled = (value: boolean) => { unecryptedJotaiStorage.set('bluetoothEnabled', value); - RNRestart.restart(); + // Show confirmation dialog + Alert.alert( + 'Restart Required', + 'The app needs to restart to apply Bluetooth settings. Restart now?', + [ + { + text: 'Later', + style: 'cancel', + }, + { + text: 'Restart', + onPress: () => RNRestart.restart(), + }, + ], + ); };suite-native/bluetooth/src/hooks/useBluetoothAdapterState.ts (1)
11-23: Consider potential race conditions in state updates.The effect hook fetches the initial state and sets up a subscription. There's a potential race condition where the subscription callback might run before the initial state is set.
useEffect(() => { + let mounted = true; + nativeBleManager.bleManager.state().then(newState => { + if (mounted) { setState(newState); + } }); const subscription = nativeBleManager.bleManager.onStateChange(newState => { + if (mounted) { setState(newState); + } }); return () => { + mounted = false; subscription.remove(); }; }, []);packages/connect/src/api/eraseBonds.ts (1)
9-12: Add JSDoc documentation for the EraseBonds class.The class lacks documentation explaining its purpose, parameters, and behavior.
+/** + * Handles the erasure of Bluetooth bonds on the device. + * @extends AbstractMethod + * @param {Object} params - The parameters for bond erasure + * @param {boolean} [params.current=true] - If true, erases only the current bond; if false, erases all bonds + */ export default class EraseBonds extends AbstractMethod< 'eraseBonds', PROTO.EraseBonds & { current?: boolean } > {suite-native/module-dev-utils/src/components/BluetoothToggle.tsx (1)
33-39: Simplify conditional rendering logic.The condition
!isBluetoothEnabledis redundant as it's the inverse of the previous condition.if (isBluetoothEnabled) { return <Button onPress={disableBluetooth}>🔴 Disable Bluetooth</Button>; } -if (!isBluetoothEnabled) { - return <Button onPress={enableBluetooth}>🔵 Enable Bluetooth</Button>; -} +return <Button onPress={enableBluetooth}>🔵 Enable Bluetooth</Button>;suite-native/bluetooth/src/components/BluetoothPermissionError.tsx (1)
26-30: Add descriptive content to AlertBox.The AlertBox is empty and could benefit from additional content explaining the error and steps to resolve it.
<AlertBox title={`Bluetooth Permission Error - ${ERROR_MESSAGES[error]}`} variant="error" -></AlertBox> +> + Please follow these steps to resolve the issue: + 1. Open your device settings + 2. Navigate to app permissions + 3. Enable the required Bluetooth permissions + 4. Return to the app +</AlertBox>suite-native/bluetooth/src/components/BluetoothAdapterStateManager.tsx (1)
30-33: Use self-closing tag.The empty closing tag can be replaced with a self-closing tag.
- <BluetoothPermissionError - error={BluetoothPermissionErrors.BluetoothAccessBlocked} - ></BluetoothPermissionError> + <BluetoothPermissionError + error={BluetoothPermissionErrors.BluetoothAccessBlocked} + />packages/connect/src/events/call.ts (2)
75-77: Consider using a logger instead of console.error.Direct use of
console.errorin production code isn't ideal. Consider:
- Using a logger that can be configured per environment
- Adding log levels for better filtering
- if (!success) { - console.error(payload); - } + if (!success && process.env.NODE_ENV === 'development') { + logger.error('Response error:', payload); + }
88-91: Simplify device property access.The device parameter is already checked with the ternary operator, making the optional chaining redundant.
- path: device?.getUniquePath(), - state: device?.getState(), - instance: device?.getInstance(), + path: device.getUniquePath(), + state: device.getState(), + instance: device.getInstance(),suite-native/module-authorize-device/src/screens/connect/ConnectAndUnlockDeviceScreen.tsx (2)
91-95: Consider extracting Bluetooth status check to a custom hook.The direct usage of
isBluetoothEnabledcould be moved to a custom hook for better reusability and testing.// useBluetoothStatus.ts export const useBluetoothStatus = () => { const [isEnabled, setIsEnabled] = useState(isBluetoothEnabled); useEffect(() => { // Add listener for Bluetooth state changes // Clean up listener }, []); return isEnabled; };Then in the component:
- {isBluetoothEnabled ? ( + {useBluetoothStatus() ? (
Line range hint
44-94: Consider decomposing the component for better maintainability.The component handles multiple responsibilities: device connection, authorization, and navigation. Consider splitting it into smaller, focused components.
Example structure:
// DeviceAuthorizationHandler.tsx - Handles authorization logic const DeviceAuthorizationHandler: FC<Props> = ({ onAuthorized }) => { // Authorization logic here return null; }; // ConnectionContent.tsx - Handles UI rendering const ConnectionContent: FC = () => { const isEnabled = useBluetoothStatus(); return isEnabled ? <DevicesScanner /> : <ConnectDeviceAnimation />; }; // ConnectAndUnlockDeviceScreen.tsx - Main component export const ConnectAndUnlockDeviceScreen: FC<Props> = ({ route }) => { return ( <Screen> <DeviceAuthorizationHandler /> <ConnectionContent /> </Screen> ); };suite-native/module-dev-utils/src/screens/DevUtilsScreen.tsx (1)
81-87: Consider moving environment variables display to a separate component.The environment variables display logic could be encapsulated in a dedicated component for better maintainability.
Consider extracting this into a new component:
const BluetoothEnvironmentInfo = () => ( <Text> EXPO_PUBLIC_BLUETOOTH_ENABLED: {process.env.EXPO_PUBLIC_BLUETOOTH_ENABLED} {'\n'} {'\n'} isBluetoothBuild: {String(isBluetoothBuild)} {'\n'} isBluetoothEnabled: {String(isBluetoothEnabled)} </Text> );suite-native/bluetooth/src/hooks/useBluetoothPermissions.ts (1)
113-129: Consider debouncing app state changes.Multiple rapid app state changes could trigger unnecessary permission checks.
Consider adding debounce logic:
import { debounce } from 'lodash'; // Inside the hook: const debouncedRequestPermissions = useCallback( debounce( (checkOnly: boolean) => requestPermissions({ checkOnly }), 500, { leading: true, trailing: false } ), [requestPermissions] ); useLayoutEffect(() => { const handleAppStateChange = (nextAppState: AppStateStatus) => { if (appState.current.match(/inactive|background/) && nextAppState === 'active') { setBluetoothPermissionError(undefined); debouncedRequestPermissions(true); } appState.current = nextAppState; }; // ... rest of the effect }, [debouncedRequestPermissions]);suite-native/receive/src/components/DeviceScreenContent.tsx (1)
37-37: Standardize all font source paths.Some font paths use absolute imports while others still use relative paths. For consistency, all font paths should use absolute imports from @trezor/theme.
Update the remaining relative paths:
const smallTouchscreenStyles = { - fontSource: require('../../../../packages/theme/fonts/RobotoMono-Regular.ttf'), + fontSource: require('@trezor/theme/fonts/RobotoMono-Regular.ttf'), fontSize: 20, lineWidth: 230, lineHeight: 25, pagerOffset: 60, }; // ... and later in the file: [DeviceModelInternal.T3W1]: { - fontSource: require('../../../../packages/theme/fonts/RobotoMono-Regular.ttf'), + fontSource: require('@trezor/theme/fonts/RobotoMono-Regular.ttf'), fontSize: 20, lineWidth: 230, lineHeight: 25, pagerOffset: 60, },Also applies to: 54-54
suite-native/app/app.config.ts (1)
238-241: Verify Bluetooth permission strings.The permission strings are clear and user-friendly. However, consider adding information about why Bluetooth is needed specifically for device communication.
- '$(PRODUCT_NAME) needs access to Bluetooth to connect to your Trezor device.', + '$(PRODUCT_NAME) needs access to Bluetooth to securely communicate with your Trezor hardware wallet.', - '$(PRODUCT_NAME) needs access to Bluetooth to connect to your Trezor device.', + '$(PRODUCT_NAME) needs access to Bluetooth to securely communicate with your Trezor hardware wallet.',packages/protobuf/src/messages.ts (1)
19-21: Add documentation for the new message types.The implementation is correct, but consider adding JSDoc comments to explain the purpose of these empty message types and their relationship to the Bluetooth transport layer.
+/** Message type for erasing Bluetooth bonds on the device */ export type EraseBonds = {}; +/** Message type for unpairing a Bluetooth device */ export type Unpair = {};🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 21-21: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🛑 Comments failed to post (18)
suite-native/bluetooth/src/components/ScannedDeviceItem.tsx (2)
58-67: 🛠️ Refactor suggestion
Optimize component updates.
The current implementation forces a rerender every second for all instances of this component, which could impact performance with multiple devices.
Consider using a shared timer service or React's useMemo:
const lastSeenText = useMemo(() => { const seconds = Math.floor((Date.now() - device.lastSeenTimestamp) / 1000); return seconds > 5 ? `Last seen: ${seconds}s ago` : ''; }, [device.lastSeenTimestamp]);
77-78:
⚠️ Potential issueRemove duplicate error alert.
The code shows both a native alert and Alert.alert for the same error. This creates a confusing user experience with two consecutive alerts.
- alert('Error connecting to device'); Alert.alert('Error connecting to device', error?.message, [{ text: 'OK' }]);📝 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.Alert.alert('Error connecting to device', error?.message, [{ text: 'OK' }]);packages/transport-native-ble/src/api/bleApi.ts (1)
27-60: 🛠️ Refactor suggestion
Remove duplication between constructor and
listen()
The constructor andlisten()method both setnativeBleManager.onconnectandnativeBleManager.ondisconnect. Consider consolidating or removinglisten()if the same logic is established at construction.public listen() { // Consider reusing constructor logic or removing if unneeded. - nativeBleManager.onconnect = async device => { - // ... - }; - nativeBleManager.ondisconnect = async device => { - // ... - }; }📝 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 class BleApi extends AbstractApi { chunkSize = 244; devices: BleDevice[] = []; constructor({ logger }: ConstructorParams) { super({ logger }); nativeBleManager.onconnect = async device => { debugLog('onconnect', device.id); const devicesDescriptors = await this.devicesToDescriptors(); this.emit('transport-interface-change', devicesDescriptors); }; nativeBleManager.ondisconnect = async device => { debugLog('ondisconnect', device.id); this.emit('transport-interface-change', await this.devicesToDescriptors()); }; } // This doesn't seems to work. Figure out why and then we can remove listener in constructor. public listen() { }packages/transport-native-ble/src/api/nativeBleManager.ts (1)
140-199: 💡 Codebase verification
⚠️ Potential issueExplicit scan error handling required
The review comment is correct. Without definitive evidence that scanning stops automatically on error, we should explicitly call
stopDeviceScan()to prevent potential background scanning and battery drain.if (error) { debugLog('Scan error'); console.error(error); + this.stopDeviceScan(); // ensure scanning is stopped if error occurs if (onError) { onError(error); } this.changeStatus({ status: 'scanError', error }); }🔗 Analysis chain
Validate scanning error handling and finalize approach
When scanning fails, the code logs the error and callsonError. Confirm that the BLE library automatically stops scanning after an error, or explicitly stop scanning to avoid unexpected scans continuing in the background.if (error) { debugLog('Scan error'); console.error(error); + this.stopDeviceScan(); // ensure scanning is stopped if error occurs // ... }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for stopDeviceScan implementation and usage echo "=== Searching for stopDeviceScan implementation ===" ast-grep --pattern 'stopDeviceScan' echo -e "\n=== Searching for BLE error handling patterns ===" rg -A 5 "bleManager.*error" packages/transport-native-ble echo -e "\n=== Looking for BLE-related test files ===" fd -e test.ts -e spec.ts . packages/transport-native-bleLength of output: 1791
Script:
#!/bin/bash # Search for stopDeviceScan method implementation echo "=== Searching for stopDeviceScan method ===" ast-grep --pattern $'public stopDeviceScan = () => { $$$ }' echo -e "\n=== Looking for BLE manager type definitions ===" rg -A 5 "interface.*BleManager" packages/transport-native-ble echo -e "\n=== Looking for BLE manager initialization ===" ast-grep --pattern $'bleManager = new' echo -e "\n=== Looking for documentation about scanning ===" rg -i "scan.*error" packages/transport-native-ble/README.md || trueLength of output: 747
packages/transport-native-ble/src/logs.ts (1)
3-6:
⚠️ Potential issueRestrict or remove logs in production
Storing logs in an array can grow unbounded. For production, consider removing or limiting this log storage to prevent memory bloat or sensitive data leaks.export const log = (message: string) => { logs.push(message); + // In production, consider removing or limiting logs to avoid excessive memory usage or exposing sensitive data. };Committable suggestion skipped: line range outside the PR's diff.
packages/transport-native-ble/src/nativeTransportBLE.ts (1)
6-7: 🛠️ Refactor suggestion
Remove type assertion and improve type safety.
The
as anytype assertion on thenameproperty should be avoided as it bypasses type checking.- public name = 'NativeTransportBLE' as any; + public readonly name = 'NativeTransportBLE';📝 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.public readonly name = 'NativeTransportBLE'; public apiType = 'bluetooth' as const;suite-native/bluetooth/src/featureFlag.ts (1)
8-8: 🛠️ Refactor suggestion
Remove console.log statement.
Production builds should not contain console.log statements.
-console.log('isBluetoothBuild', isBluetoothBuild);suite-native/bluetooth/src/hooks/useBluetoothAdapterState.ts (1)
25-27: 🛠️ Refactor suggestion
Add error handling for Bluetooth enable operation.
The
turnOnBluetoothAdapterfunction should handle potential errors that may occur when enabling Bluetooth.const turnOnBluetoothAdapter = async () => { - await nativeBleManager.bleManager.enable(); + try { + await nativeBleManager.bleManager.enable(); + } catch (error) { + console.error('Failed to enable Bluetooth:', error); + throw error; + } };📝 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.const turnOnBluetoothAdapter = async () => { try { await nativeBleManager.bleManager.enable(); } catch (error) { console.error('Failed to enable Bluetooth:', error); throw error; } };packages/connect/src/api/eraseBonds.ts (1)
28-37: 🛠️ Refactor suggestion
Add error handling and logging to run method.
The run method should handle potential errors from the device commands and provide meaningful error messages.
async run() { - const cmd = this.device.getCommands(); - const response = await cmd.typedCall( - this.params.current ? 'Unpair' : 'EraseBonds', - 'Success', - {}, - ); - - return response.message; + try { + const cmd = this.device.getCommands(); + const response = await cmd.typedCall( + this.params.current ? 'Unpair' : 'EraseBonds', + 'Success', + {}, + ); + return response.message; + } catch (error) { + console.error('Failed to erase bonds:', error); + throw error; + } }📝 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.async run() { try { const cmd = this.device.getCommands(); const response = await cmd.typedCall( this.params.current ? 'Unpair' : 'EraseBonds', 'Success', {}, ); return response.message; } catch (error) { console.error('Failed to erase bonds:', error); throw error; } }suite-native/module-dev-utils/src/components/BluetoothToggle.tsx (1)
6-39: 🛠️ Refactor suggestion
Add loading state handling during Bluetooth state changes.
The component should handle loading states while Bluetooth is being enabled/disabled to prevent multiple clicks.
export const BluetoothToggle = () => { + const [isLoading, setIsLoading] = useState(false); + + const handleSetBluetoothEnabled = async (enabled: boolean) => { + setIsLoading(true); + try { + await setBluetoothEnabled(enabled); + } finally { + setIsLoading(false); + } + }; + const enableBluetooth = () => { Alert.alert('Enable Bluetooth?', 'This will restart the app and enable bluetooth', [ { text: 'Cancel', style: 'cancel', }, { text: 'Enable', - onPress: () => setBluetoothEnabled(true), + onPress: () => handleSetBluetoothEnabled(true), }, ]); }; // ... similar changes for disableBluetooth if (isBluetoothEnabled) { - return <Button onPress={disableBluetooth}>🔴 Disable Bluetooth</Button>; + return <Button onPress={disableBluetooth} isLoading={isLoading}>🔴 Disable Bluetooth</Button>; } - return <Button onPress={enableBluetooth}>🔵 Enable Bluetooth</Button>; + return <Button onPress={enableBluetooth} isLoading={isLoading}>🔵 Enable Bluetooth</Button>; };📝 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 BluetoothToggle = () => { const [isLoading, setIsLoading] = useState(false); const handleSetBluetoothEnabled = async (enabled: boolean) => { setIsLoading(true); try { await setBluetoothEnabled(enabled); } finally { setIsLoading(false); } }; const enableBluetooth = () => { Alert.alert('Enable Bluetooth?', 'This will restart the app and enable bluetooth', [ { text: 'Cancel', style: 'cancel', }, { text: 'Enable', onPress: () => handleSetBluetoothEnabled(true), }, ]); }; const disableBluetooth = () => { Alert.alert('Disable Bluetooth?', 'This will restart the app and disable bluetooth', [ { text: 'Cancel', style: 'cancel', }, { text: 'Disable', onPress: () => handleSetBluetoothEnabled(false), }, ]); }; if (isBluetoothEnabled) { return <Button onPress={disableBluetooth} isLoading={isLoading}>🔴 Disable Bluetooth</Button>; } return <Button onPress={enableBluetooth} isLoading={isLoading}>🔵 Enable Bluetooth</Button>; };suite-native/bluetooth/src/components/BluetoothPermissionError.tsx (1)
20-22: 🛠️ Refactor suggestion
Add error handling for openSettings operation.
The
handleOpenSettingsfunction should handle potential errors when opening settings.const handleOpenSettings = async () => { - await openSettings(); + try { + await openSettings(); + } catch (error) { + console.error('Failed to open settings:', error); + // Consider showing a fallback UI or instructions + } };📝 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.const handleOpenSettings = async () => { try { await openSettings(); } catch (error) { console.error('Failed to open settings:', error); // Consider showing a fallback UI or instructions } };suite-native/module-authorize-device/src/screens/connect/ConnectingDeviceScreen.tsx (1)
20-21:
⚠️ Potential issueTemporary workaround needs proper fix.
Commenting out the back navigation handler to prevent screen freeze is a temporary solution. This could lead to a poor user experience as users might get stuck on this screen.
Please:
- Investigate the root cause of the screen freeze with BT devkit
- Implement a proper fix that maintains back navigation functionality
- If the workaround must stay temporarily, add a TODO comment with an issue reference
suite-native/app/src/App.tsx (1)
28-28: 🛠️ Refactor suggestion
Avoid adding TrezorConnect to global scope.
Adding to global scope, even in dev mode:
- Makes debugging harder as it's not clear where TrezorConnect is coming from
- Creates inconsistency between dev and prod environments
- Lacks type safety
Consider alternatives:
- Use React Context for global access
- Create a debug utility that doesn't pollute global scope
// debug.ts export const debugUtils = { TrezorConnect: __DEV__ ? TrezorConnect : undefined, };suite-native/bluetooth/src/components/DevicesScanner.tsx (1)
67-72:
⚠️ Potential issueDebug information exposed in production.
The component displays internal state information that should only be visible in development builds.
Wrap the debug information in a development check:
- <Text> - hasBluetoothPermissions: {hasBluetoothPermissions ? 'true' : 'false'} {'\n'} - bluetoothPermissionError: {bluetoothPermissionError} {'\n'} - bluetoothAdapterState: {bluetoothAdapterState} {'\n'} - bluetoothState: {bluetoothState.status} {'\n'} - </Text> + {__DEV__ && ( + <Text> + hasBluetoothPermissions: {hasBluetoothPermissions ? 'true' : 'false'} {'\n'} + bluetoothPermissionError: {bluetoothPermissionError} {'\n'} + bluetoothAdapterState: {bluetoothAdapterState} {'\n'} + bluetoothState: {bluetoothState.status} {'\n'} + </Text> + )}📝 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.{__DEV__ && ( <Text> hasBluetoothPermissions: {hasBluetoothPermissions ? 'true' : 'false'} {'\n'} bluetoothPermissionError: {bluetoothPermissionError} {'\n'} bluetoothAdapterState: {bluetoothAdapterState} {'\n'} bluetoothState: {bluetoothState.status} {'\n'} </Text> )}suite-native/module-dev-utils/src/screens/DevUtilsScreen.tsx (1)
42-60: 🛠️ Refactor suggestion
Improve error handling in
handleEraseBonds.The function could benefit from more specific error handling and type safety.
Consider this improved implementation:
const handleEraseBonds = async () => { setIsErasingBonds(true); try { Alert.alert('Please confirm erasing BT bonds on device.'); const result = await TrezorConnect.eraseBonds({}); - console.log('result', result); + console.log('Erase bonds result:', result); if (!result.success) { - throw new Error(`${result.payload.code} - ${result.payload.error}`); + throw new Error( + `Failed to erase bonds: ${result.payload.code} - ${result.payload.error}` + ); } Alert.alert( 'BT bonds erased - please follow these steps:', - `1. Accept request on Trezor \n2. Restart Trezor device by cutting the power \n2. Click on "Forget device" in system settings \n3. Restart mobile app`, + [ + '1. Accept request on Trezor', + '2. Restart Trezor device by cutting the power', + '3. Click on "Forget device" in system settings', + '4. Restart mobile app' + ].join('\n'), ); } catch (error) { - console.error(error); + console.error('Error erasing BT bonds:', error); Alert.alert('Error erasing BT bonds', error.message); } finally { setIsErasingBonds(false); } };📝 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.const handleEraseBonds = async () => { setIsErasingBonds(true); try { Alert.alert('Please confirm erasing BT bonds on device.'); const result = await TrezorConnect.eraseBonds({}); console.log('Erase bonds result:', result); if (!result.success) { throw new Error( `Failed to erase bonds: ${result.payload.code} - ${result.payload.error}` ); } Alert.alert( 'BT bonds erased - please follow these steps:', [ '1. Accept request on Trezor', '2. Restart Trezor device by cutting the power', '3. Click on "Forget device" in system settings', '4. Restart mobile app' ].join('\n'), ); } catch (error) { console.error('Error erasing BT bonds:', error); Alert.alert('Error erasing BT bonds', error.message); } finally { setIsErasingBonds(false); } };suite-native/bluetooth/src/hooks/useBluetoothPermissions.ts (1)
45-46: 🛠️ Refactor suggestion
Improve type safety for OS version check.
The OS version comparison could be more robust with proper type handling.
Consider this safer implementation:
-const deviceOSVersion = Constants.systemVersion || 0; +const deviceOSVersion = Number(Constants.systemVersion) || 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const deviceOSVersion = Number(Constants.systemVersion) || 0;suite-common/connect-init/src/connectInitThunks.ts (1)
160-160:
⚠️ Potential issueGuard debug logs behind a development flag.
Enabling debug logs by default could expose sensitive information in production. Consider guarding this with an environment check.
- debug: true, // Enable debug logs in TrezorConnect + debug: process.env.NODE_ENV !== 'production', // Enable debug logs only in development📝 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.debug: process.env.NODE_ENV !== 'production', // Enable debug logs only in developmentsuite-native/app/package.json (1)
112-112: 💡 Codebase verification
Update react-native-permissions to latest patch version
The dependency versions are secure with no known vulnerabilities. Consider updating
react-native-permissionsfrom^5.2.3to^5.2.4to get the latest patch fixes.🔗 Analysis chain
Verify package versions for security and compatibility.
The added dependencies align with the PR objectives:
react-native-ble-plx@^3.4.0for Bluetooth functionalityreact-native-permissions@^5.2.3for permission handlingLet's verify the latest versions and any security advisories:
Also applies to: 117-117
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check latest versions and security advisories for the new dependencies echo "Checking react-native-ble-plx..." npm view react-native-ble-plx versions --json | jq -r '.[-1]' gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "react-native-ble-plx") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }' echo "Checking react-native-permissions..." npm view react-native-permissions versions --json | jq -r '.[-1]' gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "react-native-permissions") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }'Length of output: 902
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
suite-native/module-dev-utils/src/screens/DevUtilsScreen.tsx (3)
21-21: Remove unused importnativeBleManager.The
nativeBleManageris imported but not used in this file.-import { logs, nativeBleManager } from '@trezor/transport-native-ble'; +import { logs } from '@trezor/transport-native-ble';
42-60: Enhance error handling and logging inhandleEraseBonds.Consider the following improvements:
- Make error messages more user-friendly
- Extract alert messages to constants for reusability
- Add more informative success logging
+const BT_BONDS_ALERTS = { + CONFIRM: 'Please confirm erasing BT bonds on device.', + SUCCESS_TITLE: 'BT bonds erased - please follow these steps:', + SUCCESS_STEPS: + '1. Accept request on Trezor\n' + + '2. Restart Trezor device by cutting the power\n' + + '3. Click on "Forget device" in system settings\n' + + '4. Restart mobile app', + ERROR_TITLE: 'Error erasing BT bonds', +}; const handleEraseBonds = async () => { setIsErasingBonds(true); try { - Alert.alert('Please confirm erasing BT bonds on device.'); + Alert.alert(BT_BONDS_ALERTS.CONFIRM); const result = await TrezorConnect.eraseBonds({}); - console.log('result', result); + console.log('BT bonds erasure result:', result); if (!result.success) { throw new Error(`${result.payload.code} - ${result.payload.error}`); } Alert.alert( - 'BT bonds erased - please follow these steps:', - `1. Accept request on Trezor \n2. Restart Trezor device by cutting the power \n2. Click on "Forget device" in system settings \n3. Restart mobile app`, + BT_BONDS_ALERTS.SUCCESS_TITLE, + BT_BONDS_ALERTS.SUCCESS_STEPS, ); } catch (error) { console.error(error); - Alert.alert('Error erasing BT bonds', error.message); + Alert.alert( + BT_BONDS_ALERTS.ERROR_TITLE, + 'Unable to erase Bluetooth bonds. Please ensure your device is connected and try again.' + ); } setIsErasingBonds(false); };
81-87: Improve environment variables display formatting.The Text component could be more readable with proper formatting and styling.
-<Text> - EXPO_PUBLIC_BLUETOOTH_ENABLED:{' '} - {process.env.EXPO_PUBLIC_BLUETOOTH_ENABLED} {'\n'} - {'\n'} - isBluetoothBuild: {String(isBluetoothBuild)} {'\n'} - isBluetoothEnabled: {String(isBluetoothEnabled)} -</Text> +<VStack spacing="sp8"> + <Text variant="body"> + <Text variant="label">EXPO_PUBLIC_BLUETOOTH_ENABLED: </Text> + {String(process.env.EXPO_PUBLIC_BLUETOOTH_ENABLED)} + </Text> + <Text variant="body"> + <Text variant="label">isBluetoothBuild: </Text> + {String(isBluetoothBuild)} + </Text> + <Text variant="body"> + <Text variant="label">isBluetoothEnabled: </Text> + {String(isBluetoothEnabled)} + </Text> +</VStack>packages/transport-native-ble/src/api/nativeBleManager.ts (4)
34-34: Add a comment explaining the MTU value.The magic number 247 for MTU (Maximum Transmission Unit) should be documented. This appears to be the maximum MTU size for BLE, but it should be explained why this specific value was chosen.
- requestMTU: 247, + // BLE 5.0 supports a maximum MTU size of 247 bytes + requestMTU: 247,
56-57: Remove or document commented UUIDs.These commented UUIDs appear to be unused. If they're for future use or reference, add a TODO comment explaining their purpose. Otherwise, consider removing them to avoid technical debt.
16-16: Make DEBUG_LOGS configurable via environment variable.Hardcoding
DEBUG_LOGSto false makes debugging harder in production environments. Consider making it configurable via an environment variable.-const DEBUG_LOGS = false; +const DEBUG_LOGS = process.env.EXPO_PUBLIC_BLE_DEBUG === 'true';
573-573: Improve singleton pattern implementation with type safety.Consider adding type safety to the singleton instance to prevent accidental instantiation.
-export const nativeBleManager = new NativeBleManager(); +export const nativeBleManager: Readonly<NativeBleManager> = new NativeBleManager(); + +// Prevent additional instances +Object.freeze(nativeBleManager);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/transport-native-ble/src/api/nativeBleManager.ts(1 hunks)suite-native/module-authorize-device/src/screens/connect/ConnectingDeviceScreen.tsx(1 hunks)suite-native/module-dev-utils/src/screens/DevUtilsScreen.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- suite-native/module-authorize-device/src/screens/connect/ConnectingDeviceScreen.tsx
| public read = (deviceId: string): Promise<Base64String> => { | ||
| debugLog(`Reading from ${deviceId}`); | ||
|
|
||
| return new Promise<Base64String>((resolve, reject) => { | ||
| const startTime = Date.now(); | ||
|
|
||
| // Define a function that tries to read the last element of the array | ||
| const tryRead = () => { | ||
| const device = this.findDevice(deviceId); | ||
| if (!device) { | ||
| debugLog(`Device ${deviceId} not found for reading`); | ||
| reject(new Error('Device disconnected or not found.')); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (device.readOutputQueue.length === 0) { | ||
| // debugLog('No data to read received yet... waiting'); | ||
| // If the array is empty and we have not exceeded 10 seconds, we try again | ||
| if (Date.now() - startTime < READ_TIMEOUT_MS) { | ||
| setTimeout(tryRead, READ_FREQUENCY_MS); // Wait for 10ms before trying again | ||
| } else { | ||
| // If we've waited more than 10 seconds, we reject the promise | ||
| reject(new Error('Read TIMEOUT: No data received in timeframe.')); | ||
| } | ||
| } else { | ||
| const message = device.readOutputQueue.pop()!; | ||
| debugLog("Data received, we're reading from the queue.", message); | ||
| debugLog( | ||
| `Remaining messages in queue (${device.readOutputQueue.length}): `, | ||
| device.readOutputQueue, | ||
| ); | ||
|
|
||
| // If the array is not empty, we resolve the promise with the last element | ||
| resolve(message.value); | ||
| } | ||
| }; | ||
|
|
||
| tryRead(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize the read polling mechanism.
The current implementation has several potential issues:
- The 3-minute timeout (READ_TIMEOUT_MS) seems excessive for a BLE operation
- Constant polling every 10ms could be resource-intensive
- Missing exponential backoff for retries
Consider implementing exponential backoff and reducing the timeout:
-const READ_TIMEOUT_MS = 3 * 60 * 1000; // 3 minutes
-const READ_FREQUENCY_MS = 10;
+const READ_TIMEOUT_MS = 30 * 1000; // 30 seconds should be sufficient
+const INITIAL_READ_FREQUENCY_MS = 10;
+const MAX_READ_FREQUENCY_MS = 1000;
const tryRead = () => {
// ...
if (device.readOutputQueue.length === 0) {
if (Date.now() - startTime < READ_TIMEOUT_MS) {
- setTimeout(tryRead, READ_FREQUENCY_MS);
+ const nextDelay = Math.min(
+ INITIAL_READ_FREQUENCY_MS * Math.pow(2, retryCount),
+ MAX_READ_FREQUENCY_MS
+ );
+ retryCount++;
+ setTimeout(tryRead, nextDelay);
} else {
reject(new Error('Read TIMEOUT: No data received in timeframe.'));
}
}
// ...
};Committable suggestion skipped: line range outside the PR's diff.
| public write = async (deviceId: string, message: Base64String) => { | ||
| const device = this.findDevice(deviceId); | ||
| if (!device) { | ||
| console.error(`Device ${deviceId} not found for writing`); | ||
| throw new Error(`Device ${deviceId} not found for writing`); | ||
| } | ||
| debugLog(`Writing to ${deviceId}: ${message}`); | ||
| try { | ||
| await device.bleDevice.writeCharacteristicWithResponseForService( | ||
| NUS_SERVICE_UUID, | ||
| device.writeUuid, | ||
| message, | ||
| ); | ||
| debugLog('Write successful'); | ||
| } catch (e) { | ||
| console.error('Error writing: ', JSON.stringify(e)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Improve error handling in write method.
The current implementation swallows errors and only logs them. This could lead to silent failures in the application. Additionally, JSON.stringify(e) might not capture all error properties.
public write = async (deviceId: string, message: Base64String) => {
const device = this.findDevice(deviceId);
if (!device) {
console.error(`Device ${deviceId} not found for writing`);
throw new Error(`Device ${deviceId} not found for writing`);
}
debugLog(`Writing to ${deviceId}: ${message}`);
try {
await device.bleDevice.writeCharacteristicWithResponseForService(
NUS_SERVICE_UUID,
device.writeUuid,
message,
);
debugLog('Write successful');
} catch (e) {
- console.error('Error writing: ', JSON.stringify(e));
+ const error = e instanceof Error ? e : new Error('Unknown write error');
+ console.error('Error writing: ', error);
+ throw error;
}
};📝 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.
| public write = async (deviceId: string, message: Base64String) => { | |
| const device = this.findDevice(deviceId); | |
| if (!device) { | |
| console.error(`Device ${deviceId} not found for writing`); | |
| throw new Error(`Device ${deviceId} not found for writing`); | |
| } | |
| debugLog(`Writing to ${deviceId}: ${message}`); | |
| try { | |
| await device.bleDevice.writeCharacteristicWithResponseForService( | |
| NUS_SERVICE_UUID, | |
| device.writeUuid, | |
| message, | |
| ); | |
| debugLog('Write successful'); | |
| } catch (e) { | |
| console.error('Error writing: ', JSON.stringify(e)); | |
| } | |
| }; | |
| public write = async (deviceId: string, message: Base64String) => { | |
| const device = this.findDevice(deviceId); | |
| if (!device) { | |
| console.error(`Device ${deviceId} not found for writing`); | |
| throw new Error(`Device ${deviceId} not found for writing`); | |
| } | |
| debugLog(`Writing to ${deviceId}: ${message}`); | |
| try { | |
| await device.bleDevice.writeCharacteristicWithResponseForService( | |
| NUS_SERVICE_UUID, | |
| device.writeUuid, | |
| message, | |
| ); | |
| debugLog('Write successful'); | |
| } catch (e) { | |
| const error = e instanceof Error ? e : new Error('Unknown write error'); | |
| console.error('Error writing: ', error); | |
| throw error; | |
| } | |
| }; |
fix: ios BT permissions fix(bt): fix connect after rebase fix log advertisment packets fix: use connect for easing bonds
Nodonisko
force-pushed
the
feat/bluetooth-mobile
branch
from
99bdf82 to
eacaeb1
Compare
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
error Error: http://10.0.0.28:4873/@trezor%2feslint: no such package available WalkthroughThe pull request introduces comprehensive Bluetooth functionality for the Trezor Suite Native application. It includes the creation of a new Assessment against linked issues
The implementation provides a solid foundation for Bluetooth functionality, with a focus on infrastructure and permission handling. Further development will be needed to complete the full set of Bluetooth device interaction capabilities. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
packages/connect/src/api/index.ts (1)
Critical: Commented exports are actively used across the codebase
The commented-out exports (
cancel,disableWebUSB,dispose,init, etc.) are actively used throughout the codebase:
cancel: Used in device interactions, transaction flows, and error handlingdispose: Critical for resource cleanup and state managementinit: Required for initialization across multiple packagesdisableWebUSB: Used in desktop suite and troubleshooting featuresThese changes would introduce breaking changes without a proper migration path or documentation.
🔗 Analysis chain
Line range hint
21-43: Verify the impact of commented-out exports.Several core exports have been commented out. While this might be intentional for the BT transport POC, we should verify that:
- No existing code depends on these exports
- The changes are properly documented
- There's a migration plan if needed
Also applies to: 45-47, 49-50, 52-52
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of commented-out exports echo "Searching for usage of commented-out exports..." rg -l "import.*\{.*(cancel|disableWebUSB|dispose|init|manifest|off|on|removeAllListeners|uiResponse).*\}.*from.*@trezor/connect"Length of output: 5011
Script:
#!/bin/bash # Search for specific commented exports individually for export in cancel disableWebUSB dispose init manifest off; do echo "=== Searching for $export ===" rg "import.*\{.*\b$export\b.*\}.*from.*@trezor/connect" -A 2 echo "=== Searching for direct usage of $export ===" rg "\b$export\b\(" -A 2 done # Check for documentation echo "=== Checking documentation ===" fd "CHANGELOG.md|README.md" --exec cat {}Length of output: 67947
🧹 Nitpick comments (21)
packages/transport-native-ble/src/api/bleApi.ts (2)
28-28: Clarify thechunkSizeconstant.Currently,
chunkSize = 244appears as a "magic number." Providing a comment or linking to a reference could help future maintainers understand why this specific size is used (e.g., inherited from desktop BLE implementation).chunkSize = 244; +// Explanation or link here: e.g., +// "Maximum payload size for Trezor BLE packets, derived from desktop references."
47-60: Avoid duplicating connection listeners.
listen()reassignsnativeBleManager.onconnectandnativeBleManager.ondisconnect, duplicating the constructor’s logic. Consider removing or consolidating to avoid confusion.packages/transport-native-ble/src/api/nativeBleManager.ts (2)
60-61: Reduce or make the read timeout configurable.
READ_TIMEOUT_MS = 3 minutesseems quite large for BLE operations. A more reasonable (or configurable) timeout could improve the user experience and protect from indefinite waits if the device is unresponsive.-const READ_TIMEOUT_MS = 3 * 60 * 1000; // 3 minutes +// Consider making this shorter or configurable: +const READ_TIMEOUT_MS = 30 * 1000; // 30 seconds
516-536: Implement a less aggressive read polling strategy.Polling every 10ms until 3 minutes is resource-intensive and might drain the device’s battery. An exponential backoff or event-based read approach could be more efficient.
packages/transport-native-ble/src/logs.ts (1)
3-6: Remove or replace debug logs for production.Storing logs in a global array could expand memory usage. Replace with a more robust logging framework if needed, or remove before releasing.
packages/transport-native-ble/src/nativeTransportBLE.ts (1)
6-6: Consider using a more specific type for thenameproperty.The
anytype assertion could be replaced with a more specific type to improve type safety.- public name = 'NativeTransportBLE' as any; + public name = 'NativeTransportBLE' as const;suite-native/bluetooth/src/featureFlag.ts (1)
1-1: Fix typo in comment.The word "standart" should be "standard".
-// due to nature of BT, we can't use standart feature flag mechanism +// due to nature of BT, we can't use standard feature flag mechanismsuite-native/module-dev-utils/src/components/BluetoothToggle.tsx (1)
33-39: Simplify conditional rendering logic.The component has redundant condition checks. The second condition is unnecessary since it's mutually exclusive with the first one.
- if (isBluetoothEnabled) { - return <Button onPress={disableBluetooth}>🔴 Disable Bluetooth</Button>; - } - - if (!isBluetoothEnabled) { - return <Button onPress={enableBluetooth}>🔵 Enable Bluetooth</Button>; - } + return isBluetoothEnabled ? ( + <Button onPress={disableBluetooth}>🔴 Disable Bluetooth</Button> + ) : ( + <Button onPress={enableBluetooth}>🔵 Enable Bluetooth</Button> + );suite-native/bluetooth/src/components/BluetoothPermissionError.tsx (1)
19-32: Add loading state and accessibility improvements.The component should show loading state while opening settings and include accessibility labels.
+import { useState } from 'react'; + export const BluetoothPermissionError = ({ error }: BluetoothPermissionErrorProps) => { + const [isOpeningSettings, setIsOpeningSettings] = useState(false); + const handleOpenSettings = async () => { + setIsOpeningSettings(true); await openSettings(); + setIsOpeningSettings(false); }; return ( <VStack spacing="small"> <AlertBox title={`Bluetooth Permission Error - ${ERROR_MESSAGES[error]}`} variant="error" + accessibilityLabel={`Bluetooth permission error: ${ERROR_MESSAGES[error]}`} ></AlertBox> - <Button onPress={handleOpenSettings}>Open Settings</Button> + <Button + onPress={handleOpenSettings} + isLoading={isOpeningSettings} + accessibilityLabel="Open phone settings to grant Bluetooth permissions" + > + Open Settings + </Button> </VStack> ); };suite-native/bluetooth/src/components/BluetoothAdapterStateManager.tsx (1)
30-33: Use self-closing tag.The
BluetoothPermissionErrorcomponent has no children, so it can be self-closing.- <BluetoothPermissionError - error={BluetoothPermissionErrors.BluetoothAccessBlocked} - ></BluetoothPermissionError> + <BluetoothPermissionError + error={BluetoothPermissionErrors.BluetoothAccessBlocked} + />suite-native/app/src/App.tsx (1)
16-16: Consider using a dedicated debug utility instead of global assignment.While assigning
TrezorConnectto the global object aids in debugging, consider creating a dedicated debug utility that:
- Centralizes all debug-only functionality
- Provides type safety
- Includes proper documentation
Example implementation:
// debug.ts export const initDebugTools = () => { if (__DEV__) { global.debugTools = { TrezorConnect, // Add other debug utilities here }; } }; // Add JSDoc for type safety declare global { var debugTools: { TrezorConnect: typeof TrezorConnect; }; }Also applies to: 28-28
suite-native/bluetooth/src/components/ScannedDeviceItem.tsx (2)
83-84: Extract magic numbers into named constants.The time thresholds should be extracted into named constants for better maintainability.
+const QUITE_LONG_AGO_THRESHOLD_SEC = 5; +const OUT_OF_RANGE_THRESHOLD_SEC = 30; + const lastSeenInSec = Math.floor((Date.now() - device.lastSeenTimestamp) / 1000); -const seenQuiteLongAgo = lastSeenInSec > 5; +const seenQuiteLongAgo = lastSeenInSec > QUITE_LONG_AGO_THRESHOLD_SEC; -if (lastSeenInSec > 30) { +if (lastSeenInSec > OUT_OF_RANGE_THRESHOLD_SEC) { // This device is probably not in range anymore or it's not advertising anymore return null; }Also applies to: 86-89
27-36: Avoid hardcoded dimensions in styles.The deviceItemStyle has hardcoded width and height values which might not scale well across different devices.
const deviceItemStyle = prepareNativeStyle(utils => ({ flexDirection: 'row', alignItems: 'center', padding: utils.spacings.sp4, backgroundColor: utils.colors.backgroundSurfaceElevation2, - width: 50, - height: 50, + width: utils.spacings.sp13, // Use theme spacing + height: utils.spacings.sp13, borderRadius: 25, justifyContent: 'center', }));suite-native/bluetooth/src/components/DevicesScanner.tsx (2)
55-63: Extract complex conditional logic into readable variables.The shouldShow* variables could be grouped and simplified for better readability.
+const hasRequiredPermissions = hasBluetoothPermissions && !bluetoothPermissionError; +const isBluetoothReady = bluetoothAdapterState === AdapterState.PoweredOn; + -const shouldShowRequestPermission = !hasBluetoothPermissions; -const shouldShowScanDevicesButton = - bluetoothState.status === 'idle' && - hasBluetoothPermissions && - bluetoothAdapterState === AdapterState.PoweredOn; -const shouldShowBluetoothAdapterManager = - bluetoothAdapterState !== AdapterState.PoweredOn && hasBluetoothPermissions; -const shouldShowBluetoothPermissionError = !!bluetoothPermissionError; -const shouldShowPairingBanner = bluetoothState.status === 'pairing'; +const shouldShowRequestPermission = !hasRequiredPermissions; +const shouldShowScanDevicesButton = bluetoothState.status === 'idle' && hasRequiredPermissions && isBluetoothReady; +const shouldShowBluetoothAdapterManager = !isBluetoothReady && hasRequiredPermissions; +const shouldShowBluetoothPermissionError = !!bluetoothPermissionError; +const shouldShowPairingBanner = bluetoothState.status === 'pairing';
24-25: Add type information for bluetoothPermissionError.The error type from useBluetoothPermissions is not properly typed.
+type BluetoothPermissionError = string | null; + const { hasBluetoothPermissions, requestBluetoothPermissions, bluetoothPermissionError } = - useBluetoothPermissions(); + useBluetoothPermissions<BluetoothPermissionError>();suite-native/module-dev-utils/src/screens/DevUtilsScreen.tsx (2)
42-60: Improve error handling inhandleEraseBonds.The error handling could be enhanced to provide more specific error messages and handle edge cases.
Apply this diff to improve error handling:
const handleEraseBonds = async () => { setIsErasingBonds(true); try { Alert.alert('Please confirm erasing BT bonds on device.'); const result = await TrezorConnect.eraseBonds({}); - console.log('result', result); if (!result.success) { throw new Error(`${result.payload.code} - ${result.payload.error}`); } Alert.alert( 'BT bonds erased - please follow these steps:', `1. Accept request on Trezor \n2. Restart Trezor device by cutting the power \n2. Click on "Forget device" in system settings \n3. Restart mobile app`, ); } catch (error) { - console.error(error); - Alert.alert('Error erasing BT bonds', error.message); + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; + console.error('Error erasing BT bonds:', error); + Alert.alert( + 'Error erasing BT bonds', + `Failed to erase Bluetooth bonds: ${errorMessage}. Please try again.`, + ); } setIsErasingBonds(false); };
91-105: Consider adding loading state feedback for BT logs copy.The "Copy BT logs" button could benefit from loading state feedback similar to the "Erase BT bonds" button.
Apply this diff to add loading state:
+ const [isCopyingLogs, setIsCopyingLogs] = useState(false); + <Button onPress={() => { + setIsCopyingLogs(true); copyToClipboard(logs.join('\n')); + setIsCopyingLogs(false); }} + isLoading={isCopyingLogs} + isDisabled={isCopyingLogs} > Copy BT logs </Button>suite-native/bluetooth/src/hooks/useBluetoothPermissions.ts (1)
42-96: Consider extracting Android version check to a constant.The Android version check could be more maintainable if extracted to a named constant.
Apply this diff to improve readability:
+ const ANDROID_12_VERSION = 12; + const requestAndroidPermission = useCallback( async ({ checkOnly = false }: { checkOnly?: boolean } = {}) => { let hasError = false; const deviceOSVersion = Constants.systemVersion || 0; - if (deviceOSVersion >= 12) { + if (deviceOSVersion >= ANDROID_12_VERSION) { let result: { [PERMISSIONS.ANDROID.BLUETOOTH_CONNECT]: PermissionStatus; [PERMISSIONS.ANDROID.BLUETOOTH_SCAN]: PermissionStatus;packages/connect/src/device/Device.ts (1)
532-532: Improve debug logging.The added console logs could be more descriptive and use a consistent logging utility. Consider using the existing
_logutility for consistent logging across the codebase.Apply this diff to improve the logging:
-console.log('initialize', !!options.useCardanoDerivation); +_log.debug('initialize: starting', { useCardanoDerivation: !!options.useCardanoDerivation }); -console.log('initialiaze getFeatures'); +_log.debug('initialize: getting features'); -console.log('initialiaze getFeatures done', message); +_log.debug('initialize: features received', message);Also applies to: 730-730, 732-732
packages/protobuf/src/messages.ts (1)
19-22: Define explicit object shapes for message types.The empty object types
{}forEraseBondsandUnpairshould be replaced with explicit object shapes to improve type safety and follow best practices.Apply this diff to define explicit object shapes:
-export type EraseBonds = {}; +export type EraseBonds = { + // Add any required fields or leave empty with a comment + readonly __type?: 'EraseBonds'; +}; -export type Unpair = {}; +export type Unpair = { + // Add any required fields or leave empty with a comment + readonly __type?: 'Unpair'; +};🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 21-21: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/protobuf/messages.json (1)
6934-6935: Consider documenting the purpose of these message types.The new message types
EraseBondsandUnpairare added with empty fields, which is fine for command messages. However, consider adding documentation to explain:
- The purpose of each message type
- When they should be used
- Any prerequisites or side effects
📜 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 (47)
packages/connect/src/api/eraseBonds.ts(1 hunks)packages/connect/src/api/index.ts(1 hunks)packages/connect/src/device/Device.ts(3 hunks)packages/connect/src/factory.ts(1 hunks)packages/connect/src/types/api/eraseBonds.ts(1 hunks)packages/connect/src/types/api/index.ts(2 hunks)packages/protobuf/messages.json(2 hunks)packages/protobuf/src/messages-schema.ts(2 hunks)packages/protobuf/src/messages.ts(2 hunks)packages/transport-native-ble/package.json(1 hunks)packages/transport-native-ble/src/api/bleApi.ts(1 hunks)packages/transport-native-ble/src/api/nativeBleManager.ts(1 hunks)packages/transport-native-ble/src/index.ts(1 hunks)packages/transport-native-ble/src/logs.ts(1 hunks)packages/transport-native-ble/src/nativeTransportBLE.ts(1 hunks)packages/transport-native-ble/tsconfig.json(1 hunks)submodules/trezor-common(1 hunks)suite-common/connect-init/src/connectInitThunks.ts(1 hunks)suite-common/test-utils/src/extraDependenciesMock.ts(1 hunks)suite-native/app/.env.development(1 hunks)suite-native/app/app.config.ts(3 hunks)suite-native/app/eas.json(2 hunks)suite-native/app/package.json(1 hunks)suite-native/app/src/App.tsx(2 hunks)suite-native/atoms/src/DiscreetText/DiscreetCanvas.tsx(1 hunks)suite-native/bluetooth/package.json(1 hunks)suite-native/bluetooth/src/components/BluetoothAdapterStateManager.tsx(1 hunks)suite-native/bluetooth/src/components/BluetoothPermissionError.tsx(1 hunks)suite-native/bluetooth/src/components/DevicesScanner.tsx(1 hunks)suite-native/bluetooth/src/components/ScannedDeviceItem.tsx(1 hunks)suite-native/bluetooth/src/featureFlag.ts(1 hunks)suite-native/bluetooth/src/hooks/useBluetoothAdapterState.ts(1 hunks)suite-native/bluetooth/src/hooks/useBluetoothPermissions.ts(1 hunks)suite-native/bluetooth/src/hooks/useBluetoothState.ts(1 hunks)suite-native/bluetooth/src/index.ts(1 hunks)suite-native/bluetooth/tsconfig.json(1 hunks)suite-native/device/src/utils.ts(1 hunks)suite-native/module-authorize-device/package.json(1 hunks)suite-native/module-authorize-device/src/components/connect/ConnectDeviceScreenView.tsx(1 hunks)suite-native/module-authorize-device/src/screens/connect/ConnectAndUnlockDeviceScreen.tsx(2 hunks)suite-native/module-authorize-device/src/screens/connect/ConnectingDeviceScreen.tsx(1 hunks)suite-native/module-dev-utils/package.json(2 hunks)suite-native/module-dev-utils/src/components/BluetoothToggle.tsx(1 hunks)suite-native/module-dev-utils/src/screens/DevUtilsScreen.tsx(4 hunks)suite-native/receive/src/components/DeviceScreenContent.tsx(3 hunks)suite-native/state/package.json(2 hunks)suite-native/state/src/extraDependencies.ts(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- packages/transport-native-ble/tsconfig.json
- suite-native/atoms/src/DiscreetText/DiscreetCanvas.tsx
- packages/transport-native-ble/src/index.ts
- suite-native/module-authorize-device/src/components/connect/ConnectDeviceScreenView.tsx
- suite-native/bluetooth/src/index.ts
- packages/transport-native-ble/package.json
- suite-native/bluetooth/package.json
- submodules/trezor-common
- suite-common/test-utils/src/extraDependenciesMock.ts
- suite-native/bluetooth/tsconfig.json
- suite-common/connect-init/src/connectInitThunks.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/protobuf/src/messages.ts
[error] 19-19: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 21-21: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (34)
suite-native/module-authorize-device/package.json (1)
21-21: LGTM! The dependency addition aligns with PR objectives.The addition of
@suite-native/bluetoothis necessary for integrating Bluetooth device discovery and connection capabilities into the device authorization flow.suite-native/app/package.json (2)
112-112: Verify the latest version of react-native-ble-plx.The library choice aligns with the PR objectives, particularly in reducing native dependencies and potential conflicts. However, let's verify the version for any security updates.
✅ Verification successful
✓ react-native-ble-plx version and security verified
The package is using the latest version (3.4.0) and has no known security vulnerabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for latest version and security advisories for react-native-ble-plx. # Check NPM for latest version echo "Latest version from NPM:" npm view react-native-ble-plx version # Check for security advisories echo -e "\nSecurity advisories from GitHub:" gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "react-native-ble-plx") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 539
117-117: Verify the latest version of react-native-permissions.The library is essential for handling Bluetooth permissions as required by the feature flag implementation. Let's verify the version for any security updates.
✅ Verification successful
Current version ^5.2.3 is secure and up-to-date
A patch update (5.2.4) is available but not critical. No security vulnerabilities found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for latest version and security advisories for react-native-permissions. # Check NPM for latest version echo "Latest version from NPM:" npm view react-native-permissions version # Check for security advisories echo -e "\nSecurity advisories from GitHub:" gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "react-native-permissions") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 547
suite-native/module-authorize-device/src/screens/connect/ConnectingDeviceScreen.tsx (1)
20-21: 🛠️ Refactor suggestionConsider implementing a proper fix for the BT development kit freeze issue.
While commenting out the back button handling works as a temporary solution for the BT development kit, this could lead to inconsistent navigation behavior. Consider investigating and implementing a proper fix that handles both BT and non-BT scenarios correctly.
Apply this diff to add a TODO comment for tracking this technical debt:
// comment this out so we can dismiss the screen by hardware back button because on BT devkit it will freeze there otherwise + // TODO: Investigate and fix the screen freeze issue with BT development kit to restore proper back button handling // useHandleHardwareBackNavigation();Let's verify if this issue is being tracked:
packages/transport-native-ble/src/api/bleApi.ts (2)
29-29: Remove unuseddevicesarray or incorporate it if necessary.According to previous feedback, this array is unused. If it’s genuinely unneeded, removing it would reduce clutter.
73-84: Confirm device type assumption.
devicesToDescriptors()unconditionally usesDEVICE_TYPE.TypeT2. If Type T1 or other device types ever need BLE, you may need logic to distinguish them instead of forcing Type T2.packages/transport-native-ble/src/api/nativeBleManager.ts (3)
34-35: Document or justify the chosen MTU size.Reusing MTU 247 might mirror Trezor’s desktop standard, but it’s not self-evident. Consider adding a comment explaining why 247 was selected and any known limits on Android/iOS.
553-570: Surface errors when writing fails.Currently, write errors are only printed. You may want to rethrow them or return a rejected promise to ensure the application flow can handle the failure appropriately.
140-155: Consider scanning lifecycle implications.
scanDevices()andstopDeviceScan()updatethis.devicesScanList. If scanning or connecting occurs concurrently, ensure the list remains consistent. Locking or carefully orchestrating these calls might prevent race conditions.packages/connect/src/types/api/eraseBonds.ts (1)
1-4: LGTM! Type declaration is well-defined.The type declaration follows TypeScript best practices and maintains consistency with other API methods.
suite-native/bluetooth/src/hooks/useBluetoothState.ts (1)
5-16: LGTM! Hook implementation follows React best practices.The hook correctly uses
useSyncExternalStorefor external state synchronization, ensuring proper subscription cleanup and state updates.packages/transport-native-ble/src/nativeTransportBLE.ts (1)
9-19: LGTM! Constructor implementation is clean and follows best practices.The constructor correctly initializes the transport with the BLE API and logger.
packages/connect/src/api/eraseBonds.ts (1)
21-23: Resolve TODO and document the default behavior.The TODO comment indicates uncertainty about the default behavior. This should be resolved before finalizing the implementation.
Please clarify:
- Should erasing all bonds be the default behavior?
- What are the implications of each choice?
suite-native/device/src/utils.ts (1)
14-14: Verify the minimal firmware version for T3W1 devices.Setting the minimal firmware version to
[0, 0, 0]effectively disables version checking for T3W1 devices. While this might be acceptable for a POC, it could lead to compatibility issues if not properly constrained before production release.Please confirm if this is intentional for the POC phase and if there are plans to update it with proper version constraints before production release.
suite-native/bluetooth/src/components/BluetoothAdapterStateManager.tsx (2)
25-25: Remove unnecessary curly braces.-return <AlertBox title={'Bluetooth Unsupported on this device'} variant="error" />; +return <AlertBox title="Bluetooth Unsupported on this device" variant="error" />;
39-41:⚠️ Potential issueFix typo in error message and remove curly braces.
There's a typo in the error message ("turn of" should be "turn on") and unnecessary curly braces.
- title={'Bluetooth is turned off. Please turn of Bluetooth to continue.'} + title="Bluetooth is turned off. Please turn on Bluetooth to continue." variant="error"Likely invalid or redundant comment.
suite-native/module-authorize-device/src/screens/connect/ConnectAndUnlockDeviceScreen.tsx (1)
91-95: LGTM! Clean implementation of conditional rendering.The conditional rendering based on Bluetooth state is well-implemented, providing a clear separation between Bluetooth and USB connection flows.
packages/connect/src/api/index.ts (1)
29-29: LGTM: New eraseBonds export added.The addition of the
eraseBondsexport aligns with the PR objectives for Bluetooth transport functionality.Also applies to: 29-29
suite-native/module-dev-utils/src/screens/DevUtilsScreen.tsx (1)
81-87: LGTM! Clear and informative Bluetooth status display.The implementation provides good visibility into the Bluetooth state through environment variables and feature flags.
suite-native/bluetooth/src/hooks/useBluetoothPermissions.ts (2)
14-18: LGTM! Well-defined error types.The enum provides clear categorization of different Bluetooth permission errors.
113-129: Verify app state subscription cleanup.The cleanup function correctly removes the app state subscription, but we should verify if there are any potential memory leaks from other resources.
Run the following script to check for potential memory leaks in app state subscriptions:
✅ Verification successful
AppState subscription cleanup is properly implemented ✅
The cleanup function correctly removes the AppState subscription, and there are no other resources that could cause memory leaks. The implementation follows the same pattern used consistently throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for AppState subscriptions and their cleanup # Test: Look for AppState.addEventListener usage and verify cleanup in useEffect/useLayoutEffect hooks rg -A 10 "AppState\.addEventListener"Length of output: 6178
suite-native/receive/src/components/DeviceScreenContent.tsx (1)
37-37: LGTM! Improved font imports.The change from relative to absolute imports for font files improves maintainability and follows best practices.
Also applies to: 54-54
packages/connect/src/factory.ts (1)
113-114: LGTM! Consistent method implementation.The
eraseBondsmethod follows the established pattern of other methods in the factory.suite-native/app/app.config.ts (2)
149-157: LGTM! Bluetooth plugins are conditionally enabled.The changes correctly add Bluetooth-related plugins (
react-native-ble-plxandreact-native-permissions) only whenEXPO_PUBLIC_BLUETOOTH_ENABLEDis true, allowing for controlled rollout of Bluetooth functionality.
238-241: LGTM! iOS Bluetooth permissions are properly configured.The iOS permission messages clearly explain why Bluetooth access is needed to connect to Trezor devices.
packages/connect/src/types/api/index.ts (1)
221-223: LGTM! TheeraseBondsmethod is properly integrated.The method is correctly added to the
TrezorConnectinterface with proper API documentation URL.packages/protobuf/src/messages.ts (1)
2295-2296: LGTM! Message types are properly integrated.The new message types are correctly added to the
MessageTypeinterface.packages/protobuf/src/messages-schema.ts (2)
2459-2461: LGTM! New message types for Bluetooth functionality.The empty object types
EraseBondsandUnpairare correctly defined as command messages without parameters.Also applies to: 2462-2464
3546-3547: LGTM! Message types properly integrated.The new types are correctly added to the
MessageTypeexport.suite-native/app/.env.development (1)
4-4: LGTM! Environment variable for Bluetooth feature flag.The
EXPO_PUBLIC_BLUETOOTH_ENABLEDvariable follows Expo's naming convention and enables Bluetooth functionality in development.suite-native/module-dev-utils/package.json (1)
20-20: LGTM! Dependencies for Bluetooth support added.The required Bluetooth-related dependencies are correctly added with workspace versioning:
@suite-native/bluetooth: For Bluetooth functionality@trezor/transport-native-ble: For Bluetooth transport layerAlso applies to: 35-35
suite-native/state/package.json (1)
25-25: LGTM! Dependencies for Bluetooth support added.The required Bluetooth-related dependencies are correctly added with workspace versioning:
@suite-native/bluetooth: For Bluetooth functionality@trezor/transport-native-ble: For Bluetooth transport layerAlso applies to: 38-38
suite-native/app/eas.json (2)
11-12: LGTM! Good security practice to enable Bluetooth only in development.The configuration enables Bluetooth in the development environment while keeping it disabled by default in production. This is a good security practice as it allows testing the feature while preventing unintended Bluetooth access in production until the feature is fully tested.
42-43: LGTM! Bluetooth disabled by default in production.The configuration correctly disables Bluetooth in the production environment by default. This is a good security practice to prevent unintended Bluetooth access until the feature is fully tested and ready for production use.
| public async write(path: string, buffer: Buffer) { | ||
| debugLog('write', buffer); | ||
| const device = nativeBleManager.findDevice(path); | ||
| if (!device) { | ||
| return this.error({ error: ERRORS.DEVICE_NOT_FOUND }); | ||
| } | ||
| try { | ||
| const base64Chunk = buffer.toString('base64'); | ||
| await nativeBleManager.write(path, base64Chunk); | ||
|
|
||
| return this.success(undefined); | ||
| } catch (err) { | ||
| return this.error({ error: ERRORS.INTERFACE_DATA_TRANSFER, message: err.message }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle large writes in chunks.
write() sends a single base64-encoded buffer. If the buffer exceeds BLE’s allowable MTU minus overhead (which might be below 244), consider splitting the data into multiple writes. This prevents dropped packets or partial writes on some BLE implementations.
|
|
||
| export const isBluetoothBuild = process.env.EXPO_PUBLIC_BLUETOOTH_ENABLED === 'true'; | ||
|
|
||
| console.log('isBluetoothBuild', isBluetoothBuild); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log statement.
Console.log statements should not be included in production code.
-console.log('isBluetoothBuild', isBluetoothBuild);| export const setBluetoothEnabled = (value: boolean) => { | ||
| unecryptedJotaiStorage.set('bluetoothEnabled', value); | ||
| RNRestart.restart(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and user confirmation for app restart.
The current implementation:
- Lacks error handling for storage operations
- Forces an immediate app restart which might be disruptive to users
Consider:
- Adding try-catch for storage operations
- Adding a user confirmation dialog before restart
export const setBluetoothEnabled = (value: boolean) => {
- unecryptedJotaiStorage.set('bluetoothEnabled', value);
- RNRestart.restart();
+ try {
+ unecryptedJotaiStorage.set('bluetoothEnabled', value);
+ // Show confirmation dialog before restart
+ Alert.alert(
+ 'Restart Required',
+ 'The app needs to restart to apply Bluetooth settings. Restart now?',
+ [
+ { text: 'Cancel', style: 'cancel' },
+ { text: 'Restart', onPress: () => RNRestart.restart() }
+ ]
+ );
+ } catch (error) {
+ console.error('Failed to update Bluetooth settings:', error);
+ // Handle error (e.g., show error message to user)
+ }
};📝 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 setBluetoothEnabled = (value: boolean) => { | |
| unecryptedJotaiStorage.set('bluetoothEnabled', value); | |
| RNRestart.restart(); | |
| }; | |
| export const setBluetoothEnabled = (value: boolean) => { | |
| try { | |
| unecryptedJotaiStorage.set('bluetoothEnabled', value); | |
| // Show confirmation dialog before restart | |
| Alert.alert( | |
| 'Restart Required', | |
| 'The app needs to restart to apply Bluetooth settings. Restart now?', | |
| [ | |
| { text: 'Cancel', style: 'cancel' }, | |
| { text: 'Restart', onPress: () => RNRestart.restart() } | |
| ] | |
| ); | |
| } catch (error) { | |
| console.error('Failed to update Bluetooth settings:', error); | |
| // Handle error (e.g., show error message to user) | |
| } | |
| }; |
| const turnOnBluetoothAdapter = async () => { | ||
| await nativeBleManager.bleManager.enable(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for enable operation.
The turnOnBluetoothAdapter function should handle potential failures when enabling Bluetooth.
const turnOnBluetoothAdapter = async () => {
- await nativeBleManager.bleManager.enable();
+ try {
+ await nativeBleManager.bleManager.enable();
+ } catch (error) {
+ console.error('Failed to enable Bluetooth:', error);
+ throw error; // Re-throw to let caller handle the error
+ }
};📝 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.
| const turnOnBluetoothAdapter = async () => { | |
| await nativeBleManager.bleManager.enable(); | |
| }; | |
| const turnOnBluetoothAdapter = async () => { | |
| try { | |
| await nativeBleManager.bleManager.enable(); | |
| } catch (error) { | |
| console.error('Failed to enable Bluetooth:', error); | |
| throw error; // Re-throw to let caller handle the error | |
| } | |
| }; |
| useEffect(() => { | ||
| nativeBleManager.bleManager.state().then(newState => { | ||
| setState(newState); | ||
| }); | ||
|
|
||
| const subscription = nativeBleManager.bleManager.onStateChange(newState => { | ||
| setState(newState); | ||
| }); | ||
|
|
||
| return () => { | ||
| subscription.remove(); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for async operations.
The async operations for state retrieval and subscription could fail silently. Consider adding error handling to provide better feedback.
useEffect(() => {
- nativeBleManager.bleManager.state().then(newState => {
- setState(newState);
- });
+ nativeBleManager.bleManager.state()
+ .then(newState => {
+ setState(newState);
+ })
+ .catch(error => {
+ console.error('Failed to get Bluetooth state:', error);
+ setState(BluetoothAdapterState.Unknown);
+ });
const subscription = nativeBleManager.bleManager.onStateChange(newState => {
setState(newState);
});
return () => {
subscription.remove();
};
}, []);📝 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.
| useEffect(() => { | |
| nativeBleManager.bleManager.state().then(newState => { | |
| setState(newState); | |
| }); | |
| const subscription = nativeBleManager.bleManager.onStateChange(newState => { | |
| setState(newState); | |
| }); | |
| return () => { | |
| subscription.remove(); | |
| }; | |
| }, []); | |
| useEffect(() => { | |
| nativeBleManager.bleManager.state() | |
| .then(newState => { | |
| setState(newState); | |
| }) | |
| .catch(error => { | |
| console.error('Failed to get Bluetooth state:', error); | |
| setState(BluetoothAdapterState.Unknown); | |
| }); | |
| const subscription = nativeBleManager.bleManager.onStateChange(newState => { | |
| setState(newState); | |
| }); | |
| return () => { | |
| subscription.remove(); | |
| }; | |
| }, []); |
| // const transportsPerDeviceType = { | ||
| // device: Platform.select({ | ||
| // ios: ['BridgeTransport'], | ||
| // android: [NativeUsbTransport], | ||
| // }), | ||
| // emulator: ['BridgeTransport'], | ||
| // } as const; | ||
|
|
||
| // let transports = transportsPerDeviceType[deviceType]; | ||
|
|
||
| const transports = transportsPerDeviceType[deviceType]; | ||
| // if (isBluetoothEnabled) { | ||
| const transports = [NativeTransportBLE]; | ||
| // } |
There was a problem hiding this comment.
💡 Codebase verification
Platform-specific transport selection is crucial for device compatibility
The codebase reveals multiple transport implementations (Bridge, USB, BLE) each designed for specific platform capabilities. Removing platform-specific transport selection in favor of BLE-only approach could indeed cause connectivity issues. The suggested changes to maintain platform-specific transports while adding BLE as an additional option are valid and align with the existing architecture.
🔗 Analysis chain
Consider platform-specific transport fallbacks.
The current implementation forces BLE transport without considering platform capabilities or providing fallbacks. This could lead to connectivity issues on devices or platforms where BLE is not available.
Consider:
- Keeping platform-specific transport selection
- Using BLE as an additional transport rather than the only option
- Implementing fallback mechanism
-// const transportsPerDeviceType = {
-// device: Platform.select({
-// ios: ['BridgeTransport'],
-// android: [NativeUsbTransport],
-// }),
-// emulator: ['BridgeTransport'],
-// } as const;
-
-// let transports = transportsPerDeviceType[deviceType];
-
-// if (isBluetoothEnabled) {
-const transports = [NativeTransportBLE];
-// }
+const baseTransports = Platform.select({
+ ios: ['BridgeTransport'],
+ android: [NativeUsbTransport],
}) ?? [];
+
+const transports = isBluetoothEnabled
+ ? [...baseTransports, NativeTransportBLE]
+ : baseTransports;🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for transport-related files and implementations
echo "=== Transport-related files ==="
fd -e ts -e js transport -i
echo -e "\n=== Transport implementations ==="
rg -i "class.*transport" -A 5
echo -e "\n=== Platform-specific transport configurations ==="
rg "Platform.select.*transport" -B 2 -A 2
echo -e "\n=== Transport-related documentation ==="
fd README -t f -X grep -l -i "transport" {} \;
Length of output: 9863
| const connectDevice = async () => { | ||
| setIsConnecting(true); | ||
| try { | ||
| await nativeBleManager.connectDeviceWithRetry({ | ||
| deviceOrId: bleDevice, | ||
| maxRetries: 2, | ||
| }); | ||
| } catch (error) { | ||
| alert('Error connecting to device'); | ||
| Alert.alert('Error connecting to device', error?.message, [{ text: 'OK' }]); | ||
| } | ||
| setIsConnecting(false); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in connectDevice.
The current error handling could be more informative and consistent:
- Using both
alertandAlert.alertis redundant - The error message could be more specific
- Error types should be properly handled
const connectDevice = async () => {
setIsConnecting(true);
try {
await nativeBleManager.connectDeviceWithRetry({
deviceOrId: bleDevice,
maxRetries: 2,
});
} catch (error) {
- alert('Error connecting to device');
- Alert.alert('Error connecting to device', error?.message, [{ text: 'OK' }]);
+ Alert.alert(
+ 'Connection Failed',
+ `Unable to connect to ${bleDevice.name}: ${error?.message || 'Unknown error'}`,
+ [{ text: 'OK' }]
+ );
}
setIsConnecting(false);
};📝 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.
| const connectDevice = async () => { | |
| setIsConnecting(true); | |
| try { | |
| await nativeBleManager.connectDeviceWithRetry({ | |
| deviceOrId: bleDevice, | |
| maxRetries: 2, | |
| }); | |
| } catch (error) { | |
| alert('Error connecting to device'); | |
| Alert.alert('Error connecting to device', error?.message, [{ text: 'OK' }]); | |
| } | |
| setIsConnecting(false); | |
| }; | |
| const connectDevice = async () => { | |
| setIsConnecting(true); | |
| try { | |
| await nativeBleManager.connectDeviceWithRetry({ | |
| deviceOrId: bleDevice, | |
| maxRetries: 2, | |
| }); | |
| } catch (error) { | |
| Alert.alert( | |
| 'Connection Failed', | |
| `Unable to connect to ${bleDevice.name}: ${error?.message || 'Unknown error'}`, | |
| [{ text: 'OK' }] | |
| ); | |
| } | |
| setIsConnecting(false); | |
| }; |
| // Rerender every second to update the "Last seen" text | ||
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| setRerender(prev => prev + 1); | ||
| }, 1000); | ||
|
|
||
| return () => { | ||
| clearInterval(interval); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize rerender interval implementation.
The current implementation rerenders every second which could impact performance. Consider using a more efficient approach.
-useEffect(() => {
- const interval = setInterval(() => {
- setRerender(prev => prev + 1);
- }, 1000);
-
- return () => {
- clearInterval(interval);
- };
-}, []);
+const [lastSeenText, setLastSeenText] = useState('');
+
+useEffect(() => {
+ const updateLastSeen = () => {
+ const seconds = Math.floor((Date.now() - device.lastSeenTimestamp) / 1000);
+ setLastSeenText(`Last seen: ${seconds}s ago`);
+ };
+
+ updateLastSeen();
+ const interval = setInterval(updateLastSeen, 5000); // Update every 5 seconds
+
+ return () => clearInterval(interval);
+}, [device.lastSeenTimestamp]);Committable suggestion skipped: line range outside the PR's diff.
| const scanDevices = () => { | ||
| nativeBleManager.scanDevices( | ||
| _newlyScannedDevices => {}, | ||
| _error => { | ||
| stopScanning(); | ||
| }, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper callback handling in scanDevices.
The current implementation has empty callbacks which could miss important events or errors.
const scanDevices = () => {
nativeBleManager.scanDevices(
- _newlyScannedDevices => {},
- _error => {
+ newlyScannedDevices => {
+ // Log or handle newly discovered devices
+ console.debug('New devices discovered:', newlyScannedDevices);
+ },
+ error => {
+ console.error('Scan error:', error);
stopScanning();
},
);
};📝 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.
| const scanDevices = () => { | |
| nativeBleManager.scanDevices( | |
| _newlyScannedDevices => {}, | |
| _error => { | |
| stopScanning(); | |
| }, | |
| ); | |
| }; | |
| const scanDevices = () => { | |
| nativeBleManager.scanDevices( | |
| newlyScannedDevices => { | |
| // Log or handle newly discovered devices | |
| console.debug('New devices discovered:', newlyScannedDevices); | |
| }, | |
| error => { | |
| console.error('Scan error:', error); | |
| stopScanning(); | |
| }, | |
| ); | |
| }; |
| <Text> | ||
| hasBluetoothPermissions: {hasBluetoothPermissions ? 'true' : 'false'} {'\n'} | ||
| bluetoothPermissionError: {bluetoothPermissionError} {'\n'} | ||
| bluetoothAdapterState: {bluetoothAdapterState} {'\n'} | ||
| bluetoothState: {bluetoothState.status} {'\n'} | ||
| </Text> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug information from production UI.
The component displays debug information that should not be visible in production.
return (
<VStack style={applyStyle(containerStyle)} spacing="large">
- <Text>
- hasBluetoothPermissions: {hasBluetoothPermissions ? 'true' : 'false'} {'\n'}
- bluetoothPermissionError: {bluetoothPermissionError} {'\n'}
- bluetoothAdapterState: {bluetoothAdapterState} {'\n'}
- bluetoothState: {bluetoothState.status} {'\n'}
- </Text>
+ {__DEV__ && (
+ <Text>
+ hasBluetoothPermissions: {hasBluetoothPermissions ? 'true' : 'false'} {'\n'}
+ bluetoothPermissionError: {bluetoothPermissionError} {'\n'}
+ bluetoothAdapterState: {bluetoothAdapterState} {'\n'}
+ bluetoothState: {bluetoothState.status} {'\n'}
+ </Text>
+ )}📝 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.
| <Text> | |
| hasBluetoothPermissions: {hasBluetoothPermissions ? 'true' : 'false'} {'\n'} | |
| bluetoothPermissionError: {bluetoothPermissionError} {'\n'} | |
| bluetoothAdapterState: {bluetoothAdapterState} {'\n'} | |
| bluetoothState: {bluetoothState.status} {'\n'} | |
| </Text> | |
| {__DEV__ && ( | |
| <Text> | |
| hasBluetoothPermissions: {hasBluetoothPermissions ? 'true' : 'false'} {'\n'} | |
| bluetoothPermissionError: {bluetoothPermissionError} {'\n'} | |
| bluetoothAdapterState: {bluetoothAdapterState} {'\n'} | |
| bluetoothState: {bluetoothState.status} {'\n'} | |
| </Text> | |
| )} |
Description
POC of BT transport layer in Suite Mobile. Implemented using
react-native-ble-plxlib which is used by all crypto apps that I explored. It's also nice for 3rd parties because they won't need to add extra native dependency and it will also help us avoid some conflicts (hopefully) that could occur when multiple libraries for BT access are used.Also please keep in mind this is very first version, even there is lot edge cases handled, some stuff is missing for example erasing bonds (compared to @szymonlesisz PR). I also did not encountered any packet duplication (but I guess it can happen), but I think it's fine to omit it now and wait for new protocol which will solve it anyway.
Video: https://satoshilabs.slack.com/files/U02RLDSFCPP/F071BCP430S/screen-20240430-154355.mp4
Summary of changes
@trezor/transport-native-blenativeBleManager.tswhich is acting like desktop api in @szymonlesisz branch for desktop. It must be single instance and we must be able to import this instance in app UI to handle stuff like connecting, scanning etc.2.
bleApi.ts- it's basically BT version ofusbApi@suite-native/appEXPO_PUBLIC_BLUETOOTH_ENABLEDenv variable - this needs to betruebefore build to include some native code for requesting permission for BT (libraries will be included in all types build anyway, but at least it won't show extra permission in stores). By default this will be enabled for Debug and Develop builds so anyone can turn ON BT for testing.@suite-native/bluetooth@suite-native/bluetooth- This is special kind of static feature flag because it must be persisted because app must be restarted when it's changed. Once we will support multi-transport restart woudn't be neccessary.Related Issue
Resolve #11751
Screenshots: