Matter - Add Vacuum Cleaner#2516
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end vacuum-cleaner support: new frontend components and i18n, new vacuum constants, Matter cluster mappings and converters, state listeners and setValue handlers, and comprehensive tests for conversions, listeners, and control paths. Changes
Sequence DiagramssequenceDiagram
participant User
participant Frontend as VacuumDockComponent
participant Backend as Matter.setValue
participant Cluster as RvcOperationalStateCluster
User->>Frontend: Click "Return to Dock"
Frontend->>Frontend: set loading = true
Frontend->>Backend: updateValue(deviceFeature, 1)
Backend->>Cluster: get child endpoint cluster client
Backend->>Cluster: goHome()
Cluster-->>Backend: ack
Backend-->>Frontend: resolve promise
Frontend->>Frontend: set loading = false (after 350ms)
sequenceDiagram
participant MatterDevice
participant Listen as listenToStateChange
participant Converter as vacuumCleanerStateMapping
participant EventBus as EventEmitter
participant Frontend
MatterDevice->>Listen: attribute callback (operationalState = 66)
Listen->>Converter: convertMatterOperationalStateToGladys(66)
Converter-->>Listen: returns 6
Listen->>EventBus: emit DEVICE.NEW_STATE (feature: ...:state, value=6)
EventBus-->>Frontend: update UI
MatterDevice->>Listen: attribute callback (batPercentRemaining = 150)
Listen->>Listen: convert half-percent to 75
Listen->>EventBus: emit DEVICE.NEW_STATE (feature: ...:battery, value=75)
EventBus-->>Frontend: update battery UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Deploying gladys-plus with
|
| Latest commit: |
d12f685
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a8f88432.gladys-plus.pages.dev |
| Branch Preview URL: | https://matter-vacuum-cleaner.gladys-plus.pages.dev |
#4143 Bundle Size — 11.45MiB (+0.13%).d12f685(current) vs c22a231 master#4138(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch matter-vacuum-cleaner Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@front/src/components/boxs/device-in-room/device-features/VacuumCleanerDockDeviceFeature.jsx`:
- Around line 14-20: The dock method currently uses await this.setState(...)
(ineffective in Preact) and a fixed 350ms timeout to clear loading, which can
re-enable the UI before the command completes and allows duplicate requests;
change dock to set loading:true synchronously via this.setState({loading:true}),
call the promise-returning this.props.updateValue(this.props.deviceFeature, 1)
and await it, catch errors, then setState({loading:false}) in finally; also
update the button/render that triggers dock to include
disabled={this.state.loading} (or equivalent) so clicks are prevented while the
request is inflight; apply the same pattern to the other similar method(s)
referenced at lines 30-35.
In
`@front/src/components/boxs/device-in-room/device-features/VacuumCleanerModeDeviceFeature.jsx`:
- Around line 11-13: The updateValue handler forwards a string from
e.currentTarget.value which should be a number; modify function updateValue to
convert e.currentTarget.value to a Number (e.g., using Number(...) or
parseInt/parseFloat as appropriate) before calling props.updateValueWithDebounce
with deviceFeature and the numeric value so downstream numeric mode
mapping/commands receive a number instead of a string.
In `@server/services/matter/lib/matter.listenToStateChange.js`:
- Around line 452-466: The listener for the RvcCleanMode cluster is wrongly
reusing convertMatterRunModeToGladys; implement a dedicated conversion function
(e.g., convertMatterCleanModeToGladys) that maps RvcCleanMode values (AUTO=0,
QUICK=1, QUIET=2, LOW_NOISE=3, DEEP_CLEAN=16384, VACUUM=16385, MOP=16386, etc.)
to the appropriate Gladys vacuum modes, and update the callback on
rvcCleanMode.addCurrentModeAttributeListener to call
convertMatterCleanModeToGladys(...) instead of
convertMatterRunModeToGladys(...); for any unmapped/unknown values, skip
emitting EVENTS.DEVICE.NEW_STATE and log a debug/warn with the
nodeId/devicePath/RvcCleanMode.Complete.id and the raw value so it’s clear why
no state was emitted.
In `@server/test/services/matter/lib/convertToGladysDevice.test.js`:
- Around line 139-150: The test assertions for run/clean mode features assert
max: 255 but the converter creates those features with max: 2; update the
expectations in the test (convertToGladysDevice.test.js) to assert max: 2 for
the run/clean-mode feature objects (the objects inside gladysDevice.features,
e.g., the feature with name 'RvcRunMode - 2' and the other run/clean feature
near the later assertions) so the expected max matches the converter output.
🪄 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: d0aef1ea-0f1a-4fb9-92da-4f4f7d4e99d7
📒 Files selected for processing (18)
front/src/components/boxs/device-in-room/DeviceRow.jsxfront/src/components/boxs/device-in-room/device-features/VacuumCleanerDockDeviceFeature.jsxfront/src/components/boxs/device-in-room/device-features/VacuumCleanerModeDeviceFeature.jsxfront/src/components/boxs/device-in-room/device-features/sensor-value/SensorDeviceFeature.jsxfront/src/components/boxs/device-in-room/device-features/sensor-value/VacuumCleanerStateDeviceValue.jsxfront/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/utils/consts.jsserver/services/matter/lib/matter.listenToStateChange.jsserver/services/matter/lib/matter.setValue.jsserver/services/matter/utils/convertToGladysDevice.jsserver/services/matter/utils/vacuumCleanerStateMapping.jsserver/test/services/matter/lib/convertToGladysDevice.test.jsserver/test/services/matter/lib/listenToStateChange.test.jsserver/test/services/matter/lib/matter.setValue.test.jsserver/test/services/matter/utils/vacuumCleanerStateMapping.test.jsserver/utils/constants.js
| dock = async () => { | ||
| await this.setState({ loading: true }); | ||
| this.props.updateValue(this.props.deviceFeature, 1); | ||
| setTimeout(() => { | ||
| this.setState({ loading: false }); | ||
| }, 350); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Preact class components, does setState return a Promise that can be awaited?
💡 Result:
No, in Preact class components, setState does not return a Promise that can be awaited. It returns void.
Citations:
- 1: https://github.com/preactjs/preact/blob/main/src/index.d.ts
- 2: https://shiningdan.github.io/2017/09/23/Preact-%E6%BA%90%E7%A0%81%E8%A7%A3%E6%9E%90%E5%85%B6%E4%B8%80/
- 3: https://preactjs.com/guide/v8/api-reference/
- 4: https://preactjs.com/guide/v10/components
- 5: https://preactjs.com/guide/v10/api-reference/
Tie loading state to the actual command promise and prevent duplicate dock requests.
await this.setState(...) has no effect in Preact (setState returns void), and resetting loading after a fixed timeout (350ms) can re-enable clicks before updateValue completes or fails. The button also lacks the disabled attribute to prevent duplicate submissions during the request.
Proposed fix
class VacuumCleanerDockDeviceFeature extends Component {
@@
dock = async () => {
- await this.setState({ loading: true });
- this.props.updateValue(this.props.deviceFeature, 1);
- setTimeout(() => {
- this.setState({ loading: false });
- }, 350);
+ if (this.state.loading) {
+ return;
+ }
+ this.setState({ loading: true });
+ try {
+ await this.props.updateValue(this.props.deviceFeature, 1);
+ } finally {
+ this.setState({ loading: false });
+ }
};
@@
<button
onClick={this.dock}
type="button"
+ disabled={loading}
class={cx('btn', 'btn-outline-primary', 'btn-sm', style.btnLoading, {
'btn-loading': loading
})}
>Also applies to: 30-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@front/src/components/boxs/device-in-room/device-features/VacuumCleanerDockDeviceFeature.jsx`
around lines 14 - 20, The dock method currently uses await this.setState(...)
(ineffective in Preact) and a fixed 350ms timeout to clear loading, which can
re-enable the UI before the command completes and allows duplicate requests;
change dock to set loading:true synchronously via this.setState({loading:true}),
call the promise-returning this.props.updateValue(this.props.deviceFeature, 1)
and await it, catch errors, then setState({loading:false}) in finally; also
update the button/render that triggers dock to include
disabled={this.state.loading} (or equivalent) so clicks are prevented while the
request is inflight; apply the same pattern to the other similar method(s)
referenced at lines 30-35.
| function updateValue(e) { | ||
| props.updateValueWithDebounce(deviceFeature, e.currentTarget.value); | ||
| } |
There was a problem hiding this comment.
Cast selected mode to number before updating value.
e.currentTarget.value is a string. Forwarding it as-is can break numeric mode mapping/commands downstream.
🔧 Proposed fix
function updateValue(e) {
- props.updateValueWithDebounce(deviceFeature, e.currentTarget.value);
+ props.updateValueWithDebounce(deviceFeature, Number(e.currentTarget.value));
}📝 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.
| function updateValue(e) { | |
| props.updateValueWithDebounce(deviceFeature, e.currentTarget.value); | |
| } | |
| function updateValue(e) { | |
| props.updateValueWithDebounce(deviceFeature, Number(e.currentTarget.value)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@front/src/components/boxs/device-in-room/device-features/VacuumCleanerModeDeviceFeature.jsx`
around lines 11 - 13, The updateValue handler forwards a string from
e.currentTarget.value which should be a number; modify function updateValue to
convert e.currentTarget.value to a Number (e.g., using Number(...) or
parseInt/parseFloat as appropriate) before calling props.updateValueWithDebounce
with deviceFeature and the numeric value so downstream numeric mode
mapping/commands receive a number instead of a string.
| expect(gladysDevice.features).to.have.lengthOf(1); | ||
| expect(gladysDevice.features[0]).to.deep.equal({ | ||
| name: 'RvcRunMode - 2', | ||
| selector: gladysDevice.features[0].selector, | ||
| category: 'vacuum-cleaner', | ||
| type: 'run-mode', | ||
| read_only: false, | ||
| has_feedback: true, | ||
| external_id: 'matter:12345:2:84', | ||
| min: 0, | ||
| max: 255, | ||
| }); |
There was a problem hiding this comment.
Fix run/clean mode max assertions to match converter output.
These tests currently expect max: 255, but the converter creates run/clean features with max: 2, so the assertions are inconsistent with the implementation.
🧪 Proposed test fix
- max: 255,
+ max: 2,
...
- max: 255,
+ max: 2,Also applies to: 169-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/test/services/matter/lib/convertToGladysDevice.test.js` around lines
139 - 150, The test assertions for run/clean mode features assert max: 255 but
the converter creates those features with max: 2; update the expectations in the
test (convertToGladysDevice.test.js) to assert max: 2 for the run/clean-mode
feature objects (the objects inside gladysDevice.features, e.g., the feature
with name 'RvcRunMode - 2' and the other run/clean feature near the later
assertions) so the expected max matches the converter output.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/test/services/matter/lib/convertToGladysDevice.test.js (2)
170-181:⚠️ Potential issue | 🔴 CriticalFix
maxassertion to match converter output — CI is failing.The pipeline failure confirms the converter creates
RvcCleanModefeatures withmax: 2, but this test expectsmax: 255.🐛 Proposed fix
expect(gladysDevice.features[0]).to.deep.equal({ name: 'RvcCleanMode - 2', selector: gladysDevice.features[0].selector, category: 'vacuum-cleaner', type: 'clean-mode', read_only: false, has_feedback: true, external_id: 'matter:12345:2:85', min: 0, - max: 255, + max: 2, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/matter/lib/convertToGladysDevice.test.js` around lines 170 - 181, Update the test assertion to match the converter output: change the expected max for the feature named 'RvcCleanMode - 2' (gladysDevice.features[0]) from 255 to 2 so it matches the value produced by convertToGladysDevice; ensure the deep.equal for gladysDevice.features[0] uses max: 2.
140-151:⚠️ Potential issue | 🔴 CriticalFix
maxassertion to match converter output — CI is failing.The pipeline failure confirms the converter creates
RvcRunModefeatures withmax: 2, but this test expectsmax: 255.🐛 Proposed fix
expect(gladysDevice.features[0]).to.deep.equal({ name: 'RvcRunMode - 2', selector: gladysDevice.features[0].selector, category: 'vacuum-cleaner', type: 'run-mode', read_only: false, has_feedback: true, external_id: 'matter:12345:2:84', min: 0, - max: 255, + max: 2, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/matter/lib/convertToGladysDevice.test.js` around lines 140 - 151, The test's expected feature max value is wrong: update the assertion for gladysDevice.features[0] to expect max: 2 (matching the converter output for RvcRunMode) so the object assertion for name 'RvcRunMode - 2' / external_id 'matter:12345:2:84' uses max: 2 instead of 255; modify the max property in the deep.equal assertion in convertToGladysDevice.test (the assertion referencing gladysDevice.features[0]) to 2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/test/services/matter/lib/convertToGladysDevice.test.js`:
- Around line 170-181: Update the test assertion to match the converter output:
change the expected max for the feature named 'RvcCleanMode - 2'
(gladysDevice.features[0]) from 255 to 2 so it matches the value produced by
convertToGladysDevice; ensure the deep.equal for gladysDevice.features[0] uses
max: 2.
- Around line 140-151: The test's expected feature max value is wrong: update
the assertion for gladysDevice.features[0] to expect max: 2 (matching the
converter output for RvcRunMode) so the object assertion for name 'RvcRunMode -
2' / external_id 'matter:12345:2:84' uses max: 2 instead of 255; modify the max
property in the deep.equal assertion in convertToGladysDevice.test (the
assertion referencing gladysDevice.features[0]) to 2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84efcc8f-83f1-4b21-87f3-e02f9d82e6eb
📒 Files selected for processing (3)
server/services/matter/lib/matter.setValue.jsserver/test/services/matter/lib/convertToGladysDevice.test.jsserver/test/services/matter/utils/vacuumCleanerStateMapping.test.js
✅ Files skipped from review due to trivial changes (1)
- server/test/services/matter/utils/vacuumCleanerStateMapping.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/services/matter/lib/matter.setValue.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/test/services/matter/lib/convertToGladysDevice.test.js (1)
83-238: Consider extracting a small fixture helper for repeated setup.
clusterClients+deviceconstruction is repeated in several tests; a helper would reduce noise and future maintenance cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/matter/lib/convertToGladysDevice.test.js` around lines 83 - 238, Extract a small test fixture helper to reduce repetition: add a function (e.g., createDeviceFixture) in convertToGladysDevice.test.js that accepts parameters like clusterId (or clusterClient object), endpointId, deviceName/number and returns the clusterClients Map and device object used by convertToGladysDevice; replace repeated blocks that construct clusterClients and device with calls to this helper in tests that reference symbols clusterClients, device, and convertToGladysDevice so each test becomes a single call to create the fixture and then awaits convertToGladysDevice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@front/src/components/boxs/device-in-room/device-features/VacuumCleanerCleanModeDeviceFeature.jsx`:
- Around line 11-13: The handler updateValue currently passes
e.currentTarget.value (a string) to props.updateValueWithDebounce which causes
conversion failures in convertGladysCleanModeToMatter; coerce the value to a
Number (e.g., using Number(...) or unary +) before calling
props.updateValueWithDebounce so the numeric VACUUM_CLEANER_CLEAN_MODE enums are
preserved; update the call in updateValue to convert e.currentTarget.value to a
number and then dispatch with props.updateValueWithDebounce(deviceFeature,
numericValue).
---
Nitpick comments:
In `@server/test/services/matter/lib/convertToGladysDevice.test.js`:
- Around line 83-238: Extract a small test fixture helper to reduce repetition:
add a function (e.g., createDeviceFixture) in convertToGladysDevice.test.js that
accepts parameters like clusterId (or clusterClient object), endpointId,
deviceName/number and returns the clusterClients Map and device object used by
convertToGladysDevice; replace repeated blocks that construct clusterClients and
device with calls to this helper in tests that reference symbols clusterClients,
device, and convertToGladysDevice so each test becomes a single call to create
the fixture and then awaits convertToGladysDevice.
🪄 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: 8bda1ade-05f4-4077-b956-34483deb9401
📒 Files selected for processing (13)
front/src/components/boxs/device-in-room/DeviceRow.jsxfront/src/components/boxs/device-in-room/device-features/VacuumCleanerCleanModeDeviceFeature.jsxfront/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonserver/services/matter/lib/matter.listenToStateChange.jsserver/services/matter/lib/matter.setValue.jsserver/services/matter/utils/convertToGladysDevice.jsserver/services/matter/utils/vacuumCleanerStateMapping.jsserver/test/services/matter/lib/convertToGladysDevice.test.jsserver/test/services/matter/lib/listenToStateChange.test.jsserver/test/services/matter/utils/vacuumCleanerStateMapping.test.jsserver/utils/constants.js
✅ Files skipped from review due to trivial changes (2)
- server/test/services/matter/lib/listenToStateChange.test.js
- server/test/services/matter/utils/vacuumCleanerStateMapping.test.js
🚧 Files skipped from review as they are similar to previous changes (7)
- front/src/components/boxs/device-in-room/DeviceRow.jsx
- server/services/matter/utils/convertToGladysDevice.js
- server/services/matter/lib/matter.setValue.js
- server/services/matter/utils/vacuumCleanerStateMapping.js
- server/utils/constants.js
- front/src/config/i18n/de.json
- front/src/config/i18n/fr.json
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2516 +/- ##
==========================================
+ Coverage 98.81% 98.82% +0.01%
==========================================
Files 1009 1010 +1
Lines 17662 17850 +188
==========================================
+ Hits 17452 17640 +188
Misses 210 210 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/services/matter/lib/matter.setValue.js`:
- Around line 199-211: The dock command handler currently ignores non-1 values
silently; update the branch that checks value === 1 in the VACUUM_CLEANER.DOCK
handling to fail fast for any unsupported value by throwing a clear error
including the received value (e.g., referencing gladysFeature,
DEVICE_FEATURE_CATEGORIES, DEVICE_FEATURE_TYPES.VACUUM_CLEANER.DOCK,
rvcOperationalState and RvcOperationalState.Complete.id) instead of doing
nothing, while still calling rvcOperationalState.goHome() when value === 1.
🪄 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: 8a0f4b84-1996-4852-9b58-8de2c90ed948
📒 Files selected for processing (1)
server/services/matter/lib/matter.setValue.js
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)Description of change
Handle vacuum cleaner
Summary by CodeRabbit
New Features
Tests