.pr_agent_auto_best_practices

Pattern 1: Add defensive guards (early return, optional chaining, and type checks) before dereferencing nested properties or iterating, so malformed/partial data cannot crash runtime code paths.

Example code before:

function handle(node) {
  node.children.forEach(visit);   // crashes if children is missing/non-array
  return node.lc.value.trim();    // crashes if lc/value missing
}

Example code after:

function handle(node) {
  if (!node || !node.lc) return;
  const children = Array.isArray(node.children) ? node.children : [];
  children.forEach(visit);
  const v = node.lc.value;
  return typeof v === 'string' ? v.trim() : '';
}

Relevant past accepted suggestions:

---

Suggestion 1:

Prevent runtime error after null check

Add a return statement inside the if (data == null) block to prevent a runtime crash from accessing a property on a null object.

tabs/magnetometer.js [61-64]

 if (data == null) {
     console.log("while settting align_mag_roll, data is null or undefined");
+    return;
 }
 self.alignmentConfig.roll = parseInt(data.value, 10) / 10;

---

Suggestion 2:

Avoid unintended behavior with null values

Add a return statement inside the if (value == null) block to prevent self.boardAlignmentConfig.roll from being unintentionally set to 0 when value is null.

tabs/magnetometer.js [254-258]

 if (value == null) {
     console.log("in updateBoardRollAxis, value is null or undefined");
+    return;
 }
 
 self.boardAlignmentConfig.roll = Number(value);

---

Suggestion 3:

[learned best practice] Add a defensive early return (or defaulting) when `node`, `node.lc`, or `node.children` are missing so malformed trees cannot crash decompilation. [Learned best practice, importance: 5]

New proposed code:

---

Suggestion 4:

[possible issue] In `getImprovedWritabilityError`, check that `nested.properties` is not empty before accessing its first key to prevent generating error suggestions containing `undefined`.

---

Suggestion 5:

[learned best practice] Guard against non-array values (not just falsy) before calling `forEach`, and prefer optional chaining to avoid runtime errors when the releases structure is missing or malformed.

---

Suggestion 6:

[learned best practice] Guard access to `navigator` to prevent ReferenceError in non-browser contexts and ensure feature detection is safe.

---

Suggestion 7:

[i].customelementitems` before the inner loop in `fillcustomelementsvalues` to prevent a potential `typeerror`. [possible issue] Add a check for `FC.OSD_CUSTOM_ELEMENTS.items[i].customElementItems` before the inner loop in `fillCustomElementsValues` to prevent a potential `TypeError`. [possible issue, importance: 7]

New proposed code:

---

Pattern 2: Always handle promise rejections for dynamic imports and promise chains with explicit .catch() (or try/catch with await) and log actionable errors to avoid unhandled rejections and silent failures.

Example code before:

import(`./${name}.html?raw`).then(({ default: html }) => {
  render(html);
});

Example code after:

import(`./${name}.html?raw`)
  .then(({ default: html }) => render(html))
  .catch(err => console.error(`Failed to load ${name} html:`, err));

Relevant past accepted suggestions:

---

Suggestion 1:

Add error handling for dynamic import

Add a .catch() block to the dynamic import() for messages.json to handle potential loading errors and prevent unhandled promise rejections.

tabs/search.js [87-89]

 import(`../locale/en/messages.json`).then(({default: messages}) => {
     this.messages = messages;
+}).catch(error => {
+    console.error('Failed to load messages:', error);
 });

---

Suggestion 2:

Add error handling for dynamic imports

Add .catch() blocks to the dynamic import() calls for tab .js and .html files to handle potential loading errors and prevent unhandled promise rejections.

tabs/search.js [141-147]

 import(`./${tabName}.js?raw`).then(({default: javascript}) => {
     this.geti18nJs(tabName, javascript);
-});
+}).catch(error => console.error(`Failed to index JS for tab ${tabName}:`, error));
 
 import(`./${tabName}.html?raw`).then(({default: html}) => {
     this.geti18nHTML(tabName, html);
-});
+}).catch(error => console.error(`Failed to index HTML for tab ${tabName}:`, error));

---

Suggestion 3:

[possible issue] Refactor the promise handling to use a `.catch()` block for errors instead of checking for an error object within the `.then()` callback.

---

Suggestion 4:

[general] Add a `.catch()` block to the `settingsPromise` chain to handle and log potential errors during the settings loading process.

---

Pattern 3: For timeouts, polling loops, and event/listener-based async flows, ensure the completion callback can only fire once and that timers/listeners are always cleaned up on success, failure, or timeout.

Example code before:

function waitForDone(cb) {
  const t = setTimeout(() => cb(false), 2000);
  conn.onReceive(data => {
    if (data.includes('OK')) cb(true); // can fire multiple times; timer not cleared
  });
}

Example code after:

function waitForDone(cb) {
  let done = false;
  let onReceive;
  const finalize = (ok) => {
    if (done) return;
    done = true;
    clearTimeout(t);
    conn.removeOnReceiveCallback(onReceive);
    cb(ok);
  };
  const t = setTimeout(() => finalize(false), 2000);
  onReceive = (data) => {
    if (data.includes('OK')) finalize(true);
  };
  conn.addOnReceiveCallback(onReceive);
}

Relevant past accepted suggestions:

---

Suggestion 1:

Guard and sanitize reboot polling loop

Refactor pollForRebootCompletion to correctly use the promise-based ConnectionSerial.getDevices API. Additionally, add a guard flag to prevent race conditions and ensure the polling interval is always cleared.

js/protocols/stm32.js [61-99]

 STM32_protocol.prototype.pollForRebootCompletion = function(port, hex, options, onSuccess, onTimeout) {
     var self = this;
     var intervalMs = 200;
     var retries = 0;
     var maxRetries = 50; // timeout after intervalMs * 50 (10 seconds)
+    var finished = false;
+
+    var finishOnce = function(fn) {
+        if (!finished) {
+            finished = true;
+            clearInterval(pollInterval);
+            fn && fn();
+        }
+    };
 
     var pollInterval = setInterval(function() {
+        if (finished) return;
         retries++;
         if (retries > maxRetries) {
-            clearInterval(pollInterval);
-            onTimeout();
+            finishOnce(onTimeout);
             return;
         }
 
         // Check for DFU devices first
         PortHandler.check_usb_devices(function(dfu_available) {
+            if (finished) return;
             if (dfu_available) {
-                clearInterval(pollInterval);
-                STM32DFU.connect(usbDevices, hex, options);
+                finishOnce(function() {
+                    STM32DFU.connect(usbDevices, hex, options);
+                });
                 return;
             }
 
-            // Check for serial port
-            ConnectionSerial.getDevices(function(devices) {
-                if (devices && devices.includes(port)) {
-                    // Serial port reappeared - try to connect
-                    CONFIGURATOR.connection.connect(port, {bitrate: self.baud, parityBit: 'even', stopBits: 'one'}, function (openInfo) {
-                        if (openInfo) {
-                            clearInterval(pollInterval);
-                            onSuccess();
-                        } else {
-                            GUI.connect_lock = false;
+            // Check for serial port (promise-based)
+            ConnectionSerial.getDevices().then(function(devices) {
+                if (finished) return;
+                if (Array.isArray(devices) && devices.includes(port)) {
+                    CONFIGURATOR.connection.connect(
+                        port,
+                        { bitrate: self.baud, parityBit: 'even', stopBits: 'one' },
+                        function(openInfo) {
+                            if (finished) return;
+                            if (openInfo) {
+                                finishOnce(onSuccess);
+                            } else {
+                                GUI.connect_lock = false;
+                            }
                         }
-                    });
+                    );
                 }
+            }).catch(function() {
+                // ignore errors and keep polling
             });
         });
     }, intervalMs);
 };

---

Suggestion 2:

Scope and harden response listener

Improve the waitForResponse function by ensuring the receive listener only processes data for the current connection and by using a more robust method for string conversion.

js/protocols/stm32.js [101-139]

-// Waits for a specific response from the serial connection with timeout
 STM32_protocol.prototype.waitForResponse = function(expectedString, timeoutMs, callback) {
     var receivedData = '';
     var timeoutHandle = null;
     var onReceiveListener = null;
+    var connectionId = CONFIGURATOR.connection && CONFIGURATOR.connection._connectionId;
 
     var cleanup = function() {
         if (timeoutHandle) {
             clearTimeout(timeoutHandle);
             timeoutHandle = null;
         }
-        if (onReceiveListener) {
+        if (onReceiveListener && CONFIGURATOR.connection) {
             CONFIGURATOR.connection.removeOnReceiveCallback(onReceiveListener);
             onReceiveListener = null;
         }
     };
 
-    // Set up timeout
+    // Bail out early if no active connection
+    if (!connectionId) {
+        callback(false, receivedData);
+        return;
+    }
+
     timeoutHandle = setTimeout(function() {
         cleanup();
         console.log('Timeout waiting for response:', expectedString);
         callback(false, receivedData);
     }, timeoutMs);
 
-    // Set up receive listener
     onReceiveListener = function(info) {
+        // Ignore data from other connections
+        if (info.connectionId && info.connectionId !== connectionId) {
+            return;
+        }
         var data = new Uint8Array(info.data);
-        var str = String.fromCharCode.apply(null, data);
+        // Robust conversion without apply to avoid call-stack issues on large buffers
+        var decoder = new TextDecoder('utf-8');
+        var str = decoder.decode(data);
         receivedData += str;
 
-        // Check if we received the expected string
-        if (receivedData.includes(expectedString)) {
+        if (receivedData.indexOf(expectedString) !== -1) {
             cleanup();
             callback(true, receivedData);
         }
     };
 
     CONFIGURATOR.connection.addOnReceiveCallback(onReceiveListener);
 };

---

Suggestion 3:

Guard and cleanup listener once

Add a one-time guard to ensure the callback fires once and the listener is always removed even on rapid multiple matches.

js/protocols/stm32.js [102-139]

 STM32_protocol.prototype.waitForResponse = function(expectedString, timeoutMs, callback) {
     var receivedData = '';
     var timeoutHandle = null;
     var onReceiveListener = null;
+    var done = false;
 
-    var cleanup = function() {
+    var finalize = function(success) {
+        if (done) return;
+        done = true;
         if (timeoutHandle) {
             clearTimeout(timeoutHandle);
             timeoutHandle = null;
         }
         if (onReceiveListener) {
             CONFIGURATOR.connection.removeOnReceiveCallback(onReceiveListener);
             onReceiveListener = null;
         }
+        callback(success, receivedData);
     };
 
-    // Set up timeout
     timeoutHandle = setTimeout(function() {
-        cleanup();
         console.log('Timeout waiting for response:', expectedString);
-        callback(false, receivedData);
+        finalize(false);
     }, timeoutMs);
 
-    // Set up receive listener
     onReceiveListener = function(info) {
         var data = new Uint8Array(info.data);
         var str = String.fromCharCode.apply(null, data);
         receivedData += str;
-
-        // Check if we received the expected string
         if (receivedData.includes(expectedString)) {
-            cleanup();
-            callback(true, receivedData);
+            finalize(true);
         }
     };
 
     CONFIGURATOR.connection.addOnReceiveCallback(onReceiveListener);
 };

---

Suggestion 4:

Prevent multiple callback invocations

To prevent a race condition in waitForResponse, add a flag to ensure the callback is only executed once, even if multiple data chunks are received in rapid succession.

js/protocols/stm32.js [114-151]

 STM32_protocol.prototype.waitForResponse = function(expectedString, timeoutMs, callback) {
     var receivedData = '';
     var timeoutHandle = null;
     var onReceiveListener = null;
+    var callbackFired = false;
 
     var cleanup = function() {
         if (timeoutHandle) {
             clearTimeout(timeoutHandle);
             timeoutHandle = null;
         }
         if (onReceiveListener) {
             CONFIGURATOR.connection.removeOnReceiveCallback(onReceiveListener);
             onReceiveListener = null;
         }
     };
 
+    var executeCallback = function(success, data) {
+        if (!callbackFired) {
+            callbackFired = true;
+            cleanup();
+            callback(success, data);
+        }
+    };
+
     // Set up timeout
     timeoutHandle = setTimeout(function() {
-        cleanup();
         console.log('Timeout waiting for response:', expectedString);
-        callback(false, receivedData);
+        executeCallback(false, receivedData);
     }, timeoutMs);
 
     // Set up receive listener
     onReceiveListener = function(info) {
         var data = new Uint8Array(info.data);
         var str = String.fromCharCode.apply(null, data);
         receivedData += str;
 
         // Check if we received the expected string
         if (receivedData.includes(expectedString)) {
-            cleanup();
-            callback(true, receivedData);
+            executeCallback(true, receivedData);
         }
     };
 
     CONFIGURATOR.connection.addOnReceiveCallback(onReceiveListener);
 };

---

Pattern 4: Prefer correct project APIs and canonical constants/enums over ad-hoc logic (e.g., using documented promise-based signatures, correct list mutation methods, and official name/enum mappings) to keep behavior aligned with upstream changes.

Example code before:

const name = Object.keys(ENUM).find(k => ENUM[k] === code).toLowerCase();
api.getDevices((devices) => { /* ... */ }); // but API returns a Promise
list.put(value); // appends; intended to overwrite a specific slot

Example code after:

const name = ENUM_NAMES[code];
api.getDevices()
  .then(devices => Array.isArray(devices) && devices.includes(port))
  .catch(() => {});
list.set(slotIndex, value);

Relevant past accepted suggestions:

---

Suggestion 1:

Correctly clear stale logic conditions

Replace FC.LOGIC_CONDITIONS.put(emptyCondition) with FC.LOGIC_CONDITIONS.set(oldSlot, emptyCondition) to correctly clear stale logic conditions at their specific index rather than appending to the list.

tabs/javascript_programming.js [658-687]

 // Find slots that need to be cleared (were occupied, now aren't)
 if (self.previouslyOccupiedSlots) {
+    const emptyCondition = {
+        enabled: 0,
+        activatorId: -1,
+        operation: 0,
+        operandAType: 0,
+        operandAValue: 0,
+        operandBType: 0,
+        operandBValue: 0,
+        flags: 0,
+
+        getEnabled: function() { return this.enabled; },
+        getActivatorId: function() { return this.activatorId; },
+        getOperation: function() { return this.operation; },
+        getOperandAType: function() { return this.operandAType; },
+        getOperandAValue: function() { return this.operandAValue; },
+        getOperandBType: function() { return this.operandBType; },
+        getOperandBValue: function() { return this.operandBValue; },
+        getFlags: function() { return this.flags; }
+    };
+
     for (const oldSlot of self.previouslyOccupiedSlots) {
         if (!newlyOccupiedSlots.has(oldSlot)) {
-            // This slot was occupied before but isn't in new script
-            // Add a disabled/empty condition to clear it
-            const emptyCondition = {
-                enabled: 0,
-                activatorId: -1,
-                operation: 0,
-                operandAType: 0,
-                operandAValue: 0,
-                operandBType: 0,
-                operandBValue: 0,
-                flags: 0,
-
-                getEnabled: function() { return this.enabled; },
-                getActivatorId: function() { return this.activatorId; },
-                getOperation: function() { return this.operation; },
-                getOperandAType: function() { return this.operandAType; },
-                getOperandAValue: function() { return this.operandAValue; },
-                getOperandBType: function() { return this.operandBType; },
-                getOperandBValue: function() { return this.operandBValue; },
-                getFlags: function() { return this.flags; }
-            };
-
-            FC.LOGIC_CONDITIONS.put(emptyCondition);
+            // This slot was occupied before but isn't in new script.
+            // Place an empty condition at the specific slot index to clear it.
+            FC.LOGIC_CONDITIONS.set(oldSlot, emptyCondition);
         }
     }
 }

---

Suggestion 2:

Use correct constant for parameter mapping

To correctly map a flight parameter number to its name in the test log, use the FLIGHT_PARAM_NAMES constant instead of converting the FLIGHT_PARAM key to lowercase.

js/transpiler/transpiler/tests/test_flight.js [181-186]

-const paramName = Object.keys(FLIGHT_PARAM).find(key => FLIGHT_PARAM[key] === p);
+const paramName = FLIGHT_PARAM_NAMES[p];
 if (paramName) {
-  console.log(`  Param ${p.toString().padStart(2)} → flight.${paramName.toLowerCase()}`);
+  console.log(`  Param ${p.toString().padStart(2)} → flight.${paramName}`);
 } else {
   console.log(`  Param ${p.toString().padStart(2)} → UNKNOWN`);
 }

---

Suggestion 3:

Use proper async handling

ConnectionSerial.getDevices() is async and returns a promise per its definition; use then/catch or await instead of a callback and guard devices shape before includes.

js/protocols/stm32.js [91-103]

-ConnectionSerial.getDevices(function(devices) {
-    if (devices && devices.includes(port)) {
-        // Serial port reappeared - try to connect
+ConnectionSerial.getDevices().then(devices => {
+    if (Array.isArray(devices) && devices.includes(port)) {
         CONFIGURATOR.connection.connect(port, {bitrate: self.baud, parityBit: 'even', stopBits: 'one'}, function (openInfo) {
             if (openInfo) {
                 clearInterval(pollInterval);
                 onSuccess();
             } else {
                 GUI.connect_lock = false;
             }
         });
     }
+}).catch(() => {
+    // ignore and keep polling
 });

---

Suggestion 4:

[learned best practice] Import and use the project enums/constants (e.g., `OPERATION`, `OPERAND_TYPE`) instead of hard-coded numeric operation/operand codes to keep tests aligned with the actual mapping.

---

Suggestion 5:

[learned best practice] Update the script to check the actual `Transpiler.transpile()` output shape (e.g., `result.warnings.errors`) and/or rely on exceptions, instead of referencing a non-existent `result.errors` field.

---

Pattern 5: Keep UI bindings consistent by matching DOM ids/attributes exactly and updating visibility/state in the correct order when dependent selections change, so the UI cannot display stale or mismatched controls.

Example code before:

if (mode === 'SERIAL') {
  $('#serialWrapper').show();
} else {
  $('#serialWrapper').hide();
}
$('#armedicon').addClass('armed-active'); // wrong id; no effect

Example code after:

if (mode === 'SERIAL') {
  $('#serialWrapper').show();
} else {
  $('#serialWrapper').hide();
  $('#frSkyOptions').hide();
}
$('#armedIcon').addClass('armed-active');

Relevant past accepted suggestions:

---

Suggestion 1:

Hide FrSky options for non-serial receivers

Hide the #frSkyOptions section when the receiver mode is changed to a non-SERIAL type to prevent it from being displayed incorrectly.

tabs/receiver.js [86-93]

 $receiverMode.on('change', function () {
     if ($(this).find("option:selected").text() == "SERIAL") {
         $serialWrapper.show();
     } else {
         $serialWrapper.hide();
+        $("#frSkyOptions").hide();
     }
 });

---

Suggestion 2:

Fix incorrect help icon association

Update the for attribute on the help icon div for the fuel unit dropdown from frSkyPitchRoll to frSkyFuelUnit to correctly associate it.

tabs/receiver.html [87]

-<div for="frSkyPitchRoll" class="helpicon cf_tip" data-i18n_title="serialrx_frSkyFuelUnitHelp"></div>
+<div for="frSkyFuelUnit" class="helpicon cf_tip" data-i18n_title="serialrx_frSkyFuelUnitHelp"></div>

---

Suggestion 3:

Correct the data unit mismatch

Update the data-unit attribute for the osdPanServoPwm2decadeg input from centideg-deg to decadeg-deg. This ensures the UI sends the correct value to the firmware, matching the unit change implied by the setting's new name.

tabs/osd.html [383]

-<input id="osdPanServoPwm2decadeg" data-setting="osd_pan_servo_pwm2decadeg" data-unit="centideg-deg" data-setting-multiplier="1" type="number" data-step="1" />
+<input id="osdPanServoPwm2decadeg" data-setting="osd_pan_servo_pwm2decadeg" data-unit="decadeg-deg" data-setting-multiplier="1" type="number" data-step="1" />

---

Suggestion 4:

Fix mismatched DOM id selector

Fix the jQuery selector for the armed status icon by changing #armedicon to #armedIcon to match the element's ID in the HTML.

js/periodicStatusUpdater.js [42-48]

 if (FC.isModeEnabled('ARM')) {
-    $("#armedicon").removeClass('armed');
-    $("#armedicon").addClass('armed-active');
+    $("#armedIcon").removeClass('armed');
+    $("#armedIcon").addClass('armed-active');
 } else {
-    $("#armedicon").removeClass('armed-active');
-    $("#armedicon").addClass('armed');
+    $("#armedIcon").removeClass('armed-active');
+    $("#armedIcon").addClass('armed');
 }

---

Suggestion 5:

[learned best practice] Trigger `receiver` mode first (or only) so it determines whether the Serial/FrSky sections should be shown before any provider-based logic runs, avoiding a brief incorrect FrSky visibility.

---

[Auto-generated best practices - 2026-06-01]