Skip to content

Bring the integration in line with Home Assistant integration standards #33

Description

@turbo5000c

Prioritized plan, no code yet. Every line reference below was checked against main at a611391. Each item states the standard it misses, the evidence, the proposed fix, and the migration risk.

Ordering is by user impact, not effort. P1 items are correctness or safety; P2 is registry integrity and gates several later fixes; P3–P4 are entity-contract and data-robustness.


P0 — Explicit non-goal, recorded so it does not get "fixed" later

Do not rewrite entity_id from the integration. The entity registry docs state the guarantee plainly:

Being registered has the advantage that the same entity will always get the same entity ID.

That stability is a promise to the user. An integration that silently renames entities on update breaks every automation and dashboard referencing them, with no warning and no opt-out. unique_id is the integration's key and is migratable; entity_id is the user's.

scripts/rename_noaa_entities.py (added in #32) is the correct shape for this: a standalone, opt-in, dry-run-by-default tool the user runs deliberately. It is not imported by the integration and must not become so. The in-app equivalent, if wanted, is a repairs issue offering the rename as a user-confirmed fix — see P2.4.


P1 — Correctness and safety

P1.1 No entity in the integration implements available

grep -c "def available" custom_components/ returns 0 across all 23 modules.

Nothing ever goes unavailable. When a fetch fails, entities keep publishing their last value — or a fabricated one — and stay available, so a consumer cannot distinguish "no data" from "data says everything is fine".

Standard: entities backed by a DataUpdateCoordinator expose available derived from coordinator.last_update_success (CoordinatorEntity already provides this; the integration's entities override or bypass it).

Fix: stop overriding availability where CoordinatorEntity supplies it, and add explicit available where a sensor depends on a sub-fetch that can fail independently of its coordinator.

Risk: low. Entities that previously showed stale values will show unavailable — visible, but correct.


P1.2 Surf data fails open, on a device_class: safety entity

This is the one to fix first.

  • coordinator.py:350-353 — on any SRF fetch error the except block fabricates result["forecast_text"] = "" and returns normally. SurfCoordinator never raises UpdateFailed on any path, so last_update_success stays True.
  • parsers.py:398parse_rip_current_risk ends in an unconditional return "Low", so empty text parses as low risk.
  • sensors/surf.py:52-53 and binary_sensor.py:87-100 consume that empty string.

Failure scenario: forecast.weather.gov returns a 503 during an active HIGH RIP CURRENT RISK statement. sensor.noaa_{office}_surf_rip_current_risk flips HighLow and binary_sensor.noaa_{office}_surf_unsafe_to_swim (device class safety) flips to off, both still available. An automation gated on either reads a clean bill of health during the hazard.

Fix: omit forecast_text from the result on failure rather than substituting ""; return None from the sensor state and is_on when the text is absent; set risk_level to Unknown. Do not raise UpdateFailed in that except block — CO-OPS water temperature and NDBC wave height are fetched in the same cycle and would be discarded with it.

Risk: low, and strictly in the safe direction.


P1.3 Three coordinators latch "already resolved" inside their except block

file:line flag effect of one transient failure
coordinator.py:503 self._urls_fetched = True Forecast URLs stay None; every later poll raises UpdateFailed. Extended + hourly forecast sensors and the weather entity's forecast are dead for the life of the entry.
coordinator.py:586 self._grid_fetched = True Gridpoint URL stays None; cloud cover permanently unavailable.
coordinator.py:306 self._station_fetched = True Station stays at the office default; observations silently report the office airport rather than the station nearest the configured coordinates. For an office absent from OFFICE_STATION_IDS, it is a permanent outage instead.

Nothing anywhere resets these flags, so recovery requires a manual reload or an HA restart.

Fix: set the flag only on success (the success assignments already exist at 300, 497, 580). Better, make the guard outcome-based — if not self._urls_fetched or not self._forecast_url: — so a 200 response missing the expected key is also retried.

Risk: none. Strictly widens retry.


P1.4 Setup swallows every coordinator failure

__init__.py:134await asyncio.gather(*refresh_tasks, return_exceptions=True).

Exceptions are discarded, so a failed first refresh leaves the entry LOADED with a poisoned coordinator instead of entering SETUP_RETRY. The user sees a working integration with dead entities and no retry.

Standard: await coordinator.async_config_entry_first_refresh(), which raises ConfigEntryNotReady and lets HA retry setup with backoff.

Fix: use async_config_entry_first_refresh() for coordinators the entry genuinely cannot function without; keep the tolerant path only for genuinely optional ones (meteor, surf for inland offices), and log those explicitly.

Risk: medium — an entry that currently "loads" against a flaky NWS API will now retry instead. Correct, but a visible behaviour change worth a changelog note.


P2 — Registry integrity

P2.1 ~20 unique_ids key on office code alone, so two entries in one office collide

binary_sensor.py:84, 168, 262, 355, 436, 534; sensors/surf.py:36, 97, 171; the eight space-weather sensors; the three meteor sensors; noaa_{office}_radar_timestamp; noaa_{office}_forecast_discussion; and the two space images.

Meanwhile config_flow.py:198-201 sets the entry unique_id to noaa_{office}_{lat}_{lon} — so two locations under one NWS office is a supported install, and _abort_if_unique_id_configured() does not fire.

Failure scenario: a user adds two beaches served by ILM. The second entry's ~20 entities are refused by the registry (unique_id is scoped to platform+domain, not config entry). They get the 13 location-scoped weather sensors and nothing else — no surf, space-weather, meteor, alert binary, radar-timestamp or discussion entities — with only an already registered log line.

The author already recognised this hazard for the global hurricane entities (HURRICANE_SENSORS_ADDED_KEY / HURRICANE_IMAGES_ADDED_KEY) but did not extend it here.

Standard: the docs list "Latitude and Longitude or other unique Geo Location" as acceptable, and "Config Entry ID" as a last resort. weather_observations.py:93-104 already builds a lat/lon suffix — reuse it.

Risk: high, and this is the gating item. Changing a unique_id orphans the existing registry row unless migrated. Must ship together with P2.3.


P2.2 Changing coordinates in the options flow orphans 14 entities

config_flow.py:326 writes new lat/lon into options and __init__.py:156 reloads. The 4-dp lat/lon is baked into 14 unique_ids (weather_observations.py:93-104, weather_extra.py:94/174/255, alerts.py:68, forecasts.py:43, weather.py:112-116).

Old registry rows survive the reload, so async_generate_entity_id finds the IDs taken and creates ..._2 variants. Every dashboard card and automation pointing at the originals now references permanently-unavailable orphans.

Fix: migrate the affected unique_ids in an options-update handler before async_reload.

Risk: medium. Same migration machinery as P2.1.


P2.3 No async_migrate_entry — there is no upgrade path at all

config_flow.py:132 declares VERSION = 1, but __init__.py defines no async_migrate_entry. There is currently no supported way to change any unique_id without orphaning every existing install.

Fix: add async_migrate_entry with VERSION/MINOR_VERSION, rewriting affected rows via entity_registry.async_update_entity(entity_id, new_unique_id=...). This must land before or with P2.1 and P2.2.

Risk: this is the piece that has to be right first time — a bad migration is not recoverable from the user side. Wants its own tests against a synthetic registry.


P2.4 Optional: repairs issue for stale entity IDs

The in-app, standards-conforming replacement for scripts/rename_noaa_entities.py: detect entities whose entity_id no longer matches what a fresh install would produce, raise an issue_registry issue, and offer the rename as a user-confirmed repair flow.

Keeps the user in control, which the script does by being manual and the automatic approach does not.

Risk: low — nothing happens without an explicit click.


P3 — Entity contract

P3.1 _map_condition maps "Partly Cloudy" and "Mostly Cloudy" to cloudy

weather.py:463 tests bare "cloudy" in desc_lower before the specific branches at 464-467, contradicting the # order matters, check most specific first comment at 442. Verified by executing the real static method: Partly Cloudy -> cloudy, Mostly Cloudy -> cloudy. The two most common NWS sky descriptions are wrong, every day, on the weather entity and every forecast period.

Fix: move the specific branches above the generic one; give mostly cloudy its own branch.

Risk: none.


P3.2 Image entities never refresh, and their state is permanently unknown

Upstream ImageEntity sets _attr_should_poll = False, and entity_platform only arms a polling timer when some entity reports should_poll — so SCAN_INTERVAL in image.py is dead code and async_update() runs exactly once. Separately, _attr_image_last_updated is never assigned anywhere, and ImageEntity.state is @final and derived solely from it, so every image entity reads unknown forever.

Fix: _attr_should_poll = True on the seven classes, and set _attr_image_last_updated when the URL changes.

Risk: low. Note the source-level guard added in #32 (tests/test_image.py) bans async_write_ha_state anywhere in image.py; the polling fix does not need it, but an async_track_time_interval approach would.


P3.3 Weather forecast card never receives pushed updates

weather.py:64 subclasses plain CoordinatorEntity, and async_update_listeners appears nowhere in the repo, while _attr_supported_features advertises FORECAST_DAILY | FORECAST_HOURLY. A subscribed forecast card gets the initial payload and then nothing until the tab is reloaded.

Fix: call async_update_listeners(("daily", "hourly")) from _handle_forecast_update, or migrate to CoordinatorWeatherEntity.

Risk: low.


P4 — Data robustness

  • parsers.py:594props.get('description', '')[:300] — the '' default does not apply when the key is present with value null. Reproduced: raises TypeError: 'NoneType' object is not subscriptable, taking out the alerts sensor's state, attributes and icon together. The sibling instruction is guarded at 575-576, which is direct evidence nulls occur here. Same unguarded slice at binary_sensor.py:191, 285, 378, 460.
  • sensors/space_weather.py:72, 107 read dst_data[0] while :172, 207 read kp_data[-1] from the same coordinator. SWPC serves both feeds oldest-first, so the Dst sensors report the oldest sample in the file rather than the newest.
  • sensors/space_weather.py:268AuroraNextTimeSensor lacks the None/non-numeric Kp guard both sibling aurora sensors have; a null kp_index raises TypeError instead of yielding an unknown state.
  • sensors/hurricanes.py:62 — reports 0 active alerts / Quiet when the alerts fetch fails but the storms fetch succeeds, because the coordinator only raises UpdateFailed if both fail. Same fail-open shape as P1.2.
  • parsers.py:657precip_prob.get('value', 0) returns None, not 0, for the {'value': null} shape api.weather.gov routinely sends.
  • parsers.py:194extract_storm_scale maps "extreme" to S4, but get_severity_level in the same module defines S5 = Extreme, S4 = Severe. The most severe solar radiation storms are reported one scale low.

Suggested sequencing

  1. P1.2 + P1.3 — safety and permanent-outage bugs, no migration needed. Smallest diff, highest impact.
  2. P1.1 + P1.4 — availability and setup retry. Behaviour-visible; changelog note.
  3. P2.3 — migration machinery, with tests, on its own.
  4. P2.1 + P2.2 — unique_id fixes riding on P2.3.
  5. P3 / P4 — independent of everything above, can be picked off in any order.

P0 stands throughout: none of this should silently rename a user's entities.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions