Joystick: Restore MANUAL_CONTROL axes saved by a newer version by rafaellehmkuhl · Pull Request #3003 · bluerobotics/cockpit

rafaellehmkuhl · 2026-09-03T02:12:44Z

Summary

1.19 rewrote joystick MANUAL_CONTROL axes onto data-lake ids (inputs/mavlink/axis-x, …) in the vehicle-synced mapping. This line still sends those axes as MAVLink, so a stick saved on 1.19 comes back as zero.
Resolve those ids when building MANUAL_CONTROL. Do not convert-and-write the mapping back on read.
The existing duplicate-axis guard now compares the resolved MANUAL_CONTROL axis, so remapping a stick after a downgrade unmaps a leftover 1.19 binding (and warns) instead of leaving both in the vehicle-synced key.
The joystick settings page labels those leftover bindings as MAVLink Axis X…T in the axis dropdown (display-only).
min/max are kept. Unrelated data-lake axes (camera zoom, …) are left alone. Going back to 1.19 remigrates forward.

To be cherry-picked onto #2997 so 1.18.3 ships this. Same class of downgrade hole as #3002, different key.

Test plan

On 1.19, confirm a gamepad moves the vehicle, then install this build (or 1.18.3 with this commit). Stick should move the vehicle without re-importing the default mapping.
Connect a fresh 1.18 topside to a vehicle whose mapping was last saved by 1.19. After sync, stick should work.
After that downgrade, open joystick settings. Each 1.19 axis should read MAVLink Axis X (etc.) in the dropdown, in line with Min/Max.
After that downgrade, re-assign one MANUAL_CONTROL axis to a different stick. The leftover should unmap with the existing duplicate warning.
Camera-zoom (or another data-lake axis) still responds. Custom min/max on a MANUAL_CONTROL axis are unchanged.
yarn vitest --run src/tests/libs/joystick/manual-control-axis-id.test.ts

github-actions · 2026-09-03T02:33:56Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 1)

2 open findings: 2 major (2.1, 3.1).

When a joystick mapping arrives that was last saved by a newer version of Cockpit, this change rewrites the six stick axes that steer the vehicle back into the form this older version understands, so the stick works again. It does that at startup and again every time the vehicle pushes a new copy of the mapping down. The conversion itself is exact and leaves everything else in the mapping untouched. The problem is what happens afterwards: the rewritten mapping is saved back to the vehicle, where every other ground station connected to that vehicle picks it up.

What still needs attention

#	Problem	What it means	Severity	Status
2.1	Downgraded mapping is written back to the vehicle	A ground station on the older version quietly overwrites the joystick settings stored on the vehicle, so a colleague running the newer version on the same boat loses stick control until they restart Cockpit.	major	❌
3.1	Three empty documentation blocks in the new file	Placeholder comment blocks were added with nothing written in them, which the project's own rules forbid; the shape that made them necessary also re-types a table the code already has.	major	❌

Change map — what was established before judging

Checkout caveat, stated first because it limits everything below. This PR targets v1.18-dev, but the checkout I am reviewing from is master at cf83753, and git branch -a shows only master and origin/master — v1.18-dev is not fetched. Every base-ref citation below is therefore to the 1.19 line, not to this PR's actual base. That is unusually useful in one direction (the 1.19 migration being inverted is right here, so the inverse table can be checked exactly) and blind in the other (I cannot read what v1.18-dev's MavlinkManualControlManager does with axesCorrespondencies, so the "1.18 sends zero" half of the symptom rests on the diff's own context lines rather than on code I read).

Claims

Symptom — "1.19 rewrote MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified. migrateMavlinkManualControlAxes at src/libs/joystick/protocols/mavlink-manual-control.ts:696-713 does exactly that rewrite, and performJoystickMappingMigrations at src/libs/joystick/protocols.ts:29 runs it. The 1.18 consumer half is unverifiable here; the only evidence is the diff's own context at src/libs/joystick/protocols.ts, which shows the base importing mavlinkManualControlAxes and running migrateMavlinkManualControlButtons alone.
Cause — the six ids and their MAVLink counterparts. Verified exactly. The new table (data-lake-axis-migration.ts:22-27) is a precise inverse of mavlinkAxisToDataLakeMap at mavlink-manual-control.ts:685-692: same six ids inputs/mavlink/axis-{x,y,z,r,s,t}, same names Axis X…Axis T, same targets axis_x…axis_t (MAVLinkAxisFunction, mavlink-manual-control.ts:18-25). No axis is missing and none is invented.
Mechanism — "convert those ids back … on read, and re-run when BlueOS sync replaces the mapping." Re-run verified, "on read" contradicted. The re-run works: settingsSyncer.ts:142 assigns refedValue.value = newValue on a vehicle update, which changes the ref's identity and so fires the new non-deep watch. But nothing is converted "on read" — src/stores/controller.ts:94 assigns the converted mapping back into protocolMapping.value, settingsSyncer.ts:109-125 persists it after the debounce, and settings-management.ts:273-281 pushes it to the vehicle. That gap is finding 2.1.
"min/max are kept." Verified. Only .action is replaced (data-lake-axis-migration.ts:44); the sibling min/max fields of JoystickAxisActionCorrespondency (src/types/joystick.ts:101-119) are never touched.
"Unrelated data-lake axes (camera zoom, …) are left alone." Verified. data-lake-axis-migration.ts:43 returns when the lookup misses.
"Going back to 1.19 remigrates forward." Verified, but only at boot. On master the forward migration runs once, synchronously, at store setup (src/stores/controller.ts:87) — there is no watcher re-running it. A 1.19 instance that is already running when the rewritten mapping arrives does not re-migrate. This is what makes 2.1 bite.

Failure site. The misbehaving code is on v1.18-dev and cannot be in this diff or this checkout. On the side I can read, the origin of the value that 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as intended for its own branch. There is no "smaller fix at the real site" to name: the real site is a different release line, and inverting on read is the correct shape for a backport. No finding.

Entry points

Function	Reached from	Frequency
`migrateDataLakeAxesToMavlinkManualControl` (`data-lake-axis-migration.ts:35`)	only `performJoystickMappingMigrations`	`per user action`
`performJoystickMappingMigrations` (`protocols.ts:26`)	only the new watch callback below	`per user action`
watch callback on `protocolMapping` (`src/stores/controller.ts:86-96`)	Vue watcher, `immediate: true`. Fired at store setup; by the settings-sync listener at `settingsSyncer.ts:142` when the vehicle pushes this key; by the legacy self-heal and its listener (`controller.ts:75`, `controller.ts:83`); by `importFunctionsMapping` (`controller.ts:429`); by the vehicle-defaults joystick import (`useVehicleDefaultsJoystickImport.ts:80`) and auto-import (`vehicleDefaultsAutoImport.ts:61`)	`per user action`

None of these is a hot path. The watcher is not deep, so it fires on whole-object replacement only, not on the deep mutations that the config UI, the duplicate-axis watcher at controller.ts:330 and the modifier-key housekeeping interval at controller.ts:358 perform. Cost per fire is four JSON.stringify passes over a ~192-entry structure — negligible at this frequency, and it does not reach dataLake:setVariable or mavlink:onIncomingMessage.

Invariants. The change relies on: the persisted mapping never carries a MANUAL_CONTROL axis as a DataLakeVariable action. Every site that can write a whole mapping into protocolMapping, from rg 'protocolMapping':

settingsSyncer.ts:142 (vehicle sync) — covered.
controller.ts:75 and controller.ts:83 (legacy self-heal) — covered.
controller.ts:429 (importFunctionsMapping, user-supplied JSON file) — covered, and this one the base's boot-time-only migration did not cover, so the watch is a genuine improvement there.
useVehicleDefaultsJoystickImport.ts:80 and vehicleDefaultsAutoImport.ts:61 (vehicle default profile import) — covered.
ConfigurationJoystickView.vue:983-985, the duplicate-axis watcher at controller.ts:330 and the interval at controller.ts:358 mutate .action in place without replacing the ref — not covered, and correctly so: a non-deep watch is what keeps a deliberate per-axis choice by the user from being silently reverted.

The chokepoint is well chosen: every whole-mapping replacement funnels through the ref assignment, and the enumeration above is exhaustive. 2.1 is not about coverage — it is about what the callback does after converting.

2. Persistence & User Data — inventory, 1 finding

Inventory

Key	Backend	What happened
`cockpit-joystick-functions-mapping-v1` (`controller.ts:49`, `controller.ts:57`)	vehicle-synced — `useBlueOsStorage`, shared by every topside computer and operator of that vehicle	Not added, not removed, not reshaped. The PR adds a new automatic in-place transformation of its contents, and changes when that transformation runs from once at boot to once per whole-mapping replacement.

No other persisted key is touched. The key is correctly cockpit--prefixed, the stored shape is unchanged (JoystickProtocolActionsMapping, src/types/joystick.ts:143-167), and nothing in it duplicates its own key. min/max, button correspondencies, name and hash all survive the transformation untouched. Nothing machine-specific is being synced. The transformation itself is individually idempotent — a second pass finds no DataLakeVariable action among the six ids and returns an identical structure — and the JSON.stringify guard at controller.ts:93 correctly stops the write and the watcher recursion when nothing changed, so there is no write amplification and no loop within a single instance.

2.1 — The converted mapping is written back to the vehicle-synced key, not kept local — major

Consequence: a ground station still on 1.18 quietly overwrites the joystick settings stored on the vehicle, so a colleague running 1.19 on the same boat loses stick control and has no way to know why until they restart Cockpit.

The PR body describes this as converting "on read". It is not a read-side conversion. src/stores/controller.ts:94 assigns the result back into protocolMapping.value; settingsSyncer.ts:86-128 sees the changed serialization and, after the debounce, calls settingsManager.setKeyValue; settings-management.ts:273-281 queues that value and pushes it to the vehicle. The correction being made is a statement about this topside ("this install speaks MAVLink axes"), but it is published to every topside on that vehicle.

Combined with the new sync trigger, that produces a live regression the base branch does not have. Sequence:

A 1.19 topside boots, runs its forward migration (controller.ts:87 on master), and writes the data-lake ids to the vehicle.
A 1.18 topside carrying this PR receives them via settingsSyncer.ts:142, the new watch fires, converts, and writes MAVLink axes back to the vehicle.
The 1.19 topside receives that. Its forward migration is boot-time only — there is no watcher — so it does not re-migrate. Its MavlinkManualControlManager keeps reading outputs/mavlink/axis-* from the data lake (mavlink-manual-control.ts:482-487), which nothing is feeding any more, and sends zeros until Cockpit is restarted.

The base branch does not do this: without the reverse migration, a 1.18 topside left the synced value alone. This PR makes it an active writer of a shape the newer version cannot use, which is the same class of bug as the one being fixed, pointed the other way. The PR body's "Going back to 1.19 remigrates forward" is true of a 1.19 restart and does not cover a 1.19 instance that is already running.

AGENTS.md:129 is explicit that "re-running the migration on a later launch must never overwrite user data", and AGENTS.md:125 that useBlueOsStorage is shared by every topside and operator of the vehicle. A per-install compatibility correction does not belong in that key.

What to do, in preference order:

Keep the conversion local. Convert where the mapping is consumed — the axis loop that drives MANUAL_CONTROL on this branch — and leave protocolMapping.value holding whatever the vehicle sent. The stick works on this install, nothing is written back, and no other topside is affected. This is also what the PR body already claims the code does.
If the store-level conversion has to stay because the config UI needs to render MAVLink actions, then suppress the write-back: mirror the converted mapping into a local, non-synced ref that the consumers and the UI read, rather than into the synced one.
If neither is acceptable and writing back is the deliberate choice, say so explicitly in the PR body and in a comment at controller.ts:86, naming the mixed-fleet consequence, per AGENTS.md:132.

3. AGENTS.md Adherence — 1 finding

3.1 — Three empty JSDoc blocks in the added file, on a type that did not need writing — major

Consequence: placeholder comment blocks with nothing written in them ship into the codebase, which the project's own rules forbid outright; the reason they are there is that the file re-declares a type the codebase already provides.

src/libs/joystick/protocols/data-lake-axis-migration.ts:8-10, :12-14 and :16-18 each add a /** */ block whose summary line is empty:

    /**
     *
     */
    protocol: JoystickProtocol.MAVLinkManualControl

AGENTS.md:102 — "Never write a JSDoc whose summary line is empty, whitespace-only, or filler … If you have nothing useful to say, omit the block entirely instead of leaving it blank." AGENTS.md:97 adds that JSDoc should be avoided on private types where the name and signature are self-describing, which protocol, id and name plainly are.

They exist because jsdoc/require-jsdoc in .eslintrc.cjs:39 includes TSPropertySignature, and the file declares an inline type literal for the map's values. Both problems have one fix: that literal is ProtocolAction, which already exists at src/types/joystick.ts:83-96 with its members documented, and is already the declared type of the field being assigned. Record<string, ProtocolAction> deletes the literal and all three blocks.

The same rewrite also removes a duplication worth naming. The six replacement values re-hardcode axis_x…axis_t and Axis X…Axis T, which are already a table on this branch: the base's src/libs/joystick/protocols.ts imports mavlinkManualControlAxes from ./protocols/mavlink-manual-control (visible in the diff's own context lines), and MAVLinkAxisFunction (mavlink-manual-control.ts:18-25) holds the same six ids. Keying the map on the data-lake id and taking the value straight from mavlinkManualControlAxes leaves one place where an axis's id and human-readable name are written down, and reduces the new file to roughly a dozen lines. Per the AGENTS.md "Before writing code" ladder, reusing the existing table beats re-typing it.

(The choice of a separate module rather than appending to the ~700-line mavlink-manual-control.ts is fine and is not part of this finding — AGENTS.md:33 allows a dedicated .ts module when a file has grown bloated, and that one has.)

Sections with nothing to report (9)

1. Correctness & Implementation Bugs — ✅ (traced the watcher's flush behaviour: non-deep on a ref, so it fires on identity change only; the JSON.stringify guard at controller.ts:93 terminates the self-retrigger after one extra pass; setup-time ordering against the initializeProtocolMapping watcher at controller.ts:148 is unchanged from the base; const [migrated] cannot destructure undefined because the migrations JSON-clone a fixed-length array; the duplicate-axis watcher at controller.ts:330 and the modifier-key interval at controller.ts:358 were checked for interaction and neither reads the axis actions the migration produces)

4. Security — ✅ (no new dependency, no network call, no build/CI/Electron change; a ripgrep for non-ASCII over pr.diff returns no match, so no hidden Unicode or homoglyphs; no encoded blobs; the PR body's "please cherry-pick onto #2997" is addressed to maintainers, and nothing in pr.json or pr.diff is addressed to this reviewer)

5. Performance — ✅ (all three changed functions trace to the same non-hot watcher, fired at setup and on whole-mapping replacement; four JSON.stringify passes over ~192 entries per fire; no interval, listener or subscription is added, and the watcher is a store-lifetime singleton like the existing ones at controller.ts:148, :249 and :330)

6. UI / UX — ✅ (no template, style, dialog or control is touched; the rewrite is silent, matching the existing silent servo→actuator migration at mavlink-manual-control.ts:665, and it is not a discrete user action so logUserAction does not apply)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised; checked the diff against .eslintrc.cjs for max-len 180 at line 47, func-style, simple-import-sort/imports at line 76 — the new import in protocols.ts sorts correctly between ./protocols/data-lake and ./protocols/mavlink-manual-control — and @typescript-eslint/explicit-function-return-type at line 84, whose allowExpressions covers the watch callback; no stray any; axisConfig.action?.protocol uses optional chaining per AGENTS.md:31; the only comment whose code changed is the one replaced at controller.ts:86, so the comment-immutability rule at AGENTS.md:34 is not breached)

8. Commit Hygiene — ✅ (one commit, joystick: restore MANUAL_CONTROL axes saved by a newer version; the scope prefix matches the git log style on this checkout, the body explains why rather than what, 124/6 lines across 4 files is one logical change, and the #2997/#3002 references are correctly confined to the PR body and absent from the commit message)

9. Tests — ✅ (two tests added at src/tests/libs/joystick/axis-migration.test.ts, asserting on ids and toMatchObject rather than on whole-object identity, so they are not brittle; the negative case covers both an already-MAVLink axis and an unrelated data-lake axis; no existing test is removed or weakened)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README change is owed; the exported function at data-lake-axis-migration.ts:30-34 carries a typed @param and @returns with real descriptions, satisfying jsdoc/require-returns at .eslintrc.cjs:46)

11. Nitpicks / Optional — ✅ (nothing worth raising beyond the findings above)

Generated by Claude. This is advisory; a human reviewer must still approve.

rafaellehmkuhl · 2026-09-03T19:48:25Z

Review follow-up — round 1

Done

src/stores/controller.ts (2.1 — converted mapping written back to the vehicle): removed the watch that assigned the rewritten mapping into protocolMapping. The vehicle-synced key is left as 1.19 sent it.
src/libs/joystick/protocols/mavlink-manual-control.ts (2.1): convert only in updateControllerData when building the in-memory mapping used to send MANUAL_CONTROL. Stick works on this topside; nothing is pushed back.
src/libs/joystick/protocols.ts: axis conversion is no longer part of performJoystickMappingMigrations (that path persists).
src/libs/joystick/protocols/data-lake-axis-migration.ts (3.1 — empty JSDoc on an inline type): map values are ProtocolAction. The three empty blocks are gone.

Done differently

src/libs/joystick/protocols/data-lake-axis-migration.ts (3.1 — reuse mavlinkManualControlAxes): the consume path lives in mavlink-manual-control.ts, so importing mavlinkManualControlAxes from the migration module is a cycle. The six ids stay in this file; they still match MAVLinkAxisFunction.

rafaellehmkuhl · 2026-09-03T19:48:27Z

/review

github-actions · 2026-09-03T20:06:24Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 2)

4 open findings: 2 major (1.1, 1.2) and 2 minor (6.1, 8.1). 2 closed this round (2.1, 3.1).

The round-1 design was reworked. The rewritten joystick mapping is no longer saved back to the vehicle: the stored settings are left exactly as the newer version wrote them, and the six stick axes are translated into the form this version understands only in memory, each time the joystick reports a new position. That closes both findings from round 1. Two new problems come with the new shape. The translation hands the MAVLink code a private copy of the mapping instead of the live one, so a separate routine that clears a joystick button the autopilot has no room for now writes into a copy that is thrown away a frame later — the button is never cleared and the error popup returns every second. Separately, the rework deleted a startup step that belonged to the branch, not to this change: the repair that renames old "Servo" button assignments to "Actuator" no longer runs at all, so a user whose stored mapping still holds the old names keeps a button that does nothing.

What still needs attention

#	Problem	What it means	Severity	Status
1.2	An existing startup repair of the saved mapping was deleted	Anyone whose stored joystick settings still use the old names for the autopilot's output buttons keeps a button that silently does nothing, because the repair that used to fix those names at startup no longer runs.	major	❌
1.1	The MAVLink code is handed a throwaway copy of the mapping	When the autopilot has no free button slot, Cockpit's attempt to clear the offending joystick button is discarded, so the error popup reopens every second and the button stays broken.	major	❌
6.1	Joystick settings screen no longer shows what the sticks actually do	After connecting to a vehicle configured by a newer Cockpit, the joystick page shows the six main axes as something other than what is really steering the vehicle, so a user checking their setup cannot tell it is working.	minor	❌
8.1	Commit message describes a mechanism that is no longer there	Anyone reading the project history is told the fix works one way when the code now works another, which misleads whoever next touches it.	minor	❌

Since round 1 — 2 closed, comparing bc875f5 → 03e32b3

Range and its reliability. Previous review was at bc875f5, current head is 03e32b3. incremental.diff is not usable as an increment this round: it reproduces the whole PR — all four files, including the two added ones as complete file bodies, with src/stores/controller.ts as +1/-7, byte-identical in substance to pr.diff. That is what a force-push produces, and pr.json confirms it: the PR carries exactly one commit, 03e32b3, so the round-1 head is no longer on the branch and there is no PREV→HEAD range to compare. Every status transition below is therefore judged from pr.diff against the base, not from the increment.

2.1 — Converted mapping written back to the vehicle-synced key — ✅ Addressed.

The finding asked for two things and both landed. (a) Stop publishing the correction to the vehicle-synced key: the round-1 watch that assigned into protocolMapping.value is gone; src/stores/controller.ts now has no write of a converted mapping at all, and migrateDataLakeAxesToMavlinkManualControl deep-clones before mutating (data-lake-axis-migration.ts:29), so the object the store owns is never touched. Nothing in the diff reaches settingsSyncer any more. (b) Convert where the mapping is consumed: the conversion moved to MavlinkManualControlManager.updateControllerData (mavlink-manual-control.ts:433 in the head). The mixed-fleet sequence the finding described — a 1.18 topside overwriting a 1.19 topside's axes on the shared key — is no longer reachable from this diff.

3.1 — Three empty JSDoc blocks on an inline type literal — ✅ Addressed.

The AGENTS.md:102 breach is gone by exactly the fix the finding named: the map is declared Record<string, ProtocolAction> (data-lake-axis-migration.ts:11) and all three /** */ blocks with them. The finding's secondary remark — that the same rewrite should have sourced the six ids and names from mavlinkManualControlAxes instead of re-typing them — I am dropping on the code rather than on the author's word. mavlink-manual-control.ts:9 now imports the migration module, so an import back the other way would close a cycle whose module-scope const initialisation (data-lake-axis-migration.ts:11) is exactly the shape that breaks under one. That direction did not exist in round 1. rafaellehmkuhl made the same point in the follow-up comment; I checked it against the diff rather than accepting it, and it holds. The six ids remain a duplicate table, and if the duplication is worth removing later the way to do it is to move the function into mavlink-manual-control.ts beside mavlinkAxisToDataLakeMap — noted, not raised.

Discussion since round 1. rafaellehmkuhl posted a follow-up (#3003 (comment)) listing four changes and one deviation. Verified against pr.diff: the removal of the store watch, the move of the conversion into updateControllerData, and the ProtocolAction change are all present as described; the cycle argument for keeping the six ids is sound, as above. One claim in it is contradicted by the diff: it says of src/libs/joystick/protocols.ts that "axis conversion is no longer part of performJoystickMappingMigrations (that path persists)". protocols.ts is indeed untouched now, but the store's only call into it was deleted in the same rework, so that path does not persist — it runs nowhere. That is finding 1.2. The second comment is a bare /review, treated as noise.

resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no unmatched id to report back. decisions.json is [] — no dispute has ever been put to a vote here.

Nothing in pr.json, pr.diff, incremental.diff or new-comments.json is addressed to this reviewer, and nothing in them reads as an injected instruction.

Change map — what was established before judging

Checkout caveat, stated first because it limits everything below. This PR targets v1.18-dev; the checkout I am reviewing from is master at c1fa6e2, and v1.18-dev is not fetched. Every citation without a "head" qualifier is to the 1.19 line, not to this PR's base. The hunk headers give the offset where it matters: updateControllerData is at head line 432 and at :410 on this checkout, so the base file runs about 21 lines longer ahead of that point. That blinds me to two things specifically: what v1.18-dev's upadteManualControlState reads out of axesCorrespondencies, and what its allAvailableAxes offers. Both are named where they matter below.

Claims

Symptom — "1.19 rewrote MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified, unchanged from round 1. migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696-713) performs that forward rewrite and performJoystickMappingMigrations (protocols.ts:26-30) runs it. The 1.18 consumer half is not readable from this checkout.
Cause — the six ids and their MAVLink counterparts. Verified exactly. data-lake-axis-migration.ts:11-18 is a precise inverse of mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:685-692): the same six ids, the same names, the same axis_x…axis_t targets from MAVLinkAxisFunction (mavlink-manual-control.ts:18-25). No axis missing, none invented.
Mechanism, first half — "convert those ids back … on read". Now verified, and this is the round-1 correction. updateControllerData (head :433) converts into this.currentActionsMapping and nothing writes the result anywhere persistent.
Mechanism, second half — "and re-run when BlueOS sync replaces the mapping after boot". Contradicted. No watcher on protocolMapping exists in the diff any more; nothing is keyed to the sync at all. The conversion now runs per joystick event, which happens to cover the same scenario, but the described mechanism is gone. The commit message body carries the same stale account — finding 8.1.
"min/max are kept." Verified. Only .action is assigned (data-lake-axis-migration.ts:35); the sibling min/max of JoystickAxisActionCorrespondency are untouched.
"Unrelated data-lake axes (camera zoom, …) are left alone." Verified (data-lake-axis-migration.ts:34 returns on a lookup miss).
"Going back to 1.19 remigrates forward." Moot in the harmful direction now that nothing is written back. What is new and not claimed anywhere is that this round's diff removes the forward migrations on this line — see Entry points and 1.2.

Failure site. The code that actually misbehaves is v1.18-dev's MANUAL_CONTROL axis consumer, which cannot be in this diff or this checkout; on the side I can read, the origin of the value 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as designed for its own branch. Converting at the point of consumption is the right shape for a backport, and there is no smaller fix at the real site to name. What is new this round is that the diff also deletes base code at src/stores/controller.ts:86-90 — that deletion is in the diff, and it is finding 1.2.

Entry points

Function	Reached from	Frequency
`migrateDataLakeAxesToMavlinkManualControl` (head `data-lake-axis-migration.ts:26`)	only `updateControllerData`, below	`per frame or pointer event`
`updateControllerData` (head `mavlink-manual-control.ts:432`)	registered as a controller-update callback at `src/stores/mainVehicle.ts:1025`; invoked from `processJoystickStateEvent` (`controller.ts:266-292`), which is fed by the `requestAnimationFrame` gamepad poll (`libs/joystick/manager.ts:554-564`) in the browser build and by the SDL IPC listener (`manager.ts:356`) in Standalone	`per frame or pointer event`
`performJoystickMappingMigrations` (`protocols.ts:26`)	nothing. `rg performJoystickMappingMigrations src` finds the definition, the import and the call at `controller.ts:87` — the diff removes both the import and the call, and adds no other	`never`
`useControllerStore` setup body (`controller.ts:83-90`, the deleted block)	Pinia store instantiation	`one-shot` (was; now absent)

The frequency of the first two rows is the fact that decides most of this review. manager.ts:531-560 emits a state event whenever any axis or button value differs from the previous poll, so while a stick is being moved this is once per animation frame; the Standalone path emits per SDL state change. The base assigned one reference there. The head performs a JSON.parse(JSON.stringify(...)) round-trip of the whole mapping — axesCorrespondencies plus buttonsCorrespondencies for both modifier states, ~190 entries — on each of those events, and protocolMapping.value is a deep reactive object, so the traversal goes through a proxy get-trap per property. The never row is required to be raised as a finding and is 1.2.

Invariants

MavlinkManualControlManager.currentActionsMapping is the same object the store holds, so writes to it reach the store. Established by the base's this.currentActionsMapping = protocolActionsMapping plus controller.ts:285, which passes protocolMapping.value itself. rg currentActionsMapping src gives eleven sites: nine reads, and two writes at mavlink-manual-control.ts:647 and :660 (about :668/:681 on the base, by the offset above). The head breaks the invariant at the single chokepoint — the assignment — and the two write sites are exactly what breaks with it. Finding 1.1.
The persisted mapping is normalised at store setup by performJoystickMappingMigrations. Held on the base at controller.ts:86-90. The head removes it and nothing re-establishes it: on this checkout that function is migrateMavlinkManualControlAxes(migrateMavlinkManualControlButtons(mappings)) (protocols.ts:26-30), whose button half is the Servo→Actuator rename at mavlink-manual-control.ts:665-683. Finding 1.2.
The stored mapping may now hold data-lake ids for the six MANUAL_CONTROL axes — deliberate, and the point of the 2.1 fix. Every consumer of axesCorrespondencies, from rg axesCorrespondencies src: the MAVLink manager (converted, covered); the data-lake protocol handler (protocols/data-lake.ts:76-89), which will call setDataLakeVariableData('inputs/mavlink/axis-x', …) per frame for variables 1.18 most likely does not register — harmless, since libs/actions/data-lake.ts:135-153 creates the entry silently and warns about nothing; the duplicate-axis watcher (controller.ts:330-353), which compares action ids only and is unaffected; and the joystick configuration view, which renders against availableAxesActions (controller.ts:501-504) and is finding 6.1.

1. Correctness & Implementation Bugs — 2 findings

1.1 — updateControllerData now stores a detached clone, so the button-slot un-mapping is discarded and its error dialog repeats every second — major

Consequence: when the autopilot has no free button slot left, Cockpit's attempt to clear the joystick button it cannot map is thrown away a frame later, so the error popup reopens every second and the button stays broken.

Head mavlink-manual-control.ts:433 replaces the reference with a copy:

-    this.currentActionsMapping = protocolActionsMapping
+    this.currentActionsMapping = migrateDataLakeAxesToMavlinkManualControl([protocolActionsMapping])[0]

migrateDataLakeAxesToMavlinkManualControl returns JSON.parse(JSON.stringify(mappings)) (data-lake-axis-migration.ts:29) — a detached plain object, by design, which is what makes the 2.1 fix work. But currentActionsMapping is not read-only state. remapActionsToVehicleButtonParameters, which the constructor runs on a 1000 ms interval (mavlink-manual-control.ts:385-387), writes to it: when a wanted MAVLink button function has no slot left on the vehicle, it shows an error dialog and unmaps the joystick button by assigning otherAvailableActions.no_function at mavlink-manual-control.ts:647 and :660 (around :668/:681 on the base). On the base that write lands on protocolMapping.value itself — controller.ts:285 passes the store's own object — so the mapping is actually cleared, the UI shows it, settingsSyncer persists it, and the next tick no longer wants an unmappable action. With the head, the write lands on a copy that the next joystick event replaces. Three things follow:

The user's joystick button keeps an action the autopilot cannot accept, and does nothing when pressed. Nothing tells them the un-mapping failed.
The interval re-derives the same finallyRemainedUnmappedRegularMavlinkActions every second and calls showDialog again each time, with timer: 6000 (mavlink-manual-control.ts:640-646). An error dialog reopening once a second, indefinitely, while the vehicle is being flown. That is the dialog-spam pattern in guideline section 6 — a dialog opened from a timed loop with no check that one is already up — and here the loop can no longer terminate because the state it was written to change is unreachable.
The store and the manager now disagree about the mapping for as long as the manager holds its copy, which is the kind of divergence the base avoided by construction.

The fix that costs least: leave the assignment as this.currentActionsMapping = protocolActionsMapping and translate at the read site instead. On this branch the axis values are read out of axesCorrespondencies inside upadteManualControlState; look the action id up in dataLakeManualControlAxisById there, per axis, when the protocol is DataLakeVariable. That is O(1) per axis, needs no clone, and leaves buttonsCorrespondencies aliased to the store so remapActionsToVehicleButtonParameters keeps working. It also removes the second cost of the current shape: a full serialise-and-parse of a ~190-entry deep reactive object on every joystick event, per the frequency established in Entry points, on the path that has to keep MANUAL_CONTROL going out at a steady rate. Caching the converted copy by mapping identity would fix the cost but not this finding, since the aliasing would still be gone. While the function is being touched, migrateDataLakeAxesToMavlinkManualControl([x])[0] also wants a single-mapping signature — the array form exists only because performJoystickMappingMigrations had it, and that caller is gone.

1.2 — The rework deletes the base's boot-time mapping migration, leaving performJoystickMappingMigrations with no caller — major

Consequence: a user whose saved joystick mapping still uses the old "Servo" names for the autopilot's output functions no longer has them repaired at startup, so those buttons silently stop doing anything, and the PR does not mention removing that repair.

pr.diff removes base code from src/stores/controller.ts:

-  // Run schema migrations on the current mapping
-  const migratedArray = performJoystickMappingMigrations([protocolMapping.value])
-  if (migratedArray.length > 0) {
-    protocolMapping.value = migratedArray[0]
-  }

along with the import at controller.ts:18. Those are - lines against v1.18-dev, and they match this checkout's controller.ts:86-90 exactly, so they are the branch's own code and not something round 1 of this PR introduced. Round 1 replaced that block with a watch; round 2 removed the watch and did not put the block back.

rg performJoystickMappingMigrations src finds no other call site, so after this PR the function at protocols.ts:26-30 runs nowhere, and everything it composed goes with it. On this checkout that is migrateMavlinkManualControlAxes — 1.19-only, irrelevant here — and migrateMavlinkManualControlButtons (mavlink-manual-control.ts:681-683), which is migrateServoSubButtonsToActuators (:665-679): it rewrites persisted MANUAL_CONTROL button actions whose id and name say Servo into Actuator, matching the MAVLinkButtonFunction values the code actually uses (mavlink-manual-control.ts:76-122). The base composition of performJoystickMappingMigrations on v1.18-dev is not readable from this checkout, but the servo→actuator rename predates the 1.19 axis work and is present on both lines, so at minimum that repair is what the deletion removes. Without it, a stored Servo 1 inc action never matches an entry in vehicleButtonParameterTable, remapActionsToVehicleButtonParameters finds no mavlinkActionValue for it (mavlink-manual-control.ts:587-588), and the button does nothing at all — with no message, because that path just returns.

Two further points about the shape of this:

It is a scope-discipline breach. AGENTS.md requires the diff to stay on the stated purpose; this PR is about restoring axes on a downgrade, and removing an unrelated boot-time button migration is not part of it. The follow-up comment says of protocols.ts that "that path persists", which is what makes this look accidental rather than decided — the file was reverted but its caller was not restored.
It leaves an exported function with zero call sites, which guideline section 3 flags in its own right and which the Entry points walk records as frequency never.

The fix is to restore the deleted block verbatim. It does not conflict with the 2.1 fix: performJoystickMappingMigrations on v1.18-dev does not contain the axis conversion, so putting the call back reinstates the branch's button migration without reintroducing any write of the axis rewrite to the vehicle-synced key. If the intention was in fact to retire that migration, it belongs in its own commit with its own justification, not folded into a downgrade fix.

2. Persistence & User Data — inventory, no findings

Inventory

Key	Backend	What happened
`cockpit-joystick-functions-mapping-v1` (`controller.ts:49`, `:57`)	vehicle-synced — `useBlueOsStorage`, shared by every topside computer and operator of that vehicle	Not added, not removed, not reshaped. Two changes to who writes it: the round-1 write-back of the converted mapping is gone (2.1 addressed), and the base's boot-time migration write at `controller.ts:86-90` is deleted (reported as 1.2).

No other persisted key is touched. Judged: the key is correctly cockpit- prefixed; the stored shape (JoystickProtocolActionsMapping, src/types/joystick.ts) is unchanged; nothing in it duplicates its own key; no machine-specific value is being synced. The PR now adds no automatic migration of user data at all — the conversion is in-memory only and provably leaves the input untouched, since data-lake-axis-migration.ts:29 clones before any assignment and every write at :35 targets the clone. That is the right side of the "automatic migrations are a last resort" rule, and it is what closes 2.1.

The one persistence consequence left is a removal rather than an addition, and it is the second row of the table above: users whose stored mapping still carries the old button names are stranded on it with nothing to carry them over. Guideline section 2's "behavior changes that strand already-configured users" bullet and section 1's regression bullet are the same finding here, so it is written out once, as 1.2, rather than counted twice.

6. UI / UX — 1 finding

6.1 — The joystick configuration view now shows something other than what is driving the vehicle — minor

Consequence: after connecting to a vehicle whose joystick settings were last saved by a newer Cockpit, the joystick settings page shows the six main axes as something other than what is really steering the vehicle, so a user checking their setup cannot tell that it works.

This is the cost of the option chosen for 2.1, and worth stating explicitly because the settings page is the one place a user goes to check this. The conversion is now confined to the MAVLink manager's private copy, so protocolMapping.value keeps the inputs/mavlink/axis-* data-lake actions for those six axes, and that is what the configuration view renders. Its options come from availableAxesActions, refreshed every second from allAvailableAxes() (controller.ts:501-504), which on this checkout is built purely from the data lake's registered variables (protocols.ts:12-14); the inputs/mavlink/axis-* variables themselves are registered by predefined-resources.ts:20-25, part of the 1.19 data-lake pipeline. Either way the display is wrong for the user: if v1.18-dev has no such variables the six rows have no matching option and render empty, and if it does they render as a data-lake binding that this version's MANUAL_CONTROL path does not actually use. In neither case does the page show the MAVLink axis action that is really being sent, and neither state is announced anywhere.

Round 1's finding 2.1 named this trade in its second option: mirror the converted mapping into a local, non-synced ref that the consumers and the view read. Converting for display only would also do — the view already re-reads its options once a second, so a translated copy passed to the axis rows costs nothing and is not persisted. Whatever the route, the page should show the user the axis assignment the code is honouring.

8. Commit Hygiene — 1 finding

8.1 — The commit message body describes the round-1 mechanism, which the code no longer has — minor

Consequence: anyone reading the project history is told the fix works one way when the code now works another, which misleads whoever next touches this file.

pr.json carries one commit, 03e32b3, subject joystick: restore MANUAL_CONTROL axes saved by a newer version. The subject is accurate and the scope prefix matches the git log style on this checkout. The body's second sentence is not:

Re-run the conversion when the vehicle sync replaces the mapping so a stick that arrives after boot is not left at zero.

There is no re-run keyed to the vehicle sync in this diff; the watch that did that was removed. The conversion now runs per joystick event inside updateControllerData, which does cover a mapping that arrives after boot, but by an entirely different route — and the difference is exactly what a future reader would need to know before touching either file. The body should describe converting at the point of consumption and say that nothing is written back, which is the property the fix now depends on. The PR body carries the same stale sentence ("and re-run when BlueOS sync replaces the mapping after boot"); worth fixing there too, though the commit message is the copy that outlives the PR.

