.pr_agent_accepted_suggestions
| PR 2734 (2026-09-03) |
[correctness] Unknown mode ranges erased
Unknown mode ranges erased
Filtering unknown permanent IDs out of `AUX_CONFIG_IDS` prevents their existing mode ranges from being rendered; saving then rebuilds `FC.MODE_RANGES` solely from rendered rows and replaces omitted entries with disabled ranges, silently deleting assignments created by newer firmware.Unknown mode IDs are removed from the mode list, so existing ranges associated with those IDs are omitted from the UI and erased when the user saves any Modes-tab change.
Keep the name and ID arrays aligned without discarding firmware-provided IDs. A fallback label such as Unknown mode (ID 69) allows the existing assignment to remain visible and preserved while also retaining the original BOXIDS position used by mode-status bits.
- js/fc.js[204-219]
- tabs/auxiliary.js[239-271]
- tabs/auxiliary.js[301-347]
- tests/fc-generate-aux-config.test.mjs[137-191]
| PR 2729 (2026-08-31) |
[correctness] WASM transport never selected
WASM transport never selected
The PR adds `ConnectionType.serialEXT` and `ConnectionExt`, but the serial backend still maps every SITL selection to `ConnectionType.TCP`, so no code ever requests the new type. In the browser, selecting SITL therefore constructs `ConnectionTcp` and fails when it calls Electron-only TCP listener APIs instead of connecting the configurator to the running WASM instance.The newly added WASM serial transport is unreachable because browser SITL selections are still routed through TCP.
ConnectionExt is registered under ConnectionType.serialEXT, but GUI.updateManualPortVisibility() chooses TCP for all SITL options and the connect path supplies a TCP address.
- js/connection/connection.js[5-10]
- js/connection/connectionFactory.js[22-27]
- js/serial_backend.js[131-151]
- js/serial_backend.js[212-226]
[correctness] Desktop development flashing broken
Desktop development flashing broken
Development-release descriptors now always use the GitHub Pages firmware mirror, including in Electron, whereas stable releases correctly use `asset.browser_download_url` on desktop. Development/nightly firmware loading therefore fails on desktop whenever the asset is not present on the browser-only mirror.The development firmware path redirects desktop downloads to the browser-only GitHub Pages mirror.
Apply the same browser-build conditional already used by the stable-release descriptor, retaining asset.browser_download_url for Electron.
- tabs/firmware_flasher.js[265-271]
- tabs/firmware_flasher.js[324-330]
[correctness] Logs split across downloads
Logs split across downloads
`appendToStream()` flushes and deletes a stream once it reaches 1 MB, so later appends to the same filename begin a new browser download rather than extending the existing log. Long CSV or onboard logs are consequently delivered as multiple partial files instead of the single append-only file promised by `appendFile`.Browser appendFile creates multiple downloads for one logical log after the 1 MB threshold.
A blob download cannot subsequently be appended to; either retain all chunks until recording stops/idle, or introduce an explicit close/finalize contract or writable file handle.
- js/browser/platform.js[53-86]
- tabs/logging.js[70-89]
- tabs/onboard_logging.js[380-400]
[reliability] File cancellation never resolves
File cancellation never resolves
`selectFile()` resolves only from the input's `change` handler and registers no cancellation path. Canceling the browser picker leaves every caller awaiting `showOpenDialog()` permanently pending, blocking restore/import workflows.Browser file-open promises remain pending when the picker is canceled.
Handle the input's cancellation event and resolve the same { canceled, filePaths } contract as the desktop dialog, with cleanup that prevents double resolution.
- js/browser/platform.js[17-35]
- js/backup_restore.js[351-371]
- js/dialog.js[1-7]
[correctness] SITL failure reports success
SITL failure reports success
When WASM initialization fails, `init()` calls the callback and rethrows, then `start()` catches the same error and calls the callback again. The tab callback also continues after its error branch and logs `SITL started with args: undefined`, producing duplicate errors and a false success message.A WASM initialization failure invokes the startup callback twice and is followed by a success log.
Choose either callback or thrown-promise error propagation, not both, and return from the tab callback after handling an error.
- js/web/SITL-Webassembly.js[224-231]
- js/web/SITL-Webassembly.js[279-307]
- tabs/sitl.js[303-308]
[performance] Stopped SITL leaks polling
Stopped SITL leaks polling
`ConnectionExt.disconnectImplementation()` returns immediately whenever WASM is no longer running, before clearing `_messageCheckInterval`. A crash or stop therefore leaves a permanent 10 ms interval behind, and reconnecting can accumulate additional polling intervals.The WASM serial polling interval survives crashes and stops.
Timer cleanup must happen unconditionally before checking whether the runtime is available; only the C disconnect call should depend on isRunning().
- js/connection/connectionExt.js[97-110]
- js/connection/connectionExt.js[141-170]
- js/web/SITL-Webassembly.js[161-183]
[reliability] Serial IDs shift between polls
Serial IDs shift between polls
Web Serial IDs include the current `getPorts()` array index, and `getDevices()` clears and rebuilds the ID map on every poll. With multiple granted ports, removing an earlier entry renumbers the remaining devices, causing port tracking to treat an unchanged connected device as removed and disconnect it or restore the wrong saved selection.Web Serial device identities change when enumeration order changes.
Use stable identity assignment tied to each SerialPort object, rather than incorporating the current array index into every poll's ID.
- js/connection/connectionWebSerial.js[34-60]
- js/port_handler.js[30-61]
- js/port_handler.js[80-93]
| PR 2728 (2026-08-31) |
[correctness] Terrain element initialization race
Terrain element initialization race
`HARDWARE.update()` invokes its completion callback without waiting for `terrain_enabled`, so OSD item generation can evaluate `OSD_TERRAIN_AGL.enabled()` while `useTerrain` is still false. The dedicated terrain element is then omitted from the tab until a later reinitialization.The OSD initialization callback can run before terrain_enabled has populated HARDWARE.capabilities.useTerrain, hiding the terrain OSD element.
Include the terrain setting request in HARDWARE.update()'s completion chain, with a safe fallback when the setting cannot be read.
- tabs/osd.js[3639-3643]
- tabs/osd.js[3661-3663]
| PR 2727 (2026-08-31) |
[correctness] Waypoint weather never fetched
Waypoint weather never fetched
When a mission waypoint exists without a GPS fix, initialization only centers the map and never calls `fetchConditionsInfo`; the approximate-location request also refuses to run when a waypoint exists. Consequently, an offline mission with waypoints never displays the advertised weather conditions.Waypoint-only offline missions are centered correctly but never request weather data.
The first waypoint must expose both its projected map coordinate and latitude/longitude so it can be used as a weather source. Preserve the intended waypoint → GPS → approximate centering priority.
- tabs/mission_control.js[512-527]
- tabs/mission_control.js[633-636]
- tabs/mission_control.js[3371-3380]
[reliability] Weather failures never retry
Weather failures never retry
`fetchConditionsInfo` records a source as attempted before the request succeeds and never clears it on failure. A temporary network or API error therefore leaves weather unavailable for the rest of the Mission Control tab session, because later GPS updates are rejected by the attempted-source guard.A failed weather request permanently blocks all later requests for the same source during the current tab session.
GPS updates run frequently, so retries should use separate in-flight/success state plus a bounded delay or cooldown rather than immediately retrying on every update.
- tabs/mission_control.js[536-584]
- tabs/mission_control.js[997-1002]
[correctness] Tested key differs from stored
Tested key differs from stored
`testGoogleApiKey` trims the entered key, but the change handler stores and publishes the untrimmed value. A pasted key with surrounding whitespace can pass the test and then fail all Mission Control location, weather, and geocoding requests.API-key testing normalizes whitespace, but persisted and runtime API-key usage retains it.
Use one normalized value for the input, store, global settings, tests, and runtime requests.
- js/configurator_main.js[89-104]
- js/configurator_main.js[632-635]
- tabs/mission_control.js[544-547]
- tabs/mission_control.js[640-642]
- tabs/mission_control.js[4605-4607]
| PR 2726 (2026-08-30) |
[correctness] Wrong MZTC payload layout
Wrong MZTC payload layout
The MZTC config parser and serializer prepend `enabled`, `port`, and `baudrate`, requiring 15 bytes even though `MSP2_MZTC_CONFIG` and `MSP2_SET_MZTC_CONFIG` are documented as 12-byte payloads containing the twelve camera settings. A valid firmware response is therefore rejected as too short, while `saveMZTCConfig()` sends three extra bytes and shifts every actual field, causing the firmware to reject or misinterpret the request.The MZTC configuration parser and serializer implement a 15-byte payload by including enabled, port, and baudrate, but the coordinated firmware protocol defines a 12-byte payload. This rejects valid reads and produces invalid writes.
Port assignment and baud rate are managed through the serial-port configuration, while the Configuration tab exposes exactly twelve MZTC settings. Keep parsing and serialization field order identical to the firmware protocol.
- js/fc.js[111-130]
- js/msp/MSPHelper.js[1729-1753]
- js/msp/MSPHelper.js[2381-2400]
[correctness] Numeric limits are bypassable
Numeric limits are bypassable
The new numeric MZTC controls rely on dynamically assigned HTML `min`/`max` attributes, but save-time processing clamps only against `data-default-min` and `data-default-max`, which are unset for ordinary multiplier-1 settings. Manually entered out-of-range values therefore reach `encodeSetting()` and are serialized without range validation, causing wrapped values for byte types or rejected/incorrect camera settings.The new numeric MZTC fields can submit values outside the min/max reported by firmware because processInput() does not use the normal input bounds when data-default-min and data-default-max are absent.
configureInputs() already receives authoritative min/max metadata. Preserve those bounds as save-time defaults for every numeric setting, or validate directly against setting-info before encoding; apply the correction to all seven new numeric MZTC fields.
- tabs/configuration.html[431-474]
- js/settings.js[180-193]
- js/settings.js[604-660]
- js/msp/MSPHelper.js[3567-3615]
| PR 2716 (2026-08-24) |
[correctness] Blocked writes continue saves
Blocked writes continue saves
When `send_message()` refuses a write, it invokes the existing MSP completion callback with `false`, but callbacks such as failsafe's `savePhaseTwo` ignore arguments and continue down the success path. A blocked failsafe write therefore proceeds to EEPROM save, logs `eepromSaved`, and reboots, presenting the save flow as completed despite the refused operation.Blocked MSP writes call the normal completion callback with false. Existing callbacks generally treat invocation itself as success, so they continue save chains, persist EEPROM, log success, and reboot even though the requested write was refused.
The callback API historically signals successful response completion by invocation rather than by a checked boolean argument. Introduce an explicit failure path that cannot accidentally execute success-only continuations, while ensuring callers do not hang.
- js/msp.js[383-400]
- tabs/failsafe.js[119-144]
[correctness] EZ Tune write bypasses block
EZ Tune write bypasses block
The new write classifier only matches `SET_`, so the real write command `MSP2_INAV_EZ_TUNE_SET` is omitted from `WRITE_CODES`. After an unreadable `MSP2_INAV_EZ_TUNE` response, saving EZ Tune remains allowed and can write the partially parsed values that this PR is intended to block.MSP2_INAV_EZ_TUNE_SET is an active write command, but its suffix-form name does not match the new /SET_/ classifier. It therefore bypasses parse-failure write protection.
This command must also be paired with its source response MSP2_INAV_EZ_TUNE; merely broadening the write regex will otherwise leave it conservatively associated with an unrelated parse failure rather than its actual source.
- js/msp.js[9-49]
- js/msp/MSPHelper.js[3635-3641]
- js/msp/MSPCodes.js[234-235]
| PR 2708 (2026-08-20) |
[correctness] Failed tiles report success
Failed tiles report success
The new packer self-check can throw—for example, any block spanning more than the 10-bit range of 2,046 m—but the per-tile conversion handler only logs that failure and continues. If another tile succeeds, the incomplete set is written or exported and the completion UI reports success without identifying the omitted tile.The version-50 packer can reject a terrain block, but its exception is swallowed by the per-tile conversion loop. This permits an incomplete terrain selection to be written or exported and reported as successful.
A 10-bit offset in 2 m steps represents at most 2,046 m above heightBase. Larger spans are clamped and subsequently rejected by the decode-back check. Conversion failures must be tracked and surfaced rather than omitted from the final result.
- tabs/map_generator.js[507-533]
- tabs/map_generator.js[2455-2467]
- tabs/map_generator.js[2475-2489]
- tabs/map_generator.js[2573-2580]
| PR 2703 (2026-08-09) |
[correctness] Invalid marker coordinate guard
Invalid marker coordinate guard
repaintLine4Waypoints() may construct RTH/heading marker geometries from oldPos even when oldPos is the string 'undefined', causing OpenLayers Point/Feature construction to fail and break map rendering for affected missions.repaintLine4Waypoints() now creates additional OpenLayers Point features for attached RTH and SET_HEAD actions. The new code checks oldPos !== undefined, but in the same function oldPos is sometimes assigned the literal string 'undefined' when an element has endMission == 0xA5. Since 'undefined' !== undefined is true, the guard is ineffective and the code can call new Point(oldPos) with a non-coordinate value, which can throw and interrupt mission rendering.
-
oldPosis used as the last waypoint coordinate for drawing and markers. - The function explicitly assigns
oldPos = 'undefined'in some cases. - Attached actions include
RTHandSET_HEAD, so the new marker paths are reachable.
- tabs/mission_control.js[1944-1966]
- tabs/mission_control.js[1992-2010]
- Replace the current guards with strict coordinate validation, e.g.:
if (Array.isArray(oldPos) && oldPos.length === 2 && Number.isFinite(oldPos[0]) && Number.isFinite(oldPos[1])) { ... }- (Optional but recommended) stop using the string sentinel
'undefined'and useundefined/nullconsistently, updating any dependent checks accordingly.
[correctness] Grid capacity text mismatch
Grid capacity text mismatch
updateGridPreview() interpolates missionGridWaypointCount with mission.getMaxWaypoints() as “remaining capacity”, so the displayed value does not match what the message string claims it represents.The new English i18n string missionGridWaypointCount says $2 is “remaining capacity”, but the preview code passes mission.getMaxWaypoints() (the maximum allowed waypoints) as $2. This yields misleading UI like “remaining capacity: 120” when it’s actually the limit, not remaining after the proposed grid.
-
mission.getMaxWaypoints()returns the configured maximum (not remaining slots). - The preview computes
totalCountalready, so remaining can be computed if that’s what the UI intends.
- tabs/mission_control.js[4371-4374]
- locale/en/messages.json[6825-6827]
- js/waypointCollection.js[21-27]
- If
$2should be remaining after generating this grid: passmaxWp - totalCount(clamped at >= 0). - If
$2should be the absolute limit: change the message text to “maximum capacity” / “limit” instead of “remaining capacity”.
| PR 2699 (2026-08-03) |
[correctness] 403 misclassified as missing
403 misclassified as missing
copernicusTileMissing() treats HTTP 403 as “tile missing” and openCopernicusImage() then marks it as notFound, so forbidden/permission failures are silently converted into “no data” and neighbour edges remain at 0m instead of surfacing as a Copernicus failure. This can yield incorrect coastal elevation grids and hides real access problems from the user.copernicusTileMissing() currently returns true for HTTP 403, which causes openCopernicusImage() to treat a Forbidden response as a normal “tile does not exist” condition (notFound). This bypasses the intended “Copernicus server unavailable” handling and can silently leave neighbour-edge samples at sea level.
The UI explicitly intends to surface Copernicus network/server problems so the user can choose SRTM fallback rather than switching silently.
- tabs/map_generator.js[610-619]
- tabs/map_generator.js[646-661]
- Make
copernicusTileMissing()only treat 404 as missing. - For 403 (and other non-OK statuses), return
false(or throw) so the originalfromUrl()error propagates and triggers the fallback prompt. - (Optional) If you keep any non-404 as “missing” for a known provider quirk, return the actual status (not just boolean) and preserve it in the thrown error message instead of hardcoding
'HTTP 404'.
[correctness] 403 misclassified as missing
403 misclassified as missing
copernicusTileMissing() treats HTTP 403 as “tile missing” and openCopernicusImage() then marks it as notFound, so forbidden/permission failures are silently converted into “no data” and neighbour edges remain at 0m instead of surfacing as a Copernicus failure. This can yield incorrect coastal elevation grids and hides real access problems from the user.copernicusTileMissing() currently returns true for HTTP 403, which causes openCopernicusImage() to treat a Forbidden response as a normal “tile does not exist” condition (notFound). This bypasses the intended “Copernicus server unavailable” handling and can silently leave neighbour-edge samples at sea level.
The UI explicitly intends to surface Copernicus network/server problems so the user can choose SRTM fallback rather than switching silently.
- tabs/map_generator.js[610-619]
- tabs/map_generator.js[646-661]
- Make
copernicusTileMissing()only treat 404 as missing. - For 403 (and other non-OK statuses), return
false(or throw) so the originalfromUrl()error propagates and triggers the fallback prompt. - (Optional) If you keep any non-404 as “missing” for a known provider quirk, return the actual status (not just boolean) and preserve it in the thrown error message instead of hardcoding
'HTTP 404'.
| PR 2672 (2026-07-06) |
[correctness] Early save disables DNA
Early save disables DNA
The DNA checkbox is exposed and the save handler is enabled before its third, sequential setting read completes, so it remains at the HTML default `false` during that window. Saving then writes `0` and silently disables an already-enabled DNA server before its actual value arrives.The DNA checkbox defaults to unchecked while its asynchronous setting read runs, but Save is already active and can persist that temporary value as 0.
The DNA read is third in a sequential promise chain. Ensure users cannot save until all supported configuration values have loaded, while still allowing older firmware without the setting to finish initialization.
- tabs/dronecan.js[139-154]
- tabs/dronecan.html[22-25]
[reliability] Missing setting rejects initialization
Missing setting rejects initialization
On older firmware, an unsupported/empty response for `dronecan_use_dna_server` makes `getSetting` throw while decoding the response, and the newly added initialization chain has no rejection handler. This produces an unhandled rejection instead of the intended graceful compatibility no-op.Loading dronecan_use_dna_server can reject when older firmware returns no setting metadata, but DroneCAN initialization does not handle that rejection.
Treat an unavailable DNA setting as unsupported, leave the checkbox disabled or unchecked, and complete tab initialization without an unhandled promise rejection. Do not prevent the existing bitrate and node-ID values from loading.
- tabs/dronecan.js[139-154]
- js/msp/MSPHelper.js[3499-3532]
| PR 2671 (2026-07-04) |
[correctness] Node ID not validated
Node ID not validated
`dronecanTab.saveConfig()` does not validate `nodeId` for NaN/out-of-range values; because `mspHelper.setSetting()` swallows encoding errors and still calls the callback, the UI can proceed to save/reboot with only a partial configuration applied.saveConfig() parses the node ID but does not enforce integer bounds or handle NaN. If the value is invalid, mspHelper.setSetting() logs and resolves anyway, and the flow continues into saveToEeprom() + reboot, leaving the user with an unexpected reboot and partially-applied settings.
The HTML input has min/max but users can still clear it or enter invalid values; programmatic validation is required.
- Validate
nodeIdis an integer in [1, 127]; show an error and return without reboot when invalid. - Validate bitrate is one of the supported options.
- Consider awaiting both setSetting calls and failing the overall save if either fails.
- tabs/dronecan.js[451-456]
- tabs/dronecan.html[19-22]
- js/msp/MSPHelper.js[3652-3658]
[reliability] Async request may stay stale
Async request may stay stale
`MSP2_INAV_DRONECAN_ASYNC_REQUEST` parsing only updates `FC.DRONECAN_ASYNC_REQUEST` when `byteLength >= 2`; if the firmware ever replies with only a status byte (or zero-length on error), the old status/seq remains and `dronecanAsyncPoll()` can act on stale data.The async-request response parser ignores short payloads, leaving FC.DRONECAN_ASYNC_REQUEST unchanged. Callers immediately read status/seq from this object to decide whether to poll and which seq to expect, so stale values can cause incorrect behavior.
Even if current firmware always returns 2 bytes, making this robust prevents hard-to-debug failures if the wire format differs across versions or error paths.
- Always reset
FC.DRONECAN_ASYNC_REQUESTat the start of the case. - If
byteLength >= 1, at least storestatus; only storeseqwhen present. - Update
dronecanAsyncPoll()to handle missing seq (treat as error/not-ready).
- js/msp/MSPHelper.js[1591-1598]
- tabs/dronecan.js[83-90]
| PR 2569 (2026-02-18) |
[reliability] `ADSB_VEHICLE_TYPE` lookup unguarded
`ADSB_VEHICLE_TYPE` lookup unguarded
The new tooltip content dereferences `ADSB_VEHICLE_TYPE[feature.get('data').emitterType].name` without validating that the lookup exists. If `emitterType` is missing or out of range, this can throw at runtime and break the GPS tab UI.The code reads .name from ADSB_VEHICLE_TYPE[emitterType] without checking that the entry exists.
emitterType comes from incoming MSP data and may be missing/out-of-range depending on firmware/protocol versions or corrupt data.
- tabs/gps.js[369-369]
[correctness] Stale ADSB warning displayed
Stale ADSB warning displayed
The warning ICAO is only fetched when vehiclesCount > 0, but the UI shows the warning rows based on vehiclePacketCount and doesn’t clear FC.ADSB_WARNING_ICAO when vehicles disappear. This can keep showing an old warning ICAO/type even when there are currently zero vehicles.ADSB warning state can become stale because warning is only requested when vehiclesCount > 0, while the UI uses vehiclePacketCount > 0 to decide whether to show warning rows.
vehiclePacketCount is parsed from the vehicle-list message and is not tied to the current vehicle count; it may remain positive even when vehiclesCount becomes 0.
- tabs/gps.js[392-402]
- tabs/gps.js[485-499]
- Change the UI show/hide condition to use
FC.ADSB_VEHICLES.vehiclesCount > 0(orFC.ADSB_VEHICLES.vehicles.length > 0). - When
vehiclesCount == 0, explicitly resetFC.ADSB_WARNING_ICAO.icao = 0andisAlert = 0before rendering. - Optionally: always request
MSP2_ADSB_WARNING_VEHICLE_ICAOand treat 0 ICAO as “none”, but don’t block UI if unsupported.
| 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.