-
Notifications
You must be signed in to change notification settings - Fork 2
feat: migrate bridge to web-usb & upgrade sdk 1.1.16 #94
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 bootloader permission and reconnect UX, moves WebUSB connect orchestration into Dashboard, swaps the hardware SDK to a common connect SDK with env: 'webusb', removes bridge release mapping/state, updates runtime flags (isUpdating, needsBootloaderPermission), and treats Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard
participant SearchDevice
participant ServiceHardware
participant HardwareCommonSdk
rect rgb(230,245,255)
Note over Dashboard,ServiceHardware: User-initiated connect (WebUSB)
end
User->>Dashboard: opens app / requests device
Dashboard->>ServiceHardware: searchDevices()
ServiceHardware->>HardwareCommonSdk: query authorized devices
HardwareCommonSdk-->>ServiceHardware: no device
ServiceHardware-->>Dashboard: result empty
Dashboard->>SearchDevice: render with onConnectDevice
User->>SearchDevice: click Connect
SearchDevice->>Dashboard: onConnectDevice()
Dashboard->>ServiceHardware: promptBootloaderDeviceAccess()
ServiceHardware->>HardwareCommonSdk: request WebUSB permission
HardwareCommonSdk->>User: browser permission prompt
User-->>HardwareCommonSdk: grant access
HardwareCommonSdk-->>ServiceHardware: device selected
ServiceHardware-->>Dashboard: device available
Dashboard->>Dashboard: set pageStatus = connected
sequenceDiagram
participant Firmware
participant BootloaderPrompt
participant ServiceHardware
participant User
rect rgb(245,235,255)
Note over Firmware,ServiceHardware: Bootloader permission flow
end
Firmware->>BootloaderPrompt: show authorize UI
User->>BootloaderPrompt: click Authorize
BootloaderPrompt->>ServiceHardware: promptBootloaderDeviceAccess()
ServiceHardware->>User: browser permission prompt
User-->>ServiceHardware: grant/deny
alt granted
ServiceHardware-->>BootloaderPrompt: success
BootloaderPrompt-->>Firmware: permission granted
else denied
ServiceHardware-->>BootloaderPrompt: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ 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. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/Dashboard.tsx (1)
74-78: Restore the connect affordance after a failed scan.When the 30 s timeout fires we flip
pageStatusto'search-timeout'but never callsetNeedUserAction(true). After a user authorizes a device but the scan still returns nothing, the connect button stays hidden, so they cannot trigger WebUSB again. Add the reset inside the timeout (and any similar failure paths) so the button comes back.setTimeout(() => { if (serviceHardware.isSearch) { dispatch(setPageStatus('search-timeout')); + setNeedUserAction(true); } }, 30000);
📜 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 ignored due to path filters (3)
src/locales/en-US.jsonis excluded by!src/locales/*.jsonsrc/locales/zh-CN.jsonis excluded by!src/locales/*.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
config/webpack.config.js(1 hunks)package.json(2 hunks)src/components/Firmware/Firmware.tsx(7 hunks)src/components/Firmware/V3UploadLocalFirmware.tsx(1 hunks)src/components/SearchDevice/index.tsx(1 hunks)src/hardware/index.ts(4 hunks)src/hardware/instance.ts(2 hunks)src/store/reducers/runtime.ts(4 hunks)src/views/Dashboard.tsx(6 hunks)
🧰 Additional context used
🪛 ESLint
src/components/Firmware/V3UploadLocalFirmware.tsx
[error] 134-135: Delete ······⏎
(prettier/prettier)
src/components/SearchDevice/index.tsx
[error] 30-80: Fragments should contain more than one child - otherwise, there’s no need for a Fragment at all.
(react/jsx-no-useless-fragment)
[error] 31-32: Delete ⏎··········
(prettier/prettier)
[error] 79-80: Delete ··⏎········
(prettier/prettier)
[error] 87-87: Delete ··········
(prettier/prettier)
[error] 93-94: Delete ⏎················
(prettier/prettier)
🔇 Additional comments (3)
config/webpack.config.js (1)
564-569: LGTM! Correct handling of CommonJS modules.The
.cjsextension is now excluded from asset processing, ensuring CommonJS modules are handled by code loaders instead. This aligns with the newhd-common-connect-sdkdependency.package.json (2)
188-190: LGTM! Formatting fixes applied.Spacing in resolutions is now consistent.
12-14: All SDK packages verified. No issues found.✓ Package versions 1.1.16 exist on npm
✓ No security vulnerabilities detected
✓ Oldhd-web-sdkcompletely removed from codebase and dependenciesThe migration to the new packages is clean and secure.
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
📜 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 (2)
src/components/Firmware/V3UploadLocalFirmware.tsx(0 hunks)src/components/SearchDevice/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Firmware/V3UploadLocalFirmware.tsx
⏰ 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). (2)
- GitHub Check: test-web
- GitHub Check: release-web
🔇 Additional comments (1)
src/components/SearchDevice/index.tsx (1)
1-11: Clean refactoring to a prop-driven component.The interface and imports are well-structured. Moving from internal state and dispatch to props makes this component more reusable and easier to test.
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: 1
♻️ Duplicate comments (3)
src/components/SearchDevice/index.tsx (3)
44-45: Redundant disabled prop.Past review flagged this: if the Button component auto-disables when
loadingis true, the explicitdisabled={isConnecting}is redundant. Keep it only if the library doesn't guarantee auto-disable behavior.
88-89: Missing space between messages.Past review flagged this: two messages run together without spacing. They'll render as "Text one.Text two" if the first ends with punctuation.
Add spacing between them:
- <p className="text-sm font-normal text-gray-500"> - {intl.formatMessage({ id: 'TR_MAKE_SURE_IT_IS_WELL_CONNECTED' })} - {intl.formatMessage({ id: 'TR_SEARCHING_FOR_YOUR_DEVICE' })} - </p> + <p className="text-sm font-normal text-gray-500"> + {intl.formatMessage({ id: 'TR_MAKE_SURE_IT_IS_WELL_CONNECTED' })}{' '} + {intl.formatMessage({ id: 'TR_SEARCHING_FOR_YOUR_DEVICE' })} + </p>
94-122: Prefer logical AND over ternary.Past review suggested this:
condition ? (...) : nullworks butcondition && (...)is more idiomatic for conditional rendering in React.Replace the ternary:
- {pageStatus === 'search-timeout' ? ( + {pageStatus === 'search-timeout' && ( <> <p className="text-sm font-normal text-gray-500"> {intl.formatMessage({ id: 'TR_SEARCHING_TAKES_TOO_LONG' })} </p> {/* ... rest of timeout block ... */} </> - ) : null} + )}
📜 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 ignored due to path filters (3)
public/static/onekey.pngis excluded by!**/*.pngsrc/locales/en-US.jsonis excluded by!src/locales/*.jsonsrc/locales/zh-CN.jsonis excluded by!src/locales/*.json
📒 Files selected for processing (2)
src/components/Firmware/V3UploadLocalFirmware.tsx(1 hunks)src/components/SearchDevice/index.tsx(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). (2)
- GitHub Check: test-web
- GitHub Check: release-web
🔇 Additional comments (3)
src/components/SearchDevice/index.tsx (3)
1-1: Clean typing and interface.The FC import and SearchDeviceProps interface are well-defined. Optional props allow flexible usage across different flows.
Also applies to: 8-11
13-21: Simplified component structure.The component now takes explicit props and limits Redux usage to only pageStatus. Clean and maintainable.
29-43: Strong WebUSB onboarding flow.The pulsing background, prominent connect button, and clear messaging guide users through WebUSB permission. Great UX improvement.
Also applies to: 46-83
Summary by CodeRabbit
New Features
UI
Chores