Support for Homebridge v2 #920
Conversation
…teristic properties, to reduce duplication.
|
|
🆗 Published SonarCloud and code coverage data. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces several enhancements to a Zigbee2MQTT Homebridge plugin, focusing on configuration improvements, code refactoring, and compatibility with Homebridge v2. Key changes include updating dependency versions, removing experimental feature flags, sanitizing accessory names, and improving characteristic handling for devices like lights and thermostats. The modifications aim to streamline the plugin's functionality, enhance error handling, and prepare for future Homebridge versions. Changes
Sequence DiagramsequenceDiagram
participant User
participant Plugin
participant Homebridge
participant Zigbee2MQTT
User->>Plugin: Configure Plugin
Plugin->>Homebridge: Register Accessories
Plugin->>Zigbee2MQTT: Connect and Discover Devices
Zigbee2MQTT-->>Plugin: Device Information
Plugin->>Plugin: Sanitize Accessory Names
Plugin->>Homebridge: Create Services
Homebridge-->>User: Expose Devices in HomeKit
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
test/testHelpers.ts (1)
218-218: LGTM! Consider updating remaining assertions for consistency.The update to use
toHaveBeenCalledWithaligns with modern Jest practices. However, there are several other assertions in the file still using the oldertoBeCalledWithsyntax.Consider updating the following assertions to use the newer syntax for consistency:
- Line 559:
toBeCalledWith→toHaveBeenCalledWith- Line 571:
toBeCalledWith→toHaveBeenCalledWith- Line 629:
toBeCalledWith→toHaveBeenCalledWithExample diff:
-expect(this.accessoryMock.queueDataForSetAction).toBeCalledTimes(1).toBeCalledWith(expectedData); +expect(this.accessoryMock.queueDataForSetAction).toHaveBeenCalledTimes(1).toHaveBeenCalledWith(expectedData);src/helpers.ts (1)
79-92: Adjust characteristic value if newly provided validValues is empty.
Currently, if validValues is empty, the function silently does nothing. If a device unexpectedly reports no valid values, you may want to fall back to a single default or log a warning.package.json (1)
93-93: Keep an eye on beta versions.
Using a beta version for development is acceptable for preparing the plugin but ensure there's a plan to upgrade once Homebridge 2.0 is officially released or the beta becomes stable.src/converters/climate.ts (1)
210-213: Evaluate error-handling coverage.
Ensure that if handleSetTargetState throws or fails for some reason, you handle the callback with an error. Right now, only a success path and a single error message exist.src/platformAccessory.ts (1)
427-427: Sanitize the user-facing Name.
Replacing invalid or emoji-laden fields is good. Consider logging a warning if you strip unexpected characters from a name that might be user-intended.CHANGELOG.md (1)
16-18: Consider using "among" instead of "amongst".The changelog entry clearly documents Homebridge v2 compatibility changes. However, consider using "among" instead of "amongst" as it's more commonly used in modern technical writing.
-- Minor changes to be compatible with the upcoming Homebridge v2 release, amongst others: ++ Minor changes to be compatible with the upcoming Homebridge v2 release, among others:🧰 Tools
🪛 LanguageTool
[style] ~16-~16: The preposition ‘amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...ith the upcoming Homebridge v2 release, amongst others: - In most services where the ...(AMONGST)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.vscode/settings.json(1 hunks)CHANGELOG.md(1 hunks)docs/config.md(0 hunks)package.json(2 hunks)src/converters/climate.ts(4 hunks)src/converters/light.ts(3 hunks)src/experimental.ts(0 hunks)src/helpers.ts(4 hunks)src/platform.ts(3 hunks)src/platformAccessory.ts(3 hunks)test/light.spec.ts(0 hunks)test/testHelpers.ts(1 hunks)
💤 Files with no reviewable changes (3)
- src/experimental.ts
- docs/config.md
- test/light.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~16-~16: The preposition ‘amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...ith the upcoming Homebridge v2 release, amongst others: - In most services where the ...
(AMONGST)
🔇 Additional comments (16)
src/helpers.ts (3)
14-29: Ensure handling of certain edge cases in sanitizeAccessoryName.
The sanitization logic effectively replaces disallowed characters and trims leading/trailing spaces or apostrophes. However, consider the following:
• What happens if the entire string is made of disallowed characters? This might yield an empty name.
• For user-friendliness, you could optionally default to a placeholder name if the sanitization rules produce an empty string.
Would you like to see a quick test script illustrating how sanitizeAccessoryName behaves with edge-case inputs (like an emoji-only name)?
49-58: Validate type conversions for numeric values.
While clamping the characteristic value within range is correct, ensure that the value is definitely a number. If it's not guaranteed to be numeric, an explicit check could prevent unexpected runtime issues (e.g., passing strings).
69-77: Caution with single-value constraints.
Restricting the characteristic's valid values, minValue, and maxValue all to one number is fine, but be aware that any attempts to set the characteristic to a different value will be rejected. Ensure this is the desired behavior, especially if the device might later support multiple values in a firmware update.
package.json (1)
35-35: Double-check compatibility with Homebridge 2.0.
Specifying "^1.6.0 || ^2.0.0-beta.0" is sensible for bridging versions. Make sure the plugin is tested against both stable 1.x and beta 2.x to catch any potential breaking changes.
src/converters/climate.ts (4)
19-24: Imports for new helpers look correct.
The introduced methods (allowSingleValueForCharacteristic, setValidValuesOnCharacteristic) help maintain consistent handling of characteristic properties.
187-187: Check correct enumeration usage.
The code sets a valid values array for CurrentHeatingCoolingState. Ensure the set of possible states (OFF/HEAT/COOL) is correct for your device’s capabilities.
224-231: Single-value usage for heat-only devices.
Using allowSingleValueForCharacteristic to force Current/Target states to “HEAT” is correct. Confirm that the device does not support OFF or COOL modes to avoid user confusion.
235-238: Locking temperature display to Celsius.
AllowSingleValueForCharacteristic ensures no Fahrenheit option appears in HomeKit. Ensure that’s valid for your region or device.
src/platformAccessory.ts (2)
18-18: Import of sanitizeAccessoryName.
This import integrates well with existing logic. No immediate concerns.
500-500: Sanitize subType-names.
Including the subType as part of the accessory name is excellent for clarity. The sanitizeAccessoryName call ensures we don’t break HomeKit’s rules.
src/converters/light.ts (3)
209-216: LGTM: Color mode filtering logic improves Home.app experience.
The code now properly filters out inactive color information based on the color_mode, preventing potential visual glitches in the Home.app interface.
307-307: LGTM: Using helper function improves code maintainability.
Good refactoring to use the copyExposesRangeToCharacteristic helper function for setting characteristic properties.
Line range hint 314-318: LGTM: Added color temperature to hue/saturation conversion.
The addition of color temperature to hue/saturation monitoring improves the user experience by providing visual feedback when color temperature changes.
CHANGELOG.md (1)
12-13: LGTM: Clear documentation of color mode changes.
The changelog entry properly documents the removal of the experimental flag for color mode and references the related issue.
src/platform.ts (2)
571-574: LGTM: Improved visibility of deprecation warning.
Changing the log level from warning to error for deprecated accessories helps users identify and plan for necessary migrations.
598-600: LGTM: Added name sanitization for Homebridge v2 compatibility.
The code now properly sanitizes accessory names and logs both the original and sanitized versions for better debugging.
In this MR I'm preparing support for Homebridge v2 (see #904 and the Homebridge wiki).
So far, I've only quickly ran it locally and made the changes needed based on the warnings and errors I saw in the logs.
Of course, this was limited to my own set of Zigbee devices, so more testing is probably needed.
Things to do before putting this in the beta version:
Characteristic.setPropswhere the valid values are being limited but no valid default value has been set first.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests