-
-
Notifications
You must be signed in to change notification settings - Fork 987
Fix custom defines sometimes not applying in firmware flasher #4575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…incorrect expert mode checkbox check
WalkthroughReplaced a runtime jQuery selector for the expert mode checkbox with a cached reference (expertMode_e) in firmware_flasher.js. The conditional logic and subsequent request construction remain unchanged. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Preview URL: https://95eabda7.betaflight-configurator.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
992-1003
: Harden commit assignment and sanitize customDefines in firmware_flasher.jsWe should guard against empty commits and avoid pushing empty custom-define tokens. In src/js/tabs/firmware_flasher.js (around lines 992–1003), replace:
- if (expertMode_e.is(":checked")) { - if (targetDetail.releaseType === "Unstable") { - request.commit = $('select[name="commits"] option:selected').val(); - } - - $('input[name="customDefines"]') - .val() - .split(" ") - .map((element) => element.trim()) - .forEach((v) => { - request.options.push(v); - }); - } + if (expertMode_e.is(":checked")) { + if (targetDetail.releaseType === "Unstable") { + const commit = $('select[name="commits"] option:selected').val(); + if (commit) { + request.commit = commit; + } + } + + const definesRaw = $('input[name="customDefines"]').val() || ""; + definesRaw + .trim() + .split(/\s+/) + .filter(Boolean) + .forEach((v) => { + request.options.push(v); + }); + }Verification confirmed:
- The
.tab-firmware_flasher input.expert_mode
checkbox exists (no stray legacy selectors).coreBuildModeCheckbox
is present in firmware_flasher.html.- Other expert-mode references live outside this context and are unaffected.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js
(1 hunks)
🔇 Additional comments (1)
src/js/tabs/firmware_flasher.js (1)
991-991
: Correct Expert Mode source used; fixes custom defines not applyingSwitching to the Firmware Flasher’s
expertMode_e
checkbox resolves the earlier mismatch with the Connected-tab checkbox. This aligns with how Expert Mode is handled elsewhere in the flasher. Good catch.
Custom defines were occasionally not applied because they were conditional based on the wrong Expert Mode checkbox (the one visible when Connected, not the one in Firmware Flasher).
Applied minimal fix to bring it in line with other places in firmware flasher that test for this checkbox.
Summary by CodeRabbit