cleanup and explicit use web prefix for protocols #4500
Conversation
## Walkthrough
The changes remove the `bluetoothMode` and `manualMode` properties from the `CONFIGURATOR` object and eliminate all logic related to these flags across the codebase. Protocol selection and connection logic are simplified, with protocol determination now based solely on the provided port path, and related side effects and logging reduced. Device type strings are updated for consistency, and minor fixes are applied to event names, CSS syntax, and locale handling.
## Changes
| Files | Change Summary |
|-------------------------------|------------------------------------------------------------------------------------------------|
| src/js/data_storage.js | Removed `bluetoothMode` and `manualMode` from `CONFIGURATOR` object. |
| src/js/serial.js | Renamed internal protocol variables for consistency; simplified `selectProtocol` to rely only on `portPath`; removed logging and `CONFIGURATOR` flag updates; removed default protocol selection on instantiation. |
| src/js/serial_backend.js | Removed references to `CONFIGURATOR.bluetoothMode` and `CONFIGURATOR.manualMode`; updated disconnect flow to remove callback and call `onClosed` immediately in virtual mode; removed debug logging. |
| src/js/protocols/WebSerial.js | Renamed log prefix from `[SERIAL]` to `[WEBSERIAL]`; reset `connectionInfo` to `null` on disconnect success or error. |
| src/js/port_handler.js | Replaced device type strings `"bluetooth"` and `"serial"` with `"webbluetooth"` and `"webserial"`; improved locale detection with fallback to `"en"` in `sortPorts`; added default case with warning in `updateDeviceList`. |
| src/js/utils/checkBrowserCompatibility.js | Fixed CSS syntax error by adding missing parenthesis; corrected jQuery selector to apply styles to the correct element. |
| src/js/protocols/WebBluetooth.js | Fixed typo in Bluetooth event name from `"gatserverdisconnected"` to `"gattserverdisconnected"`. |
| src/components/port-picker/PortsInput.vue | Changed emitted event name from `"ports-input:request-permission"` to `"ports-input:request-permission-serial"` when requesting permission. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant SerialBackend
participant Serial
participant CONFIGURATOR
User->>SerialBackend: connectDisconnect(port)
SerialBackend->>CONFIGURATOR: set virtualMode (based on port)
SerialBackend->>Serial: selectProtocol(portPath)
Note right of Serial: Protocol determined by portPath only
User->>SerialBackend: finishClose()
SerialBackend->>Serial: disconnect()
alt CONFIGURATOR.virtualMode is true
SerialBackend->>User: onClosed(true)
endPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
haslinghuis
force-pushed
the
serial-connection
branch
from
55d8b3d to
dfedaba
Compare
haslinghuis
force-pushed
the
serial-connection
branch
from
dfedaba to
3873dff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/js/port_handler.js(9 hunks)src/js/protocols/WebBluetooth.js(1 hunks)src/js/serial.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/serial.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/js/port_handler.js
[error] 344-344: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (3)
src/js/protocols/WebBluetooth.js (1)
47-47: LGTM! Fixed typo in Bluetooth GATT event name.The event name has been corrected from
"gatserverdisconnected"to"gattserverdisconnected". This ensures the event listener properly handles Bluetooth GATT server disconnection events.src/js/port_handler.js (2)
53-54: LGTM! Consistent device type string updates.The standardization of device type strings from
"bluetooth"/"serial"to"webbluetooth"/"webserial"improves naming consistency across the codebase and aligns with the Web API naming conventions.Also applies to: 63-63, 65-65, 84-85, 115-115, 123-124, 287-287, 309-309, 311-311, 319-319, 321-321, 334-334
180-182: LGTM! Improved locale handling with defensive programming.The addition of
typeof window !== "undefined" && window.navigatorcheck prevents runtime errors in environments wherewindowmight not be available (e.g., Node.js, web workers). Defaulting to"en"locale is a sensible fallback.
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/js/port_handler.js (1)
180-182: SafeguardlocaleCompareagainst undefinedwindow.navigator.
Using a default"en"locale prevents errors in non-browser contexts.
Consider extracting the default locale into a constant for reuse.src/js/serial.js (1)
119-133: Consolidate protocol selection logic.
TheselectProtocolmethod has anif/elsechain mapping port paths to instances. Consider simplifying with:newProtocol = this._getProtocolByType(portPath.toLowerCase()) || this._protocol;This leverages
_protocolMapand reduces duplication.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/js/data_storage.js(0 hunks)src/js/port_handler.js(9 hunks)src/js/protocols/WebBluetooth.js(1 hunks)src/js/protocols/WebSerial.js(2 hunks)src/js/serial.js(5 hunks)src/js/serial_backend.js(1 hunks)src/js/utils/checkBrowserCompatibility.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/js/data_storage.js
🔇 Additional comments (25)
src/js/utils/checkBrowserCompatibility.js (2)
111-111: LGTM: CSS syntax fix.The background-image property syntax is now correct with proper URL formatting.
117-117: Good fix: Target the correct DOM element.Changing from
$(div)to$(newDiv)ensures the error message and styling are applied only to the intended container element, not all div elements in the DOM.src/js/protocols/WebBluetooth.js (1)
47-47: LGTM: Corrected Web Bluetooth API event name.Fixed the typo from "gatserverdisconnected" to "gattserverdisconnected". The correct event name ensures the
handleRemovedDevicehandler will be properly invoked when the Bluetooth GATT server disconnects.src/js/protocols/WebSerial.js (3)
4-4: LGTM: Protocol naming standardization.Updated the log prefix from "[SERIAL]" to "[WEBSERIAL]" to align with the codebase-wide protocol naming conventions.
311-311: Good practice: Explicit connection state cleanup.Resetting
connectionInfotonullensures consistent cleanup of connection state after successful disconnect.
319-320: Thorough error handling: Cleanup state even on disconnect failure.Ensuring
connectionInfois reset even when disconnect encounters errors prevents potential memory leaks and maintains consistent state management.src/js/serial_backend.js (2)
188-190: LGTM: Immediate virtual mode disconnect handling.For virtual mode, calling
onClosed(true)immediately after disconnect makes sense since no actual network disconnection is required. This ensures prompt UI updates for virtual connections.
186-186:✅ Verification successful
Verify the disconnect API change.
The
serial.disconnect()call no longer uses a callback parameter. Ensure this aligns with the updated protocol implementation and that disconnection events are properly handled through event listeners instead.
🏁 Script executed:
#!/bin/bash # Verify that serial.disconnect() method signature doesn't expect a callback ast-grep --pattern 'disconnect($_) { $$$ }'Length of output: 47
To confirm the
disconnectmethod’s signature and callback usage, let’s locate all definitions and invocations in the serial backend:
🏁 Script executed:
#!/bin/bash # Show definitions and calls to disconnect() in the serial backend rg -n "disconnect(" -C 3 src/js/serial_backend.js rg -n "disconnect(" -C 3 src/jsLength of output: 231
Let's retry with properly escaped patterns to locate both the definition and all calls of
disconnect:
🏁 Script executed:
#!/bin/bash # Search for the disconnect method definition and its invocations rg -n "disconnect\\(" -C 3 src/js/serial_backend.js || true rg -n "disconnect\\(" -C 3 src/js || trueLength of output: 11288
To verify that disconnection events are handled correctly via listeners and that
onClosedis wired up, let’s inspect the surrounding context:
🏁 Script executed:
#!/bin/bash # Show onClosed usage and any disconnect event listeners in serial_backend.js rg -n "onClosed" -C 8 src/js/serial_backend.js rg -n "serial\\.addEventListener" -C 8 src/js/serial_backend.jsLength of output: 3988
Disconnect API change verified
The call to
serial.disconnect()inserial_backend.jsnow relies on the existing"disconnect"event listener (lines 141–142) to invokeonClosed(event.detail), and in virtual mode falls back to an immediateonClosed(true)(lines 188–190). Thedisconnectmethod insrc/js/serial.jsstill accepts an optional callback for backward compatibility but correctly dispatches adisconnectevent once the underlying protocol finishes teardown. No further changes are required here.src/js/port_handler.js (12)
53-55: Event handler deviceType updated for consistency.
Wiring permission requests to"webbluetooth"and"webserial"aligns with the renaming of port types.
62-66: Adapt device addition logic to new port type identifiers.
Correctly routes incoming devices through"webbluetooth"or"webserial"handlers.
84-86: Parallel device list refresh uses updated identifiers.
refreshAllDeviceListsnow invokesupdateDeviceList("webserial")andupdateDeviceList("webbluetooth")in parallel per the new naming.
113-119: Handle missing device path by refreshingwebserial.
Gracefully recovers from removal events lacking a path by refreshing serial ports only.
122-124: Switch removal logic updated for new type check.
Determines removal path prefix and calls the correct update for"webbluetooth"vs"webserial".
286-288: Unify device addition promise with new identifiers.
handleDeviceAddedconsistently updates lists for"webbluetooth"or"webserial"based on the event.
309-314: Conditional Bluetooth device retrieval.
ChecksshowBluetoothOptionbefore callingserial.getDevices("webbluetooth"), avoiding unnecessary calls.
319-323: Conditional Serial device retrieval.
Mirrors Bluetooth logic by verifyingshowSerialOptionprior toserial.getDevices("webserial").
324-327: Handle unknown deviceType gracefully.
Logs a warning and returns an empty array on unexpecteddeviceTypevalues.
333-338: Update state for Bluetooth devices.
Correctly setsbluetoothAvailableandcurrentBluetoothPortsafter sorting.
344-348: Update state for Serial devices.
Maps the sorted ports tocurrentSerialPortsand togglesportAvailable.
349-352: Handle unknown port update with warning.
Prevents silent failures when updating an unrecognized port category.src/js/serial.js (5)
20-23: Rename protocol instances for clarity.
Introducing_webBluetoothand_webSocketfollows consistent camelCase naming.
26-30: Update_protocolMapkeys to new protocol names.
Mappingwebserial,webbluetooth, andwebsocketmatches the updated identifiers used elsewhere.
58-63: Extend_getProtocolTypeto include new protocols.
Now returns"webbluetooth"and"websocket"alongside"webserial", improving type detection.
76-78: Include all protocols in event forwarding setup.
Forwarding events for_webBluetoothand_webSocketensures complete coverage of device events.
255-258: Update JSDoc forgetDevices.
The parameter description now accurately lists the supported types:'webserial', 'webbluetooth', 'websocket', 'virtual'.
newProtocol = this._getProtocolByType(portPath.toLowerCase()) || this._protocol;Obvious this suggestion does not work as we are using a counter suffix for serial and bluetooth port instead of using connectionId. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@haslinghuis Thank you for the clarification! You're absolutely right that the suggested approach wouldn't work with counter prefixes. Let me examine the current implementation to better understand how the protocol detection works with your port naming scheme. 🏁 Script executed: Length of output: 12039 Now I understand the issue! Looking at the current implementation, I can see that your // Current working implementation:
if (portPath.startsWith("bluetooth")) {
newProtocol = this._webBluetooth;
} else {
newProtocol = this._webSerial; // Default for serial ports
}The suggested code
Your current implementation with Thank you for the clarification - it's a good example of why understanding the specific port naming conventions in a codebase is crucial for protocol selection logic! ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3127 🏁 Script executed: Length of output: 94 🏁 Script executed: Length of output: 2665 🏁 Script executed: Length of output: 520 @haslinghuis You're absolutely right! That suggestion wouldn't work with your port naming convention. The issue is that A simple
For your naming convention, you'd need something like: _getProtocolByPortPath(portPath) {
if (!portPath) return null;
const path = portPath.toLowerCase();
if (path === "virtual") return this._virtual;
if (path === "manual") return this._webSocket;
if (path.startsWith("serial")) return this._webSerial;
if (path.startsWith("bluetooth")) return this._webBluetooth;
return null;
}This would properly handle your counter suffixes while maintaining the protocol selection logic. ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@haslinghuis I understand your point about the counter suffix approach. Let me examine the current protocol detection implementation to better understand how the port naming works. 🏁 Script executed: Length of output: 1326 🏁 Script executed: Length of output: 90167 @haslinghuis You're absolutely right! I apologize for that suggestion. Looking at the current implementation in The current logic in
This approach makes perfect sense given your port naming convention with counter suffixes. My suggestion would have failed because "COM1" or "bluetooth_1" aren't valid keys in the protocol map. Thank you for the correction! 🐰 |
haslinghuis
force-pushed
the
serial-connection
branch
from
5b7765f to
49687df
Compare
haslinghuis
force-pushed
the
serial-connection
branch
from
49687df to
bd73ee1
Compare
|
|
Preview URL: https://bd73ee15.betaflight-configurator.pages.dev |
Summary by CodeRabbit
No changes to visible features or user interface beyond improved clarity in port permission options; these updates improve internal reliability and maintainability.