strictNullChecks slice 2: instrument widgets + context-less misfiling fix (#6, #209)#210
Conversation
setPathContext filed a delta value with an empty/absent context under a literal "undefined.<path>" key, so widgets subscribed to self.<path> never saw it. Fold empty/undefined context to the self root, matching the ISkPathData/IMeta "empty context assumes Self" contract. Foreign contexts still route to their own root. Closes #209
Slice 2 of #6: the gauge, boolean-SVG, and numeric/slider widget families. 268 issues fixed, betterer baseline 842 -> 574. Honest type widening only — no non-null assertions, casts, or suppressions. - gauges (gauge-steel, gauge-ng-linear, gauge-ng-radial, simple-linear): optional inputs typed T | undefined; theme() guarded before deref; cfg/theme effects early-return during the pre-merge startup window. - boolean SVG (button/switch/light): data/theme inputs and colour fields widened to match their real null defaults; guards mirror the parent widget. - numeric/slider: SK values widened to number | null and skipped (not coerced) when null, so a timeout/sensor-loss null can't corrupt tracked min/max or reach .toFixed(); canvas context threaded as a narrowed local. Two edge-case behaviour fixes the honest typing surfaced (both flagged): - gauge-steel: a zone with an undefined bound previously produced a NaN section bound (the `!== null` clamp missed undefined -> convertToUnit -> NaN); now clamps to min/max, as the adjacent comment always intended. - minichart.verticalChart: typed `boolean | null` (was inferred `null`) so widget-numeric can assign it; null default and truthy usage unchanged. These widgets have no unit specs (canvas rendering); validated via AOT build (strictTemplates), lint, and betterer. No file regressed vs the baseline. Refs #6
Code review — PR #210 (strictNullChecks slice 2: instrument widgets + #209)Scope: 11 source files + a spec vs Headline: no bugs — every null-guard replaces a path where the original code dereferenced undefined and crashed, so the guarded paths are strictly safer, and fallback values were confirmed to match each widget's own P2 — behavior changes to accept or revert
P3 — smaller
Verified clean (adversarial + correctness)
Coverage5/5 reviewers returned. No P0/P1. Suppressed: a couple of pre-existing awareness notes ( Verdict: Ready with decisions. No defects. Decide whether to keep the two behavior improvements (#1 gauge zones, #2 numeric min/max) — both are more correct and #1 is safety-relevant (visible alarm zones) — or preserve base behavior and file them separately. Revert the scope-creep config change (#5). Tests for #1/#2 recommended (extract the numeric min/max reducer to a pure function is the cheap path; gauge-steel zones via a |
…behaviour Review follow-ups (#210): the two behaviour changes this slice made — kept as deliberate improvements — now have tests. - widget-numeric: extract the running min/max fold into a pure reduceMinMax() util and unit-test it, including the regression guard that a null sample (sensor dropout) preserves the tracked extremes instead of wiping them. - gauge-steel: a spec drives buildOptions() with an open-ended low-alarm zone and asserts it renders as a band clamped to the gauge minimum (was a NaN section that did not draw). Also a one-line note on why zones render before the theme loads, unlike the other widgets. Refs #6
Review addressed (commit
|
Slice 2 of the
strictNullChecksmigration (#6), plus the folded-in #209 fix you OK'd.Widget slice (268 issues, betterer baseline 842 → 574)
The mechanical instrument-widget families, migrated by three Sonnet agents on disjoint files under hard guardrails (no
!, casts,any, or suppressions — honest type widening + real guards only). I reconciled, scanned every diff, and reviewed the null-semantics.T | undefined;theme()guarded before deref; cfg/theme effects early-return during the pre-merge startup window.data/themeinputs and colour fields widened to their real null defaults (colour fields feed[attr.*]bindings, which acceptnull); guards mirror the parent widget.number | nulland skipped (not coerced) when null, so a timeout/sensor-loss null can't corrupt tracked min/max or reach.toFixed(); canvas context threaded as a narrowed local.Two edge-case behavior fixes (deliberate — flagging per behavior-preservation)
The honest typing surfaced two genuine bugs; both are consciously fixed, not preserved:
NaNsteelseries section bound (the!== nullclamp missedundefined→convertToUnit(units, undefined)→ NaN). Now clamps to min/max, as the adjacent comment always intended. Real-bounded zones are unchanged.boolean | null(was inferrednull) sowidget-numericcan assign it. Null default and truthy usage unchanged — a one-line cross-file unblock.#209 — context-less values misfiled
DataService.setPathContextfiled a delta value with an empty/absent context under a literal"undefined.<path>"key, so widgets subscribed toself.<path>never saw it. Folded empty/undefined context to the self root (per theISkPathData/IMeta"empty context assumes Self" contract). Foreign contexts still route to their own root. Two end-to-end DataService tests pin both directions.Coverage note
The touched widgets have no unit specs (canvas rendering isn't unit-testable and none existed). Validated via AOT build (
strictTemplatesover every widened binding), lint, andbetterer:ci. The #209 fix has real tests.Remaining under #6
Charts —
widget-data-chart(164) andwidget-windtrends-chart(100) — are the least mechanical and stay for slice 3, along withwidget-autopilot(65) and the remaining components.Verification
npm run lint,npm run build:dev(AOT/strictTemplates), andbetterer:ci(574) all pass; no file regressed vs the baseline; data.service specs pass locally.Refs #6
Closes #209