-
Notifications
You must be signed in to change notification settings - Fork 21
feat: support blur resource #550
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
Conversation
WalkthroughAdds support for a third upload stream (blur) alongside data and thumbnail. Updates message schema, types, and device upload flow to include BlurRequest and blur_data_length. Implements chunk selection per request type, unified progress tracking, and removes a UI close message. Updates firmware submodule reference. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Host App
participant Core as DeviceUploadResource
participant Dev as Device (Firmware)
App->>Core: start ResourceUpload(data, thumbnail, blur, meta)
Core->>Dev: ResourceUpload{... lengths incl. blur_data_length}
Dev-->>Core: ResourceAck | Error
alt Ack
loop Until Success
Dev-->>Core: ResourceRequest | ZoomRequest | BlurRequest
alt ResourceRequest (main)
Core->>Dev: Chunk(main, offset, len)
else ZoomRequest (thumbnail)
Core->>Dev: Chunk(thumbnail, offset, len)
else BlurRequest (blur)
Core->>Dev: Chunk(blur, offset, len)
end
Note over Core: Update uploadProgress<br/>total/uploaded/currentFile
end
Dev-->>Core: Success
Core-->>App: Completed
else Error
Core-->>App: Failed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/api/device/DeviceUploadResource.ts (2)
57-65
: Validate all payload hex fields as hex; make blur optional for back-compatdataHex and thumbnailDataHex are validated as string, blurDataHex as hexString and required. Align all to hexString. If some firmware doesn’t support blur, make blurDataHex optional and default to empty bytes.
Apply:
- { name: 'dataHex', type: 'string', required: true }, - { name: 'thumbnailDataHex', type: 'string', required: true }, - { name: 'blurDataHex', type: 'hexString', required: true }, + { name: 'dataHex', type: 'hexString', required: true }, + { name: 'thumbnailDataHex', type: 'hexString', required: true }, + { name: 'blurDataHex', type: 'hexString' }, @@ - this.paramsData = { - data: new Uint8Array(hexToBytes(dataHex)), - thumbnailData: new Uint8Array(hexToBytes(thumbnailDataHex)), - blurData: new Uint8Array(hexToBytes(blurDataHex)), - }; + this.paramsData = { + data: new Uint8Array(hexToBytes(dataHex)), + thumbnailData: new Uint8Array(hexToBytes(thumbnailDataHex)), + blurData: blurDataHex ? new Uint8Array(hexToBytes(blurDataHex)) : new Uint8Array(), + };Also applies to: 67-75
83-86
: Bug: hashing the wrong data type (string vs bytes)blake2s expects bytes. Passing a hex string hashes its UTF-8, not the binary. Use the parsed bytes.
- const fileHash = bytesToHex(blake2s(this.payload.dataHex)).slice(0, 8); + const fileHash = bytesToHex(blake2s(this.paramsData.data)).slice(0, 8);
📜 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
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/core/src/api/device/DeviceUploadResource.ts
(4 hunks)packages/core/src/data/messages/messages.json
(3 hunks)packages/core/src/types/api/deviceUploadResource.ts
(1 hunks)packages/hd-transport/src/types/messages.ts
(3 hunks)submodules/firmware
(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: Analyze (javascript-typescript)
- GitHub Check: build (22)
- GitHub Check: lint (22)
🔇 Additional comments (7)
packages/core/src/data/messages/messages.json (2)
7562-7574
: BlurRequest mirrors ZoomRequest — good symmetryField names and IDs match the ZoomRequest shape. Looks consistent.
Verify host handling is updated to respond to BlurRequest with ResourceAck chunks.
12386-12386
: Transport and switch mappings for BlurRequest added — Verifiedhd-transport/src/types/messages.ts
includesBlurRequest
in the enum→type mapping andDeviceUploadResource.ts
handlesBlurRequest
in its switch/case logic.packages/hd-transport/src/types/messages.ts (3)
2763-2764
: Blur length optionality: confirm protocol contractzoom_data_length is required (number), but blur_data_length is optional. If blur is now a required third stream (core code requires blurDataHex), make this non-optional to match the contract; otherwise, keep it optional and make host-side blur optional too. Please align both sides.
2772-2776
: New BlurRequest type: looks correctShape matches ResourceRequest/ZoomRequest (offset, data_length). No issues.
4644-4645
: MessageType mapping updated: goodBlurRequest is properly added to MessageType.
packages/core/src/api/device/DeviceUploadResource.ts (2)
88-97
: blur_data_length populated: OK, but sync with transport typeYou always send blur_data_length. If transport keeps it optional, that’s fine; if blur is mandatory, make the transport field required too. Keep them consistent.
169-175
: TypedCall response union updated: looks goodAllows BlurRequest in both ResourceAck and initial ResourceUpload. Flow matches the new three-stream protocol.
Also applies to: 182-189
* feat: support blur resource (#550) * fix: example * feat: add btc params * chore: update blur screent example * chore: fix lint * chore: release 1.1.11-alpha.2
Summary by CodeRabbit
New Features
Chores