Surface firing error causes and label the charts for screen readers (#164, #170)

[BenSeverson]

Closes #164. Closes #170.

#164 — the error cause was never shown

The firmware has always known why a firing failed and has always sent it. The web UI dropped it at every one of the three places it could have been shown:

Surface	Before	After
Settings → Last Error	E3	Kiln not heating (E3)
History detail	badge reading error	cause + remedy, role="alert"
Live dashboard	Error badge, nothing else	banner with cause + remedy

describeFiringError() mirrors error_code_description() in components/display/dashboard.c word for word — someone standing at the kiln comparing the LCD to their phone should get one story, not two. firingErrorGuidance() returns null rather than filler when there is nothing honest to add: over-temp and runaway have no safe instruction you can act on from a phone, so they get a description and no false confidence.

The emergency-stop row said ACTIVE and stopped there. My first draft told the operator to "clear the emergency stop in Settings" — there is no such control anywhere in the UI. Starting a firing calls safety_clear_emergency() (firing_engine.c:809) and safety_task re-trips within 500 ms if the fault is real. The copy now says that.

One wrinkle worth flagging

The live status payload carries no error code — only /api/v1/system does. useSystemInfo() never refetches on its own, so reading it directly would have shown whatever was cached when Settings last loaded, which is almost always "none". The dashboard now invalidates that query on the edge into error, and renders the generic line until the code arrives so the banner is never empty.

Adding errorCode to the status payload would be cleaner, but that means touching firing_progress_t, the REST serializer, the WS broadcast, the mock and the fixtures. Left for a follow-up rather than smuggled in here.

#170 — the charts were silent

Recharts emits a bare <svg> of <path> elements with no accessible name, so a screen reader announced nothing where a sighted user sees the whole firing. Both charts are now role="img" with a generated summary; role="img" also makes the descendants presentational, so axis ticks stop leaking out as a stream of loose numbers. The status badge is a polite live region — status changes on its own as the firing advances, with no user action to anchor them.

Tests

chartAria.test.ts (7) and firingError.test.ts (5). Two of the a11y assertions were checked by breaking the implementation and confirming they fail:

• dropping the empty-array guard → 1 failed (Math.max() of [] is -Infinity, so an idle dashboard would announce "peak -Infinity°C")
• changing the duration divisor → expected 'Temperature trace for Bisque. 3 readi…' to contain '60 minutes'

There is a real double-conversion trap here: buildChartData already converts to the display unit, so describeFiringChart takes the raw Celsius series instead of the assembled ChartPoint[]. A test pins that (572°F, not 1062).

Check	Result
npx vitest run	201 passed, 1 skipped (22 files)
npx tsc --noEmit	0
npx eslint . --max-warnings 0	0

Verified in the browser

Not just unit-tested — driven against real API responses on the dev server, with /api/v1/system stubbed to report lastErrorCode: 1 and emergencyStop: true:

• Settings → Last Error renders Thermocouple disconnected or shorted (E1)
• Emergency Stop row renders the guidance line beneath ACTIVE
• Dashboard chart aria-label → Temperature chart. 1 reading, currently 20°C, peak 20°C.
• Error banner (state forced, then reverted) → role="alert", aria-live="assertive", cause + remedy

Known gap

The mock server cannot produce an error firing at all — every history record is hardcoded errorCode: 0 and the simulator never enters status: "error". So none of the #164 UI is reachable in the published demo or in local mock-server development, which is why the verification above needed stubbing. Same mock/firmware divergence class as #131. Filing separately.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 97c4dc40d6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Clear the cached cause before showing a new failure

When a second firing enters error after a previous failure, React Query retains the earlier systemInfo value while invalidateQueries refetches, so this immediately displays the previous firing's error cause and guidance. For example, an earlier thermocouple fault followed by a not-rising fault will initially report the thermocouple fault, and it remains wrong indefinitely if the refetch fails; use the generic description while this error transition's request is pending or otherwise associate the fetched code with the current transition.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Do not announce the placeholder as a real reading

When the controller is offline or the initial REST and WebSocket requests have not produced data, currentTempData still contains the store's synthetic 20°C seed, so this label announces “1 reading, currently 20°C” even though no temperature has been received. That can leave a screen-reader user with a fabricated kiln reading indefinitely during an outage; pass an empty series until either the status request or a WebSocket frame establishes that the data is real.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Include the trace endpoint in the accessible summary

For a history trace that cools after reaching its peak, this accessible replacement reports only duration and peak, so a firing ending near room temperature and one ending at peak temperature announce identically despite conveying very different curves visually. Include at least the final temperature in the summary so screen-reader users can determine whether and how far the firing cooled.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Use the actual trip code for emergency-stop guidance

When emergencyStop is active because of a thermocouple fault, over-temperature event, not-rising condition, or runaway, this always renders the code-5 guidance telling the operator that starting a new firing clears a stale stop. All of those safety trips set the same emergency flag, while lastErrorCode preserves their actual cause, so this can show unsafe and contradictory advice such as “Over temperature” followed by an instruction to start another firing; select guidance from systemInfo.lastErrorCode and omit it for causes that deliberately have no safe remote remedy.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Avoid spreading unbounded traces into Math.max

For a sufficiently long firing trace, spreading every temperature as function arguments throws RangeError: Maximum call stack size exceeded during render and blanks the history detail. The firmware records one point per minute, supports an indefinite hold, and has enough SPIFFS space for more than the roughly 125,000 arguments at which this fails in V8, so multi-month or otherwise very long traces can reach this path; compute the peak with an iterative reduction instead.

Useful? React with 👍 / 👎.

[BenSeverson]

All five findings verified against the firmware and fixed in f0a3ed2. Details, in severity order.

P1 — emergency-stop guidance used a hardcoded code

Confirmed, and it was the worst kind of wrong: the copy was actively unsafe.

safety_emergency_stop_cause() is called with SAFETY_TRIP_TC_FAULT and SAFETY_TRIP_OVER_TEMP as well as OTHER (safety.c:369, :388, :400), and firing_err_from_trip() maps each to a distinct lastErrorCode. So every trip raises the one flag while the cause survives separately — and keying the copy off the flag printed "starting a new firing clears a stale stop" under an over-temperature trip.

New emergencyStopExplanation(lastErrorCode) drives the copy from the recorded cause. Over-temp and runaway get a cause and guidance: null — no reassuring sentence where there is no safe remote remedy. The "start a firing to clear it" line survives only for lastErrorCode === 0, which is the genuine bare-stop case: s_last_error_code is only assigned inside the firing loop, so no code means no firing was running.

Verified end-to-end in the browser with the mock reporting lastErrorCode: 2, emergencyStop: true:

Last Error     Over temperature (E2)
Emergency Stop ACTIVE
               Over temperature          ← no "start a new firing"

P2 — cached cause shown for a new failure

Confirmed. My PR text claimed the banner "renders the generic line until the code arrives"; that was only true on first load. On a second failure React Query keeps serving the first failure's value while refetching, and keeps it forever if the refetch fails.

Fixed by recording errorSince in the kiln store, stamped on the edge into error while folding the frame that reported it, and comparing it against the query's dataUpdatedAt.

Worth noting why it landed in the store: my first two attempts were a useState and then a useRef in the component, and lint rejected both — react-hooks/set-state-in-effect on the first, Cannot access refs during render on the second. Both were right to. The store already does exactly this transition bookkeeping (isNewFiring, endedFiring), and putting it there means the value is correct on the first render of a failure rather than one effect later.

The first version of this test was vacuous. receivedAt is Date.now() and frames pumped synchronously all land in the same millisecond, so "held for the duration" and "re-stamped every frame" were indistinguishable — breaking the implementation still passed. With vi.useFakeTimers() it fails properly:

AssertionError: expected 1767225606000 to be 1767225601000 // Object.is equality

Also fixed resetStore() in the store tests, which wasn't clearing the new field.

P2 — Math.max(...) RangeError on long traces

Confirmed reachable, not theoretical. history_record_temp() writes one sample per minute with no cap, and HOLD_UNTIL_SKIP holds run until a human intervenes — an unattended kiln really can get there. Replaced with an iterative reduction. Verified load-bearing by reverting just that line:

AssertionError: expected [Function] to not throw an error but 'RangeError: Maximum call stack size exceeded' was thrown

P2 — synthetic 20°C announced as a real reading

Confirmed: kilnStore.ts:37 seeds [{ time: 0, temp: 20, target: 20 }]. The chart draws it, which is cosmetic, but the label asserted it as a reading — a fabricated kiln temperature for the whole of an outage is worse than silence. The series now only counts once a WS frame has arrived (lastUpdateAt !== null) or the mount-time status fetch succeeded.

P2 — trace summary omitted the endpoint

Fair. Added; a firing that cooled and one that ended at peak no longer read alike:

peak 1200°C, ending at 25°C
peak 1200°C, ending at 1200°C

Check	Result
npx vitest run	213 passed, 1 skipped (22 files) — was 201
npx tsc --noEmit	0
npx eslint . --max-warnings 0	0
npx prettier --check .	0

The one thing I did not fix: the synthetic seed is still drawn on the chart itself, so a sighted user still sees a phantom 20°C point during an outage. That predates this PR and belongs with #239 rather than here.

[BenSeverson]

Rebased onto 8ecb4c4. Worth flagging what that changed, because #247 landed one of these findings independently.

The synthetic-20°C finding is now fixed upstream, not by me. #247 deleted the seed outright — initialTempData is [] with a comment describing the same defect from the chart's side ("load the page six hours into a firing with the kiln at 900°C and the chart claimed it started at 20°C"). So the empty-series branch of describeFiringChart is now reached naturally, and I removed the statusSeeded/haveRealTemps gate I had added. It was duplicate machinery guarding a case that can no longer arise.

errorSince moved into the store's seedFromStatus. #247 replaced the component's inline mount-time seed with a store action, which is where the transition stamp belonged anyway — a page loaded after a failure never sees the transition frame, so the snapshot is the only thing that can date it. Three tests cover that path, and the "don't re-date a failure already seen" one is load-bearing (the Dashboard tab isn't forceMounted, so revisiting re-seeds and must not read as a newer failure):

AssertionError: expected 1785343500105 to be 1785343440105 // Object.is equality

One conflict-resolution catch worth recording. The chart conflict was mine-vs-theirs on the whole <LineChart> body — my role="img" wrapper re-indented it. I took my side, which silently dropped #247's dot={showMeasuredDots} on the current/target series. Lint caught it as an unused variable. I restored it and then diffed my resolved chart against origin/main's ignoring indentation to confirm nothing else was lost:

IDENTICAL (ignoring indentation)

Check	Result
npx vitest run	254 passed, 1 skipped (23 files)
npx tsc --noEmit	0
npx eslint . --max-warnings 0	0
npx prettier --check .	0