Skip to content

Fix settings sync logic for key-value pairs that never stop updating#2818

Merged
rafaellehmkuhl merged 2 commits into
bluerobotics:masterfrom
rafaellehmkuhl:settings-syncer-configurable-debounce
Jul 3, 2026
Merged

Fix settings sync logic for key-value pairs that never stop updating#2818
rafaellehmkuhl merged 2 commits into
bluerobotics:masterfrom
rafaellehmkuhl:settings-syncer-configurable-debounce

Conversation

@rafaellehmkuhl

Copy link
Copy Markdown
Member

When a settings keep being updated, like the odometer implemented on #2676, the value is never pushed to the vehicle, since it never stops being updated and the timeout for pushing (debounce) never resets.

This PR introduces the usage of maxWait, so every few seconds (30), the value is pushed independently of it not having stalled.

It also introduces support for callers to configure both the debouncing and maxWait times.

…wait

The reset-on-every-change debounce in useBlueOsStorage never persisted a
value that kept changing faster than the debounce interval (e.g. live map
camera, in-progress mission edits), so those keys were only written to
BlueOS once the value went quiet. Clamp the debounce against a max-wait so
a pending value is forced out after at most `settingsSyncMaxWaitMs`, and
name the previously-magic debounce interval.
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Automated PR Review (Claude)

0. Summary

Verdict: MINOR SUGGESTIONS

Minor items to address: 1.1, 6.1.

This PR fixes a real bug in useBlueOsStorage where a continuously-changing value (e.g. an odometer) would keep resetting the debounce timeout and never actually persist to BlueOS. The fix introduces a firstPendingChangeEpoch timestamp to clamp the debounce against a maxWaitMs ceiling, and exposes both the debounce and max-wait as optional per-key overrides via a new BlueOsStorageOptions type. The approach is clean and minimal.

1. Correctness & Implementation Bugs

1.1 (minor) Comment references wrong variable namesrc/composables/settingsSyncer.ts (diff line 67, added code): the inline comment says "pending for settingsSyncMaxWaitMs" but the actual variable is maxWaitMs (local) / defaultSettingsSyncMaxWaitMs (module-level constant). Should read maxWaitMs to match the code it documents.

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style

6.1 (nit) JSDoc @param for options uses inline-doc stylesrc/composables/settingsSyncer.ts (diff around line 40): the added @param line is @param { BlueOsStorageOptions } [options] - Per-key overrides for the write debounce and max-wait. Per the eslint jsdoc config (typescript mode), the type should be omitted from JSDoc when TypeScript already provides it, or at a minimum the existing params in the same block already include the type braces so this is consistent — but the trailing dash-separated description style ([options] - Per-key…) diverges from the other @param lines which have no description. Not a correctness issue, just a minor inconsistency.

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

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

…nfigurable

Add an optional per-key options argument so callers can override the write
debounce and max-wait, falling back to the named default constants. Lets
high-frequency keys tune their checkpoint cadence instead of sharing one
hard-coded interval.
@rafaellehmkuhl rafaellehmkuhl force-pushed the settings-syncer-configurable-debounce branch from 1ad0785 to 984c2cf Compare July 2, 2026 03:39
@rafaellehmkuhl

Copy link
Copy Markdown
Member Author

Review follow-up — round 1

Done

  • src/composables/settingsSyncer.ts (1.1 — comment referenced wrong variable): the inline comment now says maxWaitMs instead of the renamed-away settingsSyncMaxWaitMs.

Won't change (with reasoning)

  • 6.1 — JSDoc @param for options style: keeping the type brace and the description. Omitting the type contradicts our AGENTS.md rule ("Always include the types of the @params"), and lint passes clean at --max-warnings=0 with it present. The description stays only on options because it's non-obvious; adding descriptions to key/defaultValue would be the obvious/filler JSDoc AGENTS.md asks us to avoid.

@rafaellehmkuhl

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Automated PR Re-review 2 (Claude)

Comparing 1ad078570441cddfbd94f1c5258c84e7207d8f83984c2cf008fab11c573a80f64dbbe716eb1d4dd6

Previous findings status

# Finding Severity Status
1.1 Comment references wrong variable name (settingsSyncMaxWaitMs → should be maxWaitMs) minor ✅ Addressed
6.1 JSDoc @param for options style inconsistency (dash-separated description vs bare params) nit ⚪ No longer applicable

Discussion since last review

  • @rafaellehmkuhl (comment): confirmed 1.1 is fixed — the comment now reads maxWaitMs. Pushed back on 6.1, noting that keeping the type brace satisfies the AGENTS.md rule "Always include the types of the @params" and that lint passes clean. This is a fair reading of the rule — marking 6.1 as no longer applicable.

New findings

0. Summary

Verdict: READY TO MERGE

The incremental change renames the module-level constants to defaultSettingsSyncDebounceMs / defaultSettingsSyncMaxWaitMs, introduces a BlueOsStorageOptions type with well-documented properties, and threads an optional options parameter through useBlueOsStorage so callers can override the debounce and max-wait per key. Both previous findings are resolved. No new issues found.

1. Correctness & Implementation Bugs — ✅

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

Copy link
Copy Markdown
Member Author

Ready.

@rafaellehmkuhl rafaellehmkuhl merged commit f21cbae into bluerobotics:master Jul 3, 2026
11 checks passed
@rafaellehmkuhl rafaellehmkuhl deleted the settings-syncer-configurable-debounce branch July 3, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants