Gate surplus feed-in to supported firmware versions #221
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a runtime capability check for Surplus Feed-in that validates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 2
Fix all issues with AI Agents 🤖
In @src/device/b2500V2.ts:
- Around line 155-165: The isSurplusFeedInSupported helper can return undefined
when deviceVersion is missing, causing an asymmetry between UI advertise and
command handling; update the command handler that checks the
surplusFeedInSupported result (the conditional currently using "=== false") to
instead use "!== true" so the command only runs when the helper explicitly
returns true, matching the advertise behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/device/b2500V2.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/device/b2500V2.ts (1)
src/types.ts (1)
B2500V2DeviceData(134-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-addon
- GitHub Check: build-addon (linux/arm/v7)
- GitHub Check: build-addon (linux/arm64)
🔇 Additional comments (1)
src/device/b2500V2.ts (1)
709-709: LGTM: Advertisement integration follows the correct pattern.Passing the function reference as
{ enabled: isSurplusFeedInSupported }allows the advertisement system to evaluate support dynamically based on device state.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af3aca612d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isSurplusFeedInSupported = ( | ||
| state: Pick<B2500V2DeviceData, 'deviceType'> & { deviceInfo?: B2500V2DeviceData['deviceInfo'] }, | ||
| ) => { | ||
| const requiredVersion = state.deviceType === 'HMJ' ? 108 : 226; | ||
| const deviceVersion = state.deviceInfo?.deviceVersion; |
There was a problem hiding this comment.
The new isSurplusFeedInSupported check keys off state.deviceType === 'HMJ' to apply the lower 108 firmware requirement (lines 155–159), but the device type stored in state is the configured value passed through the parser (parser.ts) and the docs show it is typically suffixed (e.g., deviceType: "HMJ-1"). For these real HMJ identifiers the equality fails, they fall into the default 226 requirement, and surplus feed-in discovery/commands are suppressed on HMJ devices even when running supported 1.08–2.25 firmware. Matching the base type (e.g., startsWith('HMJ') or normalizing like getDeviceDefinition does) would avoid disabling the feature on valid HMJ firmware.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @src/device/b2500V2.ts:
- Around line 155-165: The deviceType equality check in isSurplusFeedInSupported
incorrectly excludes HMJ variants with suffixes (e.g., "HMJ-1"); update the
check in the isSurplusFeedInSupported function to detect HMJ prefixes (for
example using deviceType.startsWith('HMJ') or a regex like /^HMJ\b/) instead of
strict equality, keep the existing requiredVersion logic and deviceVersion null
handling unchanged so HMJ-* devices use the 108 threshold.
- Around line 715-721: The current check lets undefined slip through because it
only blocks when isSurplusFeedInSupported(...) returns false; update the logic
so commands only proceed when support is explicitly true. Locate the call to
isSurplusFeedInSupported and the surplusFeedInSupported variable in this block
and change the guard to treat undefined as unsupported (e.g., require
surplusFeedInSupported === true to proceed, or convert with Boolean and check
truthiness), and adjust the logger.warn to include the unknown-version branch
when deviceState.deviceInfo?.deviceVersion is undefined so commands are
fail-closed for unknown firmware.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdsrc/device/b2500V2.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/device/b2500V2.ts (1)
src/types.ts (1)
B2500V2DeviceData(134-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-addon
- GitHub Check: build-addon (linux/arm64)
- GitHub Check: build-addon (linux/arm/v7)
🔇 Additional comments (3)
CHANGELOG.md (1)
4-5: LGTM!The changelog entry accurately describes the user-facing fix and aligns with the PR objectives.
src/device/b2500V2.ts (2)
708-708: Good addition of default value.Providing a
defaultValueprevents undefined states in the UI and ensures the switch has a sensible initial state when surplus feed-in status is unavailable.
710-710: Correct conditional enablement.Using
enabled: isSurplusFeedInSupportedproperly gates the UI control, hiding it from Home Assistant when the feature is unsupported or version information is unavailable.
| const isSurplusFeedInSupported = ( | ||
| state: Pick<B2500V2DeviceData, 'deviceType'> & { deviceInfo?: B2500V2DeviceData['deviceInfo'] }, | ||
| ) => { | ||
| const requiredVersion = state.deviceType === 'HMJ' ? 108 : 226; | ||
| const deviceVersion = state.deviceInfo?.deviceVersion; | ||
| if (deviceVersion == null) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return deviceVersion >= requiredVersion; | ||
| }; |
There was a problem hiding this comment.
Critical: deviceType equality check fails for suffixed HMJ identifiers.
The check state.deviceType === 'HMJ' will not match real HMJ devices that include suffixes (e.g., 'HMJ-1'), causing them to incorrectly fall through to the 226 firmware requirement instead of the correct 108 threshold. This will disable surplus feed-in on HMJ devices running firmware 1.08–2.25.
🔎 Proposed fix
- const isSurplusFeedInSupported = (
- state: Pick<B2500V2DeviceData, 'deviceType'> & { deviceInfo?: B2500V2DeviceData['deviceInfo'] },
- ) => {
- const requiredVersion = state.deviceType === 'HMJ' ? 108 : 226;
+ const isSurplusFeedInSupported = (
+ state: Pick<B2500V2DeviceData, 'deviceType'> & { deviceInfo?: B2500V2DeviceData['deviceInfo'] },
+ ) => {
+ const requiredVersion = state.deviceType.startsWith('HMJ') ? 108 : 226;
const deviceVersion = state.deviceInfo?.deviceVersion;
if (deviceVersion == null) {
return undefined;
}
return deviceVersion >= requiredVersion;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isSurplusFeedInSupported = ( | |
| state: Pick<B2500V2DeviceData, 'deviceType'> & { deviceInfo?: B2500V2DeviceData['deviceInfo'] }, | |
| ) => { | |
| const requiredVersion = state.deviceType === 'HMJ' ? 108 : 226; | |
| const deviceVersion = state.deviceInfo?.deviceVersion; | |
| if (deviceVersion == null) { | |
| return undefined; | |
| } | |
| return deviceVersion >= requiredVersion; | |
| }; | |
| const isSurplusFeedInSupported = ( | |
| state: Pick<B2500V2DeviceData, 'deviceType'> & { deviceInfo?: B2500V2DeviceData['deviceInfo'] }, | |
| ) => { | |
| const requiredVersion = state.deviceType.startsWith('HMJ') ? 108 : 226; | |
| const deviceVersion = state.deviceInfo?.deviceVersion; | |
| if (deviceVersion == null) { | |
| return undefined; | |
| } | |
| return deviceVersion >= requiredVersion; | |
| }; |
🤖 Prompt for AI Agents
In @src/device/b2500V2.ts around lines 155-165, The deviceType equality check in
isSurplusFeedInSupported incorrectly excludes HMJ variants with suffixes (e.g.,
"HMJ-1"); update the check in the isSurplusFeedInSupported function to detect
HMJ prefixes (for example using deviceType.startsWith('HMJ') or a regex like
/^HMJ\b/) instead of strict equality, keep the existing requiredVersion logic
and deviceVersion null handling unchanged so HMJ-* devices use the 108
threshold.
| const surplusFeedInSupported = isSurplusFeedInSupported(deviceState); | ||
| if (surplusFeedInSupported === false) { | ||
| logger.warn( | ||
| `Surplus feed-in is not supported on ${deviceState.deviceType} version ${deviceState.deviceInfo?.deviceVersion}`, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Critical: Inconsistent undefined handling creates fail-open behavior.
The check surplusFeedInSupported === false allows commands to proceed when deviceVersion is unavailable (returns undefined), since undefined !== false. This creates asymmetry: the UI hides the feature (line 710 treats undefined as falsy), but the command still executes. This fail-open behavior could send unsupported commands to devices with unknown firmware versions.
🔎 Proposed fix for fail-closed behavior
const surplusFeedInSupported = isSurplusFeedInSupported(deviceState);
- if (surplusFeedInSupported === false) {
+ if (surplusFeedInSupported !== true) {
logger.warn(
- `Surplus feed-in is not supported on ${deviceState.deviceType} version ${deviceState.deviceInfo?.deviceVersion}`,
+ `Surplus feed-in is not supported on ${deviceState.deviceType} version ${deviceState.deviceInfo?.deviceVersion ?? 'unknown'}`,
);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const surplusFeedInSupported = isSurplusFeedInSupported(deviceState); | |
| if (surplusFeedInSupported === false) { | |
| logger.warn( | |
| `Surplus feed-in is not supported on ${deviceState.deviceType} version ${deviceState.deviceInfo?.deviceVersion}`, | |
| ); | |
| return; | |
| } | |
| const surplusFeedInSupported = isSurplusFeedInSupported(deviceState); | |
| if (surplusFeedInSupported !== true) { | |
| logger.warn( | |
| `Surplus feed-in is not supported on ${deviceState.deviceType} version ${deviceState.deviceInfo?.deviceVersion ?? 'unknown'}`, | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In @src/device/b2500V2.ts around lines 715-721, The current check lets undefined
slip through because it only blocks when isSurplusFeedInSupported(...) returns
false; update the logic so commands only proceed when support is explicitly
true. Locate the call to isSurplusFeedInSupported and the surplusFeedInSupported
variable in this block and change the guard to treat undefined as unsupported
(e.g., require surplusFeedInSupported === true to proceed, or convert with
Boolean and check truthiness), and adjust the logger.warn to include the
unknown-version branch when deviceState.deviceInfo?.deviceVersion is undefined
so commands are fail-closed for unknown firmware.
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.