fix(client): stop widgets writing back settings they just read if unchanged or failed to read

[mrramam]

Three effects in ChoreWidget and CalendarWidget persisted their settings whenever a load finished, rather than when a value changed. The loaded flag was a dependency, so completing the load was itself a trigger and the PATCH sent back exactly what the GET returned. Every mount was a write.

On a display that rotates tabs this is continuous: a widget absent from the next tab unmounts and writes again on the way back. Measured on a kiosk at three writes per rotation cycle, unaffected by whether the dashboard was locked.

The tab-specific effect in CalendarWidget is worse than the other two; it lists activeTab as a dependency, so it fires on every tab change even when the widget never unmounts.

More serious than the churn: both widgets set their loaded flag in a finally block, so it flips to true even when the GET fails. The component is then holding its own defaults, and the persist effect wrote those over whatever the server had. A failed settings read silently replaced the user's stored settings with defaults.

Both rules now live in one place. shouldPersistSettings refuses when the values match the ones loaded, and refuses outright when no load has succeeded - the snapshot stays null in that case, which is what distinguishes "unchanged" from "never read". Snapshots update only after a confirmed write; the tab-specific persist previously swallowed its error and resolved anyway, which would have marked the snapshot clean after a failed PATCH and suppressed the retry.

The tab snapshot is keyed by tab number because that effect re-runs per tab, and comparing against another tab's values would keep writing on every rotation.

Unit tests cover the helper, including the failed-load case. They are in client/src/utils/ because the client suite runs in node with no JSX renderer, so the decision is testable only once it is out of the component.

[jherforth]

Reviewed critically and merging. I reproduced both the churn and the data loss on main before judging the fix, rather than taking the description on trust.

The bug is real, and the measurement is right

Set up a device with both widgets on one tab and counted writes to /api/devices/*/settings during mount:

[MAIN]  settings writes during mount: 3
    PATCH …/settings  {"calendarWidgetSettings":{…}}
    PATCH …/settings  {"choreWidgetSettings":{…}}
    PATCH …/widget-assignments/layout  {"widget_name":"calendar",…}
[PR171] settings writes during mount: 0

Three per mount, exactly as you measured on the kiosk.

The data loss reproduces — and it is the serious one

This took some care to trigger. Failing every GET to /settings proves nothing: the widgets never render at all, so nothing persists and the bug hides. The real scenario needs the app-level read to succeed and the widget's own read to fail. Letting the first reads through and failing the rest:

stored before:  {"showBonusChores":false,"soundEnabled":false,"hiddenUserIds":[]}

[MAIN]  chore rendered: true | settings PATCHes: 2
    {"choreWidgetSettings":{"showBonusChores":true,"soundEnabled":true,"hiddenUserIds":[]}}
  stored now: {"showBonusChores":true,"soundEnabled":true,"hiddenUserIds":[]}   ← clobbered

[PR171] chore rendered: true | settings PATCHes: 0
  stored now: {"showBonusChores":false,"soundEnabled":false,"hiddenUserIds":[]}  ← preserved

A user's stored preferences replaced by component defaults because a GET failed. Worth fixing on its own.

No regression on real edits

writes on mount (expect 0): 0
stored before: {"showBonusChores":false,"soundEnabled":false,…}
[click "Enable chore sounds on this display"]
writes after a real edit: 1
    {"choreWidgetSettings":{"showBonusChores":false,"soundEnabled":true,…}}
stored after:  {"showBonusChores":false,"soundEnabled":true,…}
extra writes while idle: 0

The last line matters: the snapshot updates after the confirmed write, so the edit does not re-fire.

On the code

Capturing adopted as the resulting value (server value or default) rather than the raw server object is the detail that makes this correct — a server holding partial settings would otherwise differ from component state and write on every mount, just more subtly. Returning true/false from persistTabSpecificSettingsForTab is right too; it previously swallowed its error and resolved, so marking the snapshot clean there would have suppressed the retry.

I checked the things that could bite:

• stable() sorts keys, so a JSON round-trip is not read as a change, and arrays stay order-sensitive. Correct.
• Freezing DEFAULT_HIDDEN_USER_IDS is safe — toggleHiddenUserId, pruneHiddenUserIds and filterVisibleUsers all return new arrays and never mutate.
• The new .then() on the tab persist has no .catch(), but the whole function body is inside its try, so it cannot reject.
• Out-of-order PATCH responses can leave the snapshot holding older values, which costs one redundant write and self-corrects. No lost write.

One thing worth a follow-up (not a blocker)

The failed-load lockout is silent. When the GET fails, loadedSettingsRef stays null for the life of the mount, so the widget can never persist — a user toggling a setting sees it take effect and then lose it on reload, with nothing said. It is strictly better than overwriting the server with defaults, so this is the right trade, and on a rotating display it self-heals on the next remount. But on a static dashboard it lasts until reload. A retry of the load, or surfacing "settings unavailable", would close it.

Server 224/224 and client 281/281 via CI, translation parity and build clean locally.