feature: Reorganize controls, add API docs - #33
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSplits device control into separate Power and Fan switches, adds fan state and duration validation in the coordinator, updates buttons/numbers/sensors accordingly, and adds comprehensive API documentation and README updates. ChangesFan Control & Platform Split
Sequence Diagram(s)sequenceDiagram
participant User as User
participant PowerUI as Power Switch UI
participant FanUI as Fan Switch UI
participant Coordinator as AromaLinkDeviceCoordinator
participant Device as Aroma-Link Device
User->>PowerUI: Toggle power on
PowerUI->>PowerUI: Validate work/pause > 0
PowerUI->>Coordinator: turn_on_off(True)
Coordinator->>Device: POST /device/switch (onOff)
Device->>Coordinator: state updated
User->>FanUI: Toggle fan on
FanUI->>Coordinator: set_fan(True)
Coordinator->>Device: POST /device/switch (fan)
Device->>Coordinator: fan flag updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 (3)
custom_components/aromalink_ha_integration/AromaLinkDeviceCoordinator.py (1)
746-748: ⚡ Quick winImprove exception chain visibility.
Consider using
raise ... from eto preserve the exception chain for better debugging, as suggested by static analysis.♻️ Proposed fix
except Exception as e: _LOGGER.error(f"Error fetching device {self.device_id} info: {e}") - raise UpdateFailed(f"Error: {e}") + raise UpdateFailed(f"Error: {e}") from e🤖 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/aromalink_ha_integration/AromaLinkDeviceCoordinator.py` around lines 746 - 748, The except block that catches Exception e and raises UpdateFailed loses the original traceback; modify the error raise to preserve the exception chain by using "raise UpdateFailed(f'Error: {e}') from e" (keep the existing _LOGGER.error(f"Error fetching device {self.device_id} info: {e}") line), locating this change in the exception handler inside AromaLinkDeviceCoordinator (the block that currently does "except Exception as e:" and calls UpdateFailed).Source: Linters/SAST tools
custom_components/aromalink_ha_integration/switch.py (1)
50-57: 💤 Low valueConsider availability semantics and user experience.
The power switch is marked unavailable when
work_durationorpause_durationare not positive. While semantically correct (the pump cannot operate without a valid schedule), users may find it confusing when the entity shows as "Unavailable" even though the device is online and reachable.This is primarily a UX design choice. The current implementation clearly enforces the configuration constraint, though an alternative would be to keep
available=Trueand letasync_turn_onfail with a user-friendly error message explaining the missing configuration.🤖 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/aromalink_ha_integration/switch.py` around lines 50 - 57, The available property currently returns False when coordinator.work_duration or coordinator.pause_duration are <= 0, causing the entity to appear "Unavailable" even if the device is reachable; change the behavior to always return super().available (so the entity reflects device reachability) and instead enforce the configuration check inside async_turn_on (or the component's turn-on handler) by validating coordinator.work_duration and coordinator.pause_duration and raising or logging a clear, user-friendly error if they are not positive; update symbols: the available property, coordinator.work_duration, coordinator.pause_duration, and async_turn_on to implement this change.custom_components/aromalink_ha_integration/sensor.py (1)
228-230: 💤 Low valueNote: Sensor returns None when work_duration is invalid.
The sensor correctly returns
Nonewhenwork_duration <= 0, aligning with the coordinator's duration validation logic. However, users may find it surprising that historical diffusion time data disappears when work/pause durations are configured to invalid values. This is a deliberate design choice that prevents misleading calculations, but consider documenting this behavior if users report confusion.As per the upstream contract in AromaLinkDeviceCoordinator._has_valid_durations, work_duration must be > 0 for valid operation.
🤖 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/aromalink_ha_integration/sensor.py` around lines 228 - 230, The sensor currently returns None when work_duration <= 0 (use the work_duration variable in the sensor method), matching the upstream contract in AromaLinkDeviceCoordinator._has_valid_durations; update the sensor to make this behavior explicit by adding a short docstring or inline comment on the sensor property/method explaining that historical diffusion time is omitted when durations are invalid, and add a debug log (using the sensor's logger) right before returning None to aid user debugging and visibility that work_duration was invalid; reference the sensor property/method that reads work_duration and AromaLinkDeviceCoordinator._has_valid_durations when making these edits.
🤖 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/aromalink_ha_integration/sensor.py`:
- Line 200: Docstrings in sensor.py use the Unicode multiplication sign "×"
which can cause encoding/readability issues; replace it with an ASCII asterisk
"*" (or 'x') in the affected docstrings—specifically the docstring that reads
"Sensor showing total diffusion time in hours (airPumpCount × work_duration)."
and the other similar docstring at the second occurrence (around line 216) so
both become "(airPumpCount * work_duration)". Locate these strings in sensor.py
and update them accordingly.
In `@docs/API.md`:
- Around line 9-30: The Table of Contents anchors are mismatched with actual
headings; update the TOC entries to use the exact heading texts: replace the
incorrect anchor for the real-time info entry with the exact heading "GET
/device/deviceInfo/now/{id}", replace the app device state anchor with "GET
/v1/app/device/newWork/{id}", update the power control anchor to match the full
heading that includes "POST /device/switch — Power On/Off and Exhaust Fan
Control", and add a new TOC entry for the existing "Schedule and Operation Mode
Endpoints" section so it appears in the TOC; ensure each TOC line uses the
literal heading text to generate correct Markdown anchors.
---
Nitpick comments:
In `@custom_components/aromalink_ha_integration/AromaLinkDeviceCoordinator.py`:
- Around line 746-748: The except block that catches Exception e and raises
UpdateFailed loses the original traceback; modify the error raise to preserve
the exception chain by using "raise UpdateFailed(f'Error: {e}') from e" (keep
the existing _LOGGER.error(f"Error fetching device {self.device_id} info: {e}")
line), locating this change in the exception handler inside
AromaLinkDeviceCoordinator (the block that currently does "except Exception as
e:" and calls UpdateFailed).
In `@custom_components/aromalink_ha_integration/sensor.py`:
- Around line 228-230: The sensor currently returns None when work_duration <= 0
(use the work_duration variable in the sensor method), matching the upstream
contract in AromaLinkDeviceCoordinator._has_valid_durations; update the sensor
to make this behavior explicit by adding a short docstring or inline comment on
the sensor property/method explaining that historical diffusion time is omitted
when durations are invalid, and add a debug log (using the sensor's logger)
right before returning None to aid user debugging and visibility that
work_duration was invalid; reference the sensor property/method that reads
work_duration and AromaLinkDeviceCoordinator._has_valid_durations when making
these edits.
In `@custom_components/aromalink_ha_integration/switch.py`:
- Around line 50-57: The available property currently returns False when
coordinator.work_duration or coordinator.pause_duration are <= 0, causing the
entity to appear "Unavailable" even if the device is reachable; change the
behavior to always return super().available (so the entity reflects device
reachability) and instead enforce the configuration check inside async_turn_on
(or the component's turn-on handler) by validating coordinator.work_duration and
coordinator.pause_duration and raising or logging a clear, user-friendly error
if they are not positive; update symbols: the available property,
coordinator.work_duration, coordinator.pause_duration, and async_turn_on to
implement this change.
🪄 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
Run ID: e25e869c-ba36-48ee-b3bd-203cb2037ea8
📒 Files selected for processing (7)
README.mdcustom_components/aromalink_ha_integration/AromaLinkDeviceCoordinator.pycustom_components/aromalink_ha_integration/button.pycustom_components/aromalink_ha_integration/number.pycustom_components/aromalink_ha_integration/sensor.pycustom_components/aromalink_ha_integration/switch.pydocs/API.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
custom_components/aromalink_ha_integration/switch.py (1)
67-68:work_duration/pause_durationaccess is valid; remove duplicated duration validation
AromaLinkDeviceCoordinatordefines public@property work_durationand@property pause_duration(they return the private_work_duration/_pause_durationvalues), soswitch.py’sself.coordinator.work_duration/self.coordinator.pause_durationusage won’t raiseAttributeError.The duration checks are duplicated:
AromaLinkPowerSwitch.async_turn_onvalidateswork/pausebefore callingAromaLinkDeviceCoordinator.turn_on_off, andturn_on_offalready guards using_has_valid_durations(). Consider delegating the validation to the coordinator to avoid repeating the same logic.🤖 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/aromalink_ha_integration/switch.py` around lines 67 - 68, The switch currently redundantly validates and defaults durations before calling the coordinator; instead, remove that duplicated validation in AromaLinkPowerSwitch.async_turn_on and pass self.coordinator.work_duration and self.coordinator.pause_duration directly to AromaLinkDeviceCoordinator.turn_on_off, relying on the coordinator's public properties (work_duration, pause_duration) and its internal _has_valid_durations()/turn_on_off checks to validate them; update async_turn_on to stop forcing zero defaults or re-checking validity and let the coordinator handle invalid-duration logic.
🤖 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.
Nitpick comments:
In `@custom_components/aromalink_ha_integration/switch.py`:
- Around line 67-68: The switch currently redundantly validates and defaults
durations before calling the coordinator; instead, remove that duplicated
validation in AromaLinkPowerSwitch.async_turn_on and pass
self.coordinator.work_duration and self.coordinator.pause_duration directly to
AromaLinkDeviceCoordinator.turn_on_off, relying on the coordinator's public
properties (work_duration, pause_duration) and its internal
_has_valid_durations()/turn_on_off checks to validate them; update async_turn_on
to stop forcing zero defaults or re-checking validity and let the coordinator
handle invalid-duration logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b299111-0e11-40f3-b0ac-6f406e346297
📒 Files selected for processing (4)
custom_components/aromalink_ha_integration/AromaLinkDeviceCoordinator.pycustom_components/aromalink_ha_integration/sensor.pycustom_components/aromalink_ha_integration/switch.pydocs/API.md
✅ Files skipped from review due to trivial changes (1)
- docs/API.md
🚧 Files skipped from review as they are similar to previous changes (2)
- custom_components/aromalink_ha_integration/sensor.py
- custom_components/aromalink_ha_integration/AromaLinkDeviceCoordinator.py
Merging master brought in dalyem#32's removal of the AROMA_LINK_SSL constant, which set_fan still referenced - swap it for the auth coordinator's ssl property. Also restore the power switch unique_id (username_deviceId_switch) so existing entity registry entries, automations, and history survive the Power/Fan split, and name it 'Power' to match the README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review notesReviewed the full diff — the Power/Fan split, duration validation, Two fixes pushed to this branch (merge of master +
Deliberately left as-is: replacing On Count / Pump Count with Total Run Time / Total Diffusion Time. Note this intersects open issue #2, where a user actively tracks those raw counters — I'll restore the raw count sensors alongside your derived-time sensors in a follow-up PR for that issue. Verified: clean merge with master, all files compile, no stale constant refs (AST-checked). Merging once checks pass. Thanks @ndizazzo! |
The control reorganization (#33) replaced the raw On Count / Pump Count sensors with derived Total Run Time / Total Diffusion Time sensors under new unique_ids, which orphans the counter entities that issue #2's reporter (and anyone tracking device activity) relies on. Bring the raw counters back alongside the derived sensors with their original unique_ids, names, and units so existing registry entries, dashboards, and history resume seamlessly, and mark them TOTAL_INCREASING so Home Assistant records long-term statistics. The class that computes Total Diffusion Time is renamed to match what it now measures (entity-neutral; unique_ids derive from the type string). The underlying polling was already fixed in March (counts come from the v1 device list, which the coordinator merges on every poll); the counters move only when the device pushes data upstream, roughly daily. Closes #2 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Covers the configurable SSL bypass (#32), the Power/Fan control split and API reference (#33), schedule disarming after momentary runs (#35), the stale-poll shield (#36), the restored count sensors with long-term statistics (#37), and the MIT license adoption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
This PR re-organizes the controls to map to supported API fields, and splits the config out into a separate pane so it looks more intelligible.
Screenshot
Summary by CodeRabbit
New Features
Bug Fixes
Documentation