Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a DFU permission UI and flow: polling for already-authorized DFU devices via a new waitForDfu, integration into webstm32 DFU reboot handling with UI notification and timed fallback permission request, plus a new localization string for the permission prompt. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as FirmwareFlasherTab
participant Protocol as webstm32
participant DFU as webusbdfu
participant USB as navigator.usb
Note over Protocol: DFU reboot detected
Protocol->>DFU: waitForDfu(5000)
DFU->>USB: getDevices()
alt Device Found
USB-->>DFU: matching device
DFU->>DFU: handleNewDevice()
DFU-->>Protocol: device
Protocol->>Protocol: resume flashing
else Device Not Found / Auth Required
USB-->>DFU: no matching devices
DFU-->>Protocol: reject (DFU_AUTH_REQUIRED)
Protocol->>UI: showDfuPermission()
UI->>User: display "Click to connect to DFU device"
User->>UI: clicks button
UI->>DFU: requestDfuPermission() / navigator.usb.requestDevice()
alt Permission Granted
DFU-->>Protocol: device acquired
Protocol->>Protocol: resume flashing
else Permission Denied / Timeout
UI-->>Protocol: permission failed
Protocol->>Protocol: handleError()
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/tabs/FirmwareFlasherTab.vue`:
- Around line 955-970: The DFU prompt state is being cleared unconditionally
which hides the retry prompt on cancel/deny; update requestDfuPermission so it
sets state.dfuAuthRequired = true before calling DFU.requestPermission(), remove
the unconditional state.dfuAuthRequired = false at the start and from the
finally block, and instead only set state.dfuAuthRequired = false when a device
is actually returned (i.e. inside the if (device) branch after
DFU.requestPermission()). Keep state.dfuAuthRequired true on denied/cancelled or
on error so the UI can still show the retry/authorization prompt; reference the
requestDfuPermission function, state.dfuAuthRequired flag, and
DFU.requestPermission() call when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
locales/en/messages.jsonsrc/components/tabs/FirmwareFlasherTab.vuesrc/js/protocols/webstm32.jssrc/js/protocols/webusbdfu.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/tabs/FirmwareFlasherTab.vue (2)
3329-3343: Consider using theme variables instead of hardcoded colors.The hardcoded colors (
#8bc34a,#111) are inconsistent with the rest of the component which uses CSS variables likevar(--primary-500),var(--success-600), etc. Using theme variables would ensure the button adapts correctly to any theme changes.Also, the
:deep()selector may not be necessary here since.dfu-auth-requestis in this component's own template rather than a child component.Suggested refactor
/* DFU permission button accent styling */ -:deep(.dfu-auth-request .dfu-permission) { - background: `#8bc34a` !important; /* green */ - border-color: `#8bc34a` !important; - color: `#111` !important; +.dfu-auth-request .dfu-permission { + background: var(--success-600) !important; + border-color: var(--success-700) !important; + color: var(--surface-50) !important; box-shadow: 0 1px 0 rgba(0, 0, 0, 0.2) inset; } -:deep(.dfu-auth-request .dfu-permission:hover) { +.dfu-auth-request .dfu-permission:hover { filter: brightness(0.95); } -:deep(.dfu-auth-request p) { +.dfu-auth-request p { margin-bottom: 8px !important; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tabs/FirmwareFlasherTab.vue` around lines 3329 - 3343, Replace the hardcoded colors and unnecessary :deep() usage in the DFU permission styles: update the selectors (.dfu-auth-request .dfu-permission, .dfu-auth-request .dfu-permission:hover, .dfu-auth-request p) to use the component/theme CSS variables (e.g., var(--success-600) or var(--primary-500) for background/border and var(--text-high) or var(--black) for color) instead of `#8bc34a` and `#111`, and remove the :deep(...) wrapper since .dfu-auth-request is in the same template so plain selectors will suffice; keep the existing box-shadow and hover brightness effect but reference variables where appropriate.
10-15: Consider moving inline styles to the scoped CSS section.The inline
style="text-align: center; margin-top: 16px"could be consolidated into the.dfu-auth-requestclass in the scoped CSS for consistency with the rest of the component.Suggested refactor
In template:
-<div v-if="state.dfuAuthRequired" class="dfu-auth-request" style="text-align: center; margin-top: 16px"> +<div v-if="state.dfuAuthRequired" class="dfu-auth-request">In CSS (around line 3329):
+.dfu-auth-request { + text-align: center; + margin-top: 16px; +} + :deep(.dfu-auth-request .dfu-permission) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tabs/FirmwareFlasherTab.vue` around lines 10 - 15, Remove the inline style on the div that uses state.dfuAuthRequired and move those rules into the component's scoped CSS: delete style="text-align: center; margin-top: 16px" from the <div class="dfu-auth-request"> and add equivalent declarations (text-align: center; margin-top: 16px) to the .dfu-auth-request selector in the <style scoped> block of FirmwareFlasherTab.vue so styling is consistent with the rest of the component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/tabs/FirmwareFlasherTab.vue`:
- Around line 3329-3343: Replace the hardcoded colors and unnecessary :deep()
usage in the DFU permission styles: update the selectors (.dfu-auth-request
.dfu-permission, .dfu-auth-request .dfu-permission:hover, .dfu-auth-request p)
to use the component/theme CSS variables (e.g., var(--success-600) or
var(--primary-500) for background/border and var(--text-high) or var(--black)
for color) instead of `#8bc34a` and `#111`, and remove the :deep(...) wrapper since
.dfu-auth-request is in the same template so plain selectors will suffice; keep
the existing box-shadow and hover brightness effect but reference variables
where appropriate.
- Around line 10-15: Remove the inline style on the div that uses
state.dfuAuthRequired and move those rules into the component's scoped CSS:
delete style="text-align: center; margin-top: 16px" from the <div
class="dfu-auth-request"> and add equivalent declarations (text-align: center;
margin-top: 16px) to the .dfu-auth-request selector in the <style scoped> block
of FirmwareFlasherTab.vue so styling is consistent with the rest of the
component.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/js/protocols/webusbdfu.js (1)
130-161: LGTM! Clean implementation for polling already-authorized DFU devices.The polling logic correctly uses
navigator.usb.getDevices()to find devices the user has previously granted permission to, with appropriate error handling and defensive coding forusbDevices.filters.One minor optional improvement: consider defining the error code as a constant to avoid string comparison fragility in callers:
💡 Optional: Use a constant for the error code
+// At the top of the file or in a shared constants module +export const DFU_AUTH_REQUIRED = "DFU_AUTH_REQUIRED"; + // In the method -throw new Error("DFU_AUTH_REQUIRED"); +throw new Error(DFU_AUTH_REQUIRED);This allows callers to import and compare against the constant rather than a magic string.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/protocols/webusbdfu.js` around lines 130 - 161, The code throws a magic-string error "DFU_AUTH_REQUIRED" in waitForDfu; define and export a named constant (e.g. DFU_AUTH_REQUIRED) and throw new Error(DFU_AUTH_REQUIRED) instead, update any callers to import and compare against that constant (replace string literal checks with the constant) to avoid fragile string comparisons; ensure the constant is declared near other protocol/errors exports so it can be imported where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/js/protocols/webusbdfu.js`:
- Around line 130-161: The code throws a magic-string error "DFU_AUTH_REQUIRED"
in waitForDfu; define and export a named constant (e.g. DFU_AUTH_REQUIRED) and
throw new Error(DFU_AUTH_REQUIRED) instead, update any callers to import and
compare against that constant (replace string literal checks with the constant)
to avoid fragile string comparisons; ensure the constant is declared near other
protocol/errors exports so it can be imported where needed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/protocols/webstm32.jssrc/js/protocols/webusbdfu.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/protocols/webstm32.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/js/protocols/webstm32.js (1)
135-149: Make fallback permission timeout cancellable.The delayed fallback
setTimeoutis not tracked, so it can still fire after state reset/navigation. Store the timeout id and clear it during cleanup/error paths to avoid stale prompts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/protocols/webstm32.js` around lines 135 - 149, The setTimeout that calls DFU.requestPermission should be cancellable: store its id (e.g. this.dfuPermissionTimeout = setTimeout(...)) when scheduling and clear it (clearTimeout(this.dfuPermissionTimeout); this.dfuPermissionTimeout = null) in error/cleanup paths such as inside handleError() and any state-reset/navigation cleanup logic so the fallback prompt can't run after teardown; update DFU.requestPermission().then/.catch branches to clear the timeout as well to ensure it cannot fire later.src/js/protocols/webusbdfu.js (1)
162-164: Prefer structured error signaling over matchingError.message.At Line 163, throwing
new Error(DFU_AUTH_REQUIRED)works, but it tightly couples callers to message text. A dedicatederror.code(or custom error type) is more robust for control flow.Proposed hardening
- throw new Error(DFU_AUTH_REQUIRED); + const error = new Error("DFU authorization required"); + error.code = DFU_AUTH_REQUIRED; + throw error;Then in callers, check
e?.code === DFU_AUTH_REQUIRED.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/protocols/webusbdfu.js` around lines 162 - 164, The code currently throws new Error(DFU_AUTH_REQUIRED) which forces callers to match message text; change the throw site in webusbdfu.js (the throw new Error(DFU_AUTH_REQUIRED) line) to produce structured error signaling instead—either instantiate a dedicated error class (e.g., class DFUAuthRequiredError extends Error) and throw that, or create an Error and set a stable property (e.g., err.code = DFU_AUTH_REQUIRED) before throwing; update callers to check e?.code === DFU_AUTH_REQUIRED (or use instanceof DFUAuthRequiredError) for control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/js/protocols/webstm32.js`:
- Around line 135-149: The setTimeout that calls DFU.requestPermission should be
cancellable: store its id (e.g. this.dfuPermissionTimeout = setTimeout(...))
when scheduling and clear it (clearTimeout(this.dfuPermissionTimeout);
this.dfuPermissionTimeout = null) in error/cleanup paths such as inside
handleError() and any state-reset/navigation cleanup logic so the fallback
prompt can't run after teardown; update DFU.requestPermission().then/.catch
branches to clear the timeout as well to ensure it cannot fire later.
In `@src/js/protocols/webusbdfu.js`:
- Around line 162-164: The code currently throws new Error(DFU_AUTH_REQUIRED)
which forces callers to match message text; change the throw site in
webusbdfu.js (the throw new Error(DFU_AUTH_REQUIRED) line) to produce structured
error signaling instead—either instantiate a dedicated error class (e.g., class
DFUAuthRequiredError extends Error) and throw that, or create an Error and set a
stable property (e.g., err.code = DFU_AUTH_REQUIRED) before throwing; update
callers to check e?.code === DFU_AUTH_REQUIRED (or use instanceof
DFUAuthRequiredError) for control flow.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/tabs/FirmwareFlasherTab.vuesrc/js/protocols/webstm32.jssrc/js/protocols/webusbdfu.js
|
|
🎉 Do you want to test this code? 🎉 |
|
Not working. Regular app.betaflight works. |
|
@Pancronos tested with URL provided here - after a few seconds ... 
Also your version is outdated 
|
|
I tested the pr4884 app, https://pr4884.betaflight-app-preview.pages.dev/ got the green message, connected to dfu, nothing....went in a loop. |
|
Being conservative here for development - still using chromium here - Version 131.0.6778.204 (Official Build) built on Ubuntu 22.04.5 LTS (64-bit) Also using firefox for browsing - and chromium purely for app development. |
3 participants
When flashing on a fresh browser DFU has no permission and waits forever.
Added a button to let user request required DFU permissions.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Localization