Clean some deps and use native#2423
Clean some deps and use native#2423bertrandda wants to merge 13 commits intoGladysAssistant:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2423 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 1009 1009
Lines 17670 17647 -23
==========================================
- Hits 17460 17441 -19
+ Misses 210 206 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
#4132 Bundle Size — 11.43MiB (-0.03%).3f53b11(current) vs b3cfa5a master#4120(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch bertrandda:deps/use-native Project dashboard Generated by RelativeCI Documentation Report issue |
801de7c to
344a08d
Compare
Pierre-Gilles
left a comment
There was a problem hiding this comment.
Nice PR! Great cleanup :)
I have a comment on the debounce change.
front/src/utils/debounce.js
Outdated
| * @param {number} wait - The number of milliseconds to delay | ||
| * @returns {Function} The debounced function | ||
| */ | ||
| export default function debounce(func, wait) { |
There was a problem hiding this comment.
What’s the benefit of bundling the code of an external dependency directly into Gladys?
- The debounce package is well-tested (https://github.com/sindresorhus/debounce).
- It’s used by 15 million developers, so the API is familiar and easy to pick up when switching projects.
- It has no dependencies and is fully tree-shakeable (https://bundlephobia.com/package/debounce@3.0.0).
I agree that some dependencies can be heavy or unnecessary, but in this case, it’s a perfect example of a small, reliable, and well-maintained dependency 🙂
There was a problem hiding this comment.
Main benefits of native instead of third party package is supply chain security (we see that a lot here or more recently axios) and time to install for CI, dev environment...
Because it does not contain any sub-dependency ans with debounce size + npm cache, it's maybe not relevant in this case. I rollback this part
344a08d to
2dad783
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoved usages of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 |
2dad783 to
4c77d8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/lib/gateway/gateway.forwardDeviceStateToAlexa.js (1)
44-44: Optional chaining may silently produceundefinedfor a required field.At line 44,
mapping?.properties?.supported?.[0]?.namecould evaluate toundefinedif the mapping structure is incomplete, resulting in an invalid Alexa payload withname: undefined. Sincemappinghas already passed the null check on line 34, and thedeviceMappings.jsstructure guaranteesproperties.supported[0].nameexists for all valid mappings, this optional chaining is technically safe but could mask future configuration errors.Consider whether a direct access (
mapping.properties.supported[0].name) with an explicit validation would be preferable for catching misconfigured mappings early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/gateway/gateway.forwardDeviceStateToAlexa.js` at line 44, The current use of optional chaining on mapping?.properties?.supported?.[0]?.name can silently yield undefined; change to direct access mapping.properties.supported[0].name and add an explicit validation in the function that builds the Alexa payload (referencing mapping, properties, supported, and name) to throw or log a clear error when any of mapping.properties, mapping.properties.supported, mapping.properties.supported[0], or mapping.properties.supported[0].name is missing so misconfigured mappings fail fast instead of producing an invalid Alexa payload.
🤖 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/package.json`:
- Line 72: Upgrading preact-router to v4 breaks use of the removed
activeClassName prop and may change route() behavior; search for all usages of
the activeClassName prop (e.g., in SettingsLayout, IntegrationMenu, header and
other Link components) and replace them by either: (a) using the :local-link CSS
pseudo-class to style active links, (b) creating a small wrapper Link/NavLink
component that computes active state and applies the previous class name, or (c)
managing active state manually where needed; for programmatic navigation, audit
every call to the global route() function and either adapt calls to the new v4
API or implement a thin compatibility wrapper function named route(path,
replace?) that delegates to the new router/history API while preserving the
boolean second-parameter “replace” behavior so existing call sites continue to
work.
- Line 59: In SettingsSystemBatteryLevelWarning.jsx the debounced method
debouncedUpdateBatteryLevelUnderWarningThreshold is created without binding so
its this is incorrect; update the declaration to call .bind(this) on the
debounced function (i.e., wrap
debounce(this.updateBatteryLevelUnderWarningThreshold, 200).bind(this)) so the
instance context is preserved when debouncing
updateBatteryLevelUnderWarningThreshold, matching the pattern used elsewhere.
---
Nitpick comments:
In `@server/lib/gateway/gateway.forwardDeviceStateToAlexa.js`:
- Line 44: The current use of optional chaining on
mapping?.properties?.supported?.[0]?.name can silently yield undefined; change
to direct access mapping.properties.supported[0].name and add an explicit
validation in the function that builds the Alexa payload (referencing mapping,
properties, supported, and name) to throw or log a clear error when any of
mapping.properties, mapping.properties.supported,
mapping.properties.supported[0], or mapping.properties.supported[0].name is
missing so misconfigured mappings fail fast instead of producing an invalid
Alexa payload.
🪄 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: bb5219d2-2aa5-4dd0-aba9-6c4f868d0250
⛔ Files ignored due to path filters (11)
front/package-lock.jsonis excluded by!**/package-lock.jsonserver/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/alexa/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/edf-tempo/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/enedis/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/energy-monitoring/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/homekit/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/melcloud/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/nextcloud-talk/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/tasmota/package-lock.jsonis excluded by!**/package-lock.jsonserver/services/telegram/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
front/package.jsonfront/src/routes/integration/index.jsserver/lib/device/device.notify.jsserver/lib/device/device.poll.jsserver/lib/device/device.setValue.jsserver/lib/gateway/enedis/gateway.enedisGetConsumptionLoadCurve.jsserver/lib/gateway/enedis/gateway.enedisGetDailyConsumption.jsserver/lib/gateway/enedis/gateway.enedisGetDailyConsumptionMaxPower.jsserver/lib/gateway/gateway.forwardDeviceStateToAlexa.jsserver/lib/gateway/gateway.getBackups.jsserver/lib/gateway/gateway.getTTSApiUrl.jsserver/lib/gateway/gateway.getUsersKeys.jsserver/lib/gateway/gateway.handleAlexaMessage.jsserver/lib/gateway/gateway.handleGoogleHomeMessage.jsserver/lib/gateway/gateway.handleNewMessage.jsserver/lib/gateway/gateway.login.jsserver/lib/gateway/gateway.openAIAsk.jsserver/lib/scene/scene.addScene.jsserver/lib/system/system.getContainers.jsserver/lib/system/system.getNetworkMode.jsserver/lib/user/user.create.jsserver/package.jsonserver/services/alexa/lib/alexa.onDiscovery.jsserver/services/alexa/lib/alexa.onExecute.jsserver/services/alexa/lib/alexa.onReportState.jsserver/services/alexa/lib/syncDeviceConverter.jsserver/services/alexa/package.jsonserver/services/edf-tempo/package.jsonserver/services/enedis/package.jsonserver/services/energy-monitoring/package.jsonserver/services/energy-monitoring/utils/addEnergyFeatures.jsserver/services/homekit/lib/createBridge.jsserver/services/homekit/package.jsonserver/services/melcloud/lib/device/melcloud.convertDevice.jsserver/services/melcloud/package.jsonserver/services/nextcloud-talk/lib/bot/bot.poll.jsserver/services/nextcloud-talk/lib/message/message.new.jsserver/services/nextcloud-talk/lib/message/message.send.jsserver/services/nextcloud-talk/package.jsonserver/services/tasmota/package.jsonserver/services/telegram/lib/message.new.jsserver/services/telegram/package.jsonserver/test/lib/device/device.migrateFromSQLiteToDuckDb.test.jsserver/test/lib/device/device.purgeAllSqliteStates.test.jsserver/test/lib/device/device.purgeStatesByFeatureId.test.jsserver/test/lib/gateway/gateway.forwardDeviceStateToAlexa.test.jsserver/test/lib/gateway/gateway.getUsersKeys.test.jsserver/test/services/alexa/lib/alexa.onDiscovery.test.jsserver/test/services/alexa/lib/alexa.onExecute.test.jsserver/test/services/alexa/lib/alexa.onReportState.test.jsserver/utils/device.js
💤 Files with no reviewable changes (3)
- server/services/edf-tempo/package.json
- server/services/enedis/package.json
- server/package.json
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)NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Clean some code, remove some dependencies
uuidby nativecryptoon server side withrandomUUIDmethod to generate uuid v4get-valuewhen it's possibleaxiosin edf service,form-datain server/package.json)Summary by CodeRabbit
Dependencies
Bug Fixes
Refactor