Skip to content

Distinguish active export from freeze export in the web header icon - #5131

Open
springfall2008 wants to merge 1 commit into
mainfrom
feat/export-icon-active-vs-freeze-5125
Open

springfall2008 wants to merge 1 commit into
mainfrom
feat/export-icon-active-vs-freeze-5125

Conversation

@springfall2008

Copy link
Copy Markdown
Owner

This is an automated draft PR generated from issue #5125 — a maintainer should review it before merging.

Fixes #5125

Summary

The export icon next to the SoC in the web header was appended whenever binary_sensor.predbat_exporting was on, with nothing to say which kind of export was running, so a freeze export (solar surplus only) looked identical to an active export discharging stored capacity.

execute_plan() now tracks which of the two export branches actually fired and set_charge_export_status() publishes it as an export_status attribute on the exporting binary sensor (target / freeze / none). get_battery_status_icon() reads that attribute and tags the icon with an export-active or export-freeze class plus an explanatory title, and get_header_html() defines those classes green/amber with light- and dark-mode variants. An install that has not published the attribute (or an unrecognised value) keeps the original uncoloured icon.

Per the triage note, battery power is not used to tell the two apart — during a freeze export the battery can still discharge to cover house load, which would mislabel freeze as active. The signal is the plan's own branch. Where inverters disagree, active export wins, matching the existing EXPORT_STATE_PRECEDENCE used for the headline status.

Testing

cd coverage && ./run_pre_commit — all 14 hooks pass, and the quick suite it runs passes (All tests passed (4 slow tests skipped)).

Red/green on the two affected modules, with only the source changes stashed:

  • tools/triage_test.sh web_functions — fails without the fix (expected export-active class for active export, got: ...mdi-transmission-tower-export... for both modes, i.e. the reported symptom, plus the four missing CSS rules), passes with it.
  • tools/triage_test.sh execute — fails without the fix (ERROR: export_status should be none for status 'Demand' got None, the attribute not being published at all), passes with it, including all 25 Exporting / Freeze exporting scenarios now asserting the right value.

The execute red run stops at the first failing scenario, so its red output only shows the "attribute absent" failures rather than a target-vs-freeze mismatch; the web_functions red run is the one that demonstrates the two modes rendering identically.

Notes

  • Blast radius (GitNexus impact, upstream): get_battery_status_icon and get_header_html are both CRITICAL — every page header calls them — and execute_plan is HIGH. All three changes are additive: two new locals and one defaulted keyword argument in execute_plan, an extra class/title on an existing <span>, and four new CSS rules. No existing output changes when the attribute is absent. set_charge_export_status reported LOW (0 callers — the indexer misses the execute.py:802 call site; the new argument is defaulted so the old signature stays valid either way).
  • The attribute is named export_status, not export_mode, deliberately: EXPORT_MODE_TARGET/_FREEZE already exist as integer window modes (what the planner chose), which is a different thing from what the inverters were actually told to do this cycle. One name is used end to end — constant, argument, attribute, docs.
  • Tests spell the three values as literals rather than importing the constants: they are published on an HA entity, so a rename would break a user automation whether or not the constant moved with it.
  • Colour alone is a weak signal for colour-blind users, so each icon also carries a title tooltip naming the mode. The issue's fallback suggestion of an "A"/"F" letter prefix was not added, to keep the header compact — easy to add if you would prefer it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@springfall2008 springfall2008 self-assigned this Sep 17, 2026
@springfall2008 springfall2008 added the BOT_REVIEW Trigger an autotriage label Sep 17, 2026
@springfall2008

Copy link
Copy Markdown
Owner Author

Automated comment from the triage bot. — Code review of PR #5131 (diff origin/main...3f0cba55, 8 files, +136/−10) at effort level "high". Overall verdict: small, correct, and well tested — the findings below are low severity and none blocks merge.

