Skip to content

.pr_agent_auto_best_practices

qodo-merge-bot edited this page Jan 6, 2026 · 5 revisions

Pattern 1: Add explicit guards and early returns before dereferencing values that may be null/undefined or not the expected type (e.g., object properties, arrays for iteration, or environment globals), so runtime execution cannot proceed into invalid access.

Example code before:

function render(items) {
  items.forEach(x => console.log(x.id));
  console.log(navigator.userAgent);
}

Example code after:

function render(items) {
  if (!Array.isArray(items)) return;
  items.forEach(x => console.log(x.id));
  if (typeof navigator !== 'undefined') console.log(navigator.userAgent);
}
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] 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 4:
  • [learned best practice] Guard access to `navigator` to prevent ReferenceError in non-browser contexts and ensure feature detection is safe.
    Suggestion 5:
  • [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: For promise-based flows (dynamic import, settings loading, IPC calls), handle failures explicitly by adding .catch() (or try/catch with async/await) and propagate errors via rejection/throw rather than returning error objects as “successful” values.

    Example code before:

    import('./feature.js').then(m => m.init());
    return apiCall().then(res => (res.error ? res.error : res));
    

    Example code after:

    import('./feature.js')
      .then(m => m.init())
      .catch(err => console.error('Failed to load feature:', err));
    return apiCall().catch(err => {
      console.error('apiCall failed:', err);
      throw 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:

    Fix silent error handling bug

    Refactor the appendFile IPC handler to be a simple async function that properly rejects on error, instead of resolving with an error object, to enable correct error handling in the renderer process.

    js/main/main.js [320-327]

    -return new Promise(async resolve => {
    -  try {
    -    await appendFile(filename, data);
    -    resolve(false)
    -  } catch (err) {
    -    resolve(err);
    -  }
    -});
    +try {
    +  await appendFile(filename, data);
    +  return false;
    +} catch (err) {
    +  // Re-throwing the error will cause the promise on the renderer side to be rejected.
    +  throw err;
    +}

    Suggestion 4:
  • [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 5:
  • [general] Add a `.catch()` block to the `settingsPromise` chain to handle and log potential errors during the settings loading process.

  • Pattern 3: In polling loops and event/listener-based async code, prevent races by ensuring completion happens once (done flag), and always clean up resources (clearInterval/clearTimeout and remove listeners) on both success and timeout/error paths.

    Example code before:

    function waitForEvent(cb) {
      const t = setTimeout(() => cb(false), 5000);
      conn.on('data', (buf) => {
        if (buf.includes('OK')) cb(true);
      });
    }
    

    Example code after:

    function waitForEvent(cb) {
      let done = false;
      let onData;
      const finalize = (ok) => {
        if (done) return;
        done = true;
        clearTimeout(t);
        conn.off('data', onData);
        cb(ok);
      };
      const t = setTimeout(() => finalize(false), 5000);
      onData = (buf) => { if (buf.includes('OK')) finalize(true); };
      conn.on('data', onData);
    }
    
    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);
     };

    Suggestion 5:

    Fix listener array mismatch

    Register receive callbacks to the correct listener array and remove from the same array to avoid broken bindings.

    js/connection/connectionUdp.js [105-111]

     addOnReceiveCallback(callback){
    -    this._onReceiveErrorListeners.push(callback);
    +    this._onReceiveListeners.push(callback);
     }
     
     removeOnReceiveCallback(callback){
    -    this._onReceiveListeners = this._onReceiveErrorListeners.filter(listener => listener !== callback);
    +    this._onReceiveListeners = this._onReceiveListeners.filter(listener => listener !== callback);
     }

    Pattern 4: Use the real source of truth (constants, correct indices, correct DOM IDs/attributes/units) instead of ad-hoc derivations, and update tests/docs/UI to match current behavior so mappings don’t silently drift.

    Example code before:

    const name = Object.keys(PARAMS).find(k => PARAMS[k] === n).toLowerCase();
    $('#armedicon').addClass('on');
    list.put(value); // appends, doesn't replace
    

    Example code after:

    const name = PARAM_NAMES[n];
    $('#armedIcon').addClass('on');
    list.set(slotIndex, value); // updates the intended slot
    
    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:

    Update test to current reality

    Update the test header to reflect that params 46–49 now exist, and add assertions that exercise these new operands to prevent regressions.

    js/transpiler/transpiler/tests/test_flight.js [6-16]

     /**
    - * KNOWN ISSUE:
    - * flight.js and inav_constants.js are missing parameters 46-49:
    + * Wind parameters now supported (46–49):
      * - 46: MIN_GROUND_SPEED
      * - 47: HORIZONTAL_WIND_SPEED
      * - 48: WIND_DIRECTION
      * - 49: RELATIVE_WIND_OFFSET
      *
    - * This test verifies existing params (0-45) work correctly and documents
    - * what's missing.
    + * This test verifies params (0–49), including wind-related telemetry.
      */

    Suggestion 4:

    Align test docs with API

    Replace the “known bug” preface with an explanation that pid.js now only exposes output and add a brief assertion that other properties are intentionally unavailable.

    js/transpiler/transpiler/tests/test_pid.js [2-26]

     /**
    - * KNOWN BUG TO DETECT:
    - * pid.js claims to expose many properties per PID:
    - * ...
    + * PID API verification:
    + * Firmware exposes only PID outputs via operands 0–3.
    + * pid.js intentionally exposes only pid[N].output; other properties are not available by design.
      * Run with: node test_pid.js
      */

    Suggestion 5:

    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 6:

    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');
     }

    Pattern 5: When wiring handlers/resources, ensure references and binding targets are correct (correct object, correct import path, correct key access), and avoid accidental shadowing or premature references to not-yet-defined objects.

    Example code before:

    import Feature from 'lib/format/Feature';
    const socket = createSocket();
    this._socket.on('message', onMsg);
    function load(model) { loader.load(model, obj => { const model = obj.scene; }); }
    

    Example code after:

    import Feature from 'lib/Feature.js';
    const socket = createSocket();
    socket.on('message', onMsg);
    function load(modelUrl) { loader.load(modelUrl, obj => { const modelScene = obj.scene; }); }
    
    Relevant past accepted suggestions:
    Suggestion 1:

    Fix UDP event binding and window guard

    Correct the UDP connection logic by attaching event listeners to the correct socket object and adding a guard to prevent calling methods on a non-window object.

    js/main/udp.js [3-34]

     const socket = dgram.createSocket('udp4');
     
     const udp = {
         _id: 1,
    -    _ip: false,
    -    _port: false,
    -    connect: function(ip, port, window = true) {
    +    _ip: null,
    +    _port: null,
    +    connect: function(ip, port, window) {
             return new Promise(resolve => {     
                 try {
    -                socket.bind(port, () => {
    +                socket.once('listening', () => {
                         this._ip = ip;
                         this._port = port;
    +                    resolve({ error: false, id: this._id++ });
                     });
     
    -                this._socket.on('error', error => {
    -                    if (!window.isDestroyed()) {
    +                socket.on('error', error => {
    +                    if (window && typeof window.isDestroyed === 'function' && !window.isDestroyed()) {
                             window.webContents.send('udpError', error); 
                         }
                     });
     
    -                this._socket.on('message', (message, _rinfo) => {
    -                    if (!window.isDestroyed()) {
    +                socket.on('message', (message, _rinfo) => {
    +                    if (window && typeof window.isDestroyed === 'function' && !window.isDestroyed()) {
                             window.webContents.send('udpMessage', message);
                         }
                     });
    -                resolve({error: false, id: this._id++});                   
    +
    +                socket.bind(port);
                 } catch (err) {
    -                resolve ({error: true, errorMsg: err});
    +                resolve({ error: true, errorMsg: err });
                 }
             });
         },

    Suggestion 2:

    Fix undefined handler target

    **

    •             * @type {ol.Pixel}
      
    •             * @private
      
    •             */
                 this.coordinate_ = null;
      
    •            /**
      
    •             * @type {string|undefined}
      
    •             * @private
      
    •             */
                 this.cursor_ = 'pointer';
      
    •            /**
      
    •             * @type {Feature}
      
    •             * @private
      
    •             */
                 this.feature_ = null;
      
    •            /**
      
    •             * @type {string|undefined}
      
    •             * @private
      
    •             */
                 this.previousCursor_ = undefined;
             }
      
    •    };
      
    •    }
      
    
    
    
    
    
    
    **Fix a reference error in the Drag class constructor by correctly referencing event handlers. The app object's methods are not defined at the time of instantiation.**
    
    [tabs/mission_control.js [2000-2014]](https://github.com/iNavFlight/inav-configurator/pull/2285/files#diff-54d84cb8c735be5909a6f1d56d93c5033b9884a1e6182fa5ac1eb619a8443786R2000-R2014)
    
    ```diff
    -class Drag extends PointerInteraction{
    +class Drag extends PointerInteraction {
         constructor() {
    -        super ({
    -            handleDownEvent: app.handleDownEvent,
    -            handleDragEvent: app.handleDragEvent,
    -            handleMoveEvent: app.handleMoveEvent,
    -            handleUpEvent: app.handleUpEvent
    +        super({
    +            handleDownEvent: (evt) => this.handleDownEvent(evt),
    +            handleDragEvent: (evt) => this.handleDragEvent(evt),
    +            handleMoveEvent: (evt) => this.handleMoveEvent(evt),
    +            handleUpEvent: (evt) => this.handleUpEvent(evt),
             });
    +        this.coordinate_ = null;
    +        this.cursor_ = 'pointer';
    +        this.feature_ = null;
    +        this.previousCursor_ = undefined;
    +    }
     
    -        /**
    -         * @type {ol.Pixel}
    -         * @private
    -         */
    -        this.coordinate_ = null;
    -        ...
    +    handleDownEvent(evt) {
    +        return app.handleDownEvent(evt);
         }
    -};
    +    handleDragEvent(evt) {
    +        return app.handleDragEvent(evt);
    +    }
    +    handleMoveEvent(evt) {
    +        return app.handleMoveEvent(evt);
    +    }
    +    handleUpEvent(evt) {
    +        return app.handleUpEvent(evt);
    +    }
    +}
    

    Suggestion 3:

    Correct OpenLayers Feature import

    Change the import for Feature from ol/format/Feature to ol/Feature.js to correctly import the feature constructor.

    js/groundstation.js [13-175]

    -import Feature from 'ol/format/Feature';
    +import Feature from 'ol/Feature.js';
     ...
     privateScope.cursorFeature = new Feature({
         geometry: privateScope.cursorPosition
     });

    Suggestion 4:

    Normalize and validate icon key mapping

    **

    •             * @type {ol.Pixel}
      
    •             * @private
      
    •             */
                 this.coordinate_ = null;
      
    •            /**
      
    •             * @type {string|undefined}
      
    •             * @private
      
    •             */
                 this.cursor_ = 'pointer';
      
    •            /**
      
    •             * @type {Feature}
      
    •             * @private
      
    •             */
                 this.feature_ = null;
      
    •            /**
      
    •             * @type {string|undefined}
      
    •             * @private
      
    •             */
                 this.previousCursor_ = undefined;
             }
      
    •    };
      
    •    }
      
         app.ConvertCentimetersToMeters = function (val) {
             return parseInt(val) / 100;
      

    @@ -2044,7 +2033,7 @@ var button = document.createElement('button');

                 button.innerHTML = ' ';
    
    •            button.style = `background: url("${icons.settings_white}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
      
    •            button.style = `background: url("${icons['settings_white']}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
                 
      
                 var handleShowSettings = function () {
      

    @@ -2073,7 +2062,7 @@ var button = document.createElement('button');

                 button.innerHTML = ' ';
    
    •            button.style = `background: url("${icons.icon_safehome_white}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
      
    •            button.style = `background: url("${icons['icon_safehome_white']}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
                 
                 var handleShowSafehome = function () {
                     $('#missionPlannerSafehome').fadeIn(300);
      

    @@ -2105,7 +2094,7 @@ var button = document.createElement('button');

                 button.innerHTML = ' ';
    
    •            button.style = `background: url("${icons.icon_geozone_white}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
      
    •            button.style = `background: url("${icons['icon_geozone_white']}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
                 
                 var handleShowGeozoneSettings = function () {
                     $('#missionPlannerGeozones').fadeIn(300);
      

    @@ -2138,7 +2127,7 @@ var button = document.createElement('button');

                 button.innerHTML = ' ';
    
    •            button.style = `background: url("${icons.icon_elevation_white}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
      
    •            button.style = `background: url("${icons['icon_elevation_white']}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
      
                 var handleShowSettings = function () {
                     $('#missionPlannerHome').fadeIn(300);
      

    @@ -2171,7 +2160,7 @@ var button = document.createElement('button');

                 button.innerHTML = ' ';
    
    •            button.style = `background: url("${icons.icon_multimission_white}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
      
    •            button.style = `background: url("${icons['icon_multimission_white']}") no-repeat 1px -1px;background-color: rgba(0,60,136,.5);`;
      
    
    
    
    
    
    
    **Normalize the icon key mapping to prevent broken image URLs. The dynamic import path is inconsistent with how icons are accessed, and the iconNames array contains a duplicate entry.**
    
    [tabs/mission_control.js [70-198]](https://github.com/iNavFlight/inav-configurator/pull/2285/files#diff-54d84cb8c735be5909a6f1d56d93c5033b9884a1e6182fa5ac1eb619a8443786R70-R198)
    
    ```diff
     const iconNames = [
         'icon_mission_airplane.png',
    -    ...
    -    'icon_multimission_white.svg'    
    +    'icon_RTH.png',
    +    'icon_safehome.png',
    +    'icon_safehome_used.png',
    +    'icon_geozone_excl.png',
    +    'icon_geozone_incl.png',
    +    'icon_home.png',
    +    'icon_position_edit.png',
    +    'icon_position_head.png',
    +    'icon_position_LDG_edit.png',
    +    'icon_position_LDG.png',
    +    'icon_position_PH_edit.png',
    +    'icon_position_PH.png',
    +    'icon_position_POI.png',
    +    'icon_position_POI_edit.png',
    +    'icon_position_WP_edit.png',
    +    'icon_position_WP.png',
    +    'icon_arrow.png',
    +    'settings_white.svg',
    +    'icon_safehome_white.svg',
    +    'icon_geozone_white.svg',
    +    'icon_elevation_white.svg',
    +    'icon_multimission_white.svg'
     ];
    -var icons = {};
    -...
    +const icons = Object.create(null);
    +
    +function iconKey(filename) {
    +    // drop extension, keep base name (e.g., "icon_RTH")
    +    return filename.replace(/\.(png|svg)$/i, '');
    +}
    +
     async function loadIcons() {
    -    for (const icon of iconNames) {
    -        const nameSplit = icon.split('.');
    -        // Vites packager needs a bit help
    -        var iconUrl;
    -        if (nameSplit[1] == 'png') {
    -            iconUrl = (await import(`./../images/icons/map/cf_${nameSplit[0]}.png?inline`)).default;
    -        } else if (nameSplit[1] == 'svg') {
    -            iconUrl = (await import(`./../images/icons/map/cf_${nameSplit[0]}.svg?inline`)).default;
    +    for (const fname of iconNames) {
    +        const base = iconKey(fname);
    +        const ext = fname.split('.').pop();
    +        let iconUrl;
    +        if (ext === 'png') {
    +            iconUrl = (await import(`./../images/icons/map/cf_${base}.png?inline`)).default;
    +        } else if (ext === 'svg') {
    +            iconUrl = (await import(`./../images/icons/map/cf_${base}.svg?inline`)).default;
             }
    -        if (iconUrl) {
    -            icons[nameSplit[0]] = iconUrl;
    +        if (!iconUrl) {
    +            throw new Error(`Missing icon URL for ${fname}`);
             }
    +        icons[base] = iconUrl;
         }
     }
     
    +// usage examples adjusted:
    +// src: icons['icon_RTH']
    +// src: icons['icon_position' + (suffixes...)]
    +
    

    Suggestion 5:

    Prevent variable shadowing

    Rename the model variable inside the loader.load callback to avoid shadowing the model variable from the outer scope, which holds the imported URL.

    tabs/magnetometer.js [741-746]

    -import(`./../resources/models/model_${model_file}.gltf`).then(({default: model}) => {
    -loader.load(model, (obj) => {
    -        const model = obj.scene;
    +import(`./../resources/models/model_${model_file}.gltf`).then(({ default: model: modelUrl }) => {
    +    loader.load(modelUrl, (obj) => {
    +        const modelScene = obj.scene;
             const scaleFactor = 15;
    -        model.scale.set(scaleFactor, scaleFactor, scaleFactor);
    -        modelWrapper.add(model);
    +        modelScene.scale.set(scaleFactor, scaleFactor, scaleFactor);
    +        modelWrapper.add(modelScene);

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

  • Clone this wiki locally