feat: live dashboard strategy (custom:givenergy) for #131#132
Conversation
Adds a Lovelace dashboard strategy that reproduces the six-tab "classic"
dashboard but resolves every entity from the live registry on each render,
so it never goes stale when a device moves area or an entity is renamed.
Resolution is by unique_id ({serial}_{key}) via the entity registry, which
is structurally immune to the HA 2026.6 area-prefix re-slug that rotted the
static generated YAML.
The bundled ge-cell-heatmap card is merged into the same module, so one JS
file serves both custom:givenergy and custom:ge-cell-heatmap. Both gain an
area-prefix-agnostic entity lookup; the strategy additionally skips disabled
entities so their rows don't dangle.
Adds a vitest JS harness mirroring the Python dashboard tests, a Python
parity guard pinning every key the strategy references to a real
EntityDescription.key, and a JS CI job. generate_dashboard remains as the
static eject path.
Refs #131
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. Warning Review limit reached
More reviews will be available in 41 minutes and 47 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 (4)
📝 WalkthroughWalkthroughThis PR introduces a Lovelace dashboard strategy ( ChangesLive dashboard strategy with registry-driven discovery
Sequence DiagramsequenceDiagram
participant User as User
participant HA as Home Assistant
participant Strategy as ge-strategy.js<br/>(strategy)
participant Registry as Entity/Device<br/>Registry
participant Cards as Custom Cards<br/>(power-flow, apexcharts)
User->>HA: Configure dashboard with strategy
HA->>Strategy: generate(config, hass)
Strategy->>Registry: query entity/device lists
Registry-->>Strategy: entity registry + device info
Strategy->>Strategy: classify devices, resolve entity_ids
Strategy->>Strategy: select plant (by serial or first)
Strategy->>Strategy: build view list with entity refs
Strategy->>Cards: feature-detect (card registered?)
Cards-->>Strategy: yes/no
Strategy->>Strategy: degrade to markdown if missing
Strategy-->>HA: dashboard object (views + cards)
HA-->>User: render multi-tab dashboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
I’ve started reviewing this PR. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. 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 a live, self-maintaining dashboard strategy (custom:givenergy) bundled with the existing cell-balance heatmap card into a single frontend module (ge-strategy.js). This registry-driven strategy resolves entities dynamically, preventing stale configurations when devices are renamed or moved. The PR also adds a comprehensive JS testing suite using Vitest, updates Python tests, and adds a parity guard to ensure strategy keys align with real entity descriptions. The review feedback focuses on improving robustness and performance: wrapping WebSocket registry calls in a try-catch block to handle non-admin users gracefully, defensively checking for hass.states to prevent TypeErrors during early bootstrap, and caching resolved entity IDs to avoid scanning all states on every update.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
README.md (2)
232-232: 💤 Low valueUse third-person voice for consistency.
The phrase "I added it because..." introduces a first-person voice that's inconsistent with the rest of the documentation (which uses neutral third-person). Consider rephrasing to maintain a consistent technical documentation tone.
📝 Suggested rephrase
-To avoid that snapshot problem entirely, there's also a dashboard *strategy* that builds the same six-tab layout but resolves every entity from the registry each time the dashboard loads — so it doesn't go stale when a device moves area or an entity is renamed. I added it because the static YAML kept silently rotting on my own install after area reassignments. To use it, create a new dashboard, open the **raw configuration editor**, and set the whole config to: +To avoid that snapshot problem entirely, there's also a dashboard *strategy* that builds the same six-tab layout but resolves every entity from the registry each time the dashboard loads — so it doesn't go stale when a device moves area or an entity is renamed. To use it, create a new dashboard, open the **raw configuration editor**, and set the whole config to:🤖 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 `@README.md` at line 232, The sentence "I added it because the static YAML kept silently rotting on my own install after area reassignments." uses first-person voice; change it to third-person/neutral technical tone by rephrasing to something like "This was added because the static YAML can silently rot after area reassignments, causing dashboards to become stale when devices move or entities are renamed." Update the README line that describes the dashboard strategy to use this neutral phrasing so the documentation remains consistent.
239-239: 💤 Low valueConsider using a clearer placeholder serial.
The example serial
SA2114G047follows the real GivEnergy format. While functional, using an obviously placeholder format (e.g.,SA1234G567) would make it clearer to users that this is an example value to replace.🔖 Alternative placeholder
- serial: SA2114G047 # optional; pin one inverter on a multi-plant install + serial: SA1234G567 # optional; pin one inverter on a multi-plant install🤖 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 `@README.md` at line 239, Replace the real-looking example serial value used in the README by swapping the current serial value (the YAML key "serial" with value "SA2114G047") for a clearly placeholder string such as "SA1234G567" so readers understand it's an example to replace; update the "serial" example line in the README to use the placeholder format.custom_components/givenergy_local/www/ge-strategy.js (2)
79-85: ⚡ Quick winConsider adding error handling for registry API failures.
If either WebSocket call fails (e.g., transient network issue, HA restarting), the unhandled rejection will propagate to HA. While HA's strategy runner catches errors, adding a try/catch here would allow returning a user-friendly diagnostic view rather than a generic failure.
🛡️ Suggested error handling
async function buildPlant(hass, opts) { - var res = await Promise.all([ - hass.callWS({ type: "config/entity_registry/list" }), - hass.callWS({ type: "config/device_registry/list" }), - ]); - var entities = res[0] || []; - var devices = res[1] || []; + var entities, devices; + try { + var res = await Promise.all([ + hass.callWS({ type: "config/entity_registry/list" }), + hass.callWS({ type: "config/device_registry/list" }), + ]); + entities = res[0] || []; + devices = res[1] || []; + } catch (err) { + // Return empty plant; generateDashboard will show "no plant found" message + return { target: null, batteries: [] }; + }🤖 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 `@custom_components/givenergy_local/www/ge-strategy.js` around lines 79 - 85, The buildPlant function currently awaits two hass.callWS calls without protection; wrap the Promise.all(...) call in a try/catch inside buildPlant, catch any errors from hass.callWS (e.g., network/HA restart), log the error (or pass to a provided logger), and return a user-friendly diagnostic view or placeholder result instead of letting the rejection propagate; ensure references to hass.callWS({ type: "config/entity_registry/list" }) and hass.callWS({ type: "config/device_registry/list" }) are preserved so callers get a clear fallback when the registries cannot be loaded.
962-965: 💤 Low valueHeatmap card constructs entity_id strings directly (unlike strategy resolution).
The main strategy uses registry-based
unique_id→entity_idresolution (lines 98-116), but the heatmap card constructs entity IDs directly using the patternsensor.givenergy_battery_{serial}_cell_{n}_voltage. The fallback map (lines 957-961) provides partial area-prefix resilience, but this approach differs from the robust resolution used elsewhere.This is likely intentional for the heatmap's simpler use case, but worth noting for future maintainers that this card won't benefit from the registry-driven resolution if entity naming conventions change beyond the current fallback handling.
🤖 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 `@custom_components/givenergy_local/www/ge-strategy.js` around lines 962 - 965, The heatmap helper cellState currently builds entity_id strings directly (`sensor.givenergy_battery_${s.toLowerCase()}_cell_${n}_voltage`) which bypasses the registry-based unique_id → entity_id resolution used elsewhere; change cellState to resolve the entity id via the same registry lookup logic (re-use the unique_id->entity_id resolution code used by the main strategy and fall back to the existing fallback map) so it returns hass.states[resolvedId] || fallback[resolvedId] instead of relying on constructed ids. Ensure you reference the same resolver function or mapping used in the main strategy and preserve fallback behavior for area-prefix resilience..github/workflows/validate.yml (1)
59-59: ⚡ Quick winConsider adding
persist-credentials: falseto the checkout step.This prevents credentials from being persisted in the workspace's git config, reducing the risk of accidental credential exposure through artifacts or subsequent steps.
🔒 Proposed hardening
steps: - uses: actions/checkout@v6 + with: + persist-credentials: false - uses: actions/setup-node@v4🤖 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 @.github/workflows/validate.yml at line 59, Update the GitHub Actions checkout step that uses actions/checkout@v6 to set persist-credentials: false so credentials are not written into the repository workspace; locate the checkout step (the action invocation "uses: actions/checkout@v6") and add the persist-credentials: false input to that step to prevent credentials from being persisted to the git config.tests/js/ge-strategy.test.js (1)
16-24: 💤 Low valueMinor duplication:
withCardsis also exported bymock-hass.js.This local redefinition adds
async/awaitsupport (line 20), whereas the mock-hass export is synchronous. Consider either:
- Using the mock-hass export directly (it works with async functions via promise chaining), or
- Updating the mock-hass export to support async and importing it here.
Not blocking, as the duplication is minimal and the async wrapper is clear.
🤖 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/js/ge-strategy.test.js` around lines 16 - 24, The test defines a local withCards function that duplicates the one exported by mock-hass.js; remove this local definition and import the shared withCards from mock-hass instead (or update the exported withCards in mock-hass to return a Promise/async so it supports await in tests). Specifically, delete the local async function withCards in ge-strategy.test.js and replace its usage by importing the shared withCards export (or modify mock-hass's withCards implementation to be async-compatible) so only one canonical implementation exists.tests/js/mock-hass.js (1)
80-82: 💤 Low valueOptional: Document the serial lowercasing convention.
The
entityIdhelper lowercases the serial number. While this is consistent throughout the mock, a brief inline comment would clarify this is intentional test fixture behavior (matching Home Assistant's entity_id slugification) rather than a production requirement.📝 Suggested comment
// entity_id marker: includes the (optional) area prefix so tests can assert the -// returned config used the registry's *current* id, not a reconstructed one. +// returned config used the registry's *current* id, not a reconstructed one. +// Serial is lowercased to match HA's entity_id slugification. function entityId(prefix, serial, key) { return "sensor." + prefix + "ge_" + String(serial).toLowerCase() + "_" + key; }🤖 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/js/mock-hass.js` around lines 80 - 82, Add a short inline comment above the entityId function explaining that the serial number is intentionally lowercased to mirror Home Assistant's entity_id slugification for test fixtures; reference the entityId helper and note this is a mock/test convention (not a production requirement) so future readers understand why String(serial).toLowerCase() is used.
🤖 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 `@README.md`:
- Line 238: Update the README example line for the configuration field
max_power_kw to state its default and valid bounds: indicate that max_power_kw
defaults to 10 and accepts integers (or numeric values) in the range 1–100;
e.g., modify the comment after "max_power_kw: 10" to something like "# default
10; valid range 1–100 — Overview 24h chart y-axis envelope (kW)" so readers know
the accepted bounds when tuning this parameter.
---
Nitpick comments:
In @.github/workflows/validate.yml:
- Line 59: Update the GitHub Actions checkout step that uses actions/checkout@v6
to set persist-credentials: false so credentials are not written into the
repository workspace; locate the checkout step (the action invocation "uses:
actions/checkout@v6") and add the persist-credentials: false input to that step
to prevent credentials from being persisted to the git config.
In `@custom_components/givenergy_local/www/ge-strategy.js`:
- Around line 79-85: The buildPlant function currently awaits two hass.callWS
calls without protection; wrap the Promise.all(...) call in a try/catch inside
buildPlant, catch any errors from hass.callWS (e.g., network/HA restart), log
the error (or pass to a provided logger), and return a user-friendly diagnostic
view or placeholder result instead of letting the rejection propagate; ensure
references to hass.callWS({ type: "config/entity_registry/list" }) and
hass.callWS({ type: "config/device_registry/list" }) are preserved so callers
get a clear fallback when the registries cannot be loaded.
- Around line 962-965: The heatmap helper cellState currently builds entity_id
strings directly
(`sensor.givenergy_battery_${s.toLowerCase()}_cell_${n}_voltage`) which bypasses
the registry-based unique_id → entity_id resolution used elsewhere; change
cellState to resolve the entity id via the same registry lookup logic (re-use
the unique_id->entity_id resolution code used by the main strategy and fall back
to the existing fallback map) so it returns hass.states[resolvedId] ||
fallback[resolvedId] instead of relying on constructed ids. Ensure you reference
the same resolver function or mapping used in the main strategy and preserve
fallback behavior for area-prefix resilience.
In `@README.md`:
- Line 232: The sentence "I added it because the static YAML kept silently
rotting on my own install after area reassignments." uses first-person voice;
change it to third-person/neutral technical tone by rephrasing to something like
"This was added because the static YAML can silently rot after area
reassignments, causing dashboards to become stale when devices move or entities
are renamed." Update the README line that describes the dashboard strategy to
use this neutral phrasing so the documentation remains consistent.
- Line 239: Replace the real-looking example serial value used in the README by
swapping the current serial value (the YAML key "serial" with value
"SA2114G047") for a clearly placeholder string such as "SA1234G567" so readers
understand it's an example to replace; update the "serial" example line in the
README to use the placeholder format.
In `@tests/js/ge-strategy.test.js`:
- Around line 16-24: The test defines a local withCards function that duplicates
the one exported by mock-hass.js; remove this local definition and import the
shared withCards from mock-hass instead (or update the exported withCards in
mock-hass to return a Promise/async so it supports await in tests).
Specifically, delete the local async function withCards in ge-strategy.test.js
and replace its usage by importing the shared withCards export (or modify
mock-hass's withCards implementation to be async-compatible) so only one
canonical implementation exists.
In `@tests/js/mock-hass.js`:
- Around line 80-82: Add a short inline comment above the entityId function
explaining that the serial number is intentionally lowercased to mirror Home
Assistant's entity_id slugification for test fixtures; reference the entityId
helper and note this is a mock/test convention (not a production requirement) so
future readers understand why String(serial).toLowerCase() is used.
🪄 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: 266f995c-b304-4cf3-8004-05746217d1cb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/validate.yml.gitignoreREADME.mdcustom_components/givenergy_local/__init__.pycustom_components/givenergy_local/www/ge-cell-heatmap.jscustom_components/givenergy_local/www/ge-strategy.jspackage.jsontests/js/ge-strategy.test.jstests/js/mock-hass.jstests/test_init.pytests/test_strategy_manifest.pyvitest.config.js
💤 Files with no reviewable changes (1)
- custom_components/givenergy_local/www/ge-cell-heatmap.js
Multi-plant correctness and defensiveness from PR #132 review: - Battery attribution: only fall back to all batteries when the registry exposes no via_device links at all. When links exist, an empty match stays empty so an inverter-only plant no longer borrows another plant's packs. - Serial pin: an unresolved explicit `serial:` no longer silently falls back to the first inverter (which would mis-target the Maintenance buttons); it shows the no-plant notice naming the missing serial. The first-inverter default applies only when no serial was supplied. - Classification: classify devices against the full registered key set so disabling a single marker entity (p_pv, ems_plant_enable) can't hide a whole device; the renderable key->entity_id map still excludes disabled entities. - Wrap the registry callWS in try/catch and surface a friendly notice rather than crashing the render on a transient connection drop. - Heatmap card: guard hass.states before iterating, and cache resolved cell entity_ids per-instance instead of scanning all states on every render. Adds JS regression tests for the multi-plant, unmatched-serial, disabled-marker, and registry-failure paths. Refs #132 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Refs #132 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I’ve started re-reviewing this PR. |
dewet22-codex
left a comment
dewet22-codex
left a comment
There was a problem hiding this comment.
Approved. I re-reviewed the current head and do not have any actionable concerns. The three earlier registry-resolution findings are addressed with targeted regression coverage, and the current GitHub checks are green.
2 participants
Closes #131 (v1 — the
classicstrategy).What this adds
A Lovelace dashboard strategy (
custom:givenergy) that builds the existing six-tab dashboard but resolves every entity from the live registry on each render, so it can't silently rot the way the staticgenerate_dashboardYAML does. It resolves byunique_id({serial}_{key}) rather than constructing entity_ids, which makes it structurally immune to the HA 2026.6 area-prefix re-slug (sensor.loft_givenergy_…) that was leaving most cards dangling on my own install after I moved the inverter into an area.Usage — create a dashboard, open the raw config editor, and set:
Notes
ge-cell-heatmap.jsis merged into the strategy module, so one file now serves bothcustom:givenergyandcustom:ge-cell-heatmap. Both gained an area-prefix-agnostic entity lookup (the heatmap was still constructingentity_ids directly and broke under the same re-slug); the strategy additionally skips disabled entities so their rows don't dangle.generate_dashboardstays as the editable static "eject" path — nothing changes there for existing users.hass) mirroring the Pythontest_dashboard.pydiscipline, plus a Python parity guard that pins every key the strategy references to a realEntityDescription.key, so a renamed key fails loudly here rather than dangling silently. Newjs testsCI job runs the vitest suite.Known limitation
On a hard refresh (cache bypass) the strategy can occasionally hit HA's "Timeout waiting for strategy element" — a 5-second registration window that a cold module re-fetch can lose when queued behind other custom-card resources. This affects all network-loaded HA strategies, not just this one; a normal reload serves from cache and isn't affected. Documented in the README. I didn't want to ship the fragile workaround (registering an inline Lovelace resource by poking the resource collection) for an edge case that only bites manual hard-reloads.
Testing
loft_prefix): all six views render and resolve correctly.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
custom:givenergy) that dynamically generates Lovelace dashboards from live Home Assistant data, automatically resolving entities and persisting across renames.Documentation
Chores