feat: adopt RefreshPartiallySucceeded/RefreshFailed from givenergy-modbus #125#71
Conversation
…dbus #125 Migrate the coordinator and config flow off the deprecated refresh_plant() onto load_config()/refresh(), and handle the new partial/total poll exceptions so one offline device no longer discards every other reading. - coordinator: steady-state partials serve exc.plant (the dropped device's entities freeze at last-known values instead of going unavailable) and bump a new partial_failures counter; (re)connect-seed partials fail and retry rather than locking in a half-populated initial plant; timeout-driven RefreshFailed keeps the existing tolerance window, non-timeout causes reset the client immediately - sensor/dashboard: new Partial Refresh Failures diagnostic sensor whose attributes name the dropping device(s) from the ReadFailure records; added to the Integration Health card; dashboard schema bumped to v3 - config_flow: _test_connection accepts a partial (the serial survives a peripheral drop); RefreshFailed still maps to cannot_connect - pin givenergy-modbus>=2.1.0a5 (the release that ships #125) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 40 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR refactors the GivEnergy Home Assistant integration to support graceful handling of partial refresh outcomes. It replaces the ChangesPartial Refresh Failure Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for handling partial refresh successes (via RefreshPartiallySucceeded) in the GivEnergy Local integration, allowing the system to serve last-known data for flaky peripheral devices instead of failing the entire poll. It also adds a new diagnostic sensor to track partial failures. The review feedback points out a critical bug in dashboard.py where the entity key was incorrectly referenced as "partial_refresh_failures" instead of "partial_failures". Additionally, it suggests cleaner handling of attributes_fn by defaulting to None instead of a dummy lambda, and recommends defensively extracting .value from f.request_type in case it is upgraded to an Enum in the future.
|
I reviewed this and do not have any actionable concerns. The new partial/total refresh paths line up with the One residual note: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/conftest.py (1)
172-177: ⚡ Quick winConstrain
mock_clientwithspec_setso deprecated client APIs fail fast
tests/conftest.py::mock_clientbuilds a bareAsyncMock(), which auto-synthesizes attributes likeclient.refresh_plant; that can let regressions slip through without failing the migration tests. Add aspec_setso the mock enforces the intended client surface area (refresh+load_config, plus the other explicitly used methods/attrs).Possible tightening
`@pytest.fixture` def mock_client(mock_plant) -> AsyncMock: - client = AsyncMock() + client = AsyncMock( + spec_set=[ + "connected", + "plant", + "refresh", + "load_config", + "connect", + "detect", + "close", + "one_shot_command", + ] + ) client.connected = True client.plant = mock_plant client.refresh = AsyncMock(return_value=mock_plant) client.load_config = AsyncMock(return_value=mock_plant) client.connect = AsyncMock() client.detect = AsyncMock() client.close = AsyncMock() client.one_shot_command = AsyncMock()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 172 - 177, The mock_client fixture currently creates a freeform AsyncMock() that can auto-synthesize unexpected attributes; update the mock creation in mock_client to pass a spec_set that matches the real client API surface you rely on (at minimum include attributes/methods: connected, plant, refresh, load_config and any other methods/attrs used in tests) so attempts to access deprecated or misspelled APIs will raise AttributeError; ensure you construct the AsyncMock with spec_set and then set client.connected, client.plant and client.refresh/load_config return values as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/givenergy_local/config_flow.py`:
- Around line 101-110: When catching RefreshPartiallySucceeded in the config
flow after calling client.refresh(), ensure the partial snapshot actually
contains an inverter serial before returning it: in the except
RefreshPartiallySucceeded as exc branch (and where you set plant = exc.plant)
check if plant and plant.inverter_serial_number are present; if the
inverter_serial_number is missing or falsy, treat it like a full failure (e.g.,
re-raise RefreshFailed or return a value that triggers the existing
"cannot_connect" handling) instead of returning a None/empty unique ID.
In `@custom_components/givenergy_local/coordinator.py`:
- Around line 117-118: On a clean successful refresh, clear any stale
partial-failure diagnostics so old dropped-device details are not reported;
inside the coordinator's success path (e.g., in _mark_success or immediately
where self._mark_success(plant) returns a clean plant) reset the plant's
last_partial_failures tracker (the last_partial_failures attribute on the
plant/state object) to an empty structure or None so prior partial-poll failures
are removed after a full success.
In `@tests/test_coordinator.py`:
- Around line 642-661: In test_active_mode_nth_tick_is_full_refresh replace the
non-ASCII en dash in the loop comment inside the for loop (the "0–10" text) with
an ASCII hyphen ("0-10") so Ruff RUF003 is resolved; update the comment located
next to the for _ in range(11) loop and then run your ruff fix/format commands
before committing.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 172-177: The mock_client fixture currently creates a freeform
AsyncMock() that can auto-synthesize unexpected attributes; update the mock
creation in mock_client to pass a spec_set that matches the real client API
surface you rely on (at minimum include attributes/methods: connected, plant,
refresh, load_config and any other methods/attrs used in tests) so attempts to
access deprecated or misspelled APIs will raise AttributeError; ensure you
construct the AsyncMock with spec_set and then set client.connected,
client.plant and client.refresh/load_config return values as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39a74c3c-6ccd-4339-bd0a-376cfe3a60a3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
custom_components/givenergy_local/config_flow.pycustom_components/givenergy_local/coordinator.pycustom_components/givenergy_local/dashboard.pycustom_components/givenergy_local/manifest.jsoncustom_components/givenergy_local/sensor.pypyproject.tomltests/conftest.pytests/test_config_flow.pytests/test_coordinator.pytests/test_sensor.py
- attributes_fn defaults to None (not a dummy lambda); extra_state_attributes guards on it, so sensors without attributes skip the call entirely - defensively extract .value from ReadFailure.request_type in case the library later upgrades it from str to an Enum Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: - config_flow: a partial that dropped the inverter read itself returns no serial → cannot_connect, rather than creating an entry with a blank unique ID - coordinator: clear last_partial_failures on a fully clean poll so the diagnostic sensor stops naming devices that have since recovered (the cumulative partial_failures counter is untouched) - tests for both; ASCII hyphen in a loop comment Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…essions Per review: the fixture's spec_set omits the deprecated refresh_plant() so any lingering call fails fast with AttributeError instead of silently auto-mocking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
What this does
Adopts the new poll contract from givenergy-modbus#125. Previously the first register read to time out aborted the whole poll, so one offline battery discarded every reading that did come back. Now
refresh()/load_config()raiseRefreshPartiallySucceeded(carrying the data that arrived) orRefreshFailed(nothing came back), and this integration handles them so a single flaky device no longer takes the rest of the plant down with it.This also retires the last consumer of the deprecated
refresh_plant()(removed in modbus 3.0).Behaviour
Coordinator
exc.plant. The dropped device's entities freeze at their last-known register values (the library's cache retains them) and stay available, rather than the whole device going unavailable for the tick. A new cumulativepartial_failurescounter records it.ConfigEntryNotReady(HA retries setup); in-process reconnect → the existing tolerance window serves the pre-disconnect snapshot.detect()having already gated device presence is what makes a seed partial most likely transient.RefreshFailed→ I kept the existing timeout-tolerance window when every underlying cause is a timeout (transient blip, serve last-known up to the threshold); any non-timeout cause resets the client and fails immediately. This deliberately diverges from the simplerRefreshFailed → UpdateFailedin the modbus PR's migration note, to preserve resilience this integration already had.Diagnostics — a new Partial Refresh Failures sensor (always-available, like the other coordinator diagnostics). Because a flaky device stays available with stale values, the entity-availability channel stays silent — so the sensor's attributes name the dropping device(s) (
0x34, bank, request type) from the structuredReadFailurerecords. Added to the auto-generated Integration Health card; dashboard schema bumped to v3 (existing installs get the usual Repairs prompt to regenerate).config_flow —
_test_connectionaccepts a partial, since it only needs to identify the inverter (device0x32), which virtually always survives a peripheral drop.RefreshFailedstill maps tocannot_connect.Testing
RefreshFailedsplit.Follow-up
The interim tradeoff is that a long partial shows a frozen value with no intrinsic "stale" marker — the counter and logs are the only signal. Decisively offlining a single silent device needs per-bank freshness timestamps, tracked in givenergy-modbus#65; the
ReadFailure.device_addresssurfaced here maps straight onto that once it lands.Dependency
Requires
givenergy-modbus>=2.1.0a5(the release that ships #125); pin bumped inpyproject.tomlandmanifest.json.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores