Fix editability of some data-lake variables

[rafaellehmkuhl]

Why

Two problems on the Data Lake page, both coming from the same root cause: Cockpit had no way of knowing who created a variable, so the page guessed from the persistence flags.

• Ownership was inferred from persistent != null, which is true for any variable that sets the flag at all — including the internal ones that explicitly set persistent: false: every BlueOS variable, Camera Tilt, Autopilot System ID, Network Latency. Those were offered full edit and delete. For compound variables the page could not guess at all, since a transforming function Cockpit sets up looks exactly like one the user wrote, so every compound got both actions — the legacy ArduPilot System ID and the POI coordinates among them.
• The camera zoom and focus speeds were unreachable. They hold a setting the operator tunes, but the table only showed the edit button for variables it considered user-defined, so there was no way to change them from the UI. Their values also reset to 3 on every boot.

What

Cockpit records who created each variable, through a new optional systemOwned flag on DataLakeVariable and on TransformingFunction. Cockpit creates almost every variable there is, so its ownership is what a variable says by staying quiet, and the three places that create one on the user's behalf say otherwise — the Data Lake dialog, the compound dialog and the custom-widget input config.

Anything read back from storage is taken as the user's instead, since it predates this being recorded. For plain variables that is provable: only those three places ever write to cockpit-persistent-data-lake-variables, as Cockpit never marks its own persistent. For compound ones it is the reading that cannot take a variable away from whoever made it, with Cockpit reclaiming its own through the setup routines. Without this, upgrading would hand every variable a user had already created over to Cockpit and remove their delete button.

Compound variables go through ensureCockpitTransformingFunction(), which creates the function or records that it is Cockpit's, leaving the expression the user may have tuned untouched. It is idempotent, so the setup routines call it on every boot, and it replaced the find/create dance that was duplicated across four call sites. The compound dialog carries the record through an edit instead of dropping it.

Known limitation. A widget input element stores a copy of the whole variable in its own options, and replays it at mount when no variable with that id is registered. A variable the user created with persistence switched off is not remembered anywhere else across a restart, so it comes back from that copy — and if the copy predates this flag it says nothing about who made it, so it is read as Cockpit's: no delete button, and the editor limited to its value. Both readings of an unflagged copy are guesses, and this is the one where nothing the app depends on can be deleted by mistake. Copies written from now on carry the flag, since the selector binds the live registry object.

allowUserToChangeValue says what the user may set. It was already the flag for "a joystick mapping or a widget element writes here"; it now reads as the user may set this value, by hand from the Data Lake page, and — when the variable is not a compound one — through a control. That is the single question the page asks: a variable is editable when it is the user's own, or when Cockpit marked it as theirs to set. Deletion stays stricter: only their own, never Cockpit's.

Marked as the user's to set: the camera zoom and focus speeds, the camera zoom and focus expressions, and the six MANUAL_CONTROL axis outputs — values and expressions that are tuned to fit the vehicle.

Editing one of Cockpit's plain variables opens the dialog limited to the value field, so it cannot rewrite the id, type or persistence the app depends on. A Cockpit compound stays fully editable in the compound dialog, name and expression included, by design — the expression is the thing meant to be tuned, and that matches what master already allowed.

No control may write to a compound variable. A compound is computed from its expression, so a write from a joystick or a widget element is undone at the next evaluation. The joystick button picker already excluded them; the axis picker and the custom widget input elements went by allowUserToChangeValue alone. Both now check as well. A widget element can still be pointed at a compound variable to display its value, it just stays read-only. This lands first, as its own commit, so no intermediate commit offers a compound as something to write to.

Persisted values are actually restored. createDataLakeVariable was unconditionally writing the default over whatever was in storage, so a persistValue variable never came back with the user's value. A saved value now wins over the initial one. savePersistentValues also merges into the stored object instead of rebuilding it from the variables registered so far — rebuilding dropped the values of variables not registered yet — and deleting a variable removes only its own key.

The camera speeds persist, now created with persistValue: true, so a tuned speed survives a reboot. The increase/decrease variables were left alone, since their value is written by whatever control is mapped to them.

Also along the way: the edit dialog filled the value field with a truthiness check, so a variable holding 0 or false opened with an empty field; and the Source column now answers who owns the variable instead of mixing that with whether it happens to be a compound one. Which rows are computed moved to an icon beside the type, the same mdi-function-variant the "Add compound variable" button uses, and searching for "compound" still finds them.

Tests

Three unit test files: persisted-value restore and delete behaviour, plus a pre-flag stored variable being read back as the user's (src/tests/libs/actions/data-lake.test.ts); recording ownership on stored and new transforming functions, and it reaching the data lake variable the page reads (src/tests/libs/actions/data-lake-transformations.test.ts); and the edit/delete rules including the unmarked-means-Cockpit's default (src/tests/libs/utils-data-lake.test.ts).

One window this does not close. A compound variable Cockpit owns is only recorded as its own when the routine that creates it runs, and two of them run late: ardupilotSystemId when a MAVLink vehicle is instantiated, and the POI coordinate functions when a map component mounts. On the first boot after upgrading, the stored entries still carry nothing, so until that moment they read as User defined with a delete button. The record is persisted once written, so every later boot is correct from load. Neither is a regression — on master both are editable and deletable outright, with no window — and closing it means moving those two calls to bootstrap, which is a change of its own.

Fix #2774
Fix #2775

[github-actions]

Automated PR Review (Claude)

0. Summary

Verdict: MINOR SUGGESTIONS

Minor items to consider: 1.1, 1.2, 6.1.

This PR fixes editability of certain data-lake variables (camera zoom/focus speed). It introduces a "value-only edit mode" in DataLakeVariableDialog for variables that have allowUserToChangeValue: true but are not user-defined (persistent !== true). It also fixes isUserDefinedVariable to use strict === true instead of != null (which incorrectly matched persistent: false), adds persistValue: true to the speed variables so their values survive reboots, and restores persisted values when createDataLakeVariable is called for persistValue variables. Fixes #2774 and #2775.

1. Correctness & Implementation Bugs

1.1 minor — In src/libs/actions/data-lake.ts, the new code in createDataLakeVariable loads the saved value when variable.persistValue is true, but then unconditionally overwrites the timestamp check and savePersistentValues() call below using the original initialValue variable (not valueToSet):

if (initialValue !== undefined) {
    dataLakeVariableTimestamps[variable.id] = performance.now()
}
...
if (variable.persistValue && initialValue !== undefined) {
    savePersistentValues()
}

When a persisted value exists and initialValue is also provided (which is the case for camera speed variables — initialValue is 3), the timestamp will be set and savePersistentValues() will be called — both fine. However, the savePersistentValues() call will re-save the just-loaded value, which is harmless but slightly wasteful. More importantly, if initialValue were undefined but a persisted value existed, the timestamp would never be set. This is a minor correctness gap in an edge case that likely doesn't occur today, but consider using valueToSet instead of initialValue in the subsequent conditions for consistency.

1.2 minor — In DataLakeVariableDialog.vue, the valueOnlyEditMode computed checks variableInfo?.persistent !== true, which means a variable with persistent: undefined would enter value-only edit mode if it has allowUserToChangeValue: true. This is the intended behavior for the camera speed variables, but it's worth noting that if a user-defined variable were to have both persistent: true and allowUserToChangeValue: true, it would bypass value-only mode and get the full edit form (which is correct). The logic is sound for the current use cases.

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style

6.1 nit — In ToolsDataLakeView.vue, the function editUserDefinedVariable was renamed to editVariable and its JSDoc was not updated to reflect its new broader scope. The current doc says "Opens the dialog to edit an existing variable" which is still accurate, but the @param description and the error-path message Variable with ID ${variableId} is not editable no longer match the broadened condition. This is minor since the JSDoc was already generic, but the snackbar message now only triggers for variables that are neither user-defined nor user-editable, which is correct.

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

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

[rafaellehmkuhl]

Review follow-up — round 1

Done

• src/libs/actions/data-lake.ts (1.1 — use valueToSet for timestamp/persist checks): timestamp init and savePersistentValues() now key off valueToSet !== undefined instead of initialValue, so a persisted value loaded without an explicit default still gets a timestamp.
• src/views/ToolsDataLakeView.vue (6.1 — editVariable JSDoc): updated to say it opens the dialog for user-defined or user-editable variables.

Done differently

• src/components/DataLakeVariableDialog.vue (1.2 — value-only edit guard): no longer uses persistent !== true; value-only mode now checks !isUserDefinedDataLakeVariable(), with the helper living in src/libs/utils-data-lake.ts (IDs under user/custom/, or userDefined: true).

Won't change (with reasoning)

• 1.2 — value-only mode for persistent: undefined: intentional; camera speed vars are internal but user-editable, and user-defined vars from the Data Lake menu get the full edit form.

[rafaellehmkuhl]

/review

[github-actions]

Automated PR Re-review 2 (Claude)

Comparing adef72e1917865cd10a70f36d92aa58ca21fef6a → e1b8787d06e61b0354f3bce50782e7bcab25d6c8. Note: the incremental diff includes unrelated changes from a rebase onto master (CI workflows, mission estimates, package.json). This re-check is based on the pr.diff (base...head) for accuracy.

Previous findings status

#	Finding	Severity	Status
1.1	data-lake.ts: use valueToSet instead of initialValue in timestamp/persist checks	minor	✅ Addressed
1.2	DataLakeVariableDialog.vue: valueOnlyEditMode guard using persistent !== true	minor	✅ Addressed
6.1	ToolsDataLakeView.vue: JSDoc not updated for renamed editVariable	nit	✅ Addressed

Discussion since last review

@rafaellehmkuhl posted a review follow-up detailing how each finding was handled:

• 1.1: timestamp init and savePersistentValues() now key off valueToSet — confirmed addressed in the diff.
• 1.2: Done differently — value-only mode now checks !isUserDefinedDataLakeVariable() instead of persistent !== true, using a new helper. This is a cleaner approach.
• 6.1: JSDoc updated — confirmed.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

Minor items to consider: 1.1, 6.1.

This PR fixes editability of certain data-lake variables (camera zoom/focus speed). It introduces a userDefined flag on DataLakeVariable, a centralized isUserDefinedDataLakeVariable helper, a "value-only edit mode" in DataLakeVariableDialog for internal but user-editable variables, and restores persisted values inside createDataLakeVariable. All previous findings have been addressed. Two minor new items surfaced.

1. Correctness & Implementation Bugs

1.1 minor — In src/libs/utils-data-lake.ts, the isUserDefinedDataLakeVariable function checks info?.userDefined === true or normalizedId.startsWith(userDefinedDataLakeVariableIdPrefix) (i.e. 'user/custom/'). However, existing user-created variables (from before this PR) were never stamped with userDefined: true — they were identified solely by having persistent != null. After this PR, isUserDefinedVariable in ToolsDataLakeView.vue delegates to isUserDefinedDataLakeVariable, which relies on the userDefined flag or the user/custom/ prefix. Since the base code auto-generates IDs with the 'user/custom/' + machinizeString(name) prefix (line 186 of DataLakeVariableDialog.vue), existing variables created through the UI will match the prefix check, so this is safe in the normal flow. However, if a user manually edited the ID (via the pencil toggle) to something that doesn't start with user/custom/, and it was created before this PR (so it has no userDefined: true flag), that variable will no longer be recognized as user-defined and will lose its edit/delete buttons. This is a narrow edge case but worth noting.

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style

6.1 nit — In src/libs/utils-data-lake.ts, normalizeDataLakeVariableId is a private helper that only strips leading whitespace and slashes. Given that the user/custom/ prefix is set programmatically by machinizeString, leading whitespace and slashes in an ID would be unusual. The helper isn't wrong, but it does defensive work that may never be exercised. Consider whether it adds enough value or if a direct id.startsWith(userDefinedDataLakeVariableIdPrefix) would suffice.

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

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

[rafaellehmkuhl]

Review follow-up — round 2

All round 1 findings (1.1, 1.2, 6.1) were confirmed addressed in the re-review. Two new minor items this round — declining both, reasoning below.

Won't change (with reasoning)

• 1.1 — pre-PR menu variables with a manually-edited custom ID losing edit/delete: real but unfixable cleanly. Going forward, menu-created variables are stamped userDefined: true, and the default auto-generated IDs use the user/custom/ prefix, so the normal flow is safe. For old persisted data there's no way to retroactively distinguish a Data-Lake-menu variable with a custom ID from a user/inputs/ input-element variable (both are persistent: true with no userDefined flag) — so a migration stamp on load would wrongly promote input-element variables to full user-defined and hand them delete buttons, which contradicts the intended "user-defined = created in the Data Lake menu only" rule. The affected case (pre-PR + menu-created + manually overridden ID) is very narrow and such variables still keep value editing through allowUserToChangeValue.
• 6.1 — normalizeDataLakeVariableId defensive trimming: intentional. The leading-space/slash normalization before the prefix check is a deliberate, explicitly requested behavior so IDs like user/custom/x or //user/custom/x still resolve as user-defined; keeping it.

[rafaellehmkuhl]

Review follow-up — round 3

Done

• branch: rebased onto current master (c38ee74). The PR was CONFLICTING against master; it is MERGEABLE again. The five commits applied without conflicts, so no code was changed by the resolution — only the base moved.

Won't change (with reasoning)

• 1.1 (round 2) — pre-PR menu variables with a manually-edited custom ID losing edit/delete, and 6.1 (round 2) — normalizeDataLakeVariableId defensive trimming: unchanged from the round 2 follow-up; the reasoning there still stands after the rebase.

[rafaellehmkuhl]

/review

[github-actions]

⚠️ IMPORTANT FIXES REQUIRED (Automated PR Review — round 3)

7 open (4 major, 2 minor, 1 nit — two of them disputed and waiting on a maintainer) · 3 closed in earlier rounds.

This PR changes which data-lake variables the Data Lake page lets you edit and delete, adds a value-only edit dialog so a built-in variable's value can be changed without touching its name or settings, marks the camera zoom and focus speed settings as user-editable and remembered across restarts, and makes variable creation reuse a previously saved value instead of overwriting it with the default. No code changed since the last review — the branch was rebased onto current master and the five commits reapplied unchanged — so this round is a full re-read of the pull request rather than of the last push, and it found five problems the earlier rounds missed, four of them major.

What still needs attention

#	Problem	What it means	Severity	Status
1.2	Camera zoom/focus buttons vanish from the joystick setup	You can no longer assign camera zoom-in, zoom-out, focus-in or focus-out to a joystick button or axis, because those entries disappear from the mapping list.	major	❌
1.3	Focus speed still forgotten on every restart	The camera focus speed you set goes back to the default every time Cockpit starts, so the PR only fixes half of what it set out to fix.	major	❌
1.4	Type check very likely fails	The project's type checker rejects the new lookup, so the build step that runs on every push would fail and the PR could not be merged as-is.	major	❌
1.5	Variables made from custom widgets can no longer be deleted	A variable you created from a custom widget input is now labelled "Cockpit internal" and its delete button is gone, so it stays in the list forever.	major	❌
1.1	Old hand-named variables lose their buttons	Someone who created a variable before this change and typed their own name for it loses the ability to edit or delete it.	minor	💬
3.1	Helper accepts an input nothing passes it	Extra unused flexibility in a new helper that someone has to read and maintain for no benefit today.	minor	❌
6.1	Defensive cleanup nothing needs	A few lines guard against variable names that the app never produces.	nit	💬

Since round 2 — 0 closed, 2 disputed, 5 new findings, comparing e1b8787 → 7549af0

Range. e1b8787d06e61b0354f3bce50782e7bcab25d6c8 → 7549af0957c787cda6c2a8360dfaab8194240b29.

The incremental diff is not usable this round. It lists roughly 240 files — CI workflows, the review guidelines, the base-station panel, the map composables, yarn.lock — none of which this PR touches. That is the rebase: the base moved to current master and everything master gained came through the comparison. All status judgements below, and every new finding, come from pr.diff (base…head), which is still the six files in pr.json.

The PR's own code did not change. @rafaellehmkuhl's round 3 follow-up says "no code was changed by the resolution — only the base moved". Checked, not taken: the code behind both carried findings is present verbatim in pr.diff (isUserDefinedDataLakeVariable with the info?.userDefined === true || normalizedId.startsWith(...) test, and normalizeDataLakeVariableId), and the six changed files and their line counts match the previous round.

Ledger rebuilt. previous-ledger.json arrived empty, so the ledger was reconstructed from previous-review.md. That review reused the ids 1.1, 1.2 and 6.1 for both its round 1 (closed) and round 2 (new) findings. To keep ids unique going forward, the three closed round 1 entries are carried as r1-1.1, r1-1.2 and r1-6.1; the open ones keep the bare 1.1 and 6.1 the author has been answering. Note that 6.1 was raised under the previous round's section numbering — under the current guidelines it belongs to 7. Code Quality & Style, which is where its body is reprinted below. Ids are never renumbered.

Status changes this round

• 1.1 — 💬 Disputed (was open). @rafaellehmkuhl calls it "real but unfixable cleanly": going forward menu-created variables are stamped, and for old data there is no way to tell a menu variable with a custom id from an input-element variable, so a retroactive stamp would wrongly hand input-element variables delete buttons. Verified against the code that his premise holds — old stored entries carry persistent: true and nothing else that separates the two — but an argument is not a code change, so the finding stays open and carries forward until the code moves or a maintainer settles it.
• 6.1 — 💬 Disputed (was open). @rafaellehmkuhl says the leading-space/slash normalization is deliberate and was explicitly requested, so ids like user/custom/x still resolve. The "explicitly requested" part is a claim about a conversation that is not in this repository and could not be checked; the code claim is accurate as far as it goes, and the finding was only ever a nit. Same rule applies: it stays open.
• No finding was closed this round, and no finding became obsolete.

Resolutions and decisions. resolutions.json is [] and decisions.json is [] — no /resolve has been issued on this PR and no dispute has been put to a vote, so nothing was closed by a maintainer and there is no unknown id to report back.

Discussion since round 2. Three comments, all from @rafaellehmkuhl: the round 2 follow-up declining both findings, the round 3 follow-up reporting the rebase and restating those declines, and the bare /review that triggered this run. Note that the round 2 follow-up's reasoning on 1.1 turns on input-element variables not being user-defined; new finding 1.5 is about a different consequence of that same decision (those variables losing their delete button and their "User defined" label), and is not answered by it.

Injection check. Nothing in the PR body, the diff, the commit messages or the comments contained text addressed to the reviewer.

Change map — what was established before judging

Claims. The PR body is only Fix #2774 / Fix #2775; the issue text is not reachable from here, so the claims below are taken from the commit messages in pr.json.

Claim	Verdict
"Internal variables with persistValue were always initialized from their default, overwriting values saved in localStorage."	Verified — src/libs/actions/data-lake.ts:80 assigns initialValue unconditionally, and loadPersistentVariables() (:34-42) only restores values for variables whose info was persisted, which a persistValue-without-persistent variable never is.
"Only zoom/focus speed settings should be configurable by the user; increase/decrease variables remain internal-only."	Contradicted as a safe change — allowUserToChangeValue is not only the Data Lake page's edit flag; it is also what the joystick mapping pickers filter on (src/views/ConfigurationJoystickView.vue:847 and :857). See 1.2.
"The Data Lake table hid the edit button for internal variables even when they were marked as user-editable."	Verified — src/views/ToolsDataLakeView.vue:117 gated the pencil on isUserDefinedVariable alone.
"Editing internal variables should only change the value, not metadata."	Verified — the value-only branch in saveVariable calls setDataLakeVariableData and never updateDataLakeVariableInfo.
"Internal variables explicitly marked persistent: false were shown as user-defined."	Verified — src/libs/vehicle/mavlink/vehicle.ts:1513 and src/stores/mainVehicle.ts:872 create telemetry variables with persistent: false, and the old test persistent != null (src/views/ToolsDataLakeView.vue:437) matched them.

Failure site.

• For the persistence symptom, the misbehaving code is createDataLakeVariable (src/libs/actions/data-lake.ts:75-96) — in the diff. But the second half of that failure lives in savePersistentValues (src/libs/actions/data-lake.ts:53-65), which is not in the diff and which the fix depends on. See 1.3.
• For the editability symptom, the misbehaving code is isUserDefinedVariable (src/views/ToolsDataLakeView.vue:437) and the pencil's v-if (:117) — both in the diff.
• No file is added by this PR, and it changes nothing under scripts/, .github/ or src/electron/.

Entry points.

Function	Reached from	Frequency
createDataLakeVariable (src/libs/actions/data-lake.ts:75)	src/main.ts:96 → setupPredefinedLakeAndActionResources → setupMavlinkCameraResources; plus ~20 registration sites (vehicle.ts:1513, omniscientLogger.ts, gnss.ts:365) and two user paths (DataLakeVariableDialog.saveVariable, InputElementConfig.saveOrUpdateParameter)	one-shot at bootstrap, then per user action
setupMavlinkCameraResources (src/libs/joystick/protocols/predefined-resources.ts:43)	src/main.ts:96	one-shot
isUserDefinedDataLakeVariable (src/libs/utils-data-lake.ts)	ToolsDataLakeView row v-if (:117, :125) and getVariableSource inside the filteredVariables computed; DataLakeVariableDialog.valueOnlyEditMode	per render of the Data Lake table, i.e. per user action
isUserEditableVariable (src/views/ToolsDataLakeView.vue:438)	same row v-if and editVariable	per render of the Data Lake table
editVariable (src/views/ToolsDataLakeView.vue:386)	pencil button click	per user action
valueOnlyEditMode, isValid, saveVariable (src/components/DataLakeVariableDialog.vue)	dialog open / Save click	per user action

Nothing here lands on a hot path: no changed function is reachable from mavlink:onIncomingMessage, dataLake:setVariable or a high-frequency watcher, and no changed function is unreachable either.

Invariants.

1. persistValue implies persistent. Enforced in the dialog (:81 disables the checkbox, :194-201 clears it) and assumed by loadPersistentVariables, which only restores values for variables whose info was persisted. The two new speed variables (predefined-resources.ts:45-49) deliberately break it, and the PR compensates inside createDataLakeVariable. Violating sites: every createDataLakeVariable caller; the PR covers the two it adds.
2. cockpit-persistent-data-lake-values holds a value for every persistValue variable. savePersistentValues rebuilds that object from scratch out of the variables registered at that moment (:53-65), so any call made before all persistValue variables exist drops the others. The PR adds exactly such a call. → 1.3.
3. allowUserToChangeValue means "the user may set this variable". Consumed by the joystick button picker (ConfigurationJoystickView.vue:847), the joystick axis picker (:857) and five custom-widget elements (Dial.vue:197, Slider.vue:121, Switch.vue:102, Checkbox.vue:110, Dropdown.vue:132), not only by the Data Lake table. The PR repurposes it as "the Data Lake table may edit this" and clears it on four variables. → 1.2.
4. user-defined = created through the Data Lake menu, recognised by a userDefined stamp or the user/custom/ id prefix. The only other producer of user-created variables is InputElementConfig.vue:516, which uses the user/inputs/ prefix and is not covered → 1.5; pre-PR menu variables with a hand-typed id are not covered either → 1.1.

1. Correctness & Implementation Bugs — 5 findings

1.2 major — Clearing allowUserToChangeValue removes camera zoom and focus from the joystick mapping pickers. src/libs/joystick/protocols/predefined-resources.ts:44 drops allowUserToChangeValue: true from commonVariableConfig, which is spread into camera-zoom-decrease, camera-zoom-increase, camera-focus-decrease and camera-focus-increase (:46-52). That flag is not private to the Data Lake page. Every data-lake variable becomes a joystick action (src/libs/joystick/protocols/data-lake.ts:37-44), and the joystick configuration screen filters the offered actions on exactly this flag:

• src/views/ConfigurationJoystickView.vue:844-848 — return dataLakeVariableInfo.allowUserToChangeValue && dataLakeVariableInfo.type !== 'string' for button actions;
• src/views/ConfigurationJoystickView.vue:853-859 — the same test for axis actions.

Those four variables exist for joystick binding: the transforming functions immediately below them compute ({{camera-zoom-increase}} - {{camera-zoom-decrease}}) * {{camera-zoom-speed}} (:64, :83). After this PR they no longer appear in the button or axis mapping lists, so a user cannot assign camera zoom or focus to a joystick at all. Existing mappings keep working — the runtime handler in data-lake.ts:63-72 does not consult the flag — so this fails silently for new setups and for anyone re-mapping a controller.

Fix: restore allowUserToChangeValue: true on the four increase/decrease variables, and gate the Data Lake table's new pencil on something that means what the PR wants it to mean. The table already has isUserEditableVariable; point it at a dedicated property (or at persistValue, which is what actually distinguishes the two speed settings) instead of overloading the flag three other features read.

1.3 major — The saved camera-focus-speed value is erased while camera-zoom-speed is being created, so focus speed still resets on every restart. The new restore block in createDataLakeVariable reads the whole persisted map and then, a few lines down, the pre-existing if (variable.persistValue && valueToSet !== undefined) savePersistentValues() fires. savePersistentValues (src/libs/actions/data-lake.ts:53-65, unchanged by this PR) does not merge — it rebuilds the stored object from dataLakeVariableInfo, which at that instant holds only the variables registered so far.

Walk it through with both speeds saved as, say, zoom 7 and focus 5 (setupMavlinkCameraResources, predefined-resources.ts:45-53, runs one-shot from src/main.ts:96):

1. camera-zoom-speed is created: its value is correctly restored to 7, then savePersistentValues() writes { camera-zoom-speed: 7 } — the stored camera-focus-speed: 5 is gone, because that variable is not registered yet.
2. camera-focus-speed is created three lines later, reads the map that was just overwritten, finds nothing, and falls back to the default 3.

User-defined variables survive this because they are persistent: true and so are already in dataLakeVariableInfo before any of this runs (loadPersistentVariables, :24-43); the new speed variables are persistValue without persistent, which is what puts them on the wrong side of the ordering. Net effect: zoom speed persists, focus speed silently reverts to 3 on every boot — half of what commit 1 sets out to fix, and the half that is broken is invisible until the user notices the focus behaving differently from the zoom.

Fix it at the chokepoint rather than at the two call sites: have savePersistentValues read the existing stored object and merge into it instead of rebuilding, and pair that with an explicit delete of the id in deleteDataLakeVariable (which currently relies on the rebuild to drop removed variables). Skipping the save when the value came from storage is a smaller change but only hides the ordering problem — the next persistValue variable registered after another one will hit it again.

1.4 major — The persisted-value lookup will not pass yarn typecheck. In the block added to createDataLakeVariable:

