-
Notifications
You must be signed in to change notification settings - Fork 25
Fix/update #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/update #584
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughThis PR bumps all packages from version 1.1.15 to 1.1.16-alpha.1 and introduces blur data upload support. Changes include a new BlurRequest message type, updated resource upload handling with blur data fields, revised message version configuration fallbacks, and removal of device selection validation guards in an example application. Changes
Sequence DiagramsequenceDiagram
actor User
participant SDK
participant Device
participant UI
User->>SDK: requestResourceUpload(connectId, {main, thumbnail, blur})
SDK->>Device: ResourceUpload (file_name_no_ext, zoom_data_length, blur_data_length)
Device->>SDK: ResourceRequest / ZoomRequest / BlurRequest
rect rgb(200, 220, 240)
Note over SDK,Device: Data transfer loop
SDK->>SDK: getDataChunk (based on request type)
SDK->>SDK: updateProgress (increment uploadedBytes, log currentFile)
SDK->>Device: send data chunk
Device->>SDK: ResourceRequest / ZoomRequest / BlurRequest (next segment)
end
Device->>SDK: Success
SDK->>UI: uploadProgress: {totalBytes, uploadedBytes, complete}
UI->>User: Upload complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The majority of changes are repetitive version bumps across package.json files (low effort). The blur data feature introduces moderate complexity: it spans multiple files with type definitions, schema updates, and implementation logic, but follows a consistent pattern. The PlaygroundExecutor change is a simple guard removal. The DeviceUploadResource changes require careful review of progress tracking and data chunking logic but are well-contained. Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/core/src/data-manager/MessagesConfig.ts (1)
21-26: Same fallback concern applies to model_touch.The 0.0.0 version fallback to
latesthas the same compatibility risk as noted for model_mini. Ensure this version code only represents detection failures, not legitimate old firmware.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
packages/connect-examples/electron-example/package.json(2 hunks)packages/connect-examples/expo-example/package.json(2 hunks)packages/connect-examples/expo-example/src/components/PlaygroundExecutor.tsx(1 hunks)packages/connect-examples/expo-playground/package.json(2 hunks)packages/core/package.json(2 hunks)packages/core/src/api/device/DeviceUploadResource.ts(4 hunks)packages/core/src/data-manager/MessagesConfig.ts(1 hunks)packages/core/src/data/messages/messages.json(3 hunks)packages/core/src/types/api/deviceUploadResource.ts(1 hunks)packages/hd-ble-sdk/package.json(2 hunks)packages/hd-common-connect-sdk/package.json(2 hunks)packages/hd-transport-electron/package.json(2 hunks)packages/hd-transport-emulator/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-web-device/package.json(2 hunks)packages/hd-transport/package.json(1 hunks)packages/hd-transport/src/types/messages.ts(3 hunks)packages/hd-web-sdk/package.json(2 hunks)packages/shared/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build (22)
- GitHub Check: lint (22)
🔇 Additional comments (30)
packages/shared/package.json (1)
3-3: Version bump looks good.The pre-release version 1.1.16-alpha.1 aligns with the broader package updates across the monorepo and follows semantic versioning correctly.
packages/hd-ble-sdk/package.json (1)
3-3: Version bump consistent with coordinated release.Internal dependencies (@onekeyfe/*) correctly updated to 1.1.16-alpha.1.
Also applies to: 23-25
packages/connect-examples/expo-example/package.json (1)
3-3: Expo example: version and dependencies aligned.All @onekeyfe packages correctly bumped to 1.1.16-alpha.1.
Also applies to: 22-25
packages/core/package.json (1)
3-3: Core package: version and dependencies synchronized.Internal @onekeyfe deps correctly updated to 1.1.16-alpha.1.
Also applies to: 28-29
packages/hd-transport/package.json (1)
3-3: Transport base layer: version bump applied.No internal dependencies to coordinate.
packages/hd-transport-web-device/package.json (1)
3-3: Web device transport: version and dependencies aligned.Internal @onekeyfe packages correctly updated to 1.1.16-alpha.1 (both deps and devDeps).
Also applies to: 23-24, 27-27
packages/connect-examples/electron-example/package.json (1)
5-5: Electron example: version and transport dependency aligned.@onekeyfe/hd-transport-electron correctly updated to 1.1.16-alpha.1.
Also applies to: 25-25
packages/hd-web-sdk/package.json (1)
3-3: Web SDK: version and dependencies synchronized.All @onekeyfe packages correctly bumped to 1.1.16-alpha.1.
Also applies to: 24-27
packages/hd-transport-electron/package.json (1)
3-3: Electron transport: version and dependencies aligned.Internal @onekeyfe packages correctly updated to 1.1.16-alpha.1.
Also applies to: 28-30
packages/hd-transport-emulator/package.json (1)
3-3: Version and dependencies correctly bumped to alpha prerelease.All internal dependencies are aligned to the same prerelease version—good coordination across the monorepo.
Verify that your CI/CD pipeline publishes alpha versions with an appropriate tag (e.g.,
npm publish --tag alphaor--tag next) rather than the defaultlatesttag. Pre-release versions should be published with a tag other than "latest" to prevent them from becoming the default install.Also applies to: 27-28
packages/connect-examples/expo-playground/package.json (1)
3-3: Version and dependencies aligned with alpha release.All internal dependencies correctly updated to
1.1.16-alpha.1. Since this is a private example package, the prerelease publishing considerations don't apply here.Also applies to: 20-22
packages/hd-transport-lowlevel/package.json (1)
3-3: Coordinated alpha version bump applied consistently.Dependencies properly aligned to the prerelease version.
Also applies to: 22-23
packages/hd-transport-http/package.json (1)
3-3: Alpha release version and dependencies consistent.Internal dependencies properly bumped; external dependencies unchanged.
Also applies to: 27-28
packages/hd-transport-react-native/package.json (1)
3-3: Version bump and dependencies aligned to alpha release.All internal packages correctly updated to
1.1.16-alpha.1.Also applies to: 22-23
packages/hd-common-connect-sdk/package.json (1)
3-3: All internal dependencies correctly bumped to alpha version.This common SDK aggregates six transport packages—all properly aligned to
1.1.16-alpha.1.Verify that all referenced internal packages (especially
@onekeyfe/hd-transport-web-device) have been updated to1.1.16-alpha.1in their respective package.json files to ensure the entire dependency graph is consistent.Also applies to: 23-28
packages/core/src/data/messages/messages.json (3)
7534-7541: Solid field extension.The extra blur metadata slots neatly into
ResourceUploadwithout breaking old clients. Good call keeping the new fields optional.
7565-7576: BlurRequest mirrors ZoomRequest cleanly.Nice job cloning the existing chunked-transfer pattern so blur data can stream just like zoom data.
12389-12390: Enum wiring looks right.
MessageType_BlurRequest = 10032lands in the correct range and keeps the sequence tidy.packages/hd-transport/src/types/messages.ts (3)
2766-2766: LGTM! Blur data length field added consistently.The optional
blur_data_lengthfield mirrors the existingzoom_data_lengthpattern and maintains backward compatibility.
2775-2779: LGTM! BlurRequest type mirrors ZoomRequest structure.The type definition is consistent with the existing ZoomRequest pattern, using optional offset and required data_length for chunked transfers.
4647-4647: LGTM! MessageType mapping added correctly.BlurRequest properly integrated into the MessageType interface.
packages/core/src/api/device/DeviceUploadResource.ts (7)
19-26: LGTM! Data structures extended properly.The
blurDatafield follows the existing pattern, and theuploadProgresstracker is well-structured with clear type annotations.
61-92: LGTM! Initialization and validation implemented correctly.The blur data initialization follows the established pattern, validation is proper, and progress tracking calculation includes all data sources.
99-103: LGTM! Safe data chunking helper.Bounds checking with
Math.minprevents reading past buffer end. Usingsubarrayis efficient.
129-137: LGTM! Method signature extended properly.BlurRequest correctly added to the type union alongside ResourceRequest and ZoomRequest.
159-162: LGTM! Data chunking and progress tracking integrated.Uses the new
getDataChunkhelper and updates progress after each chunk.
169-174: LGTM! Response types updated correctly.BlurRequest properly added to the expected response types for ResourceAck.
182-186: LGTM! ResourceUpload call updated consistently.BlurRequest included in the initial upload call response types.
packages/core/src/data-manager/MessagesConfig.ts (1)
13-18: The review comment is incorrect.Version 0.0.0 is a sentinel value for detection failures, not actual firmware. The codebase uses it as a default when the bridge API fails to return a version, when firmware is absent, or when the device is in bootloader mode. Real old firmware versions (0.0.1 and below) are handled separately with the v1 message format. Using
latestfor 0.0.0 is the intended behavior to handle these detection edge cases safely.Likely an incorrect or invalid review comment.
packages/core/src/types/api/deviceUploadResource.ts (1)
8-8: Required field added to public API—breaking change confirmed.Adding
blurDataHexas a required field toDeviceUploadResourceParamsis a breaking change. The field appears required throughout: in the type definition, validation (required: true), and implementation.Search of the codebase found no consumers outside example code, but this doesn't account for external packages. Ensure this change aligns with your versioning strategy (major version bump required if semver).
The implementation doesn't support making this optional—it's marked required everywhere.
packages/connect-examples/expo-example/src/components/PlaygroundExecutor.tsx
Show resolved
Hide resolved
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
Summary by CodeRabbit
New Features
Chores