Allow reseting individual settings

[rafaellehmkuhl]

Fix #2804

[github-actions]

Automated PR Review (Claude)

0. Summary

Verdict: IMPORTANT FIXES REQUIRED

Critical items to address: 1.1

This PR adds the ability to reset individual v2 settings in the Cockpit settings manager. It introduces a deleteKeyValue method on SettingsManager that removes a key from local storage and adds it to a one-time vehicle-sync ignore list (persisted under cockpit-settings-sync-ignore-once). A reset button is shown per-row in the settings UI, and a "Restart" button appears when resets are pending. The ignore-once mechanism prevents the vehicle from re-syncing the deleted value on the next boot, and then clears the ignore entry so subsequent syncs behave normally.

1. Correctness & Implementation Bugs

1.1 major — Reset is not permanent; the vehicle value will return after one boot.

deleteKeyValue removes the key from local storage and adds it to the ignore-once list. On the next sync (getBestSettingsBetweenLocalAndVehicle), the key is skipped and removed from the ignore list. However, the key still exists on the vehicle — pushSettingsToVehicleUpdateQueue only pushes keys that are present in the merged result; it never deletes keys from the vehicle. On the second boot (or next vehicle reconnection), the ignore list is empty, the key is absent locally but present on the vehicle, so the sync's !hasLocalSetting && hasVehicleSetting branch pulls it back from the vehicle, undoing the reset.

The user-facing dialog says "The variable was reset" with no caveat that it will reappear. Either:

• (a) the ignored key should also be deleted from the vehicle during sync (requires an API call or writing a tombstone), or
• (b) the ignore list should be persistent (not cleared after one sync) until the consumer re-creates the key with a new default, or
• (c) the dialog and tooltip should clearly communicate that the reset is temporary (only effective until the next vehicle sync), though this seems like unexpected UX.

(src/libs/settings-management.ts, sync logic around the new clearKeysFromVehicleSyncIgnoreList call)

1.2 minor — deleteKeyValue does not guard against missing intermediate objects.

The check localSettings[userId!]?.[vehicleId!]?.[key] correctly uses optional chaining, but if localSettings[userId!] itself is undefined, the subsequent delete localSettings[userId!][vehicleId!][key] would crash. In practice the guard on the line above protects this, but the code relies on the truthiness of the optional-chain result rather than an explicit existence check on the parent objects. This is fragile if getLocalSettings() ever returns an empty object for a user that doesn't exist yet. A safer pattern:

