Fix/ble connect #402
Fix/ble connect #402
Conversation
WalkthroughThis pull request involves a comprehensive version update across multiple packages within the OneKey SDK ecosystem. The primary change is a version bump from Changes
Sequence DiagramsequenceDiagram
participant Client
participant ReactNativeBleTransport
participant BleTransport
Client->>ReactNativeBleTransport: acquire(input)
ReactNativeBleTransport->>ReactNativeBleTransport: release(uuid)
ReactNativeBleTransport->>BleTransport: new instance
BleTransport-->>ReactNativeBleTransport: transport created
ReactNativeBleTransport-->>Client: return transport
The sequence diagram illustrates the enhanced connection acquisition process, showing the additional step of releasing existing resources before creating a new transport instance. 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 (
|
|
🚨 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 a critical CVE?Contains a Critical Common Vulnerability and Exposure (CVE). Remove or replace dependencies that include known critical CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. 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
|
|
Report too large to display inline |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
packages/connect-examples/electron-example/package.json(1 hunks)packages/connect-examples/expo-example/package.json(2 hunks)packages/core/package.json(2 hunks)packages/hd-ble-sdk/package.json(2 hunks)packages/hd-common-connect-sdk/package.json(2 hunks)packages/hd-transport-http/package.json(2 hunks)packages/hd-transport-lowlevel/package.json(2 hunks)packages/hd-transport-react-native/package.json(2 hunks)packages/hd-transport-react-native/src/index.ts(1 hunks)packages/hd-transport-webusb/package.json(2 hunks)packages/hd-transport/package.json(1 hunks)packages/hd-web-sdk/package.json(2 hunks)packages/shared/package.json(1 hunks)
🔇 Additional comments (8)
packages/hd-transport-lowlevel/package.json (1)
3-3: Verify alpha version compatibilityThe package and its dependencies are moving to an alpha version. Ensure all consumers are prepared for potential breaking changes.
Also applies to: 22-23
✅ Verification successful
Alpha version update is safe to proceed
The package is properly imported and managed through package.json files. No hardcoded version references found in the codebase. Package managers will handle version constraints appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any hardcoded version references rg -g '!package*.json' -g '!yarn.lock' '1\.0\.18'Length of output: 51
Script:
#!/bin/bash # Check for package references in code rg -g '!package*.json' -g '!yarn.lock' 'hd-transport-lowlevel' # Find other package.json files that might depend on this fd package.json --exec grep -l "hd-transport-lowlevel" {} # Look for dynamic requires or imports ast-grep --pattern 'require("hd-transport-lowlevel")' ast-grep --pattern 'from "hd-transport-lowlevel"'Length of output: 673
packages/shared/package.json (1)
3-3: LGTM!Version bump is consistent with the project-wide update.
packages/hd-ble-sdk/package.json (1)
3-3: Version bump to alpha needs changelog entryMoving to an alpha version (1.0.19-alpha.0) suggests significant changes. This aligns with the PR title mentioning BLE connection fixes.
Please add a changelog entry describing:
- What BLE connection issues were fixed
- Why an alpha release is needed
- Breaking changes, if any
Also applies to: 23-25
packages/hd-transport-webusb/package.json (1)
3-3: Version alignment looks goodPackage version and dependencies are correctly aligned with the alpha release.
Also applies to: 23-24
packages/hd-transport-react-native/package.json (1)
3-3: Verify BLE dependencies compatibilitySince this PR fixes BLE connectivity:
- Consider updating
react-native-ble-plx(3.0.1) andreact-native-ble-manager(8.1.0)- Test compatibility with the alpha release
Also applies to: 22-25
packages/hd-transport-http/package.json (1)
3-3: Check axios security updatesPackage versions are aligned correctly. However, axios 0.27.2 might need an update.
Also applies to: 27-29
packages/hd-common-connect-sdk/package.json (1)
3-3: Dependencies alignment is correctAll dependencies are properly aligned to version 1.0.19-alpha.0.
Also applies to: 23-27
packages/hd-web-sdk/package.json (1)
3-3: Verify necessity of web SDK updatesThe PR focuses on BLE fixes, but web SDK changes are included. Let's verify if these changes are necessary.
Check for related changes:
Also applies to: 24-27
✅ Verification successful
Version update is necessary for transport layer consistency
The web SDK depends on core transport packages and shares common interfaces. Version bump maintains consistency across the package ecosystem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for BLE-related changes in web SDK rg -l "BLE|Bluetooth" packages/hd-web-sdk/Length of output: 44
Script:
#!/bin/bash # Check package dependencies and imports cat packages/hd-web-sdk/package.json # Look for transport-related imports rg -l "transport|HardwareSDK" packages/hd-web-sdk/src/Length of output: 1987
| // release transport before new transport instance | ||
| await this.release(uuid); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove redundant release call
This release call is redundant. The code already releases the transport at lines 352-357 if it exists in the cache.
- // release transport before new transport instance
- await this.release(uuid);
-
packages/hd-transport/package.json
Outdated
| "version": "1.0.19-alpha.0", | ||
| "description": "> TODO: description", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update package description before release
The description field still contains a TODO placeholder. Consider updating it before the 1.0.19 release.
packages/core/package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@onekeyfe/hd-core", | |||
| "version": "1.0.18", | |||
| "version": "1.0.19-alpha.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Version bump appears unnecessary
The core package only has package.json changes. No code changes found to justify the version bump to 1.0.19-alpha.0.
🔗 Analysis chain
Version bump needs clarification
The version bump to alpha suggests significant changes, but the PR title only mentions BLE fixes. Core package changes might have broader impact.
Let's check if there are actual code changes in the core package:
Also applies to: 28-29
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for code changes in the core package
git diff HEAD~1 packages/core/ -- ':!package.json'
Length of output: 89
Script:
#!/bin/bash
# List modified files in core package
git diff --name-status HEAD~1 packages/core/
# Show actual changes in core package (excluding package.json)
git diff HEAD~1 -- 'packages/core/*' ':!packages/core/package.json'
# Check for any new files
git ls-files --others --exclude-standard packages/core/
Length of output: 203
| @@ -2,7 +2,7 @@ | |||
| "name": "hardware-example", | |||
| "productName": "HardwareExample", | |||
| "executableName": "onekey-hardware-example", | |||
| "version": "1.0.18", | |||
| "version": "1.0.19-alpha.0", | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider keeping example version stable
Example packages don't typically need version bumps. Consider keeping it at 1.0.18 unless there are breaking changes.
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "expo-example", | |||
| "version": "1.0.18", | |||
| "version": "1.0.19-alpha.0", | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review version bump strategy
The wholesale version bump to 1.0.19-alpha.0 across all packages seems excessive for BLE-specific fixes. Consider:
- Only bump versions of directly affected packages
- Use patch version for targeted fixes
Also applies to: 3-3, 3-3, 3-3
| "@onekeyfe/hd-ble-sdk": "^1.0.19-alpha.0", | ||
| "@onekeyfe/hd-common-connect-sdk": "^1.0.19-alpha.0", | ||
| "@onekeyfe/hd-core": "^1.0.19-alpha.0", | ||
| "@onekeyfe/hd-web-sdk": "^1.0.19-alpha.0", |
There was a problem hiding this comment.
💡 Codebase verification
Add BLE-specific test coverage
The SDK update lacks test cases for the BLE connectivity fixes. Add tests to:
- Verify BLE connection handling
- Test BLE-specific error scenarios
- Validate fixed connectivity issues
🔗 Analysis chain
BLE SDK update aligns with PR purpose
The BLE SDK update matches the PR's focus on fixing BLE connectivity.
However, consider:
- Documenting the specific BLE fixes in the changelog
- Adding test cases for the fixed BLE connectivity issues
Let's check for related test updates:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for BLE-related test changes
rg -l "test.*ble|ble.*test" packages/
Length of output: 211
Script:
#!/bin/bash
# Look for bluetooth/test-related files with broader patterns
echo "=== Searching for Bluetooth-related tests ==="
rg -l "bluetooth|Bluetooth" packages/ | grep -i "test"
echo -e "\n=== Finding all test files ==="
fd "test" packages/ --type f | grep -iE "test\.(js|ts|jsx|tsx)$"
Length of output: 1690
Script:
#!/bin/bash
# Check device-related test content for BLE/Bluetooth testing
echo "=== Checking device test content ==="
rg -A 5 "describe|it\(" "packages/connect-examples/expo-example/src/testTools/functionalTesting/lockDevice/InitDeviceTest.tsx" "packages/connect-examples/expo-example/src/testTools/functionalTesting/lockDevice/LockDeviceTest.tsx"
echo -e "\n=== Checking transport tests ==="
rg -A 5 "describe|it\(" "packages/hd-transport/__tests__/build-receive.test.js"
Length of output: 885
Summary by CodeRabbit
Version Updates
1.0.19-alpha.0, including:Dependency Updates
Pre-release Preparation