Skip to content

Fix service lifecycle and action refresh handling#174

Open
ghislaingaillot wants to merge 1 commit intoad-ha:mainfrom
ghislaingaillot:fix/service-lifecycle-refreshes
Open

Fix service lifecycle and action refresh handling#174
ghislaingaillot wants to merge 1 commit intoad-ha:mainfrom
ghislaingaillot:fix/service-lifecycle-refreshes

Conversation

@ghislaingaillot
Copy link
Copy Markdown

Review Fixes - 2026-03-21

This note documents the fixes applied after the repository review and explains
why each change was made.

1. Non-blocking action refresh scheduling

Files:

  • custom_components/mg_saic/coordinator.py

What changed:

  • Replaced the blocking schedule_action_refresh() implementation with a
    background task.
  • Added task cancellation and coordinator shutdown cleanup.

Why:

  • The previous implementation awaited multiple sleep() calls directly inside
    service/entity actions. A lock, climate or charging command could therefore
    remain pending for minutes before returning control to Home Assistant.
  • The new version schedules the follow-up refresh sequence in the background,
    keeps only one active action-refresh sequence per coordinator and cancels it
    safely when the entry is unloaded.

2. Correct multi-VIN service routing

Files:

  • custom_components/mg_saic/__init__.py
  • custom_components/mg_saic/services.py

What changed:

  • Added clients_by_vin alongside coordinators_by_vin.
  • Registered services only once for the integration domain.
  • Updated every service handler to resolve the correct client/coordinator from
    the VIN provided in the service call.

Why:

  • Services were previously registered once per config entry and closed over the
    client/coordinator of that entry. With multiple vehicles, the global service
    handlers could operate on the wrong coordinator and refresh the wrong VIN.
  • Resolving resources by VIN at call time makes service behavior deterministic.

3. Proper unload and resource cleanup

Files:

  • custom_components/mg_saic/__init__.py
  • custom_components/mg_saic/coordinator.py
  • custom_components/mg_saic/api.py

What changed:

  • Removed entry-specific client and coordinator objects on unload.
  • Removed VIN indexes on unload.
  • Added coordinator shutdown logic to cancel background tasks and scheduled
    refresh callbacks.
  • Closed the API client cleanly and made close() tolerate an uninitialized
    session.

Why:

  • The previous unload path left stale coordinators behind and could keep the
    service registry alive even when the integration had no remaining entries.
  • Cleaning the coordinator tasks and API client prevents leaked callbacks and
    stale references.

4. Heated seats service and switch fixes

Files:

  • custom_components/mg_saic/services.py
  • custom_components/mg_saic/services.yaml
  • custom_components/mg_saic/switch.py

What changed:

  • Aligned the service schema with the documented left_level and
    right_level fields.
  • Fixed the heated seat switch constructor to use stable seat identifiers.
  • Mapped each heated seat switch to the correct status attribute from the
    vehicle payload.

Why:

  • The service schema and service handler disagreed about the accepted payload.
  • The seat switches used inconsistent identifiers and therefore never reached
    the branch that sends the command, while also reading the wrong status field.

5. Sunroof service compatibility

Files:

  • custom_components/mg_saic/api.py
  • custom_components/mg_saic/services.py

What changed:

  • Made control_sunroof() accept either a boolean or an "open"/"close"
    string and normalize both forms internally.

Why:

  • The service layer passed a boolean while the API wrapper expected a string,
    which could invert behavior and always resolve to closing the sunroof.

6. Preserve the configured default polling interval

Files:

  • custom_components/mg_saic/coordinator.py

What changed:

  • Added a dedicated default_update_interval.
  • Reused that value whenever the vehicle returns to the idle/default polling
    state.

Why:

  • The previous logic fell back to the constant UPDATE_INTERVAL, which meant a
    user-defined polling interval was silently lost after state transitions.

7. API errors now propagate to callers

Files:

  • custom_components/mg_saic/api.py

What changed:

  • Re-raised exceptions in lock_vehicle(), unlock_vehicle() and
    open_tailgate().

Why:

  • The service/entity layers were able to log success after a failed API call
    because those methods swallowed the exception after logging it once.

Validation performed

  • python3 -m compileall custom_components/mg_saic
  • python3 -m unittest discover -s tests -p "test_*.py"

8. Lightweight automated tests

Files:

  • custom_components/mg_saic/logic.py
  • tests/test_logic.py
  • .github/workflows/python-tests.yaml

What changed:

  • Extracted pure helper logic for sunroof normalization and update interval
    selection.
  • Added standard-library unit tests for these rules.
  • Added a CI workflow to run the tests on pushes and pull requests.

Why:

  • The repository only had HACS and Hassfest validation. These new tests cover
    two regression-prone code paths without requiring a full Home Assistant test
    harness.

Remaining gap

  • The repository still does not include end-to-end Home Assistant tests for the
    config flow, entity lifecycle and service registration. The most critical
    pure logic now has automated coverage, but runtime HA integration coverage is
    still a future improvement.

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.

1 participant