Skip to content

fix(init): register lovelace strategy resource during async_setup_entry - #702

Open
firstof9 wants to merge 4 commits into
FutureTense:mainfrom
firstof9:fix/lovelace-strategy-resource-lifecycle
Open

fix(init): register lovelace strategy resource during async_setup_entry#702
firstof9 wants to merge 4 commits into
FutureTense:mainfrom
firstof9:fix/lovelace-strategy-resource-lifecycle

Conversation

@firstof9

@firstof9 firstof9 commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary of Changes

  • Ensure async_register_strategy_resource(hass) is called during async_setup_entry so reloading an integration config entry re-registers/preserves the Lovelace strategy resource.
  • Add unit test coverage in tests/test_init.py.

Ref #699

@codecov-commenter

codecov-commenter commented Aug 2, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.28%. Comparing base (cdb4922) to head (7f6fb54).
⚠️ Report is 206 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

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     
Flag Coverage Δ
python 93.17% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@firstof9
firstof9 requested review from raman325 and tykeal August 2, 2026 03:18
@secondof9

Copy link
Copy Markdown

Line-by-line Code Review: FutureTense/keymaster PR #702

Fix: Register lovelace strategy resource during async_setup_entry

✅ Looks Good

  • custom_components/keymaster/__init__.py:141await async_register_strategy_resource(hass) is placed correctly at the top of async_setup_entry (after async_setup_services and before coordinator initialization). This ensures the resource is registered on config entry reload/re-setup.
  • custom_components/keymaster/resources.py:85-87async_cleanup_strategy_resource correctly short-circuits when hass_data doesn't indicate auto-registration (resources: False). This prevents leaking the resource when cleanup is not required.
  • custom_components/keymaster/resources.py:97-98 — Cleanup deletes resource and sets flag to False atomically.
  • custom_components/keymaster/__init__.py:78hass.data.setdefault(DOMAIN, {"resources": False}) correctly initializes the tracking flag in async_setup.
  • tests/test_init.py:449-467 — New test test_async_setup_entry_registers_strategy_resource correctly mocks async_register_strategy_resource, exercises async_setup_entry, and asserts the mock was called. Validates the integration point.
  • tests/test_resources.py — All existing tests for async_register_strategy_resource and async_cleanup_strategy_resource are well-structured with proper mocking. They validate the full lifecycle (register, duplicate detection, YAML mode warning, cleanup).
  • Diff minimal and focused: Only 3 files changed, 19 lines added across the entire repo.
  • Fix addresses the known issue ISSUE: Zigbee2MQTT provider never receives lock state — wrong subscribe topic, action vocabulary, and lock_state desync (4 chained bugs) + reload deletes Lovelace strategy resource #699 — strategy resource preservation across config entry reloads.

⚠️ Warnings

  • tests/test_init.py:~421-451 — The existing test_async_setup_entry_setup_success_false test does not assert that async_register_strategy_resource was called during setup, even though the mock is patched (the function is called at line 141 of init.py before coordinator init). This means the test doesn't fully validate the registration path. However, this is intentional since the test focuses on the cleanup/rollback path after coordinator failure.
  • tests/test_init.py:407hass.data.setdefault(DOMAIN, {}) doesn't match the real async_setup flow which does hass.data.setdefault(DOMAIN, {"resources": False}). The resources flag isn't set, but since this test doesn't check for it, it's acceptable.

💡 Suggestions

  • Add an assertion in test_async_setup_entry_setup_success_false to confirm async_register_strategy_resource is called during setup (the function is invoked at init.py:141 before coordinator setup).
  • Consider mocking async_setup_services in the new test_async_setup_entry_registers_strategy_resource test since async_setup_entry calls it.
  • The async_register_strategy_resource function (resources.py:30-67) is already well-tested with 6 dedicated tests covering all code paths. The new test only validates the integration point, which is appropriate.

Summary

This is a well-structured fix for the known issue (#699). The strategy resource registration is now performed during async_setup_entry, ensuring it persists across config entry reloads. The code is clean, the tests are comprehensive, and the existing resource management functions handle edge cases properly (YAML mode, not registered, not found). The only improvement is an assertion in the existing test_async_setup_entry_setup_success_false test to confirm async_register_strategy_resource is invoked during setup.

PR is ready to merge. All changes are minimal, well-tested, and address the architectural gap identified in #699.

@tykeal tykeal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 — call async_register_strategy_resource(hass) in async_setup_entry, after async_setup_services and before coordinator creation.
  • custom_components/keymaster/resources.py — clear the resources ownership flag after async_delete_item in async_cleanup_strategy_resource.
  • tests/test_init.py — new test_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.py pre-image blob is cfe9f0c5, identical to upstream/main, so the bare * keyword-only markers from #698 are present. No rebase required.
  • #695 (refactor/682-dirty-lock-refresh-pipeline) touches tests/test_init.py at lines ~29 (new KeymasterLock import) and ~154-200 (test_unload_entry_preserves_pending_global_notification rename + 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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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
        return

Caveat 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.

Comment thread tests/test_init.py Outdated
@firstof9

firstof9 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough review!

I've verified the findings:

  • async_register_strategy_resource(hass) is correctly invoked at the beginning of async_setup_entry so re-registering or reloading config entries preserves the Lovelace strategy resource.
  • All lifecycle functions (async_cleanup_strategy_resource, atomic tracking flag updates, and entry setup/unload hooks) operate cleanly as validated in tests/test_init.py and tests/test_resources.py.

PR is ready to merge.

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

Labels

bugfix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants