Conversation
Surface the integration's debug controls on a dedicated, per-config-entry "addhOn diagnostica" service device instead of hiding them in Options -> Configure and the developer-tools services. One set of entities per entry (the loggers are process-global, so they are NOT per appliance): - switch: Debug logging, MQTT realtime debug -- mirror entry.options (single source of truth), re-apply log levels live via the existing options update listener (no reload), and stay in sync with the Options flow / Reset button through an entry update listener. - sensor: Debug status (enum off/integration/mqtt/full), Integration/MQTT log level (effective level, reflects runtime set_log_level overrides), Appliances discovered, Last refresh (timestamp, seeded on add, advanced on refresh). - binary_sensor: Update OK (connectivity; stays available through a failed refresh so the OFF state is actually shown). - button: Refresh now, Reset debug (clears runtime overrides + persists both toggles off). Shared foundations in base_entity.py (account_device_info, HonAccountEntity, HonAccountCoordinatorEntity); sw_version resolved once in __init__ via a lazy async_get_integration. en/it translations at parity. New docs/debug-device.md with core-card YAML, plus README + discovery/MQTT doc cross-links. Tests: new tests/test_debug_panel.py (switch persistence/sync, sensor mappings, last-refresh seed/advance, button reset) with a scoped lifecycle patch over the bare shared HA stubs; existing platform tests filter the account entities out of their appliance-level assertions.
…bug steps
The "addhOn diagnostica" service device carried a hardcoded Italian name, the
odd one out in an otherwise English-base / EN-IT-translated UI. Make it follow
the UI language like the entity names: drop the literal name for
translation_key="diagnostics" and add a "device" section to en.json
("addhOn diagnostics") and it.json ("addhOn diagnostica"). model has no
translation hook in HA, so it becomes English ("Diagnostics & debug").
Docs/README: the device name and its entity-id slug now depend on the HA
language; README uses the English name, docs/debug-device.md shows the English
slug (addhon_diagnostics_*) and notes the Italian one. README also gains a
concise step-by-step "Capture debug logs" block (enable -> reproduce ->
download full log -> reset).
Tests: extend the translation_key-literal check to recognize the top-level
"device" translation section (still required in both languages).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces a synthetic per-account "addhOn diagnostics" Home Assistant service device grouping two debug config switches, five diagnostic sensors, a coordinator-health binary sensor, and two action buttons. Refines diagnostics coverage to partition meta/noise from telemetry signal. Adds fixture-based validation for AC switch parameters. New base entity classes ( ChangesaddhOn diagnostics device
AC switch parameter schema validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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 docstrings
🧪 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 |
![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 (1)
custom_components/addhon/sensor.py (1)
965-1001: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider using a tuple for immutable class attribute.
The static analyzer flags
_attr_optionsas a mutable list. Since these options are never modified at runtime, a tuple would be more appropriate and silence the warning.♻️ Suggested change
- _attr_options = ["off", "integration", "mqtt", "full"] + _attr_options = ("off", "integration", "mqtt", "full")🤖 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/addhon/sensor.py` around lines 965 - 1001, The _attr_options class attribute in the HonDebugStatusSensor class is currently defined as a mutable list, which triggers a static analyzer warning. Convert this list to a tuple by replacing the square brackets with parentheses, since these options are never modified at runtime and should be immutable. This will silence the warning while maintaining the same functionality.Source: Linters/SAST tools
🤖 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 `@docs/debug-device.md`:
- Around line 72-73: The documentation file contains incorrect entity_id
references for the button entity. The actual entity_id generated by the
implementation is button.addhon_diagnostics_force_refresh (based on the
force_refresh translation key and object_id suffix), but the docs incorrectly
reference button.addhon_diagnostics_refresh_now which is the translated display
name, not the entity_id. Update all occurrences of
button.addhon_diagnostics_refresh_now to button.addhon_diagnostics_force_refresh
on lines 72, 101, and 107 in the docs/debug-device.md file to ensure the
dashboard examples work as copy/paste templates.
---
Nitpick comments:
In `@custom_components/addhon/sensor.py`:
- Around line 965-1001: The _attr_options class attribute in the
HonDebugStatusSensor class is currently defined as a mutable list, which
triggers a static analyzer warning. Convert this list to a tuple by replacing
the square brackets with parentheses, since these options are never modified at
runtime and should be immutable. This will silence the warning while maintaining
the same functionality.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e34a56bf-b886-4415-b610-a29fc1b76b58
📒 Files selected for processing (18)
README.mdcustom_components/addhon/__init__.pycustom_components/addhon/base_entity.pycustom_components/addhon/binary_sensor.pycustom_components/addhon/button.pycustom_components/addhon/manifest.jsoncustom_components/addhon/sensor.pycustom_components/addhon/switch.pycustom_components/addhon/translations/en.jsoncustom_components/addhon/translations/it.jsondocs/debug-device.mdtests/test_debug_panel.pytests/test_entity_availability.pytests/test_entity_translation_keys.pytests/test_number_setpoints.pytests/test_program_select.pytests/test_sensor_per_type.pytests/test_tier2_sensors.py
Comment on lines
+72
to
+73
| - entity: button.addhon_diagnostics_refresh_now | ||
| name: Refresh now |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Inspect button implementation keys/object suffixes"
rg -n -C2 'class HonForceRefreshButton|_attr_translation_key|super\(.*force_refresh' custom_components/addhon/button.py
echo
echo "2) Find documented entity_ids using refresh_now vs force_refresh"
rg -n -C2 'addhon_diagnostics_(refresh_now|force_refresh)' docs/debug-device.md README.md
echo
echo "3) Check tests (if present) for expected button entity_id/object_id"
rg -n -C2 '(refresh_now|force_refresh)' testsRepository: tis24dev/addhOn
Length of output: 3000
🏁 Script executed:
find . -type f \( -name "*.json" -o -name "*.yaml" -o -name "*.yml" \) | grep -i -E "(string|translation|locale|en\.json)" | head -20Repository: tis24dev/addhOn
Length of output: 155
🏁 Script executed:
rg -n "Refresh now|refresh.?now" --type yaml --type jsonRepository: tis24dev/addhOn
Length of output: 139
🏁 Script executed:
sed -n '630,650p' custom_components/addhon/translations/en.jsonRepository: tis24dev/addhOn
Length of output: 502
Change entity_id from refresh_now to force_refresh in button entity references.
The implementation uses force_refresh as the translation key and object_id suffix, which generates the entity_id button.addhon_diagnostics_force_refresh. The docs incorrectly reference button.addhon_diagnostics_refresh_now (the translated display name, not the entity_id). Update lines 72, 101, and 107 to use the correct entity_id so the dashboard examples work as copy/paste templates.
Affected lines
- entity: button.addhon_diagnostics_force_refresh
name: Refresh now
Also applies to: 101-107
🤖 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 `@docs/debug-device.md` around lines 72 - 73, The documentation file contains
incorrect entity_id references for the button entity. The actual entity_id
generated by the implementation is button.addhon_diagnostics_force_refresh
(based on the force_refresh translation key and object_id suffix), but the docs
incorrectly reference button.addhon_diagnostics_refresh_now which is the
translated display name, not the entity_id. Update all occurrences of
button.addhon_diagnostics_refresh_now to button.addhon_diagnostics_force_refresh
on lines 72, 101, and 107 in the docs/debug-device.md file to ensure the
dashboard examples work as copy/paste templates.
…ta noise A live test on 4 real appliances showed coverage.attributes_unmapped / command_params_unmapped buried the mappable "what to map" signal under protocol envelope blobs and debug plumbing (commandHistory, lastConnEvent, activity, parameters, resultCode, transfer-rate scalars, program1..N slots, the command selector and cloud endpoints). Partition the unmapped sets so the maintainer reads pure signal first, nothing dropped: - attributes_unmapped - mappable telemetry candidates (the gold); - attributes_unmapped_statistics - statistics-container keys (unchanged); - attributes_unmapped_meta - envelope blobs (value-type dict/list, no list to maintain) + scalar debug/protocol denylist + program-slot regex; - command_params_unmapped / command_params_unmapped_meta - same split. The value-type rule (dict/list-valued bare key = envelope) needs no name list and auto-catches future blobs; only the scalar residue (resultCode, debug/transfer-rate flags, programStats and cloud/test plumbing) and program1..N use a small denylist. Both denominators (attributes_total, command_params_total) now count mapped + signal (excluding statistics + meta) so unmapped/total reads as a real coverage gap. Validated against the live REF/AC/TD/WM dumps and a 3-round adversarial refuter pool: no over-suppression (genuine warnings softWarn/detWarn and program code prCode kept as signal, confirmed via the decompiled app), partition lossless/disjoint, denominators non-negative and symmetric. Tests extended (32 in test_diagnostics, full suite green); test stubs realigned to the platforms' current homeassistant.const/device_registry imports.
The per-type entity tests check the entity `key` but not the `param` (the Haier command-parameter name a switch reads/writes). A wrong `param` makes a capability-gated switch silently never created on any device, and no key-based test catches it - this was raised as a false-positive "echoStatus -> ecoStatus" fix, but echoStatus is the genuine Haier name (verified live on a real AS35 and in the decompiled hOn app; ecoStatus exists nowhere). Add tests/test_switch_params.py: - reality check: every _AC_SWITCHES.param must exist in a real AC's settings schema (fixture tests/fixtures/ac_as35/, param names captured from a live AS35PBPHRA-PRE diagnostics dump on 2026-06-22; names only, no values/identity); - a drift-guard pin of the key->param mapping documenting echoStatus as correct; - fixture-sanity + uniqueness guards. Verified the test catches the regression: changing eco's param to a non-existent `ecoStatus` fails both the reality and pin tests. Full suite green (414 passed).
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.
Automated release PR for
v5.1.0.Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Improvements
Greptile Summary
This PR introduces the "addhOn diagnostics" device — a synthetic per-account service device that surfaces the integration's debug controls (two persisted switches, two action buttons, five read-only sensors, one binary sensor) directly from a Home Assistant dashboard, without requiring the Options flow dialog. It also refactors the
_coverage()diagnostic helper to partition unmapped attributes into signal, statistics, and meta/noise buckets.HonAccountEntity/HonAccountCoordinatorEntitybase classes group all diagnostics-device entities and keep them distinct from appliance entities via the_addhon_accountmarker.diagnostics.pycoverage calculation now correctly carves out protocol envelope blobs, scalar debug/protocol noise, and program-definition slots into an_unmapped_metabucket so theattributes_totaldenominator reflects only genuine telemetry and control candidates; tests pin both the denylists and the partition invariants.test_debug_panel.pycovers lifecycle, state sync, idempotency, and reset behavior with hand-rolled HA stubs consistent with the project's existing test style.Confidence Score: 5/5
Safe to merge — all new entities follow established HA lifecycle patterns, state sync is covered by tests, and no existing appliance entity logic is touched.
The diagnostics device entities are well-isolated behind the _addhon_account marker, the entry-listener teardown is handled via async_on_remove, and the coordinator-always-available override is intentional and tested. The two carry-over style observations from previous reviews (a redundant state write in HonDebugSwitch._async_set and a dead-code seeding block with a misleading comment in HonLastRefreshSensor) are harmless and non-blocking.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD subgraph HA["Home Assistant"] OPT["Options Flow\n(Configure dialog)"] SVC["set_log_level service"] COORD["DataUpdateCoordinator"] end subgraph DEV["addhOn diagnostics device (per account)"] SW1["HonDebugSwitch\n(debug_logging)"] SW2["HonDebugSwitch\n(mqtt_realtime_debug)"] BTN1["HonForceRefreshButton\n(Refresh now)"] BTN2["HonResetDebugButton\n(Reset debug)"] SEN1["HonDebugStatusSensor\n(off/integration/mqtt/full)"] SEN2["HonLogLevelSensor\n(integration)"] SEN3["HonLogLevelSensor\n(mqtt)"] SEN4["HonAppliancesCountSensor"] SEN5["HonLastRefreshSensor"] BIN["HonUpdateOkBinarySensor"] end ENTRY["entry.options\n(CONF_ENABLE_DEBUG\nCONF_ENABLE_MQTT_DEBUG)"] LOGGERS["Python Loggers\n(custom_components.addhon\n+ mqtt transport)"] SW1 -->|async_update_entry| ENTRY SW2 -->|async_update_entry| ENTRY BTN2 -->|async_update_entry + reset levels| ENTRY BTN2 --> LOGGERS BTN1 -->|async_request_refresh| COORD OPT -->|async_update_entry| ENTRY ENTRY -->|add_update_listener fires| SW1 ENTRY -->|add_update_listener fires| SW2 ENTRY -->|add_update_listener fires| SEN1 SVC --> LOGGERS LOGGERS -->|poll| SEN2 LOGGERS -->|poll| SEN3 COORD -->|_handle_coordinator_update| SEN4 COORD -->|_handle_coordinator_update| SEN5 COORD -->|last_update_success| BIN%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD subgraph HA["Home Assistant"] OPT["Options Flow\n(Configure dialog)"] SVC["set_log_level service"] COORD["DataUpdateCoordinator"] end subgraph DEV["addhOn diagnostics device (per account)"] SW1["HonDebugSwitch\n(debug_logging)"] SW2["HonDebugSwitch\n(mqtt_realtime_debug)"] BTN1["HonForceRefreshButton\n(Refresh now)"] BTN2["HonResetDebugButton\n(Reset debug)"] SEN1["HonDebugStatusSensor\n(off/integration/mqtt/full)"] SEN2["HonLogLevelSensor\n(integration)"] SEN3["HonLogLevelSensor\n(mqtt)"] SEN4["HonAppliancesCountSensor"] SEN5["HonLastRefreshSensor"] BIN["HonUpdateOkBinarySensor"] end ENTRY["entry.options\n(CONF_ENABLE_DEBUG\nCONF_ENABLE_MQTT_DEBUG)"] LOGGERS["Python Loggers\n(custom_components.addhon\n+ mqtt transport)"] SW1 -->|async_update_entry| ENTRY SW2 -->|async_update_entry| ENTRY BTN2 -->|async_update_entry + reset levels| ENTRY BTN2 --> LOGGERS BTN1 -->|async_request_refresh| COORD OPT -->|async_update_entry| ENTRY ENTRY -->|add_update_listener fires| SW1 ENTRY -->|add_update_listener fires| SW2 ENTRY -->|add_update_listener fires| SEN1 SVC --> LOGGERS LOGGERS -->|poll| SEN2 LOGGERS -->|poll| SEN3 COORD -->|_handle_coordinator_update| SEN4 COORD -->|_handle_coordinator_update| SEN5 COORD -->|last_update_success| BINComments Outside Diff (2)
custom_components/addhon/sensor.py, line 495-503 (link)CoordinatorEntity.async_added_to_hass()already callsself._handle_coordinator_update()whencoordinator.data is not None— and sinceasync_setup_entryrunsasync_config_entry_first_refresh()before entities are registered, data is always non-None at this point. As a result,_attr_native_valueis already populated (viaHonLastRefreshSensor._handle_coordinator_update) by the time the seedingifblock runs, so the condition is alwaysFalsein normal operation. The comment claiming "the update only fires on subsequent cycles" contradicts howCoordinatorEntity.async_added_to_hassworks.The seeding block is harmless (covers the theoretical
data is Noneedge case) but the misleading comment could confuse future maintainers reviewing HA lifecycle behaviour here.custom_components/addhon/switch.py, line 615-628 (link)async_update_entryasync_update_entryfires the entry update listener synchronously in HA, which triggers_async_entry_updated → async_write_ha_state(). The explicitself.async_write_ha_state()on line 628 then writes a second time within the same task. HA coalesces these, so there is no correctness issue, but it is redundant. The_async_entry_updatedlistener alone already covers this case.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (2): Last reviewed commit: "test(switch): validate AC switch params ..." | Re-trigger Greptile