Phase 3.0 UAT fixes — metrics_won + duplicate entities + Flow Power default#53
Merged
Merged
Conversation
Three bugs surfaced during the post-Phase-3.0d UAT walkthrough on a fresh-wiped HA install. None caught by the test suite — they're sensor-rendering-vs-coordinator-data shape mismatches that only manifest at runtime. 🟢 metrics_won returned fake "0/3" when Amber not configured - coordinator's `metrics_won = None` round-2 fix was correct, but `MetricsWonSensor.native_value`'s inline-compute fallback returned the literal string "0/3" when amber_import or current_plan_import was None. Now returns None — sensor renders "unavailable" honestly instead of fake-comparing against a phantom zero-cost provider. 🟢 Duplicate sensor entity sets for the user's current plan - Generic per-provider sensors (cost / import_rate / export_rate) were registered for the user's CURRENT plan AND comparators. The current plan already has hardcoded `current_plan_*` sensors, so the generic ones produced duplicates like `sensor.pricehawk_globird_zerohero_residential_flexible_rate_united_energy_cost_today`. sensor.py now skips the current plan in the providers loop; comparators (Amber, FlowPower, LocalVolts) keep their per-provider entities. 🟢 Flow Power default-OFF on new installs - Wizard defaulted `flow_power_enabled = True` regardless of user choice. Every install got a placeholder `sensor.pricehawk_flow_power_cost_today: $1.0` whether the user cared or not. Now opt-in: enabled only when user picks Flow Power as the primary at credentials, OR enables it via the comparators OptionsFlow step. Same default flipped in the comparators step schema. 623/623 non-pydantic tests pass; ruff clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdjusts metrics_won sensor behavior to correctly represent unavailable Amber comparisons, avoids registering duplicate provider sensors for the current plan, and changes Flow Power to be opt-in instead of always enabled in the config flow. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Accessing
coordinator._current_plan_provider.idrelies on a private attribute and assumes the provider IDs inproviders_blockare the same type; consider using a public accessor (or storing the current provider ID in the coordinator data) and defensively handling missing/mismatched IDs before comparing. - In
MetricsWonSensor.native_value, the inline fallback now returnsNonewhen Amber is not configured but still performs numeric comparisons on other fields; it may be safer to short-circuit and returnNoneif any of the required rate/daily fields are missing or not numeric to avoid subtle runtime type issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Accessing `coordinator._current_plan_provider.id` relies on a private attribute and assumes the provider IDs in `providers_block` are the same type; consider using a public accessor (or storing the current provider ID in the coordinator data) and defensively handling missing/mismatched IDs before comparing.
- In `MetricsWonSensor.native_value`, the inline fallback now returns `None` when Amber is not configured but still performs numeric comparisons on other fields; it may be safer to short-circuit and return `None` if any of the required rate/daily fields are missing or not numeric to avoid subtle runtime type issues.
## Individual Comments
### Comment 1
<location path="custom_components/pricehawk/sensor.py" line_range="556-562" />
<code_context>
+ # `sensor.pricehawk_current_plan_*`). Comparators (Amber, Flow
+ # Power, LocalVolts) keep their per-provider entities.
providers_block = coordinator.data.get("providers", {}) if coordinator.data else {}
+ current_plan_id = (
+ coordinator._current_plan_provider.id
+ if hasattr(coordinator, "_current_plan_provider")
+ else None
+ )
for provider_id, snap in providers_block.items():
+ if provider_id == current_plan_id:
+ continue
provider_name = snap.get("name", provider_id.title())
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `_current_plan_provider` being present but `None`, and double-check ID type consistency.
`hasattr(coordinator, "_current_plan_provider")` only checks for existence, so if the attribute exists but is `None`, `coordinator._current_plan_provider.id` will raise an `AttributeError`. Consider:
```python
current_plan_provider = getattr(coordinator, "_current_plan_provider", None)
current_plan_id = getattr(current_plan_provider, "id", None)
```
Also, `provider_id == current_plan_id` may fail silently if their types differ (e.g., dict key vs model ID). If their types aren’t guaranteed to match, normalize before comparing (e.g., `str(provider_id) == str(current_plan_id)`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+556
to
+562
| current_plan_id = ( | ||
| coordinator._current_plan_provider.id | ||
| if hasattr(coordinator, "_current_plan_provider") | ||
| else None | ||
| ) | ||
| for provider_id, snap in providers_block.items(): | ||
| if provider_id == current_plan_id: |
There was a problem hiding this comment.
issue (bug_risk): Guard against _current_plan_provider being present but None, and double-check ID type consistency.
hasattr(coordinator, "_current_plan_provider") only checks for existence, so if the attribute exists but is None, coordinator._current_plan_provider.id will raise an AttributeError. Consider:
current_plan_provider = getattr(coordinator, "_current_plan_provider", None)
current_plan_id = getattr(current_plan_provider, "id", None)Also, provider_id == current_plan_id may fail silently if their types differ (e.g., dict key vs model ID). If their types aren’t guaranteed to match, normalize before comparing (e.g., str(provider_id) == str(current_plan_id)).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three runtime bugs surfaced during the post-Phase-3.0d UAT walkthrough
on a fresh-wiped HA install. None caught by the test suite — they're
sensor-rendering-vs-coordinator-data shape mismatches that only
manifest at runtime against a real entry.
Stacked on PR #28 (
phase-3-multi-plan). Will auto-rebase to devonce #28 merges.
Fixes
🟢 metrics_won returned fake
0/3when Amber not configuredMetricsWonSensor.native_valueinline fallback returned literalstring
"0/3"whenamber_importorcurrent_plan_importwasNone. Now returns None — sensor renders
unavailablehonestlyinstead of fake-comparing against a phantom zero-cost provider.
🟢 Duplicate sensor entity sets for the user's current plan
registered for the user's CURRENT plan AND comparators. The
current plan already has hardcoded
current_plan_*sensors, sothe generic ones produced duplicates like
sensor.pricehawk_globird_zerohero_residential_flexible_rate_united_energy_cost_today.(Amber, FlowPower, LocalVolts) keep their per-provider entities.
🟢 Flow Power default-OFF on new installs
flow_power_enabled = Trueregardless of userchoice. Every install got
sensor.pricehawk_flow_power_cost_today: $1.0placeholder. Now opt-in: only enabled when user picks FlowPower at credentials, OR enables via comparators OptionsFlow step.
Stats
sensor.py,config_flow.pyTest plan
🤖 Generated with Claude Code
Summary by Sourcery
Fix UAT issues around metrics comparison reporting, duplicate provider sensors, and Flow Power comparator defaults.
Bug Fixes:
Enhancements:
Summary of Changes
What Changed:
metrics_won Sensor Unavailability: Modified
MetricsWonSensor.native_valueto returnNone(rendering as unavailable) instead of the literal string "0/3" when comparison data (amber_importorcurrent_plan_import) is unavailable. Backward-compatible fallback preserved for older coordinator data shapes.Duplicate Provider Sensors Eliminated: Updated
async_setup_entryin sensor.py to skip registering generic per-provider sensors for the user's current plan. This prevents duplicate entities between hardcodedcurrent_plan_*sensors and per-providersensor.pricehawk_<brand>_<planid>_*sensors. Comparator providers (Amber, FlowPower, LocalVolts) retain their per-provider entities.Flow Power Opt-In Default: Changed Flow Power from always-enabled to opt-in behavior:
CONF_FLOW_POWER_ENABLEDnow becomesTrueonly when the primary provider selected isPROVIDER_FLOW_POWER(previously unconditionallyTrue)CONF_FLOW_POWER_ENABLEDtoggle changed fromTruetoFalseWhy:
These fixes address runtime issues discovered during Phase 3.0 UAT on a fresh Home Assistant installation. The changes prevent phantom sensor comparisons, eliminate duplicate entities, and make Flow Power configuration more intuitive by defaulting to opt-in.
Breaking Changes:
None. Backward compatibility is maintained; older coordinator shapes continue to work with fallback logic.
Files Changed