Milbeta 460 norn config UI branch #4660
Conversation
WalkthroughAdds a new "NORN Config" tab (UI, localization, template and README), a dynamic tab module, new MSP/port enum entries and port rules, new OSD fields for Manticore, firmware flasher auto-detect/port event handling, a change that suppresses the problem-reporting dialog, and runtime deps Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Browser UI
participant Main as main.js
participant Norn as tabs/norn_config.js
participant I18N as i18n
participant Tmpl as Handlebars Template
User->>UI: Click "NORN Config"
UI->>Main: request load("norn_config")
Main->>Norn: dynamic import + initialize(content_ready)
Norn->>UI: inject `norn_config.html`
Norn->>I18N: localize UI
Norn->>UI: populate selects & bind events
Note over Norn,UI: periodic MSP_STATUS while active
User->>Norn: Click Generate / Copy / Save
Norn->>Tmpl: render(template, selectedKeys)
Tmpl-->>Norn: generated output
Norn->>UI: display / copy / trigger download
sequenceDiagram
autonumber
participant EB as EventBus
participant Flasher as firmware_flasher.js
participant AD as AutoDetect
Note over Flasher: on activation or port/device events
EB-->>Flasher: device detected / ports-input:change
alt reboot/flash-on-connect enabled
Flasher->>Flasher: start flashing sequence
else
Flasher->>AD: verifyBoard()
AD-->>Flasher: board info / no-match
end
EB-->>Flasher: device-removed
Flasher->>Flasher: clear board selection & firmware
Note over Flasher: cleanup removes listeners
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 13
🧹 Nitpick comments (1)
src/tabs/norn_config.html (1)
80-80: Use i18n for the placeholder text.The placeholder text "Enter craft name" is hardcoded in English, which breaks localization. Use an i18n attribute or data attribute for consistency with the rest of the UI.
Apply this diff to fix the localization:
-<input type="text" id="norn_craft_name" placeholder="Enter craft name" style="width: 100%;" /> +<input type="text" id="norn_craft_name" data-i18n-placeholder="nornCraftNamePlaceholder" style="width: 100%;" />Then add the corresponding key to the locale files:
"nornCraftNamePlaceholder": { "message": "Enter craft name" }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
locales/en/messages.json(3 hunks)locales/uk/messages.json(1 hunks)package.json(1 hunks)src/index.html(1 hunks)src/js/gui.js(1 hunks)src/js/main.js(1 hunks)src/js/msp/MSPHelper.js(1 hunks)src/js/serial_backend.js(1 hunks)src/js/tabs/firmware_flasher.js(3 hunks)src/js/tabs/norn_config.js(1 hunks)src/js/tabs/osd.js(4 hunks)src/js/tabs/ports.js(1 hunks)src/norn-configs/template.ejs(1 hunks)src/tabs/norn_config.html(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T11:37:09.758Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#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
🧬 Code graph analysis (3)
src/js/tabs/firmware_flasher.js (3)
src/js/protocols/webstm32.js (1)
STM32(1007-1007)src/js/gui.js (1)
GUI(466-466)src/js/port_handler.js (2)
devicePath(110-110)PortHandler(16-50)
src/js/main.js (1)
src/js/tabs/norn_config.js (1)
norn_config(12-14)
src/js/tabs/norn_config.js (3)
src/js/gui.js (2)
GUI(466-466)TABS(8-8)src/js/msp.js (1)
MSP(5-467)src/js/msp/MSPCodes.js (1)
MSPCodes(2-210)
⏰ 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 (11)
src/js/tabs/firmware_flasher.js (2)
743-748: LGTM!The auto-detection fallback when neither reboot mode nor flash-on-connect is active is a sensible enhancement that improves user experience.
750-765: LGTM!The new
detectedSerialDevicehandler correctly mirrors the USB device detection logic, ensuring consistent behavior across connection types.src/js/gui.js (1)
33-33: LGTM!The addition of "norn_config" to the default allowed tabs is correct and consistent with the new NORN Config tab introduced in this PR.
src/js/msp/MSPHelper.js (1)
70-74: LGTM!The new serial port function definitions are properly structured with sequential bit positions (18-22) that don't conflict with existing entries. The naming convention is consistent, and these entries align with the corresponding function rules added in
src/js/tabs/ports.js.src/js/tabs/ports.js (1)
76-80: LGTM!The new peripheral function rules are well-structured and consistent with existing patterns:
- All are properly categorized as "peripherals" with
maxPorts: 1- MANTICORE_INITIATOR_V1 and V2 appropriately declare
sharableWith: ["msp"]for MSP compatibility- These rules correspond to the SERIAL_PORT_FUNCTIONS entries added in
src/js/msp/MSPHelper.jsThe localization keys (e.g.,
portsFunction_MANTICORE_INITIATOR_V1) are expected to be defined in the locale files.src/js/tabs/osd.js (2)
1821-1847: LGTM: MANTICORE fields follow proper conventions.The three MANTICORE fields correctly use i18n message keys for localization and have appropriate draw orders (1075/1076). The shared draw order of 1075 for V1 and V2 is acceptable if they are mutually exclusive alternatives.
2295-2302: LGTM: Fields correctly gated by API version.The new fields are properly added to the
DISPLAY_FIELDSarray only whenAPI_VERSION >= 1.47, following the established pattern for version-specific feature additions.src/index.html (1)
139-139: LGTM!The new NORN Config tab entry follows the established pattern for tab list items, with proper i18n attributes and icon class. The placement in the connected mode tab list is appropriate.
src/js/main.js (1)
282-284: LGTM!The new tab route case follows the established dynamic import pattern used by other tabs. The implementation correctly imports the norn_config module and calls its initialize method with the content_ready callback.
src/js/tabs/norn_config.js (2)
1-7: LGTM!Imports are well-organized and all dependencies are necessary for the tab functionality.
9-10: LGTM!Using
import.meta.globwith eager loading is appropriate for this use case, and loading templates as raw strings is correct for EJS rendering.
| "message": "Manticore V1" | ||
| }, | ||
| "osdTextElementManticoreV2": { | ||
| "message": "Manticore V2" | ||
| }, | ||
| "osdTextElementManticoreV2Logger": { | ||
| "message": "Manticore V2 Logger" | ||
| }, |
There was a problem hiding this comment.
Add the missing OSD description strings.
Every OSD element string in this file has a paired osdDescElement… entry. Without the new descriptions, the UI falls back to the raw key (or shows nothing) for the tooltips of these Manticore elements. Please add the descriptions alongside the text entries so the help popovers remain readable.
"osdTextElementManticoreV1": {
"message": "Manticore V1"
},
+ "osdDescElementManticoreV1": {
+ "message": "Displays the status/state for the Manticore V1 data feed."
+ },
"osdTextElementManticoreV2": {
"message": "Manticore V2"
},
+ "osdDescElementManticoreV2": {
+ "message": "Displays the status/state for the Manticore V2 data feed."
+ },
"osdTextElementManticoreV2Logger": {
"message": "Manticore V2 Logger"
},
+ "osdDescElementManticoreV2Logger": {
+ "message": "Displays the logger status for the Manticore V2 data feed."
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "message": "Manticore V1" | |
| }, | |
| "osdTextElementManticoreV2": { | |
| "message": "Manticore V2" | |
| }, | |
| "osdTextElementManticoreV2Logger": { | |
| "message": "Manticore V2 Logger" | |
| }, | |
| "osdTextElementManticoreV1": { | |
| "message": "Manticore V1" | |
| }, | |
| "osdDescElementManticoreV1": { | |
| "message": "Displays the status/state for the Manticore V1 data feed." | |
| }, | |
| "osdTextElementManticoreV2": { | |
| "message": "Manticore V2" | |
| }, | |
| "osdDescElementManticoreV2": { | |
| "message": "Displays the status/state for the Manticore V2 data feed." | |
| }, | |
| "osdTextElementManticoreV2Logger": { | |
| "message": "Manticore V2 Logger" | |
| }, | |
| "osdDescElementManticoreV2Logger": { | |
| "message": "Displays the logger status for the Manticore V2 data feed." | |
| }, |
🤖 Prompt for AI Agents
In locales/en/messages.json around lines 7793-7800, three OSD text keys
(osdTextElementManticoreV1, osdTextElementManticoreV2,
osdTextElementManticoreV2Logger) are missing their corresponding description
keys; add matching osdDescElementManticoreV1, osdDescElementManticoreV2, and
osdDescElementManticoreV2Logger entries directly alongside the existing text
entries with concise tooltip strings (e.g., brief descriptions of each Manticore
element) so the UI help popovers display human-readable descriptions instead of
raw keys.
| "message": "Копіювати в буффер" | ||
| }, | ||
| "nornSave": { | ||
| "message": "Зберегти" | ||
| }, |
There was a problem hiding this comment.
Fix typo in user-facing text.
"Копіювати в буффер" має одну зайву літера ф. Будь ласка, виправте на "Копіювати в буфер", щоб уникнути помилки в інтерфейсі.
🤖 Prompt for AI Agents
In locales/uk/messages.json around lines 7545 to 7549, the Ukrainian translation
for the copy-to-clipboard label contains a typo ("Копіювати в буффер" with an
extra "ф"); replace that value with the correct spelling "Копіювати в буфер"
while preserving the JSON key structure and surrounding formatting.
| "node": "20.x" | ||
| }, | ||
| "dependencies": { | ||
| "ejs": "^3.1.9", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify EJS version for security advisories and latest updates.
The EJS dependency version 3.1.9 was published in March 2023. Given the security-sensitive nature of template engines (server-side template injection risks are documented for EJS), ensure this version has no known vulnerabilities and consider whether a newer version is available.
Based on learnings
Run the following script to check for security advisories and the latest version:
🏁 Script executed:
#!/bin/bash
# Check for the latest version of EJS
curl -s https://registry.npmjs.org/ejs | jq -r '.["dist-tags"].latest'
# Check for security advisories using GitHub API
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NPM, package: "ejs") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 1108
Upgrade EJS to ≥3.1.10. Version 3.1.9 is vulnerable to a moderate‐severity “pollution protection” issue (published 2024-04-28) fixed in 3.1.10.
🤖 Prompt for AI Agents
In package.json around line 44, the ejs dependency is pinned to "3.1.9" which is
vulnerable; update the dependency to ">=3.1.10" (e.g., set the version to
"3.1.10" or a caret range "^3.1.10"), then run your package manager to update
node_modules and the lockfile (npm install / npm ci or yarn install), run the
test suite and any build steps to verify nothing breaks, and commit the updated
package.json and lockfile.
| } | ||
| } | ||
|
|
||
| needsProblemReportingDialog = false; |
There was a problem hiding this comment.
Remove the unconditional suppression of the problem-reporting dialog.
This line unconditionally sets needsProblemReportingDialog = false immediately before the conditional check, effectively disabling all problem reporting to users. The code above (lines 482-509) carefully detects firmware compatibility issues, calibration problems, and other configuration issues, but this line prevents users from ever seeing these warnings.
This appears to be debug or development code that should not be in production. Users need to see these warnings to diagnose and fix problems with their flight controllers.
Remove this line to restore proper problem reporting:
- needsProblemReportingDialog = false;
if (needsProblemReportingDialog) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| needsProblemReportingDialog = false; | |
| if (needsProblemReportingDialog) { |
🤖 Prompt for AI Agents
In src/js/serial_backend.js around line 511, there is an unconditional
assignment "needsProblemReportingDialog = false" that suppresses the
problem-reporting dialog; remove this line so that the boolean set earlier by
the detection logic is preserved and the subsequent conditional that shows the
dialog can run normally, restoring user-facing warnings about
firmware/calibration/configuration issues.
| // Also listen for port changes to catch reconnections | ||
| function onPortChange(port) { | ||
| console.log(`${self.logHead} Port changed to:`, port); | ||
| // Auto-detect board when port changes and we're on firmware flasher tab | ||
| if (port && port !== "0" && !$("input.flash_on_connect").is(":checked") && !STM32.rebootMode) { | ||
| console.log(`${self.logHead} Auto-detecting board for port change`); | ||
| setTimeout(() => { | ||
| AutoDetect.verifyBoard(); | ||
| }, 500); // Small delay to ensure port is ready | ||
| } else if (!port || port === "0") { | ||
| // Clear board selection when no port is selected | ||
| console.log(`${self.logHead} Clearing board selection - no port selected`); | ||
| $('select[name="board"]').val("0").trigger("change"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Store timeout references for cleanup.
The setTimeout callback at line 777 is not stored, so it cannot be canceled if the tab closes or changes before the 500ms delay completes. This can lead to AutoDetect.verifyBoard() executing in an invalid context.
Store the timeout reference and clear it in the cleanup function:
+let portChangeTimeout = null;
+
function onPortChange(port) {
console.log(`${self.logHead} Port changed to:`, port);
+ // Clear any pending timeout
+ if (portChangeTimeout) {
+ clearTimeout(portChangeTimeout);
+ portChangeTimeout = null;
+ }
// Auto-detect board when port changes and we're on firmware flasher tab
if (port && port !== "0" && !$("input.flash_on_connect").is(":checked") && !STM32.rebootMode) {
console.log(`${self.logHead} Auto-detecting board for port change`);
- setTimeout(() => {
+ portChangeTimeout = setTimeout(() => {
AutoDetect.verifyBoard();
+ portChangeTimeout = null;
}, 500); // Small delay to ensure port is readyThen in firmware_flasher.cleanup, add:
firmware_flasher.cleanup = function (callback) {
+ // Clear any pending timeouts
+ if (portChangeTimeout) {
+ clearTimeout(portChangeTimeout);
+ portChangeTimeout = null;
+ }
// unbind "global" events📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Also listen for port changes to catch reconnections | |
| function onPortChange(port) { | |
| console.log(`${self.logHead} Port changed to:`, port); | |
| // Auto-detect board when port changes and we're on firmware flasher tab | |
| if (port && port !== "0" && !$("input.flash_on_connect").is(":checked") && !STM32.rebootMode) { | |
| console.log(`${self.logHead} Auto-detecting board for port change`); | |
| setTimeout(() => { | |
| AutoDetect.verifyBoard(); | |
| }, 500); // Small delay to ensure port is ready | |
| } else if (!port || port === "0") { | |
| // Clear board selection when no port is selected | |
| console.log(`${self.logHead} Clearing board selection - no port selected`); | |
| $('select[name="board"]').val("0").trigger("change"); | |
| } | |
| } | |
| // At top of the module scope | |
| let portChangeTimeout = null; | |
| // Also listen for port changes to catch reconnections | |
| function onPortChange(port) { | |
| console.log(`${self.logHead} Port changed to:`, port); | |
| // Clear any pending timeout | |
| if (portChangeTimeout) { | |
| clearTimeout(portChangeTimeout); | |
| portChangeTimeout = null; | |
| } | |
| // Auto-detect board when port changes and we're on firmware flasher tab | |
| if (port && port !== "0" && !$("input.flash_on_connect").is(":checked") && !STM32.rebootMode) { | |
| console.log(`${self.logHead} Auto-detecting board for port change`); | |
| portChangeTimeout = setTimeout(() => { | |
| AutoDetect.verifyBoard(); | |
| portChangeTimeout = null; | |
| }, 500); // Small delay to ensure port is ready | |
| } else if (!port || port === "0") { | |
| // Clear board selection when no port is selected | |
| console.log(`${self.logHead} Clearing board selection - no port selected`); | |
| $('select[name="board"]').val("0").trigger("change"); | |
| } | |
| } | |
| // ... | |
| firmware_flasher.cleanup = function (callback) { | |
| // Clear any pending timeouts | |
| if (portChangeTimeout) { | |
| clearTimeout(portChangeTimeout); | |
| portChangeTimeout = null; | |
| } | |
| // unbind "global" events | |
| // ... | |
| }; |
🤖 Prompt for AI Agents
In src/js/tabs/firmware_flasher.js around lines 770 to 784, the setTimeout
started for AutoDetect.verifyBoard() is not stored so it cannot be cancelled
when the tab closes; create a scoped variable (e.g., autoDetectTimeout) on the
module/this so the timeout ID is assigned whenever you call setTimeout, clear
any existing timeout before creating a new one
(clearTimeout(autoDetectTimeout)), and replace setTimeout(...) with
autoDetectTimeout = setTimeout(...); then, in firmware_flasher.cleanup, call
clearTimeout(autoDetectTimeout) and null out the variable to ensure the pending
callback is cancelled when the tab is torn down.
| // Example dropdown wiring | ||
| // none for now |
There was a problem hiding this comment.
Remove obsolete commented code and placeholder comments.
- Lines 86-87: Placeholder comment "Example dropdown wiring // none for now" serves no purpose
- Line 122: Comment "No save/copy handlers for now" is outdated—handlers are implemented below
Apply this diff to remove:
- // Example dropdown wiring
- // none for now
-
// Button wiring- // No save/copy handlers for now (kept minimal per request)
-
function readFileRaw(pathMap, path) {Also applies to: 122-122
🤖 Prompt for AI Agents
In src/js/tabs/norn_config.js around lines 86-87 and line 122, remove the
obsolete placeholder comments: delete the two-line "Example dropdown wiring //
none for now" comment at 86-87 and replace or remove the outdated "No save/copy
handlers for now" comment at line 122 since handlers exist below; ensure no
extra blank lines remain and keep surrounding code indentation and spacing
intact.
| } else { | ||
| // Fallback to old behavior if template missing | ||
| const parts = []; | ||
| const genericPath = Object.keys(genericFiles)[0]; | ||
| parts.push(readFileRaw(genericFiles, genericPath)); | ||
| const manticorePath = $("select[name='norn_manticore']").val(); | ||
| if (manticorePath) parts.push(readFileRaw(manticoreFiles, manticorePath)); | ||
| const vtxPath = $("select[name='norn_vtx']").val(); | ||
| if (vtxPath) parts.push(readFileRaw(vtxFiles, vtxPath)); | ||
| result = parts.filter(Boolean).join("\n\n").trim(); | ||
| } |
There was a problem hiding this comment.
Critical: Undefined variables in fallback path.
The fallback code references three undefined variables:
genericFiles(line 155)manticoreFiles(line 158)vtxFiles(line 160)
None of these are defined in the file or imported. If the template is missing, this code will throw a ReferenceError.
Since the template is loaded eagerly at line 10, consider one of these fixes:
Option 1: Remove the fallback entirely (preferred if template.ejs is always present):
function on_generate_handler(e) {
e?.preventDefault?.();
const templatePath = Object.keys(templateFiles)[0];
- let result;
- if (templatePath) {
- const tpl = readFileRaw(templateFiles, templatePath);
- result = ejs.render(tpl, getSelectedKeys());
- } else {
- // Fallback to old behavior if template missing
- const parts = [];
- const genericPath = Object.keys(genericFiles)[0];
- parts.push(readFileRaw(genericFiles, genericPath));
- const manticorePath = $("select[name='norn_manticore']").val();
- if (manticorePath) parts.push(readFileRaw(manticoreFiles, manticorePath));
- const vtxPath = $("select[name='norn_vtx']").val();
- if (vtxPath) parts.push(readFileRaw(vtxFiles, vtxPath));
- result = parts.filter(Boolean).join("\n\n").trim();
- }
+ if (!templatePath) {
+ console.error("Template not found");
+ $("#norn_config_output").val("Error: Template not found");
+ return;
+ }
+ const tpl = readFileRaw(templateFiles, templatePath);
+ const result = ejs.render(tpl, getSelectedKeys());
$("#norn_config_output").val(result);
}Option 2: Define the missing file maps (if legacy paths are needed):
+// Discover legacy config sources
+const genericFiles = import.meta.glob("../../norn-configs/generic/*.txt", { eager: true, as: "raw" });
+const manticoreFiles = import.meta.glob("../../norn-configs/manticore/*.txt", { eager: true, as: "raw" });
+const vtxFiles = import.meta.glob("../../norn-configs/vtx/*.txt", { eager: true, as: "raw" });
+
// Discover config sources (text-based)
const templateFiles = import.meta.glob("../../norn-configs/template.ejs", { eager: true, as: "raw" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| // Fallback to old behavior if template missing | |
| const parts = []; | |
| const genericPath = Object.keys(genericFiles)[0]; | |
| parts.push(readFileRaw(genericFiles, genericPath)); | |
| const manticorePath = $("select[name='norn_manticore']").val(); | |
| if (manticorePath) parts.push(readFileRaw(manticoreFiles, manticorePath)); | |
| const vtxPath = $("select[name='norn_vtx']").val(); | |
| if (vtxPath) parts.push(readFileRaw(vtxFiles, vtxPath)); | |
| result = parts.filter(Boolean).join("\n\n").trim(); | |
| } | |
| function on_generate_handler(e) { | |
| e?.preventDefault?.(); | |
| const templatePath = Object.keys(templateFiles)[0]; | |
| if (!templatePath) { | |
| console.error("Template not found"); | |
| $("#norn_config_output").val("Error: Template not found"); | |
| return; | |
| } | |
| const tpl = readFileRaw(templateFiles, templatePath); | |
| const result = ejs.render(tpl, getSelectedKeys()); | |
| $("#norn_config_output").val(result); | |
| } |
| } else { | |
| // Fallback to old behavior if template missing | |
| const parts = []; | |
| const genericPath = Object.keys(genericFiles)[0]; | |
| parts.push(readFileRaw(genericFiles, genericPath)); | |
| const manticorePath = $("select[name='norn_manticore']").val(); | |
| if (manticorePath) parts.push(readFileRaw(manticoreFiles, manticorePath)); | |
| const vtxPath = $("select[name='norn_vtx']").val(); | |
| if (vtxPath) parts.push(readFileRaw(vtxFiles, vtxPath)); | |
| result = parts.filter(Boolean).join("\n\n").trim(); | |
| } | |
| // Discover legacy config sources | |
| const genericFiles = import.meta.glob("../../norn-configs/generic/*.txt", { eager: true, as: "raw" }); | |
| const manticoreFiles = import.meta.glob("../../norn-configs/manticore/*.txt", { eager: true, as: "raw" }); | |
| const vtxFiles = import.meta.glob("../../norn-configs/vtx/*.txt", { eager: true, as: "raw" }); | |
| // Discover config sources (text-based) | |
| const templateFiles = import.meta.glob("../../norn-configs/template.ejs", { eager: true, as: "raw" }); |
🤖 Prompt for AI Agents
In src/js/tabs/norn_config.js around lines 152-162, the fallback branch
references undefined variables (genericFiles, manticoreFiles, vtxFiles) causing
a ReferenceError; remove the fallback block entirely if template.ejs is
guaranteed to be present (preferred), otherwise replace it with a safe behavior:
either compute/require the missing file maps from the same source that populates
the main template variables or log a clear error and set result to an empty
string or a known default so no undefined variables are accessed.
| norn_config.cleanup = function (callback) { | ||
| if (callback) callback(); | ||
| }; |
There was a problem hiding this comment.
Add cleanup for MSP status polling interval.
The status_pull interval (added at line 110) is not removed when the tab is closed. This will cause the interval to continue running, leading to unnecessary MSP traffic and potential resource leaks.
Apply this diff to properly clean up the interval:
norn_config.cleanup = function (callback) {
+ GUI.interval_remove("status_pull");
if (callback) callback();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| norn_config.cleanup = function (callback) { | |
| if (callback) callback(); | |
| }; | |
| norn_config.cleanup = function (callback) { | |
| GUI.interval_remove("status_pull"); | |
| if (callback) callback(); | |
| }; |
🤖 Prompt for AI Agents
In src/js/tabs/norn_config.js around lines 220 to 222 (status_pull defined
around line 110), the cleanup function does not clear the MSP status polling
interval; update norn_config.cleanup to check if norn_config.status_pull (or the
variable name used at line 110) is set, call
clearInterval(norn_config.status_pull), set it to null/undefined, then invoke
the callback if provided so the interval stops when the tab is closed.
| MB_OSD_STATS: { | ||
| name: "MB_OSD_STATS", | ||
| text: "MB_OSD_STATS", | ||
| desc: "MB_OSD_STATS", | ||
| defaultPosition: -1, | ||
| draw_order: 610, | ||
| positionable: true, | ||
| preview: "MB_OSD_STATS", | ||
| }, | ||
| MB_VRX_RSSI: { | ||
| name: "MB_VRX_RSSI", | ||
| text: "MB_VRX_RSSI", | ||
| desc: "MB_VRX_RSSI", | ||
| defaultPosition: -1, | ||
| draw_order: 610, | ||
| positionable: true, | ||
| preview: "MB_VRX_RSSI", | ||
| }, | ||
| MB_INIK_INITIATOR: { | ||
| name: "MB_INIK_INITIATOR", | ||
| text: "MB_INIK_INITIATOR", | ||
| desc: "MB_INIK_INITIATOR", | ||
| defaultPosition: -1, | ||
| draw_order: 610, | ||
| positionable: true, | ||
| preview: "MB_INIK_INITIATOR", | ||
| }, | ||
| MB_USE_KOLIBRI_INITIATOR_OSD: { | ||
| name: "MB_USE_KOLIBRI_INITIATOR_OSD", | ||
| text: "MB_USE_KOLIBRI_INITIATOR_OSD", | ||
| desc: "MB_USE_KOLIBRI_INITIATOR_OSD", | ||
| defaultPosition: -1, | ||
| draw_order: 610, | ||
| positionable: true, | ||
| preview: "MB_USE_KOLIBRI_INITIATOR_OSD", | ||
| }, | ||
| MB_LASERDETON_INITIATOR_OSD: { | ||
| name: "MB_LASERDETON_INITIATOR_OSD", | ||
| text: "MB_LASERDETON_INITIATOR_OSD", | ||
| desc: "MB_LASERDETON_INITIATOR_OSD", | ||
| defaultPosition: -1, | ||
| draw_order: 610, | ||
| positionable: true, | ||
| preview: "MB_LASERDETON_INITIATOR_OSD", | ||
| }, |
There was a problem hiding this comment.
Critical: Missing i18n keys and potential draw order collision.
The first five new OSD fields have two issues:
-
Localization broken: The
text,desc, andpreviewproperties use hardcoded strings instead of i18n message keys. All other OSD fields use keys like"osdTextElementMainBattVoltage"that get translated viai18n.getMessage(). These fields will not be localized for non-English users. -
Draw order collision: All five fields (plus
OSD_LIDAR_DISTat line 1767) sharedraw_order: 610. When multiple elements occupy the same position, identical draw orders can cause unpredictable rendering behavior. Consider assigning unique or sequential draw orders (e.g., 611, 612, 613, 614, 615).
Apply this pattern to fix the i18n issue for each field:
MB_OSD_STATS: {
name: "MB_OSD_STATS",
- text: "MB_OSD_STATS",
- desc: "MB_OSD_STATS",
+ text: "osdTextElementMbOsdStats",
+ desc: "osdDescElementMbOsdStats",
defaultPosition: -1,
- draw_order: 610,
+ draw_order: 611,
positionable: true,
- preview: "MB_OSD_STATS",
+ preview: "MB OSD STATS",
},Then add corresponding entries to locale files (e.g., locales/en/messages.json):
"osdTextElementMbOsdStats": { "message": "MB OSD Stats" },
"osdDescElementMbOsdStats": { "message": "Milbeta OSD statistics display" }Repeat for all five MB_* fields with incremented draw_order values.
🤖 Prompt for AI Agents
In src/js/tabs/osd.js around lines 1776-1820, the five MB_* OSD entries use
hardcoded text/desc/preview and share draw_order=610; update each entry to use
i18n message keys for text, desc and preview (e.g., osdTextElementMbOsdStats /
osdDescElementMbOsdStats / osdPreviewElementMbOsdStats) instead of literal
strings, and assign unique/sequential draw_order values (suggest
611,612,613,614,615) to avoid rendering collisions; then add corresponding
locale entries in locales/en/messages.json for each text/desc/preview key with
appropriate "message" strings (one pair at minimum per field: text and desc;
preview can reuse text key) so translations work.
src/norn-configs/template.ejs
Outdated
| defaults nosave | ||
|
|
||
| batch start | ||
|
|
||
| defaults nosave | ||
|
|
||
| <% if (fcKey === 'f4') { %> | ||
| # Flight Controller: F4 tweaks | ||
| set pid_process_denom = 2 | ||
| <% } %> | ||
|
|
||
| <% if (fcKey === 'f7') { %> | ||
| # Flight Controller: F7 tweaks | ||
| set pid_process_denom = 1 | ||
| <% } %> | ||
|
|
||
| <% if (fcKey === 'h7') { %> | ||
| # Flight Controller: H7 tweaks | ||
| set pid_process_denom = 1 | ||
| set cpu_overclock = ON | ||
| <% } %> | ||
|
|
||
| <% if (droneSize) { %> | ||
| # Drone Size: <%= droneSize %>" | ||
| <% if (droneSize === '7') { %> | ||
| set motor_pwm_rate = 32000 | ||
| set motor_poles = 12 | ||
| <% } else if (droneSize === '8') { %> | ||
| set motor_pwm_rate = 24000 | ||
| set motor_poles = 14 | ||
| <% } else if (droneSize === '9') { %> | ||
| set motor_pwm_rate = 24000 | ||
| set motor_poles = 16 | ||
| <% } else if (droneSize === '10') { %> | ||
| set motor_pwm_rate = 16000 | ||
| set motor_poles = 18 | ||
| <% } else if (droneSize === '13') { %> | ||
| set motor_pwm_rate = 12000 | ||
| set motor_poles = 20 | ||
| <% } else if (droneSize === '15') { %> | ||
| set motor_pwm_rate = 8000 | ||
| set motor_poles = 22 | ||
| <% } %> | ||
| <% } %> | ||
| <% if (manticoreKey === 'uart') { %> | ||
| # Manticore UART variant | ||
| set uart1_function = RX_SERIAL | ||
| set uart2_function = TELEMETRY | ||
| <% } %> | ||
| <% if (manticoreKey === 'gpio') { %> | ||
| # Manticore GPIO variant | ||
| set gpio_output = ON | ||
| set gpio_mode = ALTERNATE | ||
| <% } %> | ||
| <% if (vtxKey === '5.8_vtx') { %> | ||
| # 5.8GHz VTX settings | ||
| set vtx_band = A | ||
| set vtx_channel = 1 | ||
| set vtx_power = 25 | ||
| <% } %> | ||
| <% if (vtxKey === '3.3_vtx') { %> | ||
| # 3.3GHz VTX settings | ||
| set vtx_band = B | ||
| set vtx_channel = 3 | ||
| set vtx_power = 100 | ||
| <% } %> | ||
| <% if (vtxKey === 'optica') { %> | ||
| # Optica VTX settings | ||
| set vtx_band = O | ||
| set vtx_channel = 2 | ||
| set vtx_power = 50 | ||
| <% } %> | ||
| <% if (gpsEnabled) { %> | ||
| # GPS Enabled | ||
| feature GPS | ||
| set gps_baudrate = AUTO | ||
| <% } %> | ||
| <% if (craftName) { %> | ||
| # Craft name | ||
| set craft_name = <%= craftName %> | ||
| <% } %> | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add "batch end" command to close the batch started on line 3.
The template begins a batch block with batch start on line 3 but never closes it. Many flight controller firmwares expect balanced batch commands. Add a batch end command after the craft name section to properly close the batch block.
Apply this diff to add the missing batch end:
<% if (craftName) { %>
# Craft name
set craft_name = <%= craftName %>
<% } %>
+batch end
+save
Additionally, consider adding a save command to persist the configuration changes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defaults nosave | |
| batch start | |
| defaults nosave | |
| <% if (fcKey === 'f4') { %> | |
| # Flight Controller: F4 tweaks | |
| set pid_process_denom = 2 | |
| <% } %> | |
| <% if (fcKey === 'f7') { %> | |
| # Flight Controller: F7 tweaks | |
| set pid_process_denom = 1 | |
| <% } %> | |
| <% if (fcKey === 'h7') { %> | |
| # Flight Controller: H7 tweaks | |
| set pid_process_denom = 1 | |
| set cpu_overclock = ON | |
| <% } %> | |
| <% if (droneSize) { %> | |
| # Drone Size: <%= droneSize %>" | |
| <% if (droneSize === '7') { %> | |
| set motor_pwm_rate = 32000 | |
| set motor_poles = 12 | |
| <% } else if (droneSize === '8') { %> | |
| set motor_pwm_rate = 24000 | |
| set motor_poles = 14 | |
| <% } else if (droneSize === '9') { %> | |
| set motor_pwm_rate = 24000 | |
| set motor_poles = 16 | |
| <% } else if (droneSize === '10') { %> | |
| set motor_pwm_rate = 16000 | |
| set motor_poles = 18 | |
| <% } else if (droneSize === '13') { %> | |
| set motor_pwm_rate = 12000 | |
| set motor_poles = 20 | |
| <% } else if (droneSize === '15') { %> | |
| set motor_pwm_rate = 8000 | |
| set motor_poles = 22 | |
| <% } %> | |
| <% } %> | |
| <% if (manticoreKey === 'uart') { %> | |
| # Manticore UART variant | |
| set uart1_function = RX_SERIAL | |
| set uart2_function = TELEMETRY | |
| <% } %> | |
| <% if (manticoreKey === 'gpio') { %> | |
| # Manticore GPIO variant | |
| set gpio_output = ON | |
| set gpio_mode = ALTERNATE | |
| <% } %> | |
| <% if (vtxKey === '5.8_vtx') { %> | |
| # 5.8GHz VTX settings | |
| set vtx_band = A | |
| set vtx_channel = 1 | |
| set vtx_power = 25 | |
| <% } %> | |
| <% if (vtxKey === '3.3_vtx') { %> | |
| # 3.3GHz VTX settings | |
| set vtx_band = B | |
| set vtx_channel = 3 | |
| set vtx_power = 100 | |
| <% } %> | |
| <% if (vtxKey === 'optica') { %> | |
| # Optica VTX settings | |
| set vtx_band = O | |
| set vtx_channel = 2 | |
| set vtx_power = 50 | |
| <% } %> | |
| <% if (gpsEnabled) { %> | |
| # GPS Enabled | |
| feature GPS | |
| set gps_baudrate = AUTO | |
| <% } %> | |
| <% if (craftName) { %> | |
| # Craft name | |
| set craft_name = <%= craftName %> | |
| <% } %> | |
| <% if (craftName) { %> | |
| # Craft name | |
| set craft_name = <%= craftName %> | |
| <% } %> | |
| batch end | |
| save |
🤖 Prompt for AI Agents
In src/norn-configs/template.ejs around lines 1 to 90, the template opens a
batch with "batch start" on line 3 but never closes it; add a matching "batch
end" after the craft name section (immediately after the block that emits set
craft_name), and optionally append a "save" command to persist changes so the
batch is properly closed and the configuration is saved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/js/tabs/norn_config.js (4)
91-92: Remove obsolete placeholder comment.The comment "Example dropdown wiring // none for now" serves no purpose and should be removed.
Apply this diff:
- // Example dropdown wiring - // none for now - // Button wiring
127-127: Remove outdated comment.The comment "No save/copy handlers for now (kept minimal per request)" is outdated—handlers are implemented below (lines 149-221).
Apply this diff:
- // No save/copy handlers for now (kept minimal per request) - function readFileRaw(pathMap, path) {
159-168: Critical: Remove fallback with undefined variables.The fallback code (lines 159-168) references three undefined variables:
genericFiles(line 161)manticoreFiles(line 164)vtxFiles(line 166)Since the template is loaded eagerly at line 15, this fallback is either unreachable or will throw a
ReferenceErrorif executed.Recommended fix: Remove the fallback entirely and handle the missing template case safely:
function on_generate_handler(e) { e?.preventDefault?.(); const templatePath = Object.keys(templateFiles)[0]; - let result; - if (templatePath) { - const tpl = readFileRaw(templateFiles, templatePath); - const template = Handlebars.compile(tpl); - result = template(getSelectedKeys()); - } else { - // Fallback to old behavior if template missing - const parts = []; - const genericPath = Object.keys(genericFiles)[0]; - parts.push(readFileRaw(genericFiles, genericPath)); - const manticorePath = $("select[name='norn_manticore']").val(); - if (manticorePath) parts.push(readFileRaw(manticoreFiles, manticorePath)); - const vtxPath = $("select[name='norn_vtx']").val(); - if (vtxPath) parts.push(readFileRaw(vtxFiles, vtxPath)); - result = parts.filter(Boolean).join("\n\n").trim(); - } + if (!templatePath) { + console.error("Template not found"); + $("#norn_config_output").val("Error: Template not found"); + return; + } + const tpl = readFileRaw(templateFiles, templatePath); + const template = Handlebars.compile(tpl); + const result = template(getSelectedKeys()); $("#norn_config_output").val(result); }
226-228: Critical: Add cleanup for MSP status polling interval.The cleanup function does not remove the "status_pull" interval added at line 115-122. This causes the interval to continue running after the tab is closed, leading to unnecessary MSP traffic and resource leaks.
Apply this diff:
norn_config.cleanup = function (callback) { + GUI.interval_remove("status_pull"); if (callback) callback(); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)src/js/tabs/norn_config.js(1 hunks)src/norn-configs/README.txt(1 hunks)src/norn-configs/template.hbs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/norn-configs/README.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/norn_config.js (3)
src/js/gui.js (2)
GUI(466-466)TABS(8-8)src/js/msp.js (1)
MSP(5-467)src/js/msp/MSPCodes.js (1)
MSPCodes(2-210)
🔇 Additional comments (16)
src/norn-configs/template.hbs (6)
9-14: Verify duplicatedefaults nosavecommand.Line 9 and Line 13 both emit
defaults nosave. This duplication may be intentional for CLI compatibility, but please confirm that emitting this command twice is correct behavior and not a copy-paste error.
16-32: LGTM!The Flight Controller configuration blocks are correctly structured and use proper Handlebars conditionals. The fcKey values match the options populated in the UI (
norn_config.jslines 41-44).
65-77: Verify Manticore configuration commands are valid.The template emits configuration commands for UART and GPIO variants:
- UART:
uart1_function,uart2_function- GPIO:
gpio_output,gpio_modeEnsure these CLI commands (
set uart1_function,set gpio_output, etc.) are valid for the target firmware and that the values (RX_SERIAL,TELEMETRY,ON,ALTERNATE) are acceptable.
80-102: Verify VTX configuration commands and values.The template emits VTX configuration for three variants with different band letters (A, B, O) and power levels (25, 100, 50). Ensure:
set vtx_band,set vtx_channel,set vtx_powerare valid CLI commands- Band values A, B, O are valid band identifiers
- Power values are within acceptable ranges
105-110: LGTM!The GPS configuration block is straightforward and uses standard commands (
feature GPS,set gps_baudrate = AUTO).
113-117: Ensure craft name input is sanitized.The template directly interpolates
{{craftName}}into theset craft_namecommand without visible sanitization. If the user input contains special characters (quotes, newlines, etc.), it could break the CLI command syntax or inject unintended commands.Verify that
norn_config.js(line 106-109) properly sanitizes the craft name input before passing it to the template.src/js/tabs/norn_config.js (10)
1-12: LGTM!Imports and Handlebars helper registration are correct. The
eqhelper provides simple equality comparison for template conditionals.Based on learnings: The codebase uses Handlebars 4.7.8, which includes important security fixes. Ensure templates are never compiled from untrusted user input.
14-15: LGTM!Template loading using
import.meta.globwitheager: trueandas: "raw"is the correct approach for loading the Handlebars template file at build time.
17-31: LGTM!The initialization structure follows the standard tab pattern. The optional chaining for
mspHelper.readFullConfigurationproperly handles cases where the method might not be available.Note:
analyticsChangesis populated but not consumed in this file. Ensure it's used by the analytics system elsewhere.
41-44: Verify resolution of duplicate FC values.The Flight Controller dropdown now uses descriptive board names as values (
FLASHHOBBYF405,SPEEDYBEEF405V3,TAKERF722SE,GEPRCF722), which are unique and match the template's conditional checks.If the past review comment about duplicate "h7" values referred to these lines, it appears to be resolved. However, please confirm the values align with your intended configuration keys.
50-89: LGTM!Dropdown population for Drone Size, Manticore, and VTX follows a consistent pattern and the values match the conditional checks in
template.hbs. Change handlers properly updateanalyticsChanges.
94-110: LGTM!Button and input wiring is correct. Event handlers properly update
analyticsChanges.Note: Craft name input (line 107-109) doesn't sanitize the value before passing to the template. Ensure the template rendering handles special characters safely (checked separately in template review).
115-122: MSP status polling interval added.The tab adds a 500ms MSP status polling interval named "status_pull". This interval must be removed in the cleanup handler to prevent resource leaks.
See separate comment on the cleanup function.
173-186: LGTM!The copy handler correctly uses the Clipboard API with proper error handling for unsupported browsers or permission issues.
188-221: LGTM!The save handler properly:
- Generates a descriptive filename from selected options
- Falls back to a default name if no options are selected
- Creates a Blob and triggers a download
- Cleans up the object URL to prevent memory leaks
230-231: LGTM!Module exports follow the standard tab pattern with both global registration and ES6 export.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
locales/en/messages.json (1)
7802-7809: Add matching description strings for new Manticore OSD elements.OSD tooltips expect paired
osdDesc…keys; without them the UI falls back to the raw IDs, regressing usability. Please add the matching description entries alongside the new text keys."osdTextElementManticoreV1": { "message": "Manticore V1" }, + "osdDescElementManticoreV1": { + "message": "Displays the status for the Manticore V1 data feed." + }, "osdTextElementManticoreV2": { "message": "Manticore V2" }, + "osdDescElementManticoreV2": { + "message": "Displays the status for the Manticore V2 data feed." + }, "osdTextElementManticoreV2Logger": { "message": "Manticore V2 Logger" }, + "osdDescElementManticoreV2Logger": { + "message": "Displays the logger state for the Manticore V2 data feed." + },
🧹 Nitpick comments (3)
src/norn-configs/template.hbs (3)
16-31: Ensureeq/nehelpers are registered for Handlebars.These helpers aren’t built‑in; without registration, blocks won’t render.
Example registration (in the code that compiles this template):
import Handlebars from 'handlebars'; Handlebars.registerHelper('eq', (a, b) => a === b); Handlebars.registerHelper('ne', (a, b) => a !== b);
125-128: Quotecraft_name(and avoid HTML escaping).Unquoted names with spaces will fail. Also, default Handlebars HTML-escapes; avoid
"in CLI output.Minimal fix:
-set craft_name = {{craftName}} +set craft_name = "{{craftName}}"If names may contain quotes, register a helper to JSON/CLI-quote safely (e.g.,
set craft_name = {{bfQuote craftName}}).
99-101: Normalizevtxtablepower labels to match values.Labels should correspond to
powervaluesfor clarity (e.g., 25→25, 100→100, etc.). Mixed units/decimals (e.g.,1.5,2.5,1W,2W) are inconsistent.Examples:
- 5.8:
powervalues 25 100 200 400 600withpowerlabels 25 100 200 400 600- 3.3: either labels
25 100 200 400 600or25mW 100mW 200mW 400mW 600mW(consistent style)Also applies to: 112-115
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json(3 hunks)src/js/tabs/norn_config.js(1 hunks)src/norn-configs/README.txt(1 hunks)src/norn-configs/template.hbs(1 hunks)src/tabs/norn_config.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/norn-configs/README.txt
- src/tabs/norn_config.html
- src/js/tabs/norn_config.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T11:37:09.758Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#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
🔇 Additional comments (2)
src/norn-configs/template.hbs (2)
96-98: Confirm custom VTX band support and regulatory complianceBetaflight vtxtable accepts CUSTOM bands from 0–5999 MHz (including sub-5.5 GHz), so these frequencies are valid—verify that your VTX hardware and local spectrum regulations allow them.
43-66: Human-readable UART port names are supported
serial UART1 …is accepted by Betaflight CLI (v4.6+), so no changes needed.Likely an incorrect or invalid review comment.
| defaults nosave | ||
| batch start | ||
| defaults nosave | ||
|
|
There was a problem hiding this comment.
Fix CLI batch/session: remove duplicate defaults and add batch end.
Running defaults twice (once inside the batch) is redundant and risky; missing batch end prevents execution of the batched commands.
Apply:
defaults nosave
batch start
-defaults nosaveAnd append at the end of the template:
{{/if}}
+
+batch endOptionally, consider making defaults nosave opt‑in to avoid accidental wipes.
Also applies to: 130-133
🤖 Prompt for AI Agents
In src/norn-configs/template.hbs lines 11-14, remove the duplicate "defaults"
inside the batch (keep a single "defaults nosave" before the batch), and append
a "batch end" after the batched commands so the batch executes; also apply the
same change (remove duplicate defaults and add "batch end") to the block at
lines 130-133; optionally change the top "defaults nosave" to be opt-in (e.g.,
comment or conditional) if you want to avoid accidental wipes.
| rxfail 2 s 1500 | ||
| rxfail 3 h | ||
| {{/if}} |
There was a problem hiding this comment.
Replace non‑breaking spaces in FAILSAFE lines.
NBSP characters will break CLI parsing.
-rxfail 2 s 1500
-rxfail 3 h
+rxfail 2 s 1500
+rxfail 3 h📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rxfail 2 s 1500 | |
| rxfail 3 h | |
| {{/if}} | |
| rxfail 2 s 1500 | |
| rxfail 3 h | |
| {{/if}} |
🤖 Prompt for AI Agents
In src/norn-configs/template.hbs around lines 121-123, the FAILSAFE lines
contain non‑breaking space (U+00A0) characters that break CLI parsing; replace
any NBSPs with normal ASCII spaces so the lines read e.g. "rxfail 2 s 1500" and
"rxfail 3 h". Update the file encoding if needed and run a quick grep/sanity
check for \u00A0 to ensure no other NBSPs remain.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/js/tabs/norn_config.js (4)
117-118: Remove obsolete placeholder comment.This comment was flagged in a previous review and remains unchanged.
183-183: Remove outdated comment.This comment was flagged in a previous review and remains unchanged. The save and copy handlers are implemented below (lines 235-335).
266-276: Critical: Undefined variables in fallback path.This issue was flagged in a previous review and remains unfixed. The fallback code references three undefined variables that would cause a
ReferenceErrorat runtime.
340-342: Add cleanup for MSP status polling interval.This issue was flagged in a previous review and remains unfixed. The
status_pullinterval added at line 171 continues running after the tab is closed, causing unnecessary MSP traffic and resource leaks.
🧹 Nitpick comments (1)
src/js/tabs/norn_config.js (1)
250-254: Consider replacing alert() with in-app notification.Browser
alert()blocks the UI and provides a poor user experience. Consider using an in-app notification system that matches the configurator's UI style.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/tabs/norn_config.js(1 hunks)src/tabs/norn_config.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tabs/norn_config.html
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T20:42:20.332Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.
Applied to files:
src/js/tabs/norn_config.js
🧬 Code graph analysis (1)
src/js/tabs/norn_config.js (3)
src/js/gui.js (2)
GUI(466-466)TABS(8-8)src/js/msp.js (1)
MSP(5-467)src/js/msp/MSPCodes.js (1)
MSPCodes(2-210)
|
What's the purpose for this PR? Betaflight is currently in feature freeze for the upcoming Firmware and Configurator release, only allowing fixes. The changes would have to wait until the following release. There's no description of anything. To merge new UI elements, tabs, and/or features, we need some sort of description, reason, or prior discussion. |
|
@mzrks what is the purpose of this? |
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/norn-configs/template.hbs (2)
12-14: Duplicatedefaults nosavecommand still present.The template contains
defaults nosaveon line 12, followed bybatch start, then anotherdefaults nosaveon line 14. This duplication was flagged in previous reviews as a critical issue.According to the past review comments, this should have been addressed in commits 4d63c86 to 796ac22, but the duplicate still appears in the current version.
Apply this diff to remove the duplicate:
defaults nosave batch start -defaults nosave
315-315: Missingbatch endcommand.The template begins a
batch starton line 13 but never closes it with abatch end. Without this command, the batched CLI commands will not execute.This critical issue was flagged in previous reviews and marked as addressed, but the
batch endcommand is still missing from the template.Add the following at the end of the template:
# restore original rateprofile selection rateprofile 0 + +batch end
🧹 Nitpick comments (3)
src/tabs/norn_config.html (2)
2-11: Move inline styles to CSS file.The
.mandatory-missingstyles are defined inline within the HTML template. For better maintainability and consistency with the project's styling approach, these should be moved to a separate CSS file.Consider creating a dedicated CSS file (e.g.,
norn_config.css) or adding these styles to an existing stylesheet:.mandatory-missing { border: 2px solid #ff4444 !important; background-color: #ffe6e6 !important; } .mandatory-missing select { border: 2px solid #ff4444 !important; }
120-121: Add accessibility attributes for better screen reader support.The form inputs lack ARIA labels and other accessibility attributes that would improve usability for users with assistive technologies.
Consider adding
aria-labelor associating inputs with their labels usingforattributes:-<input type="text" id="norn_craft_name" placeholder="Enter craft name" +<input type="text" id="norn_craft_name" aria-label="Craft Name" i18n_placeholder="nornCraftNamePlaceholder" style="width: 100%;" />-<input type="checkbox" id="norn_gps" class="toggle" /> +<input type="checkbox" id="norn_gps" class="toggle" aria-label="Enable GPS" />For select elements, ensure they have proper labels associated:
+<label for="norn_fc" i18n="nornController"></label> <select name="norn_fc" id="norn_fc">Also applies to: 133-133, 149-149
src/norn-configs/template.hbs (1)
1-315: Provide documentation for template usage and integration.This template appears to be a critical component of the NORN Config feature, but there's no inline documentation explaining:
- What NORN Config is and what it configures
- The relationship between UI selections and template variables
- Expected format/constraints for dynamic values (craftName, mbId, etc.)
- How this integrates with the Betaflight CLI command structure
- Any FC-specific limitations or requirements
Consider adding a header comment block at the top of the template:
This documentation would help maintainers understand the template's purpose and integration points, especially given the lack of PR description.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/index.html(1 hunks)src/js/gui.js(2 hunks)src/js/tabs/norn_config.js(1 hunks)src/norn-configs/template.hbs(1 hunks)src/tabs/norn_config.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/tabs/norn_config.js
🔇 Additional comments (3)
src/index.html (1)
133-133: LGTM! Tab entries follow existing patterns.The new NORN Config tab entries are properly integrated into both disconnected and connected tab lists with correct i18n bindings and icon classes.
However, as noted in the PR comments by the maintainer, this PR lacks a description explaining what NORN Config is, why it's needed, and how it aligns with the current feature freeze. Please provide this context to help reviewers understand the purpose and scope of these changes.
Also applies to: 140-140
src/js/gui.js (2)
22-29: Verify that disconnected access is intentional.The NORN Config tab is added to
defaultAllowedTabsWhenDisconnected, making it accessible without an active connection. This is unusual for configuration tabs and should be verified as intentional.Most configuration-related tabs (e.g., "configuration", "ports", "pid_tuning") are only available when connected, while disconnected tabs typically handle application-level features (e.g., "landing", "firmware_flasher", "options").
Please confirm whether the NORN Config tab needs to function without a flight controller connection, or if it should only be available in the connected state.
31-49: LGTM! Tab properly registered for connected mode.The NORN Config tab is correctly added to the
defaultAllowedTabsarray, making it available when connected to a flight controller.
| rxfail 0 s 1500 | ||
| rxfail 1 s 1500 | ||
| rxfail 2 s 1500 | ||
| rxfail 3 h |
There was a problem hiding this comment.
🧩 Analysis chain
Verify FAILSAFE lines for non-breaking spaces.
Previous reviews flagged lines 136-137 (now 134-137 after the failsafe block start) as containing non-breaking space (U+00A0) characters that would break CLI parsing. The issue was marked as addressed, but it's important to verify the current state.
Run the following script to check for non-breaking spaces in the template:
🏁 Script executed:
#!/bin/bash
# Check for non-breaking spaces (U+00A0) in the template file
echo "Checking for non-breaking spaces in template.hbs..."
if grep -P '\u00A0' src/norn-configs/template.hbs; then
echo "ERROR: Non-breaking spaces found!"
echo "Lines with NBSPs:"
grep -nP '\u00A0' src/norn-configs/template.hbs
exit 1
else
echo "No non-breaking spaces found."
fi
# Also check the specific failsafe lines
echo -e "\nDisplaying failsafe section (lines 132-142):"
sed -n '132,142p' src/norn-configs/template.hbs | cat -ALength of output: 458
Replace non-breaking spaces in FAILSAFE lines
The rxfail entries in src/norn-configs/template.hbs still contain U+00A0 characters (shown as M-BM- in cat -A), which will break CLI parsing. Replace them with standard ASCII spaces.
🤖 Prompt for AI Agents
In src/norn-configs/template.hbs around lines 134 to 137, the four `rxfail`
lines contain U+00A0 non-breaking space characters which will break CLI parsing;
replace each non-breaking space with a standard ASCII space (0x20) so the lines
read e.g. `rxfail 0 s 1500`, `rxfail 1 s 1500`, `rxfail 2 s 1500`, `rxfail 3 h`,
and verify no U+00A0 remain in these lines.
| <input type="text" id="norn_craft_name" placeholder="Enter craft name" | ||
| style="width: 100%;" /> |
There was a problem hiding this comment.
Replace hardcoded placeholders with i18n keys.
The placeholder text "Enter craft name" and "Enter MB ID" are hardcoded in English, breaking the internationalization pattern used throughout the rest of the file.
Apply these changes to use i18n placeholders:
-<input type="text" id="norn_craft_name" placeholder="Enter craft name"
+<input type="text" id="norn_craft_name" i18n_placeholder="nornCraftNamePlaceholder"
style="width: 100%;" />-<input type="text" id="norn_mb_id" placeholder="Enter MB ID" style="width: 100%;" />
+<input type="text" id="norn_mb_id" i18n_placeholder="nornMBIdPlaceholder" style="width: 100%;" />Then add the corresponding keys to the locale files:
"nornCraftNamePlaceholder": "Enter craft name",
"nornMBIdPlaceholder": "Enter MB ID"Also applies to: 133-133
🤖 Prompt for AI Agents
In src/tabs/norn_config.html around lines 120-121 (and also line 133), replace
the hardcoded English placeholder attributes ("Enter craft name" and "Enter MB
ID") with the i18n key usage consistent with the file (e.g. set the placeholder
to the localized string via the existing i18n helper or data-binding used in
this file), and then add the corresponding keys "nornCraftNamePlaceholder" and
"nornMBIdPlaceholder" with the provided values to the locale JSON files; ensure
the markup matches the project’s i18n pattern (same helper/function or binding
syntax used elsewhere in this file) so placeholders render from locales.
3 participants
Important considerations when opening a pull request:
Make sure you do not make the changes you want to open a pull request for on the
masterbranch of your fork, or open the pull request from themasterbranch of your fork. Some of our integrations will fail if you do this, resulting in your pull request not being accepted. If this is your first pull request, it is probably a good idea to first read up on how opening pull requests work (https://opensource.com/article/19/7/create-pull-request-github is a good introduction);Pull requests will only be accepted if they are opened against the
masterbranch of our repository. Pull requests opened against other branches without prior consent from the maintainers will be closed;Please follow the coding style guidelines: https://betaflight.com/docs/development/CodingStyle
Keep your pull requests as small and concise as possible. One pull request should only ever add / update one feature. If the change that you are proposing has a wider scope, consider splitting it over multiple pull requests. In particular, pull requests that combine changes to features and one or more new targets are not acceptable.
Ideally, a pull request should contain only one commit, with a descriptive message. If your changes use more than one commit, rebase / squash them into one commit before submitting a pull request. If you need to amend your pull request, make sure that the additional commit has a descriptive message, or - even better - use
git commit --amendto amend your original commit.All pull requests are reviewed. Be ready to receive constructive criticism, and to learn and improve your coding style. Also, be ready to clarify anything that isn't already sufficiently explained in the code and text of the pull request, and to defend your ideas.
We use continuous integration (CI) with GitHub Actions to build all targets and run the test suite for every pull request. Pull requests that fail any of the builds or fail tests will most likely not be reviewed before they are fixed to build successfully and pass the tests. In order to get a quick idea if there are things that need fixing before opening a pull request or pushing an update into an existing pull request, run
yarn lintto verify formatting andyarn run vitest runto execute the test suite. Note: This is not an exhaustive test, so even if this passes the CI build might still fail.If your pull request is a fix for one or more issues that are open in GitHub, add a comment to your pull request, and add the issue numbers of the issues that are fixed in the form
Fixes #<issue number>. This will cause the issues to be closed when the pull request is merged;Remove this Text :).
Summary by CodeRabbit