Skip to content

Queued offline updates#911

Draft
rwalker777 wants to merge 7 commits into
esphome:mainfrom
rwalker777:queued-offline-updates
Draft

Queued offline updates#911
rwalker777 wants to merge 7 commits into
esphome:mainfrom
rwalker777:queued-offline-updates

Conversation

@rwalker777

Copy link
Copy Markdown

What does this implement/fix?

Provides support for queued offline updates.

Related issue or feature (if applicable):

  • fixes

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Checklist

  • The code change is tested and works locally.
  • npm run lint passes.
  • npm run test passes.
  • Tests have been added to verify that the new code works (where applicable).

@github-actions github-actions Bot added the new-feature New feature label Jun 20, 2026
@esphbot

esphbot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

PR Review — Queued offline updates

Queued-offline-update feature is not wired up end-to-end — it can't render or be cleared as written. Needs the connecting plumbing, translation keys, and a CSS-class fix before merge.

What's solid:

  • The type addition (queued_update?: boolean), the new firmwareClearQueuedUpdate API wrapper, and the clock-outline/cancel icon registrations are correct and follow existing patterns.
  • Centralizing the offline-vs-online completion message in one place is a reasonable instinct (the location is just wrong).

What needs attention:

  • Feature is dead end-to-end: ?queued-update is never passed in render-content.ts, the clear-queued-update event has no listener, and firmwareClearQueuedUpdate has no caller — verified by grep. The badge/button never render and the clear action does nothing.
  • Missing translation keys: dashboard.clear_queued, dashboard.queued_successfully, firmware.install_successful are absent from en.json; some strings are hardcoded English.
  • CSS mismatch: badge uses non-existent .status-badge + inline #ff9800; the added .device-status.queued { --wa-color-warning } rule is never applied.
  • Offline branch in shared compileAndWait: clobbered by the next line for local-flash flows and conflates network-OFFLINE with USB flashing.
  • Scope creep / lost context: unrelated rspack test changes; several load-bearing comments deleted; no tests added; checklist boxes unchecked.

🔴 Blocking

1. Feature is unwired end-to-end — queuedUpdate never set, clear event never handled (`src/components/dashboard/render-content.ts`, L100-129)

The whole user-facing path is dead because nothing connects the backend data to the card or the card's events back to the API. I verified this across the codebase:

  • queued-update is never passed. render-content.ts (the only place <esphome-device-card> is instantiated) sets ?has-update-available, ?api-enabled, etc., but never ?queued-update=${device.queued_update}. So card.queuedUpdate is always false → the new renderStatusBadge "Update Queued" branch and the _renderAccentAction clear button never render.
  • clear-queued-update has no listener. _onClearQueue dispatches clear-queued-update, but grep across src/ finds zero @clear-queued-update= bindings. Clicking the clear button (if it ever rendered) does nothing.
  • firmwareClearQueuedUpdate has no caller. The new API method is never invoked anywhere.

Net effect: the PR claims "support for queued offline updates" but the feature can neither display nor be cleared. The plumbing additions (queued_update type, API method, badge, button) are correct in isolation but not connected.

Fix: pass ?queued-update=${device.queued_update === true} in render-content.ts, add @clear-queued-update=${() => host._api.firmwareClearQueuedUpdate(device.configuration)} (plus optimistic refresh), and confirm the backend actually sets queued_update on ConfiguredDevice.

<esphome-device-card
  ?has-update-available=${device.update_available}
  ?api-enabled=${device.api_enabled === true}
  <!-- missing: ?queued-update=${device.queued_update === true} -->
  <!-- missing: @clear-queued-update=${() => ...firmwareClearQueuedUpdate...} -->

🟡 Important

1. Missing translation keys + hardcoded English strings

Three localize keys referenced by this PR do not exist in en.json (verified by grep): dashboard.clear_queued, dashboard.queued_successfully, and firmware.install_successful. Per CLAUDE.md ("Add new copy to en.json only"), each must be added in this PR; the upload workflow then pushes them to Lokalise.

The _localize(key) || "English fallback" pattern masks the missing keys at runtime but is an anti-pattern here — _localize already returns the English base for any key present in en.json, so the || fallback only fires when the key is absent, i.e. it's papering over the omission. Two strings are not localized at all:

  • render-bits.ts: the literal Update Queued badge text is hardcoded.
  • device-card.ts / install-flow.ts: || "Clear Queue" and || "Update Queued Successfully!".

Fix: add the three keys to en.json and drop the hardcoded fallbacks; wrap the "Update Queued" badge text in _localize.

this._localize("dashboard.clear_queued") || "Clear Queue"
host._localize("dashboard.queued_successfully") || "Update Queued Successfully!"
host._localize("firmware.install_successful")  // none present in en.json
2. Badge uses a non-existent CSS class; the styles.ts rule is dead (`src/components/device-card/render-bits.ts`, L89-98)