What was verified:

  • Producer: set_charge_export_status in apps/predbat/output.py (~3035) has exactly one caller — apps/predbat/execute.py:810, the modified one. No other caller, no mock, no startup-time publisher of binary_sensor.predbat_exporting. The export_status if isExporting else EXPORT_STATUS_NONE ternary makes it impossible to publish "target"/"freeze" while the sensor state is "off".
  • Flag logic: both isExporting = True sites (~519, ~552) also set the matching flag, so isExporting == True with attribute "none" is unreachable. The if/elif "active export wins" rule matches EXPORT_STATE_PRECEDENCE (const.py:142), so the attribute and the headline status text resolved by resolve_multi_inverter_status agree today.
  • Consumer: get_battery_status_icon in apps/predbat/web.py (~4699-4713) — the title string is a fixed literal (no injection risk), real ha.py get_state (ha.py:790-818) returns the caller's default when the attribute is missing, and every WebInterface-routable implementation (component_base.py:360, predbat.py:188, mock_base.py:80) accepts attribute= + default= together. Missing attribute, restart-before-first-execute, and ha_interface is None all degrade to the designed plain icon.
  • Persistence: dashboard_item → set_state_wrapper → ha_interface.set_state stores the full attributes dict in both the HA path (ha.py:1067-1086) and the standalone DB mirror (db_engine.py:152/196, read back at db_engine.py:83-92); unrecorded_attributes = ["results"] does not affect the new attribute. State and attribute are published together in one call, so the producer never lets them diverge.
  • Tests: the new run_execute_test assertion (test_execute.py:464-474) was checked against all 136 assert_status= values — no false failures. In particular "Hold exporting" (2209) and "Hold exporting, Hold for car" are safe: the case-sensitive substring check means "Exporting" never matches "Hold exporting", so the expectation is "none", matching what the Hold branch publishes. "Demand [Freeze exporting]" splits to base "Demand" → "none", also matching. The new web-function tests restore state with set_exporting(False) before later tests.
  • CSS/consumers: the new .export-active/.export-freeze rules sit in get_header_html's <style> block, which all three get_battery_status_icon call sites (web.py:1018, 1796, 2481) render through; the status-API consumer inserts via innerHTML so the new title works. body.dark-mode matches the existing convention. No other consumer matches the old exact span string; the base mdi mdi-transmission-tower-export class is preserved in every branch. Line lengths within limits; no removed-behaviour regression found.

