Fix cleanup of autodetect event handlers #4664
Conversation
WalkthroughConverted AutoDetect.verifyBoard to async, added precondition checks for port availability and targets loaded, reworked connect flow into a guarded try/catch/finally with one-time listeners and local result tracking, and introduced a centralized async cleanup() that removes listeners, clears MSP state, and disconnects serial on failure or finish. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AD as AutoDetect.verifyBoard()
participant SP as SerialPort
participant MSP as MSP Layer
Note over AD: async verifyBoard()
User->>AD: invoke verifyBoard()
alt already connected
AD-->>User: return (connected)
else not connected
AD->>AD: register one-time listeners & init local result
AD->>AD: check port available & target.isLoaded?
alt preconditions fail
AD->>AD: cleanup()
AD-->>User: fail
else preconditions ok
AD->>SP: connect() (await in try/catch)
alt connect success
AD->>MSP: confirm target/isLoaded (optional chaining)
alt target loaded
AD-->>User: success
else target not loaded
AD->>AD: cleanup()
AD-->>User: fail
end
else connect failure
AD->>AD: cleanup()
AD-->>User: fail
end
end
end
rect rgba(230,245,255,0.6)
Note right of AD: cleanup(): await serial.disconnect(), remove serial/MSP listeners, clear MSP state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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-09-02T07:45:48.606ZApplied to files:
🧬 Code graph analysis (1)src/js/utils/AutoDetect.js (4)
🔇 Additional comments (3)
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 |
haslinghuis
changed the title
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/utils/AutoDetect.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/utils/AutoDetect.js (4)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/gui.js (1)
TABS(8-8)src/js/gui_log.js (1)
gui_log(7-21)src/js/msp.js (1)
MSP(5-455)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/utils/AutoDetect.js (1)
139-153:this.logHeadis undefined; logs will print “undefined …”.Define
this.logHeadin the constructor (or remove it from the message).Add in constructor:
constructor() { this.logHead = "[AUTODETECT]"; this.board = FC.CONFIG.boardName; this.targetAvailable = false; // ... }
🧹 Nitpick comments (1)
src/js/utils/AutoDetect.js (1)
49-51: Re-check port availability after permission flow to avoid false negatives.
PortHandler.portAvailablecan be stale immediately after a permission grant. Refresh the device list before gating on availability (e.g., ensure WebSerial.getDevices() is re-queried in the flow that setsportAvailable).Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/utils/AutoDetect.js(3 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/utils/AutoDetect.js
🧬 Code graph analysis (1)
src/js/utils/AutoDetect.js (5)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/serial.js (2)
serial(319-319)serial(319-319)src/js/gui.js (1)
TABS(8-8)src/js/gui_log.js (1)
gui_log(7-21)src/js/msp.js (1)
MSP(5-455)
…up if a connect was attempted. Move listener registration to just-in-time.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/utils/AutoDetect.js (1)
40-68: Register listeners just-in-time to avoid disconnecting unrelated sessions and leaving stale handlers.Listeners are registered at lines 40-41 before the guard conditions at lines 50-61. When guards reject the attempt (no port, targets not loaded, or already connected),
resultstaysfalseandcleanup()runs at line 66—potentially disconnecting an existing session or leaving the once-registered disconnect listener attached for the next unrelated connection.Apply this diff to register listeners only when you will actually call
serial.connect():async verifyBoard() { const port = PortHandler.portPicker.selectedPort; if (!port.startsWith("virtual")) { - serial.addEventListener("connect", this.boundHandleConnect, { once: true }); - serial.addEventListener("disconnect", this.boundHandleDisconnect, { once: true }); - // Safely check firmware_flasher.targets (use optional chaining so this doesn't throw when undefined) const isLoaded = TABS.firmware_flasher?.targets ? Object.keys(TABS.firmware_flasher.targets).length > 0 : false; let result = false; + let attempted = false; try { if (!PortHandler.portAvailable) { gui_log(i18n.getMessage("firmwareFlasherNoValidPort")); } else if (!isLoaded) { gui_log(i18n.getMessage("firmwareFlasherNoTargetsLoaded")); } else if (serial.connected || serial.connectionId) { console.warn("Attempting to connect while there still is a connection", serial.connected); gui_log(i18n.getMessage("serialPortOpenFail")); } else { + // Register one-time listeners only when we are actually going to connect + serial.addEventListener("connect", this.boundHandleConnect, { once: true }); + serial.addEventListener("disconnect", this.boundHandleDisconnect, { once: true }); console.log("Connecting to serial port", port); gui_log(i18n.getMessage("firmwareFlasherDetectBoardQuery")); + attempted = true; result = await serial.connect(port, { baudRate: PortHandler.portPicker.selectedBauds || 115200 }); } } catch (error) { console.error("Failed to connect:", error); } finally { - if (!result) { + if (!result && attempted) { this.cleanup(); } } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/utils/AutoDetect.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T07:45:48.606Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#4583
File: src/js/tabs/firmware_flasher.js:949-961
Timestamp: 2025-09-02T07:45:48.606Z
Learning: In src/js/tabs/firmware_flasher.js, the config file loading code path after firmware loading (in the load_file click handler) cannot be reached when UF2 firmware is loaded, according to the maintainer blckmn. This code is maintained for backward compatibility with unified target settings and will be refactored in a future PR.
Applied to files:
src/js/utils/AutoDetect.js
🧬 Code graph analysis (1)
src/js/utils/AutoDetect.js (5)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/serial.js (2)
serial(319-319)serial(319-319)src/js/gui.js (1)
TABS(8-8)src/js/gui_log.js (1)
gui_log(7-21)src/js/msp.js (1)
MSP(5-455)
🔇 Additional comments (2)
src/js/utils/AutoDetect.js (2)
43-46: LGTM—optional chaining guards against undefinedTABS.firmware_flasher.The optional chaining at line 44 properly prevents a TypeError if
TABS.firmware_flasheris undefined, addressing the past critical issue.
116-116: LGTM—centralized cleanup after board verification.Calling
this.cleanup()here consolidates resource release after the board verification flow completes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/utils/AutoDetect.js (1)
196-196: Consider using a string literal directly for clarity.While the fallback
this.logHead || "AutoDetect"is functional,this.logHeadis undefined in this class. Using"AutoDetect"directly (as suggested in previous reviews) would be simpler and avoid referencing an undefined property.Apply this diff:
- console.error(`${this.logHead || "AutoDetect"}: serial.disconnect() failed:`, error); + console.error("AutoDetect: serial.disconnect() failed:", error);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/utils/AutoDetect.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T07:45:48.606Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#4583
File: src/js/tabs/firmware_flasher.js:949-961
Timestamp: 2025-09-02T07:45:48.606Z
Learning: In src/js/tabs/firmware_flasher.js, the config file loading code path after firmware loading (in the load_file click handler) cannot be reached when UF2 firmware is loaded, according to the maintainer blckmn. This code is maintained for backward compatibility with unified target settings and will be refactored in a future PR.
Applied to files:
src/js/utils/AutoDetect.js
🧬 Code graph analysis (1)
src/js/utils/AutoDetect.js (5)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/gui.js (1)
TABS(8-8)src/js/gui_log.js (1)
gui_log(7-21)src/js/serial.js (2)
serial(319-319)serial(319-319)src/js/msp.js (1)
MSP(5-455)
🔇 Additional comments (3)
src/js/utils/AutoDetect.js (3)
36-74: LGTM! Well-structured async conversion with proper safeguards.The changes correctly address previous review concerns:
- Optional chaining prevents TypeError when
TABS.firmware_flasheris undefined- Listeners registered just-in-time only when actually connecting, preventing stale registrations
- The
attemptedflag ensures cleanup runs only when needed- Precondition checks prevent connection attempts in invalid states
The try/catch/finally structure with conditional cleanup is sound.
120-120: LGTM! Proper centralization of cleanup logic.Calling the centralized
cleanup()method here consolidates resource release, making the flow easier to maintain.
190-207: LGTM! Robust cleanup with proper error handling.The try/catch/finally structure correctly ensures:
- Serial disconnection triggers the once-registered disconnect handler (line 193)
- Errors don't block the cleanup flow (lines 194-196)
- Listeners and MSP state are always cleaned up (lines 199-205)
- The once-registered disconnect listener auto-removes, so explicit removal is unnecessary (line 201)
|
|
Preview URL: https://pr4664.betaflight-app-preview.pages.dev |
3 participants
Summary by CodeRabbit
Bug Fixes
Refactor