The new badge markup and the new CSS rule don't match, so neither does its job:

  • The badge is rendered with class="status-badge", but there is no .status-badge rule anywhere in device-card/styles.ts. Every sibling badge uses class="device-status ${state}" (see lines 69/84/101 in this same file).
  • The rule added to styles.ts, .device-status.queued { color: var(--wa-color-warning); }, is therefore never applied — the markup never carries the device-status queued classes. Dead CSS.
  • The badge instead hardcodes style="color: var(--status-queued-color, #ff9800)". --status-queued-color is defined nowhere, so it always falls back to the literal #ff9800 instead of the design token --wa-color-warning the styles.ts change intended.

Consequence: the queued badge skips the shared .device-status layout (gap, font-size, icon sizing) and renders an off-token orange inconsistent with all other badges. It also violates the repo's "styles live in the component's static styles, not inline" convention.

Fix: render class="device-status queued" and delete the inline style, so the .device-status base rules + the new .device-status.queued color rule apply.

return html`<div
  class="status-badge"
  style="color: var(--status-queued-color, #ff9800);"
>
3. Offline-queue branch lives in shared compileAndWait and is immediately overwritten (`src/components/firmware-install-dialog/install-flow.ts`, L414-423)

compileAndWait is shared by three callers (verified): the web-serial flash flow (line 147), startDownload (309), and startArtifactDownload (340). All of them continue executing after the promise resolves — e.g. line 150 immediately sets _statusMessage = status_downloading and proceeds to fetch binaries and flash.

So the new offline branch that sets host._step = "done" and "Update Queued Successfully!" is clobbered on the very next line and the local flash/download continues anyway. The "done" state never sticks for these flows.

Separately, gating on host._device?.state === DeviceState.OFFLINE conflates network state with the install transport. A device flashed over USB (web-serial) is almost always network-OFFLINE, so this branch fires for ordinary cable flashing where queuing makes no sense.

The "queued offline update" concept only holds for a server-side OTA install that the backend defers until the device reconnects — not for any of the local-flash paths that funnel through compileAndWait. Putting the branch here makes it both wrong-for-USB and ineffective (overwritten). Consider moving the queued-success terminal state into the specific OTA path and returning early there, rather than inside the shared compile helper.

if (result.status === JobStatus.COMPLETED) {
  if (host._device?.state === DeviceState.OFFLINE) {
    host._step = "done";
    host._statusMessage = ... "Update Queued Successfully!";
  } else {
    host._statusMessage = host._localize("firmware.install_successful");
  }
  resolve();
  return;
}
4. Unrelated deletion of load-bearing comments (`src/components/firmware-install-dialog/install-flow.ts`, L387-396)

This PR removes several explanatory comments that document non-obvious behavior, unrelated to queued updates:

  • The _compileReject capture comment ("so a mid-flight detach … can settle this promise") and the _jobSource capture comment ("so a compile failure can pick the right hint variant") here.
  • In render-bits.ts: the renderLabels "+N overflow" cap comment, the renderEncryptionIcon rationale, and the transport-agnostic-icons explanation.

These encode reasoning that CLAUDE.md explicitly flags under "Things that have bitten us" (the _compileReject reject hook and the ?aria-checked / transport-icon notes). Deleting them strips context future readers rely on and isn't part of this feature.

Fix: restore the deleted comments; keep the diff scoped to the queued-update feature.

🟢 Suggestions

1. Scope creep — Windows path-normalization changes unrelated to queued updates (`test/build-scripts/rspack-config.test.ts`, L21-48)

This test file's changes (adding normalizePath to handle backslash separators, plus removing the regression-explaining comments on each it) have nothing to do with queued offline updates. They look like a beneficial cross-platform fix, but bundling them here muddies the PR's labeling (it's tagged new-feature) and release notes.

Suggest splitting these into a separate bugfix/maintenance PR, and keep the explanatory regression comments ("without this pattern, the published wheel ships … no logos/board images") rather than deleting them.

2. Typo in doc comment (`src/api/esphome-api.ts`, L1255-1256)

/** Clear aqueued update for an offline device. */ — "aqueued" should be "a queued".

/** Clear aqueued update for an offline device. */

Checklist

  • Feature wired end-to-end (data in, events out) — critical #1
  • New copy added to en.json — warning #2
  • Styling follows component/token conventions — warning #3
  • Completion-state logic correct across flows — warning #4
  • No unrelated deletions / scope creep — warning #5
  • Tests added for new behavior
  • No hardcoded user-facing strings — warning #2

To rebase specific severity levels, mention me: @esphbot rebase critical (fixes 🔴 only), @esphbot rebase important (fixes 🔴 + 🟡), or just @esphbot rebase for all.


Automated review by Kōan (Claude) HEAD=2b1bd29 4 min 13s

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see the review comment above.

@rwalker777 rwalker777 marked this pull request as draft June 20, 2026 00:59
@rwalker777

Copy link
Copy Markdown
Author

@jesserockz I am really struggling on the frontend. If someone could help me out. Remaining items:

  1. Fix Queued badge (not sure why this is not working).
  2. Fix Clear queued update on action menu (probably related to Add translations #1).
  3. Need to add to the detail device card showing the Queued update (maybe queued since?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants