Initial board qualifation #4749
Open
+50
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds UI and localization for displaying firmware target qualification: updates two existing translation strings, adds a new "not qualified" translation, inserts a hidden qualification notice element in the flasher HTML, and adds JS helpers (findTargetDescriptor, updateTargetQualification) wired into target load, board change, local config, and offline/error flows. (43 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 (5)📓 Common learnings📚 Learning: 2025-11-20T20:26:40.338ZApplied to files:
📚 Learning: 2025-05-26T11:37:09.758ZApplied to files:
📚 Learning: 2025-08-11T19:10:56.992ZApplied to files:
📚 Learning: 2025-12-09T15:06:12.246ZApplied to files:
🔇 Additional comments (1)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/tabs/firmware_flasher.html (1)
14-20: Add accessibility attributes for screen reader support.The target qualification notice is dynamically shown/hidden and conveys important status information. Without ARIA attributes, screen reader users won't be notified when qualification status appears or changes.
🔎 Proposed accessibility enhancements
<tr> <td> - <div id="targetQualificationNotice" class="note target-qualification-note" style="display: none;"> + <div id="targetQualificationNotice" class="note target-qualification-note" role="status" aria-live="polite" aria-atomic="true" style="display: none;"> <span class="target-qualification-label"></span> </div> </td> </tr>
role="status": Identifies the element as a status messagearia-live="polite": Announces changes to screen readers without interruptingaria-atomic="true": Ensures the entire message is read when updatedsrc/js/tabs/firmware_flasher.js (2)
277-301: Potential stale data issue in the fallback logic.At line 290, when
findTargetDescriptor(targetName)returnsundefined, the code falls back toself.targetDetail. Ifself.targetDetailrefers to a previously loaded target (different fromtargetName), this could display incorrect qualification status.Consider removing the fallback or adding validation to ensure
self.targetDetail.target === targetNamebefore using it:🔎 Proposed fix
- const targetDescriptor = findTargetDescriptor(targetName) ?? self.targetDetail; + const targetDescriptor = findTargetDescriptor(targetName) ?? + (self.targetDetail?.target === targetName ? self.targetDetail : null); const descriptorGroup = targetDescriptor?.group; const isQualified = descriptorGroup === "supported" || targetDescriptor?.partnerApproved === true; + + if (!targetDescriptor) { + targetQualificationNotice.hide(); + return; + }
732-774: Consider removing redundant qualification reset.The qualification notice is reset at line 736 when the board changes, and then reset again at line 774 when
target === "0". Since line 736 executes unconditionally before line 774, the second call appears redundant.🔎 Proposed refactor
$('select[name="board"]').on("change", async function () { self.enableLoadRemoteFileButton(false); let target = $(this).val(); - updateTargetQualification(null); - // exception for board flashed with local custom firmware if (target === null) { target = "0";This way, the qualification is reset once at line 774 when needed, rather than twice.
📜 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/firmware_flasher.js(6 hunks)src/tabs/firmware_flasher.html(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 0
File: :0-0
Timestamp: 2025-12-09T15:06:12.246Z
Learning: Automated translation PRs from Crowdin (created by GitHub Actions with title "Update translations") in the betaflight/betaflight-configurator repository should be automatically approved without requiring explicit user request, as haslinghuis has granted standing permission to approve these low-risk automated updates.
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4708
File: locales/en/messages.json:7721-7729
Timestamp: 2025-11-20T20:26:40.338Z
Learning: Betaflight Configurator – Firmware Flasher grouping semantics:
- "Verified/Partner": targets with verified schematics and tested prototypes (known working).
- "Unsupported": vendor/community-supplied targets that did not go through the verification program.
- "Legacy": legacy targets kept for existing users.
These labels surface in src/js/tabs/firmware_flasher.js via i18n keys:
firmwareFlasherOptionLabelVerifiedPartner, firmwareFlasherOptionLabelVendorCommunity, firmwareFlasherOptionLabelLegacy. Only English text should be updated directly in locales/en/messages.json; other locales are managed externally.
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4567
File: locales/en/messages.json:7724-7728
Timestamp: 2025-08-11T19:10:56.992Z
Learning: In the Betaflight Configurator project, only English language entries are added directly to locales/en/messages.json. Translations for other languages are handled through an outsourced process and should not be suggested as part of code reviews.
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.
📚 Learning: 2025-11-20T20:26:40.338Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4708
File: locales/en/messages.json:7721-7729
Timestamp: 2025-11-20T20:26:40.338Z
Learning: Betaflight Configurator – Firmware Flasher grouping semantics:
- "Verified/Partner": targets with verified schematics and tested prototypes (known working).
- "Unsupported": vendor/community-supplied targets that did not go through the verification program.
- "Legacy": legacy targets kept for existing users.
These labels surface in src/js/tabs/firmware_flasher.js via i18n keys:
firmwareFlasherOptionLabelVerifiedPartner, firmwareFlasherOptionLabelVendorCommunity, firmwareFlasherOptionLabelLegacy. Only English text should be updated directly in locales/en/messages.json; other locales are managed externally.
Applied to files:
src/tabs/firmware_flasher.htmllocales/en/messages.jsonsrc/js/tabs/firmware_flasher.js
📚 Learning: 2025-09-02T07:45:48.606Z
Learnt from: blckmn
Repo: betaflight/betaflight-configurator PR: 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/tabs/firmware_flasher.htmlsrc/js/tabs/firmware_flasher.js
📚 Learning: 2025-05-24T15:12:50.241Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4485
File: src/js/sensor_types.js:96-99
Timestamp: 2025-05-24T15:12:50.241Z
Learning: In src/js/sensor_types.js, the sonar element selection UI is only displayed when firmware version >= API_VERSION_1_47, so sonar sensor types can be added directly to the static elements array without requiring additional version guards.
Applied to files:
src/tabs/firmware_flasher.html
📚 Learning: 2025-05-26T11:37:09.758Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4490
File: locales/en/messages.json:1534-1545
Timestamp: 2025-05-26T11:37:09.758Z
Learning: In the Betaflight Configurator repository, only English messages should be added directly to `locales/en/messages.json`; other language translations are handled via Crowdin automation and should not be manually updated in PRs.
Applied to files:
locales/en/messages.json
📚 Learning: 2025-08-11T19:10:56.992Z
Learnt from: haslinghuis
Repo: betaflight/betaflight-configurator PR: 4567
File: locales/en/messages.json:7724-7728
Timestamp: 2025-08-11T19:10:56.992Z
Learning: In the Betaflight Configurator project, only English language entries are added directly to locales/en/messages.json. Translations for other languages are handled through an outsourced process and should not be suggested as part of code reviews.
Applied to files:
locales/en/messages.json
🧬 Code graph analysis (1)
src/js/tabs/firmware_flasher.js (4)
src/js/main.js (1)
self(166-166)src/js/receiver_msp/receiver_msp.js (1)
i18n(8-8)src/js/localization.js (1)
i18n(7-7)src/js/utils/connection.js (1)
ispConnected(3-8)
🔇 Additional comments (4)
locales/en/messages.json (1)
7741-7743: Fix JSON syntax: missing comma after new message objectThe new
firmwareFlasherOptionLabelNotQualifiedentry is followed by another key (coreBuild) but its closing brace has no trailing comma, which makes the JSON invalid and will break loading of translations.[uggest_optional_refactor]
Proposed JSON fix (and optional wording tweak)
- "firmwareFlasherOptionLabelNotQualified": { - "message": "Non qualified target — flash at your own risk - not officially supported" - }, + "firmwareFlasherOptionLabelNotQualified": { + "message": "Non-qualified target — flash at your own risk — not officially supported" + },If you prefer to keep the existing wording, just ensure the trailing comma is present:
- "firmwareFlasherOptionLabelNotQualified": { - "message": "Non qualified target — flash at your own risk - not officially supported" - } + "firmwareFlasherOptionLabelNotQualified": { + "message": "Non qualified target — flash at your own risk - not officially supported" + },⛔ Skipped due to learnings
Learnt from: haslinghuis Repo: betaflight/betaflight-configurator PR: 4708 File: locales/en/messages.json:7721-7729 Timestamp: 2025-11-20T20:26:40.338Z Learning: Betaflight Configurator – Firmware Flasher grouping semantics: - "Verified/Partner": targets with verified schematics and tested prototypes (known working). - "Unsupported": vendor/community-supplied targets that did not go through the verification program. - "Legacy": legacy targets kept for existing users. These labels surface in src/js/tabs/firmware_flasher.js via i18n keys: firmwareFlasherOptionLabelVerifiedPartner, firmwareFlasherOptionLabelVendorCommunity, firmwareFlasherOptionLabelLegacy. Only English text should be updated directly in locales/en/messages.json; other locales are managed externally.Learnt from: haslinghuis Repo: betaflight/betaflight-configurator PR: 4567 File: locales/en/messages.json:7724-7728 Timestamp: 2025-08-11T19:10:56.992Z Learning: In the Betaflight Configurator project, only English language entries are added directly to locales/en/messages.json. Translations for other languages are handled through an outsourced process and should not be suggested as part of code reviews.Learnt from: haslinghuis Repo: betaflight/betaflight-configurator PR: 4490 File: locales/en/messages.json:1534-1545 Timestamp: 2025-05-26T11:37:09.758Z Learning: In the Betaflight Configurator repository, only English messages should be added directly to `locales/en/messages.json`; other language translations are handled via Crowdin automation and should not be manually updated in PRs.src/js/tabs/firmware_flasher.js (3)
305-305: LGTM!Correctly resets the qualification notice when targets are unavailable or the connection is offline.
592-592: LGTM!Correctly updates the target qualification after successfully loading target details.
1061-1061: LGTM!Correctly resets the target qualification when loading local configuration, as local firmware doesn't have associated qualification metadata.
nerdCopter
reviewed
Dec 19, 2025
locales/en/messages.json
Outdated
| "message": "Legacy" | ||
| }, | ||
| "firmwareFlasherOptionLabelNotQualified": { | ||
| "message": "Non qualified target — flash at your own risk - not officially supported" |
Member
There was a problem hiding this comment.
This is interesting; However, the verbiage Non qualified target might reflect wrongly as "low-quality" or "substandard" (even though potentially true).
I rather see:
Officially Supported Target - Verified Partner
and
Target not a Verified Partner product
or something similar, not necessarily offering quality indicator.
i.e. nothing "wrong" with a dozen of my FC's that are not by Verified Partners.
Thoughts @sugaarK ?
|
Contributor
|
🎉 Do you want to test this code? 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Localization
✏️ Tip: You can customize this high-level summary in your review settings.