Cleanup serial #4670
Cleanup serial #4670
Conversation
WalkthroughRefactors serial orchestration to a data-driven Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User UI
participant Main as main.js
participant Serial as Serial
rect rgb(245,247,250)
note over Main,UI: Tab switch (updated flow)
end
UI->>Main: request tab switch(tab)
Main->>Main: allowedTabs.includes(tab)?
alt not allowed
alt tab == firmware_flasher
Main->>Main: if connected/connecting -> trigger connection button (trigger("click"))
Main->>Main: open firmware_flasher via firmware_flasher_button__link (trigger("click"))
else other tab
Main->>Main: log tabSwitchUpgradeRequired, abort
end
else allowed
Main->>Main: set tab_switch_in_progress, cleanup, load content, analytics
Main->>Serial: ensure protocol/connect if needed
end
sequenceDiagram
participant App as Application
participant Serial as Serial (orchestrator)
participant PList as _protocols array
rect rgb(245,255,245)
note over Serial: Protocol selection & connect (refactored)
end
App->>Serial: selectProtocol(portPath)
Serial->>PList: find(protocol matching portPath/name)
PList-->>Serial: protocol instance or undefined
App->>Serial: connect(path, options)
Serial->>Serial: protocol = selectProtocol(path)
Serial->>PList: protocol?.connect(path, options)
rect rgb(255,250,240)
note over Serial: unified error handling & single callback
end
PList-->>Serial: result / error
Serial->>App: callback(result)
sequenceDiagram
participant MSP as MSP
participant Serial as Serial
participant Proto as Protocol
MSP->>MSP: parse message, verify CRC
alt CRC mismatch
alt Serial.protocol == "bluetooth" and Proto.shouldBypassCrc(expected)
MSP->>MSP: treat as bypassed (assign dataView, clear crcError)
else
MSP->>MSP: set crcError, reject message
end
else CRC OK
MSP->>MSP: dispatch normally
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-06-19T22:13:09.136ZApplied to files:
🔇 Additional comments (7)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/serial.js (2)
147-166: getDevices(): handle null to query current or aggregate; add graceful fallback.When
protocolTypeis null, this now returns[], contrary to the JSDoc. Either fallback to current protocol or return the union across all protocols.- async getDevices(protocolType = null) { + async getDevices(protocolType = null) { try { - // Get the appropriate protocol - const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; - - if (!targetProtocol) { - console.warn(`${this.logHead} No valid protocol for getting devices`); - return []; - } - - if (typeof targetProtocol.getDevices !== "function") { - console.error(`${this.logHead} Selected protocol does not implement getDevices`); - return []; - } - - return targetProtocol.getDevices?.() || []; + if (!protocolType) { + // Aggregate across protocols, best-effort + const lists = await Promise.all( + this._protocols.map(({ instance }) => + typeof instance.getDevices === "function" ? instance.getDevices() : Promise.resolve([]), + ), + ); + return lists.flat(); + } + const targetProtocol = this._getProtocol?.(protocolType.toLowerCase()); + if (!targetProtocol || typeof targetProtocol.getDevices !== "function") return []; + return (await targetProtocol.getDevices()) ?? []; } catch (error) { console.error(`${this.logHead} Error getting devices:`, error); return []; } }
36-67: Multiple listeners will break due to event detail shape inconsistency; implement standardized, backward-compatible transformation.The transformation in serial.js wraps "receive" events as
{ data: event.detail, protocolType: name }, breaking downstream listeners that expect the raw Uint8Array structure:
- handleReceiveBytes in WebSerial, WebBluetooth, and WebSocket access
info.detail.byteLength, expecting a Uint8Array- AutoDetect.js:33 calls
MSP.read(event.detail)expecting raw bytes, not{ data: Uint8Array, protocolType: name }- webstm32.js:25 calls
read_serial(event.detail.buffer)expecting a.bufferproperty that won't existOnly serial_backend.js:302 (
read_serial(event.detail.data)) is compatible with the new shape—this inconsistency indicates the transformation was added mid-process without fully accounting for all consumers.Implement the suggested standardization: preserve the original detail structure and spread it alongside protocolType for backward compatibility, whether dealing with Uint8Array or structured objects.
🧹 Nitpick comments (2)
src/js/serial.js (2)
18-24: Initialize protocols defensively and reduce lookup churn.Looks good. To avoid accidental mutation and repeated linear searches, consider a tiny helper or a Map. This keeps lookups O(1) and the list immutable.
- this._protocols = [ - { name: "webserial", instance: new WebSerial() }, - { name: "webbluetooth", instance: new WebBluetooth() }, - { name: "websocket", instance: new Websocket() }, - { name: "virtual", instance: new VirtualSerial() }, - ]; + this._protocols = Object.freeze([ + { name: "webserial", instance: new WebSerial() }, + { name: "webbluetooth", instance: new WebBluetooth() }, + { name: "websocket", instance: new Websocket() }, + { name: "virtual", instance: new VirtualSerial() }, + ]); + this._getProtocol = (name) => + this._protocols.find((p) => p.name === name)?.instance ?? null;
196-205: LGTM on getters; minor polish.Connected and connectionId getters look good. Consider documenting the connectionId format (protocol-specific) to set expectations for consumers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/js/main.js(1 hunks)src/js/protocols/WebSerial.js(0 hunks)src/js/serial.js(8 hunks)src/js/tabs/firmware_flasher.js(0 hunks)
💤 Files with no reviewable changes (2)
- src/js/protocols/WebSerial.js
- src/js/tabs/firmware_flasher.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/main.js (3)
src/js/gui.js (1)
GUI(473-473)src/js/gui_log.js (1)
gui_log(7-21)src/js/localization.js (1)
i18n(7-7)
🪛 Biome (2.1.2)
src/js/serial.js
[error] 107-107: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 124-124: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 182-182: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/js/serial.js (2)
90-101: Add protocol null guard to prevent runtime errors.If
selectProtocol()returnsundefined(which will happen with the bug in lines 77-82), line 95 will throw aTypeError: Cannot read property 'connect' of undefined.Additionally, the callback is passed to the protocol at line 95 and invoked again at line 99, which can cause double-invocation for protocols that use it (e.g., VirtualSerial).
Apply this diff to add the guard and clarify callback handling:
async connect(path, options, callback) { // Select the appropriate protocol based directly on the port path let result = false; try { this._protocol = this.selectProtocol(path); + if (!this._protocol) { + throw new Error("No protocol selected for path: " + path); + } result = await this._protocol.connect(path, options, callback); } catch (error) { console.error(`${this.logHead} Error during connection:`, error); } callback && callback(result); return result; }
164-173: Add explicit null guard for consistency and correct return value.While the
finallyblock issue is resolved, line 168 uses optional chaining which returnsundefinedwhentargetProtocolis null, notfalse. This meansresultbecomesundefinedinstead of remainingfalse, making the return value inconsistent.For consistency with
getDevices()(lines 141-144), add an explicit null guard.Apply this diff to add the guard:
async requestPermissionDevice(showAllDevices = false, protocolType) { - let result = false; try { const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; - result = await targetProtocol?.requestPermissionDevice(showAllDevices); + if (!targetProtocol) { + console.warn(`${this.logHead} No valid protocol for permission request`); + return false; + } + return await targetProtocol.requestPermissionDevice(showAllDevices); } catch (error) { console.error(`${this.logHead} Error requesting device permission:`, error); + return false; } - return result; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial.js(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#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/serial.js
🔇 Additional comments (4)
src/js/serial.js (4)
18-24: LGTM! Clean data-driven protocol initialization.The unified
_protocolsarray provides a cleaner approach to protocol management and enables consistent lookup patterns throughout the class.
36-67: LGTM! Event forwarding correctly iterates protocols.The iteration pattern with destructuring and protocol name injection provides clean event propagation with proper source identification.
108-117: LGTM! Past issues withfinallyblock resolved.The
finallyblock withreturnhas been removed, and the method now uses optional chaining for safe protocol access with a single callback invocation.
185-194: LGTM! Safe getter implementations with appropriate fallbacks.Both
connectedandconnectionIdgetters use optional chaining with sensible fallback values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/js/serial.js (2)
170-179: Add null guard for consistent return value.When
targetProtocolisnullorundefined, line 174's optional chaining returnsundefined, notfalse. The method should consistently returnfalsefor missing protocols, similar to howgetDevices()handles this with an explicit null guard.Apply this diff to add the guard:
async requestPermissionDevice(showAllDevices = false, protocolType) { - let result = false; try { const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; - result = await targetProtocol?.requestPermissionDevice(showAllDevices); + if (!targetProtocol) { + console.warn(`${this.logHead} No valid protocol for permission request`); + return false; + } + return await targetProtocol.requestPermissionDevice(showAllDevices); } catch (error) { console.error(`${this.logHead} Error requesting device permission:`, error); + return false; } - return result; }
73-89: Add null-safety and supportwss://protocol.The method has two issues:
- Null-safety: Line 83 calls
.startsWith()onportPath, but the JSDoc (line 71) indicates it can benull. This will throw a TypeError.- Missing
wss://support: Line 81's regex only matchestcp://andws://, notwss://(WebSocket Secure).A past review flagged both concerns and was marked as addressed, but they remain unresolved.
Apply this diff to fix both issues:
selectProtocol(portPath) { // Determine which protocol to use based on port path - let protocol; - + const s = typeof portPath === "string" ? portPath : ""; // Select protocol based on port path. Default to webSerial for // typical serial device identifiers (e.g., COM1, /dev/ttyUSB0). - if (portPath === "virtual") { - protocol = this._protocols.find((p) => p.name === "virtual")?.instance; - } else if (portPath === "manual" || /^(tcp|ws):\/\/([A-Za-z0-9.-]+)(?::(\d+))?$/.test(portPath)) { - protocol = this._protocols.find((p) => p.name === "websocket")?.instance; - } else if (portPath.startsWith("bluetooth")) { - protocol = this._protocols.find((p) => p.name === "webbluetooth")?.instance; - } else { - protocol = this._protocols.find((p) => p.name === "webserial")?.instance; - } - return protocol; + if (s === "virtual") { + return this._protocols.find((p) => p.name === "virtual")?.instance; + } else if (s === "manual" || /^(tcp|wss?):\/\/([A-Za-z0-9.-]+)(?::(\d+))?$/.test(s)) { + return this._protocols.find((p) => p.name === "websocket")?.instance; + } else if (s.startsWith("bluetooth")) { + return this._protocols.find((p) => p.name === "webbluetooth")?.instance; + } else { + return this._protocols.find((p) => p.name === "webserial")?.instance; + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/serial.js(8 hunks)src/js/tabs/firmware_flasher.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/js/tabs/firmware_flasher.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#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/serial.js
🔇 Additional comments (5)
src/js/serial.js (5)
18-24: LGTM! Clean protocol initialization.The unified
_protocolsarray provides a clean data-driven approach for protocol management, making lookups and iteration straightforward.
36-66: LGTM! Robust event forwarding.The event forwarding correctly iterates over protocols, uses optional chaining for safety, and properly injects the protocol name into event details for tracking.
114-123: LGTM! Clean disconnect implementation.The method correctly uses optional chaining to prevent errors, has a single callback invocation, and properly returns the result outside the try-catch block.
142-162: LGTM! Robust device retrieval.The method has proper null guards, validates that the protocol implements
getDevices, and consistently returns an empty array on errors.
191-200: LGTM! Safe getter implementations.Both getters correctly use optional chaining with appropriate fallback values (
falseforconnected,nullforconnectionId).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/js/serial.js (1)
170-179: Refactor for consistency withgetDevices()pattern.Line 174 uses optional chaining
targetProtocol?.requestPermissionDevice(), which returnsundefinediftargetProtocolis missing. However, line 171 initializesresult = false, suggesting a boolean return type. This inconsistency could confuse callers.Consider adding explicit guards like
getDevices()does (lines 147-155) for clarity and consistent error handling.Apply this diff for consistency:
async requestPermissionDevice(showAllDevices = false, protocolType) { let result = false; try { const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; + if (!targetProtocol) { + console.warn(`${this.logHead} No valid protocol for permission request`); + return false; + } result = await targetProtocol.requestPermissionDevice(showAllDevices); } catch (error) { console.error(`${this.logHead} Error requesting device permission:`, error); } return result; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/msp.js(1 hunks)src/js/serial.js(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#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/serial.js
🧬 Code graph analysis (1)
src/js/msp.js (1)
src/js/serial.js (2)
serial(211-211)serial(211-211)
🔇 Additional comments (7)
src/js/serial.js (7)
18-24: LGTM: Clean refactor to unified protocol collection.The shift from individual protocol fields to a data-driven
_protocolsarray with metadata improves maintainability and makes protocol lookup more consistent.
36-66: LGTM: Event forwarding properly updated.The event forwarding logic correctly iterates through the new
_protocolscollection and properly injects the protocol name into event details using the destructurednameproperty.
73-87: LGTM: Protocol selection with proper null-safety.The method correctly handles null/non-string
portPathby coercing to empty string, includeswss://in the websocket regex pattern, and uses optional chaining to safely return the protocol instance.
112-121: LGTM: Disconnect with proper optional chaining.The method correctly uses optional chaining for
this._protocol?.disconnect(), returns outside the try-catch block, and invokes the callback exactly once. Good error handling.
126-135: LGTM: Send method with proper error handling.The method correctly uses optional chaining and returns a well-formed result object
{ bytesSent: 0 }on error, which maintains consistency with the expected return shape.
142-162: LGTM: getDevices with comprehensive guards.The method properly checks for missing protocol and missing method implementation, and uses optional chaining for the final call. Good defensive programming.
191-207: LGTM: Clean getter implementations.All three getters (
connected,connectionId,protocolType) properly use optional chaining and provide sensible default values when the protocol is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/serial.js (1)
173-182: Return value inconsistency when protocol is not found.If
targetProtocolis undefined at line 176, the optional chaining at line 177 assignsundefinedtoresult, which is then returned. This is inconsistent with:
- The initialization
result = falseat line 174- The JSDoc return type
Promise<Object>at line 171- The pattern in
getDevices()which returns[]when protocol is missingConsider adding a guard similar to
getDevices()to ensure a consistent falsy value is returned.Apply this diff to ensure consistent return behavior:
async requestPermissionDevice(showAllDevices = false, protocolType) { - let result = false; try { const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; + if (!targetProtocol) { + console.warn(`${this.logHead} No valid protocol for permission request`); + return false; + } - result = await targetProtocol?.requestPermissionDevice(showAllDevices); + return await targetProtocol.requestPermissionDevice(showAllDevices); } catch (error) { console.error(`${this.logHead} Error requesting device permission:`, error); + return false; } - return result; }
🧹 Nitpick comments (1)
src/js/serial.js (1)
145-165: Optional: Remove redundant optional chaining.Line 160 uses optional chaining
targetProtocol.getDevices?.()after already verifying thattargetProtocolexists and implementsgetDevices(lines 150-158). The optional chaining is redundant here.If desired, simplify line 160:
- return targetProtocol.getDevices?.() || []; + return targetProtocol.getDevices() || [];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial.js(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#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/serial.js
🔇 Additional comments (6)
src/js/serial.js (6)
18-24: LGTM: Clean data-driven protocol management.The unified
_protocolsarray with metadata provides a maintainable, consistent approach for protocol management.
36-66: LGTM: Event forwarding correctly updated.The refactored event forwarding properly iterates over the new
_protocolsstructure and injects protocol type information into event details.
73-87: LGTM: Protocol selection with proper null-safety.The method correctly handles null/undefined portPath values and includes
wss://in the protocol detection regex. The optional chaining allows undefined returns, which are handled by try-catch in the calling code per the maintainer's design choice.
94-108: LGTM: Connection logic with enhanced error reporting.The method correctly uses try-catch to handle potential protocol selection failures, with improved error logging that includes both the path and protocol information as requested. Callback is invoked exactly once after the try-catch block.
115-124: LGTM: Disconnect logic with proper null-safety.The method correctly uses optional chaining for safe protocol access and invokes the callback exactly once.
194-210: LGTM: Getters with proper null-safety.All getter implementations correctly use optional chaining or ternary checks to safely handle cases where
this._protocolmay be undefined, with appropriate default values.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/serial.js (1)
173-182: Fix: method returns undefined instead of false when protocol not found.When
targetProtocolis undefined (e.g., invalidprotocolType), the optional chaining on line 177 causesresultto becomeundefined, overwriting thefalseinitialization. This is inconsistent with the method's boolean return pattern used elsewhere.Apply this diff to ensure consistent false return:
async requestPermissionDevice(showAllDevices = false, protocolType) { - let result = false; try { const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; - result = await targetProtocol?.requestPermissionDevice(showAllDevices); + if (!targetProtocol) { + console.warn(`${this.logHead} No valid protocol for permission request`); + return false; + } + return await targetProtocol.requestPermissionDevice(showAllDevices); } catch (error) { console.error(`${this.logHead} Error requesting device permission:`, error); + return false; } - return result; }
🧹 Nitpick comments (2)
src/js/serial.js (2)
115-124: Consider optional chaining for graceful null handling.The method correctly handles errors and avoids the
finallyreturn issue from past reviews. However,this._protocol.disconnect()on line 118 will throw if the protocol is null (e.g., before any connection). While the try-catch handles this, optional chaining would be more graceful:async disconnect(callback) { let result = false; try { - result = await this._protocol.disconnect(); + result = await this._protocol?.disconnect() ?? false; } catch (error) { console.error(`${this.logHead} Error during disconnect:`, error); } callback?.(result); return result; }
129-138: Consider optional chaining for consistency.The method correctly returns
{ bytesSent: 0 }on error. For consistency with the graceful null handling in other methods, consider using optional chaining:async send(data, callback) { let result; try { - result = await this._protocol.send(data, callback); + result = await this._protocol?.send(data, callback) ?? { bytesSent: 0 }; } catch (error) { result = { bytesSent: 0 }; console.error(`${this.logHead} Error sending data:`, error); } return result; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/msp.js(1 hunks)src/js/serial.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/msp.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#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/serial.js
🔇 Additional comments (6)
src/js/serial.js (6)
18-24: LGTM! Clean data-driven protocol management.The refactor to a
_protocolsarray simplifies protocol management and enables uniform iteration for event forwarding and lookups.
36-66: LGTM! Event forwarding properly implemented.The iteration over
_protocolscorrectly sets up event forwarding with protocol type injection, making events more informative.
73-87: LGTM! Protocol selection is robust.The method correctly handles null/undefined
portPath, includeswss://support, and uses optional chaining appropriately. The past critical bug with_getProtocolhas been resolved.
94-108: LGTM! Connect error handling is consistent.The method correctly handles connection errors with try-catch, includes protocol information in error logging as discussed in past reviews, and ensures single callback invocation outside the try block.
145-165: LGTM! Robust device retrieval with proper guards.The method includes comprehensive null checks and method existence validation. The optional chaining on line 160 is redundant after the check on line 155, but this doesn't affect correctness.
194-210: LGTM! Getters properly handle null protocol.All three getters use appropriate optional chaining and provide sensible fallback values when
this._protocolis null.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/serial.js (1)
146-166: Behavior mismatch: getDevices() doesn’t honor “specific type or current protocol”.
With protocolType=null it returns [], not current protocol devices; also not awaited, so try/catch won’t catch async errors.Apply this diff:
- const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; + let targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; + if (!targetProtocol) { + // Fallback: use current protocol, else default to webserial. + targetProtocol = this._protocol ?? this._protocols.find((p) => p.name === "webserial")?.instance; + } @@ - if (!targetProtocol) { + if (!targetProtocol) { console.warn(`${this.logHead} No valid protocol for getting devices`); return []; } @@ - return targetProtocol.getDevices?.() || []; + return (await targetProtocol.getDevices()) ?? [];Based on learnings.
♻️ Duplicate comments (2)
src/js/serial.js (2)
96-107: Add protocol guard to avoid avoidable throw; keep single-callback pattern.
Try/catch logs the error, but guarding keeps logs cleaner and control explicit.Apply this diff:
- try { - this._protocol = this.selectProtocol(path); - result = await this._protocol.connect(path, options); + try { + this._protocol = this.selectProtocol(path); + if (this._protocol) { + result = await this._protocol.connect(path, options); + } else { + console.error(`${this.logHead} No protocol selected for path '${path}'.`); + } } catch (error) {
129-139: Fix double-callback risk and add explicit protocol guard in send.
The protocol may also invoke the callback; then Serial calls it again. Align with connect/disconnect and own the public callback.Apply this diff:
- async send(data, callback) { - let result; - try { - result = (await this._protocol?.send(data, callback)) ?? { bytesSent: 0 }; - } catch (error) { - result = { bytesSent: 0 }; - console.error(`${this.logHead} Error sending data:`, error); - } - callback?.(result); - return result; - } + async send(data, callback) { + let result = { bytesSent: 0 }; + try { + if (!this._protocol || typeof this._protocol.send !== "function") { + throw new Error("No protocol available for send"); + } + // Serial owns public callbacks; do not pass callback into protocol. + result = await this._protocol.send(data); + } catch (error) { + console.error(`${this.logHead} Error sending data:`, error); + } + callback?.(result); + return result; + }Check if any protocol
sendstill expects a callback:#!/usr/bin/env bash rg -nP -C1 '\bsend\s*\(([^\)]*callback[^\)]*)\)' src/js/protocols
🧹 Nitpick comments (2)
src/js/serial.js (2)
36-67: Harden event forwarding: guard non-object details and normalize 'receive' shape.
Spreading a non-object detail will throw. Also, keep a consistent payload shape for 'receive'.Apply this diff:
- instance.addEventListener(eventType, (event) => { - let newDetail; - if (event.type === "receive") { - // For 'receive' events, we need to handle the data differently - newDetail = { - data: event.detail, - protocolType: name, - }; - } else { - // For other events, we can use the detail directly - newDetail = { - ...event.detail, - protocolType: name, - }; - } + instance.addEventListener(eventType, (event) => { + const baseDetail = + event.type === "receive" + ? { data: event.detail } + : (event && typeof event.detail === "object" && event.detail !== null) + ? { ...event.detail } + : { value: event?.detail }; + const newDetail = { ...baseDetail, protocolType: name }; // Dispatch the event with the new detail this.dispatchEvent( new CustomEvent(event.type, { detail: newDetail, bubbles: event.bubbles, cancelable: event.cancelable, }), ); });
174-183: Guard invalid protocol types and accept public API synonyms.
JSDoc uses “serial/bluetooth”, registry uses “webserial/webbluetooth”. Add mapping and guard.Apply this diff:
- async requestPermissionDevice(showAllDevices = false, protocolType) { - let result = false; - try { - const targetProtocol = this._protocols.find((p) => p.name === protocolType?.toLowerCase())?.instance; - result = await targetProtocol?.requestPermissionDevice(showAllDevices); - } catch (error) { - console.error(`${this.logHead} Error requesting device permission:`, error); - } - return result; - } + async requestPermissionDevice(showAllDevices = false, protocolType) { + const normalized = protocolType?.toLowerCase?.(); + const mapped = + normalized === "serial" ? "webserial" : + normalized === "bluetooth" ? "webbluetooth" : + (normalized === "ws" || normalized === "wss" || normalized === "websocket") ? "websocket" : + normalized === "virtual" ? "virtual" : + normalized; + try { + const targetProtocol = this._protocols.find((p) => p.name === mapped)?.instance; + if (!targetProtocol || typeof targetProtocol.requestPermissionDevice !== "function") { + console.warn(`${this.logHead} No valid protocol for permission request: ${protocolType}`); + return false; + } + return await targetProtocol.requestPermissionDevice(showAllDevices); + } catch (error) { + console.error(`${this.logHead} Error requesting device permission:`, error); + return false; + } + }Based on learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/serial.js(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#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/serial.js
🔇 Additional comments (3)
src/js/serial.js (3)
18-24: Protocol registry LGTM.
Initialization is clear and self-contained. No issues here.
116-124: Disconnect flow LGTM.
Optional chaining with nullish coalesce is fine; callback invoked once afterward.
195-211: Getters LGTM.
Accessors are null-safe and consistent with protocol selection.
|
|
Preview URL: https://pr4670.betaflight-app-preview.pages.dev |
|
i dont understand how coderabbitAI went from "(1)" duplicate comment to "(2)", and are now different than the original. |
|
Already told the bot it is already handled in try catch block. |
|
unsure how to "fully" test, but i noticed during flashing i was able to tab-out, then upon return to flashing tab, flashing continued unobstructed. |
3 participants
Summary by CodeRabbit
Refactor
Bug Fixes
Chore