Skip to content

Conversation

@haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Dec 17, 2025

Reported Issue: for new installs, new devices, there is no permission yet to allow for connection and after flashing the port is empty and still refers to the DFU port. Trying to connect leaves the configuration in unrecoverable state.

Fix:

  • Removed duplicate check in onOpen
  • Fixed handler (in webusbdfu.js) to remove DFU port after flashing
  • Port now resets to no selection after requesting permissions
    • when a new port gets detected a new event is fired
    • requesting permissions needs to be user initiated.

Issue is here:

image

After the fix:

image

Summary by CodeRabbit

  • Bug Fixes

    • Prevented invalid connection attempts when no port is selected.
    • Removed an unnecessary API version compatibility warning dialog while preserving essential version checks.
  • Improvements

    • Added USB removal handling so device lists update and ports are reselected automatically; emits a removal event when the active port disconnects.
    • Generalized permission requests: a single permission event pattern is emitted and selection is reset to "noselection" after request.

✏️ Tip: You can customize this high-level summary in your review settings.

@haslinghuis haslinghuis added this to the 2025.12 milestone Dec 17, 2025
@haslinghuis haslinghuis self-assigned this Dec 17, 2025
@haslinghuis haslinghuis moved this to App in 2025.12.0 Dec 17, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 17, 2025

Walkthrough

Adds USB device removal handling across WEBUSB DFU and PortHandler, updates permission-request handling in the port picker, and removes the previous max-supported API-version dialog/abort path in the serial open flow while adding an early guard for missing selected ports.

Changes

Cohort / File(s) Summary
Serial connection flow
src/js/serial_backend.js
Removed the max-supported API-version user dialog/abort path in onOpen; added an early-return guard when selectedPort is absent so connect/open exits quietly.
Port handler — USB removal
src/js/port_handler.js
Added PortHandler.removedUsbDevice(device) and registered a removedDevice listener in initialize; normalizes/derives device path, updates USB list, reselects active port, and emits port-handler:device-removed if the removed device was the selected port.
WEBUSB DFU protocol — disconnect handling
src/js/protocols/webusbdfu.js
Added handleRemovedDevice(device) and changed the USB disconnect listener to call it (emit removedDevice), switching semantics to explicit removals instead of treating disconnects as new devices.
Port picker — permission events
src/components/port-picker/PortsInput.vue
Generalized permission handling: replaced explicit requestpermission* checks with a startsWith-based branch, emits ports-input:request-permission-${type}, and resets modelValue.selectedPort to noselection after emitting.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Browser as Browser USB API
participant WEBUSB as WEBUSBDFU_protocol
participant PortH as PortHandler
participant UI as UI / listeners

Browser->>WEBUSB: disconnect event (device)
WEBUSB->>WEBUSB: handleRemovedDevice(device)
WEBUSB->>PortH: emit removedDevice(device)
PortH->>PortH: removedUsbDevice(device) — normalize path, update USB list
alt removed was selectedPort
    PortH->>PortH: reselect active port
    PortH->>UI: emit port-handler:device-removed
    UI->>UI: update UI/state
else not selected
    PortH->>PortH: update list without selected-port event
end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Inspect path normalization and fallback when device.path is absent in PortHandler.removedUsbDevice.
  • Verify removedDevice emission payload and listeners consume it as expected (WEBUSBDFU ↔ PortHandler).
  • Confirm serial_backend.js early-return does not break other open/close lifecycle code.

Possibly related PRs

Suggested reviewers

  • blckmn
  • VitroidFPV
  • nerdCopter

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing connection issues on new installs and devices without permission.
Description check ✅ Passed The pull request description clearly explains the issue, provides specific details about the fix, and includes before/after screenshots demonstrating the problem and solution.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/js/protocols/webusbdfu.js (1)

94-97: Consider returning the removed port for consistency.

The handleRemovedDevice method is well-structured and mirrors handleNewDevice. For consistency, consider returning the removed port object (line 95) since handleNewDevice returns the added port at line 92, though this is optional if consumers don't need the return value.

 handleRemovedDevice(device) {
     const removed = this.createPort(device);
     this.dispatchEvent(new CustomEvent("removedDevice", { detail: removed }));
+    return removed;
 }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36f2b27 and f032469.