const savedValues = settingsManager.getKeyValue(persistentValuesKey)
if (savedValues && typeof savedValues === 'object' && savedValues[variable.id] !== undefined) {

getKeyValue is <T extends SettingValue>(key) => T | undefined (src/libs/settings-management.ts:279) and there is no inference site, so T falls back to its constraint string | number | boolean | object | null | undefined (:37). The truthiness test plus typeof … === 'object' narrows that to bare object, which has no index signature — so savedValues[variable.id] is TS7053 ("expression of type 'string' can't be used to index type 'object'") under the strict: true of tsconfig.app.json:35. yarn typecheck runs in CI at .github/workflows/ci.yml:141, and AGENTS.md requires the final implementation to be clean, so this fails the run rather than reaching a user. Note that the existing reader two functions above avoids it by going through Object.entries (:36) rather than indexing.

Fix — this is also shorter and is the optional-chaining form AGENTS.md asks for ("Use optional chaining (?.) when possible in typescript"), replacing the three-part guard and the cast at once:

const savedValues = settingsManager.getKeyValue<Record<string, string | number | boolean>>(persistentValuesKey)
const savedValue = savedValues?.[variable.id]
if (savedValue !== undefined) valueToSet = savedValue

1.5 major — Variables created from custom-widget input elements lose their delete button and are relabelled "Cockpit internal". isUserDefinedVariable (src/views/ToolsDataLakeView.vue:436-438) changes from persistent != null to isUserDefinedDataLakeVariable(id), which matches only the userDefined stamp or the user/custom/ prefix. InputElementConfig.vue:512-523 creates its variables with persistent: true, allowUserToChangeValue: true and the user/inputs/ prefix, and never stamps userDefined. For those variables, after this PR:

• the delete button disappears — its v-if is isCompoundVariable(item.id) || isUserDefinedVariable(item.id) (:125), untouched by the diff, and deleteVariable (:413-426) would refuse them anyway;
• the Source column reads "Cockpit internal" instead of "User defined" (getVariableSource, :321-331);
• editing narrows to value-only, since allowUserToChangeValue is true but isUserDefinedDataLakeVariable is false.

The user created these. The only other way to remove one is the delete action inside the input element's own configuration panel (InputElementConfig.vue:495-509), which is unreachable once the widget holding that element is gone — so the variable becomes permanently undeletable and keeps a row on the Data Lake page labelled as if Cockpit had made it. @rafaellehmkuhl's round 2 follow-up reasons about not stamping these variables, which is a different question; the removal of an affordance they already had is not covered by it and is not mentioned anywhere in the PR.

Fix: decide it explicitly rather than by side effect. Either recognise user/inputs/ as user-created for the delete and Source purposes while keeping full edit for user/custom/ — a second exported prefix beside userDefinedDataLakeVariableIdPrefix, checked in ToolsDataLakeView — or state in the PR that these variables are deliberately no longer deletable from this page and say where the user is meant to delete them instead.

1.1 minor (carried from round 2 — disputed) — A menu-created variable from before this PR whose id was typed by hand stops being recognised as user-defined. isUserDefinedDataLakeVariable (src/libs/utils-data-lake.ts) recognises a variable by info?.userDefined === true or by the user/custom/ id prefix. Variables created through the Data Lake dialog before this PR carry neither if the user turned on manual id editing (DataLakeVariableDialog.vue:173-177, :182-189) and typed an id of their own — the userDefined stamp is new in this PR, and the auto-generated prefix is the only other marker. In the Data Lake table (ToolsDataLakeView.vue:117, :125) such a variable loses both its edit and its delete button, and its Source flips to "Cockpit internal". Variables that kept the generated id are unaffected, which is the normal flow.

Author's position (see the since-last-round block): old stored data cannot distinguish a menu variable with a custom id from an input-element variable, so a retroactive stamp would wrongly grant delete buttons to the latter; the affected variables keep value editing through allowUserToChangeValue. That reasoning checks out against the code, but no code changed, so the finding stays open for a maintainer to settle. If it is accepted as-is, the cheapest mitigation is to stamp userDefined: true on the next full edit of such a variable — which saveVariable already does — and say so in the PR, so the recovery path is at least documented.

2. Persistence & User Data — inventory, no findings of its own

Two persisted keys are touched, both through settingsManager (src/libs/settings-management.ts), which keeps the local copy and syncs the key to the connected vehicle's BlueOS storage — so anything wrong here reaches every topside computer that talks to that vehicle, not just the one that made the change.

Key	Backend	What happened
cockpit-persistent-data-lake-values	settings-management.ts (local + vehicle-synced)	Added entries. camera-zoom-speed and camera-focus-speed become persistValue variables (predefined-resources.ts:45-49), so their values now live in this key. It is also now read during variable creation (data-lake.ts, createDataLakeVariable) rather than only at module load. The rebuild-not-merge behaviour of savePersistentValues is what finding 1.3 is about.
cockpit-persistent-data-lake-variables	settings-management.ts (local + vehicle-synced)	Reshaped, additively. DataLakeVariable gains an optional userDefined flag (src/types/data-lake.ts:38-41), written by saveVariable for variables created or fully edited through the dialog. Entries stored before this PR simply lack it.

Judgement on each: both keys are cockpit-prefixed and already existed; neither holds a machine-specific value (a zoom speed is a preference, not a device path), so vehicle syncing is the right backend. The userDefined addition is optional and read with a prefix fallback, so no automatic migration was written — which is the right call under the "migrations are a last resort" rule, and worth noting as a point in the PR's favour. The stored shape does not duplicate its own key. The one place where already-configured users are stranded on the old shape is finding 1.1 (disputed) and its user/inputs/ counterpart 1.5, both written up in section 1.

3. AGENTS.md Adherence — 1 finding

3.1 minor — isUserDefinedDataLakeVariable accepts a DataLakeVariable that no caller passes. In src/libs/utils-data-lake.ts the new helper is typed (variableOrId: string | DataLakeVariable) and spends three lines resolving that union:

const id = typeof variableOrId === 'string' ? variableOrId : variableOrId.id
const info = typeof variableOrId === 'string' ? getDataLakeVariableInfo(id) : variableOrId

Both call sites in this PR pass a string — ToolsDataLakeView.vue:437 and DataLakeVariableDialog.vue (valueOnlyEditMode) — and the object branch, along with the DataLakeVariable type import it forces, has no call site anywhere. AGENTS.md's minimalism ladder and the "no groundwork for future PRs" rule both point the same way: take a string, and let the PR that needs the object form add it next to the usage that justifies it. That drops the union, both ternaries and the import, for a helper that is then three lines long.

7. Code Quality & Style — 1 finding

6.1 nit (carried from round 2 — disputed; the id predates the current section numbering) — normalizeDataLakeVariableId guards against ids the app cannot produce. In src/libs/utils-data-lake.ts, const normalizeDataLakeVariableId = (id: string): string => id.trimStart().replace(/^\/+/, '') strips leading whitespace and slashes before the prefix test. Every id that reaches it comes from 'user/custom/' + machinizeString(name) (DataLakeVariableDialog.vue:186), from 'user/inputs/' + machinizeString(name) (InputElementConfig.vue:516), or from a hard-coded literal — none of which can carry a leading space or slash. A hand-typed id could, but that same path is the one finding 1.1 says is not recognised anyway.

Author's position: the normalization is deliberate and was explicitly requested, so user/custom/x and //user/custom/x still resolve. That request is not visible in this repository and could not be verified; the finding is a nit either way, and it is recorded here only so the ledger stays complete.

Sections with nothing to report (7)

4. Security — ✅ (no new dependency, no network call, no eval/v-html, no encoded blob, no env or secret use; the one added regex /^\/+/ is linear; the PR touches no workflow, build script or Electron main-process file — the CI and workflow changes visible in the incremental diff came from master via the rebase, not from this author)

5. Performance — ✅ (all seven changed functions traced to bootstrap or user-action entry points, none to onIncomingMessage, setVariable or a high-frequency watcher; the per-row work swaps a linear find for a hash lookup in isUserDefinedVariable and adds one back in isUserEditableVariable, so the table render is a wash; no listener, timer or watcher added, so nothing needs teardown)

6. UI / UX — ✅ (the value-only mode reuses the existing dialog: title stays centred, the mx-10 divider above the footer is intact, no divider under the header, globalGlassMenuStyles still on the one surface; no overlay-teleporting control added, so no missing theme="dark"; the pencil is an existing v-btn icon and the edit action is logged past-tense through logUserAction at ToolsDataLakeView.vue:391; no new snackbar, no dialog that can reopen from a loop)

8. Commit Hygiene — ✅ (five commits read from pr.json, each one logical step — restore, flag, button, dialog mode, classification — none reverting another, none bundling unrelated work, all under ~40 lines, no wip/fixup! noise, and the Fix #2774/Fix #2775 references are in the PR body where they belong rather than in any commit message)

9. Tests — ✅ (no test file is touched and none is weakened; src/tests/ contains nothing covering data-lake.ts or the Data Lake view, so nothing was removed to make this pass)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README change is owed; the JSDoc on the new exported helper and on the new userDefined field is present, typed and non-empty, and the renamed editVariable's block was updated)

11. Nitpicks / Optional — ✅ (checked the added template for sentence-case labels and stacked insets; Edit Variable Value matches the Title Case of the Edit Variable/New Variable siblings it sits beside, so it is left alone)

Complexity: complexity-report.json was not present for this head, so the measurement was unavailable — which of CI timing, a failed measurement or no report at all caused that is not something this run can tell. No complexity findings are raised this round.

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

[github-actions]

🙋 Decision needed — 1.1

Pre-PR menu-created variables with a hand-typed id stop being recognised as user-defined

The author's argument: Old stored data cannot distinguish a Data-Lake-menu variable with a custom id from an input-element variable, so stamping them retroactively would wrongly give input-element variables delete buttons; the affected case is narrow and those variables keep value editing through allowUserToChangeValue.

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.

[github-actions]

🙋 Decision needed — 6.1

normalizeDataLakeVariableId trims leading whitespace and slashes no known id carries

The author's argument: The leading-space and leading-slash normalization before the prefix check is deliberate and was explicitly requested, so ids like ' user/custom/x' or '//user/custom/x' still resolve as user-defined.

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]

Review follow-up — round 4

Done

• src/libs/joystick/protocols/predefined-resources.ts (1.2 — camera zoom/focus vanish from the joystick pickers): allowUserToChangeValue: true is back on all six camera variables; only persistValue: true is now added, and only to the two speed ones. Confirmed the flag is read by ConfigurationJoystickView.vue:847 and :857 plus the five custom-widget elements, so it is not the Data Lake page's to clear. The pencil gate stays on allowUserToChangeValue, which does mean "the user may set this variable's value" — the value-only dialog is exactly that, and it is the same capability a custom-widget Slider or Switch bound to those variables already has. So the increase/decrease rows now also get a value-only pencil.
• src/libs/actions/data-lake.ts (1.3 — saved camera-focus-speed erased while camera-zoom-speed is created): fixed at the chokepoint, as suggested. savePersistentValues merges into the stored object instead of rebuilding it from the variables registered so far, and deleteDataLakeVariable now removes its own id explicitly rather than relying on the rebuild to drop it.
• src/tests/libs/actions/data-lake.test.ts (1.3): new test covering the registration order and the delete path. It fails with expected 3 to be 5 against the old savePersistentValues — i.e. it reproduces the reported symptom of focus speed falling back to its default — and passes with the fix.
• src/libs/utils-data-lake.ts, src/types/data-lake.ts, src/views/ToolsDataLakeView.vue, src/components/DataLakeVariableDialog.vue (1.5, 1.1, 3.1, 6.1 — see below): isUserDefinedDataLakeVariable is now just getDataLakeVariableInfo(id)?.persistent === true. The userDefined flag, the exported user/custom/ prefix, normalizeDataLakeVariableId and the string | DataLakeVariable union are all gone, and so is the src/types/data-lake.ts change.

Done differently

• 1.4 — the persisted-value lookup and yarn typecheck: took the suggested rewrite (typed getKeyValue<Record<string, string | number | boolean>> plus optional chaining, which drops the three-part guard and the cast), so the code is as proposed. But the premise is wrong: every check passed on 7549af0, including the job that runs yarn typecheck at ci.yml:141. yarn typecheck resolves to vue-tsc --noEmit -p tsconfig.vitest.json, and with vue-tsc 2.0.10 it bails out in under a second on languageId not found for src/App.vue / !!sourceScript and exits 0 — I reproduced that locally both with the old indexing code and with the new. So this was never a merge blocker. That vue-tsc is silently a no-op is a real problem, but it is the whole repo's problem and not this PR's; I would rather fix it in its own PR than smuggle it in here.
• 1.5 (input-element variables losing delete and being relabelled) and 1.1 (pre-PR hand-named menu variables): rather than the second exported prefix, both are fixed by dropping the prefix-and-stamp approach altogether. persistent === true is the classifier the last commit was always reaching for — its own message says "stop treating persistent:false variables as user-defined", and != null → === true is the whole of that. Checked every createDataLakeVariable call site: the only two that pass persistent: true are DataLakeVariableDialog.vue:154 and InputElementConfig.vue:521, both user-created; the internal ones pass false or nothing. Every other persistent: true in src/ is a snackbar or v-dialog prop. So user/inputs/ variables keep their delete button and their "User defined" label, pre-PR hand-named menu variables keep both buttons, telemetry variables marked persistent: false stop being user-defined, and the two speed variables (persistValue without persistent) still get value-only editing. 1.1 no longer applies to any variable, so I have withdrawn the argument it was put to a vote on — the ballot is moot rather than unresolved.
• 3.1 and 6.1 fall out of the same deletion: no union to resolve, no DataLakeVariable import, and no normalizeDataLakeVariableId. On 6.1 specifically I am dropping the argument I made in round 2 rather than continuing to defend it, so that ballot is moot too.

One consequence worth flagging, since it reverses an answer from round 1: valueOnlyEditMode is once again driven by persistent, which is what r1-1.2 pointed at. That finding was closed as "the logic is sound for the current use cases" and never asked for a change; the helper I introduced in its place is what later grew 1.1, 1.5, 3.1 and 6.1, so this is a walk-back rather than a regression.

Net effect on the classification change: no variable loses an affordance it had before this PR.

[rafaellehmkuhl]

/review

[github-actions]

⚠️ IMPORTANT FIXES REQUIRED (Automated PR Review — round 4)

3 open (1 major, 2 minor) · 10 closed (7 of them this round).

This round the PR was largely rewritten: the camera zoom and focus variables keep the flag that puts them in the joystick mapping lists, the "remember this value between boots" storage now merges instead of being rebuilt from scratch, and the whole id-prefix scheme for telling "a variable the user made" from "a variable Cockpit made" was deleted in favour of a single question — was this variable asked to survive a restart? That last simplification closes four findings and opens one: a variable the user creates through the Data Lake page while declining to keep it between restarts is now treated as one of Cockpit's own, so it loses its delete and rename buttons and is labelled as internal.

What still needs attention

#	Problem	What it means	Severity	Status
1.6	Variables you create without "persist between boots" become undeletable	A variable you make yourself, with the "keep between restarts" box unticked, is shown as one of Cockpit's own and loses its rename and delete buttons, so you cannot get rid of a row you just created.	major	❌
1.7	Value editor shows blank for a variable sitting at zero	Opening the new value editor on a variable whose value is 0 (or off) shows an empty box with Save greyed out, so it looks like the variable has no value and you must retype one.	minor	❌
6.2	Pencil offered on live joystick axis readings	The Data Lake page now offers to edit the joystick axis values, but anything you type there is wiped the instant you move the stick.	minor	❌

Since round 3 — 7 closed (5 addressed, 2 no longer applicable), 3 new findings, comparing 7549af0 → 875df00

Range. 7549af0957c787cda6c2a8360dfaab8194240b29 → 875df000c718474107652a32a729a814189967a3.

The incremental diff is the whole PR again. incremental.diff lists the same six files as pr.json, with per-file line counts identical to the PR totals (+32/-10, +22/-7, +3/-2, +11/-0, +39/-0, +12/-7), and the five commits in pr.json all carry new oids with a fresh committedDate — i.e. the branch was rewritten, not appended to. So it tells us nothing about what moved since round 3 that pr.diff does not, and every status judgement below was made against pr.diff (base…head) and the base checkout.

Status changes this round

• 1.2 — ✅ Addressed. predefined-resources.ts:44 keeps allowUserToChangeValue: true in commonVariableConfig; the new speedVariableConfig only adds persistValue: true and only the two speed variables use it. All six camera variables therefore still pass the ConfigurationJoystickView.vue:848 / :858 filters.
• 1.3 — ✅ Addressed, at the chokepoint that was named. savePersistentValues now seeds itself from the stored object (data-lake.ts, getPersistentValues) instead of rebuilding it, and deleteDataLakeVariable removes its own id explicitly rather than relying on the rebuild to prune. Walked the bootstrap order again with both speeds stored: camera-zoom-speed's save now writes {zoom, focus} rather than {zoom}, so camera-focus-speed still finds its value three lines later.
• 1.4 — ✅ Addressed. The code is now the typed-generic plus optional-chaining form that was proposed: getKeyValue<Record<string, string | number | boolean>>(persistentValuesKey) ?? {}, which satisfies the T extends SettingValue constraint (settings-management.ts:37 includes object) and indexes a Record, not a bare object. @rafaellehmkuhl disputes the finding's premise rather than its fix, reporting that vue-tsc exits 0 without checking anything on this repo; that is a claim about a run on his machine which cannot be checked from here, and it does not change the status — the recommended code landed.
• 1.5 — ✅ Addressed. isUserDefinedDataLakeVariable is now getDataLakeVariableInfo(id)?.persistent === true, and InputElementConfig.vue:521 passes persistent: true, so custom-widget input variables keep their delete button and their "User defined" label. Checked every createDataLakeVariable call site in src/: only that one and DataLakeVariableDialog.vue:280 can pass persistent: true; the internal registrations pass false (vehicle.ts:1513, mainVehicle.ts:892) or leave it unset.
• 3.1 — ✅ Addressed. The helper takes a string, the union and both ternaries are gone, and so is the DataLakeVariable import it forced.
• 1.1 — ⚪ No longer applicable. The id-prefix classification the finding was about was deleted outright. Classification is now persistent === true, and anything that survived a boot is in cockpit-persistent-data-lake-variables only because savePersistentVariables (data-lake.ts:47) filtered on persistent being truthy — so a pre-PR menu variable with a hand-typed id keeps both buttons. Its ballot (decision comment) is pending with no votes cast; the vote is technically still open but now moot, since the code closed the finding rather than the argument.
• 6.1 — ⚪ No longer applicable. normalizeDataLakeVariableId no longer exists in the diff. Its ballot (decision comment) is also pending with no votes, and moot for the same reason.
• New this round: 1.6, 1.7 (section 1) and 6.2 (section 6), all from a full re-read of pr.diff.

Resolutions and decisions. resolutions.json is [] — no /resolve has been issued on this PR, so nothing was closed by command and there is no unknown id to report back. decisions.json holds two entries, both gated: true and both pending with empty up/down; a pending verdict decides nothing, and both findings closed on the code regardless.

Discussion since round 3. Two comments, both from @rafaellehmkuhl: a round-4 follow-up walking through each finding, and the bare /review that triggered this run. The follow-up's factual claims were checked rather than taken — the allowUserToChangeValue restoration, the merge-and-explicit-delete rewrite, the persistent === true classifier and the enumeration of persistent: true call sites all hold up against the code. One claim does not: "no variable loses an affordance it had before this PR". The old test was persistent != null (ToolsDataLakeView.vue:437), which matched persistent: false as well as true, and the Data Lake dialog lets a user create or edit a variable with "Persist variable between boots" unticked — that is finding 1.6 below. The follow-up also flags, correctly, that valueOnlyEditMode is once again keyed on persistent, which is where round 1 started.

Injection check. Nothing in the PR body, the diff, the commit messages, the comments or the decision entries contained text addressed to the reviewer. The follow-up comment is written as HTML markup; it is summarised above rather than quoted verbatim.

Change map — what was established before judging

Claims. The PR body is only Fix #2774 / Fix #2775; the issues are not reachable from here, so the claims are read off the commit messages in pr.json, plus the one claim the round-4 follow-up adds.

Claim	Verdict
"Internal variables with persistValue were always initialized from their default, overwriting values saved in localStorage."	Verified — base data-lake.ts:80 assigns initialValue unconditionally, and loadPersistentVariables (:34-42) restores values only for variables whose info was persisted, which a persistValue-without-persistent variable never is.
"The speed settings are the two camera variables a user tunes … the increase/decrease variables stay as they were."	Verified — predefined-resources.ts:44-53: commonVariableConfig is unchanged and persistValue: true is added only via speedVariableConfig.
"The Data Lake table hid the edit button for internal variables even when they were marked as user-editable."	Verified — base ToolsDataLakeView.vue:117 gated the pencil on isUserDefinedVariable alone.
"Editing internal variables should only change the value, not metadata."	Verified — the value-only branch of saveVariable calls setDataLakeVariableData and never updateDataLakeVariableInfo.
"Internal variables explicitly marked persistent: false were shown as user-defined."	Verified — vehicle.ts:1513 and mainVehicle.ts:892 create telemetry variables with persistent: false, and the old persistent != null test matched them.
(follow-up) "No variable loses an affordance it had before this PR."	Contradicted — persistent != null also matched persistent: false, which is what the Data Lake dialog writes when the user unticks its persist checkbox (DataLakeVariableDialog.vue:70-75, :141, :280). → 1.6

Failure site.

• Persistence symptom: createDataLakeVariable (data-lake.ts:75-96) and savePersistentValues (:53-65) — both now in the diff, which is what closed 1.3.
• Editability symptom: isUserDefinedVariable (ToolsDataLakeView.vue:436) and the pencil's v-if (:117) — both in the diff.
• One file is added, src/tests/libs/actions/data-lake.test.ts, read in full: it mocks settings-management with a plain object, dynamically imports the module under test in each case, and asserts both the registration-order symptom and the delete path. It shares module state across its two cases (case 2 asserts on values case 1 wrote), but both also pass in isolation because the fixture seeds the same numbers. loadPersistentVariables() runs at import (data-lake.ts:265) and is harmless under the mock. Nothing under scripts/, .github/ or src/electron/ is touched.

Entry points.

Function	Reached from	Frequency
getPersistentValues (data-lake.ts, new)	savePersistentValues, createDataLakeVariable, deleteDataLakeVariable	one-shot at bootstrap, then per user action (and per joystick frame if a persistValue variable is bound to an axis — see section 5)
savePersistentValues (data-lake.ts)	createDataLakeVariable, updateDataLakeVariableInfo, setDataLakeVariableData	as above
createDataLakeVariable (data-lake.ts:75)	src/main.ts:96 → setupPredefinedLakeAndActionResources; ~20 registration sites; two user paths (DataLakeVariableDialog.saveVariable, InputElementConfig.saveOrUpdateParameter)	one-shot at bootstrap, then per user action
deleteDataLakeVariable (data-lake.ts:140)	ToolsDataLakeView.deleteVariable, InputElementConfig	per user action
setupMavlinkCameraResources (predefined-resources.ts:43)	src/main.ts:96	one-shot
isUserDefinedDataLakeVariable (utils-data-lake.ts)	ToolsDataLakeView row v-ifs (:117, :125) and getVariableSource (:321-331); DataLakeVariableDialog.valueOnlyEditMode	per render of the Data Lake table, i.e. per user action
isUserEditableVariable (ToolsDataLakeView.vue, new)	same row v-if and editVariable	per render of the Data Lake table
editVariable (ToolsDataLakeView.vue)	pencil click	per user action
valueOnlyEditMode, isValid, saveVariable (DataLakeVariableDialog.vue)	dialog open / Save click	per user action

No changed function is unreachable, and none sits on onIncomingMessage, addToDataLake or notifyListeners.

Invariants.

1. persistValue implies persistent. Enforced in the dialog (:81 disables the checkbox, :194-201 clears it) and assumed by loadPersistentVariables. The two speed variables deliberately break it and the PR compensates inside createDataLakeVariable. Violating sites: every createDataLakeVariable caller; the PR covers the two it adds.
2. cockpit-persistent-data-lake-values holds a value for every persistValue variable. Now maintained by merging, with deleteDataLakeVariable as the only pruner. The one site that can still leave an orphan is updateDataLakeVariableInfo when a user turns persistValue off — it takes the if (variable?.persistValue) branch not at all, so the old entry is simply left behind. Harmless (loadPersistentVariables filters on persistValue), and written up as prose in section 2 rather than as a finding.
3. user-defined ⇔ persistent === true. The new classifier. Producers of user-created variables: InputElementConfig.saveOrUpdateParameter (hard-codes true, covered) and DataLakeVariableDialog.saveVariable (takes it from a user checkbox, so it can be false — not covered → 1.6). Consumers: the pencil, the delete button and the Source column in ToolsDataLakeView, plus valueOnlyEditMode. No internal registration passes persistent: true, which was checked across all createDataLakeVariable call sites in src/.
4. allowUserToChangeValue means "the user may set this variable". Restored on all six camera variables, and now also the Data Lake pencil's gate. Every holder of the flag therefore gains a pencil, which includes the six joystick axis inputs created at predefined-resources.ts:158-162 → 6.2.

1. Correctness & Implementation Bugs — 2 findings

1.6 major — A variable created through the Data Lake dialog with "Persist variable between boots" unticked is classified as Cockpit-internal and loses its delete and full-edit buttons. isUserDefinedDataLakeVariable (src/libs/utils-data-lake.ts) is now getDataLakeVariableInfo(id)?.persistent === true, replacing the old persistent != null (ToolsDataLakeView.vue:437). Those two differ on exactly one value: persistent: false, which is what the dialog writes whenever the user unticks its own checkbox:

• DataLakeVariableDialog.vue:70-75 renders the "Persist variable between boots" checkbox bound to variable.persistent (default true, :141);
• saveVariable (:280) spreads that reactive object straight into createDataLakeVariable/updateDataLakeVariableInfo, so persistent is false, not absent.

After this PR such a variable, in ToolsDataLakeView: loses its delete button (v-if="isCompoundVariable(item.id) || isUserDefinedVariable(item.id)", :125, untouched by the diff — and deleteVariable at :420 would refuse it anyway); reads "Cockpit internal" in the Source column (getVariableSource, :321-331); and drops from full edit to the new value-only dialog, so its name, type and description can no longer be changed. Before this PR it had all three.

The permanent case is the edit path, not the create path. updateDataLakeVariableInfo only calls savePersistentVariables() when the new value of persistent is truthy (data-lake.ts:105-107), so turning the checkbox off on an existing user variable never removes it from cockpit-persistent-data-lake-variables. It is reloaded on the next boot with persistent: false and, from this PR on, can never be deleted from this page again.

The premise stated in the new helper's own JSDoc — "Only variables the user creates ask to be persisted between boots" — inverts what the flag means. src/types/data-lake.ts:27-29 documents persistent as "whether the variable existance should be persisted between boots", which is a user choice, not a provenance marker; the diff overloads it as the latter.

Fix: mark provenance explicitly instead of inferring it, without going back to id prefixes. Have the two user-creating call sites stamp userDefined: true (DataLakeVariableDialog.saveVariable already builds a fresh object there, and InputElementConfig.saveOrUpdateParameter:513-523 is one more property), and make the helper info?.userDefined === true || info?.persistent === true. The second half is the legacy fallback for data stored before this PR, and it is exactly correct for it: anything that survived a boot is in storage only because savePersistentVariables filtered on persistent (data-lake.ts:47). That covers old data, new non-persistent user variables, and still excludes the persistent: false telemetry variables the last commit set out to exclude, since nothing in src/ stamps them.

1.7 minor — The value-only editor opens blank, with Save disabled, for any variable whose current value is 0 or false. The dialog fills its field with initialValue.value = currentValue ? currentValue.toString() : '' (DataLakeVariableDialog.vue:219, base numbering — the line is not in the diff), and the new value-only branch of isValid requires initialValue.value !== '' (:261-266 of the diff). So for a variable resting at 0 the user sees an empty "Value (number)" box and a greyed-out Save until they type something.

That falsy check is pre-existing, but this PR is what routes 0-valued variables into this dialog: camera-zoom-increase, camera-zoom-decrease, camera-focus-increase and camera-focus-decrease are created with the value 0 (predefined-resources.ts:46-52) and sit at 0 whenever the bound button is not held, and the same is true of every joystick axis input (see 6.2). In the old full-edit path the blank box was cosmetic — Save stayed enabled because the name and id were filled in, and an empty value simply left the value alone. In the new path it blocks the dialog's only action.

Fix: initialValue.value = currentValue !== undefined ? String(currentValue) : ''. That is one line, it fixes the misleading display in both modes, and it lets isValid's !== '' test mean what it says.

2. Persistence & User Data — inventory, one behaviour change worth recording

One persisted key is touched, through settingsManager (src/libs/settings-management.ts), which keeps a local copy and syncs the key to the connected vehicle's BlueOS storage — so anything wrong here reaches every topside computer that talks to that vehicle. The userDefined field that round 3 reviewed is gone: src/types/data-lake.ts is no longer in the PR at all, so cockpit-persistent-data-lake-variables is now untouched in shape and only affected indirectly (1.6).

Key	Backend	What happened
cockpit-persistent-data-lake-values	settings-management.ts (local + vehicle-synced)	Added entries, and its write strategy changed. camera-zoom-speed and camera-focus-speed become persistValue variables (predefined-resources.ts:45, :49), so their values now live in this key. It is also read during variable creation rather than only at module load, and savePersistentValues now merges into the stored object instead of rebuilding it.
cockpit-persistent-data-lake-variables	settings-management.ts (local + vehicle-synced)	Unchanged by the diff. Listed because 1.6 turns on what it already stores: entries written before this PR carry persistent: true, which is what makes the proposed `userDefined

Judgement: both keys are cockpit-prefixed and already existed; a camera zoom speed is a vehicle preference rather than a machine-specific value (no device path, no window geometry), so vehicle-syncing is the right backend. No automatic migration is written, and none is needed — the merge is additive and the stored shape does not duplicate its own key. Two consequences of the new write strategy are recorded here rather than raised as findings, because neither is reachable as a defect: pruning now happens only in deleteDataLakeVariable, so an entry whose variable has persistValue turned off is left behind (inert, since loadPersistentVariables filters on persistValue); and getPersistentValues() returns a live reference into the settings manager's cachedSettings (settings-management.ts:328-338 returns the cache itself, and getKeyValue returns the stored value by reference), which both new writers mutate in place before calling the debounced setKeyValue. That happens to be safe today — the debounced write re-wraps the same object — but it is a coupling to an implementation detail of another module that nothing documents.

6. UI / UX — 1 finding

6.2 minor — The new pencil appears on the six joystick axis input variables, where the typed value is overwritten by the next joystick frame. The gate is now isUserDefinedVariable(item.id) || isUserEditableVariable(item.id) (ToolsDataLakeView.vue:117), and isUserEditableVariable is allowUserToChangeValue === true. Besides the camera variables, that flag is carried by every variable created in setupJoystickAxesResources (predefined-resources.ts:158-162): inputs/mavlink/axis-x, -y, -z, -r, -s, -t. Those are live readings — src/libs/joystick/protocols/data-lake.ts:84-88 writes the scaled axis value on every controller update for each mapped axis, and the default profiles map four of them (src/assets/joystick-profiles.ts:51-54). So with a joystick connected, a user who opens the pencil on "Axis X", types a value and saves sees it replaced before the dialog finishes closing, with no feedback explaining why.

This is the part of the widened gate that the round-4 follow-up's reasoning does not cover: it argues, reasonably, that a value-only pencil on camera-zoom-increase is the same capability a custom-widget Slider bound to that variable already has, and that holds for variables nothing else writes continuously. It does not hold for a variable that is written every frame from hardware.

Fix: exclude the continuously-written inputs rather than widening for all of them — gate the pencil on the variable also not being a joystick input (the inputs/mavlink/ ids are already generated from one table, predefined-resources.ts:20-25), or drop allowUserToChangeValue: true from setupJoystickAxesResources' commonVariableConfig if those axes never needed to be user-settable in the first place. Note the second option is not free: that same flag is what lists them in the joystick pickers (ConfigurationJoystickView.vue:848, :858) — the trap that finding 1.2 was about — so check that before taking it.

Sections with nothing to report (8)

3. AGENTS.md Adherence — ✅ (no dependency added, package.json untouched; the new exported helper's JSDoc is typed and non-empty; optional chaining used in both new lookups; the editUserDefinedVariable → editVariable rename is justified by the widened gate rather than incidental; nothing exported without a call site in this PR — the string | DataLakeVariable union and the id-prefix export from round 3 are both gone, which is a net deletion)

4. Security — ✅ (no new dependency, no network call, no eval/v-html, no encoded blob, no env or secret use; no regex left in the diff at all after normalizeDataLakeVariableId was deleted; no workflow, build script, Dockerfile or Electron main-process file touched; the added test mocks a local module and reaches nothing outside the process)

5. Performance — ✅ (all nine changed functions traced to bootstrap or user-action entry points, none to onIncomingMessage, addToDataLake or notifyListeners; getLocalSettings serves getPersistentValues from cachedSettings, so the merge adds an in-memory read rather than a localStorage parse; the one frequency worth naming is setDataLakeVariableData → savePersistentValues, which is O(all data-lake variables) and now also fires for the two speed variables — but only if a user maps a speed to a joystick axis, which is not a default mapping, and that path predates this PR for user-created persistValue variables; no listener, timer or watcher added, so nothing needs teardown)

7. Code Quality & Style — ✅ (complexity-report.json is present for this head: 108 functions measured across the 6 changed files, triggeredCount 0 and not truncated, so nothing the diff writes crosses the complexity-12 or depth-4 thresholds; both reworded comments in data-lake.ts sit on lines the diff changes, so the comment-immutability rule is not engaged; no any, no scoped CSS added, no re-implementation of an existing helper — getPersistentValues is the extraction of a read that was about to appear three times; the added test file needs no JSDoc under jsdoc/require-jsdoc's ArrowFunctionExpression: false)

8. Commit Hygiene — ✅ (five commits read from pr.json, all rewritten this round but still one logical step each — restore, persist the speeds, show the button, value-only dialog, classification — none reverting another, none bundling unrelated work, all small, no wip/fixup! noise, subjects fix:-prefixed in line with the repository's history, and the Fix #2774/Fix #2775 references kept in the PR body rather than in any commit message; the last commit's subject, "stop treating persistent:false variables as user-defined", is an accurate description of what the code now does)

9. Tests — ✅ (no existing test is touched or weakened; the PR adds src/tests/libs/actions/data-lake.test.ts, which reproduces the 1.3 symptom and the delete path. Worth knowing rather than fixing here: CI runs yarn lint, yarn typecheck and yarn build only (.github/workflows/ci.yml:137-142) and never yarn test:ci, so this test will not run on any push — that is the repository's gap, not this PR's)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README change is owed; the one added export carries a complete typed JSDoc — although its summary sentence is the mistaken premise behind finding 1.6 and needs rewording with the fix)

11. Nitpicks / Optional — ✅ (checked the added template for sentence case and stacked insets: "Edit Variable Value" matches the Title Case of the Edit Variable/New Variable siblings it sits beside, and the added name line reuses the existing flex flex-col gap-4 spacing rather than inventing a margin)

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

[rafaellehmkuhl]

Review follow-up — round 5

Done

• src/types/data-lake.ts, src/libs/utils-data-lake.ts, src/components/DataLakeVariableDialog.vue, src/components/InputElementConfig.vue (1.6 — dialog-created variables with persistent: false lose delete and full edit): took the proposed fix. Provenance is stamped explicitly — userDefined: true on the object saveVariable already builds (DataLakeVariableDialog.vue:301) and on saveOrUpdateParameter's (InputElementConfig.vue:523) — and the helper is now info?.userDefined === true || info?.persistent === true, with the second half as the fallback for data stored before the stamp. Nothing is migrated: an unticked-persist variable was never in cockpit-persistent-data-lake-variables to begin with, and stored variables all carry persistent: true, so the fallback covers them where they are. The helper's JSDoc no longer claims persistence implies provenance.
• src/tests/libs/utils-data-lake.test.ts (1.6): new test over the five cases — stamped and persistent, stamped and not persistent, legacy persisted, persistent: false telemetry, and a persistValue camera speed. Reverting just the classifier makes it fail with expected false to be true on the stamped-but-not-persistent case, i.e. it reproduces 1.6.
• src/components/DataLakeVariableDialog.vue (1.7 — value editor blank at zero): initialValue.value = currentValue !== undefined ? String(currentValue) : '', as proposed. Fixes the misleading blank box in the full-edit path too.

Won't change (with reasoning)

• 6.2 — pencil offered on live joystick axis readings: the symptom is real, but the class it belongs to is not "the six joystick axis inputs" — it is "any variable a joystick control is currently mapped to". src/libs/joystick/protocols/data-lake.ts:63-72 writes a mapped button's target on every frame as well, 0 when the button is not held, so camera-zoom-increase is overwritten just as fast as inputs/mavlink/axis-x the moment a user maps a button to it — and that is the variable the round-4 reasoning was accepted on. Excluding the six generated ids fixes the default-profile instance and leaves the identical sibling case broken.

  The other suggested option is out for the reason the finding already flags: allAvailableAxes() (src/libs/joystick/protocols.ts:12-14) builds the axis picker from data-lake actions and ConfigurationJoystickView.vue:857 filters it on allowUserToChangeValue, so clearing the flag in setupJoystickAxesResources would drop all six axes from the axis mapping picker — 1.2 again.

  The only gate that matches the real class is "is this variable currently the target of a joystick mapping", read live from controllerStore.protocolMapping. That means a new store dependency in the Data Lake view and a button that appears and disappears as mappings change, to prevent a typed value being replaced in a table whose Value column shows it being replaced. Not worth it here; if you'd rather have it, I'll take it in its own PR.

Notes

• Commit 5's subject was reworded — it now stamps provenance rather than only excluding persistent: false, so "stop treating persistent:false variables as user-defined" no longer described it.
• The 1.1 and 6.1 ballots both carry a maintainer +1, so both are settled as accepted; the code had already made them moot.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 5)

1 open (1 minor, disputed) · 12 closed (2 of them this round).

Both findings from round 4 that were about code are fixed, and fixed at the root. Provenance is now stamped explicitly when a variable is created by the user, instead of being guessed from whether the user asked to keep it between restarts, so a variable you make with the "keep between restarts" box unticked keeps its rename and delete buttons; and the value editor now shows a value of zero as 0 instead of an empty box with a greyed-out Save. The one remaining item is not a code question but a scope question: the pencil the PR adds also appears on readings a connected joystick overwrites many times a second, and the author argues the right fix is bigger than this PR and belongs in its own.

What still needs attention

#	Problem	What it means	Severity	Status
6.2	Pencil offered on live joystick axis readings	The Data Lake page offers to edit the joystick axis values, but anything you type there is wiped the instant you move the stick. The author agrees the symptom is real and argues the proper gate is out of scope here.	minor	💬

Since round 4 — 2 closed (both addressed), 1 disputed, 0 new findings, comparing 875df00 → 5b83ffb

Range. 875df000c718474107652a32a729a814189967a3 → 5b83ffb5f590e68d85f941b784e2d8a879d10297.

The incremental diff looks usable this round. PREV_SHA is not among the five commits in pr.json, so the branch was rewritten again (the last two commits carry fresh committedDates while the first three keep theirs), but unlike round 3→4 the compare is a genuine subset of the PR: six files, not the nine pr.json lists, and the per-file counts are smaller than the PR totals. It is consistent with an amend of the last two commits. I still made every status judgement below against pr.diff (base…head) and the base checkout, and re-ran the whole review over pr.diff, not over the increment.

Status changes this round

• 1.6 — ✅ Addressed, and every part of it. The finding asked for three things and all three landed: (a) the two user-creating call sites stamp provenance — DataLakeVariableDialog.saveVariable now builds { ...variable, allowUserToChangeValue: true, userDefined: true } and InputElementConfig.saveOrUpdateParameter adds userDefined: true (InputElementConfig.vue:523); (b) the classifier is info?.userDefined === true || info?.persistent === true (utils-data-lake.ts); (c) the JSDoc no longer claims that persistence implies provenance. Walked the two cases the finding named. A variable created from the dialog with the persist box unticked now has userDefined: true in the info registry, so the pencil (ToolsDataLakeView.vue:117), the delete button (:125), deleteVariable's own guard (:420) and the Source column (:321-331) all see it as user-defined again, and valueOnlyEditMode keeps it in the full editor. The permanent-lockout case is covered too: unticking persist on an existing user variable does not rewrite cockpit-persistent-data-lake-variables (data-lake.ts:105-107 only saves on a truthy persistent), so the stored record still carries persistent: true and the fallback recognises it after a reboot. Re-checked that the fallback cannot capture anything internal: of the 30 createDataLakeVariable call sites in src/, only the dialog and InputElementConfig can pass persistent: true — every other persistent: true in the tree is a snackbar or dialog prop, and the telemetry registrations pass false (vehicle.ts:1513, mainVehicle.ts:892). MiniWidgetInstantiator.vue:110 re-creates a variable from widget options stored earlier; those objects carry persistent: true from before the stamp existed, so they land on the fallback rather than losing anything.
• 1.7 — ✅ Addressed. initialValue.value = currentValue !== undefined ? String(currentValue) : '', exactly the proposed one-liner. Checked it round-trips rather than just displaying: saveVariable's if (initialValue.value) guard treats the string '0' as truthy, validateValue accepts '0' for a number and 'false' for a boolean, and the value-only branch of isValid now passes because the field is no longer empty. The full-edit path gets the same correction for free.
• 6.2 — 💬 Disputed, so it stays open. The author does not change the code and argues the finding under-describes its own class. I checked the argument against the code and its factual half holds: src/libs/joystick/protocols/data-lake.ts:63-72 writes a mapped button's target variable on every controller update — 0 when the button is not held — so camera-zoom-increase is overwritten just as fast as inputs/mavlink/axis-x once a user maps a button to it, and excluding the six generated axis ids would leave that sibling case alone. The other half is a scope judgement, not a fact: he prefers a gate that reads the live joystick mapping from the controller store, and would rather ship it separately. That is a reasonable position and it is a maintainer's call, not mine — the symptom both of us agree on is still in this PR, so the finding is carried rather than closed. For the record, the default ROV profile does map four of the six axes (joystick-profiles.ts:51-54), so the instance is reachable out of the box.
• New this round: none. The full re-read of pr.diff surfaced nothing that was not already recorded.

Resolutions and decisions. resolutions.json is [] — no /resolve has been issued on this PR, so nothing was closed by command and there is no unknown id to report back. decisions.json is [] for this run: no vote was delivered, so nothing was applied from it and no finding was closed, dropped or re-opened on that basis. The follow-up comment states that the round-3 ballots for 1.1 and 6.1 now carry a maintainer +1. I cannot corroborate that from the inputs I am given, and nothing turns on it either way: both findings were already closed as obsolete on the code in round 4, and an accepted argument on an already-closed finding changes nothing. If a maintainer wants that acceptance recorded, a /resolve naming the ids is the way to do it.

Discussion since round 4. Two comments, both from @rafaellehmkuhl: a round-5 follow-up and the bare /review that triggered this run. The follow-up's factual claims were checked rather than taken. Verified: the explicit stamp at both call sites; the userDefined || persistent classifier; "an unticked-persist variable was never in cockpit-persistent-data-lake-variables to begin with" (both createDataLakeVariable:87 and savePersistentVariables:47 gate on a truthy persistent, so no migration is owed); the added test does reproduce 1.6, since reverting only the classifier fails its stamped-but-not-persistent case; and the whole of the 6.2 reasoning above. Not corroborated: the claim about the two ballots, above.

Injection check. Nothing in the PR body, the diff, the commit messages, the comments or the decision entries contained text addressed to the reviewer. The follow-up comment is written as HTML markup; it is summarised above rather than quoted verbatim.

Change map — what was established before judging

Claims. The PR body is only Fix #2774 / Fix #2775; the issues are not reachable from here, so the claims are read off the commit messages in pr.json, plus the ones the round-5 follow-up adds.

Claim	Verdict
"Internal variables with persistValue were always initialized from their default, overwriting values saved in localStorage."	Verified — base data-lake.ts:80 assigns initialValue unconditionally, and loadPersistentVariables (:34-42) restores values only for variables already in the info registry at module load, which a persistValue-without-persistent variable never is.
"The speed settings are the two camera variables a user tunes … the increase/decrease variables stay as they were."	Verified — predefined-resources.ts:44-53: commonVariableConfig is unchanged and persistValue: true arrives only through the new speedVariableConfig.
"The Data Lake table hid the edit button for internal variables even when they were marked as user-editable."	Verified — base ToolsDataLakeView.vue:117 gated the pencil on isUserDefinedVariable alone.
"Editing internal variables should only change the value, not metadata."	Verified — the value-only branch of saveVariable calls setDataLakeVariableData and never updateDataLakeVariableInfo.
"The page inferred ownership from the persistence flags … variables created from the page or from a custom widget input now say so, and the flag is read only as a fallback."	Verified — base :437 was persistent != null, which matched the persistent: false telemetry variables; both user-creating sites now stamp userDefined: true, and the fallback is unreachable for internal registrations (all 30 call sites enumerated).
(follow-up) "An unticked-persist variable was never in cockpit-persistent-data-lake-variables to begin with, so nothing needs migrating."	Verified — createDataLakeVariable:87 and savePersistentVariables:47 both filter on a truthy persistent.
(follow-up) "A mapped button's target is overwritten every frame too, so the class is wider than the six axis inputs."	Verified as fact — joystick/protocols/data-lake.ts:63-72. It widens finding 6.2 rather than answering it.
(follow-up) "The 1.1 and 6.1 ballots both carry a maintainer +1."	Not corroborated — decisions.json is empty this run. Moot: both are already closed as obsolete.

Failure site.

• Persistence symptom: createDataLakeVariable (data-lake.ts:75-96) and savePersistentValues (:53-65) — both in the diff.
• Editability symptom: isUserDefinedVariable (ToolsDataLakeView.vue:436) and the pencil's v-if (:117) — both in the diff.
• Provenance: now a single chokepoint, isUserDefinedDataLakeVariable (utils-data-lake.ts), with two producers that stamp and one legacy fallback — which is what closed 1.6 rather than patching the symptom at the table.
• Two added files, both read in full. src/tests/libs/actions/data-lake.test.ts mocks settings-management with a plain object, dynamically imports the module under test in each case, and asserts the registration-order symptom and the delete path; it shares module state across its two cases, but both also pass in isolation because the fixture seeds the same numbers. src/tests/libs/utils-data-lake.test.ts mocks the same module with vi.fn() stubs (so getKeyValue returns undefined, loadPersistentVariables is a no-op at import and the new restore branch in createDataLakeVariable sees an empty object) and asserts the classifier over five cases. Nothing under scripts/, .github/ or src/electron/ is touched, and package.json is unchanged.

Entry points.

Function	Reached from	Frequency
getPersistentValues (data-lake.ts)	savePersistentValues, createDataLakeVariable, deleteDataLakeVariable	one-shot at bootstrap, then per user action
savePersistentValues (data-lake.ts)	createDataLakeVariable, updateDataLakeVariableInfo, setDataLakeVariableData	as above (and per joystick frame if a persistValue variable is mapped — see section 5)
createDataLakeVariable (data-lake.ts:75)	src/main.ts → setupPredefinedLakeAndActionResources; ~30 registration sites; three user paths (dialog, InputElementConfig, MiniWidgetInstantiator re-registration)	one-shot at bootstrap, then per user action
deleteDataLakeVariable (data-lake.ts:140)	ToolsDataLakeView.deleteVariable, InputElementConfig	per user action
setupMavlinkCameraResources (predefined-resources.ts:43)	src/main.ts	one-shot
isUserDefinedDataLakeVariable (utils-data-lake.ts, new)	ToolsDataLakeView row v-ifs (:117, :125), getVariableSource (:321-331), deleteVariable (:420), editVariable (:390); DataLakeVariableDialog.valueOnlyEditMode	per render of the Data Lake table
isUserEditableVariable (ToolsDataLakeView.vue, new)	same row v-if and editVariable	per render of the Data Lake table
valueOnlyEditMode, isValid, saveVariable, the modelValue watch (DataLakeVariableDialog.vue)	dialog open / Save click	per user action
saveOrUpdateParameter (InputElementConfig.vue)	custom-widget input config	per user action

No changed function is unreachable, and none sits on onIncomingMessage, addToDataLake or notifyListeners.

Invariants.

1. persistValue implies persistent. Enforced in the dialog (:81 disables the checkbox, :194-201 clears it) and assumed by loadPersistentVariables. The two speed variables deliberately break it, and the PR compensates inside createDataLakeVariable by restoring from the values key at registration time. Holds.
2. cockpit-persistent-data-lake-values holds a value for every persistValue variable. Maintained by merging, with deleteDataLakeVariable as the only pruner. updateDataLakeVariableInfo can still leave an orphan when a user turns persistValue off; inert, since loadPersistentVariables filters on persistValue. Recorded in section 2 as prose, not as a finding.
3. user-defined ⇔ userDefined === true, or persistent === true for records written before the stamp. Producers: DataLakeVariableDialog.saveVariable and InputElementConfig.saveOrUpdateParameter, both stamping; MiniWidgetInstantiator re-registers from stored widget options, which carry the stamp when written by this version and persistent: true when written before it. No internal registration can reach either branch, checked across all createDataLakeVariable call sites. Holds — this is what closed 1.6.
4. allowUserToChangeValue means "the user may set this variable". It gates both the joystick mapping pickers (ConfigurationJoystickView.vue:848, :858, via allAvailableAxes) and, now, the Data Lake pencil. Every holder of the flag therefore gains a pencil, which includes the six joystick axis inputs (predefined-resources.ts:159-162) → 6.2.

2. Persistence & User Data — inventory, no finding

Two persisted keys are touched, both through settingsManager (src/libs/settings-management.ts), which keeps a local copy and syncs the key to the connected vehicle's BlueOS storage — so anything wrong here reaches every topside computer that talks to that vehicle.

Key	Backend	What happened
cockpit-persistent-data-lake-values	settings-management.ts (local + vehicle-synced)	Added entries, and its write strategy changed. camera-zoom-speed and camera-focus-speed become persistValue variables (predefined-resources.ts:45, :49), so their values now live in this key. It is also read during variable creation rather than only at module load, and savePersistentValues merges into the stored object instead of rebuilding it.
cockpit-persistent-data-lake-variables	settings-management.ts (local + vehicle-synced)	Stored shape gains one optional field. Records written from now on carry userDefined: true (types/data-lake.ts), because savePersistentVariables stores whole variable objects.

Judgement: both keys are cockpit-prefixed and already existed, and a camera zoom speed is a vehicle preference rather than a machine-specific value (no device path, no window geometry), so vehicle-syncing is the right backend for it. No migration is written and none is owed, which is the non-destructive route: the new field is optional, an older Cockpit reading a newer record ignores it, and a newer Cockpit reading an older record falls back to persistent === true — the fallback is exactly correct for that data, because a record is in this key only if savePersistentVariables filtered it in on persistent (data-lake.ts:47). Two consequences of the values key's new write strategy are recorded here rather than raised as findings, because neither is reachable as a defect: pruning now happens only in deleteDataLakeVariable, so an entry whose variable had persistValue turned off is left behind (inert); and getPersistentValues() returns a live reference into the settings manager's cachedSettings (settings-management.ts:328-338, and getKeyValue returns the stored value by reference), which both new writers mutate in place before calling the debounced setKeyValue. That is safe against the current setKeyValue, which re-wraps the value with a fresh epoch and writes unconditionally with no equality guard (:230-269) — but it is a coupling to another module's internals that nothing documents.

6. UI / UX — 1 finding (carried from round 4, disputed)

6.2 minor — The new pencil appears on the six joystick axis input variables, where the typed value is overwritten by the next joystick frame. (Raised in round 4, reprinted so this comment stands alone. Disputed by the author; see the status block above.) The gate is now isUserDefinedVariable(item.id) || isUserEditableVariable(item.id) (ToolsDataLakeView.vue:117), and isUserEditableVariable is allowUserToChangeValue === true. Besides the camera variables, that flag is carried by every variable created in setupJoystickAxesResources (predefined-resources.ts:159-162): inputs/mavlink/axis-x, -y, -z, -r, -s, -t. Those are live readings — src/libs/joystick/protocols/data-lake.ts:84-88 writes the scaled axis value on every controller update for each mapped axis, and the default ROV profile maps four of them (src/assets/joystick-profiles.ts:51-54). So with a joystick connected, a user who opens the pencil on "Axis X", types a value and saves sees it replaced before the dialog finishes closing, with no feedback explaining why.

The author's counter-argument, checked and correct on its facts, is that the class is wider than the six generated ids: data-lake.ts:63-72 writes a mapped button's target on every frame as well, so camera-zoom-increase behaves identically once a user maps a button to it. He also confirms the finding's own warning about the alternative fix — clearing allowUserToChangeValue in setupJoystickAxesResources would drop all six axes from the axis mapping picker (allAvailableAxes → ConfigurationJoystickView.vue:857), which is finding 1.2 again. His conclusion is that the only gate matching the real class is "is this variable currently the target of a joystick mapping", read live from controllerStore.protocolMapping, and that this belongs in its own PR.

That is a scope decision for a maintainer rather than something the code settles, which is why this is carried as disputed rather than closed. Either outcome is defensible: accept it and the symptom ships with the PR that introduces the pencil, reject it and the mapping-aware gate lands separately. What the argument does not do is remove the symptom from this diff — before this PR the axis rows had no pencil at all.

Sections with nothing to report (9)

1. Correctness & Implementation Bugs — ✅ (both round-4 findings fixed at their root and re-verified end to end; walked the headline path once more with the fix in place — a user edits Camera Zoom Speed through the value-only dialog, setDataLakeVariableData sees persistValue and merges the value into storage, and on the next boot createDataLakeVariable restores it before the default 3 can win, which loadPersistentVariables alone could never do because the variable is not in the info registry at module load; no openSnackbar-plus-console pairing added, optional chaining used at every new lookup, no Electron-only API introduced, no widget default-options merge touched)

3. AGENTS.md Adherence — ✅ (no dependency added, package.json untouched; both new exports carry typed, non-empty JSDoc; the editUserDefinedVariable → editVariable rename remains justified by the widened gate; the new userDefined field is not groundwork — it is written at two sites and read at one in this same PR)

4. Security — ✅ (no new dependency, no network call, no eval/v-html, no encoded blob, no env or secret use, no regex added; no workflow, build script, Dockerfile or Electron main-process file touched; both added tests mock a local module and reach nothing outside the process)

5. Performance — ✅ (every changed function traced to a bootstrap or user-action entry point, none to onIncomingMessage, addToDataLake or notifyListeners; getLocalSettings serves getPersistentValues from cachedSettings, so the merge is an in-memory read rather than a localStorage parse; the classifier is an O(1) registry lookup per table row; the one frequency worth naming is setDataLakeVariableData → savePersistentValues, which is O(all data-lake variables) and now also fires for the two speed variables — but only if a user maps a speed to a joystick axis, which is not a default mapping, and that path predates this PR for user-created persistValue variables; no listener, timer or watcher added, so nothing needs teardown)

7. Code Quality & Style — ✅ (complexity-report.json is present for this head: 132 functions measured across the 9 changed files, triggeredCount 0 and not truncated, so the report attributes no complexity or nesting trigger to anything the diff writes; both reworded comments in data-lake.ts sit on lines the diff changes, so the comment-immutability rule is not engaged; no any, no scoped CSS, no re-implementation of an existing helper; the added arrow functions carry explicit return types where they are not bare expressions, and the test files need no JSDoc under jsdoc/require-jsdoc's ArrowFunctionExpression: false)

8. Commit Hygiene — ✅ (five commits read from pr.json, still one logical step each — restore, persist the speeds, show the button, value-only dialog, stamp provenance — none reverting another, none bundling unrelated work, no wip/fixup! noise, subjects fix:-prefixed in line with the repository's history, and the Fix #2774/Fix #2775 references kept in the PR body rather than in any commit message; commit 5's subject was reworded to match what it now does, and the fact that commit 4 also carries a fresh committedDate is consistent with the one-line 1.7 fix being amended into the commit that introduced the value-only mode, which is where it belongs)

9. Tests — ✅ (no existing test touched or weakened; the PR now adds two files — src/tests/libs/actions/data-lake.test.ts reproduces the round-3 persistence symptom and the delete path, and the new src/tests/libs/utils-data-lake.test.ts covers the classifier over all five interesting cases, including the stamped-but-not-persistent one that is finding 1.6; both sit under src/tests/libs/ alongside the repository's existing suites. Worth knowing rather than fixing here: CI runs yarn lint, yarn typecheck and yarn build only and never yarn test:ci, so neither file runs on any push — that is the repository's gap, not this PR's)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README change is owed; the new exported helper's JSDoc is complete, correctly typed, and its summary now states what the flag actually means, which was the wording problem behind 1.6)

11. Nitpicks / Optional — ✅ (the added template text matches its surroundings: "Edit Variable Value" keeps the Title Case of the Edit Variable/New Variable siblings it sits beside, and the read-only name line reuses the existing flex flex-col gap-4 spacing rather than inventing a margin)

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

[github-actions]

🙋 Decision needed — 6.2

Value-only pencil offered on joystick axis inputs whose value is overwritten every frame

The author's argument: The author agrees the symptom is real but argues the class is wider than the six joystick axis inputs, since a variable a joystick button is mapped to is also overwritten every frame, so excluding those six ids would fix one instance and leave the identical sibling case broken; the only gate matching the real class would read the live joystick mappings from the controller store in the Data Lake view, which he would rather do in a separate PR.

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]

Review follow-up — round 6

Done

• src/libs/joystick/protocols/data-lake.ts, src/views/ToolsDataLakeView.vue (6.2 — value-only pencil offered on variables a joystick overwrites every frame): the ballot came back -1, so the gate lands here rather than in its own PR, and it is the wide version — not an exclusion list of the six generated axis ids. New joystickDrivenDataLakeVariableIds(mapping) reads the live mapping and returns the ids of every data-lake variable an axis or a button of either modifier layout is currently mapped to; canEditVariable in the Data Lake view drops the pencil for a user-editable variable that appears in it. Squashed into the commit that added the pencil (db72a72), whose body now says why the exclusion exists — the symptom never existed before that commit.
• src/tests/libs/joystick/protocols/data-lake.test.ts (6.2): covers an axis-mapped id, a regular-layout button, a shift-layout button, and the two non-data-lake protocols. Dropping the button half of the helper fails it with expected [ 'inputs/mavlink/axis-x' ] to include 'camera-zoom-increase', which is the sibling case the round-5 argument was about.

Done differently

• src/views/ToolsDataLakeView.vue (6.2): the gate gets applied to the value-only path only. A user-defined variable keeps its pencil even while a joystick drives it, because that button opens the full editor, where name, type and persistence are the point and are not overwritten by anything. Taking it away there would cost a user the only way to rename or retype a variable they created, to protect a value field they may not have come for. editVariable's own guard moved to the same helper, so the funnel and the v-if cannot disagree.

Notes

• The button appears and disappears as mappings change, which is the behaviour the round-5 argument flagged as the cost of this gate. It is memoised in a computed over controllerStore.protocolMapping, so a table of N rows walks the mapping once per change rather than once per row.

[rafaellehmkuhl]

/review

[github-actions]

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

0 open · 13 closed (1 of them this round).

The PR makes two internal camera settings (zoom speed and focus speed) remember their value between restarts, and lets the Data Lake page edit the value of variables Cockpit itself created but marked as user-changeable, through a stripped-down dialog that changes the value only. It also stops guessing which variables the user created from the persistence checkboxes and records it explicitly instead. This round adds the last missing piece: a variable a joystick control is currently mapped to no longer offers the edit button, because the controller rewrites it many times a second and a typed value would not survive.

What still needs attention

Nothing is open. Every finding raised across the six rounds is closed.

Since round 5 — 1 closed (addressed), 1 dispute settled by vote, 0 new findings, comparing 5b83ffb → 6f08119

Range. 5b83ffb5f590e68d85f941b784e2d8a879d10297 → 6f0811968f07caf5261304a3b6f37bb947c2c35c.

The incremental diff is usable, but inflated by another rebase. PREV_SHA is not among the five commits in pr.json, and the last three commits share one committedDate, so the branch was rewritten again. incremental.diff lists eight files, but only three carry new work: src/libs/joystick/protocols/data-lake.ts, the new src/tests/libs/joystick/protocols/data-lake.test.ts, and src/views/ToolsDataLakeView.vue. I compared the other five hunk by hunk against what round 5 reviewed and they are identical in content (only the context sizes of the hunk headers differ), so they are base-drift from the rebase rather than edits. Every status judgement below was made against pr.diff (base…head) and the base checkout, and the whole review was re-run over pr.diff, not over the increment.

Decisions applied first. decisions.json carries one gated entry: the argument on 6.2 was refused by @rafaellehmkuhl (-1, no +1) on its decision comment, #2776 (comment). That settles the argument, not the finding, so the author_argument is dropped and 6.2 was carried back as a plain open finding and judged against the code like any other — where it turned out to be fixed, below. No other vote is pending, tied or ungated.

Status changes this round

• 6.2 — ✅ Addressed, and wider than the finding asked for. The finding asked that the pencil the PR adds not be offered on values a connected joystick overwrites many times a second. What landed: joystickDrivenDataLakeVariableIds (src/libs/joystick/protocols/data-lake.ts:53-58) walks the live mapping — axesCorrespondencies plus buttonsCorrespondencies for both modifier layouts — and returns the ids mapped to a JoystickProtocol.DataLakeVariable action; canEditVariable (ToolsDataLakeView.vue:447-450) drops the button for a user-editable variable whose id is in that list. I checked each half against the writer it is meant to cover: the axis half matches data-lake.ts:81-88 (only mapped axes are written, which is why the two axes the default ROV profile leaves unmapped keep their pencil and correctly so — nothing else in src/ writes inputs/mavlink/axis-*), and the button half matches :59-72, which is the sibling case the round-5 argument was about and which an exclusion list of the six generated axis ids would have missed. The funnel agrees with the template: editVariable (:394) now guards on the same canEditVariable, so clicking through cannot reach a state the v-if forbids. A user-defined variable keeps its pencil even while a joystick drives it — that path opens the full editor, where name, type and persistence are the point, and it had that pencil before this PR, so nothing here regresses it.
• New this round: none. The full re-read of pr.diff surfaced nothing that was not already recorded.

Resolutions. resolutions.json is [] — no /resolve has been issued on this PR, so nothing was closed by command and there is no unknown id to report back.

Discussion since round 5. Two comments, both from @rafaellehmkuhl: the round-6 follow-up and the bare /review that triggered this run. Its factual claims were checked against the code rather than taken. Verified: the helper reads the live mapping and covers axes and both button layouts; the gate applies to the value-only path only, since canEditVariable returns early for a user-defined variable; editVariable's own guard now calls the same helper; the work was squashed into db72a72, whose body states why the exclusion exists; and the new test does pin the button half — dropping it would leave camera-zoom-increase and camera-zoom-decrease out of the returned ids and fail the assertion. One claim needs a small correction: the mapping is indeed walked once per change rather than once per row (joystickDrivenVariableIds is a computed, and it reads the nested action of each correspondency, so a deep mapping edit does invalidate it), but each row still runs an Array.includes over the resulting ids. That is a handful of entries against a paginated table, so it is not worth changing.

Injection check. Nothing in the PR body, the diff, the commit messages, the comments or the decision entry contained text addressed to the reviewer. The follow-up comment is written as HTML markup; it is summarised above rather than quoted verbatim.

Change map — what was established before judging

Claims. The PR body is only Fix #2774 / Fix #2775; the issues are not reachable from here, so the claims are read off the commit messages in pr.json, plus the ones the round-6 follow-up adds.

Claim	Verdict
"Internal variables with persistValue were always initialized from their default, overwriting values saved in localStorage."	Verified — base data-lake.ts:80 assigns initialValue unconditionally, and loadPersistentVariables (:34-42) restores values only for variables already in the info registry at module load, which a persistValue-without-persistent variable never is.
"The speed settings are the two camera variables a user tunes … the increase/decrease variables stay as they were."	Verified — predefined-resources.ts:44-53: commonVariableConfig is unchanged and persistValue: true arrives only through the new speedVariableConfig.
"The Data Lake table hid the edit button for internal variables even when they were marked as user-editable."	Verified — base ToolsDataLakeView.vue:117 gated the pencil on isUserDefinedVariable alone.
(commit 3, new body) "Variables a joystick control is currently mapped to stay without the button, since the controller update rewrites them on every frame."	Verified — ToolsDataLakeView.vue:447-450 against joystick/protocols/data-lake.ts:59-72 (buttons) and :81-88 (axes).
"Editing internal variables should only change the value, not metadata."	Verified — the value-only branch of saveVariable calls setDataLakeVariableData and never updateDataLakeVariableInfo.
"The page inferred ownership from the persistence flags … variables created from the page or from a custom widget input now say so, and the flag is read only as a fallback."	Verified — base :437 was persistent != null, which matched the persistent: false telemetry variables; both user-creating sites now stamp userDefined: true, and the fallback is unreachable for internal registrations (all createDataLakeVariable call sites enumerated in round 5).
(follow-up) "The gate is the wide version, not an exclusion list of the six generated axis ids."	Verified — the helper filters on the action's protocol, so it covers any data-lake variable an axis or a button of either layout is mapped to.
(follow-up) "It is memoised in a computed, so a table of N rows walks the mapping once per change rather than once per row."	Verified with a correction — the walk is memoised, but each row still scans the resulting id array. Immaterial at this size.

Failure site.

• Persistence symptom: createDataLakeVariable (data-lake.ts:75-96) and savePersistentValues (:53-65) — both in the diff.
• Editability symptom: isUserDefinedVariable (ToolsDataLakeView.vue:436) and the pencil's v-if (:117) — both in the diff.
• Provenance: a single chokepoint, isUserDefinedDataLakeVariable (utils-data-lake.ts), with two producers that stamp and one legacy fallback.
• Joystick-overwrite symptom (6.2): the writers are joystick/protocols/data-lake.ts:59-72 and :81-88, which the PR leaves alone; the gate is placed at the single consumer that offers the edit, which is the correct end of it — the writers exist to be fast, and there is nothing wrong with them.
• Three added files, all read in full. src/tests/libs/actions/data-lake.test.ts and src/tests/libs/utils-data-lake.test.ts are unchanged since round 5. The new src/tests/libs/joystick/protocols/data-lake.test.ts mocks settings-management and @/stores/controller with vi.fn() stubs and asserts the helper over an axis, a regular-layout button, a shift-layout button and two non-data-lake protocols; importing the module under test is side-effect-safe because setupPostPiniaConnection only pushes onto an array (post-pinia-connections.ts:7-9) and never calls the mocked store. Nothing under scripts/, .github/ or src/electron/ is touched, and package.json is unchanged.

Entry points.

Function	Reached from	Frequency
getPersistentValues (data-lake.ts)	savePersistentValues, createDataLakeVariable, deleteDataLakeVariable	one-shot at bootstrap, then per user action
savePersistentValues (data-lake.ts)	createDataLakeVariable, updateDataLakeVariableInfo, setDataLakeVariableData	as above (and per joystick frame if a persistValue variable is mapped — see section 5)
createDataLakeVariable (data-lake.ts:75)	src/main.ts → setupPredefinedLakeAndActionResources; ~30 registration sites; three user paths (dialog, InputElementConfig, MiniWidgetInstantiator re-registration)	one-shot at bootstrap, then per user action
deleteDataLakeVariable (data-lake.ts:140)	ToolsDataLakeView.deleteVariable, InputElementConfig	per user action
setupMavlinkCameraResources (predefined-resources.ts:43)	src/main.ts	one-shot
isUserDefinedDataLakeVariable (utils-data-lake.ts)	ToolsDataLakeView row v-ifs, getVariableSource, deleteVariable, canEditVariable; DataLakeVariableDialog.valueOnlyEditMode	per render of the Data Lake table
joystickDrivenDataLakeVariableIds (joystick/protocols/data-lake.ts:53, new)	the joystickDrivenVariableIds computed in ToolsDataLakeView	per joystick-mapping change, while the Data Lake page is mounted — the store's 500 ms modifier-key sweep (controller.ts:358-387) only assigns when a mapping is actually invalid, so it does not invalidate the computed on a timer
canEditVariable / isUserEditableVariable (ToolsDataLakeView.vue, new)	the pencil's v-if and editVariable	per render of the Data Lake table
valueOnlyEditMode, isValid, saveVariable, the modelValue watch (DataLakeVariableDialog.vue)	dialog open / Save click	per user action
saveOrUpdateParameter (InputElementConfig.vue)	custom-widget input config	per user action

No changed function is unreachable, and none sits on onIncomingMessage, addToDataLake or notifyListeners.

Invariants.

1. persistValue implies persistent. Enforced in the dialog (:81 disables the checkbox, :194-201 clears it) and assumed by loadPersistentVariables. The two speed variables deliberately break it, and the PR compensates inside createDataLakeVariable by restoring from the values key at registration time. Holds.
2. cockpit-persistent-data-lake-values holds a value for every persistValue variable. Maintained by merging, with deleteDataLakeVariable as the only pruner. updateDataLakeVariableInfo can still leave an orphan when a user turns persistValue off; inert, since loadPersistentVariables filters on persistValue. Recorded in section 2 as prose, not as a finding.
3. user-defined ⇔ userDefined === true, or persistent === true for records written before the stamp. Producers: DataLakeVariableDialog.saveVariable and InputElementConfig.saveOrUpdateParameter, both stamping; MiniWidgetInstantiator re-registers from stored widget options, which carry the stamp when written by this version and persistent: true when written before it. No internal registration can reach either branch. Holds.
4. A variable offered a value-only edit is not being written by something else. The only other writer of these ids is the joystick protocol handler, and the new gate enumerates its two write paths from the same mapping object the handler reads (controllerStore.protocolMapping), so producer and gate cannot drift apart by construction. Both write paths are covered; the gate is keyed on the mapping alone and not on whether a joystick is connected or forwarding is enabled, which errs on the side of hiding the button — see section 6's clause.

2. Persistence & User Data — inventory, no finding

Two persisted keys are touched, both through settingsManager (src/libs/settings-management.ts), which keeps a local copy and syncs the key to the connected vehicle's BlueOS storage — so anything wrong here reaches every topside computer that talks to that vehicle. Neither changed this round.

Key	Backend	What happened
cockpit-persistent-data-lake-values	settings-management.ts (local + vehicle-synced)	Added entries, and its write strategy changed. camera-zoom-speed and camera-focus-speed become persistValue variables (predefined-resources.ts:45, :49), so their values now live in this key. It is also read during variable creation rather than only at module load, and savePersistentValues merges into the stored object instead of rebuilding it.
cockpit-persistent-data-lake-variables	settings-management.ts (local + vehicle-synced)	Stored shape gains one optional field. Records written from now on carry userDefined: true (types/data-lake.ts), because savePersistentVariables stores whole variable objects.

Judgement: both keys are cockpit-prefixed and already existed, and a camera zoom speed is a vehicle preference rather than a machine-specific value (no device path, no window geometry), so vehicle-syncing is the right backend for it. No migration is written and none is owed, which is the non-destructive route: the new field is optional, an older Cockpit reading a newer record ignores it, and a newer Cockpit reading an older record falls back to persistent === true — the fallback is exactly correct for that data, because a record is in this key only if savePersistentVariables filtered it in on persistent (data-lake.ts:47). Two consequences of the values key's new write strategy are recorded here rather than raised as findings, because neither is reachable as a defect: pruning now happens only in deleteDataLakeVariable, so an entry whose variable had persistValue turned off is left behind (inert); and getPersistentValues() returns a live reference into the settings manager's cachedSettings, which both new writers mutate in place before calling the debounced setKeyValue. That is safe against the current setKeyValue, which re-wraps the value with a fresh epoch and writes unconditionally with no equality guard — but it is a coupling to another module's internals that nothing documents.

One key this round's change reads without writing: cockpit-joystick-functions-mapping, via controllerStore.protocolMapping (stores/controller.ts:57). It is vehicle-synced, so the Data Lake page's new gate is driven by a mapping that may have been configured on another topside computer — read-only, so nothing is at risk in this key.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (the new helper typechecks under the strict config that failed round-2 finding 1.4: action is non-optional on both JoystickAxisActionCorrespondency and JoystickButtonActionCorrespondency (types/joystick.ts:109, :132), so the union array's .map((c) => c.action.id) needs no narrowing, and the ?. on the preceding filter matches how the handler in the same file already reads those entries; the computed tracks the mapping deeply because it reads each correspondency's action, so a mapping edit does re-hide or re-show the button; no openSnackbar-plus-console pairing added, no Electron-only API introduced, no widget default-options merge touched)

3. AGENTS.md Adherence — ✅ (no dependency added, package.json untouched; the new export carries typed, non-empty JSDoc; the one added comment explains why the gate exists rather than what the line does; the helper is not groundwork — it is written and called in this same PR; it went into the joystick protocol module that already owns reading a mapping, not into the view, which is where the write paths it mirrors live)

4. Security — ✅ (no new dependency, no network call, no eval/v-html, no encoded blob, no env or secret use, no regex added; no workflow, build script, Dockerfile or Electron main-process file touched; the new test mocks two local modules and reaches nothing outside the process)

5. Performance — ✅ (the added path is a computed over the joystick mapping, invalidated per mapping edit and not by the store's 500 ms sweep, which only assigns on an invalid modifier mapping; each row then does an Array.includes over a handful of ids in a paginated table; the view's new useControllerStore() creates nothing new, since the store is already instantiated at bootstrap through setupPostPiniaConnections() in main.ts:93; importing joystick/protocols/data-lake into the view adds no bootstrap ordering risk either, because libs/joystick/protocols.ts:4 already imports it statically; no listener, timer or watcher added, so nothing needs teardown)

6. UI / UX — ✅ (the gate removes an affordance rather than adding a surface, so no dialog anatomy, theme="dark", token, padding, glass or stacking clause is engaged; the pencil is still an icon button in the existing action row; editVariable's logUserAction call is unchanged and the gate needs none of its own; the one behaviour worth knowing rather than fixing is that the gate keys on the mapping alone, so a mapping synced from another operator's machine hides the pencil here even with no joystick connected and forwarding off (controller.ts:67, :279) — checking enableForwarding too would make the button flicker on every connect and disconnect, which is the worse trade)

7. Code Quality & Style — ✅ (complexity-report.json is present for this head and reports 153 functions measured across the 11 changed files, triggeredCount 0 and not truncated, so it attributes no complexity or nesting trigger to anything the diff writes; the new helper is one expression with an explicit return type, no any, no scoped CSS, no re-implementation of an existing helper; vue/no-unused-properties is not engaged by the script-only joystickDrivenVariableIds, matching the existing script-only getVariableSource in the same file; imports stay in simple-import-sort order in both changed files)

8. Commit Hygiene — ✅ (five commits read from pr.json, still one logical step each — restore, persist the speeds, show the button, value-only dialog, stamp provenance — none reverting another; the 6.2 gate was squashed into db72a72, the commit that introduced the pencil, which is exactly where it belongs since the symptom did not exist before it, and that commit's body was extended to say why; no wip/fixup! noise, subjects fix:-prefixed in line with the repository's history, and the Fix #2774/Fix #2775 references kept in the PR body rather than in any commit message)

9. Tests — ✅ (no existing test touched or weakened; a third file, src/tests/libs/joystick/protocols/data-lake.test.ts, pins both halves of the new helper including the shift layout, and sits alongside the repository's existing suites under src/tests/libs/. Worth knowing rather than fixing here: CI runs yarn lint, yarn typecheck and yarn build only and never yarn test:ci, so none of the three files runs on any push — that is the repository's gap, not this PR's)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README change is owed; both new exported helpers carry complete, correctly typed JSDoc)

11. Nitpicks / Optional — ✅ (nothing left over: the added template text still matches its surroundings, and the new code introduces no user-facing string at all)

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

[rafaellehmkuhl]

Review follow-up — round 7

Round 6 closed every finding (0 open, verdict ready to merge), so nothing was implemented this round and no /review was triggered — the code is unchanged from 6f08119.

Done

• ballots: read the reactions directly rather than off the review text. 1.1 and 6.1 carry a +1, so both arguments stand and stay obsolete. 6.2 carries a -1, and that finding was already implemented in round 6 as joystickDrivenDataLakeVariableIds (src/libs/joystick/protocols/data-lake.ts:57-62) gating canEditVariable (src/views/ToolsDataLakeView.vue:450-453) — verified present at HEAD, nothing owed.
• branch: no conflicts against master, and every check that has finished is green (the electron/flatpak matrices are still running).

Done differently

• src/views/ToolsDataLakeView.vue (round-6 correction): the correction is accepted — the mapping walk is memoised in the computed, but each table row still runs an Array.includes over the resulting ids. Left as is, since it is a handful of entries against a paginated table.

[rafaellehmkuhl]

Done — the MANUAL_CONTROL axes (inputs/mavlink/axis-*) and the zoom/focus increase/decrease variables no longer get an edit button, under any joystick profile. The only internal variables left editable are camera-zoom-speed and camera-focus-speed, which is what this PR set out to fix.

The gate no longer derives editability from allowUserToChangeValue: that flag means "a joystick control or a widget element may write to this", which is true for every live input. A new optional userTunable flag (src/types/data-lake.ts) marks the ones holding a setting the user is meant to type in, and it is stamped only on the two speed variables (predefined-resources.ts:45). The table's edit button (ToolsDataLakeView.vue:441) and the dialog's value-only mode (DataLakeVariableDialog.vue:138) both read it through isUserTunableDataLakeVariable() (src/libs/utils-data-lake.ts).

That also removed joystickDrivenDataLakeVariableIds() and its test file. It excluded only the variables the current mapping wrote to, which is exactly why an unmapped axis still showed the button — the new gate is unconditional, so it covers what finding 6.2 widened to buttons and more.

Rebased into the commits it belongs to, so no new commits on the branch. fix: show edit action for allowUserToChangeValue data lake variables was reworded to fix: show edit action for user-tunable data lake variables, since its subject no longer described what it does. The PR body was updated too.

[rafaellehmkuhl]

/review

[github-actions]

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

0 open · 13 closed (none of them this round).

The PR makes two internal camera settings — zoom speed and focus speed — remember their value between restarts, and gives the Data Lake page a way to change them: a stripped-down dialog that edits the value alone and cannot rewrite Cockpit's own metadata. It also stops guessing which variables the user created from the persistence checkboxes and records that explicitly instead. This round rebuilt the last piece: instead of hiding the edit button for whatever a joystick is currently mapped to, the two speed settings are now marked as the only internal variables meant to be typed in, and everything else internal is uneditable regardless of mapping.

What still needs attention

Nothing is open. Every finding raised across the seven rounds is closed.

Since round 6 — the edit gate was rebuilt on a different mechanism, 0 findings reopened, 0 new, comparing 6f08119 → 2dd00a3

Range. 6f0811968f07caf5261304a3b6f37bb947c2c35c → 2dd00a3b296cdc73f80823400d14e0943ebfeae2.

The incremental diff is not usable this round. incremental.diff lists seven files and reproduces the PR's own base-to-head hunks for most of them, including changes that landed and were closed in earlier rounds — the initialValue.value = currentValue !== undefined ? … line closed as 1.7 in round 5, and the editUserDefinedVariable → editVariable rename from round 1. It also omits two files the PR does change, src/libs/actions/data-lake.ts and src/tests/libs/actions/data-lake.test.ts. That is a rewritten branch rather than a delta: PREV_SHA is not among the five commits in pr.json, and the last three commits carry new shas and timestamps, which matches the rebase the author reports. Exactly one hunk in it is a genuine delta (predefined-resources.ts, persistValue: true gaining userTunable: true). Every judgement below was made against pr.diff (base…head) and the base checkout, and the whole review was re-run over pr.diff.

Resolutions and decisions. resolutions.json is [] — no /resolve has ever been issued on this PR, so nothing was closed by command and there is no unknown id to report back. decisions.json is [] as well: the vote refusing the argument on 6.2 was applied in round 6 and does not come back, and no new dispute has been raised. Nothing was settled by a human this round, so nothing needed applying before judging.

What actually changed, and what it does to the closed findings. The mechanism that closed 6.2 in round 6 was removed and replaced. Head deletes joystickDrivenDataLakeVariableIds and its test file, and gates the pencil on a new optional userTunable flag (src/types/data-lake.ts:38-42) read through isUserTunableDataLakeVariable (src/libs/utils-data-lake.ts:21) by canEditVariable (src/views/ToolsDataLakeView.vue:441) and by the dialog's valueOnlyEditMode (src/components/DataLakeVariableDialog.vue:138). No finding reopened:

• 6.2 (closed, addressed) — re-checked against the replacement rather than assumed. The flag is stamped once, on speedVariableConfig (predefined-resources.ts:45), and reaches only camera-zoom-speed (:49) and camera-focus-speed (:54); userTunable appears nowhere else in src/. The six MANUAL_CONTROL axis inputs are registered at predefined-resources.ts:163 with commonVariableConfig and the four zoom/focus increase/decrease variables at :47-48, :52-53 — no userTunable, no persistent, so canEditVariable is false for all ten and the pencil is gone whether or not a joystick is mapped to them. That is wider than the mapping walk it replaces, which only excluded what the current profile happened to write to. One case moves the other way: the joystick axis picker still offers the two speed variables (ConfigurationJoystickView.vue:858 filters on allowUserToChangeValue, which finding 1.2 required them to keep), so a user who maps a control to Camera Zoom Speed by hand keeps a pencil whose typed value the controller overwrites. Recorded in section 6's clause rather than raised, because that mapping is the user's own configuration and the variable genuinely is a setting.
• 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 3.1, r1-1.1, r1-1.2, r1-6.1 (closed) — all re-verified against the current pr.diff, since the rebase could have dropped any of them. commonVariableConfig still carries allowUserToChangeValue (1.2); savePersistentValues still merges (1.3); getKeyValue is still called with an explicit Record<string, string | number | boolean> type argument, which satisfies its T extends SettingValue constraint (1.4); both user-creating sites still stamp userDefined: true (1.5, InputElementConfig.vue:523; 1.6, DataLakeVariableDialog.vue:301); the blank-field fix is still at DataLakeVariableDialog.vue:232 (1.7); the helper still takes an id (3.1).
• 1.1, 6.1 (closed, obsolete) — unaffected; the id normalisation both concerned is still absent from the diff.
• New this round: none. The full re-read of pr.diff surfaced nothing that was not already recorded.

Discussion since round 6. Three comments, all from @rafaellehmkuhl: the round-7 follow-up, the description of the rework, and the bare /review that triggered this run (ignored as a command). Their factual claims were checked against the code rather than taken. Verified: the axes and the increase/decrease variables no longer get an edit button under any profile (the ten registrations above); userTunable is stamped only on the two speed variables (one producer, predefined-resources.ts:45); the joystick helper and its test are gone from the diff, so that file matches base again; the commit count is still five and c7c47f45 carries the reworded subject. One claim is stale rather than wrong: the earlier follow-up says the code is unchanged from 6f08119, which held when it was written and was superseded by the rework a day later. The same comment reports reading the round-6 ballots itself; nothing was acted on from it — decisions.json decides votes here, and the ledger already carried 6.2 as closed.

Injection check. Nothing in the PR body, the diff, the commit messages or the comments contained text addressed to the reviewer. The round-7 follow-up is written as HTML markup; it is summarised above rather than quoted verbatim.

Change map — what was established before judging

Claims. Read from the PR body (rewritten this round) and the five commit messages in pr.json.

Claim	Verdict
"The page inferred user ownership from persistent != null, which is true for any variable that sets the flag at all — including internal ones that set persistent: false."	Verified — base ToolsDataLakeView.vue:437 is exactly that test, and the telemetry/BlueOS registrations set persistent: false explicitly.
"Camera zoom/focus speeds were unreachable … the table only showed the edit button for variables it considered user-defined."	Verified — base :117 gated the pencil on isUserDefinedVariable alone, and neither speed variable satisfied it.
"Their values reset to 3 on every boot."	Verified — base data-lake.ts:80 assigns initialValue unconditionally, and loadPersistentVariables (:24-43) restores only variables already in the info registry at module load, which a persistValue-without-persistent variable never is.
"userDefined … stamped at the two places where the user actually creates a variable."	Verified — DataLakeVariableDialog.vue:301 and InputElementConfig.vue:523. Every createDataLakeVariable call site in src/ was enumerated; no internal one stamps it, and none sets persistent: true, so the legacy fallback cannot misfire on Cockpit's own variables.
"A second flag, userTunable, marks the internal variables that hold a setting the user is meant to type in, which today is only camera-zoom-speed and camera-focus-speed."	Verified — one producer (predefined-resources.ts:45), two consumers (ToolsDataLakeView.vue:441, DataLakeVariableDialog.vue:138), no other occurrence in src/.
"The live inputs … carry allowUserToChangeValue so the joystick pickers keep offering them, but no userTunable, so they stay uneditable."	Verified — predefined-resources.ts:163 (axes) and :47-48, :52-53 (increase/decrease) against the picker filters at ConfigurationJoystickView.vue:848, :858.
"The dialog opens showing only the value field … so editing one cannot rewrite Cockpit's own metadata."	Verified — the value-only branch of saveVariable (DataLakeVariableDialog.vue:296-300) calls setDataLakeVariableData and never updateDataLakeVariableInfo, and the userDefined: true stamp now sits inside the other branch (:301), so it cannot be written onto an internal variable.
"savePersistentValues also merges … and deleting a variable removes only its own key."	Verified — data-lake.ts:58 reads the stored object first; :167-169 deletes one key instead of rebuilding.

Failure site.

• Persistence symptom: createDataLakeVariable (data-lake.ts:80-109) and savePersistentValues (:58-70) — both in the diff.
• Editability symptom: isUserDefinedVariable (ToolsDataLakeView.vue:437) and the pencil's v-if (:117) — both in the diff.
• Provenance: one chokepoint, isUserDefinedDataLakeVariable (utils-data-lake.ts:10), with two stamping producers and one legacy fallback.
• Joystick-overwrite symptom (6.2): the writers are joystick/protocols/data-lake.ts:59-72 and :81-88; the PR leaves them alone and now excludes their targets by construction, since none of the ids they can be pointed at carries userTunable unless a user maps one there by hand.
• Two added files, both read in full: src/tests/libs/actions/data-lake.test.ts and src/tests/libs/utils-data-lake.test.ts. Nothing under scripts/, .github/ or src/electron/ is touched, and package.json is unchanged.

Entry points.

Function	Reached from	Frequency
getPersistentValues (data-lake.ts:52, new)	savePersistentValues, createDataLakeVariable:88, deleteDataLakeVariable:167	one-shot at bootstrap, then per user action
savePersistentValues (data-lake.ts:58)	createDataLakeVariable, updateDataLakeVariableInfo, setDataLakeVariableData:146	per user action (and per controller update if a persistValue variable is mapped — see section 5)
createDataLakeVariable (data-lake.ts:80)	src/main.ts → setupPredefinedLakeAndActionResources; ~30 registration sites; three user paths (dialog, InputElementConfig, MiniWidgetInstantiator re-registration)	one-shot at bootstrap, then per user action
deleteDataLakeVariable (data-lake.ts:153)	ToolsDataLakeView.deleteVariable, InputElementConfig, the transforming-function rebuild (data-lake-transformations.ts:223-227)	per user action
setupMavlinkCameraResources (predefined-resources.ts:43)	src/main.ts	one-shot
isUserDefinedDataLakeVariable (utils-data-lake.ts:10, new)	ToolsDataLakeView row v-ifs, getVariableSource, deleteVariable, canEditVariable; DataLakeVariableDialog.valueOnlyEditMode	per render of the Data Lake table
isUserTunableDataLakeVariable (utils-data-lake.ts:21, new)	canEditVariable (ToolsDataLakeView.vue:441), valueOnlyEditMode (DataLakeVariableDialog.vue:138)	per render of the Data Lake table
canEditVariable / isUserDefinedVariable (ToolsDataLakeView.vue:437-443)	the pencil's v-if (:117) and editVariable's own guard	per render of the Data Lake table
editVariable (ToolsDataLakeView.vue:387, renamed)	the pencil's click handler	per user action
valueOnlyEditMode, isValid, saveVariable, the modelValue watch (DataLakeVariableDialog.vue)	dialog open / Save click	per user action
saveOrUpdateParameter (InputElementConfig.vue:523)	custom-widget input config	per user action

No changed function is unreachable, and none sits on onIncomingMessage, addToDataLake or notifyListeners.

Invariants.

1. persistValue implies persistent. Enforced in the dialog (the persistValue checkbox is disabled unless persistent is ticked, and the persistent watcher clears it) and assumed by loadPersistentVariables. The two speed variables deliberately break it, and the PR compensates inside createDataLakeVariable by restoring from the values key at registration time. Holds.
2. cockpit-persistent-data-lake-values holds a value for every persistValue variable. Maintained by merging, with deleteDataLakeVariable as the only pruner. updateDataLakeVariableInfo can still leave an orphan when a user turns persistValue off; inert, since loadPersistentVariables:38 filters on persistValue. Recorded in section 2 as prose, not as a finding.
3. user-defined ⇔ userDefined === true, or persistent === true for records written before the stamp. Producers: DataLakeVariableDialog.saveVariable and InputElementConfig.saveOrUpdateParameter, both stamping; MiniWidgetInstantiator re-registers from stored widget options, which carry the stamp when written by this version and persistent: true when written before it. A grep for persistent: true across src/ returns only those user paths, so no internal registration can reach either branch, and the transforming-function registrations (data-lake-transformations.ts:283) set neither flag. Holds.
4. A variable offered a value-only edit is not being written by something else. This round the invariant moved from being derived (walking the live joystick mapping) to being declared (userTunable stamped at the registration site). The declaration is exhaustive by inspection — one producer, two ids — and the writers that could contradict it write only what a user mapped (joystick/protocols/data-lake.ts:59-72, :81-88) or their own widget variable. It holds for every default configuration; the single way to break it is to map a joystick control to a speed variable by hand, which the picker still permits. Recorded in section 6's clause.

2. Persistence & User Data — inventory, no findings

Two persisted keys are touched, both through settingsManager (src/libs/settings-management.ts), which keeps a local copy and syncs the key to the connected vehicle's BlueOS storage — so anything wrong here reaches every topside computer that talks to that vehicle.

Key	Backend	What happened
cockpit-persistent-data-lake-values	settings-management.ts (local + vehicle-synced)	Added entries, and its write strategy changed. camera-zoom-speed and camera-focus-speed become persistValue variables (predefined-resources.ts:45, :49, :54), so their values now live in this key. It is also read during variable creation rather than only at module load, savePersistentValues merges into the stored object instead of rebuilding it, and deleteDataLakeVariable prunes a single key.
cockpit-persistent-data-lake-variables	settings-management.ts (local + vehicle-synced)	Stored shape gains two optional fields. Records written from now on carry userDefined: true (types/data-lake.ts:46), because savePersistentVariables stores whole variable objects. userTunable (:42) is part of the same shape but never reaches this key today, since it is stamped only on variables that are not persistent.

Judgement: both keys are cockpit-prefixed and already existed, and a camera zoom speed is a vehicle preference rather than a machine-specific value (no device path, no window geometry), so vehicle-syncing is the right backend. No migration is written and none is owed, which is the non-destructive route AGENTS.md asks for: the new fields are optional, an older Cockpit reading a newer record ignores them, and a newer Cockpit reading an older record falls back to persistent === true — exactly correct for that data, because a record is in this key only if savePersistentVariables filtered it in on persistent (data-lake.ts:47). Users already configured are carried over rather than stranded: their existing variables keep both actions through the fallback, and a saved speed that predates this PR is picked up by the new restore path instead of being overwritten with 3. Two consequences of the values key's write strategy are recorded here rather than raised, because neither is reachable as a defect: pruning happens only in deleteDataLakeVariable, so an entry whose variable had persistValue turned off is left behind (inert); and getPersistentValues() returns a live reference into the settings manager's cachedSettings (settings-management.ts:294), which both new writers mutate in place before calling the debounced setKeyValue. That is safe against the current setKeyValue, which re-wraps the value with a fresh epoch and writes unconditionally with no equality guard — but it is a coupling to another module's internals that nothing documents.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (traced the value-only path end to end: valueOnlyEditMode reads props.idVariableBeingEdited and returns false for a user-defined variable, so the full editor is unreachable for one and the metadata rewrite unreachable for the other; saveVariable's value-only branch uses variable.id, which the modelValue watch populates from the registry before the dialog paints; isValid short-circuits on the same computed so Save is not blocked by the hidden name field; the name watcher cannot regenerate an id in edit mode; both new helpers use optional chaining and compare with === true, so a missing info record and an explicit false both read as false; the table's row-level calls read the module registry while filteredVariables still tracks availableDataLakeVariables, so the info listener keeps the buttons in step; no openSnackbar-plus-console pairing, no Electron-only API, no widget default-options merge touched)

3. AGENTS.md Adherence — ✅ (no dependency added, package.json untouched; both new exported helpers carry typed, non-empty JSDoc with @param and @returns, and both new interface fields carry the doc comment jsdoc/require-jsdoc's TSPropertySignature context requires; the one added comment, on savePersistentValues, explains why the merge exists rather than what the line does, and no existing comment was reworded or deleted; nothing is groundwork — both flags and both helpers are read in this same PR; the classification helpers went into src/libs/utils-data-lake.ts, which already owns exactly this kind of framework-agnostic data-lake predicate, rather than into the view; the editUserDefinedVariable → editVariable rename is forced by the behaviour change rather than incidental)

4. Security — ✅ (no new dependency, no network call, no eval/v-html, no encoded blob, no env or secret use, no regex added; no workflow, build script, Dockerfile or Electron main-process file touched; the two new test files mock @/libs/settings-management and reach nothing outside the process)

5. Performance — ✅ (the round-6 computed over the joystick mapping is gone, and both replacements are a single object lookup per call — cheaper than the availableDataLakeVariables.find() the view did before; ToolsDataLakeView no longer instantiates the controller store or imports the joystick protocol module, so bootstrap ordering is untouched; no listener, timer or watcher added, so nothing needs teardown; the one cost worth knowing rather than fixing is that a persistValue variable driven by a joystick writes through setDataLakeVariableData:146 → savePersistentValues, and settingsManager.setKeyValue debounces per key at 100 ms (settings-management.ts:35), so a user who maps an axis to one of the two speeds pays a settings write and a BlueOS push roughly ten times a second while that axis moves — reachable before this PR through any user-created persistValue variable, and unchanged in kind by it)

6. UI / UX — ✅ (the gate removes affordances rather than adding a surface: no dialog was added, so no anatomy, theme="dark", token, padding, glass or stacking clause is engaged, and the value-only mode only hides fields inside the existing v-dialog, leaving its centred title, its divider and its footer as they were; the pencil is still an icon button in the existing action row and editVariable still calls logUserAction; saving is confirmed by the dialog closing and the row's Current Value changing, so the action is not silent; delete stays gated on isUserDefinedVariable, so a tunable internal variable gains an edit button without gaining a delete one, and its Source column still reads "Cockpit internal", which is true; the one behaviour worth knowing rather than fixing is that the two speed variables keep the pencil even when a joystick control is mapped to them, where a typed value is overwritten on the next controller update — the narrow case the removed mapping walk covered, now traded for a gate that also covers unmapped axes)

7. Code Quality & Style — ✅ (complexity-report.json is present for this head and reports 133 functions measured across the 9 changed files, triggeredCount 0 and not truncated, so it attributes no complexity or nesting trigger to anything the diff writes; the two new helpers are one expression each with explicit return types, no any, no scoped CSS, no re-implementation of an existing predicate; @typescript-eslint/explicit-function-return-type is satisfied on every added arrow, including the ones in the test mocks; vue/no-unused-properties is not engaged, since canEditVariable and editVariable are both referenced in the template; imports stay in simple-import-sort order in both changed components; max-len 180 is not approached by the added JSDoc)

8. Commit Hygiene — ✅ (five commits read from pr.json, one logical step each — restore persisted values, persist the two speeds, show the button for tunable variables, add the value-only mode, stamp provenance — none reverting or reimplementing another, and the round-6 gate was not left as a separate commit to be undone but rebased away entirely; c7c47f45 was reworded this round because its old subject named allowUserToChangeValue, which it no longer keys on; no wip/fixup! noise, subjects fix:-prefixed in line with the repository's history and each describing its own change; the Fix #2774/Fix #2775 references stay in the PR body rather than in any commit message)

9. Tests — ✅ (no existing test touched or weakened; src/tests/libs/joystick/protocols/data-lake.test.ts disappears with the helper it pinned, leaving no orphan; the two remaining files sit alongside the repository's existing suites under src/tests/libs/ and pin the two behaviours this PR turns on — the merge-preserving restore, and userTunable being what grants editability where allowUserToChangeValue no longer does; both mock @/libs/settings-management, and the mock factory in the actions test refers to an outer storage binding safely, because the module under test is imported dynamically inside each test rather than at file scope. Worth knowing rather than fixing here: CI runs yarn lint, yarn typecheck and yarn build only and never yarn test:ci, so neither file runs on any push — the repository's gap, not this PR's)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README change is owed; both new exported helpers and both new interface fields carry complete, correctly typed doc blocks, and the userTunable one states how it differs from allowUserToChangeValue, which is the distinction the whole change turns on)

11. Nitpicks / Optional — ✅ (nothing left over: the one user-facing string added this round is the "Edit Variable Value" dialog title, which matches the two titles it sits beside; the value-only layout reuses the existing field styling and introduces no new class beyond the name line)

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

[rafaellehmkuhl]

Review follow-up — round 8

Round 7 closed with READY TO MERGE, so nothing here comes from a finding. The provenance model was reworked on my own initiative, because it was paying the price of a whole provenance system and still missing part of #2775. The branch was rebased onto current master (262a797) and force-pushed; the two behaviour commits at the base (e073d09, 54351f7) are unchanged.

Why it changed

• ardupilotSystemId was never fixed. userDefined/userTunable lived only on DataLakeVariable, never on TransformingFunction, so the delete gate stayed isCompoundVariable(id) || isUserDefinedVariable(id) and every compound kept both actions — including the legacy ArduPilot System ID, which Several internal variables are being treated as "user defined" #2775 names by hand, and the POI coordinates, which syncPoiCoordinateVariables silently reverts after an edit.
• userDefined bought almost nothing. isUserDefinedDataLakeVariable was userDefined === true || persistent === true, and both sites that create a user variable already set persistent. The only case the stamp decided was a variable created with Persist variable between boots unticked — which does not survive a boot anyway, so the stamp preserved two buttons on a session-scoped variable.
• userTunable was one letter of meaning away from allowUserToChangeValue. Telling the two apart required reading both JSDocs. That is a defect regardless of correctness.

What replaced them

• systemOwned, on DataLakeVariable and on TransformingFunction. Absent means the user's own, so anything stored by an earlier version, and anything a script creates through the globally exposed createDataLakeVariable, stays theirs. ensureCockpitTransformingFunction() creates the function or records that it is Cockpit's, leaving a tuned expression alone, and replaced the find/create dance duplicated at four call sites. TransformingFunctionDialog carries the mark through an edit instead of dropping it.
• allowUserToChangeValue now reads as "the user may set this value" — by hand from the page, and, when the variable is not a compound one, through a control. The edit gate is !systemOwned || allowUserToChangeValue; delete is !systemOwned, full stop.
• A new first commit (e0778a9) stops any control writing to a compound variable. The button picker already excluded them; the axis picker and the five custom widget input elements went by the flag alone. It lands before anything is marked, so no intermediate commit offers a compound as a write target. A widget element can still be bound to a compound to display it, read-only.

Deliberately reversing finding 6.2

Round 6 removed the value-only pencil from the MANUAL_CONTROL axis inputs and the zoom/focus increase/decrease variables. They get it back here: they carry allowUserToChangeValue, and that is now the single question the page asks. Accepted knowingly — the edit is value-only so it cannot damage metadata, the write is overwritten by the next controller update, and with no joystick mapped it is a usable way to exercise zoom or an axis. The alternative was a third state whose only job was excluding these ten rows, which is what made the previous model hard to hold in your head.

Correction to the PR body

The old description claimed a variable created with persistence unticked "would have lost both actions". That was wrong — persistent: false satisfies != null, so master still treats it as the user's. Half the justification for userDefined rested on it. The description has been rewritten.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 8)

6 open — 2 minor (one of them disputed) and 4 nits — and 13 closed across rounds 1–7.

The pull request gives Cockpit a way to remember which data-lake variables it created itself, instead of guessing from whether a variable happened to set a persistence flag. Variables the app creates are now stamped as its own; anything unstamped — including everything already stored on a user's machine and anything a script creates — counts as the user's, so users keep the edit and delete buttons they had. On the Data Lake page the user can freely edit and delete their own variables, can change the value (but not the name, type or persistence) of the handful the app deliberately exposes for tuning, and can do neither to the rest. Separately it stops joysticks and widget controls from being pointed at computed variables, whose value is recalculated and would immediately discard anything written to it, and it fixes two value-restoring bugs so the camera zoom and focus speeds survive a reboot and the edit dialog shows a stored 0 or false instead of an empty box.

What still needs attention

#	Problem	What it means	Severity	Status
1.8	Telemetry variables can be relabelled as the user's own	A vehicle-telemetry value that an existing dashboard widget is already showing can appear on the Data Lake page as if the user had created it, offering a delete button that removes something the app depends on.	minor	❌
6.3	Editable value box on ten controller inputs	The Data Lake page still lets you type a value into the six joystick axis readings and the four zoom/focus step values, and a controller overwrites whatever you typed a fraction of a second later.	minor	💬
0.1	Description overstates the protection	The pull request text says editing one of Cockpit's variables is limited to the value; for computed variables the full editor still opens, so the description does not match what a reviewer will find.	nit	❌
6.4	Computed variables no longer identifiable in the table	The Source column no longer says which rows are calculated from an expression, and typing "compound" in the search box no longer finds them.	nit	❌
6.5	One edit button left unguarded	The pencil next to a computed variable can still open its editor if it is ever reached by any route other than the table, unlike its two neighbours which now refuse.	nit	❌
9.1	A test only passes when its neighbours run first	Running one of the new tests on its own fails, which will confuse whoever next debugs that file.	nit	❌

Since round 7 — 0 closed, 6 raised, comparing 2dd00a3 → e28dce5

Range. 2dd00a3b296cdc73f80823400d14e0943ebfeae2 → e28dce57cd514cdd1d1c2190c1824957951684ea. Every commit SHA in pr.json is new since round 7, so the branch was force-pushed again. incremental.diff reflects that: it reproduces base→head hunks rather than a true PREV_SHA...HEAD_SHA delta, so it is not usable as a record of what moved. All status judgements below, and every new finding, were made against pr.diff and the base checkout.

Status transitions — none. All 13 entries carried in from round 7 were already closed (11 addressed, 2 obsolete), and they stay closed. That is not a formality this round: the mechanism underneath most of them was rebuilt, so each was re-verified against the new diff rather than waved through.

• 1.2 (addressed) — camera-zoom-speed and camera-focus-speed still carry allowUserToChangeValue: true via speedVariableConfig (src/libs/joystick/protocols/predefined-resources.ts:49), so the joystick axis picker still offers them.
• 1.3 (addressed) — savePersistentValues still merges into the stored object instead of rebuilding it (src/libs/actions/data-lake.ts, hunk at @@ -49,9 +49,14 @@), and deleteDataLakeVariable still removes only its own key.
• 1.4 (addressed) — the getKeyValue result is read through the typed getPersistentValues() helper, not indexed directly.
• 1.5, 1.6 (addressed) — the userDefined stamp those findings were about no longer exists; the replacement default ("no systemOwned means the user's") reaches the same outcome by construction, and src/tests/libs/utils-data-lake.test.ts pins it.
• 1.7 (addressed) — initialValue.value = currentValue !== undefined ? String(currentValue) : '' is still in src/components/DataLakeVariableDialog.vue.
• 3.1 (addressed) — the three new helpers in src/libs/utils-data-lake.ts all take an id, no DataLakeVariable overload.
• 6.2 (addressed) — stays closed. The behaviour it described has been reintroduced by a different mechanism, which is raised fresh below as 6.3 rather than reopening a closed entry.
• 1.1, 6.1, r1-1.1, r1-1.2, r1-6.1 — the code they referred to is gone or unchanged; no change to their status.

Resolutions and votes. resolutions.json is [] and decisions.json is [] — nothing to apply, and no id was submitted that the ledger does not know.

Discussion since round 7. Two comments, both from rafaellehmkuhl: a bare /review (ignored as a command) and a "Review follow-up — round 8" note describing the rework. Three of its factual claims mattered and all three check out against the code: createDataLakeVariable is exposed globally at src/libs/cosmos.ts:643, so the "unmarked means the user's" default really does protect script-created variables; the joystick button picker already excluded transforming functions at src/views/ConfigurationJoystickView.vue:1032-1033, so only the axis picker needed the new guard; and Object.assign(stored, flags) in ensureCockpitTransformingFunction does reach the data-lake variable, because saveTransformingFunctions() → updateTransformingFunctionListeners() tears down and recreates every transforming-function variable. The same comment declares a deliberate reversal of finding 6.2; that is recorded as the author argument on 6.3, which is disputed and therefore still open — an argument cannot close a finding, only a maintainer can.

Change map — what was established before judging

Claims.

• Symptom: ownership was inferred from persistent != null, so internal variables that set persistent: false got full edit and delete. Verified. Base src/views/ToolsDataLakeView.vue isUserDefinedVariable reads …?.persistent != null, and src/libs/vehicle/mavlink/vehicle.ts:1599 and src/stores/omniscientLogger.ts create internal variables with an explicit persistent: false.
• Symptom: every compound variable got both actions, because Cockpit's transforming functions are indistinguishable from the user's. Verified. Base getVariableSource returned 'Compound' and the pencil for compounds was unguarded; the delete v-if used isUserDefinedVariable(item.id) || isCompoundVariable(item.id).
• Symptom: the camera zoom/focus speeds were unreachable and reset to 3 every boot. Verified. Base commonVariableConfig omitted persistValue, and base createDataLakeVariable wrote initialValue over storage unconditionally.
• Mechanism: a systemOwned flag on DataLakeVariable and TransformingFunction; absent means the user's. Verified in src/types/data-lake.ts (hunk @@ -32,9 +32,15 @@) and src/libs/actions/data-lake-transformations.ts (@@ -259,8 +259,15 @@), and enforced by canUserChangeDataLakeVariable / canUserDeleteDataLakeVariable in the new src/libs/utils-data-lake.ts.
• Mechanism: editing one of Cockpit's opens the dialog limited to the value field, so it cannot rewrite the id, type or persistence. Contradicted for compound variables — see finding 0.1.
• Mechanism: the compound-exclusion commit lands first so no intermediate commit offers a compound as a write target. Verified. e0778a9c precedes 4cb5abaf and e28dce57, which are what give compounds allowUserToChangeValue: true.

Failure site. The misbehaving code is isUserDefinedVariable in src/views/ToolsDataLakeView.vue (base line 439) and the two v-ifs that consumed it, plus createDataLakeVariable in src/libs/actions/data-lake.ts:75-96, which set dataLakeVariableData[variable.id] = initialValue with no regard for what loadPersistentVariables() had restored. Both are in the diff, and the fix is at the single consumer in each case rather than at the call sites.

Entry points.

Function	Reached from	Frequency
createDataLakeVariable (data-lake.ts)	setupPredefinedLakeAndActionResources() from src/main.ts:96; MAVLink addToDataLake; widget mount	one-shot at boot, then per new variable
getPersistentValues / savePersistentValues	createDataLakeVariable, setDataLakeVariableData, deleteDataLakeVariable	per value write on a persistValue variable
ensureCockpitTransformingFunction	setupPredefinedLakeAndActionResources, poi-data-lake.ts, mainVehicle.ts	one-shot at boot
isCompoundDataLakeVariable	isInput in the five custom widget elements; filteredAndSortedAxisActions; isCompoundVariable in the table	per render of the element / per keystroke in the picker
canUserChangeDataLakeVariable, canUserDeleteDataLakeVariable, isSystemOwnedDataLakeVariable	the Data Lake table's v-ifs and getVariableSource; DataLakeVariableDialog setup	per rendered table row
editVariable, deleteVariable, editCompoundVariable	pencil / bin click in the table	per user action
saveVariable (DataLakeVariableDialog)	Save button	per user action
updateTransformingFunction (TransformingFunctionDialog)	Save button	per user action
registerCockpitActions (MiniWidgetInstantiator)	onMounted	one-shot per widget instance

Nothing changed is unreached; the per-row helpers are three object lookups each, and isCompoundDataLakeVariable is an Array.some over a list that is a few dozen entries at most, so none of this lands on a hot path.

Invariants. The change rests on one rule: every variable Cockpit itself creates carries systemOwned: true; anything unstamped is the user's. Enumerating the producers — 28 createDataLakeVariable( call sites under src/ — the stamp is present at all of the internal ones: src/libs/generic-websocket.ts:148, src/libs/sensors/gnss.ts (gnssVariablesForDevice), src/libs/vehicle/ardupilot/ardusub.ts:87, five sites in src/libs/vehicle/mavlink/vehicle.ts (1562, 1599, 1611, 1634, 1647), three in src/stores/mainVehicle.ts (296, 306, 892), seven in src/stores/omniscientLogger.ts, and predefined-resources.ts. The three deliberately unstamped ones are the user-facing creators (InputElementConfig.vue:534, DataLakeVariableDialog.vue:288, MiniWidgetInstantiator.vue:110), and data-lake-transformations.ts propagates the flag through the transforming-function path. The enumeration is exhaustive but nothing enforces it — a future internal producer that forgets the flag fails towards the user keeping their buttons, which is the safe direction. The one producer the enumeration shows as genuinely uncovered is MiniWidgetInstantiator.vue:110, which is finding 1.8.

0. Summary — 1 finding

0.1 nit — the PR description overstates what value-only editing protects.

The description states: "Editing one of Cockpit's opens the dialog limited to the value field, so it cannot rewrite the id, type or persistence the app depends on." That holds for plain variables, where DataLakeVariableDialog.vue sets valueOnlyEditMode from isSystemOwnedDataLakeVariable and hides every metadata field. It does not hold for compound ones: src/views/ToolsDataLakeView.vue:445 editCompoundVariable opens TransformingFunctionDialog, which exposes name, type and expression, and the pencil beside a compound is shown whenever canEditVariable(item.id) is true — which the PR newly makes true for camera-zoom, camera-focus and the six MANUAL_CONTROL axis outputs, since ensureCockpitTransformingFunction gives them allowUserToChangeValue: true.

This is not a regression — base showed that pencil for every compound and a delete button besides — so it is a documentation problem, not a code one. Either narrow the sentence to non-compound variables, or say that a Cockpit compound stays fully editable by design because its expression is meant to be tuned.

Consequence. A reviewer reading the description forms the wrong idea of what the change guarantees, and the next person to touch this code inherits that idea.

1. Correctness & Implementation Bugs — 1 finding

1.8 minor — a widget replaying a pre-PR options snapshot recreates a Cockpit variable without the stamp.

src/components/MiniWidgetInstantiator.vue:103-117 recreates the variable a widget element is bound to, from the copy stored in the widget's own options:

const registerCockpitActions = (): void => {
  if (miniWidget.value.options.dataLakeVariable &&
      getDataLakeVariableInfo(miniWidget.value.options.dataLakeVariable.id) !== undefined) return
  if (miniWidget.value.options.dataLakeVariable) {
    createDataLakeVariable(miniWidget.value.options.dataLakeVariable, …)
  }
}
onMounted(() => registerCockpitActions())

The existence guard means it never clobbers a variable that is already registered, and that covers the common case. It does not cover the case where the widget mounts before the real registrar has run. Those registrars all create-if-absent and never update: src/libs/vehicle/mavlink/vehicle.ts:1599 and the GNSS, stream-stats and ArduSub producers all skip an id that already exists. So once the widget has created the variable unstamped, nothing ever stamps it — the telemetry producer finds it present and moves on. And the snapshot in an existing user's profile was written before this PR, so it has no systemOwned field at all.

The visible result: a Dial or Slider bound to, say, mavlink/1/1/VFR_HUD/heading in a profile saved today makes that telemetry variable show as User defined on the Data Lake page, with both the pencil and the bin. Deleting it removes a variable the vehicle plumbing expects to exist. Base did not do this, because those same variables' snapshots carry no persistent field either, and undefined != null is false — so base classified them as internal for the wrong reason but with the right outcome. This PR's default flips that.

The fix that closes it at the chokepoint rather than at N producers: in registerCockpitActions, do not recreate a variable this element did not create — or, if the recreation is needed for the offline case, carry systemOwned from getDataLakeVariableInfo when it is known and drop the stored copy's ownership fields otherwise. Refreshing the stored snapshot on save would fix new profiles but not existing ones.

Consequence. A vehicle-telemetry value that an existing dashboard widget is already showing can appear on the Data Lake page as if the user had created it, offering a delete button that removes something the app depends on.

2. Persistence & User Data — inventory, no findings

Key	Backend	What happened
cockpit-persistent-data-lake-variables	machine-local (settings-management.ts)	Reshaped by addition: stored DataLakeVariable objects may now carry systemOwned. Optional, so an old record reads back fine and lands in the "user's own" bucket — the safe side.
cockpit-persistent-data-lake-values	machine-local (settings-management.ts)	Write path changed, shape unchanged. savePersistentValues now merges rather than rebuilding (this is the fix for closed finding 1.3), deleteDataLakeVariable removes only its own key, and createDataLakeVariable reads the stored value back through getPersistentValues(). Two new entries appear here: camera-zoom-speed and camera-focus-speed, from persistValue: true.
cockpit-transforming-functions	vehicle-synced (useBlueOsStorage)	Reshaped by addition: systemOwned and allowUserToChangeValue on TransformingFunction. ensureCockpitTransformingFunction writes the flags onto already-stored functions in place via Object.assign(stored, flags) + saveTransformingFunctions().

Judgement on each:

• Both machine-local keys hold machine-local things (a user's own variable definitions and their last values), and both are cockpit--prefixed. The variable objects do repeat their own id inside the value — but they are stored as an array, not keyed by id, so there is no duplicate source of truth; and cockpit-persistent-data-lake-values is keyed by id with a bare scalar value.
• The transforming-functions key is vehicle-synced, which is right: an expression is about the vehicle, not the topside computer. The in-place stamping is the only thing here that rewrites a user's stored data automatically, so it is worth being explicit about why it is acceptable: it adds two boolean fields to functions the app itself defines by a fixed id list, it leaves expression, name, type and description untouched (the PR body's claim, verified in the code — flags contains only systemOwned and allowUserToChangeValue), it early-returns when the flags already match, and re-running it converges. That is idempotent and non-destructive, so it does not need the versioned-key treatment AGENTS.md-adjacent guidance reserves for reshaping migrations.
• No machine-specific value (device path, COM port, window geometry) is synced by any of this.
• Nobody is stranded on an old default: the two new persistValue variables keep their existing 3 until the user changes them, and an unstamped stored variable resolves to "the user's own", which is the behaviour the user already had.

6. UI / UX — 3 findings

6.3 minor — the value-only pencil returns to the ten controller-written inputs. 💬 disputed

commonVariableConfig in src/libs/joystick/protocols/predefined-resources.ts now carries both flags:

const commonVariableConfig = {
  type: 'number' as DataLakeVariableType,
  allowUserToChangeValue: true,
  systemOwned: true,
}

canUserChangeDataLakeVariable returns true for systemOwned && allowUserToChangeValue, so the Data Lake page shows a pencil on the four camera step variables (camera-zoom-decrease, camera-zoom-increase, camera-focus-decrease, camera-focus-increase) and on the six MANUAL_CONTROL axis inputs created at the second commonVariableConfig (same file, hunk @@ -156,22 +159,24 @@). Those ten hold a value written by whatever control is mapped to them; a value typed into the dialog is overwritten by the next controller update. This is the behaviour closed as 6.2 in round 7, arriving by a different route — the flag that used to be omitted for them is now set for all of them at once.

The narrow fix is to split the config the way speedVariableConfig already splits out persistValue: keep allowUserToChangeValue: true on the two speeds and the compound expressions, which are genuinely tuned by hand, and drop it from the increase/decrease pair and the axis inputs. That is one more object literal, not a third flag.

Author's argument (recorded, not adopted): the edit is value-only so it cannot damage metadata, the write being overwritten is simply what a mapped control does, and with no joystick mapped it is a usable way to exercise zoom or an axis — the alternative was a third flag whose only job was excluding these ten rows.

Consequence. The Data Lake page still lets you type a value into the six joystick axis readings and the four zoom/focus step values, and a controller overwrites whatever you typed a fraction of a second later.

---

6.4 nit — the Source column loses the compound distinction, and the search that used it.

src/views/ToolsDataLakeView.vue:216 narrows the type from 'Compound' | 'Cockpit internal' | 'User defined' to two values, and getVariableSource (line 327) becomes a single ternary on isSystemOwnedDataLakeVariable. The PR body says this deliberately — the column now answers ownership rather than mixing it with computedness — and that is a defensible split. The cost is not mentioned: the search box filters on v.source.toLowerCase() (src/views/ToolsDataLakeView.vue:349), so typing "compound" used to bring up exactly the calculated variables and now brings up nothing, and the table no longer tells the user which rows are derived from an expression at all. The page still knows — isCompoundVariable(item.id) decides which pencil to show — so a second column, or a small chip beside the name, would keep both answers without putting them back in one field.

Consequence. The Source column no longer says which rows are calculated from an expression, and typing "compound" in the search box no longer finds them.

---

6.5 nit — editCompoundVariable did not get the guard its two neighbours did.

editVariable (src/views/ToolsDataLakeView.vue:385) gained if (variable && canEditVariable(variableId)) with a snackbar on the else, and deleteVariable (line 411) gained an early if (!canDeleteVariable(id)) with a snackbar and a return. editCompoundVariable (line 445) still opens the dialog unconditionally. In practice the template's v-if="isCompoundVariable(item.id) && canEditVariable(item.id)" is the only caller, so nothing reaches it wrongly today — but the handler-level guard is exactly what the other two just added, and the asymmetry invites the next caller to skip the check.

Consequence. The pencil next to a computed variable can still open its editor if it is ever reached by any route other than the table, unlike its two neighbours which now refuse.

9. Tests — 1 finding

9.1 nit — the transformations test file's third test only passes because the first one ran.

src/tests/libs/actions/data-lake-transformations.test.ts (added, 85 lines) runs its three tests against one module instance and one mutable storage object. Test 1 calls ensureCockpitTransformingFunction on a pre-existing unmarked camera-zoom and asserts the flag was recorded. Test 3, "what is recorded reaches the data lake variable the page reads", then asserts canUserDeleteDataLakeVariable('camera-zoom') === false — which holds only because test 1 stamped it. Run test 3 with -t or .only and it fails.

The two sibling files do not have this problem: data-lake.test.ts uses distinct ids per test and utils-data-lake.test.ts creates what it asserts on. Either stamp camera-zoom inside test 3 as well, or give test 3 its own id.

Consequence. Running one of the new tests on its own fails, which will confuse whoever next debugs that file.

Sections with nothing to report (6)

3. AGENTS.md Adherence — ✅ (no new dependency and no package.json change; both added TSPropertySignature fields carry JSDoc as jsdoc/require-jsdoc demands; the three new utils-data-lake.ts helpers and ensureCockpitTransformingFunction are all called in this PR, so nothing is groundwork; the reworded JSDoc above allowUserToChangeValue? in src/types/data-lake.ts is the one comment touched without its own line changing, and it is permitted because the flag's meaning is what this PR changes; ensureCockpitTransformingFunction replaces a find/create dance duplicated across four call sites, which is a net simplification)

4. Security — ✅ (no new dependency, no network call, no eval/Function/v-html, no environment variable or credential, nothing under scripts/, .github/ or src/electron/; the only encoded-looking additions are the MAVLink axis expressions in predefined-resources.ts, which are the existing template strings unchanged; no hidden Unicode in the added identifiers)

5. Performance — ✅ (the three new per-row helpers are object lookups, and isCompoundDataLakeVariable is an Array.some over a few dozen functions, called per rendered table row and per widget-element render, not on mavlink:onIncomingMessage or dataLake:setVariable; getPersistentValues() parses the stored object once per createDataLakeVariable on a persistValue variable, which is boot-time and a handful of variables; no listener, interval or watcher is added, so nothing needs a new teardown)

7. Code Quality & Style — ✅ (complexity-report.json for head e28dce57 reports 644 functions measured across 24 changed files with triggeredCount 0 and truncated false, so nothing tripped the complexity or depth thresholds; added lines stay inside max-len 180, every added arrow function has an explicit return type, the new @/libs/utils-data-lake imports sit in simple-import-sort order in all four consumers, and no any is introduced — CockpitFunctionFlags is a Pick of the interface it flags; { ...props.editFunction, ...otherProps } in TransformingFunctionDialog is safe because the id field is disabled while editing, so otherProps.id cannot diverge)

8. Commit Hygiene — ✅ (six commits, each one logical change, largest well under a few hundred lines; no wip/fixup!/address review noise; no #N or closing keyword in any subject or body — the Fix #2774 / Fix #2775 references are in the PR body where they belong; five fix: prefixes each describe a genuine defect and the sixth, data-lake: record which variables and compound variables Cockpit created, correctly avoids fix: for the new mechanism; no commit undoes an earlier one, and the compound-exclusion commit e0778a9c is ordered before the two that give compounds allowUserToChangeValue, so no intermediate state offers a compound as a write target)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README note is owed; the two new interface fields, all three new utils-data-lake.ts helpers, ensureCockpitTransformingFunction, isCompoundDataLakeVariable and getPersistentValues each carry a typed JSDoc block with non-empty @param/@returns; the renamed editVariable's JSDoc was updated with it — closed finding r1-6.1 — and the only documentation gap found is the PR-body mismatch raised as 0.1)

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

[github-actions]

🙋 Decision needed — 6.3

Value-only pencil returns to the six axis inputs and four zoom/focus step variables

The author's argument: The edit is value-only so it cannot damage metadata, the write being overwritten is simply what a mapped control does, and with no joystick mapped it is a usable way to exercise zoom or an axis - the alternative was a third flag whose only job was excluding these ten rows.

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]

Review follow-up — round 9

Round 8 raised 6. Four are addressed below, one closes by construction and one is voted. Branch force-pushed: e28dce5 → c990abf, same six commits, the last two rewritten.

Done

• src/types/data-lake.ts, src/libs/actions/data-lake.ts, src/libs/actions/data-lake-transformations.ts (1.8) — the flag's default is inverted. systemOwned is now assumed when absent, and the three user-facing creators say otherwise, which removes the stamp from fifteen internal call sites and takes stores/omniscientLogger.ts, stores/mainVehicle.ts, libs/sensors/gnss.ts and libs/generic-websocket.ts out of the diff entirely. MiniWidgetInstantiator.vue:110, the uncovered producer the finding names, is covered by construction: an options snapshot with no flag now replays as Cockpit's rather than as the user's.

  What this costs is paid at the two load boundaries, which read an unflagged stored entry as the user's. For cockpit-persistent-data-lake-variables that is provable — only the three user-facing creators ever write there, since Cockpit never marks its own persistent — and for cockpit-transforming-functions it is the reading that cannot take a compound away from whoever wrote it, with Cockpit reclaiming its own through the setup routines. Without it, upgrading would hand every variable a user had already created over to Cockpit and remove their delete button. src/tests/libs/actions/data-lake.test.ts pins the first and the transformations test pins the second; removing either default fails three tests.

• src/views/ToolsDataLakeView.vue (6.4) — which rows are computed moves to an mdi-function-variant icon beside the name, the same one the "Add compound variable" button uses, in a fixed 18px slot so names stay aligned across rows. Its tooltip says more than the old cell did. Search is restored by a term in the filter predicate, using startsWith rather than includes so a stray "o" does not surface every compound.

• src/views/ToolsDataLakeView.vue (6.5) — editCompoundVariable got the early guard and snackbar its two neighbours have.

• src/tests/libs/actions/data-lake-transformations.test.ts (9.1) — the third test claims camera-zoom itself instead of relying on the first having run. Verified with vitest -t.

• PR description (0.1) — corrected. It now separates the two cases: a plain Cockpit variable opens limited to the value, a Cockpit compound stays fully editable by design, which is what master already allowed.

Voted

• 6.3 — 👍 on the decision comment, accepting the argument. The value-only pencil stays on the six axis inputs and four zoom/focus step variables.

Correction to round 8's follow-up

That note implied the default inversion would also fix the late marking of ardupilotSystemId. It does not. The load boundary has to read an unflagged compound as the user's, so the legacy alias still shows as "User defined" until a vehicle connects and createPredefinedDataLakeVariables() runs — the same window the POI coordinates have until a map component mounts. Both self-heal and neither is a regression against master, where both were editable and deletable outright. Closing the window means moving those two ensure calls to bootstrap, which is a separate change and not in this PR.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 9)

2 open — both minor, both raised this round — and 19 closed across rounds 1–9.

The pull request gives Cockpit a way to remember which data-lake variables it created itself, instead of guessing from whether a variable happened to set a persistence flag. This round the flag's default was turned around: a variable that says nothing about who made it is now taken as Cockpit's, and the three places that create one for the user — the Data Lake dialog, the compound dialog and the custom-widget input config — say so explicitly. Anything read back from a user's existing storage is still treated as theirs, so nobody loses the delete button on a variable they already made. On the Data Lake page the user can freely edit and delete their own variables, can change the value (but not the name, type or persistence) of the handful the app deliberately exposes for tuning, and can do neither to the rest. Separately it stops joysticks and widget controls from being pointed at computed variables, whose value is recalculated and would immediately discard anything written to it, and it fixes two value-restoring bugs so the camera zoom and focus speeds survive a reboot and the edit dialog shows a stored 0 or false instead of an empty box.

Everything round 8 left open is now closed. What remains are two things this round introduced: one narrow leftover of the ownership question at the same place round 8's finding 1.8 named, and some re-wrapped lines that no longer carry a change.

What still needs attention

#	Problem	What it means	Severity	Status
1.9	A user's own non-persistent variable can be relabelled as Cockpit's	A variable the user created with "Persist variable between boots" switched off, and then pointed a widget element at, comes back after a restart marked as Cockpit's — no delete button, and the editor limited to its value.	minor	❌
3.2	Four re-wrapped lines with no change in them	Three files carry hunks that only move existing code onto more lines; one of them, ardusub.ts, is in the diff for nothing else at all.	minor	❌

Since round 8 — 6 closed, 2 raised, comparing e28dce5 → c990abf

Range. e28dce57cd514cdd1d1c2190c1824957951684ea → c990abf366fc0bbcad30cd81f6bd479c79d17f58. The branch was force-pushed: the same six commit subjects are present but the last two carry new SHAs.

incremental.diff is not usable as a record of what moved this round. It is a three-dot comparison, so its baseline is the last commit the two heads share (e0778a9c), not e28dce5 — it therefore reproduces everything the final two commits do, including work that was already in place at round 8 and did not move. Two tells: it shows src/libs/utils-data-lake.ts as +32/-0 as though the helpers were new this round, and it lists src/tests/libs/actions/data-lake.test.ts as modified where pr.diff shows it as a new file. All status judgements below, and both new findings, were made against pr.diff and the base checkout.

Status transitions — 6 closed, 0 still open from round 8.

• 0.1 ✅ Addressed. The PR body no longer claims value-only editing across the board. It now splits the two cases — "Editing one of Cockpit's plain variables opens the dialog limited to the value field… A Cockpit compound stays fully editable in the compound dialog, name and expression included, by design" — which is the second of the two remedies the finding named, and it matches what the code does. Verified against the body in pr.json, not against a claim about it.
• 1.8 ✅ Addressed. Closed by the default inversion rather than by a change at the producer. isSystemOwnedDataLakeVariable is now getDataLakeVariableInfo(id)?.systemOwned !== false (src/libs/utils-data-lake.ts), so the unflagged snapshot MiniWidgetInstantiator.vue:110 replays reads as Cockpit's. A Dial bound to mavlink/1/1/VFR_HUD/heading in an existing profile therefore shows as Cockpit internal with no bin, which is what the finding asked for, and src/tests/libs/utils-data-lake.test.ts pins exactly that id. The reverse case the inversion opens at the same producer is narrower and is raised separately as 1.9 — that is a new finding, not this one reopened.
• 6.3 ☑️ Resolved by maintainer vote. decisions.json records verdict accept on the decision comment, gated against the ledger, with rafaellehmkuhl in favour and nobody against. The author's argument — that the edit is value-only so it cannot damage metadata, that being overwritten is simply what a mapped control does, and that with no joystick attached it is a usable way to exercise zoom or an axis — is accepted. The value-only pencil stays on the six axis inputs and the four zoom/focus step variables. The code is unchanged here; this closes on the vote.
• 6.4 ✅ Addressed. Both halves of the finding landed. Which rows are computed is back in the table as an mdi-function-variant icon in a fixed w-[18px] slot beside the name (src/views/ToolsDataLakeView.vue:75-86), with a tooltip that says what a compound variable is; the name column shrank 390px → 364px to make room, so rows stay aligned. Search is back through a term in the filter predicate — (isCompoundVariable(v.id) && 'compound'.startsWith(query)) — so typing "compound", or any prefix of it, surfaces exactly the computed rows again.
• 6.5 ✅ Addressed. editCompoundVariable now opens with if (!canEditVariable(id)) { openSnackbar(…); return }, the same shape its two neighbours have.
• 9.1 ✅ Addressed. The third test in src/tests/libs/actions/data-lake-transformations.test.ts now claims camera-zoom itself before asserting on it (ensureCockpitTransformingFunction({ ...cameraZoom, allowUserToChangeValue: true })), with a comment saying why. Traced both orderings: run after test 1 the stored.systemOwned === true early return fires and the stamp is already there; run alone, Object.assign + saveTransformingFunctions() puts it there. The assertion holds either way.

The 13 entries carried in already closed stay closed. Unlike round 8 that is close to a formality this time — the mechanism they were verified against is the same one, only with its default reversed, and each still resolves the same way. Two are worth naming because the inversion moved the ground under them: 1.5 and 1.6 (input-element and dialog-created variables keeping delete and full edit) no longer hold because unflagged means the user's; they now hold because those creators write systemOwned: false outright at InputElementConfig.vue:523 and DataLakeVariableDialog.vue:300, and because loadPersistentVariables reads an unflagged stored entry as theirs. Both routes were re-traced; both still land the same way.

Resolutions. resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no id was submitted that the ledger does not know.

Votes. One entry in decisions.json, applied above. gated is true, so it was checked against the ledger and answers the argument the ledger was actually carrying. No vote is left open.

Discussion since round 8. Two comments, both from rafaellehmkuhl: a bare /review (ignored as a command) and a "Review follow-up — round 9" note. The note is a claim about the code, so its load-bearing assertions were checked rather than taken:

• "only the three user-facing creators ever write there, since Cockpit never marks its own persistent" — verified. savePersistentVariables filters on variable?.persistent being truthy, and the only persistent: true reaching createDataLakeVariable anywhere under src/ is InputElementConfig.vue:521 and the user's checkbox in DataLakeVariableDialog.vue:141. The other five persistent: true matches in the tree are snackbar and dialog props, not data-lake variables. Internal creators either set persistent: false explicitly or omit it, so none of them can reach that key. This is what makes the "read storage as the user's" default safe, so it mattered.
• "MiniWidgetInstantiator.vue:110 … is covered by construction" — verified for the case 1.8 described, and not for its mirror. See finding 1.9.
• "the third test … verified with vitest -t" — verified by reading, see 9.1 above. The tests were not executed here.
• The correction to round 8's follow-up — that ardupilotSystemId and the POI coordinate functions still read as "User defined" between boot and the moment a vehicle connects or a map mounts — is correct, and is the honest reading. Round 8's note left the opposite impression and this withdraws it. Both windows self-heal and neither is a regression against master, where those compounds were editable and deletable outright; that is recorded in the change map below rather than raised as a finding.

Raised this round. 1.9 (minor) and 3.2 (minor), both found by re-running the full sweep over pr.diff, not over the increment.

Change map — what was established before judging

Claims.

• Symptom: ownership was inferred from persistent != null, so internal variables that set persistent: false got full edit and delete. Verified. Base src/views/ToolsDataLakeView.vue isUserDefinedVariable reads …?.persistent != null, and src/libs/vehicle/mavlink/vehicle.ts:1599 and src/stores/omniscientLogger.ts create internal variables with an explicit persistent: false.
• Symptom: every compound variable got both actions. Verified. Base getVariableSource returned 'Compound', the pencil beside a compound was unguarded, and the delete v-if was isUserDefinedVariable(item.id) || isCompoundVariable(item.id).
• Symptom: the camera zoom/focus speeds were unreachable and reset to 3 every boot. Verified. Base commonVariableConfig omitted persistValue, and base createDataLakeVariable wrote initialValue over storage unconditionally.
• Mechanism: systemOwned on DataLakeVariable and TransformingFunction; absent means Cockpit's, and the three user-facing creators set it false. Verified in src/types/data-lake.ts, src/libs/actions/data-lake-transformations.ts, and enforced by canUserChangeDataLakeVariable / canUserDeleteDataLakeVariable in src/libs/utils-data-lake.ts. This is the reversal of what rounds 1–8 reviewed.
• Mechanism: editing one of Cockpit's plain variables opens the dialog limited to the value field; a Cockpit compound stays fully editable by design. Verified, and the PR body now says both halves — this was finding 0.1.
• Mechanism: the compound-exclusion commit lands first so no intermediate commit offers a compound as a write target. Verified. e0778a9c still precedes 17e846bc and c990abf3, which are what give compounds allowUserToChangeValue: true.

Failure site. Unchanged from round 8: isUserDefinedVariable in src/views/ToolsDataLakeView.vue and the two v-ifs that consumed it, plus createDataLakeVariable in src/libs/actions/data-lake.ts, which set dataLakeVariableData[variable.id] = initialValue with no regard for what loadPersistentVariables() had restored. Both are in the diff, and each is fixed at its single consumer rather than at the call sites.

Entry points.

Function	Reached from	Frequency
createDataLakeVariable (data-lake.ts)	setupPredefinedLakeAndActionResources() from src/main.ts:96; MAVLink addToDataLake; setupAllTransformingFunctionsVariables; widget mount	one-shot at boot, then per new variable
loadPersistentVariables / loadTransformingFunctions	module import, before any component mounts	once per app start
getPersistentValues / savePersistentValues	createDataLakeVariable, setDataLakeVariableData, deleteDataLakeVariable	per value write on a persistValue variable
ensureCockpitTransformingFunction	setupPredefinedLakeAndActionResources (boot); MAVLinkVehicle on vehicle connect	one-shot at boot / on connect
isCompoundDataLakeVariable	isInput in the five custom widget elements; filteredAndSortedAxisActions; isCompoundVariable in the table, now twice per row	per render of the element / per rendered table row
canUserChangeDataLakeVariable, canUserDeleteDataLakeVariable, isSystemOwnedDataLakeVariable	the Data Lake table's v-ifs and getVariableSource; DataLakeVariableDialog setup	per rendered table row
editVariable, deleteVariable, editCompoundVariable	pencil / bin click in the table	per user action
registerCockpitActions (MiniWidgetInstantiator)	onMounted	one-shot per widget instance

Nothing changed is unreached. loadPersistentVariables() and loadTransformingFunctions() are both called at module scope (data-lake.ts:265 and its sibling), so they complete before registerCockpitActions can run for any widget — which is why a stored variable is never clobbered by a widget snapshot, and why finding 1.9 needs a variable that was not stored to reach it.

Invariants. The rule is now inverted from the one rounds 1–8 checked: every variable is Cockpit's unless it says otherwise, and only the user-facing creators say otherwise. Enumerating the exceptions rather than the rule makes the check much shorter — systemOwned: false appears at exactly three producers (DataLakeVariableDialog.vue:300, InputElementConfig.vue:523, and TransformingFunctionDialog.vue:230 via createTransformingFunction's new flags argument) — and it removed four files from the diff entirely. Three consequences follow, and each was traced:

• Storage has to read the other way, or upgrading would confiscate variables. Both load boundaries default to the user (data-lake.ts { systemOwned: false, ...variable }, data-lake-transformations.ts { systemOwned: false, ...withTrimmedId(func) }). For the plain-variable key that is provable, per the verified persistent claim above. For the compound key it is not provable, so Cockpit reclaims its own through ensureCockpitTransformingFunction.
• Reclaiming is late for two of them. The six axis outputs and the two camera compounds are reclaimed at src/main.ts:96, before any UI exists. ardupilotSystemId is reclaimed only when a vehicle connects, and the POI coordinate functions only when syncPoiCoordinateVariables first runs from usePointsOfInterest. Until then they read as the user's, with a bin. Deleting one during that window is undone by the next ensure call, so it self-heals, and master offered edit and delete on those same rows permanently — so this is a shrinking of an existing hole, not a new one. Worth knowing about; not a finding.
• A variable created by a script through the global createDataLakeVariable is now Cockpit's. Round 8's default protected those; this one does not. That is a reversal of round 8's design but not a regression against master, where a script-created variable has no persistent field and undefined != null already classified it as internal.

The three remaining createTransformingFunction call sites all pass explicit flags ({ systemOwned: false } from the dialog, { systemOwned: true } from POI), so no path leaves a freshly created function to be re-read as something else on the next boot — which is what the comment at poi-data-lake.ts:66 is guarding against.

1. Correctness & Implementation Bugs — 1 finding

1.9 minor — the inverted default leaves the user's non-persistent variables exposed at the same producer 1.8 named.

Closed finding 1.8 was about a widget snapshot replaying a telemetry variable as the user's. The inversion fixes that and opens the mirror case at the same spot, for the smaller set of variables that storage cannot vouch for.

src/components/InputElementConfig.vue:284 binds the whole variable object into the element's options, not just its id:

<select v-model="currentElement.options.dataLakeVariable">
  <option v-for="variable in availableDataLakeVariables" :value="variable">

and MiniWidgetInstantiator.vue:103-117 replays that stored object at mount when no variable with that id is registered. The reachable chain, all of it on master today:

1. The user opens the Data Lake page, adds a variable, and unchecks "Persist variable between boots". DataLakeVariableDialog creates it with persistent: false and allowUserToChangeValue: true.
2. They add a Slider (or Dial, Switch, Dropdown, Checkbox) to a custom widget and pick that variable from the selector above. The whole object — no systemOwned, because this profile predates the flag — is saved into the widget's options.
3. They restart. loadPersistentVariables does not restore the variable, because savePersistentVariables only ever stored persistent ones. registerCockpitActions recreates it from the snapshot instead, unflagged.
4. On the Data Lake page it is now Cockpit internal: canUserDeleteDataLakeVariable is false, so the bin is gone, and the pencil that remains (allowUserToChangeValue is in the snapshot) opens the value-only dialog. On master the same row read User defined with a full editor and a bin, because persistent: false != null is true.

The variable is theirs, it only exists at all because the widget resurrected it, and the only way left to remove it is to delete the widget element. Note what is not affected, since it bounds this tightly: a variable created with the checkbox left at its default is in cockpit-persistent-data-lake-variables, is restored at module import with systemOwned: false, and the existence guard in registerCockpitActions then stops the snapshot from touching it. So this needs a user who deliberately turned persistence off.

The failure direction is now the safe one — nothing the app depends on can be deleted by mistake, which is the half that mattered — so this is worth less than 1.8 was. The chokepoint fix is the same one 1.8 suggested and is unchanged by the inversion: in registerCockpitActions, drop the ownership fields from the stored copy before recreating, or carry them from getDataLakeVariableInfo when it knows. Alternatively, decide that this configuration is not worth the code and say so in the PR body, which is a defensible answer — the point is that it is currently unstated.

Consequence. A variable the user created with "Persist variable between boots" switched off, and then pointed a widget element at, comes back after a restart marked as Cockpit's: no delete button, and the editor limited to its value.

3. AGENTS.md Adherence — 1 finding

3.2 minor — four re-wrapped hunks that no longer carry a change.

Round 8's approach added systemOwned: true to internal creators, which pushed several one-line object literals past a comfortable width and onto multiple lines. This round removed the flag from those literals. The wrapping stayed:

Location	Head	Base
src/libs/vehicle/ardupilot/ardusub.ts:87-91	createDataLakeVariable({ over 5 lines	the same call on 1 line
src/libs/vehicle/mavlink/vehicle.ts:1609-1613	createDataLakeVariable({ over 5 lines	the same call on 1 line
src/libs/joystick/protocols/predefined-resources.ts:44-47	commonVariableConfig over 4 lines	the same object on 1 line
src/libs/joystick/protocols/predefined-resources.ts:161-164	commonVariableConfig over 4 lines	the same object on 1 line

In all four the content is byte-identical to base — same keys, same values, same order. AGENTS.md lists "reformatting or re-wrapping lines outside your diff" among the things forbidden unless asked for, and the cost here is concrete rather than theoretical: src/libs/vehicle/ardupilot/ardusub.ts appears in this pull request for no other reason, so it shows up in the changed-files list, in git log --follow, and in the blame for those five lines, all pointing at a change that did not touch it.

This will not clean itself up. Prettier is configured at printWidth: 120 and every one of these fits on one line at 102–110 characters, but Prettier preserves an object literal's multi-line form whenever the author put a newline after the { — so yarn lint:fix leaves all four exactly as they are. Collapsing them back to their base text is a four-line edit and takes ardusub.ts out of the pull request entirely.

Consequence. Three files carry hunks that only move existing code onto more lines, and one file is in the diff with nothing else in it, which costs every future reader of that blame a detour.

2. Persistence & User Data — inventory, no findings

Key	Backend	What happened
cockpit-persistent-data-lake-variables	machine-local (settings-management.ts)	Reshaped by addition: stored DataLakeVariable objects may now carry systemOwned. Read back through { systemOwned: false, ...variable }, so an old record lands in the "user's own" bucket.
cockpit-persistent-data-lake-values	machine-local (settings-management.ts)	Write path changed, shape unchanged. savePersistentValues merges rather than rebuilding (closed finding 1.3), deleteDataLakeVariable removes only its own key, and createDataLakeVariable reads the stored value back through getPersistentValues(). Two new entries appear here: camera-zoom-speed and camera-focus-speed, from persistValue: true.
cockpit-transforming-functions	vehicle-synced (useBlueOsStorage)	Reshaped by addition: systemOwned and allowUserToChangeValue. Read back through { systemOwned: false, ...withTrimmedId(func) }; ensureCockpitTransformingFunction then writes the flags onto Cockpit's own in place via Object.assign(stored, flags) + saveTransformingFunctions().

Judgement on each:

• Both machine-local keys hold machine-local things (a user's own variable definitions and their last values), and both are cockpit--prefixed. The transforming-functions key being vehicle-synced is right: an expression is about the vehicle, not the topside computer. No machine-specific value (device path, COM port, window geometry) is synced by any of this.
• Two automatic in-place writes, both worth being explicit about rather than passing over, since AGENTS.md treats automatic user-data rewrites as a last resort:
  • ensureCockpitTransformingFunction stamps already-stored functions. It adds two booleans to functions the app defines by a fixed id list, leaves expression, name, type and description untouched (verified: flags contains only the two fields), early-returns when they already match, and converges after one boot. poi-data-lake.ts's ensureCoordinateFunction does the same for POI functions, once, via updateTransformingFunction. Idempotent and non-destructive.
  • New this round: loadPersistentVariables writes systemOwned: false into the in-memory record, and savePersistentVariables stores whole info objects — so the next time any persistent variable is created or deleted, the flag is written back onto every pre-flag entry. The value written is identical to what the read default computes, so it is a no-op in meaning and re-running it cannot diverge. Not a migration in any sense that needs a versioned key, but it does mean the key stops looking untouched after the first save.
• Nobody is stranded on an old default: the two new persistValue variables keep their existing 3 until the user changes them, and an unstamped stored variable resolves to "the user's own", which is the behaviour the user already had.

Sections with nothing to report (8)

0. Summary — ✅ (the PR body's account now matches the code on every point checked, including the plain/compound split that was finding 0.1, and the follow-up comment volunteers the one correction the body does not cover — the reclaim window on ardupilotSystemId and the POI functions — which is recorded in the change map)

4. Security — ✅ (no new dependency and no package.json change, no network call, no eval/Function/v-html added — evaluateDataLakeExpression is pre-existing and untouched — no environment variable or credential, nothing under scripts/, .github/ or src/electron/; the only encoded-looking additions are the MAVLink axis expressions in predefined-resources.ts, which are the existing template strings unchanged; the only non-ASCII character in the whole added diff is an em dash inside one JSDoc block, matching the prose style already in these files)

5. Performance — ✅ (the per-row helpers are object lookups, and isCompoundDataLakeVariable is an Array.some over a few dozen functions; the new icon column adds one more such call per rendered table row, which is a table the user opens deliberately, not a telemetry path; nothing new runs on mavlink:onIncomingMessage or dataLake:setVariable; getPersistentValues() parses the stored object once per createDataLakeVariable on a persistValue variable, which is boot-time and a handful of variables; no listener, interval or watcher is added, so nothing needs a new teardown)

6. UI / UX — ✅ (6.3 settled by vote, 6.4 and 6.5 addressed; the new compound marker is a non-interactive indicator rather than a control, so it owes no label or focus handling, and it sits in a fixed-width slot that is rendered whether or not the icon is, keeping names aligned; both new refusal paths — editCompoundVariable and deleteVariable — tell the user with a snackbar rather than failing silently, and every existing logUserAction on these handlers survived the rename from editUserDefinedVariable to editVariable)

7. Code Quality & Style — ✅ (complexity-report.json for head c990abf3 reports 466 functions measured across 21 changed files with triggeredCount 0 and truncated false, so nothing tripped the complexity or depth thresholds; added lines stay inside max-len 180, every added arrow function has an explicit return type, the new imports sit in simple-import-sort order in all consumers, and no any is introduced — TransformingFunctionFlags is a Pick of the interface it flags; { ...props.editFunction, ...otherProps } in TransformingFunctionDialog was re-traced against newFunction's initialiser, which carries exactly id, name, type, expression and description and so cannot overwrite the provenance flags with undefined; the five copies of the new two-line isInput guard extend a duplication that base already had five ways, and following the existing shape is the right call for this diff, but it is the obvious candidate if a sixth element ever appears)

8. Commit Hygiene — ✅ (six commits, each one logical change, largest well under a few hundred lines; no wip/fixup!/squash!/address review noise survived the force-push; no #N, owner/repo#N or closing keyword in any subject or body — checked across all six, with the Fix #2774 / Fix #2775 references in the PR body where they belong; no AI-authorship trailer on any commit; five fix: prefixes each describe a genuine defect and the sixth, data-lake: record which variables and compound variables the user created, correctly avoids fix: for the new mechanism; the two rewritten commits split cleanly — 17e846bc builds the ownership record, c990abf3 consumes it on the page — so the second is not a fixup of the first; and the compound-exclusion commit e0778a9c is still ordered first, so no intermediate state offers a compound as a write target)

9. Tests — ✅ (9.1 addressed; three focused unit files covering the persisted-value restore and delete behaviour, the pre-flag read defaults on both storage keys, the stamping of stored and new transforming functions with the flags reaching the data lake variable the page actually reads, and the edit/delete matrix including the unmarked-means-Cockpit's default; each of the three files now passes standalone, and the assertions are on observable outcomes — what is in storage, what canUserDeleteDataLakeVariable returns — rather than on call counts)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README note is owed; both added interface fields, all three utils-data-lake.ts helpers, ensureCockpitTransformingFunction, isCompoundDataLakeVariable, TransformingFunctionFlags and the new flags parameter on createTransformingFunction each carry a typed JSDoc with non-empty text, satisfying jsdoc/require-param and jsdoc/require-returns; the reworded JSDoc above allowUserToChangeValue? is the one comment touched without its own line changing, and it is permitted because the flag's meaning is precisely what this PR changes; the PR-body gap that was 0.1 is closed)

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

[rafaellehmkuhl]

Review follow-up — round 10

Round 9 raised 2, both this round. One addressed, one declined. Branch force-pushed: c990abf → cba30c6, same six commits, 17e846bc rewritten.

Done

• src/libs/vehicle/ardupilot/ardusub.ts, src/libs/vehicle/mavlink/vehicle.ts, src/libs/joystick/protocols/predefined-resources.ts (3.2 — four re-wrapped hunks with no change in them): all four collapsed back to their base text. ardusub.ts is now byte-identical to base and out of the pull request entirely, taking the changed-files count from 21 to 20. The diagnosis was exact, including that yarn lint:fix would not undo it — Prettier keeps an object literal multi-line once there is a newline after the {, so these had to be collapsed by hand.

Won't change (with reasoning)

• 1.9 — a user's own non-persistent variable can be relabelled as Cockpit's. Accurate, reachable, and declined on purpose. For an unflagged widget snapshot the information simply is not there: a replayed telemetry variable and a replayed non-persistent user variable are the same bytes, so any rule has to guess. The two guesses are 1.8 and 1.9, and this PR takes the one where nothing the app depends on can be deleted by mistake.

  The remedy the finding suggests — dropping the ownership fields before recreating — lands on that same guess and so leaves 1.9 where it is; carrying them from getDataLakeVariableInfo cannot help either, since the whole reason registerCockpitActions is recreating is that no variable with that id is registered.

  The real fix is that InputElementConfig.vue:284 binds the entire variable object into the element's options instead of its id. That is the root cause of both findings and of the snapshot going stale in general, but changing it is a widget-options schema change with a migration, which does not belong in this PR.

  Scope, for the record: it needs a profile that predates the flag and a user who deliberately unchecked "Persist variable between boots". Snapshots written from now on carry systemOwned: false, because the selector binds the live registry object and both user-facing creators set it. The PR body now states the case, which is what the finding asked for as its alternative remedy.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 10)

1 open — one minor, disputed by the author and awaiting a maintainer's call — and 20 closed across rounds 1–10.

The pull request gives Cockpit a way to remember which data-lake variables it created itself, instead of guessing from whether a variable happened to set a persistence flag. A variable that says nothing about who made it is taken as Cockpit's, and the three places that create one for the user — the Data Lake dialog, the compound dialog and the custom-widget input config — say so explicitly; anything read back from a user's existing storage is treated as theirs, so nobody loses the delete button on a variable they already made. On the Data Lake page the user can freely edit and delete their own variables, can change the value (but not the name, type or persistence) of the handful the app deliberately exposes for tuning, and can do neither to the rest. Separately it stops joysticks and widget controls from being pointed at computed variables, whose value is recalculated and would immediately discard anything written to it, and it fixes two value-restoring bugs so the camera zoom and focus speeds survive a reboot and the edit dialog shows a stored 0 or false instead of an empty box.

This round is small: the formatter-only re-wraps are gone and ardusub.ts has left the pull request entirely, which was finding 3.2 in full. The single open item is 1.9, which the author has declined on the record with reasoning — the code is unchanged, so it stays open as a dispute for a maintainer to settle.

What still needs attention

#	Problem	What it means	Severity	Status
1.9	A user's own non-persistent variable can be relabelled as Cockpit's	A variable the user created with "Persist variable between boots" switched off, and then pointed a widget element at, comes back after a restart marked as Cockpit's — no delete button, and the editor limited to its value. Declined on purpose, and now stated as a known limitation in the PR body.	minor	💬

Since round 9 — 1 closed, 1 disputed, comparing c990abf → cba30c6

Range. c990abf366fc0bbcad30cd81f6bd479c79d17f58 → cba30c64b1b4f44806b1a6b92e8279d54c347512. The branch was force-pushed again: the same six commit subjects are present, with the last two carrying new SHAs (79bd0bb8 in place of 17e846bc, cba30c64 in place of c990abf3).

incremental.diff is not usable as a record of what moved this round, for the same reason as last round. It is a three-dot comparison, so its baseline is the last commit the two heads share (e0778a9c) rather than c990abf, and it therefore reproduces everything the final two commits do instead of what changed between them. Three tells: it lists src/libs/utils-data-lake.ts as +32/-0 as though the helpers were new this round; it shows src/tests/libs/actions/data-lake.test.ts as modified where pr.diff shows it as a new file; and it contains no trace of src/libs/vehicle/ardupilot/ardusub.ts at all, even though that file leaving the pull request is the single most significant thing that happened this round. Every status judgement below, and the whole new-finding sweep, was made against pr.diff and the base checkout.

Status transitions — 1 closed, 1 disputed.

• 3.2 ✅ Addressed. All four re-wrapped hunks were collapsed back to their base text, and each was checked against the base checkout rather than against the claim that it was done:
  • src/libs/vehicle/ardupilot/ardusub.ts — gone from pr.diff entirely. Base line 87 is the one-line createDataLakeVariable({ id: aliasId, name: \Celsius Temperature (Probably)`, type: 'number' }), and pr.jsonnow reportschangedFiles: 20` where round 9 had 21. The file is out of the pull request, out of its changed-files list and out of the blame detour.
  • src/libs/vehicle/mavlink/vehicle.ts — base line 1611 is the one-line createDataLakeVariable({ id: oldVariablePath, … }), and the diff's only remaining hunks in that file are the import swap and the ensureCockpitTransformingFunction call, both of which carry real changes.
  • src/libs/joystick/protocols/predefined-resources.ts — base lines 44 and 159 are both the one-line const commonVariableConfig = { type: 'number' as DataLakeVariableType, allowUserToChangeValue: true }. Line 44 now appears in the diff as a context line under the genuinely-new speedVariableConfig, and line 159 is outside every hunk.
• 1.9 💬 Disputed. The code is unchanged here, and the author has declined the finding deliberately rather than overlooked it, so it stays open. Their three load-bearing claims were checked against the code, not taken:
  • The remedies the finding suggested do not work. Correct, and this is a correction to round 9's text rather than the author's. registerCockpitActions (src/components/MiniWidgetInstantiator.vue:103-115) recreates only when getDataLakeVariableInfo(id) !== undefined is false, so there is by construction nothing in the registry to carry flags from; and dropping the ownership fields before recreating leaves systemOwned absent, which isSystemOwnedDataLakeVariable's !== false reads as Cockpit's — landing on exactly the behaviour the finding complained about. Round 9 named two code fixes and neither of them fixes anything.
  • The real fix is that InputElementConfig.vue:284 binds the whole variable object rather than its id, and changing it needs a widget-options migration. Verified as the root cause, and the scope call is a reasonable one — AGENTS.md treats automatic rewrites of user data as a last resort, so a schema change with a migration is not something to bolt onto this PR.
  • "Copies written from now on carry the flag, since the selector binds the live registry object." Verified. availableDataLakeVariables is getAllDataLakeVariablesInfo() (InputElementConfig.vue:460-462), the live registry, and both user-facing creators put systemOwned: false into the object that lands there (DataLakeVariableDialog.vue:295, InputElementConfig.vue:523). So the exposure needs a profile that predates the flag.
  • The PR body now carries a "Known limitation" paragraph describing the case, which is the second of the two remedies round 9 named. Verified in pr.json, not against a claim about it. That closes the "currently unstated" half of the complaint but not the behaviour, and a body paragraph is not a code change — so this is disputed and open, not addressed. It is a good candidate for a maintainer to accept: the failure direction is the safe one, the trigger is narrow, and the honest fix lives in another PR.

The 19 entries carried in already closed stay closed. Nothing in this round's diff touches the mechanism any of them was verified against — the only code change is whitespace being un-done — so they were re-confirmed by checking that the hunks backing them are byte-identical to round 9, rather than re-traced from scratch.

Resolutions. resolutions.json is [] — no /resolve has been issued on this PR, so there is nothing to apply and no id was submitted that the ledger does not know.

Votes. decisions.json is []. The accept that settled 6.3 last round is correctly absent: a verdict already applied does not come back, and 6.3 stays resolved in the ledger on that vote. No vote is currently open, and none is pending on anything. If a maintainer agrees with the author on 1.9, a reaction on its decision comment or a /resolve 1.9 will close it.

Discussion since round 9. Two comments, both from rafaellehmkuhl: a bare /review (a command, ignored as noise) and a "Review follow-up — round 10" note. The note is a claim about the code, so its assertions were checked rather than accepted; all of them held, and they are recorded against 3.2 and 1.9 above. One is worth naming on its own because it credits the review with more than it earned: "The diagnosis was exact, including that yarn lint:fix would not undo it." The diagnosis of the re-wraps was exact; the diagnosis of the remedy for 1.9, in the very same round, was not.

Raised this round. Nothing. The full sweep over pr.diff — not over the increment — was re-run across all twelve sections and turned up no new finding.

Change map — what was established before judging

Claims.

• Symptom: ownership was inferred from persistent != null, so internal variables that set persistent: false got full edit and delete. Verified. Base src/views/ToolsDataLakeView.vue:436 reads …?.persistent != null, and src/libs/vehicle/mavlink/vehicle.ts and src/stores/omniscientLogger.ts create internal variables with an explicit persistent: false.
• Symptom: every compound variable got both actions. Verified. Base getVariableSource returned 'Compound' (base line 323), the pencil beside a compound was unguarded (base line 117 region), and the delete v-if was isCompoundVariable(item.id) || isUserDefinedVariable(item.id) at base line 125.
• Symptom: the camera zoom/focus speeds were unreachable and reset to 3 every boot. Verified. Base commonVariableConfig (predefined-resources.ts:44) omits persistValue, and base createDataLakeVariable (data-lake.ts:80) writes initialValue over storage unconditionally.
• Mechanism: systemOwned on DataLakeVariable and TransformingFunction; absent means Cockpit's, and the three user-facing creators set it false. Verified in src/types/data-lake.ts, src/libs/actions/data-lake-transformations.ts, and enforced by canUserChangeDataLakeVariable / canUserDeleteDataLakeVariable in src/libs/utils-data-lake.ts.
• Mechanism: editing one of Cockpit's plain variables opens the dialog limited to the value field; a Cockpit compound stays fully editable by design. Verified, and the PR body says both halves.
• Mechanism: the compound-exclusion commit lands first so no intermediate commit offers a compound as a write target. Verified. e0778a9c still precedes 79bd0bb8 and cba30c64, which are what give compounds allowUserToChangeValue: true.
• New this round: ardusub.ts is byte-identical to base and out of the PR. Verified against the base checkout and against pr.json's file list.

Failure site. Unchanged: isUserDefinedVariable in src/views/ToolsDataLakeView.vue and the three v-ifs that consumed it, plus createDataLakeVariable in src/libs/actions/data-lake.ts, which set dataLakeVariableData[variable.id] = initialValue with no regard for what loadPersistentVariables() had restored. Both are in the diff, and each is fixed at its single consumer rather than at the call sites. Every base reference to the removed isUserDefinedVariable / editUserDefinedVariable (lines 117, 122, 125, 326, 388, 390, 394, 420, 436) is accounted for in the diff, so nothing dangles.

Entry points.

Function	Reached from	Frequency
createDataLakeVariable (data-lake.ts)	setupPredefinedLakeAndActionResources() from src/main.ts; MAVLink addToDataLake; setupAllTransformingFunctionsVariables; widget mount	one-shot at boot, then per new variable
loadPersistentVariables / loadTransformingFunctions	module import, before any component mounts	once per app start
getPersistentValues / savePersistentValues / deleteDataLakeVariable	createDataLakeVariable, setDataLakeVariableData, the bin button	per value write on a persistValue variable
ensureCockpitTransformingFunction	setupMavlinkCameraResources / setupJoystickAxesResources (boot); MAVLinkVehicle on vehicle connect	one-shot at boot / on connect
ensureCoordinateFunction (poi-data-lake.ts)	syncPoiCoordinateVariables from usePointsOfInterest	when a map first mounts, then per POI edit
isCompoundDataLakeVariable	isInput in the five custom widget elements; filteredAndSortedAxisActions; isCompoundVariable in the table, now twice per row	per render of the element / per rendered table row
canUserChangeDataLakeVariable, canUserDeleteDataLakeVariable, isSystemOwnedDataLakeVariable	the table's v-ifs and getVariableSource; valueOnlyEditMode in DataLakeVariableDialog	per rendered table row / per dialog open
editVariable, deleteVariable, editCompoundVariable, filteredVariables	pencil / bin click, and the search box	per user action
saveVariable (DataLakeVariableDialog), saveTransformingFunction (TransformingFunctionDialog), saveOrUpdateParameter (InputElementConfig)	the Save button in each dialog	per user action
registerCockpitActions (MiniWidgetInstantiator)	onMounted	one-shot per widget instance

Nothing changed is unreached. loadPersistentVariables() and loadTransformingFunctions() are both called at module scope, so they complete before registerCockpitActions can run for any widget — which is why a stored variable is never clobbered by a widget snapshot, and why finding 1.9 needs a variable that was not stored to reach it.

Invariants. Every variable is Cockpit's unless it says otherwise, and only the user-facing creators say otherwise. systemOwned: false appears at exactly three producers (DataLakeVariableDialog.vue:295, InputElementConfig.vue:523, and TransformingFunctionDialog.vue:231 via createTransformingFunction's flags argument). Three consequences follow, each traced:

• Storage has to read the other way, or upgrading would confiscate variables. Both load boundaries default to the user (data-lake.ts { systemOwned: false, ...variable }, data-lake-transformations.ts { systemOwned: false, ...withTrimmedId(func) }). For the plain-variable key that is provable: savePersistentVariables filters on variable?.persistent, and only the two user-facing creators ever set persistent: true. For the compound key it is not provable, so Cockpit reclaims its own through ensureCockpitTransformingFunction.
• The reclaim reaches what the page actually reads. ensureCockpitTransformingFunction writes the flags onto the function, but the page gates on the data lake variable. Re-traced end to end: Object.assign(stored, flags) → saveTransformingFunctions() → updateTransformingFunctionListeners() → setupAllTransformingFunctionsVariables(), which does createDataLakeVariable({ ...func }) and so carries both flags across. On the following boot loadTransformingFunctions reads the stamped entry and the same chain reapplies it. The third test in data-lake-transformations.test.ts pins this.
• Reclaiming is late for two of them. The six axis outputs and the two camera compounds are reclaimed at boot. ardupilotSystemId is reclaimed only when a vehicle connects, and the POI coordinate functions only when syncPoiCoordinateVariables first runs. Until then they read as the user's, with a bin. Deleting one during that window is undone by the next ensure call, so it self-heals, and master offered edit and delete on those same rows permanently — a shrinking of an existing hole, not a new one. Worth knowing; not a finding.

The three remaining createTransformingFunction call sites all pass explicit flags ({ systemOwned: false } from the dialog, { systemOwned: true } from POI), so no path leaves a freshly created function to be re-read as something else on the next boot — which is what the comment at poi-data-lake.ts:66 guards against.

1. Correctness & Implementation Bugs — 1 finding

1.9 minor — the inverted default leaves the user's non-persistent variables exposed at the same producer 1.8 named. (raised round 9, disputed by the author; reprinted in full so this comment stands on its own)

Closed finding 1.8 was about a widget snapshot replaying a telemetry variable as the user's. The inversion fixes that and opens the mirror case at the same spot, for the smaller set of variables that storage cannot vouch for.

src/components/InputElementConfig.vue:284-292 binds the whole variable object into the element's options, not just its id:

<option v-for="variable in availableDataLakeVariables" :key="variable.name" :value="variable">

and MiniWidgetInstantiator.vue:103-115 replays that stored object at mount when no variable with that id is registered. The reachable chain, all of it on master today:

1. The user opens the Data Lake page, adds a variable, and unchecks "Persist variable between boots". DataLakeVariableDialog creates it with persistent: false and allowUserToChangeValue: true.
2. They add a Slider (or Dial, Switch, Dropdown, Checkbox) to a custom widget and pick that variable from the selector above. The whole object — no systemOwned, because this profile predates the flag — is saved into the widget's options.
3. They restart. loadPersistentVariables does not restore the variable, because savePersistentVariables only ever stored persistent ones. registerCockpitActions recreates it from the snapshot instead, unflagged.
4. On the Data Lake page it is now Cockpit internal: canUserDeleteDataLakeVariable is false, so the bin is gone, and the pencil that remains (allowUserToChangeValue is in the snapshot) opens the value-only dialog. On master the same row read User defined with a full editor and a bin, because persistent: false != null is true.

What is not affected bounds this tightly: a variable created with the checkbox left at its default is in cockpit-persistent-data-lake-variables, is restored at module import with systemOwned: false, and the existence guard in registerCockpitActions then stops the snapshot from touching it. So this needs a user who deliberately turned persistence off, and a profile written before this PR.

Correction to round 9. The two code remedies this finding originally suggested were both wrong, and the author is right to say so. Carrying the flags from getDataLakeVariableInfo cannot work, because registerCockpitActions only runs when that lookup returned undefined; and stripping the ownership fields before recreating leaves systemOwned absent, which is precisely what the new default reads as Cockpit's. The real fix is to store the variable's id in the element's options instead of a copy of the whole object — which is also what stops the snapshot going stale in general — and that is a widget-options schema change needing a migration, which is a fair thing to keep out of this PR.

Author's position. Declined on purpose, with the reasoning that for an unflagged snapshot the information genuinely is not there — a replayed telemetry variable and a replayed non-persistent user variable are the same bytes — so any rule has to guess, and this PR takes the guess where nothing the app depends on can be deleted by mistake. The PR body now carries a "Known limitation" paragraph saying so, which is the alternative remedy this finding asked for. That is a coherent answer and the finding is left open only because a body paragraph is not a code change; it is for a maintainer, not for this review, to close it.

Consequence. A variable the user created with "Persist variable between boots" switched off, and then pointed a widget element at from a pre-existing profile, comes back after a restart marked as Cockpit's: no delete button, and the editor limited to its value.

2. Persistence & User Data — inventory, no findings

Key	Backend	What happened
cockpit-persistent-data-lake-variables	machine-local (settings-management.ts)	Reshaped by addition: stored DataLakeVariable objects may now carry systemOwned. Read back through { systemOwned: false, ...variable }, so an old record lands in the "user's own" bucket.
cockpit-persistent-data-lake-values	machine-local (settings-management.ts)	Write path changed, shape unchanged. savePersistentValues merges rather than rebuilding (closed finding 1.3), deleteDataLakeVariable removes only its own key, and createDataLakeVariable reads the stored value back through getPersistentValues(). Two new entries appear here: camera-zoom-speed and camera-focus-speed, from persistValue: true.
cockpit-transforming-functions	vehicle-synced (useBlueOsStorage)	Reshaped by addition: systemOwned and allowUserToChangeValue. Read back through { systemOwned: false, ...withTrimmedId(func) }; ensureCockpitTransformingFunction then writes the flags onto Cockpit's own in place via Object.assign(stored, flags) + saveTransformingFunctions().

Judgement on each:

• Both machine-local keys hold machine-local things (a user's own variable definitions and their last values), and both are cockpit--prefixed. The transforming-functions key being vehicle-synced is right: an expression is about the vehicle, not the topside computer. No machine-specific value (device path, COM port, window geometry) is synced by any of this.
• Two automatic in-place writes, both worth being explicit about rather than passing over, since AGENTS.md treats automatic user-data rewrites as a last resort:
  • ensureCockpitTransformingFunction stamps already-stored functions. It adds two booleans to functions the app defines by a fixed id list, leaves expression, name, type and description untouched (verified: flags contains only the two fields), early-returns when they already match, and converges after one boot. poi-data-lake.ts's ensureCoordinateFunction does the same for POI functions, once, via updateTransformingFunction. Idempotent and non-destructive. saveTransformingFunctions still round-trips the unusable entries it holds aside, so the vehicle-synced key loses nothing.
  • loadPersistentVariables writes systemOwned: false into the in-memory record, and savePersistentVariables stores whole info objects — so the next time any persistent variable is created or deleted, the flag is written back onto every pre-flag entry. The value written is identical to what the read default computes, so it is a no-op in meaning and re-running it cannot diverge. Not a migration in any sense that needs a versioned key, but it does mean the key stops looking untouched after the first save.
• Nobody is stranded on an old default: the two new persistValue variables keep their existing 3 until the user changes them, and an unstamped stored variable resolves to "the user's own", which is the behaviour the user already had.

Sections with nothing to report (10)

0. Summary — ✅ (the PR body's account matches the code on every point checked, including the plain/compound split that was finding 0.1; the new "Known limitation" paragraph added this round is accurate on all three of its factual claims — the whole-object binding at InputElementConfig.vue:284, the replay at MiniWidgetInstantiator.vue:103-115, and new copies carrying the flag because the selector binds the live registry — and it is honest that both readings of an unflagged copy are guesses; the one thing the body still does not mention, the reclaim window on ardupilotSystemId and the POI functions, is recorded in the change map)

3. AGENTS.md Adherence — ✅ (3.2 addressed in full this round; the diff is now scoped to what the PR is for, with ardusub.ts gone and no hunk left anywhere that only re-wraps existing text — all four sites were compared against the base checkout; conventional-commit prefixes throughout, yarn not npm, no new dependency, no .env, no file created that the change did not need)

4. Security — ✅ (no new dependency and no package.json change, no network call, no eval/Function/v-html added — evaluateDataLakeExpression is pre-existing and untouched — no environment variable or credential, nothing under scripts/, .github/ or src/electron/; the only encoded-looking additions are the MAVLink axis expressions in predefined-resources.ts, which are the existing template strings unchanged; and nothing in the diff, the commit messages, the PR body or the two new comments contains text addressed to this reviewer or anything resembling an injected instruction)

5. Performance — ✅ (the per-row helpers are object lookups, and isCompoundDataLakeVariable is an Array.some over a few dozen functions; the compound-marker column adds one more such call per rendered table row, which is a table the user opens deliberately, not a telemetry path; nothing new runs on mavlink:onIncomingMessage or dataLake:setVariable; getPersistentValues() parses the stored object once per createDataLakeVariable on a persistValue variable, which is boot-time and a handful of variables; no listener, interval or watcher is added, so nothing needs a new teardown)

6. UI / UX — ✅ (6.3 settled by vote, 6.2/6.4/6.5 addressed; the compound marker is a non-interactive indicator rather than a control, so it owes no label or focus handling, and it sits in a fixed w-[18px] slot that is rendered whether or not the icon is, keeping names aligned after the 390px → 364px shrink; searching "compound" still finds the computed rows through the 'compound'.startsWith(query) term; both refusal paths — editCompoundVariable and deleteVariable — tell the user with a snackbar rather than failing silently, and every existing logUserAction survived the rename from editUserDefinedVariable to editVariable)

7. Code Quality & Style — ✅ (complexity-report.json for head cba30c64 reports 458 functions measured across 20 changed files with triggeredCount 0 and truncated false, so nothing tripped the complexity or depth thresholds; added lines stay inside max-len 180, every added arrow function has an explicit return type, the new imports sit in simple-import-sort order in all consumers, and no any is introduced — TransformingFunctionFlags is a Pick of the interface it flags; { ...props.editFunction, ...otherProps } in TransformingFunctionDialog was re-traced against defaultValues, which carries exactly id, name, type, expression and description, so it cannot overwrite the provenance flags with undefined; the five copies of the new two-line isInput guard extend a duplication that base already had five ways, and following the existing shape is the right call for this diff, but it is the obvious candidate if a sixth element ever appears)

8. Commit Hygiene — ✅ (six commits, each one logical change, largest well under a few hundred lines; no wip/fixup!/squash!/address review noise survived either force-push; no #N, owner/repo#N or closing keyword in any subject or body — checked across all six, with the Fix #2774 / Fix #2775 references in the PR body where they belong; no AI-authorship trailer on any commit; five fix: prefixes each describe a genuine defect and the sixth, data-lake: record which variables and compound variables the user created, correctly avoids fix: for the new mechanism; 79bd0bb8 builds the ownership record and cba30c64 consumes it on the page, so the second is not a fixup of the first; and the compound-exclusion commit e0778a9c is still ordered first, so no intermediate state offers a compound as a write target)

9. Tests — ✅ (9.1 addressed; three focused unit files covering the persisted-value restore and delete behaviour, the pre-flag read defaults on both storage keys, the stamping of stored and new transforming functions with the flags reaching the data lake variable the page actually reads, and the edit/delete matrix including the unmarked-means-Cockpit's default; each of the three files passes standalone by inspection of its own setup, and the assertions are on observable outcomes — what is in storage, what canUserDeleteDataLakeVariable returns — rather than on call counts; the tests were read, not executed)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so no README note is owed; both added interface fields, all three utils-data-lake.ts helpers, ensureCockpitTransformingFunction, isCompoundDataLakeVariable, TransformingFunctionFlags and the new flags parameter on createTransformingFunction each carry a typed JSDoc with non-empty text, satisfying jsdoc/require-param and jsdoc/require-returns; the reworded JSDoc above allowUserToChangeValue? is the one comment touched without its own line changing, and it is permitted because the flag's meaning is precisely what this PR changes; the JSDoc above the renamed editVariable was updated with it)

11. Nitpicks — ✅ (nothing left; the two nits still standing at round 9 were 3.2's re-wraps, now collapsed, and the rest closed in earlier rounds)

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

[github-actions]

🙋 Decision needed — 1.9

A user's non-persistent variable replayed from a widget snapshot is read as Cockpit's after the default inversion

The author's argument: For an unflagged widget snapshot the information simply is not there, since a replayed telemetry variable and a replayed non-persistent user variable are the same bytes, so any rule has to guess and this PR deliberately takes the guess where nothing the app depends on can be deleted by mistake, states the case as a known limitation in the PR body, and leaves the real fix of storing only the variable id in the element's options to a change that can carry the widget-options migration it needs.

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]

Review follow-up — round 11

Round 10 left nothing to implement — 3.2 closed in full and 1.9 is the standing dispute, which now carries a maintainer 👍. What follows is presentation work I picked up after reading the round-10 body, not a response to a finding. Branch force-pushed: cba30c6 → 13951b0, same six commits, cba30c64 rewritten.

Done

• src/views/ToolsDataLakeView.vue — the compound icon moved from beside the name to the right of the Type value, which reads better against the number/string/boolean it qualifies. The Name column goes back to its original 390px, the Type column widens 70px → 92px to hold the 16px slot, and the slot stays reserved on every row so the column does not go ragged. Same icon, same tooltip.

• PR description — three corrections, all found by re-reading it against the head rather than raised by review:

  • The "Known limitation" paragraph had been inserted mid-way through the ownership explanation, splitting the storage paragraph from the compound one. Moved to the end of that block.

  • The line about the Source column said it stopped mixing ownership with computedness but not where the distinction went, so it read as if the information had been dropped. It now names the icon and the restored search.

  • Added "One window this does not close": ardupilotSystemId and the POI coordinate functions are only recorded as Cockpit's when the routines that create them run, which is at vehicle instantiation and at map mount. On the first boot after upgrading they therefore read as User defined, with a delete button, until that moment. The record persists once written, so every later boot is correct from load, and neither is a regression against master, where both are editable and deletable with no window at all. Closing it means moving those two calls to bootstrap, which is a change of its own.

    This is stated because the pull request closes Several internal variables are being treated as "user defined" #2775 and that issue names ArduPilot System ID directly. Round 9's follow-up carried the correction; the description did not, and a reviewer reads the description.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 11)

1 open (1 nit) and 21 closed.

Cockpit now records who created each data-lake variable instead of guessing from whether the variable was marked persistent. The Data Lake page uses that record to decide which rows get an edit button and which get a delete button: the user can delete only their own variables, and can edit Cockpit's only where Cockpit says the value is theirs to set — which is what finally makes the camera zoom and focus speeds adjustable. Those two speeds are also remembered between boots now, and values that were supposed to be remembered are actually restored at startup instead of being overwritten by their defaults. Separately, a variable whose value is computed from an expression can no longer be picked as the target of a joystick axis or a widget control, since anything written to it is recalculated away a moment later.

What still needs attention

#	Problem	What it means	Severity	Status
8.1	Commit message names the wrong column	Anyone reading the merged history is told the "computed" marker sits next to the variable's name, when the same commit puts it next to its type.	nit	❌

Since round 10 — 1 settled, comparing cba30c6 → 13951b0

Range. cba30c64b1b4f44806b1a6b92e8279d54c347512 → 13951b03babfd99f6c1cf481461b69b2a1f91e65. The branch was force-pushed and the previous head commit was rewritten in place, so incremental.diff compares from the commit before it (79bd0bb) and replays the whole of the final commit, including everything that was already reviewed in round 10 — the value-only dialog, utils-data-lake.ts, the ownership gating. It is unreliable as a picture of what moved this round, so every status transition below was worked out against pr.diff and the base checkout instead.

Findings that changed status.

• 1.9 — ☑️ Resolved. A maintainer accepted the author's argument by voting on its decision comment (1 for, 0 against, checked against the ledger): rafaellehmkuhl. The argument accepted was that an unflagged widget-element copy cannot be told apart from a replayed telemetry variable, so the reading has to be a guess either way, and the PR takes the guess where nothing the app depends on can be deleted by mistake, states it as a known limitation in the PR body, and leaves the real fix — storing only the variable id in the element's options — to a change that can carry the widget-options migration it needs.

No /resolve commands have been issued on this PR, so nothing else was settled by command.

Discussion since round 10. rafaellehmkuhl posted a follow-up describing presentation work not asked for by any finding (comment). Checked against the code rather than taken as read:

• The compound icon did move out of the Name cell and into the Type cell, and the 16px slot holding it is rendered on every row with the v-if on the tooltip inside it, so the column cannot go ragged (src/views/ToolsDataLakeView.vue, the <td> rendering item.type). The Name column is no longer in the diff at all, which is what "back to its original 390px" means in practice, and the Type header goes 70px → 92px to hold the slot and its gap. Verified.
• The three PR-description corrections are in the body of pr.json, and the sentence that used to read "beside the name" now reads "beside the type". Verified — with the exception of the commit message carrying the same sentence, which is finding 8.1 below.
• The new "One window this does not close" paragraph is accurate: ardupilotSystemId is stamped from MAVLinkVehicle's setup (src/libs/vehicle/mavlink/vehicle.ts:1565), which runs at vehicle instantiation, and the POI coordinate functions from syncPoiCoordinateVariables (src/libs/poi/poi-data-lake.ts:74), reached through the immediate watcher in usePointsOfInterest when a POI or map consumer first mounts. Both are late relative to bootstrap, so the first-boot window it describes is real, and neither is a regression against master.

The bare /review comment is a command and was treated as noise.

Change map — what was established before judging

Claims.

• "Ownership was inferred from persistent != null." Verified — base isUserDefinedVariable at src/views/ToolsDataLakeView.vue:437 is exactly availableDataLakeVariables.value.find((v) => v.id === id)?.persistent != null, and src/stores/mainVehicle.ts:892 and src/libs/vehicle/mavlink/vehicle.ts:1562 create their variables with an explicit persistent: false, which satisfies it.
• "For compound variables the page could not guess at all, so every compound got both actions." Verified — the base template gates the compound pencil on isCompoundVariable(item.id) alone (pr.diff hunk at ToolsDataLakeView.vue:106), with no ownership test anywhere in the row.
• "The camera zoom and focus speeds were unreachable, and reset to 3 on every boot." Verified on both halves — base predefined-resources.ts:48,53 creates them with allowUserToChangeValue: true but no persistValue, so nothing was ever stored, and base createDataLakeVariable (src/libs/actions/data-lake.ts:80) assigns dataLakeVariableData[variable.id] = initialValue unconditionally, so even a stored value would have been overwritten by the 3.
• "The joystick button picker already excluded them." Verified, but not for the reason the sentence implies: the exclusion is a list-level filter in buttonActionsToShow (src/views/ConfigurationJoystickView.vue:1027-1037) that drops every id in getAllTransformingFunctions(), not the allowUserToChangeValue test at :848. This matters, because the PR newly sets allowUserToChangeValue: true on eight Cockpit compounds; without that pre-existing filter they would have appeared in the button picker as a side effect. They do not.
• "Copies written from now on carry the flag, since the selector binds the live registry object." Verified — src/components/InputElementConfig.vue:284-292 binds :value="variable" straight from availableDataLakeVariables, and the newly-built variable at :513 now carries systemOwned: false.

Failure site. Two bugs, both with the misbehaving code in the diff. For #2775 it is isUserDefinedVariable at src/views/ToolsDataLakeView.vue:437, replaced by canUserChangeDataLakeVariable/canUserDeleteDataLakeVariable. For #2774 it is the unconditional initial-value assignment at src/libs/actions/data-lake.ts:80 plus the missing persistValue on the two speed variables at src/libs/joystick/protocols/predefined-resources.ts:48,53; the diff makes the stored value win over the initial one and marks both speeds persistValue: true.

Entry points.

Function	Reached from	Frequency
loadPersistentVariables / loadTransformingFunctions	module import of data-lake.ts / data-lake-transformations.ts at bootstrap	one-shot
createDataLakeVariable (data-lake.ts:75)	setupPredefinedLakeAndActionResources (src/main.ts:96); first sighting of a MAVLink field (vehicle.ts:1599); the three creating dialogs	one-shot at boot, then per incoming message on a field's first sighting
getPersistentValues (new) / savePersistentValues	createDataLakeVariable when persistValue; setDataLakeVariableData when the written variable has persistValue; updateDataLakeVariableInfo	per user action (no telemetry variable sets persistValue)
deleteDataLakeVariable	the Data Lake bin button; deleteAllTransformingFunctionsVariables on every function save	per user action
ensureCockpitTransformingFunction (new)	setupPredefinedLakeAndActionResources (src/main.ts:96); MAVLinkVehicle setup (vehicle.ts:1565)	one-shot per boot; once per vehicle instantiation
createTransformingFunction	the compound dialog; ensureCockpitTransformingFunction; ensureCoordinateFunction	per user action / one-shot
isCompoundDataLakeVariable (new)	isInput in the five widget input elements; filteredAndSortedAxisActions; three v-ifs per Data Lake row	per element render; per user action on the two config pages (10 rows a page)
isSystemOwnedDataLakeVariable, canUserChangeDataLakeVariable, canUserDeleteDataLakeVariable (new)	Data Lake rows and handlers; valueOnlyEditMode in the variable dialog	per Data Lake table render; per user action
getVariableSource, filteredVariables, editVariable, deleteVariable, editCompoundVariable, canEditVariable, canDeleteVariable, isCompoundVariable	the Data Lake page and its buttons	per user action
valueOnlyEditMode, isValid, saveVariable, the modelValue watcher	the variable dialog opening and its Save	per user action
isInput × 5 (custom-widget-elements/*.vue)	element render, through isInteractive	per render
filteredAndSortedAxisActions	the joystick axis picker	per user action
saveOrUpdateParameter, saveTransformingFunction	Save in the element config / compound dialog	per user action
ensureCoordinateFunction	syncPoiCoordinateVariables, from the immediate watcher in usePointsOfInterest	one-shot on the first POI/map mount; per POI edit

No changed function traced to never, and none reached mavlink:addToDataLake with added per-message work.

Invariants.

1. No control may be pointed at a compound variable. Sites that can offer one as a write target, enumerated by grepping allowUserToChangeValue: the joystick button picker (ConfigurationJoystickView.vue:1027-1037, already covered on base by the transforming-function filter), the joystick axis picker (:854, covered by this PR), and the five widget input elements (covered). The Data Lake value editor is a sixth, and it is closed by construction: the plain pencil is gated on !isCompoundVariable(item.id), so a compound only ever opens the compound dialog. Guarding six producers rather than the setDataLakeVariableData chokepoint is the right call here and not the incomplete form the guidelines warn about — the chokepoint cannot distinguish a control's write from the transforming function's own, which arrives through the same call. Nothing enforces the rule for a producer added later; the enumeration is exhaustive today.
2. An absent systemOwned means Cockpit's. Every site creating a variable on the user's behalf must say otherwise: the variable dialog (DataLakeVariableDialog.vue, systemOwned: false), the element config (InputElementConfig.vue:522), and the compound dialog (TransformingFunctionDialog.vue, { systemOwned: false }). All three are covered. Of the remaining createDataLakeVariable call sites — generic-websocket.ts:148, gnss.ts:365, omniscientLogger.ts (×7), mainVehicle.ts, ardusub.ts:87, vehicle.ts, predefined-resources.ts — every one is Cockpit's own and correctly says nothing. The one replay path that cannot know, MiniWidgetInstantiator.vue:110, was finding 1.9 and is now settled by vote.
3. A stored Cockpit compound is reclaimed by whichever setup routine owns it. Three owners: setupPredefinedLakeAndActionResources at bootstrap (camera zoom/focus and the six axis outputs), MAVLinkVehicle at vehicle instantiation (ardupilotSystemId), and the POI sync at first map mount. The last two run late, which is the first-boot window the PR body now documents.

2. Persistence & User Data — inventory, no findings

Key	Backend	What happened
cockpit-persistent-data-lake-values	settings-management.ts — stored locally and queued to the vehicle by the settings manager	Not reshaped. The write path changes from rebuild to merge, deletion becomes a targeted key removal, creation now prefers a stored value over the initial one, and two new entries (camera-zoom-speed, camera-focus-speed) start being written.
cockpit-persistent-data-lake-variables	same	Shape unchanged. Entries written from now on carry systemOwned: false; entries read back are given systemOwned: false at load, as a read-time default rather than a stored rewrite.
cockpit-transforming-functions	same	Two optional fields added (systemOwned, allowUserToChangeValue). Existing entries are stamped systemOwned: true in place by ensureCockpitTransformingFunction when a setup routine claims them.

Judging each:

• All three keys pre-date the PR, start with cockpit-, and keep their backend. Nothing machine-specific enters them: the values now stored are a camera zoom speed and a camera focus speed, which belong to the vehicle rather than to the topside computer, so carrying them across a vehicle's operators is the right behaviour and no automatic action is taken on a synced value.
• The in-place stamp on cockpit-transforming-functions is an automatic rewrite of stored user data, which is a last resort by the guidelines, and it clears the bar: it assigns two booleans and never touches expression, name or type; the early return above Object.assign(stored, flags) makes a second run a no-op; and src/tests/libs/actions/data-lake-transformations.test.ts asserts that a stored expression of '1' survives a call carrying a different default. A versioned key would not help here, because what is being recorded is ownership of the entries already in that key.
• The merge in savePersistentValues reads from storage before writing, but then overwrites every registered persistValue variable's entry from memory. The only entries it carries over untouched are ones no variable in the session owns — precisely the not-yet-registered ones the merge exists to protect. That is what keeps the read-modify-write sound against settingsManager.setKeyValue's 100 ms debounce, during which the settings cache does not yet hold a pending write.
• One asymmetry the merge introduces, checked and judged harmless: switching "Persist value between boots" off on a user's variable no longer drops its stored value, where the old rebuild would have on the next save. The orphaned entry is inert — createDataLakeVariable reads a saved value only when persistValue is set — and if the user switches persistence back on they get their old value back rather than the default, which is the friendlier of the two outcomes.
• Already-configured users are not stranded: the two speeds simply start being remembered from this release, keeping their current 3 until the operator changes one, which the PR body states.

8. Commit Hygiene — 1 finding

8.1 — The final commit's message describes the icon landing in a column it does not land in · nit

13951b03 ("fix: data-lake: offer edit and delete only for variables the user owns") ends its message with "Which rows are computed moves to an icon beside the name, the same one the 'Add compound variable' button uses". The commit itself puts that icon in the Type cell of src/views/ToolsDataLakeView.vue — inside the <div> that renders {{ item.type }} — and widens the Type header from 70px to 92px to make room, while the Name column is not touched at all and stays at its base 390px. The PR description was corrected this round to say "beside the type"; the commit body carrying the same sentence was not, and it is the copy that survives the merge.

The rest of the sentence checks out: mdi-function-variant is the same glyph as the "Add compound variable" button at src/views/ToolsDataLakeView.vue:35, and the 'compound'.startsWith(query) clause in filteredVariables does keep the word searchable.

Fix: on the next rebase, change "beside the name" to "beside the type" in that commit's body. Nothing else in the six commits needs touching — each is one logical change, the ownership recording lands before the gating that reads it, the compound write-target fix rides alone as its own commit, and no message references an issue number.

Sections with nothing to report (9)

1. Correctness & Implementation Bugs — ✅ (traced the flags from ensureCockpitTransformingFunction through saveTransformingFunctions → updateTransformingFunctionListeners → setupAllTransformingFunctionsVariables at data-lake-transformations.ts:225-234, which is what puts them on the backing data lake variable the page reads; the new getPersistentValues() in createDataLakeVariable cannot throw at bootstrap, since initLocalSettings always seeds localSettings[user][vehicle] at settings-management.ts:1093 before main.ts:96 runs)

3. AGENTS.md Adherence — ✅ (all 30 createDataLakeVariable/createTransformingFunction call sites accounted for against the ownership default; ardusub.ts is out of the diff again, so no formatter-only reflow is left; every new export — the three utils-data-lake predicates, isCompoundDataLakeVariable, ensureCockpitTransformingFunction — has a call site in this PR)

4. Security — ✅ (no new dependency, no network call, no eval/Function/v-html, no workflow, build-script or Electron change; the two added storage reads go through settingsManager, and the diff carries no encoded blob or non-ASCII identifier)

5. Performance — ✅ (getPersistentValues reads the in-memory cache at settings-management.ts:336 and setKeyValue debounces at 100 ms, so the merge adds no I/O per write; createDataLakeVariable only reaches it when persistValue is set, which no telemetry variable is, leaving addToDataLake untouched; isCompoundDataLakeVariable is an Array.some over a few dozen functions on a table paginated at 10 rows)

6. UI / UX — ✅ (the compound marker is a non-interactive indicator with a tooltip, matching the bare <span class="mdi …"> already at ToolsDataLakeView.vue:66; its 16px slot renders on every row so the column cannot go ragged, the Type column was widened by the slot plus its gap, and the new snackbars and logUserAction entries keep the past-tense voice)

7. Code Quality & Style — ✅ (the complexity report measured 458 functions across all 20 changed files with nothing triggered and no truncation; the new JSDoc blocks carry typed @param/@returns with no empty entries, the added import groups stay simple-import-sort ordered, and no comment whose code is unchanged was reworded)

9. Tests — ✅ (each of the three files establishes its own state, the transformations test re-claiming camera-zoom in its third case rather than leaning on the first; no existing test was removed or weakened)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so the README table is untouched; systemOwned is documented on both DataLakeVariable and TransformingFunction, and allowUserToChangeValue's doc comment was rewritten to match its widened meaning)

11. Nitpicks / Optional — ✅ (nothing beyond the commit wording reported as 8.1)

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

[rafaellehmkuhl]

Review follow-up — round 12

Round 11 left one nit and closed 1.9 on the vote. Branch force-pushed: 13951b0 → d2ee804; only the last commit's message changed, its tree is byte-identical.

Done

• d2ee804 commit message (8.1 — commit message names the wrong column): reworded to "an icon beside the type it qualifies". The sentence was written when the icon sat next to the name, and survived the move to the Type column unedited. Caught precisely: the same commit that says "name" is the one that puts it next to the type.

[rafaellehmkuhl]

/review

[github-actions]

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

0 open · 22 closed (2 of them settled by maintainer vote).

Cockpit now records who created each data-lake variable instead of guessing from whether the variable was marked persistent. The Data Lake page uses that record to decide which rows get an edit button and which get a delete button: the user can delete only their own variables, and can edit Cockpit's only where Cockpit says the value is theirs to set — which is what finally makes the camera zoom and focus speeds adjustable. Those two speeds are also remembered between boots now, and values that were supposed to be remembered are actually restored at startup instead of being overwritten by their defaults. Separately, a variable whose value is computed from an expression can no longer be picked as the target of a joystick axis or a widget control, since anything written to it is recalculated away a moment later.

What still needs attention

Nothing. Every finding raised across the twelve rounds is closed.

Since round 11 — 1 closed, comparing 13951b0 → d2ee804

Range. 13951b03babfd99f6c1cf481461b69b2a1f91e65 → d2ee804258844b96d4e65ae4cb3664da47acbbbf.

The incremental diff is not a picture of this round. The branch was force-pushed and the previous head was rewritten in place, so the compare falls back to the commit before it (79bd0bb) and replays the whole of the final commit — the value-only dialog, utils-data-lake.ts, the ownership gating — all of which was already reviewed in rounds 10 and 11. It is the same five files it replayed last round. Every judgement below was therefore made against pr.diff and the base checkout instead.

That fallback is also what makes the scope of the push checkable without the old head. Because the compare's merge base is 79bd0bb, that commit is an ancestor of both the old head and the new one, so the first five commits are the same objects in both; only the last commit could differ. Its file changes in pr.diff still match every detail recorded in round 11 — the compound icon in the Type cell, the Type header at 92px with the Name column out of the diff, the 'compound'.startsWith(query) clause in filteredVariables, the canEditVariable guard on editCompoundVariable, and the third transformations test claiming camera-zoom for itself. So the code did not move this round; the commit message did.

Findings that changed status.

• 8.1 — ✅ Addressed. The finding asked for one thing: that the final commit's body stop saying the computed-variable icon lands beside the variable's name when the same commit puts it beside its type. d2ee804's body now reads "Which rows are computed moves to an icon beside the type it qualifies, the same one the 'Add compound variable' button uses, and searching for 'compound' still finds them." Checked against pr.json's commit list, not against the follow-up describing it. The rest of that sentence was already accurate and still is.

Resolutions and decisions. resolutions.json is [] and decisions.json is [] — no /resolve has been issued on this PR and no vote is outstanding, so nothing was settled by a maintainer this round and there is no unrecognised id to report back. The two findings settled by vote in earlier rounds (6.3 and 1.9) stay closed in the ledger.

Discussion since round 11. rafaellehmkuhl posted a follow-up stating that the branch was force-pushed, that only the last commit's message changed, and that its tree is byte-identical. The reworded message is verified above; the byte-identical claim cannot be checked directly from here, since the previous head is no longer reachable, but everything that can be checked about it holds — the five earlier commits are provably the same objects, and the final commit's content matches what round 11 read. The bare /review comment is a command and was treated as noise.

Injection check. Nothing in the PR body, the diff, the commit messages or the comments contained text addressed to the reviewer.

Change map — what was established before judging

Claims.

• "Ownership was inferred from the persistence flags." Verified — the base isUserDefinedVariable at src/views/ToolsDataLakeView.vue:437 is exactly availableDataLakeVariables.value.find((v) => v.id === id)?.persistent != null, and src/stores/mainVehicle.ts:892 and src/libs/vehicle/mavlink/vehicle.ts:1562 both create their variables with an explicit persistent: false, which satisfies it.
• "For compound variables the page could not guess at all, so every compound got both actions." Verified — the base template gates the compound pencil on isCompoundVariable(item.id) alone (ToolsDataLakeView.vue:117 in the diff's before-image), with no ownership test anywhere in the row.
• "The camera zoom and focus speeds were unreachable and reset to 3 on every boot." Verified on both halves — base predefined-resources.ts:48,53 creates them with allowUserToChangeValue: true but no persistValue, so nothing was ever stored, and base createDataLakeVariable (src/libs/actions/data-lake.ts:80) assigns dataLakeVariableData[variable.id] = initialValue unconditionally, so even a stored value would have been overwritten by the 3.
• "The joystick button picker already left compounds out." Verified, though not by the mechanism the sentence implies: the exclusion is a list-level filter in buttonActionsToShow (src/views/ConfigurationJoystickView.vue:1027-1037) dropping every id in getAllTransformingFunctions(), not the allowUserToChangeValue test at :848. It matters, because the PR newly sets allowUserToChangeValue: true on eight Cockpit compounds; without that pre-existing filter they would have surfaced in the button picker as a side effect. They do not.
• "Nothing reads any of this yet" (the ownership-recording commit). Verified — 79bd0bb adds the flag, the helper and the stamping; every consumer of it lands in d2ee804.

Failure site. Two bugs, both with the misbehaving code in the diff. For the editability bug it is isUserDefinedVariable at src/views/ToolsDataLakeView.vue:437, replaced by canUserChangeDataLakeVariable/canUserDeleteDataLakeVariable. For the forgotten speeds it is the unconditional initial-value assignment at src/libs/actions/data-lake.ts:80 plus the missing persistValue on the two speed variables at src/libs/joystick/protocols/predefined-resources.ts:48,53; the diff makes a stored value win over the initial one and marks both speeds persistValue: true.

Entry points.

Function	Reached from	Frequency
loadPersistentVariables / loadTransformingFunctions	module import of data-lake.ts / data-lake-transformations.ts at bootstrap	one-shot
createDataLakeVariable (data-lake.ts:75)	setupPredefinedLakeAndActionResources (src/main.ts:96); first sighting of a MAVLink field (vehicle.ts:1599); the three creating dialogs; setupAllTransformingFunctionsVariables (data-lake-transformations.ts:232)	one-shot at boot, then per incoming message on a field's first sighting
getPersistentValues (new) / savePersistentValues	createDataLakeVariable when persistValue; setDataLakeVariableData when the written variable has persistValue (data-lake.ts:133); updateDataLakeVariableInfo	per user action — no telemetry variable sets persistValue
deleteDataLakeVariable	the Data Lake bin button; deleteAllTransformingFunctionsVariables on every function save	per user action
ensureCockpitTransformingFunction (new)	setupPredefinedLakeAndActionResources (src/main.ts:96); MAVLinkVehicle setup (vehicle.ts:1565)	one-shot per boot; once per vehicle instantiation
createTransformingFunction	the compound dialog; ensureCockpitTransformingFunction; ensureCoordinateFunction	per user action / one-shot
isCompoundDataLakeVariable (new)	isInput in the five widget input elements; filteredAndSortedAxisActions; three v-ifs per Data Lake row	per element render; per user action on the two config pages, 10 rows a page
isSystemOwnedDataLakeVariable, canUserChangeDataLakeVariable, canUserDeleteDataLakeVariable (new)	Data Lake rows and handlers; valueOnlyEditMode in the variable dialog	per Data Lake table render; per user action
getVariableSource, filteredVariables, editVariable, deleteVariable, editCompoundVariable, canEditVariable, canDeleteVariable, isCompoundVariable	the Data Lake page and its buttons	per user action
valueOnlyEditMode, isValid, saveVariable, the modelValue watcher	the variable dialog opening and its Save	per user action
isInput × 5 (custom-widget-elements/*.vue)	element render, through isInteractive	per render
filteredAndSortedAxisActions	the joystick axis picker	per user action
saveOrUpdateParameter, saveTransformingFunction	Save in the element config / compound dialog	per user action
ensureCoordinateFunction	syncPoiCoordinateVariables, from the immediate watcher in usePointsOfInterest	one-shot on the first POI/map mount; per POI edit

No changed function traced to never, and none reached mavlink:addToDataLake with added per-message work.

Invariants.

1. No control may be pointed at a compound variable. Sites that can offer one as a write target, enumerated by grepping allowUserToChangeValue across src/: the joystick button picker (ConfigurationJoystickView.vue:1027-1037, already covered on base by the transforming-function filter), the joystick axis picker (:858, covered by this PR), and the five widget input elements (all five covered). The Data Lake value editor is a sixth and is closed by construction — the plain pencil is gated on !isCompoundVariable(item.id), so a compound only ever opens the compound dialog. Guarding six producers rather than the setDataLakeVariableData chokepoint is right here rather than the incomplete form the guidelines warn about: the chokepoint cannot tell a control's write from the transforming function's own, which arrives through the same call. The enumeration is exhaustive today; nothing enforces the rule for a producer added later.
2. An absent systemOwned means Cockpit's. Every site creating a variable on the user's behalf must say otherwise: the variable dialog (DataLakeVariableDialog.vue:296, systemOwned: false), the element config (InputElementConfig.vue:523), and the compound dialog (TransformingFunctionDialog.vue:230, { systemOwned: false }). All three are covered. Of the remaining createDataLakeVariable call sites — generic-websocket.ts:148, gnss.ts:365, omniscientLogger.ts (×7), mainVehicle.ts (×3), ardusub.ts:87, vehicle.ts (×5), predefined-resources.ts — every one is Cockpit's own and correctly says nothing. The one replay path that cannot know, MiniWidgetInstantiator.vue:110, was finding 1.9 and is settled by vote.
3. A stored Cockpit compound is reclaimed by whichever setup routine owns it. Three owners: setupPredefinedLakeAndActionResources at bootstrap (camera zoom/focus and the six axis outputs), MAVLinkVehicle at vehicle instantiation (ardupilotSystemId), and the POI sync at first map mount. The last two run late, which is the first-boot window the PR body documents. The stamp reaches the variable the page reads because saveTransformingFunctions ends in updateTransformingFunctionListeners (data-lake-transformations.ts:73), which rebuilds every backing variable through setupAllTransformingFunctionsVariables (:232) by spreading the whole function object.

2. Persistence & User Data — inventory, no findings

Key	Backend	What happened
cockpit-persistent-data-lake-values	settings-management.ts — stored locally and queued to the vehicle by the settings manager	Not reshaped. The write path changes from rebuild to merge, deletion becomes a targeted key removal, creation now prefers a stored value over the initial one, and two new entries (camera-zoom-speed, camera-focus-speed) start being written.
cockpit-persistent-data-lake-variables	same	Shape unchanged. Entries written from now on carry systemOwned: false; entries read back are given systemOwned: false at load, as a read-time default rather than a stored rewrite.
cockpit-transforming-functions	same	Two optional fields added (systemOwned, allowUserToChangeValue). Existing entries are stamped systemOwned: true in place by ensureCockpitTransformingFunction when a setup routine claims them.

Judging each:

• All three keys pre-date the PR, start with cockpit-, and keep their backend. Nothing machine-specific enters them: the values now stored are a camera zoom speed and a camera focus speed, which belong to the vehicle rather than to the topside computer, so carrying them across a vehicle's operators is right and no automatic action is taken on a synced value.
• The in-place stamp on cockpit-transforming-functions is an automatic rewrite of stored user data, a last resort by the guidelines, and it clears the bar: it assigns two booleans and never touches expression, name or type; the early return above Object.assign(stored, flags) makes a second run a no-op; and src/tests/libs/actions/data-lake-transformations.test.ts asserts that a stored expression of '1' survives a call carrying a different default. A versioned key would not help, since what is being recorded is ownership of the entries already in that key.
• The merge in savePersistentValues reads storage before writing, then overwrites every registered persistValue variable's entry from memory. The only entries it carries over untouched are ones no variable in the session owns — precisely the not-yet-registered ones the merge exists to protect. That is what keeps the read-modify-write sound against settingsManager.setKeyValue's debounce.
• One asymmetry the merge introduces, checked and judged harmless: switching "Persist value between boots" off on a user's variable no longer drops its stored value, where the old rebuild would have on the next save. The orphaned entry is inert — createDataLakeVariable reads a saved value only when persistValue is set — and switching persistence back on returns the old value rather than the default, which is the friendlier outcome.
• Already-configured users are not stranded: the two speeds simply start being remembered from this release, keeping their current 3 until the operator changes one, which the PR body states.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (traced the flags from ensureCockpitTransformingFunction through saveTransformingFunctions → updateTransformingFunctionListeners → setupAllTransformingFunctionsVariables at data-lake-transformations.ts:229-238, which is what puts them on the data lake variable the page reads; TransformingFunctionDialog's newFunction holds only the five form fields (:173-179), so spreading ...props.editFunction first genuinely preserves the stamp through an edit; getPersistentValues() cannot throw at bootstrap, since initLocalSettings seeds localSettings[user][vehicle] before main.ts:96 runs)

3. AGENTS.md Adherence — ✅ (all 30 createDataLakeVariable/createTransformingFunction call sites in src/ accounted for against the ownership default; ardusub.ts is out of the diff, so no formatter-only reflow is left behind; every new export — the three utils-data-lake predicates, isCompoundDataLakeVariable, ensureCockpitTransformingFunction — has a call site in this PR)

4. Security — ✅ (no new dependency, no network call, no added eval/Function/v-html, no workflow, build-script or Electron change; the two added storage reads go through settingsManager, and the diff carries no encoded blob or non-ASCII identifier)

5. Performance — ✅ (getPersistentValues reads settingsManager's in-memory cache and setKeyValue is debounced, so the merge adds no I/O per write; createDataLakeVariable reaches it only when persistValue is set, which no telemetry variable is, leaving addToDataLake untouched; isCompoundDataLakeVariable is an Array.some over a few dozen functions, called from a table paginated at 10 rows and from five element-render computeds)

6. UI / UX — ✅ (the compound marker is a non-interactive indicator with a tooltip, matching the bare <span class="mdi …"> already at ToolsDataLakeView.vue:66; its 16px slot renders on every row so the column cannot go ragged, the Type header was widened by exactly that slot plus its gap, and the added snackbars and logUserAction entries keep the past-tense voice)

7. Code Quality & Style — ✅ (complexity-report.json measured 458 functions across all 20 changed files with nothing triggered and no truncation; settingsManager.getKeyValue<T extends SettingValue> accepts the new Record<string, string | number | boolean> argument since SettingValue includes object; the new JSDoc blocks carry typed @param/@returns with no empty entries, and no comment whose code is unchanged was reworded)

8. Commit Hygiene — ✅ (six commits, each one logical change: the ownership recording lands before the gating that reads it, the compound write-target fix rides alone, and d2ee804's body now names the Type column the commit actually touches; no message references an issue or pull request, and none is a fixup or a self-correction)

9. Tests — ✅ (each of the three added test files establishes its own state, the transformations test re-claiming camera-zoom in its third case rather than leaning on the first; no existing test was removed or weakened)

10. Documentation — ✅ (nothing here differs between Lite and Standalone, so the README table is untouched; systemOwned is documented on both DataLakeVariable and TransformingFunction, and allowUserToChangeValue's doc comment was rewritten to match its widened meaning)

11. Nitpicks / Optional — ✅ (nothing left to raise)

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

[rafaellehmkuhl]

@ArturoManzoli putting this one on #fast-track since it's needed for the 4K Cam launch.