Findings (all low severity):

  1. apps/predbat/execute.py ~581 (functional gap) — The set_freeze_export_during_demand path pauses charging so PV surplus genuinely exports during a demand period and appends "[Freeze exporting]" to the status text (595/815), but it never sets isExporting/exporting_freeze, so the attribute publishes "none" and the icon stays plain. Same gap in the sibling path: set_export_freeze_only=True while the plan still carries a TARGET-mode window falls into the "Hold exporting" branch (559) with adjust_force_export(False) — real freeze-export behaviour, published as "none". The PR's stated use case (user automations keyed on export_status) never sees "freeze" on these paths, and the added docs sentence (output-data.md ~427, "the attribute says which sort of export is running") over-claims accordingly. Sensor state was equally off before the PR, so this is an over-claim/gap, not a regression — but it is the one case where the new attribute contradicts its documented contract, and the PR's own test at test_execute.py:2940 enshrines it.

  2. apps/predbat/execute.py ~804 (drift risk) — The exporting_to_target/exporting_freeze flags plus the if-chain re-encode, in a third hand-written place, the most-active-first ordering that EXPORT_STATE_PRECEDENCE already defines (consumed by resolve_multi_inverter_status at :798 and dominant_slot_status at output.py:3471). If the const list is reordered or a new export sub-state added, the headline text changes while the hardcoded attribute order does not (and any unrecognised state silently maps to "none"), so sensor text and attribute/tooltip can contradict. Deriving the attribute from resolve_multi_inverter_status's result would leave one ordering to maintain. Related nit: the two-line "spelt out as literals" rationale comment is copy-pasted in three places (test_web_functions.py:49, test_execute.py:466) with the mapping then re-derived a fourth way as the test oracle.

  3. apps/predbat/tests/test_execute.py ~470 (test-oracle coupling) — The new assertion derives its expectation from the headline status string rather than from the modelled per-inverter flags, so the genuinely new logic (execute.py:804-806, "active export wins") has no direct test. It only agrees with the implementation because the precedence list happens to order the same way as the string heuristics; a future mixed-fleet test (one inverter target, one freeze) or a "Cross-charging" scenario (headline ≠ winning flag) would either fail spuriously or silently validate the wrong path. Asserting from a flag/seed would decouple it. Related: TestHAInterface.get_state (test_infra.py:241) returns "" instead of the caller's default on a missing attribute, so the new "no attribute published" web test exercises the "" path while production runs the default=EXPORT_STATUS_NONE path — currently indistinguishable (both hit the plain-icon branch), but vacuous if the two ever diverge in handling.

  4. apps/predbat/web.py ~4682/4703 (non-atomic read) — State and export_status are read in two separate get_state_wrapper calls against the shared state_data dict the main thread republishes each cycle, so a render can straddle two publish generations (e.g. stale "on" + fresh "none", or a wrong-coloured icon). Self-corrects on the next 5s /api/status poll; a single raw fetch (raw=True already returns state and attributes in one call) would avoid it. This also slightly amplifies the pre-existing KeyError exposure from update_states() rebinding state_data wholesale (ha.py:802-806/902), which can 500 an unguarded get_header render.

  5. apps/predbat/web.py ~4709 (tooltip wording, nit) — The freeze tooltip asserts "exporting solar surplus only" in present tense even when a freeze window runs with zero PV and nothing is being exported (the inverter was only told to disable charging / drop force export). The existing "Freeze exporting" status had the same semantics, but the new tooltip states it as fact to the user; similarly, a restart-before-first-execute keeps the previous run's state=on/attribute pair until the first execute_plan republishes (pre-existing staleness, now an affirmative wrong claim).

Run notes: all finder angles completed (several after the review compiled); every angle was also executed and verified directly. The one candidate claiming the new assertion fails for "Hold exporting" scenarios was checked against the test code and refuted (case-sensitive substring). No flipped config defaults, no setup/teardown asymmetry, no dropped guards in moved code. Test execution was not possible in this session (permissions), so verification is static; run ./run_all --test execute --test web_functions on the branch to confirm.

@springfall2008 springfall2008 removed the BOT_REVIEW Trigger an autotriage label Sep 17, 2026
@chalfontchubby
chalfontchubby marked this pull request as ready for review September 17, 2026 17:12
@springfall2008
springfall2008 requested a lite review from Copilot September 19, 2026 14:45

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Mixed-inverter precedence is not covered by a regression test.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Updates the web header to distinguish active and freeze exports using status attributes, colour classes, and tooltips.

Changes:

  • Publishes export_status from execution state.
  • Adds coloured active/freeze export icons with dark-mode styling.
  • Adds documentation and tests.
File Description
apps/​predbat/​const.py Defines export status constants.
apps/​predbat/​execute.py Tracks export mode and publishes precedence.
apps/​predbat/​output.py Adds the sensor attribute.
apps/​predbat/​web.py Applies status-specific icon classes and tooltips.
apps/​predbat/​web_helper.py Adds light/dark CSS rules.
apps/​predbat/​tests/​test_execute.py Tests published statuses.
apps/​predbat/​tests/​test_web_functions.py Tests rendering and CSS.
docs/​output-data.md Documents the new attribute.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apps/predbat/execute.py
Comment on lines +804 to +807
if exporting_to_target:
export_status = EXPORT_STATUS_TARGET
elif exporting_freeze:
export_status = EXPORT_STATUS_FREEZE

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Differentiate Export icon colour for Active Export vs Freeze Export

2 participants