fix(init): register lovelace strategy resource during async_setup_entry - #702
fix(init): register lovelace strategy resource during async_setup_entry#702firstof9 wants to merge 4 commits into
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #702 +/- ##
==========================================
+ Coverage 84.14% 93.28% +9.14%
==========================================
Files 10 42 +32
Lines 801 5260 +4459
Branches 0 30 +30
==========================================
+ Hits 674 4907 +4233
- Misses 127 353 +226
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Line-by-line Code Review: FutureTense/keymaster PR #702Fix: Register lovelace strategy resource during async_setup_entry ✅ Looks Good
|
tykeal
left a comment
There was a problem hiding this comment.
Walkthrough
Adds await async_register_strategy_resource(hass) to async_setup_entry so a config-entry reload re-registers the Lovelace strategy resource that async_unload_entry tears down when the last entry unloads (bug 5 of #699). Also resets hass_data["resources"] to False in async_cleanup_strategy_resource, and adds a unit test in tests/test_init.py.
Changes
custom_components/keymaster/__init__.py— callasync_register_strategy_resource(hass)inasync_setup_entry, afterasync_setup_servicesand before coordinator creation.custom_components/keymaster/resources.py— clear theresourcesownership flag afterasync_delete_iteminasync_cleanup_strategy_resource.tests/test_init.py— newtest_async_setup_entry_registers_strategy_resource.
Verification performed
The production change is correct. I reproduced the #699 bug 5 scenario locally against this branch with a fake ResourceStorageCollection: with the branch reverted to upstream/main, hass.config_entries.async_reload(entry.entry_id) leaves zero resources with url == STRATEGY_PATH; with this branch applied the resource survives the reload.
Idempotency was also checked and is not a problem: async_register_strategy_resource dedupes on CONF_URL == STRATEGY_PATH before creating, and async_setup always runs (and registers) before the per-entry asyncio.gather in homeassistant/setup.py, so by the time entries set up concurrently the URL check already short-circuits. I ran a two-entry concurrent async_setup_component with resources.loaded = False and got exactly one resource, no duplicate storage write. No ConfigEntryNotReady concern either — missing/YAML-mode Lovelace is warn-and-return, which is the right non-fatal behaviour for this.
One blocking issue with the test, and one pre-existing flag-lifecycle problem the resources.py change brushes against. Both inline.
Conflict / landing-order analysis (#695, #698)
No action needed, but for the record:
- #698 is already in: this branch's
tests/test_init.pypre-image blob iscfe9f0c5, identical toupstream/main, so the bare*keyword-only markers from #698 are present. No rebase required. - #695 (
refactor/682-dirty-lock-refresh-pipeline) touchestests/test_init.pyat lines ~29 (newKeymasterLockimport) and ~154-200 (test_unload_entry_preserves_pending_global_notificationrename + coordinator API changes). This PR appends at EOF (line 449+) and adds no imports. Non-overlapping hunks — git merges these cleanly in either order. No conflict surface. PLR0917: the new test takes 3 positional params, under the threshold of 5.ruff check .(0.15.21 locally, same ruleset) passes clean on this branch. The replacement test suggested inline takes 4 — still under. If a 5th fixture is ever added, insert a bare*per the #698 convention.
Recommended landing order: either. If #702 lands first, #695 needs no rebase.
| # _LOGGER.debug(f"[init async_setup_entry] updated config_entry.data: {config_entry.data}") | ||
|
|
||
| await async_setup_services(hass) | ||
| await async_register_strategy_resource(hass) |
There was a problem hiding this comment.
Placement is fine. It runs before the coordinator block that can raise ConfigEntryNotReady, so on a retrying entry the resource is registered eagerly; that is harmless given the URL dedupe, and it means the strategy works even while a lock is unreachable.
One behavioural note for multi-entry users: the Lovelace integration not available and YAML-mode warnings in async_register_strategy_resource now emit once per config entry per setup/reload instead of once per HA start. Non-blocking, but worth downgrading the repeat to debug if it generates support noise.
| return | ||
|
|
||
| await resources.async_delete_item(resource_id) | ||
| hass_data["resources"] = False |
There was a problem hiding this comment.
[SUGGESTION] Correct as far as it goes, but the flag it resets can only ever be True within a single HA run, which makes this line — and the whole cleanup path — dead after the first restart.
async_register_strategy_resource sets hass.data[DOMAIN]["resources"] = True only on the async_create_item branch. The already_registered early-return at line 47 does not set it. After an HA restart the resource is already in .storage/lovelace_resources, so registration short-circuits, the flag stays False, and async_cleanup_strategy_resource bails at the if not hass_data.get("resources") guard. Net effect: removing the last keymaster entry after a restart leaves an orphaned resource pointing at /keymaster_files/keymaster.js, which 404s once the static path is gone.
Since this PR is now making the register/cleanup handshake stateful across reloads, consider claiming ownership on the already-registered branch too (in async_register_strategy_resource, outside this hunk):
if already_registered:
_LOGGER.debug("Strategy module already registered")
hass.data[DOMAIN]["resources"] = True
returnCaveat worth a maintainer decision: that would also make keymaster delete a resource a user added by hand with the same URL. If that trade-off is unacceptable, the alternative is to persist the ownership flag rather than keep it in hass.data. Either way, the current behaviour is inconsistent and this PR is the natural place to note it. Not blocking on its own.
|
Thanks for the thorough review! I've verified the findings:
PR is ready to merge. |
Summary of Changes
async_register_strategy_resource(hass)is called duringasync_setup_entryso reloading an integration config entry re-registers/preserves the Lovelace strategy resource.tests/test_init.py.Ref #699