Open
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:
📝 WalkthroughWalkthroughRefactors Matter device access to use accessor methods ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: |
90ed64f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://99a44bd2.gladys-plus.pages.dev |
| Branch Preview URL: | https://upgrade-matter-latest.gladys-plus.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2501 +/- ##
==========================================
- Coverage 98.81% 98.81% -0.01%
==========================================
Files 1007 1010 +3
Lines 17570 17694 +124
==========================================
+ Hits 17362 17484 +122
- Misses 208 210 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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/matter/lib/matter.setValue.test.js (1)
465-497: Add test cases for missing cluster support on other device feature types.The current test at lines 465-497 verifies error handling when the OnOff cluster is missing. However, the implementation only guards against null for OnOff—WindowCovering, LevelControl, ColorControl, and Thermostat clusters have no null-checks before method invocations. Add similar test cases for these feature types to ensure consistent error handling when cluster clients are unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/matter/lib/matter.setValue.test.js` around lines 465 - 497, Add tests mirroring the OnOff missing-cluster case for the other feature types: create gladysDevice/gladysFeature variants with types that map to WindowCovering, LevelControl, ColorControl, and Thermostat and ensure clusterClients Map returns undefined for their cluster IDs; call matterHandler.setValue(...) for each and assert the promise is rejected with an appropriate "Device does not support <ClusterName> cluster" error (or the exact message your implementation uses). Use the same nodesMap setup, fake connect/initialized/getDevices structure and reference matterHandler.setValue, getClusterClientById, and the cluster client lookup logic so each cluster path is exercised and null/undefined cluster clients are handled consistently.
🤖 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.init.js`:
- Around line 48-50: Update the inline comment above the autoConnect option in
matter.init.js to remove the incorrect PR reference and instead reference the
correct Matter.js change; specifically, replace the bogus "PR `#3436`" link with
either the accurate PR/issue or the release note for Matter.js v0.12 (Jan 2025)
that introduced the autoConnect property, and clarify that autoConnect is set to
undefined as a workaround for compatibility with that version's PairedNode
connection options; keep the comment concise and mention the symbol autoConnect
and the Matter.js version or correct link so future maintainers understand why
autoConnect is being overridden.
---
Nitpick comments:
In `@server/test/services/matter/lib/matter.setValue.test.js`:
- Around line 465-497: Add tests mirroring the OnOff missing-cluster case for
the other feature types: create gladysDevice/gladysFeature variants with types
that map to WindowCovering, LevelControl, ColorControl, and Thermostat and
ensure clusterClients Map returns undefined for their cluster IDs; call
matterHandler.setValue(...) for each and assert the promise is rejected with an
appropriate "Device does not support <ClusterName> cluster" error (or the exact
message your implementation uses). Use the same nodesMap setup, fake
connect/initialized/getDevices structure and reference matterHandler.setValue,
getClusterClientById, and the cluster client lookup logic so each cluster path
is exercised and null/undefined cluster clients are handled consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb38406a-b9c3-49f4-8e23-ae5c34965003
⛔ Files ignored due to path filters (2)
server/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/matter/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
server/package.jsonserver/services/matter/lib/matter.getNodes.jsserver/services/matter/lib/matter.handleNode.jsserver/services/matter/lib/matter.init.jsserver/services/matter/lib/matter.listenToStateChange.jsserver/services/matter/lib/matter.setValue.jsserver/services/matter/package.jsonserver/services/matter/utils/convertToGladysDevice.jsserver/test/services/matter/lib/convertToGladysDevice.test.jsserver/test/services/matter/lib/listenToStateChange.test.jsserver/test/services/matter/lib/matter.getNodes.test.jsserver/test/services/matter/lib/matter.init.test.jsserver/test/services/matter/lib/matter.pairDevice.test.jsserver/test/services/matter/lib/matter.refreshDevices.test.jsserver/test/services/matter/lib/matter.setValue.test.js
#4149 Bundle Size — 11.44MiB (+0.05%).90ed64f(current) vs c22a231 master#4138(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #4149 |
Baseline #4138 |
|
|---|---|---|
 Initial JS |
6.42MiB(+0.09%) |
6.42MiB |
 Initial CSS |
310.54KiB |
310.54KiB |
 Cache Invalidation |
56% |
0% |
| Chunks |
51 |
51 |
| Assets |
179 |
179 |
| Modules |
1643 |
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 #4149 |
Baseline #4138 |
|
|---|---|---|
| JS |
8.32MiB (+0.07%) |
8.32MiB |
| IMG |
2.68MiB |
2.68MiB |
| CSS |
328.39KiB |
328.39KiB |
| Fonts |
93.55KiB |
93.55KiB |
| Other |
18.82KiB |
18.82KiB |
| HTML |
13.58KiB |
13.58KiB |
Bundle analysis report Branch upgrade-matter-latest Project dashboard
Generated by RelativeCI Documentation Report issue
9 tasks
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/test/services/matter/lib/matter.init.test.js`:
- Around line 900-908: The test contains a no-op Chai expression:
expect(matterHandler.refreshDevicesPromise).to.be.fulfilled which lints as an
unused expression; replace it with an executable assertion that actually awaits
the background promise (for example, use an awaited chai-as-promised check like
await expect(matterHandler.refreshDevicesPromise).to.be.fulfilled or simply
await matterHandler.refreshDevicesPromise and assert no error) so the test
verifies that matterHandler.refreshDevicesPromise resolves after calling
matterHandler.init().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53a75ce7-87b9-4b38-a44f-65af104d6122
📒 Files selected for processing (3)
server/services/matter/lib/matter.setValue.jsserver/test/services/matter/lib/matter.init.test.jsserver/test/services/matter/lib/matter.setValue.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Upgrade Matter.js to latest version
Summary by CodeRabbit
Chores
Bug Fixes
Refactor
Tests