Add Save and Erase button for onboard logging #4673
Conversation
WalkthroughAdds a "Save & Erase" UI action and English translation; implements conditional erase that runs only after a complete, validated blackbox download; updates save flow, button handlers, disabled-state selector, and end-of-file/chunk handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "Onboard UI"
participant Saver as "flash_save_begin(alsoErase=true)"
participant File as "Local file"
participant Eraser as "conditionallyEraseFlash()"
User->>UI: Click "Save & Erase"
UI->>Saver: Start save (alsoErase=true)
loop per chunk
Saver->>File: Write chunk
File-->>Saver: Ack
end
alt completed and validated download
Saver->>Eraser: conditionallyEraseFlash(maxBytes, nextAddress)
Eraser->>Eraser: Verify fullness & continuity
Eraser-->>UI: Erase result
UI-->>User: Download + Erase complete
else incomplete or cancelled
Saver-->>UI: Log dataflashSaveIncompleteWarning
UI-->>User: Download complete (no erase)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/js/tabs/onboard_logging.js (1)
547-549: EOF handling is correct, but consider adding error handling for the erase operation.The code correctly delegates validation to
conditionallyEraseFlashwhen EOF is reached early. However, neither this path nor the erase command itself includes error handling for MSP command failures.Consider adding an error callback to handle erase failures:
function conditionallyEraseFlash(maxBytes, nextAddress) { if (!isNaN(maxBytes) && nextAddress >= maxBytes) { $(".dataflash-confirm-erase").addClass("erasing"); - MSP.send_message(MSPCodes.MSP_DATAFLASH_ERASE, false, false, poll_for_erase_completion); + MSP.send_message(MSPCodes.MSP_DATAFLASH_ERASE, false, false, poll_for_erase_completion, () => { + gui_log(i18n.getMessage("dataflashEraseError") || "Failed to erase flash."); + $(".dataflash-confirm-erase").removeClass("erasing"); + }); } else { gui_log( i18n.getMessage("dataflashSaveIncompleteWarning") || "Downloaded size did not match expected size - not erasing flash.", ); } }Note: This assumes MSP.send_message supports an error callback parameter. Verify the signature before applying this change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
locales/en/messages.json(1 hunks)src/js/tabs/onboard_logging.js(3 hunks)src/tabs/onboard_logging.html(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/onboard_logging.js (5)
src/js/msp.js (1)
MSP(5-455)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/gui_log.js (1)
gui_log(7-21)src/js/receiver_msp/receiver_msp.js (1)
i18n(8-8)src/js/localization.js (1)
i18n(7-7)
🔇 Additional comments (3)
locales/en/messages.json (1)
3561-3564: LGTM
String addition is consistent with surrounding keys and follows existing formatting conventions.src/js/tabs/onboard_logging.js (2)
481-491: Validation logic is sound.The conditional erase correctly verifies that the download completed by checking both that
maxBytesis valid and thatnextAddress >= maxBytes. The fallback message ensures graceful degradation if the translation key is missing.
530-533: Correctly implements conditional erase after successful download.The logic properly checks both cancellation status and the
alsoEraseflag before triggering the erase operation. Closing the file beforehand avoids potential file handle conflicts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/js/tabs/onboard_logging.js (1)
548-550: Consider adding!saveCancelledcheck for consistency.The erase is triggered when receiving a zero-byte EOF block, but unlike the similar logic at lines 532-533, this doesn't verify that the user hasn't cancelled the operation. While this code path represents a successful download completion, adding the
!saveCancelledcheck would ensure consistency and handle the edge case where a user cancels between receiving the final data block and the EOF marker.Apply this diff for consistency:
- if (alsoErase) { + if (!saveCancelled && alsoErase) { conditionallyEraseFlash(maxBytes, nextAddress); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/onboard_logging.js (5)
src/js/msp.js (1)
MSP(5-455)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/gui_log.js (1)
gui_log(7-21)src/js/receiver_msp/receiver_msp.js (1)
i18n(8-8)src/js/localization.js (1)
i18n(7-7)
🔇 Additional comments (4)
src/js/tabs/onboard_logging.js (4)
106-106: LGTM! Click handler for "Save & Erase" properly implemented.The click handler correctly invokes
flash_save_begin(true)to enable the conditional erase flow, addressing the missing handler identified in previous reviews.
482-492: LGTM! Conditional erase logic properly implements integrity check.The function correctly verifies that the downloaded size (
nextAddress) meets the expected size (maxBytes) before erasing the dataflash, addressing the PR objective to ensure download integrity. The warning message with fallback is appropriate for cases where the download is incomplete.
494-494: LGTM! Function signature properly extended with backward compatibility.The
alsoEraseparameter with a default value offalsemaintains backward compatibility for existing callers while enabling the new "Save & Erase" workflow.
531-534: LGTM! Conditional erase correctly integrated after successful download.The erase logic is properly gated by both
!saveCancelledandalsoErasechecks, ensuring that erase only occurs when the user hasn't cancelled the operation and has explicitly requested the "Save & Erase" action.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/onboard_logging.js (4)
src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/gui_log.js (1)
gui_log(7-21)src/js/receiver_msp/receiver_msp.js (1)
i18n(8-8)src/js/localization.js (1)
i18n(7-7)
🔇 Additional comments (2)
src/js/tabs/onboard_logging.js (2)
359-361: LGTM: disabled-state now covers Save, Erase, and Save&Erase.Selector correctly includes
save-flash-erasealongside the others. Nice catch-up with the new button.
493-493: LGTM: erase is gated behind explicit intent and size validation.The
alsoEraseflow only triggers after successful completion and defers toconditionallyEraseFlashfor integrity checks. This matches the “download and erase only after full, validated download” requirement.Please confirm the UX expectation: no dialog is shown during Save+Erase, but the erase progress/notification is handled via
poll_for_erase_completion. If a visible spinner is desired, consider showing the existing.dataflash-confirm-erasedialog in non-interactive mode during Save+Erase.Also applies to: 531-534, 547-549
|
Tested few times, works fine |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/onboard_logging.js (1)
556-558: Consider adding explicit cancellation check for defensive programming.The conditional erase after EOF is functionally correct, as cancellation prevents further callbacks. However, for clarity and defensive programming, consider adding an explicit
!saveCancelledcheck here to match the pattern at Line 540.Apply this diff for additional safety:
- if (alsoErase) { + if (!saveCancelled && alsoErase) { conditionallyEraseFlash(maxBytes, nextAddress); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
locales/en/messages.json(1 hunks)src/js/tabs/onboard_logging.js(6 hunks)src/tabs/onboard_logging.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- locales/en/messages.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/onboard_logging.js (4)
src/js/fc.js (1)
FC(132-1006)src/js/msp.js (1)
MSP(5-455)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/gui_log.js (1)
gui_log(7-21)
🔇 Additional comments (8)
src/tabs/onboard_logging.html (2)
113-113: LGTM!The new "Save & Erase" button for the non-MSC path is properly structured with appropriate CSS classes and i18n integration. The placement between the erase and save buttons provides a logical workflow.
115-116: No issues found; translation key exists and is correctly referenced.The translation key
dataflashSaveFileDepreciationHintis correctly implemented and exists consistently across all 19 locale files. The code at lines 115-116 properly references this key, which uses the "Depreciation" spelling throughout the entire translation system—this is intentional, not a typo in the PR.Likely an incorrect or invalid review comment.
src/js/tabs/onboard_logging.js (6)
105-113: LGTM!The event handlers are correctly implemented with explicit boolean parameters and proper
preventDefault()calls. This ensures thealsoEraseparameter receives the intended boolean value rather than a jQuery event object.
367-369: LGTM!The selector correctly includes all relevant buttons (
erase-flash,save-flash-erase, andsave-flash) in the disabled state toggle. The logic appropriately disables all actions whenFC.DATAFLASH.usedSize === 0.
489-500: LGTM!The
conditionallyEraseFlashfunction correctly validates that the download completed successfully before erasing. The use ofNumber.isFinite(maxBytes)is more robust than!isNaN(), and resettingeraseCancelled = falseensures proper polling behavior even after a previous cancellation.
502-502: LGTM!The function signature update with a default parameter
alsoErase = falsemaintains backward compatibility while enabling the new erase-after-download feature.
522-522: LGTM!Replacing
isNaN()withNumber.isNaN()is a best practice improvement that provides stricter type checking without unexpected type coercion.
539-542: LGTM!The conditional erase logic correctly ensures that flash is only erased when: (1) the download completed successfully (
nextAddress >= maxBytes), (2) the user didn't cancel (!saveCancelled), and (3) the erase variant was triggered (alsoErase).
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/tabs/onboard_logging.js (1)
454-467: Fix compression logging guard to handle null and zero safely.
isNaN(null)is false, so the current check logs with null and divides by zero. Use a finite-and-nonzero check.- if (!isNaN(totalBytesCompressed)) { + if (Number.isFinite(totalBytesCompressed) && totalBytesCompressed > 0) { console.log( "Compressed into", totalBytesCompressed, "bytes with mean compression factor of", - totalBytes / totalBytesCompressed, + (totalBytesCompressed > 0 ? totalBytes / totalBytesCompressed : "n/a"), ); }
🧹 Nitpick comments (2)
src/js/tabs/onboard_logging.js (2)
522-529: Use finite checks and guard progress when maxBytes is 0.Covers Infinity/NaN and avoids invalid progress when invoked despite disabled UI.
- if (Number.isNaN(bytesCompressed) || Number.isNaN(totalBytesCompressed)) { + if (!Number.isFinite(bytesCompressed) || !Number.isFinite(totalBytesCompressed)) { totalBytesCompressed = null; } else { totalBytesCompressed += bytesCompressed; } - $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); + const progressPct = maxBytes > 0 ? (nextAddress / maxBytes) * 100 : 0; + $(".dataflash-saving progress").attr("value", progressPct);
560-564: Respect cancellation on retry when a bad block is received.Avoid continuing reads after user cancels.
- // There was an error with the received block (address didn't match the one we asked for), retry - mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + // Bad block: retry only if not cancelled + if (saveCancelled) { + dismiss_saving_dialog(); + FileSystem.closeFile(openedFile); + return; + } + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/onboard_logging.js (5)
src/js/fc.js (1)
FC(132-1006)src/js/msp.js (1)
MSP(5-455)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/gui_log.js (1)
gui_log(7-21)src/js/localization.js (1)
i18n(7-7)
⏰ 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: deploy
🔇 Additional comments (4)
src/js/tabs/onboard_logging.js (4)
367-369: LGTM: disabled-state now covers Save & Erase.Selector includes erase-flash, save-flash, and save-flash-erase consistently.
502-502: LGTM: explicit default prevents accidental truthy-arg erase.
alsoErase = falseavoids the classic event-object-as-truthy pitfall.
540-543: LGTM: conditional erase only after verified full download.The erase trigger is gated behind size verification and cancellation checks.
Also applies to: 556-558
105-113: Review comment is incorrect; CSS already prevents clicks on disabled anchors.The stylesheet
src/css/main.less(line 47–48) definesa.disabled { pointer-events: none; }globally, which prevents pointer events on all anchor elements with the.disabledclass. The.save-flashand.save-flash-eraseanchors inherit this rule, so clicks are already blocked at the browser level before the event handler runs. The suggested JavaScript guard is redundant.Likely an incorrect or invalid review comment.
|
|
Preview URL: https://pr4673.betaflight-app-preview.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/tabs/onboard_logging.js (2)
583-599: Fix malformed closing span tagUpdate the closing span from
</span class="message-negative>to</span>:- })}</span class="message-negative></strong>`, + })}</span></strong>`,
531-553: Add catch handler to FileSystem.writeChunck promise
FileSystem.writeChunck(...) currently lacks a .catch, leading to unhandled rejections and potential dangling dialogs/files on I/O failure—add a .catch to setself.writeError, log the error, dismiss the dialog, and close the file.- FileSystem.writeChunck(openedFile, blob).then(() => { + FileSystem.writeChunck(openedFile, blob) + .then(() => { // existing success handling... }) + .catch(err => { + self.writeError = true; + console.error("Error writing blackbox chunk:", err); + gui_log(i18n.getMessage("dataflashFileWriteFailed")); + dismiss_saving_dialog(); + FileSystem.closeFile(openedFile); + });
🧹 Nitpick comments (3)
src/js/tabs/onboard_logging.js (3)
522-526: Harden bytesCompressed handling to avoid NaN propagationFirmware may omit or vary bytesCompressed (undefined/Infinity). Make the guard type- and range-safe.
- if (Number.isNaN(bytesCompressed) || Number.isNaN(totalBytesCompressed)) { - totalBytesCompressed = null; - } else { - totalBytesCompressed += bytesCompressed; - } + // Robustly handle missing/invalid compression metrics + if ( + totalBytesCompressed === null || + typeof bytesCompressed !== "number" || + !Number.isFinite(bytesCompressed) + ) { + totalBytesCompressed = null; + } else { + totalBytesCompressed += bytesCompressed; + }
453-467: Avoid logging Infinity/incorrect stats when compression is unknownWhen totalBytesCompressed becomes null, isNaN(null) is false → logs “Infinity”. Gate on finite, >0.
- if (!isNaN(totalBytesCompressed)) { + if (typeof totalBytesCompressed === "number" && Number.isFinite(totalBytesCompressed) && totalBytesCompressed > 0) { console.log( "Compressed into", totalBytesCompressed, "bytes with mean compression factor of", - totalBytes / totalBytesCompressed, + (totalBytes / totalBytesCompressed).toFixed(2), ); }
517-527: Early-exit on cancel to stop I/O ASAPSkip blob writes when user has already cancelled; reduces I/O and latency to close dialog.
- function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { + function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { + if (saveCancelled) { + dismiss_saving_dialog(); + if (openedFile) FileSystem.closeFile(openedFile); + return; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/onboard_logging.js (4)
src/js/fc.js (1)
FC(132-1006)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)src/js/gui_log.js (1)
gui_log(7-21)src/js/localization.js (1)
i18n(7-7)
🔇 Additional comments (4)
src/js/tabs/onboard_logging.js (4)
105-113: Save/Save+Erase handlers: explicit boolean + preventDefault — LGTMCorrectly avoids event→truthy coercion and unintended erase; good separation of actions.
367-369: Disabled-state now includes Save+Erase — LGTMSelector covers erase-flash, save-flash-erase, and MSC save-flash when no data present.
489-500: Conditional erase only after full verified download — LGTMFinite/max-bytes check before erase is sound; integrates cleanly with notifications.
609-617: Guarded dialog.close() after auto-erase — LGTMPrevents InvalidStateError when erase dialog was never opened in Save+Erase flow.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Localization