.pr_agent_accepted_suggestions
| PR 2536 (2026-01-25) |
New proposed code:
| PR 2514 (2026-01-06) |
| PR 2490 (2025-12-21) |
| PR 2489 (2025-12-20) |
| PR 2488 (2025-12-20) |
| PR 2485 (2025-12-18) |
| PR 2483 (2025-12-18) |
| PR 2480 (2025-12-15) |
| PR 2474 (2025-12-12) |
| PR 2473 (2025-12-12) |
| PR 2466 (2025-12-08) |
New proposed code:
| PR 2461 (2025-12-06) |
[learned best practice] Fix incorrect method call on string literal
✅ Fix incorrect method call on string literal
Remove the stray space to call String.prototype.repeat on the literal correctly, preventing a runtime TypeError.
js/transpiler/transpiler/tests/test_flight_axis_override.js [59]
-console.log('=' .repeat(60));
+console.log('='.repeat(60));Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Ensure UI elements and event handlers reference the correct identifiers and instances; avoid mismatches like calling methods on undefined or wrong instances.
| PR 2452 (2025-12-03) |
[possible issue] Correctly clear stale logic conditions
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 importance[1-10]: 9
__
Why: The suggestion correctly identifies a functional bug where using put() would append a condition instead of overwriting a specific slot, and proposes using set() which correctly clears the stale data.
| PR 2450 (2025-12-02) |
[possible issue] Use correct constant for parameter mapping
✅ 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 importance[1-10]: 6
__
Why: The suggestion correctly identifies a bug in a new test file where the test's log output would be misleading due to incorrect string formatting, and it provides the correct fix using the FLIGHT_PARAM_NAMES constant.
[learned best practice] Update test to current reality
✅ 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 importance[1-10]: 6
__
Why: Relevant best practice - Gate UI/async flows and validate dynamic resources before use; avoid stale documentation comments that contradict current code and cause confusion.
[learned best practice] Align test docs with API
✅ 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 importance[1-10]: 5
__
Why: Relevant best practice - Ensure event handlers/references target correct identifiers; remove misleading comments that no longer match implemented API to prevent mismatches and runtime confusion.
| PR 2447 (2025-12-01) |
[possible issue] Prevent runtime error after null check
✅ 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.
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 importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical bug where the application would crash with a TypeError if data is null, because the execution flow isn't stopped after the null check.
[possible issue] Avoid unintended behavior with null values
✅ 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 importance[1-10]: 7
__
Why: The suggestion correctly points out that not returning after the null check will cause self.boardAlignmentConfig.roll to be silently set to 0, which is unintended behavior and a potential bug.
| PR 2446 (2025-12-01) |
[possible issue] Add error handling for dynamic import
✅ 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.
import(`../locale/en/messages.json`).then(({default: messages}) => {
this.messages = messages;
+}).catch(error => {
+ console.error('Failed to load messages:', error);
});Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that error handling present in the original fetch call was removed during the refactoring to a dynamic import(), which could lead to unhandled promise rejections.
[possible issue] Add error handling for dynamic imports
✅ 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.
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 importance[1-10]: 8
__
Why: The suggestion correctly points out that error handling was removed from the dynamic import() calls for tab assets, which could cause unhandled promise rejections if a file fails to load.
| PR 2436 (2025-11-26) |
[learned best practice] Guard modal open call
✅ Guard modal open call
Ensure the modal instance is valid and not destroyed before calling open to prevent runtime errors if initialization failed or it was disposed.
function showMSPWarning() {
- if (mspWarningModal) {
+ if (mspWarningModal && typeof mspWarningModal.open === 'function') {
mspWarningModal.open();
}
}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Gate UI behavior with guards and validate DOM elements before use to avoid runtime errors.
| PR 2434 (2025-11-26) |
[possible issue] Fix silent error handling bug
✅ 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.
-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 importance[1-10]: 8
__
Why: The suggestion correctly identifies a promise anti-pattern in the new appendFile handler that would lead to silent failures and provides the correct fix.
| PR 2433 (2025-11-25) |
[possible issue] Guard and sanitize reboot polling loop
✅ 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.
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 importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical bug where the pollForRebootCompletion function uses ConnectionSerial.getDevices with a callback, but the function was changed in the PR to be async and no longer accepts one, which would break the serial reconnection logic.
[incremental [*]] Scope and harden response listener
✅ 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 importance[1-10]: 7
__
Why: The suggestion correctly identifies a potential race condition where data from a different connection could be processed, and proposes a robust fix by checking connectionId, which improves the reliability of the response handling logic.
[learned best practice] Guard and cleanup listener once
✅ 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 importance[1-10]: 6
__
Why: Relevant best practice - Avoid leaking listeners; always remove event handlers after use to prevent broken bindings and memory leaks.
[possible issue] Prevent multiple callback invocations
✅ 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 importance[1-10]: 6
__
Why: The suggestion correctly identifies a potential race condition in the new waitForResponse function where the callback could be invoked multiple times, and proposes a standard and effective fix using a flag to ensure it only runs once.
[learned best practice] Use proper async handling
✅ 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 importance[1-10]: 6
__
Why: Relevant best practice - Gate asynchronous flows with explicit guards and correct API usage; validate objects and await/handle promises before use.
[learned best practice] Fix listener array mismatch
✅ 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);
}Suggestion importance[1-10]: 5
__
Why: Relevant best practice - Ensure event handler registration uses correct identifiers/instances to avoid mismatches.
| PR 2416 (2025-10-19) |
[possible issue] Hide FrSky options for non-serial receivers
✅ 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.
$receiverMode.on('change', function () {
if ($(this).find("option:selected").text() == "SERIAL") {
$serialWrapper.show();
} else {
$serialWrapper.hide();
+ $("#frSkyOptions").hide();
}
});Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a UI logic bug where the frSkyOptions section remains visible after switching from a SERIAL receiver to a non-serial one, and provides the correct fix.
[possible issue] Fix incorrect help icon association
✅ 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.
-<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 importance[1-10]: 5
__
Why: The suggestion correctly identifies a copy-paste error in the for attribute of a help icon div, which should be associated with frSkyFuelUnit instead of frSkyPitchRoll.
| PR 2414 (2025-10-15) |
[possible issue] Correct the data unit mismatch
✅ 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.
-<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 importance[1-10]: 10
__
Why: The suggestion correctly identifies a critical bug where the UI data-unit (centideg-deg) was not updated to match the renamed setting (osd_pan_servo_pwm2decadeg), which would cause incorrect values to be sent to the firmware.
| PR 2285 (2024-12-15) |
[possible issue] Fix UDP event binding and window guard
✅ 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.
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 importance[1-10]: 10
__
Why: The suggestion correctly identifies two critical bugs in the new udp.connect function that would cause it to fail at runtime and prevent any UDP communication.
[possible issue] Fix undefined handler target
✅ 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 importance[1-10]: 9
__
Why: The suggestion correctly identifies that app.handle... functions are undefined when new Drag() is called, which will cause a runtime error.
[possible issue] Correct OpenLayers Feature import
✅ Correct OpenLayers Feature import
Change the import for Feature from ol/format/Feature to ol/Feature.js to correctly import the feature constructor.
-import Feature from 'ol/format/Feature';
+import Feature from 'ol/Feature.js';
...
privateScope.cursorFeature = new Feature({
geometry: privateScope.cursorPosition
});Suggestion importance[1-10]: 9
__
Why: This suggestion correctly identifies a critical bug where an incorrect module is imported, which would cause a runtime error when new Feature() is called.
[possible issue] Fix mismatched DOM id selector
✅ 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 importance[1-10]: 9
__
Why: The suggestion correctly identifies a bug where the jQuery selector armedicon does not match the HTML element ID armedIcon, which would prevent the armed status icon from updating.
[possible issue] Normalize and validate icon key mapping
✅ 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 importance[1-10]: 8
__
Why: The suggestion correctly points out that the dynamic import path cf_${nameSplit[0]} is inconsistent with how icons are accessed later, which will lead to broken image URLs.
[possible issue] Fix async HTML/icon load order
✅ Fix async HTML/icon load order
Refactor load_html to be an async function. Await the completion of loadIcons() before calling GUI.load to ensure icons are loaded before process_html is executed.
-function load_html() {
- import('./gps.html?raw').then(({default: html}) => GUI.load(html, Settings.processHtml(loadIcons().then(process_html))));
+async function load_html() {
+ const { default: html } = await import('./gps.html?raw');
+ await loadIcons();
+ GUI.load(html, Settings.processHtml(process_html));
}Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a critical race condition where process_html could execute before loadIcons completes, leading to runtime errors.
[possible issue] Prevent variable shadowing
✅ 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);Suggestion importance[1-10]: 4
__
Why: The suggestion correctly identifies variable shadowing which is poor practice, but the proposed improved_code contains a syntax error in the destructuring assignment.
[possible issue] Remove duplicate API property
✅ Remove duplicate API property
Remove the duplicate storeSet property from the object exposed via contextBridge to improve code quality and prevent potential future errors.
contextBridge.exposeInMainWorld('electronAPI', {
listSerialDevices: () => ipcRenderer.invoke('listSerialDevices'),
storeGet: (key, defaultValue) => ipcRenderer.sendSync('storeGet', key, defaultValue),
storeSet: (key, value) => ipcRenderer.send('storeSet', key, value),
- storeSet: (key, value) => ipcRenderer.send('storeSet', key, value),
storeDelete: (key) => ipcRenderer.send('storeDelete', key),
- ...
+ appGetPath: (name) => ipcRenderer.sendSync('appGetPath', name),
+ appGetVersion: () => ipcRenderer.sendSync('appGetVersion'),
+ appGetLocale: () => ipcRenderer.sendSync('appGetLocale'),
+ showOpenDialog: (options) => ipcRenderer.invoke('dialog.showOpenDialog', options),
+ showSaveDialog: (options) => ipcRenderer.invoke('dialog.showSaveDialog', options),
+ alertDialog: (message) => ipcRenderer.sendSync('dialog.alert', message),
+ confirmDialog: (message) => ipcRenderer.sendSync('dialog.confirm', message),
+ tcpConnect: (host, port) => ipcRenderer.invoke('tcpConnect', host, port),
+ tcpClose: () => ipcRenderer.send('tcpClose'),
+ tcpSend: (data) => ipcRenderer.invoke('tcpSend', data),
+ onTcpError: (callback) => ipcRenderer.on('tcpError', (_event, error) => callback(error)),
+ onTcpData: (callback) => ipcRenderer.on('tcpData', (_event, data) => callback(data)),
+ onTcpEnd: (callback) => ipcRenderer.on('tcpEnd', (_event) => callback()),
+ serialConnect: (path, options) => ipcRenderer.invoke('serialConnect', path, options),
+ serialClose: () => ipcRenderer.invoke('serialClose'),
+ serialSend: (data) => ipcRenderer.invoke('serialSend', data),
+ onSerialError: (callback) => ipcRenderer.on('serialError', (_event, error) => callback(error)),
+ onSerialData: (callback) => ipcRenderer.on('serialData', (_event, data) => callback(data)),
+ onSerialClose: (callback) => ipcRenderer.on('serialClose', (_event) => callback()),
+ udpConnect: (ip, port) => ipcRenderer.invoke('udpConnect', ip, port),
+ udpClose: () => ipcRenderer.invoke('udpClose'),
+ udpSend: (data) => ipcRenderer.invoke('udpSend', data),
+ onUdpError: (callback) => ipcRenderer.on('udpError', (_event, error) => callback(error)),
+ onUdpMessage: (callback) => ipcRenderer.on('udpMessage', (_event, data) => callback(data)),
+ writeFile: (filename, data) => ipcRenderer.invoke('writeFile', filename, data),
+ readFile: (filename, encoding = 'utf8') => ipcRenderer.invoke('readFile', filename, encoding),
+ rm: (path) => ipcRenderer.invoke('rm', path),
+ chmod: (path, mode) => ipcRenderer.invoke('chmod', path, mode),
+ startChildProcess: (command, args, opts) => ipcRenderer.send('startChildProcess', command, args, opts),
+ killChildProcess: () => ipcRenderer.send('killChildProcess'),
+ onChildProcessStdout: (callback) => ipcRenderer.on('onChildProcessStdout', (_event, data) => callback(data)),
+ onChildProcessStderr: (callback) => ipcRenderer.on('onChildProcessStderr', (_event, data) => callback(data)),
+ onChildProcessError: (callback) => ipcRenderer.on('onChildProcessError', (_event, error) => callback(error)),
});Suggestion importance[1-10]: 3
__
Why: The suggestion correctly identifies a duplicate key storeSet which, while not causing a functional bug here, is a code quality issue that should be fixed.
[security] Sanitize user-provided profile name
✅ Sanitize user-provided profile name
Sanitize the user-provided profile name before inserting it into the DOM to prevent a potential cross-site scripting (XSS) vulnerability.
profileNewBtn_e.on('click', function () {
- smalltalk.prompt(i18n.getMessage('sitlNewProfile'), i18n.getMessage('sitlEnterName')).then(name => {
- if (!name)
- return;
+ smalltalk.prompt(i18n.getMessage('sitlNewProfile'), i18n.getMessage('sitlEnterName')).then(rawName => {
+ const name = (rawName || '').trim();
+ if (!name) return;
- if (profiles.find(e => { return e.name == name })) {
+ if (profiles.find(e => e.name === name)) {
dialog.alert(i18n.getMessage('sitlProfileExists'));
return;
}
- var eerpromName = name.replace(/[^a-z0-9]/gi, '_').toLowerCase() + ".bin";
- var profile = {
- name: name,
- sim: "RealFlight",
+ const safeText = $('<div>').text(name).html(); // escape
+ const eepromName = name.replace(/[^a-z0-9]/gi, '_').toLowerCase() + '.bin';
+ const profile = {
+ name,
+ sim: 'RealFlight',
isStdProfile: false,
simEnabled: false,
- eepromFileName: eerpromName,
+ eepromFileName: eepromName,
port: 49001,
- ip: "127.0.0.1",
+ ip: '127.0.0.1',
useImu: false,
- channelMap: [ 1, 13, 14, 0, 0, 0, 0, 0, 0, 0, 0, 0],
+ channelMap: [1, 13, 14, 0, 0, 0, 0, 0, 0, 0, 0, 0],
useSerialReceiver: true,
serialPort: serialPorts_e.val(),
serialUart: 3,
- serialProtocol: "SBus",
+ serialProtocol: 'SBus',
baudRate: false,
stopBits: false,
parity: false
- }
+ };
profiles.push(profile);
- profiles_e.append(`<option value="${name}">${name}</option>`)
+ profiles_e.append(`<option value="${safeText}">${safeText}</option>`);
profiles_e.val(name);
updateCurrentProfile();
saveProfiles();
- }).catch(() => {} );
+ }).catch(() => {});
});Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a cross-site scripting (XSS) vulnerability by using unescaped user input to construct HTML, which is a critical security issue.
The latest version of INAV Configurator can be found in the Releases section of this repository.
For more information about the features of the INAV firmware. Check out the firmware Wiki or technical documents. For supported flight controllers, take a look here and here.