Zigbee2MQTT: add WOOX R7051 smart siren - #2477
Conversation
📝 WalkthroughWalkthroughAdds end-to-end siren support: new frontend components and translations, new constants and icon mappings, zigbee2mqtt expose mappings for siren composite features, composite-aware MQTT ingest/emit and set/read handling, updated tests and fixtures to cover composite siren exposures and conversions. ChangesSiren feature support (single DAG)
Sequence DiagramsequenceDiagram
participant Device as Siren Device
participant MQTT as MQTT Broker
participant Handler as handleMqttMessage
participant Find as findMatchingExpose
participant Read as readValue
participant State as State Manager
participant UI as Frontend
Device->>MQTT: Publish composite state (warning: {mode, level, strobe})
MQTT->>Handler: Deliver message
Handler->>Handler: Detect composite object value
Handler->>Find: Find expose for parent 'warning'
Find-->>Handler: {expose: warningExpose, parent: undefined}
Handler->>Handler: Iterate sub-fields
loop per sub-field
Handler->>Find: Find expose for sub-field
Find-->>Handler: {expose: subExpose, parent: warningExpose}
Handler->>Read: readValue(subExpose, subValue)
Read-->>Handler: Converted numeric/binary value
Handler->>State: Emit NEW_STATE for subFeature
end
State->>UI: New state updates
UI->>UI: Render siren mode/level/strobe controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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)
server/test/services/zigbee2mqtt/lib/findMatchingExpose.test.js (1)
51-56: Rename this test for accuracy.The title says “with parent” but the assertions validate
parentisundefined. Please rename to avoid ambiguity.✏️ Suggested rename
- it('expose discovered with parent on cover position', () => { + it('expose discovered without parent on cover position', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/zigbee2mqtt/lib/findMatchingExpose.test.js` around lines 51 - 56, The test title is misleading: the it() string "expose discovered with parent on cover position" claims a parent but assertions check parent is undefined; update the test description to reflect that no parent is present (e.g., "expose discovered without parent on cover position" or "expose discovered with no parent on cover position") in the test that calls zigbee2MqttService.device.findMatchingExpose('0x00158d00045b2740', 'position') so the name matches the assertions checking result.parent === undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/test/services/zigbee2mqtt/lib/findMatchingExpose.test.js`:
- Around line 59-61: The test currently dereferences sirenDevice and its
friendly_name without asserting the fixture exists; add an explicit assertion
that the fixture was found (e.g., assert(sirenDevice) or
expect(sirenDevice).toBeDefined()) before assigning to
zigbee2MqttService.device.discoveredDevices and before calling
zigbee2MqttService.device.findMatchingExpose('0x00158d00045b2741','mode');
repeat the same explicit existence check for the other fixture use around the
block referenced at lines 70-72 so failures report a clear missing-fixture error
rather than a TypeError; use the sirenDevice variable and the
zigbee2MqttService.device.discoveredDevices lookup to locate where to add the
assertion.
---
Nitpick comments:
In `@server/test/services/zigbee2mqtt/lib/findMatchingExpose.test.js`:
- Around line 51-56: The test title is misleading: the it() string "expose
discovered with parent on cover position" claims a parent but assertions check
parent is undefined; update the test description to reflect that no parent is
present (e.g., "expose discovered without parent on cover position" or "expose
discovered with no parent on cover position") in the test that calls
zigbee2MqttService.device.findMatchingExpose('0x00158d00045b2740', 'position')
so the name matches the assertions checking result.parent === undefined.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
front/src/components/boxs/device-in-room/DeviceRow.jsxfront/src/components/boxs/device-in-room/device-features/SirenLevelDeviceFeature.jsxfront/src/components/boxs/device-in-room/device-features/SirenModeDeviceFeature.jsxfront/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/utils/consts.jsserver/services/zigbee2mqtt/exposes/binaryType.jsserver/services/zigbee2mqtt/exposes/compositeType.jsserver/services/zigbee2mqtt/exposes/enumType.jsserver/services/zigbee2mqtt/exposes/numericType.jsserver/services/zigbee2mqtt/lib/findMatchingExpose.jsserver/services/zigbee2mqtt/lib/handleMqttMessage.jsserver/services/zigbee2mqtt/lib/readValue.jsserver/services/zigbee2mqtt/lib/setValue.jsserver/test/services/zigbee2mqtt/exposes/compositeType.test.jsserver/test/services/zigbee2mqtt/exposes/warningLevelEnumType.test.jsserver/test/services/zigbee2mqtt/exposes/warningModeEnumType.test.jsserver/test/services/zigbee2mqtt/lib/findMatchingExpose.test.jsserver/test/services/zigbee2mqtt/lib/getDiscoveredDevices.test.jsserver/test/services/zigbee2mqtt/lib/handleMqttMessage.test.jsserver/test/services/zigbee2mqtt/lib/payloads/event_device_result.jsonserver/test/services/zigbee2mqtt/lib/payloads/mqtt_devices_get.jsonserver/test/services/zigbee2mqtt/lib/readValue.test.jsserver/test/services/zigbee2mqtt/lib/setValue.test.jsserver/utils/constants.js
| const sirenDevice = discoveredDevices.find((d) => d.friendly_name === '0x00158d00045b2741'); | ||
| zigbee2MqttService.device.discoveredDevices[sirenDevice.friendly_name] = sirenDevice; | ||
| const result = zigbee2MqttService.device.findMatchingExpose('0x00158d00045b2741', 'mode'); |
There was a problem hiding this comment.
Add an explicit fixture existence assertion before dereferencing.
If the fixture entry is missing/renamed, this will fail with a generic TypeError instead of a clear test failure.
✅ Suggested hardening
const sirenDevice = discoveredDevices.find((d) => d.friendly_name === '0x00158d00045b2741');
+ assert.isDefined(sirenDevice, 'Expected siren fixture 0x00158d00045b2741 to exist in mqtt_devices_get.json');
zigbee2MqttService.device.discoveredDevices[sirenDevice.friendly_name] = sirenDevice;Also applies to: 70-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/test/services/zigbee2mqtt/lib/findMatchingExpose.test.js` around lines
59 - 61, The test currently dereferences sirenDevice and its friendly_name
without asserting the fixture exists; add an explicit assertion that the fixture
was found (e.g., assert(sirenDevice) or expect(sirenDevice).toBeDefined())
before assigning to zigbee2MqttService.device.discoveredDevices and before
calling
zigbee2MqttService.device.findMatchingExpose('0x00158d00045b2741','mode');
repeat the same explicit existence check for the other fixture use around the
block referenced at lines 70-72 so failures report a clear missing-fixture error
rather than a TypeError; use the sirenDevice variable and the
zigbee2MqttService.device.discoveredDevices lookup to locate where to add the
assertion.
#3964 Bundle Size — 11.4MiB (+0.1%).4919757(current) vs 8db6b0d master#3902(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #3964 |
Baseline #3902 |
|
|---|---|---|
 Initial JS |
6.39MiB(+0.07%) |
6.38MiB |
 Initial CSS |
310.18KiB |
310.18KiB |
 Cache Invalidation |
64.81% |
0% |
| Chunks |
51 |
51 |
| Assets |
179 |
179 |
| Modules |
1645(+0.12%) |
1643 |
| Duplicate Modules |
21 |
21 |
| Duplicate Code |
0.94% |
0.94% |
| Packages |
136 |
136 |
| Duplicate Packages |
2 |
2 |
Bundle size by type 1 change 1 regression
| Current #3964 |
Baseline #3902 |
|
|---|---|---|
| JS |
8.29MiB (+0.13%) |
8.28MiB |
| IMG |
2.66MiB |
2.66MiB |
| CSS |
328.03KiB |
328.03KiB |
| Fonts |
93.55KiB |
93.55KiB |
| Other |
18.82KiB |
18.82KiB |
| HTML |
13.58KiB |
13.58KiB |
Bundle analysis report Branch William-De71:features/woox-siren Project dashboard
Generated by RelativeCI Documentation Report issue
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2477 +/- ##
==========================================
+ Coverage 98.74% 98.79% +0.04%
==========================================
Files 990 1006 +16
Lines 17114 17514 +400
==========================================
+ Hits 16900 17303 +403
+ Misses 214 211 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
front/src/components/boxs/device-in-room/DeviceRow.jsx (1)
35-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ROW_TYPE_BY_FEATURE_TYPE['mode']is overwritten: AC mode controls will render as siren mode controls.
DEVICE_FEATURE_TYPES.AIR_CONDITIONING.MODEandDEVICE_FEATURE_TYPES.SIREN.MODEboth equal the string'mode'. In a JS object literal, the second key assignment (SirenModeDeviceFeatureat line 40) silently replaces the first (AirConditioningModeDeviceFeatureat line 35). Any non-read-only AC device feature with type'mode'will now renderSirenModeDeviceFeature, displaying siren alert mode options (burglar/fire/emergency…) instead of AC modes (auto/cooling/heating…).
DeviceSetValue.jsxalready handles this correctly via a compound(category, type)guard. The same approach should be applied here — guard on bothcategoryandtypebefore selecting the component.🛠️ Proposed fix (partial — structural change needed)
The cleanest fix without a large refactor is to keep
AirConditioningModeDeviceFeaturein the map under the'mode'key and add an explicit pre-check for the siren mode case, similar toDeviceSetValue.jsx:const elementType = ROW_TYPE_BY_FEATURE_TYPE[props.deviceFeature.type]; + // Disambiguate types that share the same type string across categories + if ( + props.deviceFeature.category === DEVICE_FEATURE_CATEGORIES.SIREN && + props.deviceFeature.type === DEVICE_FEATURE_TYPES.SIREN.MODE + ) { + return createElement(SirenModeDeviceFeature, { ...props, rowName }); + } + if (!elementType) {And remove the colliding entry from the map:
- [DEVICE_FEATURE_TYPES.SIREN.MODE]: SirenModeDeviceFeature,You'll also need to import
DEVICE_FEATURE_CATEGORIESat the top of the file.🤖 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 `@front/src/components/boxs/device-in-room/DeviceRow.jsx` around lines 35 - 47, ROW_TYPE_BY_FEATURE_TYPE currently maps both AIR_CONDITIONING.MODE and SIREN.MODE to the same 'mode' key so AirConditioningModeDeviceFeature is being overwritten by SirenModeDeviceFeature; fix by importing DEVICE_FEATURE_CATEGORIES, remove the conflicting [DEVICE_FEATURE_TYPES.SIREN.MODE] entry from ROW_TYPE_BY_FEATURE_TYPE, keep the AIR_CONDITIONING.MODE mapping, and add an explicit compound pre-check in DeviceRow.jsx (similar to DeviceSetValue.jsx) that checks feature.category === DEVICE_FEATURE_CATEGORIES.SIREN && feature.type === DEVICE_FEATURE_TYPES.SIREN.MODE to render SirenModeDeviceFeature, otherwise fall back to the ROW_TYPE_BY_FEATURE_TYPE lookup for mode.server/utils/constants.js (1)
652-661:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
SIREN.MODEandAIR_CONDITIONING.MODEboth evaluate to'mode', causing a collision inDeviceRow.jsx.In
server/utils/constants.js,DEVICE_FEATURE_TYPES.SIREN.MODE = 'mode'andDEVICE_FEATURE_TYPES.AIR_CONDITIONING.MODE = 'mode'. InDeviceRow.jsx, theROW_TYPE_BY_FEATURE_TYPEobject uses these strings as keys (lines 35 and 40). Since both resolve to the same key, the later definitionSIREN.MODE(line 40) overwritesAIR_CONDITIONING.MODE(line 35). This causes AC unit mode controls to renderSirenModeDeviceFeatureinstead ofAirConditioningModeDeviceFeature.Additionally,
SIREN.VOLUMEandTELEVISION.VOLUMEboth evaluate to'volume'(lines 46 and 29 in DeviceRow.jsx), causingSIREN.VOLUMEto overwriteTELEVISION.VOLUMEwith the same consequence.Both require fixing in
DeviceRow.jsxby ensuring unique keys, or alternatively by changing the constant strings to be distinct (e.g.,SIREN.MODE = 'siren-mode',SIREN.VOLUME = 'siren-volume').🤖 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 `@server/utils/constants.js` around lines 652 - 661, The issue is key collisions between DEVICE_FEATURE_TYPES constants (e.g., SIREN.MODE and AIR_CONDITIONING.MODE both equal 'mode', and SIREN.VOLUME and TELEVISION.VOLUME both equal 'volume') which makes ROW_TYPE_BY_FEATURE_TYPE in DeviceRow.jsx pick the wrong component; fix by making the feature type keys unique or by changing how ROW_TYPE_BY_FEATURE_TYPE indexes them — either update the constants (e.g., change SIREN.MODE -> 'siren-mode' and SIREN.VOLUME -> 'siren-volume') so DEVICE_FEATURE_TYPES.SIREN.* are distinct, or modify DeviceRow.jsx to use a composite key (e.g., `${feature.category}.${feature.type}`) when building ROW_TYPE_BY_FEATURE_TYPE and when looking up rows so identical type names across categories don't collide.
🧹 Nitpick comments (2)
front/src/components/device/SelectSirenMode.jsx (1)
52-61: 💤 Low value
defaultValueis ignored in controlled mode and should be removed.When
valueis provided toreact-select, the component operates in controlled mode anddefaultValuehas no effect.defaultValue={''}is misleading and should be removed.🛠️ Proposed fix
<Select class="select-device-feature" - defaultValue={''} value={selectedOption}🤖 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 `@front/src/components/device/SelectSirenMode.jsx` around lines 52 - 61, The Select component in SelectSirenMode.jsx is being used in controlled mode via value={selectedOption}, so the defaultValue={''} prop is ignored and should be removed; update the JSX for the Select (the element using value={selectedOption} and onChange={this.handleValueChange}) by deleting the defaultValue prop to avoid confusion and rely solely on the controlled value prop.server/services/zigbee2mqtt/exposes/numericType.js (1)
1084-1091:volumename mapped globally toSIRENcategory — may misclassify other Zigbee devices.The
namesmap uses the Zigbee2MQTT expose name as a flat key without device-type discrimination. Any Zigbee device that exposes a numeric property namedvolume(e.g., a Zigbee speaker or media player) will now be classified asDEVICE_FEATURE_CATEGORIES.SIREN/DEVICE_FEATURE_TYPES.SIREN.VOLUME. This is consistent with how the existing file works (e.g.,batteryglobally maps toBATTERY), butvolumeis a more widely shared expose name than most.This is worth verifying against the existing Zigbee2MQTT device database to confirm no known non-siren devices expose a
volumenumeric.🤖 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 `@server/services/zigbee2mqtt/exposes/numericType.js` around lines 1084 - 1091, The current global mapping for the expose name "volume" in the names map assigns it to DEVICE_FEATURE_CATEGORIES.SIREN / DEVICE_FEATURE_TYPES.SIREN.VOLUME (the "volume" entry in numericType.js), which may misclassify non-siren devices; change this by removing the flat "volume" => SIREN mapping and instead handle "volume" in the parsing logic where device context is available (e.g., inside the numeric expose handler that receives the expose object or endpoint info): only map to SIREN.VOLUME when the device model/endpoint/cluster indicates a siren (or when expose.endpoint/name matches a known siren endpoint), otherwise leave it unmapped or map to a generic audio/media category; update the code paths that reference DEVICE_FEATURE_CATEGORIES.SIREN and DEVICE_FEATURE_TYPES.SIREN.VOLUME accordingly so they only trigger when the contextual check passes.
🤖 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
`@front/src/components/boxs/device-in-room/device-features/MultiLevelWithInputDeviceFeature.jsx`:
- Around line 28-32: handleSlider currently reads e.target.value (a string) and
passes it to setLocalValue and props.updateValueWithDebounce, causing a type
mismatch with commitInput which uses Number from clamp(); fix by coercing the
slider value to a Number (e.g., Number(...) or parseFloat(...)) inside
handleSlider before calling setLocalValue and props.updateValueWithDebounce so
both code paths (handleSlider and commitInput) always work with numeric values;
reference functions: handleSlider, commitInput, setLocalValue,
props.updateValueWithDebounce, and clamp.
In `@front/src/components/boxs/device-in-room/device-features/style.css`:
- Around line 82-87: The .numericRow CSS uses invalid values: replace
align-items: right with a valid vertical alignment (e.g., align-items: center)
and replace justify-content: right with the proper horizontal value
(justify-content: flex-end) so the flex container behaves correctly; update the
.numericRow rule accordingly.
In `@front/src/components/device/SelectSirenMode.jsx`:
- Around line 14-26: The current getOptions/componentDidMount flow defers
building deviceFeatureOptions so the initial render passes undefined to
<Select>, causing a flicker; instead compute the options once from the static
SIREN_MODE at module-level and remove state usage and lifecycle setup. Locate
getOptions, componentDidMount, deviceFeatureOptions and replace them by
exporting/defining a constant (e.g., SIREN_MODE_OPTIONS) built from
Object.keys(SIREN_MODE) with the same label/value mapping using
this.props.intl.dictionary keys if needed (or compute labels lazily via a small
helper that Select can call), then update the Select to receive that constant
(or a prop-derived array) directly and delete
getOptions/setState/componentDidMount code.
---
Outside diff comments:
In `@front/src/components/boxs/device-in-room/DeviceRow.jsx`:
- Around line 35-47: ROW_TYPE_BY_FEATURE_TYPE currently maps both
AIR_CONDITIONING.MODE and SIREN.MODE to the same 'mode' key so
AirConditioningModeDeviceFeature is being overwritten by SirenModeDeviceFeature;
fix by importing DEVICE_FEATURE_CATEGORIES, remove the conflicting
[DEVICE_FEATURE_TYPES.SIREN.MODE] entry from ROW_TYPE_BY_FEATURE_TYPE, keep the
AIR_CONDITIONING.MODE mapping, and add an explicit compound pre-check in
DeviceRow.jsx (similar to DeviceSetValue.jsx) that checks feature.category ===
DEVICE_FEATURE_CATEGORIES.SIREN && feature.type ===
DEVICE_FEATURE_TYPES.SIREN.MODE to render SirenModeDeviceFeature, otherwise fall
back to the ROW_TYPE_BY_FEATURE_TYPE lookup for mode.
In `@server/utils/constants.js`:
- Around line 652-661: The issue is key collisions between DEVICE_FEATURE_TYPES
constants (e.g., SIREN.MODE and AIR_CONDITIONING.MODE both equal 'mode', and
SIREN.VOLUME and TELEVISION.VOLUME both equal 'volume') which makes
ROW_TYPE_BY_FEATURE_TYPE in DeviceRow.jsx pick the wrong component; fix by
making the feature type keys unique or by changing how ROW_TYPE_BY_FEATURE_TYPE
indexes them — either update the constants (e.g., change SIREN.MODE ->
'siren-mode' and SIREN.VOLUME -> 'siren-volume') so DEVICE_FEATURE_TYPES.SIREN.*
are distinct, or modify DeviceRow.jsx to use a composite key (e.g.,
`${feature.category}.${feature.type}`) when building ROW_TYPE_BY_FEATURE_TYPE
and when looking up rows so identical type names across categories don't
collide.
---
Nitpick comments:
In `@front/src/components/device/SelectSirenMode.jsx`:
- Around line 52-61: The Select component in SelectSirenMode.jsx is being used
in controlled mode via value={selectedOption}, so the defaultValue={''} prop is
ignored and should be removed; update the JSX for the Select (the element using
value={selectedOption} and onChange={this.handleValueChange}) by deleting the
defaultValue prop to avoid confusion and rely solely on the controlled value
prop.
In `@server/services/zigbee2mqtt/exposes/numericType.js`:
- Around line 1084-1091: The current global mapping for the expose name "volume"
in the names map assigns it to DEVICE_FEATURE_CATEGORIES.SIREN /
DEVICE_FEATURE_TYPES.SIREN.VOLUME (the "volume" entry in numericType.js), which
may misclassify non-siren devices; change this by removing the flat "volume" =>
SIREN mapping and instead handle "volume" in the parsing logic where device
context is available (e.g., inside the numeric expose handler that receives the
expose object or endpoint info): only map to SIREN.VOLUME when the device
model/endpoint/cluster indicates a siren (or when expose.endpoint/name matches a
known siren endpoint), otherwise leave it unmapped or map to a generic
audio/media category; update the code paths that reference
DEVICE_FEATURE_CATEGORIES.SIREN and DEVICE_FEATURE_TYPES.SIREN.VOLUME
accordingly so they only trigger when the contextual check passes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a4b9bac-1c49-4365-83d7-0fa700fc1e65
📒 Files selected for processing (12)
front/src/components/boxs/device-in-room/DeviceRow.jsxfront/src/components/boxs/device-in-room/SupportedFeatureTypes.jsxfront/src/components/boxs/device-in-room/device-features/MultiLevelWithInputDeviceFeature.jsxfront/src/components/boxs/device-in-room/device-features/style.cssfront/src/components/device/SelectSirenMode.jsxfront/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/scene/edit-scene/actions/DeviceSetValue.jsxfront/src/utils/consts.jsserver/services/zigbee2mqtt/exposes/numericType.jsserver/utils/constants.js
✅ Files skipped from review due to trivial changes (4)
- front/src/components/boxs/device-in-room/SupportedFeatureTypes.jsx
- front/src/utils/consts.js
- front/src/config/i18n/en.json
- front/src/config/i18n/fr.json
🚧 Files skipped from review as they are similar to previous changes (1)
- front/src/config/i18n/de.json
| const handleSlider = e => { | ||
| const v = e.target.value; | ||
| setLocalValue(v); | ||
| props.updateValueWithDebounce(deviceFeature, v); | ||
| }; |
There was a problem hiding this comment.
handleSlider sends a string; commitInput sends a number — type inconsistency.
e.target.value on a range input is always a string, but commitInput uses clamp() which returns a Number. The backend/debounce handler likely expects a number, creating a silent type mismatch between the two code paths.
🛠️ Proposed fix
const handleSlider = e => {
- const v = e.target.value;
+ const v = Number(e.target.value);
setLocalValue(v);
props.updateValueWithDebounce(deviceFeature, v);
};📝 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 handleSlider = e => { | |
| const v = e.target.value; | |
| setLocalValue(v); | |
| props.updateValueWithDebounce(deviceFeature, v); | |
| }; | |
| const handleSlider = e => { | |
| const v = Number(e.target.value); | |
| setLocalValue(v); | |
| props.updateValueWithDebounce(deviceFeature, v); | |
| }; |
🤖 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
`@front/src/components/boxs/device-in-room/device-features/MultiLevelWithInputDeviceFeature.jsx`
around lines 28 - 32, handleSlider currently reads e.target.value (a string) and
passes it to setLocalValue and props.updateValueWithDebounce, causing a type
mismatch with commitInput which uses Number from clamp(); fix by coercing the
slider value to a Number (e.g., Number(...) or parseFloat(...)) inside
handleSlider before calling setLocalValue and props.updateValueWithDebounce so
both code paths (handleSlider and commitInput) always work with numeric values;
reference functions: handleSlider, commitInput, setLocalValue,
props.updateValueWithDebounce, and clamp.
| .numericRow { | ||
| display: flex; | ||
| align-items: right; | ||
| justify-content: right; | ||
| margin-bottom: -0.5rem; | ||
| } |
There was a problem hiding this comment.
align-items: right is not a valid CSS property value — browsers will silently ignore it.
align-items accepts values such as flex-start, flex-end, center, baseline, stretch, etc. right is not in the spec for this property (it is partially supported for justify-content in some browsers but not align-items). If vertical centering is intended, use align-items: center.
🛠️ Proposed fix
.numericRow {
display: flex;
- align-items: right;
+ align-items: center;
justify-content: flex-end;
margin-bottom: -0.5rem;
}🤖 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 `@front/src/components/boxs/device-in-room/device-features/style.css` around
lines 82 - 87, The .numericRow CSS uses invalid values: replace align-items:
right with a valid vertical alignment (e.g., align-items: center) and replace
justify-content: right with the proper horizontal value (justify-content:
flex-end) so the flex container behaves correctly; update the .numericRow rule
accordingly.
| getOptions = () => { | ||
| const deviceFeatureOptions = Object.keys(SIREN_MODE).map(key => { | ||
| const value = SIREN_MODE[key]; | ||
| return { | ||
| label: get(this.props.intl.dictionary, `deviceFeatureAction.category.siren.mode.${key.toLowerCase()}`, { | ||
| default: key.toLowerCase() | ||
| }), | ||
| value | ||
| }; | ||
| }); | ||
|
|
||
| this.setState({ deviceFeatureOptions }); | ||
| }; |
There was a problem hiding this comment.
Options are undefined on first render; use a static constant instead.
deviceFeatureOptions is undefined until componentDidMount fires. On the initial render, <Select options={undefined}> shows an empty dropdown, then re-renders with options — causing a flicker. Since the options are derived entirely from the static SIREN_MODE constant, there is no reason to defer computation to componentDidMount or store them in state at all.
🛠️ Proposed fix
Compute options once as a module-level constant and remove the getOptions/componentDidMount machinery:
+const SIREN_MODE_KEYS = Object.keys(SIREN_MODE);
+
class SelectSirenMode extends Component {
- getOptions = () => {
- const deviceFeatureOptions = Object.keys(SIREN_MODE).map(key => {
- const value = SIREN_MODE[key];
- return {
- label: get(this.props.intl.dictionary, `deviceFeatureAction.category.siren.mode.${key.toLowerCase()}`, {
- default: key.toLowerCase()
- }),
- value
- };
- });
- this.setState({ deviceFeatureOptions });
- };
- componentDidMount() {
- this.getOptions();
- }
- render(props, { deviceFeatureOptions }) {
+ render(props) {
+ const deviceFeatureOptions = SIREN_MODE_KEYS.map(key => ({
+ value: SIREN_MODE[key],
+ label: get(props.intl.dictionary, `deviceFeatureAction.category.siren.mode.${key.toLowerCase()}`, {
+ default: key.toLowerCase()
+ })
+ }));
const selectedOption = this.getSelectedOption();
return (
<SelectAlso applies to: 45-47
🤖 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 `@front/src/components/device/SelectSirenMode.jsx` around lines 14 - 26, The
current getOptions/componentDidMount flow defers building deviceFeatureOptions
so the initial render passes undefined to <Select>, causing a flicker; instead
compute the options once from the static SIREN_MODE at module-level and remove
state usage and lifecycle setup. Locate getOptions, componentDidMount,
deviceFeatureOptions and replace them by exporting/defining a constant (e.g.,
SIREN_MODE_OPTIONS) built from Object.keys(SIREN_MODE) with the same label/value
mapping using this.props.intl.dictionary keys if needed (or compute labels
lazily via a small helper that Select can call), then update the Select to
receive that constant (or a prop-derived array) directly and delete
getOptions/setState/componentDidMount code.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
github-actions
Bot
added
area:server
1 participant
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
npm teston both front/server)npm run eslinton both front/server)npm run prettieron both front/server)npm run compare-translationson front)front/src/config/demo.js) so that the demo website is working without a backend? (if needed) See https://demo.gladysassistant.com.NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Add support of WOOX R7051 Smart siren
Summary by CodeRabbit
New Features
Localization
Tests