Fix flash state #4653
Fix flash state #4653
Conversation
WalkthroughPreserves and restores pre-flash UI state in firmware_flasher (adds preFlashingMessage/preFlashingMessageType, preserve/reset methods), centralizes STM32 protocol error handling via STM32Protocol.handleError(resetRebootMode = true) to unlock GUI and restore flasher UI state, and guards AutoDetect.verifyBoard when no port is available. (38 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Firmware UI
participant DFU as DFU Prompt
participant STM as STM32Protocol
User->>UI: Click "Flash Firmware"
UI->>UI: preservePreFlashingState()
UI->>DFU: requestPermission()
alt Permission granted
DFU-->>UI: device.path
UI->>STM: connect (DFU/UF2)
alt connect success
STM-->>UI: flashing progress / success
UI->>UI: resetFlashingState()
else connect error
STM->>STM: handleError()
note right of STM #D6EAF8: unlock GUI.connect_lock\noptional rebootMode reset\nreset flasher UI state
STM-->>UI: error
UI-->>User: show error, ready to retry
end
else Permission canceled / error
DFU-->>UI: canceled / error
UI->>UI: resetFlashingState()
UI-->>User: ready to retry
end
sequenceDiagram
autonumber
participant STM as STM32Protocol
participant GUI as GUI State
participant FFS as firmware_flasher
STM->>STM: on timeout/failure -> handleError(resetRebootMode = true)
STM-->>GUI: unlock connect_lock
STM-->>FFS: resetFlashingState()
note right of FFS #FDEBD0: restore pre-flash message or show not-loaded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ 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 (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-02T07:45:48.606ZApplied to files:
🧬 Code graph analysis (2)src/js/protocols/webstm32.js (2)
src/js/utils/AutoDetect.js (2)
🔇 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 |
haslinghuis
force-pushed
the
fix-flash-state
branch
from
7a79766 to
c28ed48
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
does not continue / does not enable flash-button |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
827-827: Minor: Trailing whitespace.Line 827 has trailing whitespace that could be removed for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/tabs/firmware_flasher.js
🧬 Code graph analysis (1)
src/js/tabs/firmware_flasher.js (5)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/localization.js (1)
i18n(7-7)src/js/gui.js (1)
GUI(465-465)src/js/Analytics.js (1)
tracking(7-7)src/js/protocols/webstm32.js (1)
STM32(1007-1007)
🔇 Additional comments (4)
src/js/tabs/firmware_flasher.js (4)
783-792: LGTM! Centralized state restoration.The
resetFlashingStatehelper correctly restores all UI state after errors: re-enables buttons, clears the flashing flag, displays a neutral message, and resumes the sponsor interval.
798-803: LGTM! Proper error handling for DFU connection.The try/catch wrapper correctly handles DFU connection failures by logging the error and invoking
resetFlashingStateto restore the UI, preventing the stuck state described in the issue.
818-823: LGTM! Consistent error handling for STM32 connection.The try/catch wrapper mirrors the DFU error handling pattern, ensuring consistent state restoration across both connection types.
828-834: LGTM! Key fix for the reported issue.The catch handler correctly addresses the issue described in #4646: when the user cancels the DFU permission request or uses the "I can't find my DFU device" flow, the state is properly restored via
resetFlashingState, allowing the user to retry without reloading or unplugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
823-830: LGTM! Permission rejection handling fixes the core issue.The rejection handler correctly addresses the stuck-state bug described in issue #4646 by restoring UI state when the user cancels DFU permission or when permission fails. The explanatory comment is helpful.
Nitpick: Remove trailing whitespace.
Line 823 has trailing whitespace (empty line with spaces). Apply this diff to clean it up:
- + DFU.requestPermission().then((device) => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/tabs/firmware_flasher.js
🧬 Code graph analysis (1)
src/js/tabs/firmware_flasher.js (5)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/localization.js (1)
i18n(7-7)src/js/gui.js (1)
GUI(465-465)src/js/Analytics.js (1)
tracking(7-7)src/js/protocols/webstm32.js (1)
STM32(1007-1007)
🔇 Additional comments (3)
src/js/tabs/firmware_flasher.js (3)
783-792: LGTM! Centralizing state restoration improves maintainability.The
resetFlashingStatehelper function correctly restores all flashing-related UI state and resumes sponsor intervals. This centralization ensures consistent state restoration across error paths and directly addresses the stuck-state issue described in the PR objectives.
798-801: LGTM! DFU connection error handling properly restores state.The rejection handler correctly logs the error and invokes
resetFlashingState(), ensuring the flash button is re-enabled if DFU connection fails.
816-819: LGTM! STM32 connection error handling properly restores state.The rejection handler correctly logs the error and invokes
resetFlashingState(), ensuring consistent state restoration for serial flash failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
823-823: Remove trailing whitespace.Line 823 has trailing whitespace.
Apply this diff:
- +
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code graph analysis (1)
src/js/tabs/firmware_flasher.js (5)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/localization.js (1)
i18n(7-7)src/js/gui.js (1)
GUI(465-465)src/js/Analytics.js (1)
tracking(7-7)src/js/protocols/webstm32.js (1)
STM32(1007-1007)
🔇 Additional comments (4)
src/js/tabs/firmware_flasher.js (4)
798-801: LGTM!The error handler correctly restores the flashing state when DFU connection fails, preventing the stuck state described in issue #4646.
816-819: LGTM!The error handler correctly restores the flashing state when STM32 connection fails, maintaining consistency with the DFU error handling.
824-833: LGTM! This fixes the core issue.The dual error handlers correctly address the issue described in #4646:
- If permission is granted but connection fails, state is restored
- If permission is denied or user cancels, state is restored
This allows users to retry flashing without reloading or unplugging.
783-792: No cleanup needed for GUI.connect_lock
GUI.connect_lockisn’t set during the flash flow, soresetFlashingStatedoesn’t need to clear it.Likely an incorrect or invalid review comment.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
1533-1548: LGTM with optional improvement suggestion.The
preservePreFlashingStatemethod correctly captures both the message content and its type by checking CSS classes. The logic comprehensively covers all four FLASH_MESSAGE_TYPES values with an appropriate default.Consider adding JSDoc comments to both new methods to document their purpose, especially since they're part of the public API of
firmware_flasher:+/** + * Resets the flashing state and UI elements after a flash operation completes or fails. + * Restores pre-flashing message if firmware is loaded, enables appropriate buttons. + */ firmware_flasher.resetFlashingState = function () {+/** + * Preserves the current firmware message and its display type before flashing starts. + * This state is restored by resetFlashingState() if needed. + */ firmware_flasher.preservePreFlashingState = function () {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/tabs/firmware_flasher.js
🧬 Code graph analysis (1)
src/js/tabs/firmware_flasher.js (3)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/localization.js (1)
i18n(7-7)src/js/gui.js (1)
GUI(465-465)
🔇 Additional comments (4)
src/js/tabs/firmware_flasher.js (4)
41-43: LGTM!The new state preservation properties are well-named and properly initialized. The comment clearly explains their purpose.
810-819: LGTM!The refactored error handling now uses centralized state restoration via
resetFlashingState(), which aligns with the PR objective to properly restore UI state when DFU permission is denied or the user cancels.
1391-1392: LGTM!Preserving the pre-flashing state before flashing begins ensures the UI can be correctly restored if the flash operation is interrupted or fails. The placement is appropriate.
1513-1531: LGTM!The
resetFlashingStatemethod properly restores UI state by re-enabling buttons, restoring the pre-flash message when firmware is loaded, and resuming intervals. The conditional logic correctly handles both firmware-loaded and not-loaded states.
recording-2025-10-03_12.59.34.mp4 |
|
|
Preview URL: https://pr4653.betaflight-app.pages.dev |
3 participants
Summary by CodeRabbit
Bug Fixes
Refactor