fix(ui): gate WeatherLink Settings inputs on live-config load#156
Conversation
The WeatherLink Settings section initialises wlArchivePeriod=30,
wlSamplePeriod=60, and wlCal={inside_temp:0, outside_temp:0,
barometer:0, outside_humidity:0, rain_cal:100} as React state
defaults, then overwrites them after fetchWeatherLinkConfig()
resolves. Serial I/O for the live read is slow (the daemon has to
take the io_lock, send WRD commands, and wait for ACKs against the
poller), so there's a visible window — sometimes several seconds —
where the form displays the placeholder defaults as if they were
the user's saved values. A user looking at the Settings page during
that window sees "30 minutes archive period, 60s sample, zero
calibration on every field" and naturally assumes the values either
got reset or are real but wrong, types corrections in, and hits
Save. The save then writes those typed corrections (computed
against placeholder values) over the user's actual settings — a
collision the user can't undo without remembering what they had
before.
Gate every input/select in the section on `wlConfig === null` (still
loading). While loading:
- Section title gets a small italic "loading current values from
hardware…" indicator so the state is visible at a glance.
- Every <input> and <select> has `disabled` set and visual opacity
reduced to 0.5 so the placeholder values clearly read as "not
yet loaded" rather than "your current settings".
- The Save button is also disabled with cursor `not-allowed` and
label "Loading...", and a tooltip explaining why.
Verified `tsc -b --noEmit` clean and `npm run build` succeeds (only
the pre-existing chunk-size warning, which CLAUDE.md flags as
non-blocking).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![vsits-codex-review-agent[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
There was a problem hiding this comment.
The input gating itself looks complete: the seven WeatherLink form fields and the Save action are all covered, and leaving Force Archive / rain-clear actions live seems reasonable because they do not depend on the placeholder config values. The main problem is that the new wlConfig === null gate now also represents every config-load failure path, so an API-level {error: ...} response leaves the section in a permanent fake loading state with Save disabled and no explanation to the user. I would separate "still loading" from "load failed" before shipping this. Scope-wise, leaving the unrelated barometer-tooltip copy out of this PR is fine.
— Codex Review Agent
| <h3 style={sectionTitle}>WeatherLink</h3> | ||
| <h3 style={sectionTitle}> | ||
| WeatherLink | ||
| {wlConfig === null && ( |
There was a problem hiding this comment.
wlConfig === null is doing double duty here: it means both "the initial read is still in flight" and "the read never produced a usable config". fetchWeatherLinkConfig() is backed by an endpoint that can return {"error": ...} as a normal 200 response when the logger is down or the link is not connected; in that case the mount effect never calls setWlConfig, never sets wlError, and the section stays stuck showing "loading current values from hardware…" with Save disabled forever. Before this PR that failure path was misleading but still operable; now it becomes a permanent lockout. Please track load status separately from config presence, and surface the load error so the user can distinguish "still waiting" from "cannot read current settings".`
…rrors Codex round-1 review on PR #156 caught that `wlConfig === null` was doing double duty as both "still loading" AND "load failed". The `/api/weatherlink/config` endpoint can legitimately return a 200 response with `{error: ...}` when the logger daemon is down or the link isn't connected; on that path, the mount effect skipped its setWlConfig call, never populated wlLoadError, and the WL section stayed in a permanent fake "loading current values from hardware…" state with Save permanently disabled and no user-visible reason. That turned an existing degraded path into a silent lockout. Now: - New `wlLoadStatus` state ("loading" | "loaded" | "error") tracks the initial read distinctly from wlConfig presence. - New `wlLoadError` carries the surfaced message. - The load is extracted into a `loadWlConfig()` callback so the error path can offer a retry. - The section title shows three distinct states: • "loading current values from hardware…" (italic muted) • "could not read current settings — [retry]" (red, with link) • nothing (when loaded) - An inline error banner under the title explains what failed and why the inputs are locked, when status is "error". - All 7 inputs/selects and the Save button are gated on `wlLoadStatus !== "loaded"` so both "loading" and "error" lock them — that part is intentional, since saving placeholder defaults over real settings is the bug we're guarding against. The difference is that "error" now SAYS so, with a retry path. - The Save button label distinguishes the two non-loaded states: "Loading..." vs "Unavailable" (with corresponding tooltip). Verified `tsc -b --noEmit` clean and `npm run build` succeeds. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
The round-1 blocker is addressed in 3d74b4c: the WeatherLink section now distinguishes "still loading" from "load failed", surfaces the failure reason inline, and gives the user a retry path without requiring a page reload. I walked both the API-level {error: ...} path and the rejected-promise path, and both now land in the same visible locked-until-retry state rather than a permanent fake spinner. Keeping the inputs disabled until a successful live read still matches the original safety goal, and the updated Save label/tooltip make that state understandable enough for this PR. I do not see a new blocker introduced by the retry flow or the error presentation here.
— Codex Review Agent
The tooltip on the Barometer Offset input still read "Subtracted from raw barometer reading (thousandths inHg)" — accurate pre-#155, but that PR inverted the sign at the kanfei-to-Davis I/O boundary so positive values in the UI now ADD to the reading (matching the intuitive UX a user expects). A stale "subtract" tooltip on the post-fix code would re-introduce exactly the confusion #154 reported. New tooltip explicitly calls out the post-#155 semantics so anyone hovering for guidance sees that positive raises and negative lowers, plus a pointer to the PR for context. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
Gate every input/select in the Settings → WeatherLink section on
wlConfig === nullso the placeholder defaults (archive_period=30, sample_period=60, all-zero calibration) can't be mistaken for the user's saved values during the serial-I/O latency window — and can't accidentally get written back over those saved values on Save.wlArchivePeriod,wlSamplePeriod, the 5 calibration fields) aredisabledand dimmed (opacity: 0.5) untilwlConfigresolves.not-allowed, label changes to"Loading...", and the tooltip explains the gate.Why
The WeatherLink section initialises React state with placeholder defaults, then overwrites them after
fetchWeatherLinkConfig()resolves. Serial I/O for the live read is slow — the daemon has to take the_io_lock, send WRD commands, and wait for ACKs in contention with the poller — so there's a visible window (sometimes several seconds) where the form displays "30/60/zeros" as if they were the user's saved values. A user looking at the page during that window naturally assumes the values either got reset or are real-but-wrong, types corrections in, and hits Save. The save then writes those typed corrections (computed against placeholder values) over the user's actual settings.Test plan
npx tsc -b --noEmitcleannpm run buildsucceeds (only the pre-existing chunk-size warning, which CLAUDE.md flags as non-blocking)Adjacent issue (flagging, not fixing in this PR)
While editing this section I noticed the Barometer Offset input's tooltip at line 2544 still reads "Subtracted from raw barometer reading (thousandths inHg)" — which was correct pre-#155 but inverted by that PR's sign-fix. The actual semantics in the running code are now add to raw reading. Per CLAUDE.md ("flag architectural issues, don't silently refactor"), I'm calling this out rather than fixing it unilaterally. Want me to roll the label fix into this PR or punt to a follow-up?
🤖 Generated with Claude Code