Support for Homebridge v2 #920
Conversation
|
|
@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
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (4)
🪧 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.
…teristic properties, to reduce duplication.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/helpers.ts (2)
69-77: Consider adding value type validationWhile the function correctly sets characteristic properties, it assumes the input value is numeric when using type assertions.
Consider adding type validation:
export function allowSingleValueForCharacteristic(characteristic: Characteristic, value: CharacteristicValue): Characteristic { + if (typeof value !== 'number') { + throw new Error('Value must be numeric'); + } characteristic.value = value; characteristic.setProps({ - minValue: value as number, - maxValue: value as number, - validValues: [value as number], + minValue: value, + maxValue: value, + validValues: [value], }); return characteristic; }
79-92: Consider adding empty array validationThe function handles array values well, but could be more defensive against empty arrays.
Consider adding validation:
export function setValidValuesOnCharacteristic(characteristic: Characteristic, validValues: number[]): Characteristic { + if (!Array.isArray(validValues)) { + throw new Error('validValues must be an array'); + } if (validValues.length > 0) { const current_value = characteristic.value as number; if (current_value === undefined || !validValues.includes(current_value)) { characteristic.value = validValues[0]; } characteristic.setProps({ minValue: Math.min(...validValues), maxValue: Math.max(...validValues), validValues: validValues, }); + } else { + throw new Error('validValues array cannot be empty'); } return characteristic; }CHANGELOG.md (1)
18-20: Consider using "among" instead of "amongst"While both are correct, "among" is 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] ~18-~18: 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)
.github/workflows/docs.yml(4 hunks).github/workflows/verify.yml(2 hunks).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)
💤 Files with no reviewable changes (2)
- src/experimental.ts
- docs/config.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .vscode/settings.json
- src/platform.ts
- src/converters/climate.ts
- package.json
- src/platformAccessory.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~18-~18: 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 (9)
src/helpers.ts (2)
14-29: Well-structured input sanitization implementation!The function effectively handles the HAP-NodeJS requirements for accessory names by:
- Replacing non-alphanumeric characters (except spaces and apostrophes)
- Ensuring proper formatting with no leading/trailing spaces or apostrophes
49-57: Good defensive programming with range validation!The added checks ensure characteristic values are always within valid ranges by:
- Setting a default value at the midpoint if undefined
- Clamping values to min/max if out of range
src/converters/light.ts (2)
213-220: Clean color mode handling implementation!The simplified logic properly filters out non-active color information to prevent UI glitches in Home.app.
311-311: Good use of helper function for range validation!The code now properly handles color temperature ranges and characteristic values using the new helper function.
Also applies to: 318-322
.github/workflows/verify.yml (2)
8-8: LGTM!The branch pattern change from double to single quotes maintains the same functionality while following YAML best practices.
20-20: Verify Node.js 22.x compatibility requirementsThe upgrade to Node.js 22.x (both for analysis and matrix) requires careful consideration:
- Node.js 22.x is currently in development and not yet LTS
- This could potentially introduce instability in the CI pipeline
Let's verify if this upgrade is required for Homebridge v2 compatibility:
Also applies to: 25-25
✅ Verification successful
Node.js 22.x is compatible and aligned with package requirements
Based on the package.json analysis, the upgrade to Node.js 22.x is actually well-aligned with the project's requirements. The engines field explicitly declares support for Node.js 22:
"node": "^18.0.0 || ^20.0.0 || ^22.0.0". Additionally, the project supports Homebridge v2 beta ("homebridge": "^1.6.0 || ^2.0.0-beta.0"), indicating this is a deliberate modernization effort.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for Homebridge version and Node.js requirements rg -A 5 '"homebridge":|"engines":|"node":' package.json # Check if there are any explicit Node.js version requirements in the codebase rg -g '!package*.json' -g '!yarn.lock' -g '!*.lock' 'requires Node.js|node.*version|engine.*node'Length of output: 653
.github/workflows/docs.yml (3)
71-71: LGTM!The string formatting changes from double to single quotes maintain consistency with other changes while preserving the same functionality.
Also applies to: 74-74
5-5: Verify impact of reduced documentation update frequencyThe cron schedule change reduces documentation updates from daily to 4 days per week. This could impact the timeliness of documentation updates, especially for rapid Zigbee2MQTT version changes.
Consider the trade-off between resource utilization and documentation freshness:
- Old schedule: Daily at 16:00 UTC
- New schedule: 19:30 UTC on Sunday, Monday, Friday, and Saturday
Let's check the typical frequency of Zigbee2MQTT updates:
23-26: Verify Node.js 22.x compatibility for documentation generationSimilar to verify.yml, the upgrade to Node.js 22.x needs verification:
- Node.js 22.x is currently in development
- Documentation generation might have different Node.js version requirements than the main application
Let's check for any Node.js version constraints in the documentation generation tools:
✅ Verification successful
Node.js 22.x is compatible with documentation generation workflow
The package.json explicitly declares Node.js 22.x as a supported version in the engines field:
"node": "^18.0.0 || ^20.0.0 || ^22.0.0". Additionally:
- No specific documentation generation tools with version constraints were found
- No documentation-specific configuration files with version requirements were detected
- The project appears to use standard TypeScript/JavaScript tooling which is compatible with Node.js 22.x
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for documentation tool dependencies rg -A 5 '"devDependencies":|"ts-node":|"typescript":' package.json # Check if there are any Node.js version requirements in the documentation generation code rg -g 'src/docgen/*' 'requires Node.js|node.*version|engine.*node'Length of output: 430
Script:
#!/bin/bash # Check for any documentation-related dependencies in package.json rg -A 2 '"dependencies"|"devDependencies"' package.json | rg -A 2 'doc|typedoc|jsdoc|vuepress|docsify|mkdocs|mdbook' # Check for any engine requirements in package.json rg '"engines"' -A 3 package.json # Look for documentation generation scripts in package.json rg '"scripts"' -A 10 package.json | rg 'doc|docs' # Find documentation configuration files fd -e json -e js -e yaml -e yml -e config.js typedoc fd -e json -e js -e yaml -e yml -e config.js jsdoc fd -e json -e js -e yaml -e yml -e config.js vuepressLength of output: 478
|
|
🆗 Published SonarCloud and code coverage data. |
|
🚀 This pull request is included in v1.11.0-beta.9. See Release 1.11.0-beta.9 for release notes. |
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