if (localSettings[userId!]?.[vehicleId!]?.[key] !== undefined) {
  delete localSettings[userId!][vehicleId!][key]

This is actually what the code does — re-reading, this is safe because ?.[vehicleId!]?.[key] returning non-undefined proves all intermediate levels exist. Withdrawing this finding — no action needed.

1.2 minor — addKeyToVehicleSyncIgnoreList uses JSON.stringify but readVehicleSyncIgnoreList uses deserialize.

JSON.stringify and deserialize (which is JSON.parse with fallback) are compatible, but this is an inconsistent pattern within the same class. Other read/write pairs in SettingsManager use the same serialisation function on both sides. Consider using JSON.stringify/JSON.parse or the project's serialize/deserialize consistently.

(src/libs/settings-management.ts, addKeyToVehicleSyncIgnoreList vs readVehicleSyncIgnoreList)

1.3 minor — Dialog-spam potential: resetSetting opens a dialog on every click without checking if one is already open.

If the user clicks the reset button on multiple settings in quick succession, each click calls showDialog(...) which may stack or replace the existing dialog. Per the guidelines (AGENTS.md: "Do not open a new dialog while a dialog of the same purpose is already open"), consider guarding against re-opens or batching into a single notification.

(src/components/configuration/CockpitSettingsManager.vue, resetSetting function)

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX

5.1 minor — Duplicated template blocks for the reset button.

The reset-button <v-tooltip> block is copy-pasted identically in two places in the template (once for the primitive-value row at ~line 182 and once for the JSON-object row at ~line 256). If the button ever needs to change (icon, tooltip text, behavior), both must be updated in sync. Consider extracting a small inline component or a shared <template> fragment.

(src/components/configuration/CockpitSettingsManager.vue)

6. Code Quality & Style

6.1 nit — restartCockpit is a trivial wrapper.

const restartCockpit = (): void => {
  reloadCockpitAndWarnUser()
}

This adds an indirection that doesn't carry its weight. @click="reloadCockpitAndWarnUser" directly in the template would be simpler.

(src/components/configuration/CockpitSettingsManager.vue)

6.2 nit — clearKeysFromVehicleSyncIgnoreList parameter could use ReadonlyArray or Set for clarity, but the current string[] is fine given the usage context. No action required.

7. Commit Hygiene — ✅

Two clean, logically separated commits: one for the backend deleteKeyValue + ignore-once mechanism, and one for the UI. Commit subjects are descriptive and appropriately typed.

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.1 nit — PR title and branch name say "reseting" — the correct spelling is "resetting" (double-t).

10.2 nit — The tooltip text "Reset this variable to its default value on the next Cockpit boot." ends with a period. Other tooltips in the same file (e.g. "Edit value") do not. Minor inconsistency.

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

[rafaellehmkuhl]

Review follow-up — round 1

Done

• src/libs/settings-management.ts (1.1 — reset not permanent): the reset is now permanent. Reset settings are deleted from the vehicle on the next sync (deletePendingResetKeysFromVehicle) so the value can't be pulled back. The pending list is only cleared after a successful vehicle write, so a failed write is retried next sync; the merge-exclusion is kept as a safety net. The pending list now stores {userId, vehicleId, key} entries (was bare keys) so a reset only deletes the setting it was performed on — the vehicle delete would otherwise target the current user/vehicle, which can differ from the selected one.
• src/components/configuration/CockpitSettingsManager.vue (1.3 — dialog spam): added a resetDialogOpen re-open guard so resetting several variables in a row doesn't stack dialogs (the reset itself still applies on every click).
• src/components/configuration/CockpitSettingsManager.vue (10.2 — tooltip period): dropped the trailing period to match sibling tooltips like "Edit value".

Won't change (with reasoning)

• 1.2 (serialization JSON.stringify vs deserialize): this matches the existing pattern in the same class — setLocalSettings writes with JSON.stringify and getLocalSettings reads with deserialize. Not an inconsistency.
• 1.2-first (missing-object guard): withdrawn by the reviewer; no action.
• 5.1 (duplicated reset-button template): the sibling edit-pencil button is duplicated the same way across the primitive and JSON rows; extracting only the reset button would be inconsistent, and refactoring both is out of scope here.
• 6.1 (restartCockpit wrapper): reloadCockpitAndWarnUser(timeout = 4000) would receive the click PointerEvent as its timeout argument if wired directly as @click="reloadCockpitAndWarnUser". The wrapper prevents that.
• 6.2: no action required (as noted).

Questions for reviewers

• 10.1 (typo "reseting"): only present in the PR title and branch name — the code/UI uses "reset". I'll fix the PR title; a branch rename isn't worth breaking the PR over. Flagging in case you disagree.

[rafaellehmkuhl]

/review

[github-actions]

Automated PR Re-review 2 (Claude)

Comparing 98b65566d97b7b9013b80df0f28706b679fbc8d8 → 023bcb65a45581aa8ab2b98d0545dc08477e078a

Previous findings status

#	Finding	Severity	Status
1.1	Reset not permanent; vehicle value returns after one boot	major	✅ Addressed
1.2	JSON.stringify vs deserialize inconsistency	minor	⚪ No longer applicable
1.3	Dialog-spam potential on resetSetting	minor	✅ Addressed
5.1	Duplicated template blocks for reset button	minor	❌ Not addressed
6.1	restartCockpit trivial wrapper	nit	⚪ No longer applicable
6.2	Parameter type suggestion	nit	⚪ No longer applicable
10.1	"reseting" typo in PR title / branch	nit	:large_yellow_circle: Partially addressed
10.2	Tooltip trailing period inconsistency	nit	✅ Addressed

Discussion since last review

@rafaellehmkuhl provided a detailed follow-up explaining the resolution of each finding:

• 1.1: Now permanent — deletePendingResetKeysFromVehicle deletes the key from the vehicle on next sync; the pending list is only cleared after a successful vehicle write and the merge-exclusion acts as a safety net. Entries now store {userId, vehicleId, key} to scope correctly. Confirmed addressed.
• 1.2 (serialization): Author notes this matches the existing setLocalSettings/getLocalSettings pattern. Accepted — marking as no longer applicable.
• 1.3: resetDialogOpen guard added. Confirmed addressed.
• 5.1 (duplicated template): Author notes the sibling edit-pencil button is duplicated the same way and extracting only the reset button would be inconsistent; refactoring both is out of scope. Fair point — keeping the finding open at minor since the duplication is real, but acknowledging the pragmatic reasoning.
• 6.1 (restartCockpit wrapper): Author correctly explains that @click="reloadCockpitAndWarnUser" would pass the PointerEvent as the timeout parameter. The wrapper is intentional and correct. Withdrawing.
• 10.1: Author will fix PR title; branch rename skipped. Reasonable.
• 10.2: Trailing period removed.

New findings

0. Summary

Verdict: MINOR SUGGESTIONS

The previous major finding (1.1 — reset not permanent) is fully addressed: deletePendingResetKeysFromVehicle now deletes keys from the vehicle on the next sync, with retry-on-failure and a merge-exclusion safety net. The dialog-spam guard and tooltip fix are also in. The remaining open items are minor template duplication (5.1, acknowledged as intentional) and one new minor finding (1.4). Overall the implementation is solid.

1. Correctness & Implementation Bugs

1.4 minor — deletePendingResetKeysFromVehicle clears the pending list even when the key was not found on the vehicle.

When changedOnVehicle is false (the key doesn't exist on the vehicle — perhaps it was already deleted or was never synced), the method skips the vehicle write but still calls this.clearEntriesFromPendingResetList(relevantEntries). This is arguably the correct behavior (no need to retry deleting something that isn't there), but it means the merge-exclusion safety net in getBestSettingsBetweenLocalAndVehicle is removed immediately. If there's a race where the vehicle actually does have the key but getValidSettingsFromVehicle returned stale data, the key could be pulled back on the next merge.

This is low-risk because the vehicle read happens synchronously before the merge in the same sync cycle, but worth noting in case the timing assumptions change.

(src/libs/settings-management.ts, deletePendingResetKeysFromVehicle, around the if (changedOnVehicle) branch)

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style — ✅

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

Round-1 items are confirmed addressed (1.1, 1.3, 10.2) or withdrawn (1.2, 6.1, 6.2). Only the items below remain, and neither needs a code change.

Won't change (with reasoning)

• 1.4 — clears pending list when the key isn't found on the vehicle: this is intentional and necessary. A changedOnVehicle === false result means the key is confirmed absent from the vehicle, so there's nothing to pull back and nothing to retry. If we didn't clear in that case, the entry would stay pending forever (the key never exists on the vehicle to delete), permanently keeping the merge-exclusion active and the "Restart (X pending)" count non-zero. The delete-read (deletePendingResetKeysFromVehicle) and the merge-read (getBestSettingsBetweenLocalAndVehicle) both hit the same BlueOS bag-of-holding within the same sync cycle with no interleaving writes from this client, so there's no stale-read window in practice; a concurrent write from another GCS in that gap is a general concurrency concern, not specific to reset.