iOS: stop presenting a suspended app's last reading as current (#147) - #253
Conversation
Two of the three fix directions on the issue. The third is a product decision, not a patch, and is left alone deliberately. **staleDate on Live Activity content.** The app declares no background modes and requests the activity with `pushType: nil`, so iOS suspends the WebSocket within seconds of backgrounding and nothing can update the activity until the app is opened again. With `staleDate: nil` the lock screen kept presenting that last temperature as current, indefinitely, across an 8-12 hour firing. For a number an operator may act on, wrong and confident is worse than visibly out of date. 90s: the firmware broadcasts every second, so any longer gap is already abnormal, but it is loose enough to ride out a Wi-Fi blip while foregrounded without flickering to stale. The terminal content keeps `staleDate: nil` on purpose — "Complete" is not a reading that decays, and ageing it would make a finished firing look like a lost connection. **Resync on foreground.** `scenePhase == .active` now refreshes status over REST and then restarts the socket. Previously the socket came back dead while `connectionState` still read `.connected`, so returning to the app mid-firing showed the reading from whenever the phone went into a pocket until the user happened to pull to refresh. REST before reconnect, because the snapshot is what makes the screen correct immediately; reconnecting alone leaves it wrong until the next broadcast. `resumeIfConnected()` is a no-op unless a connection was already established, so a foreground event cannot start dialling a kiln the user never connected to, or paper over an `.error` they still need to read. **Not fixed: background delivery.** The "Kiln Error / check kiln immediately" notification still requires the app to be running, which is the safety gap the issue is really about. Closing it means either wiring the firmware's existing webhookUrl to a push bridge, or an APNs relay for ActivityKit push tokens — infrastructure choices for a LAN-only device, not something to pick unilaterally. #147 stays open for that. Not verified by compilation: no Swift toolchain on Linux. CI's build-ios job is the only compile check, and staleness rendering plus scenePhase transitions need a device to confirm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PyJ47o5sWvpb9vMk1RNKkb
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1594c9648b
ℹ️ 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".
| webSocket.disconnect() | ||
| webSocket.connect(host: host, port: port, apiToken: apiToken) |
There was a problem hiding this comment.
Whenever resumeIfConnected() runs while the old receive loop is still awaiting a message, disconnect() cancels that socket and connect() immediately sets shouldReconnect back to true. The canceled loop then reaches receiveLoop's catch and schedules another reconnect; when that fires it cancels the replacement socket, whose loop schedules the next reconnect, producing continuous socket churn and missed updates after foregrounding. Suppress reconnects from the canceled generation or wait for the old receive loop to finish before enabling reconnection.
Useful? React with 👍 / 👎.
|
|
||
| Task { | ||
| await activity.update(.init(state: state, staleDate: nil)) | ||
| await activity.update(.init(state: state, staleDate: Self.staleDate())) |
There was a problem hiding this comment.
When the app remains suspended beyond 90 seconds, staleDate only causes ActivityKit to set ActivityViewContext.isStale; it does not replace the displayed state automatically. The lock-screen and Dynamic Island views still render currentTemp, status, and remaining time identically and never inspect context.isStale, so the old temperature continues to look current indefinitely despite this timestamp. Update both Live Activity presentations to visibly label or hide stale readings.
Useful? React with 👍 / 👎.
| guard phase == .active else { return } | ||
| Task { | ||
| if let client = connection.apiClient { | ||
| await store.refreshAll(using: client) |
There was a problem hiding this comment.
When a firing reaches complete, error, or idle while the app is suspended, this call enters refreshAll, which assigns previousStatus = status.status without invoking handleStatusTransition; the next WebSocket frame has the same status and therefore cannot recover the skipped transition. As a result, the existing Live Activity is never ended and peakTempC is not reset, so its old high-water mark can contaminate the next firing's reported peak. Apply the transition handling before replacing previousStatus during this foreground resync.
Useful? React with 👍 / 👎.
| guard phase == .active else { return } | ||
| Task { | ||
| if let client = connection.apiClient { | ||
| await store.refreshAll(using: client) |
There was a problem hiding this comment.
On foregrounding, this waits for refreshAll, but that method only applies the fetched status after the profiles and settings requests also succeed. If either unrelated endpoint fails or hangs, a successful current status is discarded and the WebSocket restart is postponed until the structured child requests finish, potentially leaving the suspended temperature on screen for the API client's 30-second resource timeout. Fetch and apply the status independently, or reconnect the live feed without waiting for the auxiliary refreshes.
Useful? React with 👍 / 👎.
… churn Four review findings, three of them P1 and two of them introduced by the first pass of this PR. **staleDate was inert.** Setting it only flips `ActivityViewContext.isStale`; ActivityKit does not alter the rendering. Neither the lock screen nor the Dynamic Island read that flag, so the frozen temperature went on looking exactly as current as before — the PR claimed a behaviour change that did not happen. Both presentations now dim the reading and, where there is room, say why. The countdown is *replaced* rather than dimmed: "~2h remaining" extrapolated from a frozen frame is a claim, not a stale observation. **The foreground resync caused continuous socket churn.** disconnect() cancels the receive loop's task and socket, but the loop reaches its catch block regardless (`Task.isCancelled` guards loop entry, not the catch) and hops to the main actor to schedule a reconnect — by which time connect() has already set shouldReconnect back to true. That reconnect then cancels the healthy socket, whose loop schedules the next one, at 1-8s intervals, indefinitely. Worse than the bug being fixed. Each openConnection() now stamps a generation; a loop reports its socket lost only while its own stamp is still current. Cancelling the task alone would not do: openConnection() can supersede a loop that was never cancelled. disconnect() bumps it too, so an explicit disconnect silences its loop by generation rather than relying on shouldReconnect. **A transition that happened while suspended was swallowed.** refreshAll assigned previousStatus directly, so a firing that completed in the background was adopted without running handleStatusTransition — and the next frame carried the same status, making the difference undetectable forever. The Live Activity was never ended and peakTempC carried its high-water mark into the next firing's completion notification: the same lie #146 was about, by a different route. Snapshots now run the transition, gated on hasObservedStatus so the first load after launch does not announce a firing the app never watched. That gate is applied to the WebSocket path too, where the same spurious announcement was possible on a first frame of "complete". **Status no longer waits on unrelated requests.** refreshAll awaited status, profiles and settings together, so a failure or stall on either of the latter discarded a good status and — on foreground — held the socket restart behind it. Status is now its own method; the foreground path calls that and reconnects, leaving profiles and settings to connect time and pull-to-refresh. Still not compiled: no Swift toolchain on Linux, so CI's build-ios job remains the only compile check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PyJ47o5sWvpb9vMk1RNKkb
|
All four correct, and two of them were introduced by this PR. Fixed in P1 — One judgement call: the countdown is replaced rather than dimmed. "~2h remaining" derived from a frozen frame is a claim, not a stale observation — dimming it would still be asserting something the app has no basis for. P1 — continuous socket churn, caused by this PR. Also correct, and worse than the bug I was fixing. Fixed with a generation stamp per P1 — swallowed transition. Correct, including the consequence: because the next frame carries the same status, the difference becomes undetectable permanently, so the Live Activity is never ended and Guarded on a new P2 — status coupled to unrelated requests. Correct. Split into Still not compiled — no Swift toolchain here, so Generated by Claude Code |
2 participants
Addresses two of the three fix directions on #147. #147 stays open for the third — see the bottom, it's the part that actually closes the safety gap and it's a decision rather than a patch.
staleDateon Live Activity contentThe app declares no background modes and requests the activity with
pushType: nil, so iOS suspends the WebSocket within seconds of backgrounding and nothing can update the activity until the app is reopened. WithstaleDate: nil, the lock screen kept presenting that last temperature as current, indefinitely — across an 8–12 hour firing.For a number an operator may act on, wrong and confident is worse than visibly out of date. Now 90 s: the firmware broadcasts every second so any longer gap is already abnormal, but it's loose enough to ride out a Wi-Fi blip while foregrounded without flickering to stale.
The terminal content keeps
staleDate: nilon purpose — "Complete" isn't a reading that decays, and ageing it would make a finished firing look like a lost connection.Resync on foreground
scenePhase == .activenow refreshes status over REST and then restarts the socket. Previously the socket came back dead whileconnectionStatestill read.connected, so returning to the app mid-firing showed the reading from whenever the phone went into a pocket, until the user happened to pull to refresh.Two deliberate details:
resumeIfConnected()is a no-op unless a connection was already established. A foreground event must not start dialling a kiln the user never connected to, or paper over an.errorthey still need to read.Not fixed — and this is the important part
The "Kiln Error / check kiln immediately" notification still requires the app to be running. That intersection — app backgrounded enough for the notification gate to allow it, but foregrounded enough for the socket to be alive — is nearly empty, which is the safety gap the issue is really about. Nothing here changes that.
Closing it means one of:
webhookUrlsetting to a push bridge (ntfy or similar), orBoth are infrastructure choices with ongoing cost and privacy implications. Not mine to pick, so I've left #147 open rather than closing it on a partial fix and letting the gap look addressed.
Verification
Not compiled — this session is a Linux container with no Swift toolchain, so CI's macOS
build-iosjob is the only compile check. Staleness rendering andscenePhasetransitions both need a device to confirm behaviourally; I've checked call sites and actor isolation statically, which is not the same thing.Generated by Claude Code