📒 Files selected for processing (3)
  • src/js/port_handler.js (2 hunks)
  • src/js/protocols/webusbdfu.js (1 hunks)
  • src/js/serial_backend.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/js/serial_backend.js
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: blckmn
Repo: betaflight/betaflight-configurator PR: 4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
Repo: betaflight/betaflight-configurator PR: 4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.

Applied to files:

  • src/js/protocols/webusbdfu.js
  • src/js/port_handler.js
📚 Learning: 2025-10-25T21:16:32.474Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4379
File: src/js/protocols/TauriSerial.js:203-259
Timestamp: 2025-10-25T21:16:32.474Z
Learning: In TauriSerial (src/js/protocols/TauriSerial.js), the requestPermissionDevice() method is not needed and not invoked. Tauri automatically discovers serial devices through the constructor's loadDevices() and startDeviceMonitoring() calls, bypassing the browser permission model that WebSerial requires. Devices are auto-detected via a 1-second polling interval without user permission prompts.

Applied to files:

  • src/js/protocols/webusbdfu.js
  • src/js/port_handler.js
📚 Learning: 2025-11-24T15:07:25.227Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:47-47
Timestamp: 2025-11-24T15:07:25.227Z
Learning: In betaflight-configurator's CapacitorSerial protocol (src/js/protocols/CapacitorSerial.js), the fire-and-forget this.loadDevices() call in the constructor is intentional background pre-loading. The actual critical device list loading for UI population is orchestrated by port_handler.js via serial.getDevices("serial"), which properly awaits CapacitorSerial.getDevices() → loadDevices() in a complete async chain. The constructor's async call does not cause race conditions.

Applied to files:

  • src/js/port_handler.js
📚 Learning: 2025-11-22T21:18:08.814Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4706
File: src/js/protocols/CapacitorSerial.js:11-46
Timestamp: 2025-11-22T21:18:08.814Z
Learning: In betaflight-configurator, protocol implementations (WebSerial, CapacitorSerial, WebBluetooth, etc.) are instantiated once in the Serial class constructor and stored as singletons in the _protocols array for the entire application lifetime. They are never destroyed or recreated, so cleanup methods to remove event listeners are not needed.

Applied to files:

  • src/js/port_handler.js
📚 Learning: 2025-11-28T22:41:59.374Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4714
File: src/js/protocols/CapacitorBluetooth.js:135-143
Timestamp: 2025-11-28T22:41:59.374Z
Learning: In betaflight-configurator, WebSerial.js uses a constant path "serial" for all devices, and CapacitorBluetooth.js uses constant path "bluetooth", consistent with the single-device-at-a-time workflow. WebBluetooth.js uses counter-based paths (bluetooth_${counter}), but that's specific to its implementation. The path strategy varies by protocol based on their specific requirements.

Applied to files:

  • src/js/port_handler.js
🧬 Code graph analysis (1)
src/js/port_handler.js (2)
src/js/protocols/webusbdfu.js (1)
  • WEBUSBDFU (1279-1279)
src/components/eventBus.js (2)
  • EventBus (3-8)
  • EventBus (3-8)
⏰ 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). (1)
  • GitHub Check: Build / build
🔇 Additional comments (3)
src/js/protocols/webusbdfu.js (1)

86-86: Critical fix: Disconnect now triggers removal handler.

The change from handleNewDevice to handleRemovedDevice correctly addresses a logical bug where USB disconnect events were incorrectly treated as device additions.

src/js/port_handler.js (2)

75-75: LGTM!

The USB device removal event listener registration is consistent with the addition listener at line 74 and integrates properly with the new removal handling flow.


149-171: Well-structured USB removal handler with good defensive programming.

The removedUsbDevice method correctly mirrors the structure of removedSerialDevice (lines 107-137), providing consistent device removal handling across protocols. The defensive path validation and conditional event emission are appropriate.

Copy link
Member

@nerdCopter nerdCopter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-briefly tested, flashed, pasted diff. did not deep dive connections. did reset permissions.

@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

🎉 Do you want to test this code? 🎉

⚠️ CAUTION: The build may be unstable and result in corrupted configurations or data loss. Use only for testing! ⚠️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: App

Development

Successfully merging this pull request may close these issues.

Connections issues that leaves the UI in a broken state.

2 participants