Otherwise clean for this section: one commit for one logical change, no wip/fixup! noise, no issue or PR reference in the message (the #2997/#3002 references stay correctly in the PR body), and nothing self-correcting in the history — the rework was amended into the single commit rather than stacked on top of it. The one caveat is that amending is also what destroyed the round-1 range, as noted in the since-round-1 block.

Sections with nothing to report (7)

3. AGENTS.md Adherence — ✅ (the round-1 empty-JSDoc breach of AGENTS.md:102 is fixed and closed as 3.1; the exported function at data-lake-axis-migration.ts:20-25 carries typed @param/@returns with real descriptions; the two added comments explain why rather than what; optional chaining is used at :32; no dependency added; the scope-discipline breach and the exported-function-with-no-call-site rule are both about the deleted migration call, reported once as 1.2 rather than again here)

4. Security — ✅ (no new dependency, no network call, no build/CI/postinstall/Electron change; a ripgrep for non-ASCII over pr.diff returns exactly one line — an em dash inside the new JSDoc at data-lake-axis-migration.ts:22 — so no zero-width, bidi or homoglyph characters; no encoded blobs, no eval/Function(), no v-html, no secret or env-var handling)

5. Performance — ✅ (both changed functions traced to the rAF gamepad poll at manager.ts:554-564 and the SDL listener at manager.ts:356, so per frame or pointer event while a stick moves; the per-event JSON.parse(JSON.stringify(...)) of a ~190-entry deep reactive object that this introduces is the cost half of 1.1 and is reported there, with its remedy, rather than counted twice; no interval, listener, subscription or watcher is added, and none is removed without its teardown)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised; the new file read in full as a program — the ?? {} at :31 and the !replacement guard at :34 make both nullish paths no-ops, and the Record<string, ProtocolAction> index is guarded before use; checked against .eslintrc.cjs for max-len 180 and prettier's printWidth 120 — the longest added line is ~109 chars — func-style with allowArrowFunctions, semi: never, explicit-function-return-type, and jsdoc/require-jsdoc, whose TSPropertySignature context no longer applies now that the inline literal is gone; simple-import-sort order correct for the new import at mavlink-manual-control.ts:9, between @/libs/communication/mavlink and @/libs/joystick/protocols/other; no stray any; the comment deleted at controller.ts:83 goes with the code it described, so the comment-immutability rule is not breached; the file the PR grows is 33 lines in a new module, not bulk added to an already-large file)

9. Tests — ✅ (src/tests/libs/joystick/axis-migration.test.ts is new and follows the existing src/tests/libs/** layout; both tests assert on ids and via toMatchObject rather than whole-object identity, so they are not brittle, and the negative case covers an already-MAVLink axis and an unrelated data-lake axis; a ripgrep for performJoystickMappingMigrations and migrateServoSubButtonsToActuators under src/tests returns nothing, so the deletion in 1.2 does not silently drop a test; no existing test is removed or weakened)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README change is owed — the change sits in the shared joystick path, and the only branch-specific code nearby is the SDL listener the PR does not touch; in-code JSDoc on the one exported function is complete and its second line states the caller contract that 2.1 turned on, which is the right thing to have written down)

11. Nitpicks / Optional — ✅ (the array-wrapper signature migrateDataLakeAxesToMavlinkManualControl([x])[0] is the only taste-level thing worth changing, and it is already named inside 1.1's remedy rather than raised on its own)

Generated by Claude. This is advisory; a human reviewer must still approve.

rafaellehmkuhl · 2026-09-03T20:18:21Z

Review follow-up — round 2

Done

src/libs/joystick/protocols/mavlink-manual-control.ts (1.1 — detached clone discarded button un-maps): updateControllerData aliases the store mapping again. Axis lookup uses manualControlAxisId so 1.19 data-lake ids still drive MANUAL_CONTROL. remapActionsToVehicleButtonParameters writes land on the live mapping.
src/libs/joystick/protocols/data-lake-axis-migration.ts (1.1): the whole-mapping clone is gone; the file is now the id lookup only.
src/stores/controller.ts (1.2 — boot-time performJoystickMappingMigrations deleted): restored the base call. On this line that is still only the Servo→Actuator button repair; it does not rewrite axes onto the vehicle key.
Commit / PR body (8.1): message and summary now say the conversion is at MANUAL_CONTROL build time and nothing is written back.

Won't change (with reasoning)

6.1 — joystick settings still show the stored data-lake axis bindings: that is the 2.1 contract. The page is bound to the vehicle-synced mapping; painting MAVLink labels over it would either lie about what is stored or tempt a save that publishes 1.18 axis ids to a live 1.19 topside.

rafaellehmkuhl · 2026-09-03T20:18:23Z

/review

github-actions · 2026-09-03T20:29:55Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 3)

3 open findings: 1 major (1.3), 1 minor (6.1, disputed) and 1 nit. 3 closed this round (1.1, 1.2, 8.1).

The approach was rewritten again and this round it is small: nothing is copied, nothing is written back, and the only behavioural change is at the moment Cockpit builds the steering message it sends to the vehicle. A tiny lookup now answers "which of the six vehicle axes does this stored stick assignment drive?", so an assignment saved by a newer Cockpit is understood in place. That closes both problems from the last round. What it introduces is that two different stored assignments can now claim the same vehicle axis, and the check that is supposed to warn a user about a duplicate assignment does not recognise those two as the same — so after downgrading, a stick a user re-assigns can silently do nothing, with no warning anywhere.

What still needs attention

#	Problem	What it means	Severity	Status
1.3	Two stick assignments can now claim the same vehicle axis, undetected	After downgrading, if a user moves one of the main vehicle controls to a different stick, the leftover assignment on a lower-numbered stick keeps winning, so the new one does nothing and Cockpit's duplicate warning never appears.	major	❌
6.1	Joystick settings screen still does not show what the sticks actually do	After connecting to a vehicle configured by a newer Cockpit, the joystick page shows the six main axes as something other than what is really steering the vehicle, so a user checking their setup cannot tell it is working.	minor	💬
11.1	A file called a migration that migrates nothing	Whoever next opens this file is told it changes stored settings when it only reads them, and may wire it into the startup routine the previous rounds established it must stay out of.	nit	❌

Since round 2 — 3 closed, 1 disputed, comparing 03e32b3 → eaf3e2c

Range and its reliability. Previous review was at 03e32b3, current head is eaf3e2c. incremental.diff is again unusable as an increment: it reproduces the whole PR — all three files, both added ones as complete file bodies, mavlink-manual-control.ts as +7/-6 byte-identical in substance to pr.diff — and pr.json still carries exactly one commit, now eaf3e2c. The round-2 head was amended away, so there is no PREV→HEAD range. Every transition below is judged from pr.diff against the base, not from the increment. Note what this hides: a file the previous head changed and this one does not appears in neither diff, so the round-2 deletion in src/stores/controller.ts had to be confirmed by its absence from pr.diff, which is done below.

1.1 — Detached clone in updateControllerData — ✅ Addressed.

The finding asked for two things and both landed, by exactly the remedy it named. (a) Leave the assignment as this.currentActionsMapping = protocolActionsMapping: updateControllerData no longer appears in pr.diff at all, so the base's aliasing assignment stands (mavlink-manual-control.ts:412 on this checkout). controller.ts:285 passes protocolMapping.value itself, so the manager holds the store's own object again and the un-mapping writes in remapActionsToVehicleButtonParameters — the ones the 1000 ms interval at mavlink-manual-control.ts:385-387 drives — land where the store and settingsSyncer can see them. The error dialog that was reopening every second can terminate again. (b) Translate at the read site instead: the six find predicates in upadteManualControlState now call manualControlAxisId (head mavlink-manual-control.ts:505-510). The JSON.parse(JSON.stringify(...)) of the whole mapping is gone from the file — rg over pr.diff finds no clone at all — so the per-event cost the finding's second half described is gone with it. The array-wrapper signature it also asked about no longer exists; the function takes one action.

1.2 — Deleted boot-time performJoystickMappingMigrations call — ✅ Addressed.

src/stores/controller.ts is no longer in pr.diff. The base's block survives untouched — on this checkout it is controller.ts:86-90 with the import at :18, and rg performJoystickMappingMigrations src finds the definition (protocols.ts:26), the import and that one call, so the function has a caller again and the servo→actuator button repair runs at store setup. The scope-discipline breach and the zero-call-site export both go with it.

8.1 — Stale commit-message mechanism — ✅ Addressed.

The commit is now eaf3e2c, subject unchanged and still accurate, body replaced with: 1.19 persisted those axes as data-lake ids; resolve them when building MANUAL_CONTROL so the stick works; and leave the vehicle-synced mapping untouched. That is what the code does, including the property the fix depends on. The PR body's stale "re-run when BlueOS sync replaces the mapping after boot" sentence is gone too — its summary now says the vehicle-synced mapping is left as 1.19 wrote it.

6.1 — Joystick configuration view shows the stored data-lake bindings — 💬 Disputed.

No code change touches it, and the author declines it in the round-2 follow-up. Their argument, in plain prose: showing the MAVLink axis names on the joystick settings page would either misrepresent what is stored in the vehicle-synced mapping or tempt a save that publishes 1.18 axis ids to a live 1.19 topside, so the page deliberately keeps showing the stored data-lake bindings. Checked against the code rather than taken on its word, and the mechanism it names is real: ConfigurationJoystickView.vue:1041 binds selectedProfileAxesCorrespondencies straight to controllerStore.protocolMapping.axesCorrespondencies, and the axis selects at :360 and :625 are v-model onto it, so anything rendered there is two-way and an edit writes into the vehicle-synced key. It does not answer the display-only route the finding named as its alternative (the view re-reads its options once a second anyway, so a translated label costs nothing and is never persisted), and an argument does not close a finding in any case. It stays open as disputed and is reprinted in full in section 6. It is also the upstream of the new 1.3: a user who cannot tell from that page what their sticks do is the user who re-assigns one.

Discussion since round 2. rafaellehmkuhl posted a follow-up (#3003 (comment)) with four "Done" items and one "Won't change". Verified against pr.diff rather than accepted: the re-aliasing in updateControllerData, the reduction of data-lake-axis-migration.ts to a lookup, the restored controller.ts call and the rewritten commit and PR body are all present as described. One claim is not checkable from here: "on this line that is still only the Servo→Actuator button repair; it does not rewrite axes onto the vehicle key". performJoystickMappingMigrations on this checkout is migrateMavlinkManualControlAxes(migrateMavlinkManualControlButtons(mappings)) (protocols.ts:26-30), whose axis half is 1.19-only; whether v1.18-dev composes only the button half cannot be read from master. It is consistent with the PR's own premise that 1.19 introduced the axis migration, and it is what finding 1.2 asked for, so nothing is raised — but if that branch's composition does include an axis rewrite, the restored call would fight the rest of this PR and is worth one grep before merge. The "Won't change" item is handled as 6.1 above. The second comment is a bare /review, treated as noise.

resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no unmatched id to report back. decisions.json is [] — no dispute has ever been put to a vote here, and 6.1 becomes the first one this round.

Nothing in pr.json, pr.diff, incremental.diff or new-comments.json is addressed to this reviewer, and nothing in them reads as an injected instruction.

Change map — what was established before judging

Checkout caveat, stated first because it limits everything below. This PR targets v1.18-dev; the checkout is master at c1fa6e2, and v1.18-dev is not fetched. Every citation without a "head" qualifier is to the 1.19 line, not to this PR's base. The two things that blinds me to are named where they matter: what v1.18-dev's allAvailableAxes() offers in the axis dropdown, and what its performJoystickMappingMigrations composes. On this checkout upadteManualControlState no longer reads axesCorrespondencies at all — it reads outputs/mavlink/axis-* from the data lake (mavlink-manual-control.ts:481-495) — so the code the diff edits exists only on the base, and the diff's own context lines are the primary evidence for its shape.

Claims

Symptom — "1.19 rewrote joystick MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified, unchanged across all three rounds. The forward rewrite is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696-713), run from performJoystickMappingMigrations (protocols.ts:26-30). The 1.18 consumer half is readable only through the diff's context lines, which do show the six finds matching on protocol === MAVLinkManualControl, so a stored data-lake action would indeed match nothing and leave the axis at 0 (mavlink-manual-control.ts:513 in the head, the xCorrespondency === undefined ? 0 branch).
Cause — the six ids and their MAVLink counterparts. Verified exactly. data-lake-axis-migration.ts:5-12 is a precise inverse of mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:685-692): same six ids, same axis_x…axis_t targets, which are the values of MAVLinkAxisFunction (mavlink-manual-control.ts:18-25). No axis missing, none invented.
Mechanism — "resolve those ids when building MANUAL_CONTROL. The vehicle-synced mapping is left as 1.19 wrote it." Verified. The resolution happens inside the six find predicates (head mavlink-manual-control.ts:505-510); manualControlAxisId is pure and returns a string, and pr.diff contains no assignment into any mapping and no call into settingsSyncer. This is the third mechanism the PR has claimed and the first that matches the code exactly.
"min/max are kept." Verified. Only the find predicate changed; the consumers still read xCorrespondency[1].min / .max off the same entry (head :513).
"Unrelated data-lake axes (camera zoom, …) are left alone." Verified — dataLakeManualControlAxisById[action.id] is undefined for any other id (data-lake-axis-migration.ts:21), and undefined never equals an axis id. The added test at axis-migration.test.ts:19-21 covers exactly this.
"Going back to 1.19 remigrates forward." Verified and now trivially so: nothing is written back, so a 1.19 topside finds the key exactly as it left it.

Failure site. The misbehaving code is v1.18-dev's six axis lookups in upadteManualControlState, and it is in the diff — that is the change. On the side I can read, the origin of the value 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as designed for its own branch. Resolving at the point of consumption is the right shape for a backport and there is no smaller fix at a different site to name.

Entry points

Function	Reached from	Frequency
`manualControlAxisId` (head `data-lake-axis-migration.ts:19`)	only the six `find` predicates below; `rg manualControlAxisId src` over the diff finds the definition, the import at `mavlink-manual-control.ts:9`, those six call sites and the test	`per frame or pointer event`
`upadteManualControlState` (changed lines head `mavlink-manual-control.ts:505-510`)	`updateControllerData` (`mavlink-manual-control.ts:410-418`), registered as a controller-update callback at `src/stores/mainVehicle.ts:1025`, invoked from `processJoystickStateEvent` (`controller.ts:266-292`), fed by the `requestAnimationFrame` gamepad poll (`manager.ts:555-564`) in the browser build and by the SDL IPC listener in Standalone	`per frame or pointer event`

While a stick is moving this is once per animation frame. Per event the changed code runs six Object.entries(...).find(...) scans — as it did on the base — and the added work inside each predicate is one enum comparison plus at most one object-property read, replacing two comparisons. So this round the frequency multiplier costs nothing: the head is at worst level with the base and strictly cheaper than round 2's per-event deep clone. No caller of manualControlAxisId is on mavlink:onIncomingMessage or any data-lake notify path.

Invariants

Each MANUAL_CONTROL axis is driven by exactly one gamepad axis, and Cockpit warns and unmaps when two claim the same action. Enforced at a single chokepoint, the deep watcher on protocolMapping at controller.ts:330-356, which detects duplicates by comparing raw action.id (:336-339). The PR makes two different action.id values — axis_x and inputs/mavlink/axis-x — resolve to the same MANUAL_CONTROL axis, so the chokepoint no longer covers the invariant it was written for. Every other site that can produce that state was enumerated: the axis selects in ConfigurationJoystickView.vue:360/:625, the partial vehicle-defaults import (useVehicleDefaultsJoystickImport.ts:68-69 via the per-row diffs built in vehicleDefaultsImportShared.ts:78-125), and default-profile-importer.ts:71-74. The PR covers none of them. Finding 1.3.
The vehicle-synced mapping is never written by this PR. Held, and now by construction rather than by careful cloning: pr.diff contains no assignment into axesCorrespondencies, no protocolMapping.value = , and no new watcher. This is what closed 2.1 and it survives the rework.
The stored mapping may hold data-lake ids for the six MANUAL_CONTROL axes. Deliberate. Consumers, from rg axesCorrespondencies src: the MAVLink manager (now resolves them, the point of the PR); the data-lake protocol handler (protocols/data-lake.ts:81-88), which will keep calling setDataLakeVariableData('inputs/mavlink/axis-x', …) per frame — harmless, since libs/actions/data-lake.ts creates an unregistered entry silently, and on this line nothing reads those variables back; the duplicate watcher (invariant 1, now broken); and the configuration view (finding 6.1).

1. Correctness & Implementation Bugs — 1 finding

1.3 — A stored data-lake axis and a stored MAVLink axis can now both claim the same MANUAL_CONTROL axis, and the duplicate-axis guard cannot see it — major

Consequence: after a downgrade, a user who moves one of the six main vehicle controls onto a different stick gets no reaction from it and no warning, because the leftover assignment on a lower-numbered stick silently keeps the axis.

Head mavlink-manual-control.ts:505-510 widens the predicate:

-    const xCorrespondency = Object.entries(...).find((entry) => entry[1].action.protocol === JoystickProtocol.MAVLinkManualControl && entry[1].action.id === mavlinkManualControlAxes.axis_x.id)
+    const xCorrespondency = Object.entries(...).find((entry) => manualControlAxisId(entry[1].action) === mavlinkManualControlAxes.axis_x.id)

Two distinct stored actions now satisfy it for the same axis: {protocol: MAVLinkManualControl, id: 'axis_x'} and {protocol: DataLakeVariable, id: 'inputs/mavlink/axis-x'} (data-lake-axis-migration.ts:6, :20-21). find returns the first match in Object.entries order, and the keys of axesCorrespondencies are integer-like strings, so iteration is ascending gamepad-axis index — the lower-numbered stick wins regardless of which one the user set last.

The state is reachable, and it is the expected state after the downgrade this PR exists to handle:

All six axes arrive carrying data-lake ids. The user opens the joystick settings, cannot tell what the sticks are bound to (that is finding 6.1, which the author has declined), and re-assigns one — ConfigurationJoystickView.vue:360 and :625 write straight into axesCorrespondencies. If they pick a different gamepad axis than the one holding the leftover — the ordinary case when moving a control from one stick to another — both entries survive and the leftover wins.
The same state arrives without any manual edit through a partial vehicle-defaults import: vehicleDefaultsImportShared.ts:78-125 builds a per-axis diff and useVehicleDefaultsJoystickImport.ts:68-69 applies only the rows the user selected, so unselected axes keep their data-lake leftovers next to the newly imported MAVLink ones. default-profile-importer.ts:71-74 has the same per-axis shape.

I cannot read v1.18-dev's allAvailableAxes() from this checkout, so I cannot quote the dropdown's contents. Three things establish that MAVLink axis actions are offered and storable on that line anyway: the base predicate this diff replaces matches on protocol === MAVLinkManualControl, which would be dead code otherwise; 1.19 ships migrateMavlinkManualControlAxes specifically to convert stored actions of that protocol, which had to come from 1.18 users' mappings; and mavlinkManualControlAxes.axis_x.id is still the right-hand side of the comparison here.

What makes it silent is that the one guard against this cannot see it. controller.ts:336-339 collects Object.values(newMapping.axesCorrespondencies).map((v) => v.action.id) and looks for repeats; 'axis_x' and 'inputs/mavlink/axis-x' are different strings, so no duplicate is found, no warning dialog is shown (controller.ts:346-348), and neither axis is unmapped. The user gets a stick that does nothing, with nothing on screen to explain it — and per the Entry points walk this decision is re-made every animation frame, so it is stable rather than intermittent, which is the failure mode hardest to attribute.

Two fixes, and the first is the smaller one:

Make the resolution prefer an explicit MAVLink binding, so the base's behaviour is preserved verbatim whenever one exists and the data-lake id is only a fallback. Hoisting the shared Object.entries call out of the six lookups is the natural place to put it, and removes six repeated scans while you are there:
```
const axisEntries = Object.entries(this.currentActionsMapping.axesCorrespondencies)
const findAxis = (axisId: string): (typeof axisEntries)[number] | undefined =>
  axisEntries.find((e) => e[1].action.protocol === JoystickProtocol.MAVLinkManualControl && e[1].action.id === axisId) ??
  axisEntries.find((e) => manualControlAxisId(e[1].action) === axisId)
```
That leaves a stale duplicate ignored rather than winning, which is the behaviour a user editing the page expects.
Independently, controller.ts:336 should compare what the axes actually drive rather than the raw id — mapping each action through manualControlAxisId before the duplicate check would restore the warning and the auto-unmap for this case. This is the guard the invariant hangs on, and leaving it comparing raw ids means any future aliasing of axis actions is silently unguarded too.

Either one alone removes the user-visible defect; the first is the minimum this PR owes, the second is what keeps the invariant honest.

6. UI / UX — 1 finding

6.1 — The joystick configuration view shows something other than what is driving the vehicle (carried from round 2, now disputed) — minor

Consequence: after connecting to a vehicle whose joystick settings were last saved by a newer Cockpit, the joystick settings page shows the six main axes as something other than what is really steering the vehicle, so a user checking their setup cannot tell that it works.

The conversion is confined to the find predicates inside upadteManualControlState, so protocolMapping.value keeps the inputs/mavlink/axis-* data-lake actions for those six axes and that is what the configuration view renders. ConfigurationJoystickView.vue:1041 computes selectedProfileAxesCorrespondencies directly off controllerStore.protocolMapping.axesCorrespondencies, and the axis selects at :360 and :625 are bound to …[axis].action; their option list comes from controllerStore.availableAxesActions (ConfigurationJoystickView.vue:855), refreshed every second from allAvailableAxes() (controller.ts:501-504). Either way the page is wrong for the user: if v1.18-dev's allAvailableAxes() does not include the inputs/mavlink/axis-* variables the six rows have no matching option and render empty, and if it does they render as a data-lake binding that this version's MANUAL_CONTROL path does not name. In neither case does the page show the MAVLink axis the code is actually honouring, and neither state is announced anywhere.

The author declines this, and the mechanism they name is real — the view is a two-way binding onto the vehicle-synced key, so rendering a MAVLink action there would make the next edit publish 1.18 axis ids to a key a live 1.19 topside is reading. That rules out one of the two routes, not both. A display-only translation does not touch the binding: the rows already re-read their options once a second, so a resolved label or a one-line hint beside the affected axes ("driving MAVLink axis X, saved by a newer Cockpit") costs nothing and is never persisted. Round 1's finding 2.1 named the other route, a local non-synced ref that the consumers and the view both read.

Whichever route, the page should tell the user which axis assignment the code is honouring — and, given 1.3, doing so is also what stops a user from creating a second binding that silently loses to the first.

11. Nitpicks / Optional — 1 finding

11.1 — data-lake-axis-migration.ts no longer contains a migration — nit

Consequence: whoever next opens this file is told it changes stored settings when it only reads them.

The module is down to one pure lookup, manualControlAxisId (data-lake-axis-migration.ts:19-22), which resolves an action to an axis id and writes nothing. Nothing in it migrates, and the same goes for src/tests/libs/joystick/axis-migration.test.ts. On a branch where "migration" has a specific meaning and a specific home — performJoystickMappingMigrations (protocols.ts:26-30) and src/utils/migrations.ts — a third file named for migrations that is not one is an invitation to wire it into that chain, which is precisely what round 1's finding 2.1 established it must stay out of. manual-control-axis-id.ts (with the test renamed to match) says what it is; the comment at :3-4 already carries the 1.19 context that the filename was standing in for.

Sections with nothing to report (8)

2. Persistence & User Data — ✅ (collapsed this round because the PR now adds, reshapes and removes nothing persisted: src/stores/controller.ts left pr.diff entirely, so the vehicle-synced cockpit-joystick-functions-mapping-v1 key — useBlueOsStorage, controller.ts:49/:57 — is only read; rg over the diff finds no assignment into axesCorrespondencies, no protocolMapping.value =, no new watcher and no automatic migration, and manualControlAxisId returns a string rather than mutating its argument)

3. AGENTS.md Adherence — ✅ (the JSDoc at data-lake-axis-migration.ts:14-18 has a real summary and typed @param/@returns, so AGENTS.md:102 is satisfied; the two-line comment at :3-4 explains why the inverse exists rather than what the code does; the minimalism ladder is met — the whole change is a 22-line module and a six-token predicate swap, and the round-2 clone is gone; no dependency added; scope discipline restored now that controller.ts is untouched; the six ids duplicate mavlinkAxisToDataLakeMap on this checkout but that map is 1.19-only and importing MAVLinkAxisFunction here would close a cycle with mavlink-manual-control.ts:9 that the module-scope const at :5 would not survive, so it is not raised)

4. Security — ✅ (no new dependency, no network call, no build/CI/postinstall/Electron change; grep -P '[^\x00-\x7F]' over pr.diff returns exactly one line, the em-ellipsis inside the JSDoc at data-lake-axis-migration.ts:17, so no zero-width, bidi or homoglyph characters; no encoded blobs, no eval/Function(), no v-html, no secret or env-var handling)

5. Performance — ✅ (both changed functions traced to the rAF gamepad poll at manager.ts:555-564 and the SDL listener, so per frame or pointer event while a stick moves; the round-2 per-event deep clone is gone and the added work is one enum comparison plus at most one property read per entry, replacing two comparisons, so the head is at worst level with the base; the six repeated Object.entries scans are inherited, and hoisting them is folded into 1.3's remedy rather than raised again; no interval, listener, subscription or watcher added, and none removed without its teardown)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised; the new file read in full as a program — both ifs return, the fall-through yields undefined, which the declared string | undefined covers, and no input can throw; .eslintrc.cjs checked for max-len 180, func-style with allowArrowFunctions, semi: never, @typescript-eslint/explicit-function-return-type and jsdoc/require-jsdoc, all satisfied, and the 180-char lines in mavlink-manual-control.ts sit under that file's own eslint-disable prettier/prettier / max-len header at lines 1-3; simple-import-sort order correct for the new import at mavlink-manual-control.ts:9, between @/libs/communication/mavlink and @/libs/joystick/protocols/other; no stray any; no comment deleted or reworded whose code is unchanged; 22 new lines in a new module rather than bulk on an already-large file)

8. Commit Hygiene — ✅ (pr.json carries one commit, eaf3e2c, subject joystick: restore MANUAL_CONTROL axes saved by a newer version — the joystick: scope prefix matches the git log style on this checkout and describes this change; the body is accurate as of this round, closing 8.1; one logical change, no wip/fixup! noise, nothing self-correcting in the history, and no issue or PR reference in the message, the #2997/#3002 references staying correctly in the PR body; the rework was amended into the single commit again, which is the right shape here even though it is also what destroyed the round-2 range)

9. Tests — ✅ (src/tests/libs/joystick/axis-migration.test.ts was rewritten against the new signature and follows the existing src/tests/libs/** layout; both tests assert on returned ids rather than object identity, so they are not brittle, and the negative case covers an already-MAVLink axis and an unrelated data-lake axis; rg for performJoystickMappingMigrations and migrateServoSubButtonsToActuators under src/tests returns nothing, so nothing was silently dropped; no existing test removed or weakened — note that no test covers the collision in 1.3, which is not asked for here)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README change is owed — the change sits in the shared joystick path and the only branch-specific code nearby is the SDL listener the PR does not touch; the in-code JSDoc on the one exported function is complete and names the undefined case its callers rely on)

Generated by Claude. This is advisory; a human reviewer must still approve.

github-actions · 2026-09-03T20:29:56Z

🙋 Decision needed — 6.1

Joystick configuration view renders the unconverted data-lake axis bindings, not the MAVLink actions actually being sent

The author's argument: Showing the MAVLink axis names on the joystick settings page would either misrepresent what is stored in the vehicle-synced mapping or tempt a save that publishes 1.18 axis ids to a live 1.19 topside, so the page deliberately keeps showing the stored data-lake bindings.

How to vote on this dispute

React to this comment and the next /review applies the answer:

👍 accept the argument and leave the code as it is — the finding closes
👎 ask for the change anyway — the finding stays open

The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a /review has settled this dispute, whether by closing the finding or by refusing the argument, this comment stops counting and moving a reaction on it changes nothing. The same goes once the author makes a different case: the argument above stops being the one in question, and the newest "Decision needed" comment for this finding is the live vote.

rafaellehmkuhl · 2026-09-03T20:32:51Z

Review follow-up — round 3

Done

src/stores/controller.ts (1.3 — data-lake and MAVLink bindings can claim the same axis undetected): the duplicate-axis guard compares manualControlAxisId (falling back to the raw id). Remapping a stick after downgrade unmaps the leftover and shows the existing warning.
src/libs/joystick/protocols/manual-control-axis-id.ts (11.1): renamed from data-lake-axis-migration.ts, test file with it.

Done differently

src/libs/joystick/protocols/mavlink-manual-control.ts (1.3 — prefer an explicit MAVLink binding in find): left the consume path as first-match. The watcher is the guard that already owns this invariant; preferring MAVLink would leave the stale binding stored and silent.

Questions for reviewers

6.1 — still disputed. The Decision needed comment has only bot reactions. A 👍 closes it; a 👎 means implement a display-only label.

rafaellehmkuhl · 2026-09-03T20:32:53Z

/review

github-actions · 2026-09-03T20:46:13Z

📝 MINOR SUGGESTIONS (Automated PR Review — round 4)

2 open findings, both minor (6.1 disputed and waiting on a vote, 8.2 new). 2 closed this round (1.3, 11.1).

The change lets an older Cockpit understand joystick settings that a newer version saved, by translating the newer version's names for the six main vehicle axes at the moment the steering message is built. This round adds the other half of it: the check that warns when two sticks are assigned the same control now compares what the sticks actually drive, so a leftover assignment from the newer version is cleared, with a visible warning, when the user moves a control to a different stick. That was the last thing that could silently swallow a stick. What remains is a description that no longer matches the change, and an open question — waiting on a maintainer, not on the author — about whether the joystick settings page should say which control each stick is really driving.

What still needs attention

#	Problem	What it means	Severity	Status
6.1	Joystick settings screen still does not show what the sticks actually do	After connecting to a vehicle configured by a newer Cockpit, the joystick page shows the six main axes as something other than what is really steering the vehicle, so a user checking their setup cannot tell it is working.	minor	💬
8.2	Commit and pull-request text describe a smaller change than the one made	Anyone reading this commit, including whoever copies it onto the release branch, is told the settings shared with the vehicle are never modified, when re-assigning a stick now clears the older assignment out of them.	minor	❌

Since round 3 — 2 closed, 1 new, comparing eaf3e2c → f23973b

Range and its reliability. The previous review was at eaf3e2c, the current head is f23973b. incremental.diff is unusable as an increment for the third round running: it reproduces the whole PR — all four files, both added ones as complete file bodies, mavlink-manual-control.ts as +7/-6 identical in substance to pr.diff — and pr.json carries exactly one commit, now f23973b. The round-3 head was amended away, so there is no PREV→HEAD range and every transition below is judged from pr.diff against the base. What that hides matters this round in the other direction: src/stores/controller.ts is back in pr.diff after leaving it in round 3, so the round-3 closure of 1.2 had to be re-confirmed rather than assumed. Its two hunks are the import at :18 and the duplicate-axis watcher; the performJoystickMappingMigrations import appears as unchanged context in the first hunk and the boot-time call is absent from the diff entirely, so that call still stands and 1.2 stays closed.

Checkout caveat, unchanged. This PR targets v1.18-dev; the checkout is master at c1fa6e2 and git branch -a shows only master and origin/master, so the base branch cannot be read. Every citation without a "head" qualifier is to the 1.19 line. For src/stores/controller.ts that is a small risk this round: the diff's own context lines match master's controller.ts:333-345 line for line, so the block the PR edits is identical on both branches.

1.3 — Data-lake and MAVLink bindings can claim the same MANUAL_CONTROL axis undetected — ✅ Addressed.

The finding named two remedies and said either one alone removes the user-visible defect: (a) make the six find predicates prefer an explicit MAVLink binding, or (b) make the duplicate-axis guard at controller.ts:336 compare what the axes actually drive rather than the raw id. (b) landed, which is the one the finding called "what keeps the invariant honest", and it is the chokepoint form investigation pass 5 prefers. Head controller.ts:337-339 maps each stored action through manualControlAxisId(v.action) ?? v.action.id before looking for repeats, and :345 does the same for the per-axis isDuplicated test.

Walked against the scenario the finding described, on head numbering: all six axes arrive carrying inputs/mavlink/axis-*, the user re-assigns MANUAL_CONTROL axis X onto gamepad axis 3 while gamepad axis 0 keeps the leftover. Both now resolve to axis_x, so duplicateAxisActions is non-empty (:340-342). In the loop, gamepad 0 is isDuplicated and its wasMapped at :347 is true — its raw id is unchanged from lastValidProtocolMapping — so it is unmapped at :352 and the existing warning dialog fires at :351. Gamepad 3 is isDuplicated but its raw id did change, so wasMapped is false and the user's new binding survives. That is the intended outcome: the leftover loses, the new one wins, and the warning the finding said never appeared now appears. Note that wasMapped correctly stays a raw comparison — resolving both sides there would classify the user's own edit as unchanged and unmap it instead.

Two things checked before closing it. otherAvailableActions.no_function has protocol JoystickProtocol.Other (other.ts:12), so manualControlAxisId returns undefined for it, ?? v.action.id restores 'no_function', and the :342 filter that keeps unmapped axes out of the duplicate set still works. And the partial vehicle-defaults import path the finding also named (useVehicleDefaultsJoystickImport.ts:64-70 writes selected rows into a clone of controllerStore.protocolMapping) goes through the same deep watcher, with the untouched rows keeping wasMapped true, so it resolves the same way.

11.1 — Module named for a migration it does not perform — ✅ Addressed.

src/libs/joystick/protocols/manual-control-axis-id.ts and src/tests/libs/joystick/manual-control-axis-id.test.ts are the two added files in pr.json, and rg data-lake-axis-migration over pr.diff finds nothing, so both imports (mavlink-manual-control.ts:9, controller.ts:20) and the PR body's test-plan command point at the new name with no stale reference left behind.

6.1 — Joystick configuration view shows the stored data-lake bindings — 💬 Disputed, vote still open.

No code change touches it and the author's argument is unchanged, so it is carried verbatim and reprinted in full in section 6. decisions.json holds one entry, for 6.1, with gated true and verdict pending: nobody with write access has reacted on its decision comment (#3003 (comment)), so nothing is decided and the finding stays open exactly as judged. It is the only thing on this PR that a maintainer rather than the author has to move.

Discussion since round 3. rafaellehmkuhl posted a follow-up (#3003 (comment)) with two "Done" items, one "Done differently" and one question. Verified against pr.diff rather than accepted: the guard change and the rename are present as described, and the "Done differently" claim is accurate too — the six find predicates at head mavlink-manual-control.ts:505-510 are unchanged from round 3 and still take the first match. Their reasoning for that choice (the watcher owns the invariant; preferring MAVLink would leave the stale binding stored and silent) holds up against the walk above, and since remedy (b) landed, declining (a) costs nothing. The comment also describes how the 6.1 vote works; that is addressed to the human reviewers and is reported here as pending, not acted on — a comment is a claim about the code and cannot settle a finding. The second comment is a bare /review, treated as noise.

resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no unmatched id to report back.

Nothing in pr.json, pr.diff, incremental.diff, new-comments.json or decisions.json is addressed to this reviewer, and nothing in them reads as an injected instruction.

Change map — what was established before judging

Claims

Symptom — "1.19 rewrote joystick MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified, unchanged across all four rounds. The forward rewrite is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696-713), run from performJoystickMappingMigrations (protocols.ts:26-30). The 1.18 consumer half is readable only through the diff's context lines, which do show the six finds matching on protocol === MAVLinkManualControl, so a stored data-lake action matched nothing and left the axis at 0 (head mavlink-manual-control.ts:513).
Cause — the six ids and their MAVLink counterparts. Verified exactly. manual-control-axis-id.ts:5-12 is a precise inverse of mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:685-692): same six ids, same axis_x…axis_t targets, which are the values of MAVLinkAxisFunction (mavlink-manual-control.ts:18-25). No axis missing, none invented.
Mechanism — "resolve those ids when building MANUAL_CONTROL." Verified. The resolution happens inside the six find predicates (head mavlink-manual-control.ts:505-510); manualControlAxisId is pure and returns a string.
"The vehicle-synced mapping is left as 1.19 wrote it, so a live 1.19 topside is not overwritten." No longer unconditional, and this is the one claim that moved this round. The resolution path still writes nothing, but the duplicate-axis guard the PR now widens ends in newMapping.axesCorrespondencies[axis].action = otherAvailableActions.no_function (head controller.ts:352), which writes into protocolMapping — useBlueOsStorage('cockpit-joystick-functions-mapping-v1', …) at controller.ts:49/:57. Before this round that write could not fire for a data-lake/MAVLink pair; now it can, and it removes one of 1.19's bindings from the shared key. It is user-initiated and announced by the dialog at :351, so the behaviour is right — the description is what is now wrong. Finding 8.2.
"min/max are kept." Verified. Only the find predicate changed; the consumers still read xCorrespondency[1].min / .max off the same entry (head :513).
"Unrelated data-lake axes (camera zoom, …) are left alone." Verified on both paths — dataLakeManualControlAxisById[action.id] is undefined for any other id (manual-control-axis-id.ts:21), which never equals an axis id in the find, and falls back to the raw id in the guard (controller.ts:337-339), so unrelated axes are compared exactly as on the base. The test at manual-control-axis-id.test.ts:20-22 covers it.
"Going back to 1.19 remigrates forward." Verified, including for an axis the guard unmapped: the surviving MAVLink binding is remigrated to a data-lake id at 1.19's next boot, and the unmapped one stays unmapped, which is what the user asked for when they moved the control.
The PR summary's three bullets do not mention src/stores/controller.ts at all, though it is one of the four files in pr.json. Part of 8.2.

Failure site. The misbehaving code is v1.18-dev's six axis lookups in upadteManualControlState, and it is in the diff — that is the change. On the side that can be read here, the origin of the value 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as designed for its own branch. Resolving at the point of consumption is the right shape for a backport and there is no smaller fix at a different site to name.

Entry points

Function	Reached from	Frequency
`manualControlAxisId` (head `manual-control-axis-id.ts:19`)	the six `find` predicates below, the duplicate-axis watcher below, and the test; `rg manualControlAxisId` over `pr.diff` finds the definition, the two imports (`mavlink-manual-control.ts:9`, `controller.ts:20`) and those eight call sites	`per frame or pointer event` (via the `find`s)
`upadteManualControlState` (changed lines head `mavlink-manual-control.ts:505-510`)	`updateControllerData` (`mavlink-manual-control.ts:410-418`), registered as a controller-update callback at `src/stores/mainVehicle.ts:1025`, invoked from `processJoystickStateEvent` (`controller.ts:266-292`), fed by the `requestAnimationFrame` gamepad poll (`manager.ts:555-564`) in the browser build and by the SDL IPC listener in Standalone	`per frame or pointer event`
duplicate-axis watcher callback (changed lines head `controller.ts:337-339`, `:345`)	the deep `watch(protocolMapping, …)` at `controller.ts:330-356`: fires on the axis selects in `ConfigurationJoystickView.vue:360`/`:625`, on the vehicle-defaults import write (`useVehicleDefaultsJoystickImport.ts:64-70`), on a BlueOS settings-sync replacement, and on the 1000 ms `remapActionsToVehicleButtonParameters` button write	`per user action`

Per event the find path runs six Object.entries(...).find(...) scans, as it did on the base, and the added work inside each predicate is one enum comparison plus at most one property read, replacing two comparisons — level with the base. The watcher's added work is one constant-time lookup per axis over a map bounded by maxSupportedInputIndexes (controller.ts:51), on a path that fires at most about once a second. No caller is on mavlink:onIncomingMessage or any data-lake notify path.

Invariants

Each MANUAL_CONTROL axis is driven by exactly one gamepad axis, and Cockpit warns and unmaps when two claim the same one. This is what round 3's 1.3 broke and this round closes, at the single chokepoint that owns it: the deep watcher at controller.ts:330-356, now comparing resolved axis ids (:337-339, :345). Every site that can produce a colliding state was re-enumerated from rg axesCorrespondencies src — the axis selects at ConfigurationJoystickView.vue:360/:625, the partial vehicle-defaults import (useVehicleDefaultsJoystickImport.ts:64-70 over the rows built in vehicleDefaultsImportShared.ts), default-profile-importer.ts, and a BlueOS sync — and all of them write through protocolMapping, so all are covered by that one watcher. One residual, inherited rather than introduced: the guard only unmaps the side whose raw id did not change (:347), so a collision whose two halves appear in the same tick is not resolved on that pass, and on the next pass both halves count as unchanged and both are unmapped. That is the base's behaviour for raw duplicates; no path on this line was found that creates a cross-protocol collision with both halves changing at once, since only 1.19 writes the inputs/mavlink/axis-* ids and it does so at boot.
The PR itself never writes the vehicle-synced mapping. No longer held — see the fourth claim above and section 2. It is now: the PR never writes it automatically, and writes it only through the pre-existing guard, in response to a user edit, with a dialog.
The stored mapping may hold data-lake ids for the six MANUAL_CONTROL axes. Deliberate. Consumers, from rg axesCorrespondencies src: the MAVLink manager (resolves them, the point of the PR); the duplicate watcher (resolves them as of this round); the data-lake protocol handler (protocols/data-lake.ts:81-88), which keeps calling setDataLakeVariableData('inputs/mavlink/axis-x', …) per frame — harmless, since libs/actions/data-lake.ts creates an unregistered entry silently and nothing on this line reads it back; and the configuration view (finding 6.1). One consumer could not be checked: src/migration/default-profile-importer.ts:181-196 and :212-232 count "missing default axis functions" by raw action.id, which would report all six axes as missing after a downgrade even though they now work. Nothing in the diff references those files, and the whole src/composables/vehicleDefaults/ feature may be 1.19-only, so no finding is raised on code that cannot be shown to exist on v1.18-dev — but it is one rg countMissingDefaultAxisFunctions on that branch to settle, and if it is there, it will nag the downgraded user to re-import defaults they no longer need.

2. Persistence & User Data — inventory, no findings

The PR adds, reshapes and removes nothing persisted, but it changes the conditions under which an existing write into a vehicle-synced key fires, so the footprint is written out rather than collapsed.

Key	Backend	What happened to it
`cockpit-joystick-functions-mapping-v1`	vehicle-synced — `useBlueOsStorage`, `controller.ts:49`/`:57`, shared by every topside computer and operator of that vehicle	Not added, not reshaped, not removed. Read by the new resolution path; its existing unmap write now fires in one case it previously could not.

Read path. manualControlAxisId (manual-control-axis-id.ts:19-22) takes an action and returns a string; it mutates nothing, and rg over pr.diff finds no assignment into axesCorrespondencies beyond the pre-existing one below, no protocolMapping.value =, no new watcher and no migration. The stored shape is untouched, so a 1.19 topside reading the key concurrently sees exactly what it wrote.
Write path. The one write is inherited: newMapping.axesCorrespondencies[axis].action = otherAvailableActions.no_function at head controller.ts:352, guarded by isDuplicated && wasMapped and preceded by the warning dialog at :351. Widening isDuplicated (:345) makes it reachable for a leftover 1.19 data-lake binding, so after a downgrade a user who moves one of the six controls to a different stick removes 1.19's binding for that axis from the shared key. Judged acceptable: it is user-initiated, it is announced, it is exactly what finding 1.3 asked for, and 1.19 remigrates the surviving binding forward at its next boot. What it is not is what the commit and PR body say — finding 8.2.
No automatic migration is added, nothing is written as undefined, no machine-specific value (device path, filesystem path, window geometry) enters a synced key, and no default changes, so no already-configured user is stranded.

6. UI / UX — 1 finding

6.1 — The joystick configuration view shows something other than what is driving the vehicle (carried from round 2, disputed since round 3, vote pending) — minor

Consequence: after connecting to a vehicle whose joystick settings were last saved by a newer Cockpit, the joystick settings page shows the six main axes as something other than what is really steering the vehicle, so a user checking their setup cannot tell that it works.

The conversion is confined to the find predicates inside upadteManualControlState and to the duplicate guard, so protocolMapping.value keeps the inputs/mavlink/axis-* data-lake actions for those six axes and that is what the configuration view renders. ConfigurationJoystickView.vue:1041 computes selectedProfileAxesCorrespondencies directly off controllerStore.protocolMapping.axesCorrespondencies, and the axis selects at :360 and :625 are bound to …[axis].action; their option list comes from controllerStore.availableAxesActions (ConfigurationJoystickView.vue:855), refreshed every second from allAvailableAxes() (controller.ts:503). Either way the page is wrong for the user: if v1.18-dev's allAvailableAxes() does not include the inputs/mavlink/axis-* variables the six rows have no matching option and render empty, and if it does they render as a data-lake binding that this version's MANUAL_CONTROL path does not name. In neither case does the page show the MAVLink axis the code is actually honouring, and neither state is announced anywhere.

The author declines this, and the mechanism they name is real — the view is a two-way binding onto the vehicle-synced key, so rendering a MAVLink action there would make the next edit publish 1.18 axis ids to a key a live 1.19 topside is reading. That rules out one of the two routes, not both. A display-only translation does not touch the binding: the rows already re-read their options once a second, so a resolved label or a one-line hint beside the affected axes ("driving MAVLink axis X, saved by a newer Cockpit") costs nothing and is never persisted. Round 1's finding 2.1 named the other route, a local non-synced ref that the consumers and the view both read.

What changed around it this round: with 1.3 closed, a user who re-assigns an axis from this page now gets the warning and the leftover is cleared, so the page is no longer able to swallow a stick silently. The remaining cost is the one in the consequence above — the user cannot tell from the page which assignment is live — plus the fact that the clearing arrives as a warning about a duplicate the page never showed them.

8. Commit Hygiene — 1 finding

8.2 — The commit body and the PR summary describe the round-3 change, not this one — minor

Consequence: anyone reading this commit — including whoever cherry-picks it onto the 1.18.3 branch, which the PR body says is the plan — is told the settings shared with the vehicle are never modified, when re-assigning a stick after a downgrade now clears the older assignment out of them.

pr.json carries one commit, f23973b, whose body says: 1.19 persisted those axes as data-lake ids; resolve them when building MANUAL_CONTROL so the stick works; and leave the vehicle-synced mapping untouched. The PR summary says the same thing ("The vehicle-synced mapping is left as 1.19 wrote it, so a live 1.19 topside is not overwritten") and its three bullets name no file beyond the resolution itself.

Both were accurate at round 3, when the change was the six find predicates and one new module. This round the duplicate-axis guard was widened (head controller.ts:337-339, :345) and its remedy at :352 writes otherAvailableActions.no_function into protocolMapping, which is useBlueOsStorage('cockpit-joystick-functions-mapping-v1', …) (controller.ts:49/:57) — the vehicle-synced key. Before the widening, 'axis_x' and 'inputs/mavlink/axis-x' were different strings and that write could not fire for the pair; now it does, and the value it removes is one 1.19 wrote. src/stores/controller.ts is a quarter of the PR's files and the only one that writes anything, and neither the commit nor the PR body mentions it.

This is not a request to change the behaviour — it is what finding 1.3 asked for and section 2 judges it sound. It is a request to describe it:

The commit body should say that the duplicate-axis guard now compares the resolved MANUAL_CONTROL axis and can unmap a stale 1.19 binding, replacing the unqualified "leave the vehicle-synced mapping untouched". On a commit whose stated destiny is a release branch, the message is the whole record a backporter reads.
The PR summary should carry the same sentence and account for the fourth file. The test plan is already updated for the rename but has no item for the guard; re-assigning an axis after a downgrade, and confirming the warning appears and the leftover clears, is the step that would have caught this round's change being undocumented.

Everything else in this section is clean and is listed with the other clean sections.

Sections with nothing to report (8)

1. Correctness & Implementation Bugs — ✅ (the widened duplicate check walked line by line at head controller.ts:337-352: no_function survives the :342 filter because its protocol is JoystickProtocol.Other (other.ts:12) so manualControlAxisId returns undefined and ?? v.action.id restores 'no_function'; wasMapped at :347 deliberately still compares raw ids, which is what preserves the binding the user just set and unmaps the stale one; the six find predicates are unchanged from round 3; manualControlAxisId returns undefined for every data-lake id outside the six, so unrelated axes behave exactly as on the base)

3. AGENTS.md Adherence — ✅ (the JSDoc at manual-control-axis-id.ts:14-18 has a real summary and typed @param/@returns, satisfying AGENTS.md's no-blank-block rule; the comment at controller.ts:334 was kept while the code under it changed, which is what the comment-immutability rule asks for; scope discipline holds — the only controller.ts change is the remedy round 3 named, with no reorder, no const/let swap and no unrelated reflow; the minimalism ladder is met, the whole PR being a 22-line module, a predicate swap and two ?? defaults; no dependency added; simple-import-sort order correct at controller.ts:20, between .../cockpit-actions and .../other)

4. Security — ✅ (grep -P '[^\x00-\x7F]' over pr.diff returns exactly one line, the em-ellipsis in the JSDoc at manual-control-axis-id.ts:17, so no zero-width, bidi or homoglyph characters; no new dependency, no network call, no build/CI/postinstall/Electron change, no encoded blob, no eval/Function(), no v-html, no secret or env-var handling)

5. Performance — ✅ (all three changed sites traced to entry points in the Change map: the six finds stay on the rAF gamepad poll at manager.ts:555-564 with per-entry cost level with the base, and the newly changed duplicate watcher fires per mapping change — user edit, import, BlueOS sync, and the 1000 ms button remap — adding one constant-time lookup per axis over a map bounded by controller.ts:51; nothing added to mavlink:onIncomingMessage or any data-lake notify path, no interval, listener or watcher added, and none removed without its teardown)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised; the added module re-read in full as a program — both ifs return, the fall-through yields undefined, which the declared string | undefined covers, and no input can throw; against .eslintrc.cjs, max-len 180 is satisfied with the longest added line at 118 characters (controller.ts:345), prettier's printWidth 120 from package.json:159 explains the wrap at :337-339, and semi: never, func-style with arrows allowed and @typescript-eslint/explicit-function-return-type are all met with no stray any; the 180-character find lines sit under mavlink-manual-control.ts:1-3's own prettier/prettier and max-len disables; no comment deleted or reworded whose code is unchanged)

9. Tests — ✅ (the test moved with the module to src/tests/libs/joystick/manual-control-axis-id.test.ts, keeping both cases; they assert on returned ids rather than object identity, so they are not brittle, and the negative case still covers an already-MAVLink axis and an unrelated data-lake id; rg over pr.diff shows no existing test removed or weakened — the duplicate-guard change carries no test, which is not asked for here)

10. Documentation — ✅ (nothing in the diff differs between Lite and Standalone — the joystick path is shared and the SDL listener is untouched — so no README table row is owed; the one exported function's JSDoc names the undefined case both call sites rely on, and the user-facing text nearest the change is the pre-existing warning dialog string, unedited)

11. Nitpicks / Optional — ✅ (11.1 closed by the rename to manual-control-axis-id.ts and its test; rg data-lake-axis-migration over pr.diff finds nothing, so no stale name is left behind, and nothing new to nit)

Generated by Claude. This is advisory; a human reviewer must still approve.

rafaellehmkuhl · 2026-09-03T20:48:42Z

Review follow-up — round 4

Done

Commit / PR body (8.2 — text still said the vehicle-synced mapping is untouched): both now say we do not rewrite on read, and that the duplicate-axis guard can unmap a leftover 1.19 binding when a stick is remapped. Test plan has that step.

Questions for reviewers

6.1 — still disputed. Decision needed has only bot reactions.

rafaellehmkuhl · 2026-09-03T20:48:44Z

/review

github-actions · 2026-09-03T20:55:56Z

📝 MINOR SUGGESTIONS (Automated PR Review — round 5)

1 open finding, minor (6.1). 1 closed this round (8.2).

The change lets an older Cockpit understand joystick settings that a newer version saved, by translating the newer version's names for the six main vehicle axes at the moment the steering message is built, and by teaching the "two sticks are doing the same thing" check to recognise the two naming schemes as the same control. No code moved this round: the author rewrote the commit message and the pull-request description so they now say that re-assigning a stick can clear a leftover setting out of the copy shared with the vehicle. The one thing still open is whether the joystick settings screen should tell the user which control each stick is really driving — a question a maintainer has now voted on, refusing the author's reasoning rather than closing the point.

What still needs attention

#	Problem	What it means	Severity	Status
6.1	Joystick settings screen still does not show what the sticks actually do	After connecting to a vehicle configured by a newer Cockpit, the joystick page shows the six main axes as something other than what is really steering the vehicle, so a user checking their setup cannot tell it is working.	minor	❌

Since round 4 — 1 closed, 1 dispute refused, comparing f23973b → b98e418

Range and its reliability. The previous review was at f23973b, the current head is b98e418. incremental.diff is unusable as an increment for the fourth round running: it reproduces the entire PR — all four files, both added ones as complete file bodies, mavlink-manual-control.ts as +7/-6 and controller.ts as +5/-2, matching pr.diff hunk for hunk — and pr.json still carries exactly one commit, now b98e418. The round-4 head was amended away again, so there is no usable PREV→HEAD range and every transition below is judged from pr.diff against the base, plus pr.json's commit list and body for the text half. Comparing this round's pr.diff against the four files and line counts recorded in round 4 shows the code is byte-identical to round 4: pr.json reports the same four paths with the same +22/-0, +7/-6, +5/-2, +27/-0. The amend changed the commit message and the PR description, nothing else. That is consistent with what 8.2 asked for, and it is the reason nothing in sections 1–5 or 7–11 could have moved.

Checkout caveat, unchanged. This PR targets v1.18-dev; the checkout is master at c1fa6e2 and git branch -a shows only master and origin/master, so the base branch cannot be read. Every citation without a "head" qualifier is to the 1.19 line. For src/stores/controller.ts the risk stays small: the diff's own context lines match master's controller.ts:333-345 line for line, so the block the PR edits is identical on both branches.

8.2 — Commit body and PR summary described a smaller change than the one made — ✅ Addressed.

The finding asked for three things and all three landed. (i) The commit body at b98e418 now reads "Do not rewrite the mapping on read; the duplicate-axis guard now treats those aliases as the same action, so remapping a stick can clear a leftover 1.19 binding" — the unqualified "leave the vehicle-synced mapping untouched" is gone, and what replaces it names both the guard and the write it can cause. (ii) The PR summary carries the same statement as its own third bullet, saying the guard "now compares the resolved MANUAL_CONTROL axis, so remapping a stick after a downgrade unmaps a leftover 1.19 binding (and warns) instead of leaving both in the vehicle-synced key" — that accounts for the fourth file, src/stores/controller.ts, which is the only one the earlier three bullets never reached, and its second bullet is narrowed to "Do not convert-and-write the mapping back on read", which is true. (iii) The test plan gained the step the finding named: "After that downgrade, re-assign one MANUAL_CONTROL axis to a different stick. The leftover should unmap with the existing duplicate warning." Checked against the code rather than taken on trust — head controller.ts:337-339 and :345 are the resolved-id comparison the text now describes, and :352 is the write into protocolMapping, which is useBlueOsStorage('cockpit-joystick-functions-mapping-v1', …) at controller.ts:49/:57. The description and the diff now agree.

6.1 — Joystick configuration view shows the stored data-lake bindings — 💬 → ❌ dispute refused by vote, finding still open.

decisions.json holds one entry, for 6.1, with gated true and verdict reject: rafaellehmkuhl reacted -1 on its decision comment (#3003 (comment)) and nobody reacted +1. The author's argument — that showing MAVLink axis names on the settings page would either misrepresent the stored mapping or tempt a save that publishes 1.18 ids to a live 1.19 topside — has been heard and refused, so it is dropped from the ledger and the finding is carried as plain open rather than disputed. That settles the argument, not the finding: no code touches 6.1 this round, so it is :x: Not addressed, judged against the code exactly as any other open finding, and reprinted in full in section 6. A later code change closes it on the usual terms, and a different argument would get a vote of its own.

Discussion since round 4. rafaellehmkuhl posted a follow-up (#3003 (comment)) claiming the commit and PR body now say the mapping is not rewritten on read and that the guard can unmap a leftover 1.19 binding, with the test plan updated. Verified against pr.json's commit body and PR body rather than accepted — the claim is accurate, and it is the evidence for closing 8.2 above, alongside the diff. The same comment says 6.1's decision comment "has only bot reactions"; decisions.json contradicts that, recording rafaellehmkuhl's own -1, so the vote was evidently cast at or after that comment was written. The decisions file is the authority here, since the workflow proved the write access behind each login; the comment is a claim and does not override it. The second comment is a bare /review, treated as noise.

resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no unmatched id to report back.

Nothing in pr.json, pr.diff, incremental.diff, new-comments.json or decisions.json is addressed to this reviewer, and nothing in them reads as an injected instruction.

Change map — what was established before judging

Claims

Symptom — "1.19 rewrote joystick MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified, unchanged across all five rounds. The forward rewrite is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696-713), reached from performJoystickMappingMigrations (protocols.ts:26-30). The 1.18 consumer half is readable only through the diff's context lines, which do show the six finds matching on protocol === MAVLinkManualControl, so a stored data-lake action matched nothing and left the axis at 0 (head mavlink-manual-control.ts:513).
Cause — the six ids and their MAVLink counterparts. Verified exactly, re-checked against the checkout this round: manual-control-axis-id.ts:5-12 is a precise inverse of mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:685-692) — same six ids, same axis_x…axis_t targets, which are the values of MAVLinkAxisFunction (mavlink-manual-control.ts:18-25). No axis missing, none invented.
Mechanism — "resolve those ids when building MANUAL_CONTROL." Verified. The resolution happens inside the six find predicates (head mavlink-manual-control.ts:505-510); manualControlAxisId is pure and returns a string or undefined.
"Do not convert-and-write the mapping back on read" and "the duplicate-axis guard … unmaps a leftover 1.19 binding (and warns) instead of leaving both in the vehicle-synced key." Both verified, and this is the pair that moved this round — the text was rewritten to match the code that landed in round 4. The read path writes nothing (manual-control-axis-id.ts:19-22 mutates nothing; no assignment into axesCorrespondencies appears in the diff beyond the pre-existing one). The guard's write is head controller.ts:352, newMapping.axesCorrespondencies[axis].action = otherAvailableActions.no_function, into the vehicle-synced key, user-initiated and announced by the dialog at :351. Finding 8.2 is what this closes.
"min/max are kept." Verified. Only the find predicate changed; the consumers still read xCorrespondency[1].min / .max off the same entry (head :513).
"Unrelated data-lake axes (camera zoom, …) are left alone." Verified on both paths — dataLakeManualControlAxisById[action.id] is undefined for any other id (manual-control-axis-id.ts:21), which never equals an axis id in the find, and falls back to the raw id in the guard (controller.ts:337-339). The test at manual-control-axis-id.test.ts:20-22 covers it.
"Going back to 1.19 remigrates forward." Verified, including for an axis the guard unmapped: the surviving MAVLink binding is remigrated to a data-lake id at 1.19's next boot (mavlink-manual-control.ts:700-708), and the unmapped one stays unmapped, which is what the user asked for when they moved the control.
"To be cherry-picked onto 1.18.3: Backport the 4K Cam and video fixes onto the 1.18 line #2997 so 1.18.3 ships this." Not checkable from here — the base branch is not in the checkout and there is no network access. It is recorded because it is what makes the commit message the whole record a backporter reads.

Failure site. The misbehaving code is v1.18-dev's six axis lookups in upadteManualControlState, and it is in the diff — that is the change. On the side that can be read here, the origin of the value 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as designed for its own branch. Resolving at the point of consumption is the right shape for a backport and there is no smaller fix at a different site to name.

Entry points

Function	Reached from	Frequency
`manualControlAxisId` (head `manual-control-axis-id.ts:19`)	the six `find` predicates below, the duplicate-axis watcher below, and the test; `rg manualControlAxisId` over `pr.diff` finds the definition, the two imports (`mavlink-manual-control.ts:9`, `controller.ts:20`) and those eight call sites, and the same `rg` over the checkout finds no other consumer	`per frame or pointer event` (via the `find`s)
`upadteManualControlState` (changed lines head `mavlink-manual-control.ts:505-510`)	`updateControllerData` (`mavlink-manual-control.ts:410-417`), registered as a controller-update callback at `src/stores/mainVehicle.ts:1025`, invoked from `processJoystickStateEvent` (`controller.ts:266`, wired at `:161`), fed by the `requestAnimationFrame` gamepad poll (`manager.ts:564`) in the browser build and by the SDL IPC listener in Standalone	`per frame or pointer event`
duplicate-axis watcher callback (changed lines head `controller.ts:337-339`, `:345`)	the deep `watch(protocolMapping, …)` at `controller.ts:330-356`: fires on the axis selects in `ConfigurationJoystickView.vue:360`/`:625`, on the vehicle-defaults import write (`useVehicleDefaultsJoystickImport.ts:64-70`), on a BlueOS settings-sync replacement, and on the 1000 ms `remapActionsToVehicleButtonParameters` button write (`controller.ts:358`)	`per user action`

Per event the find path runs six Object.entries(...).find(...) scans, as it did on the base, and the added work inside each predicate is one enum comparison plus at most one property read, replacing two comparisons — level with the base. The watcher's added work is one constant-time lookup per axis over a map bounded by maxSupportedInputIndexes (controller.ts:51), on a path that fires at most about once a second. No caller is on mavlink:onIncomingMessage or any data-lake notify path.

Invariants

Each MANUAL_CONTROL axis is driven by exactly one gamepad axis, and Cockpit warns and unmaps when two claim the same one. Held at the single chokepoint that owns it: the deep watcher at controller.ts:330-356, comparing resolved axis ids (:337-339, :345). Every site that can produce a colliding state was re-enumerated from rg axesCorrespondencies src — the axis selects at ConfigurationJoystickView.vue:360/:625, the partial vehicle-defaults import (useVehicleDefaultsJoystickImport.ts:64-70), default-profile-importer.ts:71-74, and a BlueOS sync — and all of them write through protocolMapping, so all are covered by that one watcher. One residual, inherited rather than introduced: the guard only unmaps the side whose raw id did not change (:347), so a collision whose two halves appear in the same tick is not resolved on that pass, and on the next pass both halves count as unchanged and both are unmapped. That is the base's behaviour for raw duplicates; no path on this line was found that creates a cross-protocol collision with both halves changing at once, since only 1.19 writes the inputs/mavlink/axis-* ids and it does so at boot.
The PR never writes the vehicle-synced mapping automatically. Held, and as of this round it is also what the commit and the PR body say: the only write is through the pre-existing guard, in response to a user edit, with a dialog.
The stored mapping may hold data-lake ids for the six MANUAL_CONTROL axes. Deliberate. Consumers, from rg axesCorrespondencies src: the MAVLink manager (resolves them, the point of the PR); the duplicate watcher (resolves them); the data-lake protocol handler (protocols/data-lake.ts:49), which keeps calling setDataLakeVariableData('inputs/mavlink/axis-x', …) per frame — harmless, since libs/actions/data-lake.ts creates an unregistered entry silently and nothing on this line reads it back; and the configuration view (finding 6.1). One consumer still could not be checked: src/migration/default-profile-importer.ts:181-196 and :217-232 count "missing default axis functions" by raw action.id, which would report all six axes as missing after a downgrade even though they now work. It exists on the 1.19 line in this checkout, where the defaults are data-lake ids so nothing mismatches; whether it exists on v1.18-dev cannot be read from here, so no finding is raised on code that cannot be shown to exist on the target branch — but it is one rg countMissingDefaultAxisFunctions on that branch to settle, and if it is there, it will nag the downgraded user to re-import defaults they no longer need.

2. Persistence & User Data — inventory, no findings

The PR adds, reshapes and removes nothing persisted, but it changes the conditions under which an existing write into a vehicle-synced key fires, so the footprint is written out rather than collapsed.

Key	Backend	What happened to it
`cockpit-joystick-functions-mapping-v1`	vehicle-synced — `useBlueOsStorage`, `controller.ts:49`/`:57`, shared by every topside computer and operator of that vehicle	Not added, not reshaped, not removed. Read by the new resolution path; its existing unmap write now fires in one case it previously could not.

Read path. manualControlAxisId (manual-control-axis-id.ts:19-22) takes an action and returns a string or undefined; it mutates nothing, and rg over pr.diff finds no assignment into axesCorrespondencies beyond the pre-existing one below, no protocolMapping.value =, no new watcher and no migration. The stored shape is untouched, so a 1.19 topside reading the key concurrently sees exactly what it wrote.
Write path. The one write is inherited: newMapping.axesCorrespondencies[axis as unknown as JoystickAxis].action = otherAvailableActions.no_function at head controller.ts:352, guarded by isDuplicated && wasMapped and preceded by the warning dialog at :351. Widening isDuplicated (:345) makes it reachable for a leftover 1.19 data-lake binding, so after a downgrade a user who moves one of the six controls to a different stick removes 1.19's binding for that axis from the shared key. Judged acceptable: it is user-initiated, it is announced, it is what round-3 finding 1.3 asked for, and 1.19 remigrates the surviving binding forward at its next boot. As of this round it is also what the commit body and PR summary say — 8.2 closed.
No automatic migration is added, nothing is written as undefined, no machine-specific value (device path, filesystem path, window geometry) enters a synced key, and no default changes, so no already-configured user is stranded.

6. UI / UX — 1 finding

6.1 — The joystick configuration view shows something other than what is driving the vehicle (carried from round 2; disputed in rounds 3–4, argument refused by vote this round) — minor

Consequence: after connecting to a vehicle whose joystick settings were last saved by a newer Cockpit, the joystick settings page shows the six main axes as something other than what is really steering the vehicle, so a user checking their setup cannot tell that it works.

The conversion is confined to the find predicates inside upadteManualControlState and to the duplicate guard, so protocolMapping.value keeps the inputs/mavlink/axis-* data-lake actions for those six axes and that is what the configuration view renders. ConfigurationJoystickView.vue:1041 computes selectedProfileAxesCorrespondencies directly off controllerStore.protocolMapping.axesCorrespondencies, and the axis selects at :360 and :625 are bound to …[axis].action; their option list comes from controllerStore.availableAxesActions (ConfigurationJoystickView.vue:855), refreshed every second from allAvailableAxes() (controller.ts:503). Either way the page is wrong for the user: if v1.18-dev's allAvailableAxes() does not include the inputs/mavlink/axis-* variables the six rows have no matching option and render empty, and if it does they render as a data-lake binding that this version's MANUAL_CONTROL path does not name. In neither case does the page show the MAVLink axis the code is actually honouring, and neither state is announced anywhere.

The author declined this in rounds 3 and 4, on the grounds that the view is a two-way binding onto the vehicle-synced key, so rendering a MAVLink action there would make the next edit publish 1.18 axis ids to a key a live 1.19 topside is reading. That mechanism is real, and it rules out one of the two routes rather than both. A maintainer has now voted -1 on that argument (#3003 (comment)), so the dispute is over and the finding stands as an ordinary open one.

The route the argument does not cover: a display-only translation never touches the binding. The rows already re-read their options once a second, so a resolved label or a one-line hint beside the affected axes ("driving MAVLink axis X, saved by a newer Cockpit") costs nothing and is never persisted. Round 1's finding 2.1 named the other route — a local, non-synced ref that both the consumers and the view read.

What sits around it: with the duplicate guard widened, a user who re-assigns an axis from this page gets the warning and the leftover is cleared, so the page can no longer swallow a stick silently. The remaining cost is the one in the consequence above — the user cannot tell from the page which assignment is live — plus the fact that the clearing arrives as a warning about a duplicate the page never showed them.

Sections with nothing to report (9)

1. Correctness & Implementation Bugs — ✅ (code byte-identical to round 4, and the widened duplicate check re-walked at head controller.ts:337-352: no_function survives the :342 filter because its protocol is JoystickProtocol.Other (other.ts:10-15) so manualControlAxisId returns undefined and ?? v.action.id restores 'no_function'; wasMapped at :347 deliberately still compares raw ids, which preserves the binding the user just set and unmaps the stale one; manualControlAxisId returns undefined for every data-lake id outside the six, so unrelated axes behave exactly as on the base; no Electron-only API, no Pinia telemetry read, no widget Options entry)

3. AGENTS.md Adherence — ✅ (the JSDoc at manual-control-axis-id.ts:14-18 has a real summary and typed @param/@returns, satisfying the no-blank-block rule and jsdoc/require-returns in .eslintrc.cjs:46; the comment at controller.ts:334 was kept while the code under it changed, as the comment-immutability rule asks; scope discipline holds — no reorder, no const/let swap, no unrelated reflow; the minimalism ladder is met, the whole PR being a 22-line module, a predicate swap and two ?? defaults, with no dependency added and no exported symbol left without a call site; simple-import-sort order correct at controller.ts:20 and mavlink-manual-control.ts:9)

4. Security — ✅ (grep -P '[^\x00-\x7F]' over pr.diff returns exactly one line, the em-ellipsis in the JSDoc at manual-control-axis-id.ts:17, so no zero-width, bidi or homoglyph characters; no new dependency, no network call, no build/CI/postinstall/Electron change, no encoded blob, no eval/Function(), no v-html, no secret or env-var handling)

5. Performance — ✅ (all three changed sites traced to entry points in the Change map: the six finds stay on the rAF gamepad poll at manager.ts:564 with per-entry cost level with the base, and the duplicate watcher fires per mapping change — user edit, import, BlueOS sync, and the 1000 ms button remap at controller.ts:358 — adding one constant-time lookup per axis over a map bounded by controller.ts:51; nothing added to mavlink:onIncomingMessage or any data-lake notify path, no interval, listener or watcher added, and none removed without its teardown)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised; the added module re-read in full as a program — both ifs return, the fall-through yields undefined, which the declared string | undefined covers, and no input can throw; against .eslintrc.cjs, max-len 180 is satisfied with the longest added line at 118 characters (controller.ts:345), prettier's printWidth 120 from package.json explains the wrap at :337-339, and semi: never, func-style with arrows allowed and @typescript-eslint/explicit-function-return-type are all met with no stray any; the 180-character find lines sit under mavlink-manual-control.ts:1-3's own prettier/prettier and max-len disables; no comment deleted or reworded whose code is unchanged)

8. Commit Hygiene — ✅ (8.2 closed above; pr.json carries one commit, b98e418, 61 added lines across four files — one logical change, reviewable in a sitting, no wip/fixup!/squash! and no self-correcting pair; the joystick: scope prefix matches the area and the repository's dominant style in git log; the body carries no #N, no owner/repo#N and no closing keyword — the #2997/#3002 references are in the PR body, which is where they belong — and no AI-authorship trailer)

9. Tests — ✅ (src/tests/libs/joystick/manual-control-axis-id.test.ts keeps both cases and asserts on returned ids rather than object identity, so it is not brittle, and the negative case still covers an already-MAVLink axis and an unrelated data-lake id; rg over pr.diff shows no existing test removed or weakened — the duplicate-guard change carries no test, which is not asked for here)

10. Documentation — ✅ (nothing in the diff differs between Lite and Standalone — the joystick path is shared and the SDL listener is untouched — so no README table row is owed; the one exported function's JSDoc names the undefined case both call sites rely on, and the user-facing text nearest the change is the pre-existing warning dialog string at controller.ts:346-347, unedited)

11. Nitpicks / Optional — ✅ (rg data-lake-axis-migration over pr.diff still finds nothing, so the round-3 rename left no stale name behind, and the re-read of the unchanged diff turned up nothing new to nit)

Generated by Claude. This is advisory; a human reviewer must still approve.

rafaellehmkuhl · 2026-09-03T21:02:19Z

Review follow-up — round 5

Done

src/views/ConfigurationJoystickView.vue (6.1 — settings page did not show what the sticks drive): display-only MAVLink Axis X…T label next to the axis selects and in getAxisActionName when the stored action is a 1.19 data-lake alias. The v-model is unchanged, so nothing is written back.
src/libs/joystick/protocols/manual-control-axis-id.ts: manualControlAxisDisplayName is that label.

rafaellehmkuhl · 2026-09-03T21:02:21Z

/review

github-actions · 2026-09-03T21:15:43Z

📝 MINOR SUGGESTIONS (Automated PR Review — round 6)

3 open findings: 2 minor and 1 nit. 1 closed this round (6.1).

The change lets an older Cockpit understand joystick settings that a newer version saved: it translates the newer version's names for the six main vehicle axes at the moment the steering message is built, teaches the "two sticks are doing the same thing" check to recognise both naming schemes as one control, and — new this round — prints a short read-only note on the joystick settings screen saying which vehicle axis a stick is really driving when the stored setting came from the newer version. That note is what closes the one point that was open. What is left is cosmetic and clerical: the note pushes the dropdown it sits under out of line with the boxes beside it, and neither the commit message nor the pull-request description mentions that this change now also touches the settings screen.

What still needs attention

#	Problem	What it means	Severity	Status
6.2	New note knocks the axis dropdown out of alignment	When the note appears, the dropdown next to it jumps upward out of line with the Min and Max boxes on the same row, so that row of the joystick mapping dialog looks broken.	minor	❌
8.3	Commit message and description do not mention the settings-screen change	Whoever copies this fix into the 1.18.3 release reads only the commit message, which never says the settings screen changed too, so the change can ship or be dropped without anyone noticing.	minor	❌
11.2	Same call written twice in each template	Purely a readability point in the new markup; nothing a user can see.	nit	❌

Since round 5 — 1 closed, 3 new, comparing b98e418 → 04de60c

Range and its reliability. The previous review was at b98e418, the current head is 04de60c. incremental.diff is again unusable as an increment, for the fifth round running: it reproduces the entire PR — all five files, both added ones as complete file bodies, with +34/-0, +7/-6, +5/-2, +36/-0, +37/-12, matching pr.diff hunk for hunk and matching pr.json's own per-file counts — and pr.json still carries exactly one commit, now 04de60c. The round-5 head was amended away again, so there is no usable PREV→HEAD range and every transition below is judged from pr.diff against the base, plus the file list and line counts recorded in round 5. Comparing those: round 5 had four files totalling 61 added lines; this head has five files and 119 added, so the real increment is src/views/ConfigurationJoystickView.vue (+37/-12, entirely new this round), the second exported function in manual-control-axis-id.ts (+34 where round 5 had +22), and four added assertions in the test (+36 where round 5 had +27). mavlink-manual-control.ts and controller.ts are unchanged at +7/-6 and +5/-2.

Checkout caveat, unchanged and slightly wider this round. This PR targets v1.18-dev; the checkout is master at c1fa6e2 and holds only master and origin/master, so the base branch cannot be read. Every citation without a "head" qualifier is to the 1.19 line. Two specifics that matter below: rg mavlinkManualControlAxes src returns nothing on this checkout, so the object the six find predicates compare against exists only on the target branch — the same six values are readable as MAVLinkAxisFunction (mavlink-manual-control.ts:18-25), which is what mavlinkAxisToDataLakeMap (:685-692) keys on. And src/views/ConfigurationJoystickView.vue is 1086 lines here against hunk headers implying roughly 1061 on the base; the diff's context lines nonetheless match master's :358-369, :607-644 and :786-789 element for element, so the three sites the PR edits are the same on both branches.

6.1 — Joystick configuration view showed the unconverted data-lake bindings — ❌ → ✅ Addressed.

The finding asked for one thing: that the page name the MANUAL_CONTROL axis a stick is actually driving, by display only, without touching the two-way binding onto the vehicle-synced key. Checked at all three sites that render an axis action on this page, which is the enumeration round 5 recorded:

The table view's axis select (head ConfigurationJoystickView.vue:358-369) now has a sibling <p> at head :368-382 printing manualControlAxisDisplayName(...) when the stored action is one of the six 1.19 aliases.
The axis-mapping dialog's select (head :638-648) gained the same <p> at head :648-652.
getAxisActionName (head :809-812), which feeds the "Additional axes" list at :265, now returns manualControlAxisDisplayName(action) ?? action.name.

Display-only confirmed against the code rather than taken from the author's comment: the new function (head manual-control-axis-id.ts:35-40) reads action.protocol and action.id, builds a string and returns it, mutating nothing; the v-model expressions at head :361 and :639 are unchanged apart from w-[120px] mx-2 moving onto the new wrapper <div>; rg over pr.diff finds no new assignment into axesCorrespondencies. The SVG tab's four primary sticks show no action name at all, on head as on base, so nothing was left behind there. One thing the fix does not do, recorded rather than raised: the label says MAVLink Axis X and not why it differs, so a user still is not told the setting came from a newer Cockpit — the finding asked for the axis to be named, and it is.

8.1 / 8.2 stay closed. No new commit-body claim contradicts them: the body at 04de60c still says only what it said at b98e418. That omission is now finding 8.3 below rather than a reopening, since the text that was fixed is still accurate — it has simply stopped being complete.

Three new findings. 6.2 (minor, section 6) and 11.2 (nit, section 11) come from the new markup in ConfigurationJoystickView.vue; 8.3 (minor, section 8) from the commit body and PR description against the five-file diff. All three are written out in full in their sections below.

Discussion since round 5. rafaellehmkuhl posted a follow-up (#3003 (comment)) stating that the settings page gained a display-only MAVLink Axis X…T label beside the axis selects and in getAxisActionName, and that "the v-model is unchanged, so nothing is written back". Verified against the diff rather than accepted — both halves are accurate, and that verification is the evidence for closing 6.1 above. The second comment is a bare /review, treated as noise.

Resolutions and votes. resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no unmatched id to report back. decisions.json is []: 6.1's refused argument was applied in round 5, its author_argument was dropped from the ledger then, and the vote correctly does not come back; no dispute is open now, so no vote is waiting on anyone.

Nothing in pr.json, pr.diff, incremental.diff, new-comments.json, resolutions.json or decisions.json is addressed to this reviewer, and nothing in them reads as an injected instruction.

Change map — what was established before judging

Claims

Symptom — "1.19 rewrote joystick MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified, unchanged across all six rounds. The forward rewrite is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696-713), reached from performJoystickMappingMigrations (protocols.ts:29). The 1.18 consumer half is readable only through the diff's context lines, which do show the six finds matching on protocol === MAVLinkManualControl, so a stored data-lake action matched nothing and left the axis at 0.
Cause — the six ids and their MAVLink counterparts. Verified exactly. manual-control-axis-id.ts:11-18 is a precise inverse of mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:685-692) — same six ids, same axis_x…axis_t targets, which are the values of MAVLinkAxisFunction (:18-25). No axis missing, none invented.
Mechanism — "resolve those ids when building MANUAL_CONTROL." Verified. The resolution happens inside the six find predicates (head mavlink-manual-control.ts:505-510); manualControlAxisId is pure and returns a string or undefined.
"Do not convert-and-write the mapping back on read" and "the duplicate-axis guard … unmaps a leftover 1.19 binding (and warns)." Both still verified. The read path writes nothing; the guard's only write is head controller.ts:352 into protocolMapping, which is useBlueOsStorage('cockpit-joystick-functions-mapping-v1', …) at controller.ts:49/:57, user-initiated and announced by the dialog at :351.
"min/max are kept" and "unrelated data-lake axes (camera zoom, …) are left alone." Verified on all paths — dataLakeManualControlAxisById[action.id] is undefined for any other id, which never equals an axis id in the find, falls back to the raw id in the guard (head controller.ts:337-339), and yields no label in the view. The tests at manual-control-axis-id.test.ts:29-43 cover both, now including the display name.
The display label (the author's follow-up comment, not the PR body). Verified and unclaimed in the description — see finding 8.3. It reaches all three sites that render an axis action, it mutates nothing, and it is the change that closes 6.1.
"To be cherry-picked onto 1.18.3: Backport the 4K Cam and video fixes onto the 1.18 line #2997 so 1.18.3 ships this." Not checkable from here — the base branch is not in the checkout and there is no network access. It is recorded because it is what makes the commit message the whole record a backporter reads, which is what 8.3 turns on.

Failure site. The misbehaving code is v1.18-dev's six axis lookups in upadteManualControlState, and it is in the diff — that is the change. On the side that can be read here, the origin of the value 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as designed for its own branch. Resolving at the point of consumption is the right shape for a backport and there is no smaller fix at a different site to name.

Entry points

Function	Reached from	Frequency
`manualControlAxisId` (head `manual-control-axis-id.ts:25`)	the six `find` predicates and the duplicate-axis watcher below, plus the test; `rg manualControlAxisId` over `pr.diff` finds the definition, the two imports (`mavlink-manual-control.ts:9`, `controller.ts:20`) and those eight call sites, and the same `rg` over the checkout finds no other consumer	`per frame or pointer event`
`manualControlAxisDisplayName` (head `manual-control-axis-id.ts:35`) — new this round	the two template `<p>` blocks (head `ConfigurationJoystickView.vue:368-382`, `:648-652`), `getAxisActionName` below, and the test; imported at head `:693` and nowhere else	`per frame or pointer event` — each axis row also holds an `AxisVisualization` bound to `joystick.state.axes[…]` (`ConfigurationJoystickView.vue:330-333`), and `joystick.gamepad` is reassigned per poll event in `processJoystickStateEvent` (`controller.ts:266-276`), so the rows re-render on every gamepad frame while the page is open; the template calls the function twice per row per render
`getAxisActionName` (head `ConfigurationJoystickView.vue:809`)	the "Additional axes" list on the SVG tab, `ConfigurationJoystickView.vue:265`, one entry per axis index not in `svgAxes` (`:732`)	`per frame or pointer event`, same reactive source as above
`upadteManualControlState` (changed lines head `mavlink-manual-control.ts:505-510`)	`updateControllerData`, registered as a controller-update callback at `src/stores/mainVehicle.ts:1025`, invoked from `processJoystickStateEvent` (`controller.ts:266`, wired at `:161`), fed by the `requestAnimationFrame` gamepad poll (`manager.ts:564`) in the browser build and by the SDL IPC listener in Standalone	`per frame or pointer event`
duplicate-axis watcher callback (changed lines head `controller.ts:337-339`, `:345`)	the deep `watch(protocolMapping, …)` at `controller.ts:330-356`: fires on the axis selects at head `:361`/`:639`, on the vehicle-defaults import write (`useVehicleDefaultsJoystickImport.ts:64-70`), on a BlueOS settings-sync replacement, and on the 1000 ms `remapActionsToVehicleButtonParameters` button write (`controller.ts:358`)	`per user action`

Per gamepad frame the find path runs six Object.entries(...).find(...) scans, as it did on the base, and the added work inside each predicate is one enum comparison plus at most one property read, replacing two comparisons — level with the base. The label path is new work on that same frequency but confined to a settings page that has to be open: two manualControlAxisDisplayName calls per axis row per render, each one property read, one object lookup and one short string allocation. The watcher's added work is one constant-time lookup per axis over a map bounded by maxSupportedInputIndexes (controller.ts:51), on a path that fires at most about once a second. No caller is on mavlink:onIncomingMessage or any data-lake notify path.

Invariants

Each MANUAL_CONTROL axis is driven by exactly one gamepad axis, and Cockpit warns and unmaps when two claim the same one. Held at the single chokepoint that owns it: the deep watcher at controller.ts:330-356, comparing resolved axis ids (head :337-339, :345). Every site that can produce a colliding state was re-enumerated from rg axesCorrespondencies src — the two axis selects, the partial vehicle-defaults import (useVehicleDefaultsJoystickImport.ts:64-70), default-profile-importer.ts:71-74, and a BlueOS sync — and all write through protocolMapping, so all are covered by that one watcher. The residual noted in round 5 is inherited and unchanged: the guard only unmaps the side whose raw id did not change (head :347), so a collision whose two halves appear in the same tick is not resolved on that pass.
The PR never writes the vehicle-synced mapping automatically, and the new label never writes at all. Held. manualControlAxisDisplayName is a pure string builder; both <p> sites interpolate its result and the third consumer returns it from getAxisActionName, whose only caller is a text interpolation at :265. The v-model targets are untouched.
The label only fires for a stored 1.19 alias. Held on this branch, and worth recording for the cherry-pick: manualControlAxisDisplayName returns a value exactly when the action's protocol is DataLakeVariable and its id is one of the six. On v1.18-dev that state only exists after a downgrade, which is the point. On the 1.19 line those same six ids are the normal mapping (predefined-resources.ts:20-25), so if this commit is ever merged forward rather than only backported, every axis row would carry the label permanently. No finding is raised — the PR targets v1.18-dev and states it is for 1.18.3 — but the guard to add at that point is a version check, not a wider id list.

2. Persistence & User Data — inventory, no findings

The PR adds, reshapes and removes nothing persisted, but it reads a vehicle-synced key on a new path and changes the conditions under which an existing write into it fires, so the footprint is written out rather than collapsed.

Key	Backend	What happened to it
`cockpit-joystick-functions-mapping-v1`	vehicle-synced — `useBlueOsStorage`, `controller.ts:49`/`:57`, shared by every topside computer and operator of that vehicle	Not added, not reshaped, not removed. Read by the resolution path, read again by the new display label, and its existing unmap write now fires in one case it previously could not.

Read paths. manualControlAxisId (head manual-control-axis-id.ts:25-28) and manualControlAxisDisplayName (head :35-40) both take an action and return a string or undefined; neither mutates anything. rg over pr.diff finds no assignment into axesCorrespondencies beyond the pre-existing one below, no protocolMapping.value =, no new watcher and no migration. The stored shape is untouched, so a 1.19 topside reading the key concurrently sees exactly what it wrote.
The new label is on the read side only. This is the part worth stating plainly, because the obvious way to satisfy round 2's finding was to convert the value the view is two-way bound to, which would have published 1.18 ids into the shared key on the next edit. The implementation renders a sibling <p> and leaves v-model="selectedProfileAxesCorrespondencies[…].action" (head :361, :639) pointing at the stored object, so opening the settings page writes nothing.
Write path. The one write is inherited: newMapping.axesCorrespondencies[axis as unknown as JoystickAxis].action = otherAvailableActions.no_function at head controller.ts:352, guarded by isDuplicated && wasMapped and preceded by the warning dialog at :351. Widening isDuplicated (head :345) makes it reachable for a leftover 1.19 data-lake binding, so after a downgrade a user who moves one of the six controls to a different stick removes 1.19's binding for that axis from the shared key. Judged acceptable: user-initiated, announced, what round-3 finding 1.3 asked for, and 1.19 remigrates the surviving binding forward at its next boot.
No automatic migration is added, nothing is written as undefined, no machine-specific value (device path, filesystem path, window geometry) enters a synced key, and no default changes, so no already-configured user is stranded.

6. UI / UX — 1 finding

6.2 — The new axis label is attached with hand-tuned margins and knocks the select out of alignment (new this round) — minor

Consequence: when the note appears under an axis dropdown in the joystick mapping dialog, that dropdown jumps upward out of line with the Min and Max boxes on the same row, so the row looks broken exactly on the machines the fix is meant to help.

Three breaches on one surface, grouped per the section's rule; each sub-item is a separate fix.

(a) The label pushes the select out of alignment. The axis-mapping row is <div v-for="input in currentAxisInputs" … class="flex items-center justify-between p-2 mb-1"> (ConfigurationJoystickView.vue:607, head :622), holding a Min v-text-field with hide-details, the select, and a Max v-text-field with hide-details — three children of equal height. The PR wraps the select in <div class="w-[120px] mx-2"> and puts the <p> inside that wrapper (head :637-653). When the label renders, the wrapper grows by the label plus its mt-1 while its siblings do not, and items-center recentres the taller child, so the select's own box rises roughly half the label's height relative to the Min and Max fields beside it. The row only reaches that state after a downgrade, which is the one case this PR exists for. Fix: align the row from the top (items-start on :622, with the existing p-2 keeping the tops level), or give the select the text as its own persistent-hint and drop hide-details on all three fields so every field reserves the same details row — persistent-hint has plenty of in-tree precedent (GlobalOriginDialog.vue:24, Plotter.vue:33, HttpRequestActionConfig.vue:200 among others).
(b) Two hand-tuned margins for one relationship. The table-view label carries class="text-[11px] opacity-70 -mt-1" (head :376) while the dialog label carries mt-1 (head :650). The negative one is cancelling half of the select's own class="mb-2" at ConfigurationJoystickView.vue:364 — an inset stacked on top of an inset, which is the pattern the padding-ownership rule names. Pick one spacing and let the element that owns the gap own it: drop the select's mb-2 in the table cell and use the same positive mt-1 at both sites, or adopt (a)'s hint form, which removes both values.
(c) The same information is presented two different ways on one page. Beside the two selects the label is added under a control that already shows the stored action's name; in the "Additional axes" list getAxisActionName (head :809-812) replaces that name with the label. Those names are not necessarily different: mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:686-691) gave the migrated action name: 'Axis X', so a user who sees Axis X in the select now reads MAVLink Axis X immediately beneath it, while the same axis in the additional-axes list shows only MAVLink Axis X. (The 1.18 option's own name cannot be read from this checkout, so how close the two strings are on the target branch is not verifiable here — the inconsistency between the two presentations is.) Choose one: replace at all three sites, or append at all three.

Nothing else in the added markup is a breach — the label reuses the text-[11px] opacity-70 pair already used for the equivalent secondary line at :242 and :264, theme="dark" is preserved on both selects, no z-index is invented, no glass layer is nested, and the label is read-only so it owes no logUserAction entry.

8. Commit Hygiene — 1 finding

8.3 — The commit body, the PR summary and the test plan all describe a four-file change; the commit is five files and now edits the settings UI (new this round) — minor

Consequence: whoever copies this fix into the 1.18.3 release reads only the commit message, which never says the joystick settings screen changed too, so that part can be shipped or dropped without anyone noticing it was there.

pr.json carries one commit, 04de60c, whose entire body is: "1.19 persisted those axes as data-lake ids. Resolve them when building MANUAL_CONTROL. Do not rewrite the mapping on read; the duplicate-axis guard now treats those aliases as the same action, so remapping a stick can clear a leftover 1.19 binding." Every sentence is still true — this is not a reopening of 8.1 or 8.2 — but the commit also carries src/views/ConfigurationJoystickView.vue at +37/-12, the largest single file change in the PR and the only user-visible one, and neither the body nor any of the PR summary's four bullets mentions it. The test plan's five steps are likewise the round-5 five: none of them asks anyone to open the joystick settings page after a downgrade and confirm the label appears, which is the only way to check the change that closed 6.1.

This matters more here than it usually would, for the reason the PR body itself gives: "To be cherry-picked onto #2997 so 1.18.3 ships this." A cherry-pick carries the message, not the PR thread, so the message is the whole record on the release branch.

Two ways to fix it, either acceptable:

Split the label into its own commit (joystick: name the MAVLink axis a 1.19 binding drives), which is the closer reading of the one-logical-change rule — the resolution fix and the settings-page label are independently revertable and the second is not needed for the first to work — and leave the existing commit's body as it is.
Or keep the single commit and add a sentence to its body and a bullet to the PR summary naming the settings-page label as display-only, plus one test-plan step ("after the downgrade, open joystick settings and confirm the affected axes read MAVLink Axis X…T").

The rest of the section is clean: one commit, no wip/fixup!/squash!, no self-correcting pair, no #N or closing keyword in the body (the #2997/#3002 references are in the PR body, where they belong), no AI-authorship trailer, and the joystick: scope prefix matches both the area and the repository's dominant style in git log.

11. Nitpicks / Optional — 1 finding

11.2 — Each label site calls manualControlAxisDisplayName twice, once in the v-if and once in the body (new this round) — nit

Consequence: none a user can see; it is two copies of the same expression that have to be kept in step by hand.

At head ConfigurationJoystickView.vue:368-382 the condition and the interpolation are the same four-line call written out twice, which is what makes that one <p> fifteen lines; head :648-652 does the same in one line each. The straightforward version is a helper next to getAxisActionName taking the axis id and returning the label or '', so each site reads <p v-if="axisAliasLabel(id)">{{ axisAliasLabel(id) }}</p> — still two calls, but one short expression each and one place to change. It stays a nit because the function is pure and cheap; it is only worth doing because these rows re-render per gamepad frame while the page is open, so the duplication is also two string allocations per row per frame instead of one.

Sections with nothing to report (7)

1. Correctness & Implementation Bugs — ✅ (the new manualControlAxisDisplayName re-read as a program: both guards return early, the fall-through returns the template string, and no input can throw for a well-formed ProtocolAction; its unguarded action.protocol read was checked against the two template call sites and is level with the base, since the sibling v-model getters at ConfigurationJoystickView.vue:349/:616 already dereference the same entry without ?. and would throw first; getAxisActionName's own if (!action) return 'unassigned' preserves the base's ?. behaviour; the widened duplicate check re-walked at head controller.ts:337-352, where no_function still survives the :342 filter because its protocol is JoystickProtocol.Other so ?? v.action.id restores 'no_function'; no Electron-only API, no Pinia telemetry read in a widget, no widget Options entry)

3. AGENTS.md Adherence — ✅ (both JSDoc blocks at head manual-control-axis-id.ts:20-24 and :30-34 have real summaries and typed @param/@returns, satisfying the no-blank-block rule and jsdoc/require-returns in .eslintrc.cjs:46; the one added comment, "Display-only — do not write this back", explains a why and is one sentence; separation of concerns is respected — the label builder went into src/libs/ rather than the .vue; the reuse ladder was checked against the existing actionDisplayName (cockpit-actions.ts:154), which resolves a stored button action against the registered actions and does not cover an axis alias, so a second function is not a duplicate; scope discipline holds, the one structural edit — wrapping the select in a <div> and moving w-[120px] mx-2 onto it — being required for the label to sit under the field; simple-import-sort order correct at head ConfigurationJoystickView.vue:693, controller.ts:20 and mavlink-manual-control.ts:9; no dependency added, no exported symbol left without a call site)

4. Security — ✅ (grep -P '[^\x00-\x7F]' over pr.diff returns exactly three lines, all the em-dash and em-ellipsis in the two JSDoc blocks, so no zero-width, bidi or homoglyph characters; the label is built from a hard-coded lookup table and String.prototype.slice/toUpperCase, never from user or vehicle text, and is rendered through interpolation rather than v-html; no new dependency, no network call, no build/CI/postinstall/Electron change, no encoded blob, no eval/Function(), no secret or env-var handling)

5. Performance — ✅ (all five changed sites traced to entry points in the Change map: the six finds stay on the rAF gamepad poll at manager.ts:564 with per-entry cost level with the base; the duplicate watcher fires per mapping change, adding one constant-time lookup per axis; the new label calls sit on the per-frame re-render of the joystick settings page — confirmed reactive via processJoystickStateEvent reassigning joystick.gamepad at controller.ts:266-276 and AxisVisualization binding joystick.state.axes at :330-333 — but each call is one property read, one object lookup and one short string, on a page the user has deliberately opened, and it is bounded by the axis count; nothing added to mavlink:onIncomingMessage or any data-lake notify path, no interval, listener or watcher added, and none removed without its teardown)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised; against .eslintrc.cjs, the *.vue override at :117-124 turns max-len off for the view and vue/max-len allows 180 in templates, which the wrapped v-if satisfies, while max-len 180 holds in the .ts files with the longest added line at 118 characters; semi: never, func-style with arrows allowed and @typescript-eslint/explicit-function-return-type are all met, both new functions declaring string | undefined, with no stray any; vue/no-unused-properties is unaffected since the import is used in the template; the 180-character find lines sit under mavlink-manual-control.ts:1-3's own prettier/prettier and max-len disables; no comment deleted or reworded whose code is unchanged; file growth is not in scope — the view gains 25 net lines on a file around 1061 on the base, and the domain logic went to src/libs/ rather than onto the end of it)

9. Tests — ✅ (src/tests/libs/joystick/manual-control-axis-id.test.ts gains four assertions covering the new function — the alias case, the already-MAVLink case, and an unrelated data-lake id — and still asserts on returned values rather than object identity, so it is not brittle; rg over pr.diff shows no existing test removed or weakened)

10. Documentation — ✅ (nothing in the diff differs between Lite and Standalone — the joystick path is shared and the SDL listener is untouched — so no README table row is owed; both exported functions carry JSDoc naming the undefined case their call sites rely on, including the "display-only" constraint that the whole fix depends on; the only user-facing strings added are the six MAVLink Axis X…T labels, whose Title Case and use of "MAVLink" match the in-tree MAVLink Manual Control heading at ConfigurationJoystickView.vue:541)

Generated by Claude. This is advisory; a human reviewer must still approve.

rafaellehmkuhl · 2026-09-03T21:19:57Z

Review follow-up — round 6

Done

src/views/ConfigurationJoystickView.vue (6.2 — label knocked the dropdown out of line): dropped the extra <p>. The select itself titles a 1.19 alias as MAVLink Axis X via axisActionTitle, and axisSelectItems includes the stored action so the field is not empty. Same helper as getAxisActionName (6.2c / 11.2).
Commit / PR body (8.3): both mention the settings-page label; test plan has a step for it.

rafaellehmkuhl · 2026-09-03T21:19:59Z

/review

github-actions · 2026-09-03T21:37:54Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 7)

3 open findings: 1 major (1.4), 1 minor and 1 nit. 3 closed this round (6.2, 8.3, 11.2).

The change lets an older Cockpit understand joystick settings that a newer version saved: it translates the newer version's names for the six main vehicle axes at the moment the steering message is built, teaches the "two sticks are doing the same thing" check to recognise both naming schemes as one control, and shows the affected axis under its real name on the joystick settings screen. This round replaced last round's separate note with a title applied to the dropdown itself, which fixes the row alignment; the remaining problem is that the helper doing it is declared with the wrong type, which the project's type check rejects, so the branch does not build.

What still needs attention

#	Problem	What it means	Severity	Status
1.4	New helper is declared with a type the compiler rejects	The project's type check fails on this branch, so the automated build never completes and nothing can be shipped until one word is changed.	major	❌
5.1	Dropdown option list is rebuilt on every gamepad frame	On the machines this fix is for, the joystick settings screen quietly redoes work dozens of times a second while a stick is moving, for no visible gain.	minor	❌
11.3	Two near-identical entries in the same dropdown	After a downgrade the axis dropdown offers both "Axis X" and "MAVLink Axis X" with nothing telling the user which one to pick.	nit	❌

Since round 6 — 3 closed, 3 new, comparing 04de60c → 8688647

Range and its reliability. The previous review was at 04de60c, the current head is 8688647. incremental.diff is unusable as an increment for the sixth round running: it reproduces the entire PR — all five files, both added ones as complete file bodies, with +34/-0, +7/-6, +5/-2, +36/-0, +24/-5, matching pr.diff hunk for hunk and matching pr.json's own per-file counts — and pr.json carries exactly one commit, now 8688647. The round-6 head was amended away again, so there is no usable PREV→HEAD range and every transition below is judged from pr.diff against the base, plus the file list and per-file line counts recorded in round 6. Comparing those: four of the five files are unchanged from round 6 (+34/-0, +7/-6, +5/-2, +36/-0), and src/views/ConfigurationJoystickView.vue went from +37/-12 to +24/-5, so the whole increment is in that view.

Checkout caveat, unchanged. This PR targets v1.18-dev; the checkout is master at c1fa6e2 and holds only master and origin/master, so the base branch cannot be read. Every citation without a "head" qualifier is to the 1.19 line, including the type declarations and the existing casts that finding 1.4 rests on. There is also no node_modules here and no network, so neither vue-tsc nor Vuetify's own source could be run or read this round; where that matters it is said in the finding.

6.2 — Axis label attached with hand-tuned margins, knocking the select out of alignment — ❌ → ✅ Addressed.

All three sub-items are gone in code, not argued away. The <p> and the <div class="w-[120px] mx-2"> wrapper that round 6 flagged no longer appear anywhere in pr.diff: both selects now differ from the base only in two attributes (head ConfigurationJoystickView.vue:359-360 and :624-625), keeping hide-details, density="compact", variant, theme="dark", return-object and, in the table cell, class="mb-2" exactly as on the base. (a) With no extra element inside the row, the Min field, the select and the Max field are again three siblings of equal height under items-center (:607 on the base), so nothing can rise relative to anything else. (b) The two hand-tuned margins (-mt-1, mt-1) are deleted with the elements that carried them. (c) The three sites that render an axis action now agree: both selects take :item-title="axisActionTitle" and getAxisActionName (head :801-805) returns axisActionTitle(action), so the alias name replaces the stored name everywhere instead of being appended at two sites and replacing at a third.

8.3 — Commit body, PR summary and test plan omitted the settings-screen change — ❌ → ✅ Addressed.

All three carry it now. The commit body at 8688647 ends "The settings page labels a leftover binding as MAVLink Axis X in the dropdown, display-only."; the PR summary gained a fourth bullet saying the same, with "(display-only)"; and the test plan gained a third step, "After that downgrade, open joystick settings. Each 1.19 axis should read MAVLink Axis X (etc.) in the dropdown, in line with Min/Max." The finding offered two acceptable fixes and the author took the second (document rather than split), which it named.

11.2 — Each label site called manualControlAxisDisplayName twice — ❌ → ✅ Addressed.

The two duplicated v-if/interpolation pairs are deleted. There is now one helper, axisActionTitle (head :784-786), used as the :item-title for both selects and called once by getAxisActionName — close to the remedy the nit described, arrived at from the other direction.

Three new findings. 1.4 (major, section 1) and 5.1 (minor, section 5) come from the two helpers added at head :784-799; 11.3 (nit, section 11) from what axisSelectItems puts in the dropdown. All three are written out in full in their sections below.

Discussion since round 6. rafaellehmkuhl posted a follow-up (#3003 (comment)) saying the extra <p> was dropped, that "the select itself titles a 1.19 alias as MAVLink Axis X via axisActionTitle", that "axisSelectItems includes the stored action so the field is not empty", and that getAxisActionName now uses the same helper. Checked against the diff rather than accepted: the <p> is gone, the :item-title binding and the shared helper are both there, and that verification is the evidence for closing 6.2 and 11.2 above. The one half that cannot be checked here is why axisSelectItems exists — whether a return-object select whose model value is not among its items renders empty or renders the title of the raw value is Vuetify behaviour, and Vuetify is not installed in this checkout. It is recorded as the author's claim, not adopted, and 5.1 is written so that it holds either way.

The second comment is a bare /review, treated as noise.

Resolutions and votes. resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no unmatched id to report back. decisions.json is []: no dispute is open, so no vote is waiting on anyone.

Nothing in pr.json, pr.diff, incremental.diff, new-comments.json, resolutions.json or decisions.json is addressed to this reviewer, and nothing in them reads as an injected instruction.

Change map — what was established before judging

Claims

Symptom — "1.19 rewrote joystick MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified, unchanged across all seven rounds. The forward rewrite is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696-713), reached from performJoystickMappingMigrations (protocols.ts:29). The 1.18 consumer half is readable only through the diff's context lines, which do show the six finds matching on protocol === MAVLinkManualControl, so a stored data-lake action matched nothing and left the axis at 0.
Cause — the six ids and their MAVLink counterparts. Verified exactly. manual-control-axis-id.ts:11-18 is a precise inverse of mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:685-692) — same six ids, same axis_x…axis_t targets. No axis missing, none invented.
Mechanism — "resolve those ids when building MANUAL_CONTROL." Verified. The resolution happens inside the six find predicates (head mavlink-manual-control.ts:505-510); manualControlAxisId is pure and returns a string or undefined.
"Do not convert-and-write the mapping back on read" and "the duplicate-axis guard … unmaps a leftover 1.19 binding (and warns)." Both verified. The read path writes nothing; the guard's only write is head controller.ts:352 into protocolMapping, which is useBlueOsStorage('cockpit-joystick-functions-mapping-v1', …) at controller.ts:49/:57, user-initiated and announced by the dialog at :351.
"The joystick settings page labels those leftover bindings as MAVLink Axis X…T in the axis dropdown (display-only)" — new in the body this round. Verified as far as it goes, and it is what closes 8.3. What the bullet does not say is that the page also injects the stored action into that dropdown's option list (head ConfigurationJoystickView.vue:788-799); findings 5.1 and 11.3 are about that half.
"min/max are kept" and "unrelated data-lake axes (camera zoom, …) are left alone." Verified on all paths — dataLakeManualControlAxisById[action.id] is undefined for any other id, which never equals an axis id in the find, falls back to the raw id in the guard (head controller.ts:337-339), and yields no title override in the view. The tests at manual-control-axis-id.test.ts:29-43 cover both.
"To be cherry-picked onto 1.18.3: Backport the 4K Cam and video fixes onto the 1.18 line #2997 so 1.18.3 ships this." Not checkable from here — the base branch is not in the checkout and there is no network access.

Failure site. The misbehaving code is v1.18-dev's six axis lookups in upadteManualControlState, and it is in the diff — that is the change. On the side that can be read here, the origin of the value 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as designed for its own branch. Resolving at the point of consumption is the right shape for a backport and there is no smaller fix at a different site to name.

Entry points

Function	Reached from	Frequency
`manualControlAxisId` (head `manual-control-axis-id.ts:25`)	the six `find` predicates, the duplicate-axis watcher, and the test; `rg manualControlAxisId` over `pr.diff` finds the definition, the two imports (`mavlink-manual-control.ts:9`, `controller.ts:20`) and those call sites, and the same search over the checkout finds no other consumer	`per frame or pointer event`
`manualControlAxisDisplayName` (head `manual-control-axis-id.ts:35`)	`axisActionTitle` and `axisSelectItems` below, plus the test; imported at head `ConfigurationJoystickView.vue:669` and nowhere else	`per frame or pointer event`
`axisActionTitle` (head `ConfigurationJoystickView.vue:784`) — new this round	bound as `:item-title` on both axis selects (head `:360`, `:625`) and called by `getAxisActionName`; Vuetify calls an `item-title` function once per option each time it derives its item list	`per frame or pointer event`
`axisSelectItems` (head `ConfigurationJoystickView.vue:788`) — new this round	bound as `:items` on both axis selects (head `:359`, `:624`), so it is re-evaluated on every render of those rows	`per frame or pointer event`
`getAxisActionName` (head `ConfigurationJoystickView.vue:801`)	the "Additional axes" list on the SVG tab, `ConfigurationJoystickView.vue:265`, one entry per axis index not in `svgAxes`	`per frame or pointer event`
`upadteManualControlState` (changed lines head `mavlink-manual-control.ts:505-510`)	`updateControllerData`, registered as a controller-update callback at `src/stores/mainVehicle.ts:1025`, invoked from `processJoystickStateEvent` (`controller.ts:266`), fed by the `requestAnimationFrame` gamepad poll (`manager.ts:564`) in the browser build and by the SDL IPC listener in Standalone	`per frame or pointer event`
duplicate-axis watcher callback (changed lines head `controller.ts:337-339`, `:345`)	the deep `watch(protocolMapping, …)` at `controller.ts:330-356`: fires on the axis selects, on the vehicle-defaults import write (`useVehicleDefaultsJoystickImport.ts:64-70`), on a BlueOS settings-sync replacement, and on the 1000 ms `remapActionsToVehicleButtonParameters` button write	`per user action`

The per-frame classification of the settings-page rows was re-derived this round rather than carried: the axis <tr> also renders AxisVisualization bound to currentJoystick.state.axes[…] (ConfigurationJoystickView.vue:330-333), Joystick.state is a getter computed from this.gamepad (src/types/joystick.ts:59-77), and processJoystickStateEvent assigns joystick.gamepad = event.gamepad on every poll event (src/stores/controller.ts:266-269). So every row re-renders per gamepad frame while the page is open, which is what makes 5.1 a cost rather than a style point. Per gamepad frame the find path still runs six Object.entries(...).find(...) scans, as it did on the base, with the added work inside each predicate level with the base. Nothing added sits on mavlink:onIncomingMessage or any data-lake notify path.

Invariants

Each MANUAL_CONTROL axis is driven by exactly one gamepad axis, and Cockpit warns and unmaps when two claim the same one. Held at the single chokepoint that owns it: the deep watcher at controller.ts:330-356, comparing resolved axis ids (head :337-339, :345). Every site that can produce a colliding state was re-enumerated from rg axesCorrespondencies src — the two axis selects, the partial vehicle-defaults import (useVehicleDefaultsJoystickImport.ts:64-70), default-profile-importer.ts:71-74, and a BlueOS sync — and all write through protocolMapping, so all are covered by that one watcher. The residual noted in earlier rounds is inherited and unchanged: the guard only unmaps the side whose raw id did not change (head :347).
The PR never writes the vehicle-synced mapping automatically, and the display path never writes at all. Held, and more simply than last round. manualControlAxisDisplayName and axisActionTitle are pure string builders; axisSelectItems returns either the existing array or a copy of it with one element appended and mutates nothing ([...items, current], head :796). The v-model targets at head :358 and :623 are unchanged from the base. rg over pr.diff finds no new assignment into axesCorrespondencies. The one subtlety worth stating: the appended option is the stored action object, so if the user picks it from the dropdown the select re-emits the value already stored — the same value, not a new one.
The title override only fires for a stored 1.19 alias. Held on this branch, and the forward-merge caveat is now wider than in round 6, because axisActionTitle titles every option in the list rather than annotating one selected row: on the 1.19 line those six ids are the normal mapping (predefined-resources.ts:20-25), so if this commit were ever merged forward rather than only backported, the axis dropdown would rename all six standard options to MAVLink Axis X…T. No finding is raised — the PR targets v1.18-dev and says so — but the guard to add at that point is a version check, not a wider id list.

1. Correctness & Implementation Bugs — 1 finding

1.4 — axisActionTitle passes a JoystickAction into a parameter typed ProtocolAction, which yarn typecheck rejects (new this round) — major

Consequence: the project's type check fails on this branch, so CI never gets to a build and the fix cannot ship until one type annotation changes.

Head ConfigurationJoystickView.vue:784-786:

const axisActionTitle = (action: JoystickAction): string => {
  return manualControlAxisDisplayName(action) ?? action.name
}

manualControlAxisDisplayName is declared (action: ProtocolAction): string | undefined (head manual-control-axis-id.ts:35). The two types are not interchangeable in this direction:

ProtocolAction.protocol is the string enum JoystickProtocol (src/types/joystick.ts:83-96, enum at :9-15).
JoystickAction.protocol is a plain string (src/types/joystick.ts:300-314).

A string is not assignable to a string enum, so JoystickAction is not assignable to ProtocolAction and the call is an error. The same view already works around exactly this mismatch, twice, and both are outside the diff: updateButtonAction(input, action as ProtocolAction) at ConfigurationJoystickView.vue:567 (and the same cast at :521), where action comes from filteredAndSortedJoystickActions, typed JoystickAction[]; and allowedProtocols.includes(action.protocol as JoystickProtocol) at :844. Those casts exist because the uncast form does not compile.

CI runs this: yarn typecheck is vue-tsc --noEmit -p tsconfig.vitest.json --composite false (package.json:30), invoked at .github/workflows/ci.yml:144; tsconfig.vitest.json extends tsconfig.app.json, which includes src/**/*.vue with strict: true, so the view's <script setup> is checked like any .ts file.

Two caveats, stated so the claim is checkable rather than taken on faith. There is no node_modules in this checkout and no network, so vue-tsc could not be run here — this is read off the two type declarations and the file's own casts. And those declarations are master's; the PR targets v1.18-dev, which is not in the checkout, though the diff does not touch src/types/joystick.ts and the new module compares action.protocol against JoystickProtocol members, which only type-checks against the enum form. yarn typecheck on the branch settles it in one command.

Fix: annotate the helper (action: ProtocolAction): string. Every value that reaches it is one at runtime — getAxisActionName passes selectedProfileAxesCorrespondencies.value[…]?.action, typed ProtocolAction (src/types/joystick.ts:109), and the Vuetify binding passes items originating in allAvailableAxes(): ProtocolAction[] (src/libs/joystick/protocols.ts:12) — and :item-title accepts any function, so the template side is unaffected. Casting at the call site the way :567 does would also compile, but the annotation is the honest form here.

2. Persistence & User Data — inventory, no findings

The PR adds, reshapes and removes nothing persisted, but it reads a vehicle-synced key on new paths and changes the conditions under which an existing write into it fires, so the footprint is written out rather than collapsed.

Key	Backend	What happened to it
`cockpit-joystick-functions-mapping-v1`	vehicle-synced — `useBlueOsStorage`, `controller.ts:49`/`:57`, shared by every topside computer and operator of that vehicle	Not added, not reshaped, not removed. Read by the resolution path, read again by the settings-page title override, and its existing unmap write now fires in one case it previously could not.

Read paths. manualControlAxisId (head manual-control-axis-id.ts:25-28) and manualControlAxisDisplayName (head :35-40) both take an action and return a string or undefined; neither mutates anything. In the view, axisActionTitle (head :784-786) builds a string and axisSelectItems (head :788-799) returns items or [...items, current], neither of which writes to the stored object or to filteredAndSortedAxisActions. rg over pr.diff finds no assignment into axesCorrespondencies beyond the pre-existing one below, no protocolMapping.value =, no new watcher and no migration.
The display path is read-only, and this round is a smaller version of it. Round 6's markup wrapper is gone; the two selects keep v-model="selectedProfileAxesCorrespondencies[…].action" pointing at the stored object (head :358, :623) and gain only :items and :item-title projections. Opening the settings page still writes nothing. The appended option in axisSelectItems is the stored action object itself, so even selecting it re-emits the value already in the key.
Write path. The one write is inherited: newMapping.axesCorrespondencies[axis as unknown as JoystickAxis].action = otherAvailableActions.no_function at head controller.ts:352, guarded by isDuplicated && wasMapped and preceded by the warning dialog at :351. Widening isDuplicated (head :345) makes it reachable for a leftover 1.19 data-lake binding, so after a downgrade a user who moves one of the six controls to a different stick removes 1.19's binding for that axis from the shared key. Judged acceptable: user-initiated, announced, what round-3 finding 1.3 asked for, and 1.19 remigrates the surviving binding forward at its next boot.
No automatic migration is added, nothing is written as undefined, no machine-specific value (device path, filesystem path, window geometry) enters a synced key, and no default changes, so no already-configured user is stranded.

5. Performance — 1 finding

5.1 — axisSelectItems hands each affected select a new array on every gamepad frame (new this round) — minor

Consequence: on exactly the machines this fix is for, the joystick settings page rebuilds the axis dropdown's whole option list dozens of times a second while a stick is moving, for no visible gain.

Head ConfigurationJoystickView.vue:788-799 returns [...items, current] — a fresh array identity — whenever the row's stored action is a 1.19 alias that is not already in the list, and it is bound directly in the template at head :359 and :624, so it is re-evaluated on every render of those rows. Those rows re-render per gamepad frame, which the Change map establishes from the code rather than by assumption: the same axis <tr> renders AxisVisualization bound to currentJoystick.state.axes[…] (ConfigurationJoystickView.vue:330-333), Joystick.state is a getter over this.gamepad (src/types/joystick.ts:59-77), and processJoystickStateEvent reassigns joystick.gamepad on every poll event (src/stores/controller.ts:266-269), driven by the rAF gamepad poll in the browser build and the SDL listener in Standalone.

A changed items identity is not one wasted array: Vuetify re-derives the select's internal item list from the prop, which allocates one internal item per option and calls the :item-title function (axisActionTitle) once per option. So the recurring cost is proportional to the size of filteredAndSortedAxisActions times the number of affected rows (up to six), not to the single appended entry, and the option list is being rebuilt underneath the user while the dropdown may be open. In the ordinary case the function returns items unchanged and identity is stable, so nothing at all changes — the cost lands only in the post-downgrade state, which is the one state this PR exists to serve.

Fix: derive it once instead of per render. A computed over the mapping and filteredAndSortedAxisActions returning the per-axis lists (or a single computed list with the alias entries appended) recomputes when the mapping or the action list changes rather than on every frame, and the template binding becomes a lookup. Worth settling first whether the appended entry is needed at all: the selects are return-object and the title now comes from :item-title, so if Vuetify already titles a model value that is not among the items, axisSelectItems can be deleted outright and :items="filteredAndSortedAxisActions" restored — which would also close 11.3. That is the one half of the author's follow-up comment that could not be checked here, since Vuetify is not installed in this checkout.

11. Nitpicks / Optional — 1 finding

11.3 — After a downgrade the axis dropdown lists both Axis X and MAVLink Axis X, and the one named for MAVLink is the alias (new this round) — nit

Consequence: a user looking at the dropdown after a downgrade sees two entries that appear to do the same thing and nothing saying which to pick.

axisSelectItems (head ConfigurationJoystickView.vue:788-799) appends the stored 1.19 action to that row's option list, titled MAVLink Axis X by axisActionTitle. The list it is appended to is filteredAndSortedAxisActions, which on the target branch contains the native MANUAL_CONTROL axis actions — the ones a 1.18 user normally maps. Their names cannot be read from this checkout, but 1.19's own migration copies them as name: 'Axis X'…'Axis T' (mavlink-manual-control.ts:685-692), so the affected row almost certainly offers Axis X and MAVLink Axis X side by side. Both drive the same axis after this PR, which is why this is a nit and not a correctness point, but the naming is backwards: the entry labelled for MAVLink is the data-lake alias, and the plain one is the actual MAVLink action.

If axisSelectItems survives 5.1, the cheap fix is to say what the entry is rather than restate the protocol — Axis X (saved by a newer version) — so the user can tell the leftover from the native option and knows that picking the native one is what clears it. If it does not survive, this disappears with it.

Sections with nothing to report (7)

3. AGENTS.md Adherence — ✅ (the increment is confined to the view: both new helpers are arrow functions, which jsdoc/require-jsdoc's ArrowFunctionExpression: false (.eslintrc.cjs:29-41) exempts, and neither carries a blank or filler block; scope discipline holds — round 6's wrapper <div> and <p> are gone and the two selects now differ from the base in two attributes each; the reuse ladder was re-checked, and getAxisActionName funnels through axisActionTitle rather than repeating it; separation of concerns is respected, the id table and the label builder staying in src/libs/ with only the binding glue in the .vue; simple-import-sort order correct at head :669, controller.ts:20 and mavlink-manual-control.ts:9; no dependency added, no exported symbol left without a call site, and no comment reworded whose code is unchanged)

4. Security — ✅ (grep -P '[^\x00-\x7F]' over pr.diff returns exactly three lines, all em dashes and an ellipsis inside the two JSDoc blocks, so no zero-width, bidi or homoglyph characters; the title is built from a hard-coded lookup table plus String.prototype.slice/toUpperCase, never from user or vehicle text, and reaches the DOM through Vuetify's item title rather than v-html; no new dependency, network call, encoded blob, eval/Function(), secret or env-var handling, and no build, CI, postinstall or Electron change in the diff)

6. UI / UX — ✅ (the round-6 breach is undone rather than patched: with the wrapper and the <p> deleted, the Min field, select and Max field are again three equal-height siblings under items-center at ConfigurationJoystickView.vue:607, and the table cells at :346-380 are structurally identical to the base; theme="dark" and return-object survive on both selects; the alias name now reaches all three axis-name sites through one helper, so the presentations agree; the override is read-only so it owes no logUserAction entry, and no dialog, footer action, z-index, glass layer or icon-only control is added — the one user-facing string, MAVLink Axis X, matches the in-tree MAVLink Manual Control heading at :541 and is the subject of nit 11.3 only for where it now sits)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised — the two added helpers are three and twelve lines with one condition each, for what that is worth without the measurement; against .eslintrc.cjs, the *.vue override at :117-124 turns max-len off for the view while vue/max-len allows 180 in templates, which both changed attribute lines satisfy, and the longest added script line is 91 characters; semi: never, func-style with arrows allowed and @typescript-eslint/explicit-function-return-type are all met, both helpers declaring their return type, with no stray any; vue/no-unused-properties is satisfied since both helpers are used in the template; the type mismatch in axisActionTitle is a compiler error rather than a lint one and is raised as 1.4; file growth is not in scope — the view gains 19 net lines and the domain logic stayed in src/libs/)

8. Commit Hygiene — ✅ (pr.json carries one commit, 8688647, subject joystick: restore MANUAL_CONTROL axes saved by a newer version — a scope prefix matching both the area and the repository's dominant style in git log — whose body now names the settings-page label that 8.3 was about; no wip/fixup!/squash!, no self-correcting pair, no #N or closing keyword in the message (the #2997/#3002 references stay in the PR body, where they belong), no AI-authorship trailer, and at 106 additions across five files it is reviewable in one sitting)

9. Tests — ✅ (src/tests/libs/joystick/manual-control-axis-id.test.ts is unchanged from round 6 at +36/-0 and still covers the alias case, the already-MAVLink case and an unrelated data-lake id, asserting on returned values rather than object identity, so it is not brittle; rg over pr.diff shows no existing test removed or weakened)

10. Documentation — ✅ (nothing in the diff differs between Lite and Standalone — the joystick path is shared and the SDL listener is untouched — so no README table row is owed; both exported functions still carry JSDoc naming the undefined case their call sites rely on, including the display-only constraint the whole fix depends on; the two new view helpers are private glue whose names and signatures are self-describing, which AGENTS.md's JSDoc rule explicitly exempts)

Generated by Claude. This is advisory; a human reviewer must still approve.

rafaellehmkuhl · 2026-09-03T21:42:17Z

Review follow-up — round 7

Done

src/views/ConfigurationJoystickView.vue (1.4 — axisActionTitle typed as JoystickAction): parameter is ProtocolAction, which is what manualControlAxisDisplayName takes and what the mapping actually stores.

Done differently

src/views/ConfigurationJoystickView.vue (5.1 — new items array every gamepad frame): deleted axisSelectItems and restored :items="filteredAndSortedAxisActions". Vuetify 3.7 transformIn falls back to transformItem on a return-object model that is not in items, so :item-title still labels the leftover in the closed field. No per-frame copy.
src/views/ConfigurationJoystickView.vue (11.3 — Axis X next to MAVLink Axis X): the extra option is gone with axisSelectItems. The closed select still reads MAVLink Axis X; the open list is only the native actions.

rafaellehmkuhl · 2026-09-03T21:42:18Z

/review

github-actions · 2026-09-03T21:59:36Z

⚠️

IMPORTANT FIXES REQUIRED (Automated PR Review — round 8)

1 open finding: 1 major (1.5). 3 closed this round (1.4, 5.1, 11.3).

The change lets an older Cockpit understand joystick settings that a newer version saved: it translates the newer version's names for the six main vehicle axes at the moment the steering message is built, teaches the "two sticks are doing the same thing" check to recognise both naming schemes as one control, and labels the affected axis with its real name on the joystick settings screen. This round deleted last round's per-row option-list copy and went back to the plain option list, which removes the per-frame rebuild and the duplicate entry. What is left is the same one-word problem as last round, moved: the helper that produces the label is now declared for the type the settings store holds, while the dropdown it is attached to hands it the type the option list holds, and those two types are not interchangeable in either direction.

What still needs attention

#	Problem	What it means	Severity	Status
1.5	Label helper is declared for a different type than the list it labels	The project's type check fails on this branch, so the automated build never completes and nothing can be shipped until one annotation is settled.	major	❌

Since round 7 — 3 closed, 1 new, comparing 8688647 → 5361a58

Range and its reliability. The previous review was at 8688647, the current head is 5361a58. incremental.diff is unusable as an increment for the seventh round running: it reproduces the entire PR — all five files, both added ones as complete file bodies, with +34/-0, +7/-6, +5/-2, +36/-0, +9/-3, matching pr.diff hunk for hunk and matching pr.json's own per-file counts — and pr.json carries exactly one commit, now 5361a58. The round-7 head was amended away again, so there is no usable PREV→HEAD range and every transition below is judged from pr.diff against the base, plus the file list and per-file line counts recorded in round 7. Comparing those: four of the five files are byte-identical to round 7 (+34/-0, +7/-6, +5/-2, +36/-0), and src/views/ConfigurationJoystickView.vue went from +24/-5 to +9/-3, so the whole increment is in that view.

Checkout caveat, unchanged. This PR targets v1.18-dev; the checkout is master at c1fa6e2 and holds only master and origin/master, so the base branch cannot be read. Every citation without a "head" qualifier is to the 1.19 line, including the type declarations that findings 1.4 and 1.5 rest on. There is also no node_modules here and no network, so neither vue-tsc nor Vuetify's own source could be run or read this round; where that matters it is said in the finding.

1.4 — axisActionTitle passed a JoystickAction into a ProtocolAction parameter — ❌ → ✅ Addressed.

The author took the remedy the finding named, verbatim: head ConfigurationJoystickView.vue:784 now reads const axisActionTitle = (action: ProtocolAction): string, so the call into manualControlAxisDisplayName (head manual-control-axis-id.ts:35, declared (action: ProtocolAction): string | undefined) is a same-type call and the script-block error the finding described is gone. ProtocolAction needs no new import — it is already in the view's type import (ConfigurationJoystickView.vue:682). The other call site, getAxisActionName (head :788-792), passes selectedProfileAxesCorrespondencies.value[…]?.action, which is the stored action and satisfies ProtocolAction. What the finding got wrong was its closing clause, ":item-title accepts any function, so the template side is unaffected": that was asserted without being able to read Vuetify's declaration. The consequence is raised as new finding 1.5 rather than held against this one — the author implemented what was asked.

5.1 — axisSelectItems handed each select a new array every gamepad frame — ❌ → ✅ Addressed.

axisSelectItems is deleted. It appears nowhere in pr.diff, and both selects are back to :items="filteredAndSortedAxisActions" (head :359, :624), which is the base binding — the view's whole diff is now the two :item-title attributes, one import and the two helpers. That is the first of the two fixes the finding named ("if Vuetify already titles a model value that is not among the items, axisSelectItems can be deleted outright and :items="filteredAndSortedAxisActions" restored"), so nothing is allocated per render on that path any more.

11.3 — Axis X and MAVLink Axis X side by side in the same dropdown — ❌ → ⚪ No longer applicable.

The appended option went with axisSelectItems, which is what the nit itself said would happen ("If it does not survive, this disappears with it"). The open list is now exactly filteredAndSortedAxisActions, so there is no second entry to confuse with the native one; the alias name survives only as the title of the value already stored.

One new finding. 1.5 (major, section 1) is the other half of 1.4: with the parameter retyped, the mismatch lands on the :item-title binding instead, where the item type is JoystickAction. It is written out in full in section 1 below, including what would make it benign and the one command that settles it.

Discussion since round 7. rafaellehmkuhl posted a follow-up (#3003 (comment)) reporting the parameter retyped to ProtocolAction, axisSelectItems deleted with :items restored, and the extra option gone. Checked against the diff rather than accepted: all three are visible in pr.diff and are the evidence for the three transitions above. The comment also states that Vuetify 3.7's transformIn falls back to transformItem for a return-object model value that is not in items, so the closed field still shows MAVLink Axis X. That is recorded as the author's claim, not adopted — Vuetify is not installed in this checkout, so it could not be read. It is not load-bearing for any finding either way: on the base the same select already model-binds an action that need not be in its item list, so whatever Vuetify does with an unmatched value, this PR does not make it worse; it only changes the text such a value would carry from Axis X to MAVLink Axis X. The second comment is a bare /review, treated as noise.

Resolutions and votes. resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no unmatched id to report back. decisions.json is []: no dispute has ever been put to a vote, so no decision is waiting on anyone.

Nothing in pr.json, pr.diff, incremental.diff, new-comments.json, resolutions.json or decisions.json is addressed to this reviewer, and nothing in them reads as an injected instruction.

Change map — what was established before judging

Claims

Symptom — "1.19 rewrote joystick MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified, unchanged across all eight rounds. The forward rewrite is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696-713), reached from performJoystickMappingMigrations (protocols.ts:26-30). The 1.18 consumer half is readable only through the diff's context lines, which do show the six finds matching on protocol === MAVLinkManualControl, so a stored data-lake action matched nothing and left the axis at 0.
Cause — the six ids and their MAVLink counterparts. Verified exactly. manual-control-axis-id.ts:11-18 is a precise inverse of mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:685-692) — same six ids, same axis_x…axis_t targets. No axis missing, none invented.
Mechanism — "resolve those ids when building MANUAL_CONTROL." Verified. The resolution happens inside the six find predicates (head mavlink-manual-control.ts:505-510); manualControlAxisId is pure and returns a string or undefined.
"Do not convert-and-write the mapping back on read" and "the duplicate-axis guard … unmaps a leftover 1.19 binding (and warns)." Both verified. The read path writes nothing; the guard's only write is head controller.ts:352 (controller.ts:349 on the base) into protocolMapping, which is useBlueOsStorage('cockpit-joystick-functions-mapping-v1', …) at controller.ts:57, user-initiated and announced by the dialog at head :351.
"The joystick settings page labels those leftover bindings as MAVLink Axis X…T in the axis dropdown (display-only)." Verified, and smaller than last round. The whole mechanism is now the two :item-title attributes (head :360, :625) plus axisActionTitle (head :784-786); the option list is the base's again. The bullet's word "dropdown" now means the closed field — the open list holds only the native actions — which is a nuance in the body, not a mismatch with the code.
"min/max are kept" and "unrelated data-lake axes (camera zoom, …) are left alone." Verified on all paths — dataLakeManualControlAxisById[action.id] is undefined for any other id, which never equals an axis id in the find, falls back to the raw id in the guard (head controller.ts:337-339), and yields no title override in the view. The tests at manual-control-axis-id.test.ts:29-43 cover both.
"To be cherry-picked onto 1.18.3: Backport the 4K Cam and video fixes onto the 1.18 line #2997 so 1.18.3 ships this." Not checkable from here — the base branch is not in the checkout and there is no network access.

Failure site. The misbehaving code is v1.18-dev's six axis lookups in upadteManualControlState, and it is in the diff — that is the change. On the side that can be read here, the origin of the value 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as designed for its own branch. Resolving at the point of consumption is the right shape for a backport and there is no smaller fix at a different site to name.

Entry points

Function	Reached from	Frequency
`manualControlAxisId` (head `manual-control-axis-id.ts:25`)	the six `find` predicates, the duplicate-axis watcher and the test; `rg manualControlAxisId` over `pr.diff` finds the definition, the two imports (`mavlink-manual-control.ts:9`, `controller.ts:20`) and those call sites, and the same search over the checkout finds no other consumer	`per frame or pointer event`
`manualControlAxisDisplayName` (head `manual-control-axis-id.ts:35`)	`axisActionTitle` (head view `:785`) and the test; imported at head `ConfigurationJoystickView.vue:670` and nowhere else	`per frame or pointer event`
`axisActionTitle` (head `ConfigurationJoystickView.vue:784-786`)	bound as `:item-title` on both axis selects (head `:360`, `:625`), where Vuetify calls it once per option each time it derives its item list, and called by `getAxisActionName`	`per frame or pointer event`
`getAxisActionName` (head `ConfigurationJoystickView.vue:788-792`)	the "Additional axes" list on the SVG tab, `ConfigurationJoystickView.vue:265`, one entry per axis index not in `svgAxes`	`per frame or pointer event`
`upadteManualControlState` (changed lines head `mavlink-manual-control.ts:505-510`)	`updateControllerData`, registered as a controller-update callback at `src/stores/mainVehicle.ts:1025`, invoked from `processJoystickStateEvent` (`controller.ts:266`), fed by the `requestAnimationFrame` gamepad poll in the browser build and by the SDL IPC listener in Standalone	`per frame or pointer event`
duplicate-axis watcher callback (changed lines head `controller.ts:337-339`, `:345`)	the deep `watch(protocolMapping, …)` at `controller.ts:330-356`: fires on the axis selects, on the vehicle-defaults import write, on a BlueOS settings-sync replacement, and on the 1000 ms `remapActionsToVehicleButtonParameters` button write	`per user action`

The per-frame classification of the settings-page rows is unchanged and was re-derived rather than carried: the axis <tr> renders AxisVisualization bound to currentJoystick.state.axes[…] (ConfigurationJoystickView.vue:330-333), Joystick.state is a getter computed from this.gamepad (src/types/joystick.ts:59-77), and processJoystickStateEvent assigns joystick.gamepad = event.gamepad on every poll event (src/stores/controller.ts:266-269). So those rows still re-render per gamepad frame — but with axisSelectItems deleted, the :items identity is stable again and the only per-frame cost the diff adds is axisActionTitle in place of Vuetify's "name" key lookup, which is why section 5 is clean this round. Per gamepad frame the send path still runs six Object.entries(...).find(...) scans, as it did on the base, with the added work inside each predicate level with the base. Nothing added sits on mavlink:onIncomingMessage or any data-lake notify path.

Invariants

Each MANUAL_CONTROL axis is driven by exactly one gamepad axis, and Cockpit warns and unmaps when two claim the same one. Held at the single chokepoint that owns it: the deep watcher at controller.ts:330-356, comparing resolved axis ids (head :337-339, :345). Every site that can produce a colliding state was re-enumerated from rg axesCorrespondencies src — the two axis selects, the partial vehicle-defaults import (useVehicleDefaultsJoystickImport.ts:64-70), default-profile-importer.ts:71-74, and a BlueOS sync — and all write through protocolMapping, so all are covered by that one watcher. The residual noted in earlier rounds is inherited and unchanged: the guard only unmaps the side whose raw id did not change (head :347).
The PR never writes the vehicle-synced mapping automatically, and the display path never writes at all. Held, and with less code than any previous round. manualControlAxisDisplayName and axisActionTitle are pure string builders; nothing in the view allocates or mutates per render any more, since axisSelectItems is gone. The v-model targets at head :358 and :623 are unchanged from the base, and rg over pr.diff finds no new assignment into axesCorrespondencies.
The title override only fires for a stored 1.19 alias. Held on this branch, with the forward-merge caveat from round 7 unchanged: axisActionTitle titles every option in the list, and on the 1.19 line those six ids are the normal mapping (predefined-resources.ts:20-25), so if this commit were ever merged forward rather than only backported, the axis dropdown would rename all six standard options to MAVLink Axis X…T. No finding is raised — the PR targets v1.18-dev and says so — but the guard to add at that point is a version check, not a wider id list.

1. Correctness & Implementation Bugs — 1 finding

1.5 — Retyping axisActionTitle to ProtocolAction moves the type mismatch into the :item-title binding, whose item type is JoystickAction (new this round) — major

Consequence: the project's type check fails on this branch, so CI never gets to a build and the fix cannot ship until the helper's parameter type is settled.

Head ConfigurationJoystickView.vue:784-786:

const axisActionTitle = (action: ProtocolAction): string => {
  return manualControlAxisDisplayName(action) ?? action.name
}

It is bound at head :360 and :625 as :item-title="axisActionTitle" on two selects whose :items is filteredAndSortedAxisActions, declared computed((): JoystickAction[] => …) at ConfigurationJoystickView.vue:854. Vuetify types item-title against the item type — in 3.7.0 (package.json:100) VSelect's generic props declare itemTitle?: SelectItemKey<ItemType<T>>, whose function member is (item: Item, fallback?: any) => any, and T is inferred from items. Under strict (tsconfig.app.json:35, inherited by tsconfig.vitest.json) a function argument's parameters are checked contravariantly, so the binding requires JoystickAction to be assignable to ProtocolAction.

It is not, and neither is the reverse — the two types are mutually non-assignable, which is the whole difficulty here:

JoystickAction.protocol is a plain string (src/types/joystick.ts:300-314) and ProtocolAction.protocol is the string enum JoystickProtocol (:83-96, enum at :9-15), so JoystickAction → ProtocolAction fails. That is exactly what round 7's finding 1.4 was, and it is why this file already writes action as ProtocolAction at :521 and :567 — an assertion is allowed where an assignment is not.
ProtocolAction → JoystickAction fails too: JoystickAction carries [key: string]: string and ProtocolAction is an interface, which gets no implicit index signature.

So exactly one of the two sides must carry a cast, and after this round's change neither does. Round 7's remedy was incomplete on this point and that is this reviewer's error, not the author's: 1.4's fix text asserted ":item-title accepts any function, so the template side is unaffected" without being able to read Vuetify's declaration.

Why the base's own bindings are not evidence that this is unchecked: the same selects model-bind selectedProfileAxesCorrespondencies[…].action (head :358, :623) against those same JoystickAction[] items and do compile, but the stored action reaches the template through Pinia's UnwrapRef mapping (controller.ts:57, view :1041), which yields a mapped object type — and mapped types, unlike interfaces, do get an implicit index signature, so they satisfy JoystickAction where the bare ProtocolAction interface does not. The same mechanism is why filteredAndSortedAxisActions can annotate a filtered ProtocolAction[] (controller.ts:65, protocols.ts:12) as JoystickAction[] at all.

Two caveats, stated so the claim is checkable rather than taken on faith. There is no node_modules in this checkout and no network, so neither vue-tsc nor Vuetify's VSelect declaration could be run or read — the Vuetify typing above is from the published 3.x shape of SelectItemKey/ItemType, and if 3.7.0's item-title were typed with an any parameter the binding would compile. And vueCompilerOptions.strictTemplates is set nowhere (neither tsconfig.app.json nor tsconfig.vitest.json has a vueCompilerOptions block), so if vue-tsc 2.0.10 (package.json:148) in its default mode does not check generic prop values, this would also compile as written. yarn typecheck (package.json:30, run at .github/workflows/ci.yml:144) settles it in one command. Nothing here is a runtime problem: at runtime every object reaching the helper carries a real JoystickProtocol string, and the label is correct either way.

Fix, and it compiles whichever way the two caveats fall — type the helper for the side the template drives and cast once at the call into the shared module, mirroring :521/:567:

const axisActionTitle = (action: JoystickAction): string => {
  return manualControlAxisDisplayName(action as ProtocolAction) ?? action.name
}

getAxisActionName (head :788-792) keeps compiling against that signature, since the store-unwrapped action satisfies JoystickAction for the mapped-type reason above. The cast-free alternative is to widen the shared helper to a structural parameter ({ protocol: string; id: string } in manual-control-axis-id.ts:35), which keeps every current caller valid — action.protocol === JoystickProtocol.DataLakeVariable still compares fine — at the cost of a looser signature on the module the whole fix rests on.

2. Persistence & User Data — inventory, no findings

The PR adds, reshapes and removes nothing persisted, but it reads a vehicle-synced key on new paths and changes the conditions under which an existing write into it fires, so the footprint is written out rather than collapsed.

Key	Backend	What happened to it
`cockpit-joystick-functions-mapping-v1`	vehicle-synced — `useBlueOsStorage`, `controller.ts:49`/`:57`, shared by every topside computer and operator of that vehicle	Not added, not reshaped, not removed. Read by the resolution path, read again by the settings-page title override, and its existing unmap write now fires in one case it previously could not.

Read paths. manualControlAxisId (head manual-control-axis-id.ts:25-28) and manualControlAxisDisplayName (head :35-40) both take an action and return a string or undefined; neither mutates anything. In the view, axisActionTitle (head :784-786) builds a string and nothing else, and with axisSelectItems deleted the view no longer even copies the option list. rg over pr.diff finds no assignment into axesCorrespondencies beyond the pre-existing one below, no protocolMapping.value =, no new watcher and no migration.
The display path is read-only, and this round is the smallest version of it yet. Both selects now differ from the base in a single attribute each (item-title="name" → :item-title="axisActionTitle"), keeping v-model, :items, hide-details, density, variant, theme="dark" and return-object exactly as on the base. Opening the settings page writes nothing.
Write path. The one write is inherited: newMapping.axesCorrespondencies[axis as unknown as JoystickAxis].action = otherAvailableActions.no_function at head controller.ts:352, guarded by isDuplicated && wasMapped and preceded by the warning dialog at head :351. Widening isDuplicated (head :345) makes it reachable for a leftover 1.19 data-lake binding, so after a downgrade a user who moves one of the six controls to a different stick removes 1.19's binding for that axis from the shared key. Judged acceptable: user-initiated, announced, what round-3 finding 1.3 asked for, and 1.19 remigrates the surviving binding forward at its next boot.
No automatic migration is added, nothing is written as undefined, no machine-specific value (device path, filesystem path, window geometry) enters a synced key, and no default changes, so no already-configured user is stranded.

Sections with nothing to report (9)

3. AGENTS.md Adherence — ✅ (the increment is a deletion plus one annotation, which is the direction AGENTS.md's minimalism ladder rewards — axisSelectItems is gone and the view is down to +9/-3; the surviving helper is an arrow function, which jsdoc/require-jsdoc's ArrowFunctionExpression: false (.eslintrc.cjs:29-41) exempts, and no added JSDoc block is blank or filler; scope discipline holds, both selects differing from the base in one attribute each and no import, hook or declaration reordered; separation of concerns is respected, the id table and the label builder staying in src/libs/ with only the binding glue in the .vue; simple-import-sort order correct at head ConfigurationJoystickView.vue:670, controller.ts:20 and mavlink-manual-control.ts:9; no dependency added and no exported symbol left without a call site)

4. Security — ✅ (grep -P '[^\x00-\x7F]' over pr.diff returns exactly three lines, all em dashes and an ellipsis inside the two JSDoc blocks, so no zero-width, bidi or homoglyph characters; the title is built from a hard-coded lookup table plus String.prototype.slice/toUpperCase, never from user or vehicle text, and reaches the DOM through Vuetify's item title rather than v-html; no new dependency, network call, encoded blob, eval/Function(), secret or env-var handling, and no build, CI, postinstall or Electron change in the diff)

5. Performance — ✅ (round 7's per-frame allocation is deleted rather than tuned: axisSelectItems appears nowhere in pr.diff and :items="filteredAndSortedAxisActions" is the base binding again, so the identity handed to both selects is stable across the per-gamepad-frame re-render traced in the Change map; the only added per-frame work is axisActionTitle replacing Vuetify's "name" key lookup, one table lookup and a ?? per option; the six Object.entries(...).find(...) scans on the send path are the base's, with the added work inside each predicate level with it; nothing added is on mavlink:onIncomingMessage, dataLake:setVariable or any notify path, and the diff registers no listener, interval or watcher needing teardown)

6. UI / UX — ✅ (the settings rows are now structurally identical to the base — the round-6 wrapper and label are gone and the round-7 option-list copy with them — so the Min field, select and Max field remain three equal-height siblings under items-center at ConfigurationJoystickView.vue:607, and theme="dark" and return-object survive on both selects; the alias name reaches both the dropdown field and the SVG tab's "Additional axes" list through one helper, so the two presentations agree; the override is read-only so it owes no logUserAction entry, and no dialog, footer action, z-index, glass layer or icon-only control is added — the one user-facing string, MAVLink Axis X, matches the in-tree MAVLink Manual Control heading at :541)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised — the one added helper is three lines with a single ??, for what that is worth without the measurement; against .eslintrc.cjs, the *.vue override at :117-124 turns max-len off for the view while vue/max-len allows 180 in templates, which both changed attribute lines satisfy, and the six long find lines in mavlink-manual-control.ts sit under that file's own /* eslint-disable max-len */ and prettier/prettier disables at its lines 1-3; semi: never, func-style with arrows allowed and @typescript-eslint/explicit-function-return-type are all met, with no stray any; vue/no-unused-properties is satisfied since axisActionTitle is used in the template; the remaining type question is a compiler matter rather than a lint one and is raised as 1.5; the view gains 6 net lines, so no file-growth concern)

8. Commit Hygiene — ✅ (pr.json carries one commit, 5361a58, subject joystick: restore MANUAL_CONTROL axes saved by a newer version — a scope prefix matching both the area and the repository's dominant style in git log — whose body still names the settings-page label and remains accurate after this round's deletion; no wip/fixup!/squash!, no self-correcting pair, no #N or closing keyword in the message (the #2997/#3002 references stay in the PR body, where they belong), no AI-authorship trailer, and at 91 additions across five files it is reviewable in one sitting)

9. Tests — ✅ (src/tests/libs/joystick/manual-control-axis-id.test.ts is byte-identical to round 7 at +36/-0 and still covers the alias case, the already-MAVLink case and an unrelated data-lake id, asserting on returned values rather than object identity, so it is not brittle; rg over pr.diff shows no existing test removed or weakened)

10. Documentation — ✅ (nothing in the diff differs between Lite and Standalone — the joystick path is shared and the SDL listener is untouched — so no README table row is owed; both exported functions still carry JSDoc naming the undefined case their call sites rely on, including the display-only constraint the whole fix depends on; the one remaining view helper is private glue whose name and signature are self-describing, which AGENTS.md's JSDoc rule explicitly exempts)

11. Nitpicks / Optional — ✅ (round 7's nit went away with the code it was about; re-read against the current diff, the helper name, the MAVLink Axis X wording and the lookup-table constant all match in-tree conventions, and there is nothing left worth raising in person)

Generated by Claude. This is advisory; a human reviewer must still approve.

1.19 persisted those axes as data-lake ids. Resolve them when building MANUAL_CONTROL. Do not rewrite the mapping on read; the duplicate-axis guard treats those aliases as the same action. The settings page labels a leftover binding as MAVLink Axis X in the dropdown, display-only.

rafaellehmkuhl · 2026-09-03T22:00:38Z

Review follow-up — round 8

Done

src/views/ConfigurationJoystickView.vue (1.5 — :item-title item type is JoystickAction): axisActionTitle takes JoystickAction again and casts once into manualControlAxisDisplayName, same as the other two call sites in this view.

rafaellehmkuhl · 2026-09-03T22:00:40Z

/review

github-actions · 2026-09-03T22:11:14Z

✅ READY TO MERGE (Automated PR Review — round 9)

No open findings. 1 closed this round (1.5); 16 findings were raised across the nine rounds and all are now closed (15 addressed, 1 no longer applicable).

The change lets an older Cockpit understand joystick settings that a newer version saved. It translates the newer version's names for the six main vehicle axes at the moment the steering message is built, teaches the "two sticks are doing the same thing" check to treat both naming schemes as one control, and shows the affected axis under its real name on the joystick settings screen. This round's only edit is two lines in the settings view: the label helper is declared again for the type the dropdown hands it, and casts once where it calls into the shared module — the same pattern that file already uses twice for button actions.

What still needs attention

Nothing is open. Every finding this PR has collected is closed: fifteen addressed in code and one (11.3) retired when the code it described was deleted.

Since round 8 — 1 closed, 0 new, comparing 5361a58 → ffafde3

Range and its reliability. The previous review was at 5361a58, the current head is ffafde3. incremental.diff is unusable as an increment for the eighth round running: it reproduces the entire PR — all five files, both added ones as complete file bodies, with +34/-0, +7/-6, +5/-2, +36/-0, +9/-3, matching pr.diff hunk for hunk and matching pr.json's own per-file counts — while pr.json carries exactly one commit, now ffafde3. The round-8 head was amended away again, so there is no usable PREV→HEAD range and the transition below is judged from pr.diff against the base, plus the file list, per-file counts and quoted code recorded in round 8. All five per-file counts are identical to round 8, so the counts locate nothing; what did change is the body of axisActionTitle in src/views/ConfigurationJoystickView.vue, which round 8 quoted as (action: ProtocolAction) calling manualControlAxisDisplayName(action) and which now reads (action: JoystickAction) calling manualControlAxisDisplayName(action as ProtocolAction). Everything else in the diff, getAxisActionName included, is byte-identical to what round 8 reviewed.

Checkout caveat, unchanged. This PR targets v1.18-dev; the checkout is master at c1fa6e2 and holds only master and origin/master, so the base branch cannot be read. Every citation without a "head" qualifier is to the 1.19 line. There is no node_modules here and no network, so neither vue-tsc nor Vuetify's own declarations could be run or read.

1.5 — axisActionTitle typed for ProtocolAction while :item-title hands it a JoystickAction — ❌ → ✅ Addressed.

The author applied the finding's remedy verbatim. Head ConfigurationJoystickView.vue:784-786 now reads:

const axisActionTitle = (action: JoystickAction): string => {
  return manualControlAxisDisplayName(action as ProtocolAction) ?? action.name
}

Both halves of what the finding asked for landed, and each was re-checked rather than taken from the comment:

The template side is the type the binding drives. :item-title="axisActionTitle" at head :360 and :625 sits on selects whose :items is filteredAndSortedAxisActions, declared computed((): JoystickAction[] => …) (ConfigurationJoystickView.vue:854), so the helper's parameter now matches the item type whichever way the two unverifiable caveats in the finding (Vuetify's SelectItemKey shape, strictTemplates) fall.
The call into the shared module carries the one cast. manualControlAxisDisplayName is declared (action: ProtocolAction) (head manual-control-axis-id.ts:29), and action as ProtocolAction mirrors the two existing in-tree call sites for button actions at ConfigurationJoystickView.vue:521 and :567, so nothing new was invented for it.
The other caller still compiles. getAxisActionName (head :788-792) passes selectedProfileAxesCorrespondencies.value[…]?.action, which is controllerStore.protocolMapping.axesCorrespondencies unwrapped by Pinia (ConfigurationJoystickView.vue:1041). That the store-unwrapped ProtocolAction satisfies JoystickAction is not an inference about mapped types taken on faith here — it is what the base already does at :854-855, where controllerStore.availableAxesActions (ref(allAvailableAxes()), controller.ts:65; allAvailableAxes(): ProtocolAction[], protocols.ts:12) is filtered with (action: JoystickAction) callbacks and returned as JoystickAction[] on a branch that passes CI.

Round 7's 1.4 and round 8's 1.5 were the same one-word problem on opposite sides of one call; this shape answers both at once, and no third position is left holding a mismatch. It remains a compile-time matter only — at runtime every object reaching the helper carries a real JoystickProtocol string and the label is the same either way — so yarn typecheck (package.json:30, run at .github/workflows/ci.yml:144) is still the authority, and this reviewer cannot run it.

No new findings. Sections 0 through 11 were re-run over the whole of pr.diff, not over the increment, with two completeness checks added this round that had not been made before: whether the leftover 1.19 binding also still drives the data-lake pipeline (it does, harmlessly — see the Change map), and whether 1.19 rewrote anything else in this same key that 1.18 cannot read (it did not — migrateMavlinkManualControlButtons at mavlink-manual-control.ts:681-683 is only the servo→actuator rename, which v1.18-dev runs itself). Both came back clean, so nothing is raised.

Discussion since round 8. rafaellehmkuhl posted a follow-up (#3003 (comment)) reporting that axisActionTitle "takes JoystickAction again and casts once into manualControlAxisDisplayName, same as the other two call sites in this view". Treated as a claim and checked against pr.diff: the signature, the cast and the two precedent call sites are all where the comment says, and they are the evidence for the transition above rather than the reason for it. The second comment is a bare /review, treated as noise.

Resolutions and votes. resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no unmatched id to report back. decisions.json is []: no dispute has ever been put to a vote, so no decision is waiting on anyone.

Nothing in pr.json, pr.diff, incremental.diff, new-comments.json, resolutions.json or decisions.json is addressed to this reviewer, and nothing in them reads as an injected instruction.

Change map — what was established before judging

Line numbers for src/libs/joystick/protocols/manual-control-axis-id.ts are the file's own this round; earlier rounds cited that file by pr.diff's line numbers, which run six higher.

Claims

Symptom — "1.19 rewrote joystick MANUAL_CONTROL axes onto data-lake ids in the vehicle-synced mapping, so a stick saved on 1.19 comes back as zero on this line." Half verified, unchanged across all nine rounds. The forward rewrite is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696-713), reached from performJoystickMappingMigrations (protocols.ts:26-30). The 1.18 consumer half is readable only through the diff's context lines, which do show the six finds matching on protocol === MAVLinkManualControl, so a stored data-lake action matched nothing and left the axis at 0.
Cause — the six ids and their MAVLink counterparts. Verified exactly. manual-control-axis-id.ts:5-12 is a precise inverse of mavlinkAxisToDataLakeMap (mavlink-manual-control.ts:685-692) — same six ids, same axis_x…axis_t targets. No axis missing, none invented.
Mechanism — "resolve those ids when building MANUAL_CONTROL." Verified. The resolution happens inside the six find predicates (head mavlink-manual-control.ts:505-510); manualControlAxisId (head manual-control-axis-id.ts:19-22) is pure and returns a string or undefined.
"Do not convert-and-write the mapping back on read." Verified. rg over pr.diff finds no assignment into axesCorrespondencies, no protocolMapping.value =, no new watcher and no migration; the only write in the diff's reach is the inherited unmap at head controller.ts:352.
"The duplicate-axis guard … unmaps a leftover 1.19 binding (and warns) instead of leaving both." Verified by walking the case. With a leftover inputs/mavlink/axis-x on one axis and a freshly assigned axis_x on another, both resolve to axis_x at head controller.ts:337-339, so duplicateAxisActions is non-empty; isDuplicated is true for both (head :345), and wasMapped (head :347, raw ids) is true only for the leftover, whose id did not change. So the leftover is unmapped and the new binding survives, which is the stated behaviour, with the pre-existing warning dialog at head :351 ahead of the write at :352.
"The joystick settings page labels those leftover bindings as MAVLink Axis X…T in the axis dropdown (display-only)." Verified. The whole mechanism is the two :item-title attributes (head :360, :625) plus axisActionTitle (head :784-786), which builds a string and nothing else. As in round 8, "dropdown" means the closed field: the open option list holds only the actions 1.18 knows about.
"min/max are kept" and "unrelated data-lake axes (camera zoom, …) are left alone." Verified on all three paths — dataLakeManualControlAxisById[action.id] is undefined for any other id, which never equals an axis id in the find, falls back to the raw id in the guard, and yields no title override in the view. manual-control-axis-id.test.ts:29-43 covers both.
"Going back to 1.19 remigrates forward." Verified for both states a downgraded topside can leave behind: an untouched mapping still holds the data-lake ids 1.19 wrote, and a mapping the user re-assigned on 1.18 holds MAVLinkManualControl actions, which migrateMavlinkManualControlAxes (:696-713) converts again on 1.19's next boot.
"To be cherry-picked onto 1.18.3: Backport the 4K Cam and video fixes onto the 1.18 line #2997 so 1.18.3 ships this." Not checkable from here — the base branch is not in the checkout and there is no network access.

Failure site. The misbehaving code is v1.18-dev's six axis lookups in upadteManualControlState, and it is in the diff — that is the change. On the side readable here, the origin of the value 1.18 cannot use is migrateMavlinkManualControlAxes (mavlink-manual-control.ts:696), working as designed for its own branch. Resolving at the point of consumption is the right shape for a backport, and there is no smaller fix at a different site to name. Completeness of that scope was checked this round: performJoystickMappingMigrations (protocols.ts:26-30) runs exactly two migrations, and the other one, migrateMavlinkManualControlButtons → migrateServoSubButtonsToActuators (mavlink-manual-control.ts:665-683), only renames Servo to Actuator inside MAVLinkManualControl button ids — no protocol change, and a migration v1.18-dev runs itself (round-3 finding 1.2 established the base's boot-time call). So the axes are the only 1.19-only reshaping of this key, and nothing on the button side is left uncovered.

Entry points

Function	Reached from	Frequency
`manualControlAxisId` (head `manual-control-axis-id.ts:19`)	the six `find` predicates (head `mavlink-manual-control.ts:505-510`), the duplicate-axis watcher (head `controller.ts:338`, `:345`) and the test; `rg manualControlAxisId` over `pr.diff` finds the definition, the two imports (`mavlink-manual-control.ts:9`, `controller.ts:20`) and those call sites, and the same search over the checkout finds no other consumer	`per frame or pointer event`
`manualControlAxisDisplayName` (head `manual-control-axis-id.ts:29`)	`axisActionTitle` (head view `:785`) and the test; imported at head `ConfigurationJoystickView.vue:670` and nowhere else	`per frame or pointer event`
`axisActionTitle` (head `ConfigurationJoystickView.vue:784-786`)	bound as `:item-title` on both axis selects (head `:360`, `:625`), where Vuetify calls it per option each time it derives its item list, and called by `getAxisActionName`	`per frame or pointer event`
`getAxisActionName` (head `ConfigurationJoystickView.vue:788-792`)	the "Additional axes" list on the SVG tab, `ConfigurationJoystickView.vue:265`, one entry per axis index not in `svgAxes`	`per frame or pointer event`
`upadteManualControlState` (changed lines head `mavlink-manual-control.ts:505-510`)	`updateControllerData`, registered as a controller-update callback at `src/stores/mainVehicle.ts:1025`, invoked from `processJoystickStateEvent` (`controller.ts:266`, subscribed at `:161`), fed by the gamepad poll in the browser build and by the SDL IPC listener in Standalone	`per frame or pointer event`
duplicate-axis watcher callback (changed lines head `controller.ts:337-339`, `:345`)	the deep `watch(protocolMapping, …)` at `controller.ts:330-356`: fires on the axis selects, on the vehicle-defaults import write, on a BlueOS settings-sync replacement, and on the 1000 ms `remapActionsToVehicleButtonParameters` button write	`per user action`

The per-frame classification of the settings rows was re-derived rather than carried: the axis <tr> renders AxisVisualization bound to currentJoystick.state.axes[…] (ConfigurationJoystickView.vue:330-333), Joystick.state is a getter computed from this.gamepad (src/types/joystick.ts:59-77), and processJoystickStateEvent assigns joystick.gamepad = event.gamepad on every poll event (controller.ts:266-269). The only per-frame work the diff adds there is axisActionTitle in place of Vuetify's "name" key lookup — one table lookup and a ?? per option — since the :items binding is the base's again. On the send path the six Object.entries(...).find(...) scans are the base's, with the added work inside each predicate level with it. Nothing added sits on mavlink:onIncomingMessage, dataLake:setVariable or any notify path.

One further path was traced this round because the fix creates a second driver for the same binding: a data-lake-protocol axis is also written into the lake every frame by the joystick data-lake pipeline (src/libs/joystick/protocols/data-lake.ts:76-89), so after a downgrade a leftover alias both drives MANUAL_CONTROL and calls setDataLakeVariableData('inputs/mavlink/axis-x', …). That is benign on this line and stays out of the way: setDataLakeVariableData (src/libs/actions/data-lake.ts:135-153) writes a dictionary entry and notifies listeners of that id, of which 1.18 has none, and it never populates dataLakeVariableInfo (only createDataLakeVariable at :82-86 does), so the alias never appears in getAllDataLakeVariablesInfo (:74-76), never becomes an entry in availableDataLakeActions (data-lake.ts:37-44), and therefore never enters the axis select's option list. No finding — it is the pre-existing behaviour of a stored data-lake binding, unchanged by this diff.

Invariants

Each MANUAL_CONTROL axis is driven by exactly one gamepad axis, and Cockpit warns and unmaps when two claim the same one. Held at the single chokepoint that owns it: the deep watcher at controller.ts:330-356, now comparing resolved axis ids. Every site that can produce a colliding state was re-enumerated from rg axesCorrespondencies src — the two axis selects, the partial vehicle-defaults import (useVehicleDefaultsJoystickImport.ts:64-70), default-profile-importer.ts:71-74, and a BlueOS sync — and all write through protocolMapping, so one watcher covers all of them. The residual noted in earlier rounds is inherited and unchanged: the guard only unmaps the side whose raw id did not change (head :347), which is what makes the downgrade case resolve the way the PR body describes.
The PR never writes the vehicle-synced mapping automatically, and the display path never writes at all. Held. manualControlAxisId, manualControlAxisDisplayName and axisActionTitle are pure; the v-model targets at head :358 and :623 are the base's, and this round's edit changed a parameter type and added a cast, neither of which touches a write path.
The title override only fires for a stored 1.19 alias. Held on this branch, with the forward-merge caveat from rounds 7 and 8 unchanged: axisActionTitle titles every option in the list, and on the 1.19 line those six ids are the normal mapping (predefined-resources.ts:158-161), so if this commit were ever merged forward rather than only backported, the axis dropdown would rename all six standard options to MAVLink Axis X…T. No finding — the PR targets v1.18-dev and says so — but the guard to add at that point is a version check, not a wider id list.

2. Persistence & User Data — inventory, no findings

The PR adds, reshapes and removes nothing persisted, but it reads a vehicle-synced key on new paths and changes the conditions under which an existing write into it fires, so the footprint is written out rather than collapsed.

Key	Backend	What happened to it
`cockpit-joystick-functions-mapping-v1`	vehicle-synced — `useBlueOsStorage`, `controller.ts:49`/`:57`, shared by every topside computer and operator of that vehicle	Not added, not reshaped, not removed. Read by the resolution path, read again by the settings-page title override, and its existing unmap write now fires in one case it previously could not.

Read paths. manualControlAxisId (head manual-control-axis-id.ts:19-22) and manualControlAxisDisplayName (head :29-34) take an action and return a string or undefined; neither mutates anything. axisActionTitle (head view :784-786) builds a string; this round's change to it is a parameter annotation and a type assertion, both erased at compile time.
The display path is read-only. Both selects differ from the base in a single attribute each (item-title="name" → :item-title="axisActionTitle"), keeping v-model, :items, hide-details, density, variant, theme="dark" and return-object exactly as on the base. Opening the settings page writes nothing.
Write path. The one write is inherited: newMapping.axesCorrespondencies[axis as unknown as JoystickAxis].action = otherAvailableActions.no_function at head controller.ts:352, guarded by isDuplicated && wasMapped and preceded by the warning dialog at head :351. Widening isDuplicated (head :345) makes it reachable for a leftover 1.19 data-lake binding, so after a downgrade a user who moves one of the six controls to a different stick removes 1.19's binding for that axis from the shared key. Judged acceptable: user-initiated, announced, what round-3 finding 1.3 asked for, and 1.19 remigrates the surviving binding forward at its next boot.
No automatic migration is added, nothing is written as undefined, no machine-specific value (device path, filesystem path, window geometry) enters a synced key, and no default changes, so no already-configured user is stranded.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (round 8's 1.5 is closed by the retype-plus-cast at head ConfigurationJoystickView.vue:784-786, checked against the item type at :854 and the in-tree cast precedent at :521/:567; the downgrade collision was walked end to end through head controller.ts:337-352 and unmaps the leftover rather than the new binding; the six resolved find predicates keep their base semantics for MAVLinkManualControl ids and return undefined for every unrelated data-lake id; the second driver of a leftover alias, data-lake.ts:76-89, was traced and writes to an id nothing on 1.18 registers or reads; no Electron-only API, no Pinia telemetry read, no widget Options entry and no x && x.y guard where ?. belongs)

3. AGENTS.md Adherence — ✅ (the increment is one annotation and one cast, the minimum that closes 1.5, and reuses the file's own as ProtocolAction idiom rather than adding an abstraction; both exported helpers have call sites in this PR, so nothing is groundwork; the two arrow helpers are exempt from jsdoc/require-jsdoc via ArrowFunctionExpression: false (.eslintrc.cjs:29-41) and the two JSDoc blocks that do exist have non-empty summaries, typed @param and typed @returns; scope discipline holds — no rename, no import or hook reorder, both selects differing from the base in one attribute each; the id table and label builder stay in src/libs/ with only binding glue in the .vue; no dependency added, and yarn is untouched)

4. Security — ✅ (grep -P '[^\x00-\x7F]' over pr.diff returns exactly three lines, all em dashes and ellipses inside the two JSDoc blocks, so no zero-width, bidi or homoglyph characters; the label is built from a hard-coded six-entry table plus slice/toUpperCase, never from user or vehicle text, and reaches the DOM as a Vuetify item title rather than through v-html; no new dependency, network call, encoded blob, eval/Function(), secret or env-var use, and no build, CI, postinstall or Electron file in the diff)

5. Performance — ✅ (the per-frame allocation of round 7 stays deleted — axisSelectItems appears nowhere in pr.diff and :items="filteredAndSortedAxisActions" is the base binding, so the identity handed to both selects is stable across the per-gamepad-frame re-render traced in the Change map; this round's edit is types only and adds no runtime work; the added per-frame cost remains one table lookup and a ?? per option in the view and one function call inside each of the six existing find predicates; nothing added is on mavlink:onIncomingMessage, dataLake:setVariable or a notify path, and the diff registers no listener, interval, watcher or subscription needing teardown)

6. UI / UX — ✅ (the settings rows are structurally identical to the base, so the Min field, select and Max field remain three equal-height siblings under items-center at ConfigurationJoystickView.vue:607, with theme="dark" and return-object intact on both selects; the alias name now reaches the dropdown field and the SVG tab's "Additional axes" list through the same helper, so the two presentations agree; the override is read-only and owes no logUserAction entry; no dialog, footer action, z-index, glass layer, icon-only control or new scoped CSS is added, and the one user-facing string, MAVLink Axis X, matches the in-tree MAVLink Manual Control heading at :541)

7. Code Quality & Style — ✅ (complexity-report.json is absent, so the complexity report was unavailable this round and no complexity finding is raised; against .eslintrc.cjs, the *.vue override at :117-124 turns max-len off for the view while vue/max-len allows 180 in templates, which both changed attribute lines satisfy, and the six long find lines sit under mavlink-manual-control.ts's own file-level eslint-disable; semi: never, func-style with arrows allowed and @typescript-eslint/explicit-function-return-type are met, with no stray any — the added as ProtocolAction is a narrowing assertion between two declared types, not an escape hatch; simple-import-sort order is correct at head ConfigurationJoystickView.vue:670, controller.ts:20 and mavlink-manual-control.ts:9; vue/no-unused-properties is satisfied since axisActionTitle is used in the template; no comment whose code is unchanged was reworded or deleted, and the view gains 6 net lines)

8. Commit Hygiene — ✅ (pr.json carries one commit, ffafde3, subject joystick: restore MANUAL_CONTROL axes saved by a newer version — a scope prefix matching both the area and the repository's dominant style in git log — whose body names the resolution, the no-write-back constraint, the duplicate-guard behaviour and the display-only label, all of which the diff still does; no wip/fixup!/squash!, no self-correcting pair, no #N or closing keyword in the message, with the #2997/#3002 references kept in the PR body where they belong, no AI-authorship trailer, and at 91 additions across five files it is reviewable in one sitting)

9. Tests — ✅ (src/tests/libs/joystick/manual-control-axis-id.test.ts is unchanged at +36/-0 and still covers the alias case, the already-MAVLink case and an unrelated data-lake id, asserting on returned values rather than object identity, so it is not brittle; its literals type-check against ProtocolAction without a cast; rg over pr.diff shows no existing test removed or weakened)

10. Documentation — ✅ (nothing in the diff differs between Lite and Standalone — the joystick mapping path is shared and the SDL listener is untouched — so no README table row is owed; both exported functions still carry JSDoc naming the undefined case their call sites rely on, including the display-only constraint the fix depends on; the two view helpers are private glue whose names and signatures are self-describing, which AGENTS.md's JSDoc rule exempts)

11. Nitpicks / Optional — ✅ (re-read against the current diff, the helper names, the MAVLink Axis X wording, the lookup-table constant and the cast placement all match in-tree conventions, and there is nothing left worth raising in person)

Generated by Claude. This is advisory; a human reviewer must still approve.

rafaellehmkuhl force-pushed the v1.18-reverse-joystick-axis-migration branch from bc875f5 to 03e32b3 Compare September 3, 2026 19:48

rafaellehmkuhl force-pushed the v1.18-reverse-joystick-axis-migration branch from 03e32b3 to eaf3e2c Compare September 3, 2026 20:18

rafaellehmkuhl mentioned this pull request Sep 3, 2026

1.18.3: Backport the 4K Cam and video fixes onto the 1.18 line #2997

Merged

5 tasks

rafaellehmkuhl force-pushed the v1.18-reverse-joystick-axis-migration branch from eaf3e2c to f23973b Compare September 3, 2026 20:32

rafaellehmkuhl force-pushed the v1.18-reverse-joystick-axis-migration branch from f23973b to b98e418 Compare September 3, 2026 20:48

rafaellehmkuhl force-pushed the v1.18-reverse-joystick-axis-migration branch from b98e418 to 04de60c Compare September 3, 2026 21:02

rafaellehmkuhl force-pushed the v1.18-reverse-joystick-axis-migration branch from 04de60c to 8688647 Compare September 3, 2026 21:19

rafaellehmkuhl force-pushed the v1.18-reverse-joystick-axis-migration branch from 8688647 to 5361a58 Compare September 3, 2026 21:42

rafaellehmkuhl force-pushed the v1.18-reverse-joystick-axis-migration branch from 5361a58 to ffafde3 Compare September 3, 2026 22:00

rafaellehmkuhl requested a review from ArturoManzoli September 4, 2026 13:23

ArturoManzoli approved these changes Sep 4, 2026

View reviewed changes

rafaellehmkuhl merged commit 221182c into v1.18-dev Sep 4, 2026
23 of 24 checks passed

rafaellehmkuhl deleted the v1.18-reverse-joystick-axis-migration branch September 4, 2026 16:38

Uh oh!

Conversation

rafaellehmkuhl commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

github-actions Bot commented Sep 3, 2026

What still needs attention

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Done

Done differently

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

What still needs attention

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Done

Won't change (with reasoning)

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

What still needs attention

Uh oh!

github-actions Bot commented Sep 3, 2026

🙋 Decision needed — 6.1

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Done

Done differently

Questions for reviewers

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

What still needs attention

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Done

Questions for reviewers

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

What still needs attention

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Done

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

What still needs attention

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Done

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

What still needs attention

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Done

Done differently

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

What still needs attention

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Done

Uh oh!

rafaellehmkuhl commented Sep 3, 2026

Uh oh!

rafaellehmkuhl commented Sep 3, 2026 •

edited

Loading