[WIP] Add detailed task/job logging for energy recalculation#2412
Open
Terdious wants to merge 117 commits intoGladysAssistant:masterfrom
Open
[WIP] Add detailed task/job logging for energy recalculation#2412Terdious wants to merge 117 commits intoGladysAssistant:masterfrom
Terdious wants to merge 117 commits intoGladysAssistant:masterfrom
Conversation
…s UpdateDeviceFeature et EditDevicePage
…dateDeviceFeature et les fichiers de traduction
…ed features - Updated internationalization files (de.json, en.json, fr.json) to include new keys for calculating consumption and cost from selected features. - Enhanced EnergyMonitoring component to allow users to select features for recalculation, including UI updates for feature selection and error handling. - Implemented backend changes to support feature selection in energy monitoring calculations, including job data handling and logging. - Modified job wrapper to build job data based on selected features for both consumption and cost calculations. - Added validation and logging for feature selectors in energy monitoring calculations.
- Updated JobList component to enhance device feature display logic. - Modified job data schema to ensure features are required for devices. - Cleaned up energy monitoring controller by removing unnecessary logger calls. - Refactored calculateConsumptionFromIndex and calculateConsumptionFromIndexFromBeginning functions for better clarity and performance. - Improved calculateCostFrom and calculateCostFromBeginning functions to streamline feature selector handling and job ID resolution. - Enhanced job data building logic to ensure accurate device and feature identification.
…a consommation d'énergie
… construction des données de travail
…ordre des caractéristiques
…des temporisateurs factices
…lecteurs multiples et gérer les cas limites
…léchées et améliorer la lisibilité
…de la consommation et du coût
…nsumption and cost calculation functions
… de calculateCostFromBeginning
…rs manquants dans le calcul de la consommation
…ériphérique et corriger le contexte dans les tests de construction des données de travail
… assertions dans les tests de consommation et de coût
… tests de consommation et de coût
…alcul de la consommation et le coût
…état dans le calcul des coûts
…le test de calcul des coûts
…s manquants dans le calcul de la consommation
… consommation d'énergie
…t la sélection multiple
…hes et la gestion des erreurs inattendues
…ique dans buildDiscoveredDevice
…r le format des identifiants dérivés dans buildDiscoveredDevice
…er les cas où les fonctionnalités existantes ne sont pas un tableau
- Updated German, English, and French localization files to include translations for "Index today" and "Index yesterday". - Added new energy index types (`index-today` and `index-yesterday`) to the constants. - Created a migration script to update existing Tasmota energy feature types in the database. - Modified energy feature generation to use the new index types for today's and yesterday's energy readings. - Refactored the `buildDiscoveredDevice` function to incorporate the new index features and ensure proper handling of existing features. - Updated tests to validate the new index features and ensure no duplication occurs.
… des appareils découverts
…et suppression des tests obsolètes
… Tasmota device creation tests - Updated all relevant test cases across various device creation tests to use async/await for handling STATUS messages. - Ensured that the expected behavior remains consistent while improving the readability and maintainability of the tests.
…message decoding - Updated multiple test files to change the test cases for decoding STATUS and STATUS11 messages from async to synchronous. - This change improves test readability and consistency across various device creation tests for Tasmota.
…r la création de dispositifs Tasmota
…tion and/or start/end date
… UpdateDeviceFeature
… and validate range dates - fix recalculation side effects when using `feature_selectors` by restoring `ENERGY_INDEX_LAST_PROCESSED` after run (range + from-beginning paths) - wrap cursor reset/recalculation flow in `try/finally` to guarantee restoration even on processing errors - add strict API validation for `start_date`/`end_date` (`YYYY-MM-DD`) while keeping empty values allowed (`null`/`''`/missing) - extend controller tests for valid empty dates and invalid date formats - add non-regression tests for cursor restoration on selector-based recalculation - strengthen `job.wrapperDetached` test to assert logger error when `finish` fails
📝 WalkthroughWalkthroughAdds comprehensive energy-monitoring recalculation features: frontend selection UI, date-range controls, new API endpoints, detached job execution, range-based consumption/cost recalculations with selectors, job-data builders, state cleanup, i18n updates, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Energy<br/>Monitoring<br/>Page
participant API as API<br/>Controller
participant JobSys as Job<br/>Wrapper
participant EnergySvc as Energy<br/>Monitoring<br/>Handler
participant DB as Database
User->>Frontend: select features & date range
Frontend->>API: POST /calculate-cost-range (selectors, start, end)
API->>JobSys: wrapperDetached(start, func, { buildJobData })
JobSys-->>API: return job_id
JobSys->>EnergySvc: calculateCostRange(start, selectors, end, jobId)
EnergySvc->>DB: resolve devices & features
loop per 30-min window
EnergySvc->>DB: destroy states for window
EnergySvc->>EnergySvc: compute costs (filtered by selectors)
EnergySvc->>DB: insert cost states
EnergySvc->>JobSys: updateProgress(percent, { current_date })
end
EnergySvc->>JobSys: finish(status=SUCCESS, data)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
server/lib/device/device.destroyStatesBetween.js-14-20 (1)
14-20:⚠️ Potential issue | 🟡 MinorValidate date inputs and range before executing deletion.
Line 19 and Line 20 assume valid
Dateobjects, and inverted ranges (from > to) currently pass through silently. Add explicit guards so callers get deterministic failures.Proposed fix
async function destroyStatesBetween(selector, from, to) { + if (!(from instanceof Date) || Number.isNaN(from.getTime())) { + throw new TypeError('`from` must be a valid Date'); + } + if (!(to instanceof Date) || Number.isNaN(to.getTime())) { + throw new TypeError('`to` must be a valid Date'); + } + if (from > to) { + throw new RangeError('`from` must be less than or equal to `to`'); + } + const existing = await db.DeviceFeature.findOne({ where: { selector } }); if (!existing) { throw new NotFoundError('DeviceFeature not found'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/device/device.destroyStatesBetween.js` around lines 14 - 20, In destroyStatesBetween, validate the date inputs before calling formatDateInUTC: ensure `from` and `to` are valid Date instances (or parsable) and that `from <= to`; if not, throw a clear error (e.g., BadRequestError) so callers fail deterministically; perform these checks after finding the DeviceFeature (db.DeviceFeature.findOne) and before computing formattedFrom/formattedTo with formatDateInUTC, rejecting inverted ranges and invalid dates.server/lib/job/job.updateProgress.js-13-14 (1)
13-14:⚠️ Potential issue | 🟡 MinorFix the JSDoc example: it references
job.finishinstead ofjob.updateProgress.The example shows
gladys.job.finish('...', 'success')but this function isupdateProgress(id, progress, dataPatch).📝 Proposed fix
* `@example` - * gladys.job.finish('18e1672b-af38-4148-a265-eea9b6549184', 'success'); + * gladys.job.updateProgress('18e1672b-af38-4148-a265-eea9b6549184', 50, { current_step: 'processing' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/job/job.updateProgress.js` around lines 13 - 14, The JSDoc example is wrong — replace the example that calls gladys.job.finish with one that calls gladys.job.updateProgress and shows the correct parameters; update the example usage to call gladys.job.updateProgress('<jobId>', <progressNumber>, <dataPatchObject?>) (matching the updateProgress(id, progress, dataPatch) signature) so it demonstrates passing the job id, a numeric progress value (0–100) and an optional dataPatch object.server/test/services/energy-monitoring/energy-monitoring.calculateCostFromEndToEnd.test.js-33-51 (1)
33-51:⚠️ Potential issue | 🟡 MinorRemove the local
clearDuckDbdefinition; it shadows the imported utility.The import on line 12 brings in
clearDuckDbfrom../../utils/duckdb, but this local definition shadows it. The local version also lacks the improvedisMissingTableErrorcheck from the utility. Remove the local function and use the imported one.🔧 Proposed fix
-const clearDuckDb = async () => { - const tables = [ - 't_device_feature_state', - 't_device_feature_state_aggregate', - 't_energy_price', - 't_device_feature', - 't_device_param', - 't_device', - ]; - // eslint-disable-next-line no-restricted-syntax - for (const table of tables) { - try { - // eslint-disable-next-line no-await-in-loop - await db.duckDbWriteConnectionAllAsync(`DELETE FROM ${table}`); - } catch (e) { - // ignore if table not present - } - } -}; -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/energy-monitoring/energy-monitoring.calculateCostFromEndToEnd.test.js` around lines 33 - 51, Remove the locally defined clearDuckDb function (the one that iterates tables and calls db.duckDbWriteConnectionAllAsync) because it shadows the imported clearDuckDb from ../../utils/duckdb; instead use the imported clearDuckDb which includes the isMissingTableError handling. Locate and delete the local clearDuckDb declaration in this test file and replace any local calls (or rely on existing calls) to use the imported clearDuckDb symbol so the improved missing-table error check is used.front/src/components/device/UpdateDeviceFeature.jsx-59-60 (1)
59-60:⚠️ Potential issue | 🟡 MinorGate the “create energy features” action behind modify permissions.
On Line 59/Line 216, the CTA can appear even when
allowModifyFeaturesis false, exposing a mutating action in read-only mode.🔧 Proposed fix
- const showCreateEnergyFeaturesButton = isEnergyIndex && !hasConsumptionFeatures; + const showCreateEnergyFeaturesButton = props.allowModifyFeatures && isEnergyIndex && !hasConsumptionFeatures;Also applies to: 216-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/components/device/UpdateDeviceFeature.jsx` around lines 59 - 60, The CTA for creating energy features is not gated by modify permissions; update the logic in the UpdateDeviceFeature component so the showCreateEnergyFeaturesButton boolean includes the allowModifyFeatures flag (e.g., set showCreateEnergyFeaturesButton = isEnergyIndex && !hasConsumptionFeatures && allowModifyFeatures) and also wrap the CTA render (the JSX block around lines 216-230) to check allowModifyFeatures before rendering, ensuring both the computed flag and the UI render require allowModifyFeatures to be true.server/services/energy-monitoring/lib/energy-monitoring.calculateCostFromBeginning.js-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorFix JSDoc example signature mismatch.
Line 9 shows a 4-argument call, but
calculateCostFromBeginningtakes(featureSelectors, jobId).📝 Proposed doc fix
- * calculateCostFromBeginning(null, [], null, '12345678-1234-1234-1234-1234567890ab'); + * calculateCostFromBeginning([], '12345678-1234-1234-1234-1234567890ab');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/energy-monitoring/lib/energy-monitoring.calculateCostFromBeginning.js` around lines 8 - 10, The JSDoc example calls calculateCostFromBeginning with four args but the function signature is calculateCostFromBeginning(featureSelectors, jobId); update the example to use two arguments only (e.g. calculateCostFromBeginning(null, '12345678-1234-1234-1234-1234567890ab') or calculateCostFromBeginning([], '12345678-1234-1234-1234-1234567890ab')) so the example matches the actual function parameters and intent.server/test/lib/job/job.test.js-293-302 (1)
293-302:⚠️ Potential issue | 🟡 MinorTest intent and assertions are misaligned here.
The title says it verifies logging on
finishfailure, but no logger assertion is performed.🔧 Proposed assertion fix
it('should log finish error when job.finish fails in wrapperDetached', async () => { + const errorStub = sandbox.stub(logger, 'error'); const finishStub = sandbox.stub(job, 'finish').rejects(new Error('finish-fail')); const wrapped = job.wrapperDetached(JOB_TYPES.GLADYS_GATEWAY_BACKUP, () => { throw new Error('boom'); }); const startedJob = await wrapped(); // Even if finish fails, we should still get a job object back expect(startedJob).to.have.property('id'); + expect(errorStub.called).to.equal(true); finishStub.restore(); + errorStub.restore(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/lib/job/job.test.js` around lines 293 - 302, The test currently stubs job.finish and calls job.wrapperDetached but never asserts that the finish error was logged; update the test to stub the logger used by the wrapper (e.g., sinon.stub(job.logger, 'error') or the actual logger object used inside wrapperDetached), call the wrapped function as before, then assert the logger.error stub was called (and optionally that the call includes the 'finish-fail' error or message), and finally restore both the finish and logger stubs; this ensures job.finish rejection triggers the expected logging behavior for job.wrapperDetached.server/test/lib/job/job.test.js-239-249 (1)
239-249:⚠️ Potential issue | 🟡 MinorIncrease polling timeout to reduce detached-job test flakiness.
Line 245 currently caps wait time to ~400ms, which is tight for async DB-backed status transitions.
🔧 Proposed stabilization
- const waitForStatus = async (jobId, status, attempts = 0) => { + const waitForStatus = async (jobId, status, attempts = 0) => { const jobs = await job.get(); const current = jobs.find((oneJob) => oneJob.id === jobId); if (current && current.status === status) { return current; } - if (attempts >= 40) { + if (attempts >= 200) { throw new Error(`Timeout waiting for job ${jobId} to reach status ${status}`); } - await sleep(10); + await sleep(25); return waitForStatus(jobId, status, attempts + 1); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/lib/job/job.test.js` around lines 239 - 249, The test helper waitForStatus currently retries at most 40 times with a 10ms sleep (≈400ms total) causing flakiness for DB-backed transitions; update the retry logic in waitForStatus to increase the timeout (for example change attempts default from 40 to 400 or increase the sleep from 10 to 50ms) so the total wait is substantially longer, and keep the same error on timeout; adjust the function signature and default parameter in waitForStatus and ensure it still calls job.get() and sleep(...) as before.front/src/routes/settings/settings-background-jobs/JobList.jsx-7-21 (1)
7-21:⚠️ Potential issue | 🟡 MinorGuard against invalid date values in formatter.
If
valueis non-empty but unparsable, this helper returns"Invalid Date"to users. Add an invalid-date guard.Suggested fix
const formatPeriodDate = (value, language) => { if (!value) { return value; } const date = new Date(value); + if (Number.isNaN(date.getTime())) { + return value; + } const hasTime =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/routes/settings/settings-background-jobs/JobList.jsx` around lines 7 - 21, In formatPeriodDate, after constructing const date = new Date(value) add a guard that checks for an unparsable date (e.g. isNaN(date.getTime())) and if true return the original value (or an empty string) instead of letting "Invalid Date" be shown; this change ensures invalid inputs are caught early and prevents date.toLocaleString/toLocaleDateString from being called on an invalid Date object.server/services/energy-monitoring/lib/energy-monitoring.calculateCostRange.js-19-20 (1)
19-20:⚠️ Potential issue | 🟡 MinorAvoid falsy-coercion when defaulting start date.
Line 19 currently treats any falsy value as “from beginning”. That can mask invalid inputs (e.g.
'') and trigger unintended full recalculations.Suggested fix
- const finalStartAt = startAt || new Date(0); + const finalStartAt = startAt === null || startAt === undefined ? new Date(0) : startAt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/energy-monitoring/lib/energy-monitoring.calculateCostRange.js` around lines 19 - 20, The current defaulting uses falsy-coercion for startAt which treats empty strings/0 as "from beginning"; update the logic in energy-monitoring.calculateCostRange.js (the block that computes finalStartAt and calls this.calculateCostFrom) to only default when startAt is null or undefined (e.g. startAt === null || startAt === undefined), and otherwise validate/coerce the provided value to a Date (reject or throw on invalid dates such as empty string or NaN via date.getTime()). Ensure you pass a valid Date to this.calculateCostFrom(jobId, selectors, ...) or surface a clear error instead of silently treating falsy values as epoch.server/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.js-560-560 (1)
560-560:⚠️ Potential issue | 🟡 MinorRename the duplicated test title for clearer failures.
Line 560 repeats the same title used at Line 361, which makes test output ambiguous when one of them fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.js` at line 560, The test title string "should skip consumption features without selector and still process valid ones" is duplicated (appears in the it(...) at the failing block); rename the second occurrence to a unique, descriptive title (for example include context like "when multiple features present" or append "(duplicate case)") so test failures are unambiguous—update the it(...) description in the test case within energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.js to a distinct string while leaving the test body and assertions unchanged.server/services/energy-monitoring/lib/energy-monitoring.jobData.js-49-57 (1)
49-57:⚠️ Potential issue | 🟡 MinorAvoid grouping job-data selections by device name.
Lines 54-57 use
deviceNameas the map key. Different devices can share a name, which merges unrelated feature selections into one entry.💡 Suggested fix
scopedSelectors.forEach((selector) => { const feature = resolveFeature(this.gladys.stateManager, selector); + const deviceId = feature && feature.device_id ? String(feature.device_id) : `unknown:${selector}`; const deviceName = feature && feature.device_id ? (this.gladys.stateManager.get('deviceById', feature.device_id) || {}).name || feature.device_id : 'Unknown device'; const featureName = (feature && feature.name) || selector; - const deviceKey = deviceName; - if (!devicesMap.has(deviceKey)) { - devicesMap.set(deviceKey, { device: deviceName, features: [] }); + if (!devicesMap.has(deviceId)) { + devicesMap.set(deviceId, { device: deviceName, features: [] }); } - devicesMap.get(deviceKey).features.push(featureName); + devicesMap.get(deviceId).features.push(featureName); });Also applies to: 61-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/energy-monitoring/lib/energy-monitoring.jobData.js` around lines 49 - 57, The code is using deviceName as the key in devicesMap which merges different devices that share the same name; change the map key to the unique device id (use feature.device_id or a fallback like selector or 'unknown-device-id') while still storing the human-readable deviceName inside the map value (update deviceKey from deviceName to feature.device_id and ensure entries created via devicesMap.set(deviceKey, { device: deviceName, features: [] }) so the name is preserved), and apply the same change to the other similar block (lines referenced around 61-64) that also uses deviceName as the map key; use this.gladys.stateManager.get('deviceById', feature.device_id) to resolve the name but do not use the name as the map key.server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexFromBeginning.js-22-24 (1)
22-24:⚠️ Potential issue | 🟡 MinorFix the JSDoc example to match the current function signature.
Line 25 defines
(featureSelectors, jobId), but Lines 23-24 still show the old multi-argument call form.✏️ Suggested fix
- * calculateConsumptionFromIndexFromBeginning(null, [], null, '12345678-1234-1234-1234-1234567890ab'); + * calculateConsumptionFromIndexFromBeginning([], '12345678-1234-1234-1234-1234567890ab');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexFromBeginning.js` around lines 22 - 24, The JSDoc example is using the old multi-argument form but the function now accepts (featureSelectors, jobId); update the example in the comment for calculateConsumptionFromIndexFromBeginning to call it with two arguments (first the feature selectors array/object, second the jobId string) so it matches the current signature and clearly demonstrates usage.front/src/routes/integration/all/energy-monitoring/EnergyMonitoring.jsx-188-192 (1)
188-192:⚠️ Potential issue | 🟡 MinorValidate remove/move indices before mutating selection.
Invalid indices can remove the wrong item (e.g.,
-1) or insertundefinedduring drag/drop edge cases.Proposed fix
removeSelectedFeature = index => { this.setState(prev => { const next = [...(prev.selectedFeaturesForRecalc || [])]; + if (index < 0 || index >= next.length) return null; next.splice(index, 1); return { selectedFeaturesForRecalc: next, showConfirmRecalculateAll: false, showConfirmCostAll: false @@ moveSelectedFeature = (from, to) => { if (from === to) return; this.setState(prev => { const list = [...(prev.selectedFeaturesForRecalc || [])]; - const item = list.splice(from, 1)[0]; + if (from < 0 || from >= list.length || to < 0 || to > list.length) return null; + const item = list.splice(from, 1)[0]; + if (!item) return null; list.splice(to, 0, item); return { selectedFeaturesForRecalc: list }; }); };Also applies to: 200-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/routes/integration/all/energy-monitoring/EnergyMonitoring.jsx` around lines 188 - 192, removeSelectedFeature currently mutates the selectedFeaturesForRecalc array using the provided index without validating it, which can remove the wrong element (e.g. -1) or cause undefined inserts during drag/drop; update removeSelectedFeature (and the similar move/insert logic around lines 200-206, e.g. moveSelectedFeature/insertSelectedFeature) to first check that the index is a finite integer and within [0, selected.length-1] before calling splice, and when moving/inserting ensure the item being moved exists and clamp the destination index into the valid range to avoid inserting undefined; if the index is invalid, return the previous state unchanged (or handle gracefully) to prevent corrupting the selection array.
🧹 Nitpick comments (10)
server/test/lib/device/device.destroyStatesBetween.test.js (1)
33-40: Optional: extract repeateddeviceInstancebootstrap into a helper.This setup is duplicated across tests; a small factory will keep future test updates simpler.
Refactor example
+function createDeviceInstance(event, job) { + const variable = { getValue: fake.resolves(null) }; + const stateManager = { get: fake.returns(null) }; + return new Device(event, {}, stateManager, {}, {}, variable, job); +} + describe('Device.destroyStatesBetween', () => { @@ - const variable = { - getValue: fake.resolves(null), - }; - const stateManager = { - get: fake.returns(null), - }; - const deviceInstance = new Device(event, {}, stateManager, {}, {}, variable, job); + const deviceInstance = createDeviceInstance(event, job); @@ - const variable = { - getValue: fake.resolves(null), - }; - const stateManager = { - get: fake.returns(null), - }; - const deviceInstance = new Device(event, {}, stateManager, {}, {}, variable, job); + const deviceInstance = createDeviceInstance(event, job); @@ - const variable = { - getValue: fake.resolves(null), - }; - const stateManager = { - get: fake.returns(null), - }; - const deviceInstance = new Device(event, {}, stateManager, {}, {}, variable, job); + const deviceInstance = createDeviceInstance(event, job);Also applies to: 60-67, 83-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/lib/device/device.destroyStatesBetween.test.js` around lines 33 - 40, Multiple tests duplicate Device bootstrap (new Device(event, {}, stateManager, {}, {}, variable, job)); extract a small factory function (e.g., createDeviceInstance or buildDevice for tests) that constructs and returns a Device using configurable overrides for event, stateManager, variable, and job, then replace the repeated instantiation in the tests (lines around device.destroyStatesBetween.test.js blocks using variable/getValue, stateManager/get, and job) with calls to that helper to reduce duplication and make future changes easier.server/models/job.js (1)
9-9: Tighten date fields to ISO-8601 format validation in the Joi schema.The
current_date,period.start_date, andperiod.end_datefields currently accept arbitrary strings. Add.isoDate()validation to enforce ISO-8601 format at the schema level and prevent malformed date payloads from entering the system.Proposed schema update
- current_date: Joi.string().allow(null, ''), + current_date: Joi.string().isoDate().allow(null, ''), @@ - start_date: Joi.string().allow(null, ''), - end_date: Joi.string().allow(null, ''), + start_date: Joi.string().isoDate().allow(null, ''), + end_date: Joi.string().isoDate().allow(null, ''),The
.allow(null, '')will bypass validation for these falsy values, while non-empty strings will be validated as ISO-8601 dates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/models/job.js` at line 9, Update the Joi schema in server/models/job.js to enforce ISO-8601 date format by adding .isoDate() to the current_date field and to the period object's start_date and end_date validators (while keeping .allow(null, '') to permit null/empty values); locate the schema definition that contains current_date and period and append .isoDate() to those Joi.string() calls so non-empty strings are validated as ISO-8601 dates.server/test/services/energy-monitoring/energy-monitoring.calculateCostFromYesterday.test.js (1)
64-69: Consider adding an assertion forargs[1].The test verifies
args[0],args[2], andargs[3], but skipsargs[1]. Based on the source code (line 13 incalculateCostFromYesterday.js),args[1]should benull. Adding this assertion would ensure complete coverage of the call signature.💡 Proposed enhancement
expect(args[0]).to.equal(yesterday); + expect(args[1]).to.equal(null); expect(args[2]).to.equal(jobId); expect(args[3]).to.equal(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/energy-monitoring/energy-monitoring.calculateCostFromYesterday.test.js` around lines 64 - 69, Add an assertion for the second argument of the calculateCostFrom stub call: in the test that inspects calculateCostFrom.getCall(0) (variable args), assert that args[1] is null to match the call signature from calculateCostFromYesterday (where args[1] is set to null); update the expectations block to include expect(args[1]).to.equal(null) so the test fully covers all call parameters.front/src/components/device/UpdateDeviceFeature.jsx (1)
53-53: Remove stale commented render signature.This comment is dead code and adds noise in a high-traffic component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/components/device/UpdateDeviceFeature.jsx` at line 53, Remove the stale commented render signature in UpdateDeviceFeature.jsx: delete the commented line "// render({ feature, featureIndex, canEditCategory, device, energyHelperBuilder, ...props }) {" so the component (UpdateDeviceFeature / its render method) no longer contains dead commented code and the file stays clean.server/services/energy-monitoring/api/energy-monitoring.controller.js (1)
74-95: Add explicit start/end ordering validation at controller level.Rejecting
start_date > end_dateearly gives clearer API behavior than queuing an effective no-op.🔧 Proposed validation
async function calculateCostRange(req, res) { const featureSelectors = Array.isArray(req.body && req.body.feature_selectors) ? req.body.feature_selectors : []; const startDate = getOptionalDate(req.body, 'start_date'); const endDate = getOptionalDate(req.body, 'end_date'); + if (startDate && endDate && startDate > endDate) { + throw new BadParameters('start_date must be before or equal to end_date'); + } const job = await energyMonitoringHandler.calculateCostRange(startDate, featureSelectors, endDate); res.json({ success: true, job_id: job && job.id, }); } @@ async function calculateConsumptionFromIndexRange(req, res) { const featureSelectors = Array.isArray(req.body && req.body.feature_selectors) ? req.body.feature_selectors : []; const startDate = getOptionalDate(req.body, 'start_date'); const endDate = getOptionalDate(req.body, 'end_date'); + if (startDate && endDate && startDate > endDate) { + throw new BadParameters('start_date must be before or equal to end_date'); + } const job = await energyMonitoringHandler.calculateConsumptionFromIndexRange(startDate, featureSelectors, endDate); res.json({ success: true, job_id: job && job.id, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/energy-monitoring/api/energy-monitoring.controller.js` around lines 74 - 95, Add explicit validation in the controller functions calculateCostRange and calculateConsumptionFromIndexRange after obtaining startDate and endDate via getOptionalDate: if both dates are present and startDate > endDate, immediately return res.status(400).json({ success: false, error: 'start_date must be on or before end_date' }) (or similar message) instead of calling energyMonitoringHandler; perform this check before building feature_selectors and before calling energyMonitoringHandler.calculateCostRange / calculateConsumptionFromIndexRange so invalid ranges are rejected at the controller level.front/src/routes/settings/settings-background-jobs/JobList.jsx (1)
107-113: Use the same date formatter forcurrent_datedisplay.Line 112 prints raw
job.data.current_date; period dates are localized/formatted. ReusingformatPeriodDate(...)here would keep date rendering consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/routes/settings/settings-background-jobs/JobList.jsx` around lines 107 - 113, The Progress date currently renders raw job.data.current_date in JobList.jsx; change it to reuse the existing formatPeriodDate(...) formatter so displayed dates match other period dates — update the Text fields prop to pass date: formatPeriodDate(job.data.current_date) (and import formatPeriodDate if not already imported) so the jobsSettings.currentDate string receives the formatted/localized date.server/test/services/energy-monitoring/lib/energy-monitoring.jobData.test.js (1)
136-150: These two tests are redundant; keep one canonical case.The scenarios at Lines 136-142 and Lines 144-150 both validate “startAt object without selectors ⇒ scope all + period with
end_date: null”. Consider removing one to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/energy-monitoring/lib/energy-monitoring.jobData.test.js` around lines 136 - 150, Two identical tests validate the same behavior of buildJobDataForConsumption: "startAt object without selectors ⇒ scope 'all' and period.end_date null"; remove one redundant test (either keep the "should return scope all when startAt object has no selectors" or the "should normalize when startAt provided without feature selectors") and leave a single canonical test that calls buildJobDataForConsumption with the same ctx and input and asserts the expected { scope: 'all', period: { start_date: '2025-04-01' (or 2025-05-01), end_date: null } } to eliminate duplication.server/test/services/energy-monitoring/energy-monitoring.calculateCostRange.test.js (1)
63-67: Strengthen default-date assertion to verify epoch behavior.Line 63 only checks type. If defaulting regresses from epoch to “now”, this test still passes. Assert
args[0]equalsnew Date(0)explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/energy-monitoring/energy-monitoring.calculateCostRange.test.js` around lines 63 - 67, Test currently only verifies type for args[0] (expect(args[0]).to.be.instanceOf(Date)), which allows regressions from epoch to "now"; replace that assertion with an explicit equality check against the epoch by asserting expect(args[0]).to.deep.equal(new Date(0)) (or add that assertion alongside the existing one) so the default-date behavior is strictly validated for the calculateCostRange test where args[0] is produced.server/lib/job/job.wrapper.js (1)
19-28: Extract duplicatedbuildJobDataflow into a shared helper.Line 19-Line 28 and Line 64-Line 73 implement the same logic. Centralizing this will keep
wrapperandwrapperDetachedbehavior aligned when this logic evolves.Also applies to: 64-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/job/job.wrapper.js` around lines 19 - 28, Extract the duplicated buildJobData logic into a single async helper (e.g., resolveJobData or buildJobDataHelper) that accepts the options object, the this-argument to use for apply, the args array, and the job type/logger; have it call options.buildJobData.apply(thisArg, args) inside a try/catch, validate that the result is an object before returning it, and on error call logger.warn(`job.wrapper: buildJobData failed for job ${type}`, e). Replace the duplicated blocks in wrapper and wrapperDetached with a call to this helper and assign its returned object to jobData (or leave undefined if nothing valid returned) so both wrappers share identical behavior.server/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.js (1)
491-503: Prefer Sinon stubbing withtry/finallyover manual method reassignment.Line 491 mutates
energyMonitoring.calculateConsumptionFromIndexdirectly, and restoration happens only on the success path. If an assertion throws first, later tests may run with a patched method.🔧 Proposed fix
- const originalCalculateConsumptionFromIndex = energyMonitoring.calculateConsumptionFromIndex; - energyMonitoring.calculateConsumptionFromIndex = async (...args) => { + const calcStub = stub(energyMonitoring, 'calculateConsumptionFromIndex').callsFake(async (...args) => { callCount += 1; if (callCount === 2) { throw new Error('Simulated error for testing'); } - return originalCalculateConsumptionFromIndex.apply(energyMonitoring, args); - }; - - const result = await energyMonitoring.calculateConsumptionFromIndexFromBeginning([], 'job-123'); - - // Restore original function - energyMonitoring.calculateConsumptionFromIndex = originalCalculateConsumptionFromIndex; + return EnergyMonitoring.prototype.calculateConsumptionFromIndex.apply(energyMonitoring, args); + }); + let result; + try { + result = await energyMonitoring.calculateConsumptionFromIndexFromBeginning([], 'job-123'); + } finally { + calcStub.restore(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.js` around lines 491 - 503, Replace the manual reassignment of energyMonitoring.calculateConsumptionFromIndex with a Sinon stub and ensure it is restored in a finally block; specifically, create a sinon.stub(energyMonitoring, 'calculateConsumptionFromIndex') that increments callCount and throws on the second call (matching the current simulated error behavior), run the test invoking energyMonitoring.calculateConsumptionFromIndexFromBeginning, then in a finally block restore the stub (or call sinon.restore()) so the originalCalculateConsumptionFromIndex is guaranteed to be reinstated even if assertions fail.
🤖 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/device/index.js`:
- Around line 62-117: The code assumes parentFeature exists and always creates
new derived features, causing duplicates and invalid parent references; first
verify parentFeature is present (check this.state.device.features[featureIndex])
and bail or show an error if missing, and ensure energy_parent_id falls back to
defaultElectricMeterFeatureId if parentFeature.id is not valid; before
creating/pushing consumptionFeature and costFeature (use symbols
consumptionFeatureId, costFeatureId, consumptionFeature, costFeature,
external_id and selector), scan this.state.device.features for existing features
with the same external_id/selector or the same type+energy_parent_id and skip
creating or reuse the existing feature IDs to prevent duplicates and save
conflicts.
In `@front/src/routes/integration/all/energy-monitoring/EnergyMonitoring.jsx`:
- Around line 439-452: The current two-step kickoff (calling httpClient.post for
consumption then for cost) is non-atomic: if consumptionResponse succeeds but
costResponse fails you enqueue a stray consumption job; fix by making the
operation atomic — either create both jobs in one server call or, if doing two
client requests, perform rollback on failure: after successful
consumptionResponse (consumptionResponse.job_id) and a subsequent cost creation
failure, call the job-cancel API (or a dedicated rollback endpoint) with
consumptionResponse.job_id to delete/cancel the created job, and surface errors
only after rollback completes; alternatively, send a combined payload (merge
consumptionSelectors and costSelectors with getRecalculateDatePayload()) to a
single endpoint to create both jobs together. Reference consumptionResponse,
costResponse, getRecalculateDatePayload, and the httpClient.post calls when
implementing the rollback or combined-request approach.
- Around line 780-785: The current logic picks the first consumption child
(consumptionFeature) then only checks that child's immediate children for a cost
child (costFeature), which can incorrectly exclude valid roots if a different
consumption child contains a cost; update the selection to choose a consumption
child that actually has a cost descendant before returning the root: replace the
two-step find with a single search over consumptionChildren that looks for a
child where consumptionTypes.has(child.type) AND (childrenByParent.get(child.id)
|| []).some(c => costTypes.has(c.type)); use those same symbols
(childrenByParent, consumptionChildren, consumptionFeature, costChildren,
costFeature, consumptionTypes, costTypes) so the code locates the correct
consumption node that has a cost child and proceeds with that node.
In
`@server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexFromBeginning.js`:
- Around line 50-62: The filter uses a unified key (feature.selector ||
feature.external_id || feature.id) to match features into consumptionFeatures,
but cleanup/reset logic only registers when feature.selector exists, leaving
stale states for features matched by external_id or id; update the
cleanup/registration code (the block that currently checks feature.selector
before resetting/deleting state) to compute the same featureSelector (const
featureSelector = feature.selector || feature.external_id || feature.id) and use
that key for all state cleanup/registration so filtering and deletion use the
identical identifier (affecting consumptionFeatures selection and the cleanup
logic that references feature.selector).
In
`@server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexRange.js`:
- Around line 68-80: The feature selection uses featureSelector =
feature.selector || feature.external_id || feature.id, but the later state
cleanup only checks feature.selector; update the code so the same computed key
(featureSelector) is used for both selection and cleanup: compute const
featureSelector = f.selector || f.external_id || f.id when iterating/processing
features (used in consumptionFeatures filter and later cleanup loop) and use
that value to look up/reset states in the state cleanup (remove the conditional
that only runs when feature.selector exists) so fallback-matched features are
also reset.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndex.test.js`:
- Around line 824-839: The test stub for device.getDeviceFeatureStates uses the
wrong selector string ('test-energy-device-index') which does not match the
fixture's selector ('test-device-index' in mockDevice), so the whitelist path
isn't exercised; update the stub inside the test (the fake passed to
device.getDeviceFeatureStates) to return the non-empty array when selector ===
'test-device-index' (or align the fixture selector to
'test-energy-device-index') so that
energyMonitoring.calculateConsumptionFromIndex(testTime,
['non-matching-selector']) actually runs the intended code path and the
assertion on device.saveHistoricalState is meaningful.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateCostEveryThirtyMinutes.test.js`:
- Around line 1-11: Remove the duplicate requires/destructurings causing
redeclaration errors: keep a single const { fake, assert } = sinon and a single
const EventEmitter = require('events') and delete the repeated lines; locate the
duplicated declarations of fake/assert and EventEmitter in the test file (they
appear twice) and consolidate them so each symbol is declared only once (ensure
any referenced variables like fake, assert, EventEmitter are still in scope for
the tests).
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateCostFrom.test.js`:
- Around line 30-46: The clearDuckDb helper currently swallows all errors in its
try/catch (inside clearDuckDb and around db.duckDbWriteConnectionAllAsync
calls), hiding real teardown failures; change the catch to only ignore the known
"missing table" condition (or check table existence before DELETE) and for any
other error rethrow or surface it (or at minimum log it via test logger).
Specifically update clearDuckDb to detect the expected duckdb/missing-table
error when calling db.duckDbWriteConnectionAllAsync for each table in the tables
array and ignore only that case; for any other exception, rethrow (or log and
throw) so test setup/teardown failures are not silently suppressed.
- Around line 707-769: The test leaks stubs when assertions throw because
restores run only on the happy path; wrap the setup + assertions in a
try/finally (or switch to a sinon.createSandbox and call sandbox.restore() in
finally) and move getStub.restore(), destroyBetweenStub.restore(),
priceStub.restore(), statesStub.restore(), and rootStub.restore() into the
finally block (or replace individual stubs with sandbox.stub(...) and call
sandbox.restore()) for the test that calls energyMonitoring.calculateCostFrom
and the other similar test at lines ~803-865 so stubs are always cleaned up.
---
Minor comments:
In `@front/src/components/device/UpdateDeviceFeature.jsx`:
- Around line 59-60: The CTA for creating energy features is not gated by modify
permissions; update the logic in the UpdateDeviceFeature component so the
showCreateEnergyFeaturesButton boolean includes the allowModifyFeatures flag
(e.g., set showCreateEnergyFeaturesButton = isEnergyIndex &&
!hasConsumptionFeatures && allowModifyFeatures) and also wrap the CTA render
(the JSX block around lines 216-230) to check allowModifyFeatures before
rendering, ensuring both the computed flag and the UI render require
allowModifyFeatures to be true.
In `@front/src/routes/integration/all/energy-monitoring/EnergyMonitoring.jsx`:
- Around line 188-192: removeSelectedFeature currently mutates the
selectedFeaturesForRecalc array using the provided index without validating it,
which can remove the wrong element (e.g. -1) or cause undefined inserts during
drag/drop; update removeSelectedFeature (and the similar move/insert logic
around lines 200-206, e.g. moveSelectedFeature/insertSelectedFeature) to first
check that the index is a finite integer and within [0, selected.length-1]
before calling splice, and when moving/inserting ensure the item being moved
exists and clamp the destination index into the valid range to avoid inserting
undefined; if the index is invalid, return the previous state unchanged (or
handle gracefully) to prevent corrupting the selection array.
In `@front/src/routes/settings/settings-background-jobs/JobList.jsx`:
- Around line 7-21: In formatPeriodDate, after constructing const date = new
Date(value) add a guard that checks for an unparsable date (e.g.
isNaN(date.getTime())) and if true return the original value (or an empty
string) instead of letting "Invalid Date" be shown; this change ensures invalid
inputs are caught early and prevents date.toLocaleString/toLocaleDateString from
being called on an invalid Date object.
In `@server/lib/device/device.destroyStatesBetween.js`:
- Around line 14-20: In destroyStatesBetween, validate the date inputs before
calling formatDateInUTC: ensure `from` and `to` are valid Date instances (or
parsable) and that `from <= to`; if not, throw a clear error (e.g.,
BadRequestError) so callers fail deterministically; perform these checks after
finding the DeviceFeature (db.DeviceFeature.findOne) and before computing
formattedFrom/formattedTo with formatDateInUTC, rejecting inverted ranges and
invalid dates.
In `@server/lib/job/job.updateProgress.js`:
- Around line 13-14: The JSDoc example is wrong — replace the example that calls
gladys.job.finish with one that calls gladys.job.updateProgress and shows the
correct parameters; update the example usage to call
gladys.job.updateProgress('<jobId>', <progressNumber>, <dataPatchObject?>)
(matching the updateProgress(id, progress, dataPatch) signature) so it
demonstrates passing the job id, a numeric progress value (0–100) and an
optional dataPatch object.
In
`@server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexFromBeginning.js`:
- Around line 22-24: The JSDoc example is using the old multi-argument form but
the function now accepts (featureSelectors, jobId); update the example in the
comment for calculateConsumptionFromIndexFromBeginning to call it with two
arguments (first the feature selectors array/object, second the jobId string) so
it matches the current signature and clearly demonstrates usage.
In
`@server/services/energy-monitoring/lib/energy-monitoring.calculateCostFromBeginning.js`:
- Around line 8-10: The JSDoc example calls calculateCostFromBeginning with four
args but the function signature is calculateCostFromBeginning(featureSelectors,
jobId); update the example to use two arguments only (e.g.
calculateCostFromBeginning(null, '12345678-1234-1234-1234-1234567890ab') or
calculateCostFromBeginning([], '12345678-1234-1234-1234-1234567890ab')) so the
example matches the actual function parameters and intent.
In
`@server/services/energy-monitoring/lib/energy-monitoring.calculateCostRange.js`:
- Around line 19-20: The current defaulting uses falsy-coercion for startAt
which treats empty strings/0 as "from beginning"; update the logic in
energy-monitoring.calculateCostRange.js (the block that computes finalStartAt
and calls this.calculateCostFrom) to only default when startAt is null or
undefined (e.g. startAt === null || startAt === undefined), and otherwise
validate/coerce the provided value to a Date (reject or throw on invalid dates
such as empty string or NaN via date.getTime()). Ensure you pass a valid Date to
this.calculateCostFrom(jobId, selectors, ...) or surface a clear error instead
of silently treating falsy values as epoch.
In `@server/services/energy-monitoring/lib/energy-monitoring.jobData.js`:
- Around line 49-57: The code is using deviceName as the key in devicesMap which
merges different devices that share the same name; change the map key to the
unique device id (use feature.device_id or a fallback like selector or
'unknown-device-id') while still storing the human-readable deviceName inside
the map value (update deviceKey from deviceName to feature.device_id and ensure
entries created via devicesMap.set(deviceKey, { device: deviceName, features: []
}) so the name is preserved), and apply the same change to the other similar
block (lines referenced around 61-64) that also uses deviceName as the map key;
use this.gladys.stateManager.get('deviceById', feature.device_id) to resolve the
name but do not use the name as the map key.
In `@server/test/lib/job/job.test.js`:
- Around line 293-302: The test currently stubs job.finish and calls
job.wrapperDetached but never asserts that the finish error was logged; update
the test to stub the logger used by the wrapper (e.g., sinon.stub(job.logger,
'error') or the actual logger object used inside wrapperDetached), call the
wrapped function as before, then assert the logger.error stub was called (and
optionally that the call includes the 'finish-fail' error or message), and
finally restore both the finish and logger stubs; this ensures job.finish
rejection triggers the expected logging behavior for job.wrapperDetached.
- Around line 239-249: The test helper waitForStatus currently retries at most
40 times with a 10ms sleep (≈400ms total) causing flakiness for DB-backed
transitions; update the retry logic in waitForStatus to increase the timeout
(for example change attempts default from 40 to 400 or increase the sleep from
10 to 50ms) so the total wait is substantially longer, and keep the same error
on timeout; adjust the function signature and default parameter in waitForStatus
and ensure it still calls job.get() and sleep(...) as before.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.js`:
- Line 560: The test title string "should skip consumption features without
selector and still process valid ones" is duplicated (appears in the it(...) at
the failing block); rename the second occurrence to a unique, descriptive title
(for example include context like "when multiple features present" or append
"(duplicate case)") so test failures are unambiguous—update the it(...)
description in the test case within
energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.js to a
distinct string while leaving the test body and assertions unchanged.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateCostFromEndToEnd.test.js`:
- Around line 33-51: Remove the locally defined clearDuckDb function (the one
that iterates tables and calls db.duckDbWriteConnectionAllAsync) because it
shadows the imported clearDuckDb from ../../utils/duckdb; instead use the
imported clearDuckDb which includes the isMissingTableError handling. Locate and
delete the local clearDuckDb declaration in this test file and replace any local
calls (or rely on existing calls) to use the imported clearDuckDb symbol so the
improved missing-table error check is used.
---
Nitpick comments:
In `@front/src/components/device/UpdateDeviceFeature.jsx`:
- Line 53: Remove the stale commented render signature in
UpdateDeviceFeature.jsx: delete the commented line "// render({ feature,
featureIndex, canEditCategory, device, energyHelperBuilder, ...props }) {" so
the component (UpdateDeviceFeature / its render method) no longer contains dead
commented code and the file stays clean.
In `@front/src/routes/settings/settings-background-jobs/JobList.jsx`:
- Around line 107-113: The Progress date currently renders raw
job.data.current_date in JobList.jsx; change it to reuse the existing
formatPeriodDate(...) formatter so displayed dates match other period dates —
update the Text fields prop to pass date:
formatPeriodDate(job.data.current_date) (and import formatPeriodDate if not
already imported) so the jobsSettings.currentDate string receives the
formatted/localized date.
In `@server/lib/job/job.wrapper.js`:
- Around line 19-28: Extract the duplicated buildJobData logic into a single
async helper (e.g., resolveJobData or buildJobDataHelper) that accepts the
options object, the this-argument to use for apply, the args array, and the job
type/logger; have it call options.buildJobData.apply(thisArg, args) inside a
try/catch, validate that the result is an object before returning it, and on
error call logger.warn(`job.wrapper: buildJobData failed for job ${type}`, e).
Replace the duplicated blocks in wrapper and wrapperDetached with a call to this
helper and assign its returned object to jobData (or leave undefined if nothing
valid returned) so both wrappers share identical behavior.
In `@server/models/job.js`:
- Line 9: Update the Joi schema in server/models/job.js to enforce ISO-8601 date
format by adding .isoDate() to the current_date field and to the period object's
start_date and end_date validators (while keeping .allow(null, '') to permit
null/empty values); locate the schema definition that contains current_date and
period and append .isoDate() to those Joi.string() calls so non-empty strings
are validated as ISO-8601 dates.
In `@server/services/energy-monitoring/api/energy-monitoring.controller.js`:
- Around line 74-95: Add explicit validation in the controller functions
calculateCostRange and calculateConsumptionFromIndexRange after obtaining
startDate and endDate via getOptionalDate: if both dates are present and
startDate > endDate, immediately return res.status(400).json({ success: false,
error: 'start_date must be on or before end_date' }) (or similar message)
instead of calling energyMonitoringHandler; perform this check before building
feature_selectors and before calling energyMonitoringHandler.calculateCostRange
/ calculateConsumptionFromIndexRange so invalid ranges are rejected at the
controller level.
In `@server/test/lib/device/device.destroyStatesBetween.test.js`:
- Around line 33-40: Multiple tests duplicate Device bootstrap (new
Device(event, {}, stateManager, {}, {}, variable, job)); extract a small factory
function (e.g., createDeviceInstance or buildDevice for tests) that constructs
and returns a Device using configurable overrides for event, stateManager,
variable, and job, then replace the repeated instantiation in the tests (lines
around device.destroyStatesBetween.test.js blocks using variable/getValue,
stateManager/get, and job) with calls to that helper to reduce duplication and
make future changes easier.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.js`:
- Around line 491-503: Replace the manual reassignment of
energyMonitoring.calculateConsumptionFromIndex with a Sinon stub and ensure it
is restored in a finally block; specifically, create a
sinon.stub(energyMonitoring, 'calculateConsumptionFromIndex') that increments
callCount and throws on the second call (matching the current simulated error
behavior), run the test invoking
energyMonitoring.calculateConsumptionFromIndexFromBeginning, then in a finally
block restore the stub (or call sinon.restore()) so the
originalCalculateConsumptionFromIndex is guaranteed to be reinstated even if
assertions fail.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateCostFromYesterday.test.js`:
- Around line 64-69: Add an assertion for the second argument of the
calculateCostFrom stub call: in the test that inspects
calculateCostFrom.getCall(0) (variable args), assert that args[1] is null to
match the call signature from calculateCostFromYesterday (where args[1] is set
to null); update the expectations block to include
expect(args[1]).to.equal(null) so the test fully covers all call parameters.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateCostRange.test.js`:
- Around line 63-67: Test currently only verifies type for args[0]
(expect(args[0]).to.be.instanceOf(Date)), which allows regressions from epoch to
"now"; replace that assertion with an explicit equality check against the epoch
by asserting expect(args[0]).to.deep.equal(new Date(0)) (or add that assertion
alongside the existing one) so the default-date behavior is strictly validated
for the calculateCostRange test where args[0] is produced.
In
`@server/test/services/energy-monitoring/lib/energy-monitoring.jobData.test.js`:
- Around line 136-150: Two identical tests validate the same behavior of
buildJobDataForConsumption: "startAt object without selectors ⇒ scope 'all' and
period.end_date null"; remove one redundant test (either keep the "should return
scope all when startAt object has no selectors" or the "should normalize when
startAt provided without feature selectors") and leave a single canonical test
that calls buildJobDataForConsumption with the same ctx and input and asserts
the expected { scope: 'all', period: { start_date: '2025-04-01' (or 2025-05-01),
end_date: null } } to eliminate duplication.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
front/src/components/device/UpdateDevice.jsxfront/src/components/device/UpdateDeviceFeature.jsxfront/src/components/device/index.jsfront/src/components/drag-and-drop/DeviceListWithDragAndDrop.jsxfront/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/integration/all/energy-monitoring/EnergyMonitoring.jsxfront/src/routes/integration/all/mqtt/device-page/setup/Feature.jsxfront/src/routes/settings/settings-background-jobs/JobList.jsxserver/lib/device/device.destroyStatesBetween.jsserver/lib/device/index.jsserver/lib/job/index.jsserver/lib/job/job.finish.jsserver/lib/job/job.updateProgress.jsserver/lib/job/job.wrapper.jsserver/models/job.jsserver/services/energy-monitoring/api/energy-monitoring.controller.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndex.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexFromBeginning.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexRange.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexThirtyMinutes.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateCostEveryThirtyMinutes.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateCostFromBeginning.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateCostFromYesterday.jsserver/services/energy-monitoring/lib/energy-monitoring.calculateCostRange.jsserver/services/energy-monitoring/lib/energy-monitoring.jobData.jsserver/services/energy-monitoring/lib/index.jsserver/test/lib/device/device.destroyStatesBetween.test.jsserver/test/lib/job/job.test.jsserver/test/services/checks.test.jsserver/test/services/energy-monitoring/api/energy-monitoring.controller.test.jsserver/test/services/energy-monitoring/contracts/contracts.buildEdfTempoDayMap.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndex.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexFromBeginning.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexRange.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndexThirtyMinutes.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateCostEveryThirtyMinutes.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateCostFrom.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateCostFromBeginning.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateCostFromEndToEnd.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateCostFromYesterday.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateCostRange.test.jsserver/test/services/energy-monitoring/energy-monitoring.init.test.jsserver/test/services/energy-monitoring/index.test.jsserver/test/services/energy-monitoring/lib/energy-monitoring.jobData.test.jsserver/test/utils/duckdb.jsserver/utils/constants.js
Comment on lines
+62
to
+117
| const parentFeature = this.state.device.features[featureIndex]; | ||
| const consumptionFeatureId = uuid.v4(); | ||
| const costFeatureId = uuid.v4(); | ||
|
|
||
| // Get translated names from dictionary | ||
| const consumptionName = get( | ||
| this.props.intl.dictionary, | ||
| `deviceFeatureCategory.${DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR}.${DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION}` | ||
| ); | ||
| const costName = get( | ||
| this.props.intl.dictionary, | ||
| `deviceFeatureCategory.${DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR}.${DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION_COST}` | ||
| ); | ||
|
|
||
| // Get default electric meter feature ID to set as energy_parent_id on the INDEX feature | ||
| let defaultElectricMeterFeatureId = null; | ||
| try { | ||
| const response = await this.props.httpClient.get('/api/v1/energy_price/default_electric_meter_feature_id'); | ||
| defaultElectricMeterFeatureId = response.feature_id; | ||
| } catch (e) { | ||
| console.error('Failed to get default electric meter feature ID', e); | ||
| } | ||
|
|
||
| // Create THIRTY_MINUTES_CONSUMPTION feature with parent as energy_parent_id | ||
| const consumptionFeature = { | ||
| id: consumptionFeatureId, | ||
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | ||
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION, | ||
| external_id: `${parentFeature.external_id}:consumption`, | ||
| selector: `${parentFeature.selector}_consumption`, | ||
| name: `${parentFeature.name} - ${consumptionName}`, | ||
| unit: DEVICE_FEATURE_UNITS.KILOWATT_HOUR, | ||
| read_only: true, | ||
| has_feedback: false, | ||
| keep_history: true, | ||
| min: 0, | ||
| max: 1000000000, | ||
| energy_parent_id: parentFeature.id | ||
| }; | ||
|
|
||
| // Create THIRTY_MINUTES_CONSUMPTION_COST feature with consumption as energy_parent_id | ||
| const costFeature = { | ||
| id: costFeatureId, | ||
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | ||
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION_COST, | ||
| external_id: `${parentFeature.external_id}:cost`, | ||
| selector: `${parentFeature.selector}_cost`, | ||
| name: `${parentFeature.name} - ${costName}`, | ||
| unit: DEVICE_FEATURE_UNITS.EURO, | ||
| read_only: true, | ||
| has_feedback: false, | ||
| keep_history: true, | ||
| min: 0, | ||
| max: 1000000000, | ||
| energy_parent_id: consumptionFeatureId | ||
| }; |
There was a problem hiding this comment.
Prevent duplicate derived features and invalid parent access.
Line 62 assumes the parent feature is always present and complete, and Lines 127/133 always push two new derived features. Re-triggering this action can create duplicates (external_id/selector) and lead to save conflicts.
🔧 Proposed fix
async createEnergyConsumptionFeatures(featureIndex) {
const parentFeature = this.state.device.features[featureIndex];
+ if (!parentFeature || !parentFeature.external_id || !parentFeature.selector) {
+ return;
+ }
+
+ const consumptionExternalId = `${parentFeature.external_id}:consumption`;
+ const costExternalId = `${parentFeature.external_id}:cost`;
+ const alreadyExists = this.state.device.features.some(
+ (feature) => feature.external_id === consumptionExternalId || feature.external_id === costExternalId,
+ );
+ if (alreadyExists) {
+ return;
+ }
+
const consumptionFeatureId = uuid.v4();
const costFeatureId = uuid.v4();
@@
- external_id: `${parentFeature.external_id}:consumption`,
+ external_id: consumptionExternalId,
@@
- external_id: `${parentFeature.external_id}:cost`,
+ external_id: costExternalId,📝 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.
Suggested change
| const parentFeature = this.state.device.features[featureIndex]; | |
| const consumptionFeatureId = uuid.v4(); | |
| const costFeatureId = uuid.v4(); | |
| // Get translated names from dictionary | |
| const consumptionName = get( | |
| this.props.intl.dictionary, | |
| `deviceFeatureCategory.${DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR}.${DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION}` | |
| ); | |
| const costName = get( | |
| this.props.intl.dictionary, | |
| `deviceFeatureCategory.${DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR}.${DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION_COST}` | |
| ); | |
| // Get default electric meter feature ID to set as energy_parent_id on the INDEX feature | |
| let defaultElectricMeterFeatureId = null; | |
| try { | |
| const response = await this.props.httpClient.get('/api/v1/energy_price/default_electric_meter_feature_id'); | |
| defaultElectricMeterFeatureId = response.feature_id; | |
| } catch (e) { | |
| console.error('Failed to get default electric meter feature ID', e); | |
| } | |
| // Create THIRTY_MINUTES_CONSUMPTION feature with parent as energy_parent_id | |
| const consumptionFeature = { | |
| id: consumptionFeatureId, | |
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | |
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION, | |
| external_id: `${parentFeature.external_id}:consumption`, | |
| selector: `${parentFeature.selector}_consumption`, | |
| name: `${parentFeature.name} - ${consumptionName}`, | |
| unit: DEVICE_FEATURE_UNITS.KILOWATT_HOUR, | |
| read_only: true, | |
| has_feedback: false, | |
| keep_history: true, | |
| min: 0, | |
| max: 1000000000, | |
| energy_parent_id: parentFeature.id | |
| }; | |
| // Create THIRTY_MINUTES_CONSUMPTION_COST feature with consumption as energy_parent_id | |
| const costFeature = { | |
| id: costFeatureId, | |
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | |
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION_COST, | |
| external_id: `${parentFeature.external_id}:cost`, | |
| selector: `${parentFeature.selector}_cost`, | |
| name: `${parentFeature.name} - ${costName}`, | |
| unit: DEVICE_FEATURE_UNITS.EURO, | |
| read_only: true, | |
| has_feedback: false, | |
| keep_history: true, | |
| min: 0, | |
| max: 1000000000, | |
| energy_parent_id: consumptionFeatureId | |
| }; | |
| const parentFeature = this.state.device.features[featureIndex]; | |
| if (!parentFeature || !parentFeature.external_id || !parentFeature.selector) { | |
| return; | |
| } | |
| const consumptionExternalId = `${parentFeature.external_id}:consumption`; | |
| const costExternalId = `${parentFeature.external_id}:cost`; | |
| const alreadyExists = this.state.device.features.some( | |
| (feature) => feature.external_id === consumptionExternalId || feature.external_id === costExternalId, | |
| ); | |
| if (alreadyExists) { | |
| return; | |
| } | |
| const consumptionFeatureId = uuid.v4(); | |
| const costFeatureId = uuid.v4(); | |
| // Get translated names from dictionary | |
| const consumptionName = get( | |
| this.props.intl.dictionary, | |
| `deviceFeatureCategory.${DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR}.${DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION}` | |
| ); | |
| const costName = get( | |
| this.props.intl.dictionary, | |
| `deviceFeatureCategory.${DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR}.${DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION_COST}` | |
| ); | |
| // Get default electric meter feature ID to set as energy_parent_id on the INDEX feature | |
| let defaultElectricMeterFeatureId = null; | |
| try { | |
| const response = await this.props.httpClient.get('/api/v1/energy_price/default_electric_meter_feature_id'); | |
| defaultElectricMeterFeatureId = response.feature_id; | |
| } catch (e) { | |
| console.error('Failed to get default electric meter feature ID', e); | |
| } | |
| // Create THIRTY_MINUTES_CONSUMPTION feature with parent as energy_parent_id | |
| const consumptionFeature = { | |
| id: consumptionFeatureId, | |
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | |
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION, | |
| external_id: consumptionExternalId, | |
| selector: `${parentFeature.selector}_consumption`, | |
| name: `${parentFeature.name} - ${consumptionName}`, | |
| unit: DEVICE_FEATURE_UNITS.KILOWATT_HOUR, | |
| read_only: true, | |
| has_feedback: false, | |
| keep_history: true, | |
| min: 0, | |
| max: 1000000000, | |
| energy_parent_id: parentFeature.id | |
| }; | |
| // Create THIRTY_MINUTES_CONSUMPTION_COST feature with consumption as energy_parent_id | |
| const costFeature = { | |
| id: costFeatureId, | |
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | |
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION_COST, | |
| external_id: costExternalId, | |
| selector: `${parentFeature.selector}_cost`, | |
| name: `${parentFeature.name} - ${costName}`, | |
| unit: DEVICE_FEATURE_UNITS.EURO, | |
| read_only: true, | |
| has_feedback: false, | |
| keep_history: true, | |
| min: 0, | |
| max: 1000000000, | |
| energy_parent_id: consumptionFeatureId | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/src/components/device/index.js` around lines 62 - 117, The code assumes
parentFeature exists and always creates new derived features, causing duplicates
and invalid parent references; first verify parentFeature is present (check
this.state.device.features[featureIndex]) and bail or show an error if missing,
and ensure energy_parent_id falls back to defaultElectricMeterFeatureId if
parentFeature.id is not valid; before creating/pushing consumptionFeature and
costFeature (use symbols consumptionFeatureId, costFeatureId,
consumptionFeature, costFeature, external_id and selector), scan
this.state.device.features for existing features with the same
external_id/selector or the same type+energy_parent_id and skip creating or
reuse the existing feature IDs to prevent duplicates and save conflicts.
Comment on lines
+439
to
+452
| const consumptionResponse = await this.props.httpClient.post(consumptionUrl, { | ||
| feature_selectors: consumptionSelectors, | ||
| ...this.getRecalculateDatePayload() | ||
| }); | ||
| if (!consumptionResponse || consumptionResponse.success !== true || !consumptionResponse.job_id) { | ||
| throw new Error('job_not_created'); | ||
| } | ||
| const costResponse = await this.props.httpClient.post(costUrl, { | ||
| feature_selectors: costSelectors, | ||
| ...this.getRecalculateDatePayload() | ||
| }); | ||
| if (!costResponse || costResponse.success !== true || !costResponse.job_id) { | ||
| throw new Error('job_not_created'); | ||
| } |
There was a problem hiding this comment.
Combined kickoff is non-atomic and can create duplicate work on retry.
If consumption job creation succeeds but cost job creation fails, the UI reports a failure path for the whole action. Retrying can enqueue a second consumption recalculation unintentionally.
Also applies to: 459-464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/src/routes/integration/all/energy-monitoring/EnergyMonitoring.jsx`
around lines 439 - 452, The current two-step kickoff (calling httpClient.post
for consumption then for cost) is non-atomic: if consumptionResponse succeeds
but costResponse fails you enqueue a stray consumption job; fix by making the
operation atomic — either create both jobs in one server call or, if doing two
client requests, perform rollback on failure: after successful
consumptionResponse (consumptionResponse.job_id) and a subsequent cost creation
failure, call the job-cancel API (or a dedicated rollback endpoint) with
consumptionResponse.job_id to delete/cancel the created job, and surface errors
only after rollback completes; alternatively, send a combined payload (merge
consumptionSelectors and costSelectors with getRecalculateDatePayload()) to a
single endpoint to create both jobs together. Reference consumptionResponse,
costResponse, getRecalculateDatePayload, and the httpClient.post calls when
implementing the rollback or combined-request approach.
Comment on lines
+780
to
+785
| const consumptionChildren = childrenByParent.get(feature.id) || []; | ||
| const consumptionFeature = consumptionChildren.find(child => consumptionTypes.has(child.type)); | ||
| if (!consumptionFeature) return; | ||
| const costChildren = childrenByParent.get(consumptionFeature.id) || []; | ||
| const costFeature = costChildren.find(child => costTypes.has(child.type)); | ||
| if (!costFeature) return; |
There was a problem hiding this comment.
Candidate discovery can skip valid recalculation targets.
Line 781 picks the first consumption child, then Lines 783-785 require a cost child under that specific node. If another consumption child has a valid cost chain, the root is incorrectly excluded.
Proposed fix
- const consumptionChildren = childrenByParent.get(feature.id) || [];
- const consumptionFeature = consumptionChildren.find(child => consumptionTypes.has(child.type));
- if (!consumptionFeature) return;
- const costChildren = childrenByParent.get(consumptionFeature.id) || [];
- const costFeature = costChildren.find(child => costTypes.has(child.type));
+ const consumptionChildren = childrenByParent.get(feature.id) || [];
+ const consumptionFeature = consumptionChildren.find(child => {
+ if (!consumptionTypes.has(child.type)) return false;
+ const costChildren = childrenByParent.get(child.id) || [];
+ return costChildren.some(costChild => costTypes.has(costChild.type));
+ });
+ if (!consumptionFeature) return;
+ const costChildren = childrenByParent.get(consumptionFeature.id) || [];
+ const costFeature = costChildren.find(child => costTypes.has(child.type));
if (!costFeature) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@front/src/routes/integration/all/energy-monitoring/EnergyMonitoring.jsx`
around lines 780 - 785, The current logic picks the first consumption child
(consumptionFeature) then only checks that child's immediate children for a cost
child (costFeature), which can incorrectly exclude valid roots if a different
consumption child contains a cost; update the selection to choose a consumption
child that actually has a cost descendant before returning the root: replace the
two-step find with a single search over consumptionChildren that looks for a
child where consumptionTypes.has(child.type) AND (childrenByParent.get(child.id)
|| []).some(c => costTypes.has(c.type)); use those same symbols
(childrenByParent, consumptionChildren, consumptionFeature, costChildren,
costFeature, consumptionTypes, costTypes) so the code locates the correct
consumption node that has a cost child and proceeds with that node.
Comment on lines
+50
to
+62
| const consumptionFeatures = energyDevice.features.filter((f) => { | ||
| const isConsumption = | ||
| f.category === DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR && | ||
| f.type === DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION, | ||
| ); | ||
|
|
||
| if (indexFeatures.length > 0 && consumptionFeatures.length > 0) { | ||
| f.type === DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION; | ||
| if (!isConsumption) { | ||
| return false; | ||
| } | ||
| if (selectorSet.size === 0) { | ||
| return true; | ||
| } | ||
| const featureSelector = f.selector || f.external_id || f.id; | ||
| return selectorSet.has(featureSelector); | ||
| }); |
There was a problem hiding this comment.
Use the same feature key for filtering and reset/deletion.
Line 60 allows matching via selector || external_id || id, but Lines 160-164 only register cleanup when feature.selector exists. That can leave old states for fallback-matched features and skew recalculation results.
💡 Suggested fix
+ const getFeatureKey = (feature) =>
+ feature.selector || feature.external_id || (feature.id != null ? String(feature.id) : null);
+
energyDevices.forEach((energyDevice) => {
@@
- const featureSelector = f.selector || f.external_id || f.id;
+ const featureSelector = getFeatureKey(f);
return selectorSet.has(featureSelector);
});
@@
const consumptionFeaturesToReset = new Map();
devicesWithBothFeatures.forEach((deviceWithBothFeatures) => {
(deviceWithBothFeatures.consumptionFeatures || []).forEach((feature) => {
- if (!feature.selector) {
+ const featureKey = getFeatureKey(feature);
+ if (!featureKey) {
return;
}
- consumptionFeaturesToReset.set(feature.selector, feature);
+ consumptionFeaturesToReset.set(featureKey, featureKey);
});
});
- await Promise.each(Array.from(consumptionFeaturesToReset.values()), async (feature) => {
- const { selector } = feature;
- await this.gladys.device.destroyStatesFrom(selector, deletionStartTime);
+ await Promise.each(Array.from(consumptionFeaturesToReset.values()), async (featureKey) => {
+ await this.gladys.device.destroyStatesFrom(featureKey, deletionStartTime);
});Also applies to: 156-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexFromBeginning.js`
around lines 50 - 62, The filter uses a unified key (feature.selector ||
feature.external_id || feature.id) to match features into consumptionFeatures,
but cleanup/reset logic only registers when feature.selector exists, leaving
stale states for features matched by external_id or id; update the
cleanup/registration code (the block that currently checks feature.selector
before resetting/deleting state) to compute the same featureSelector (const
featureSelector = feature.selector || feature.external_id || feature.id) and use
that key for all state cleanup/registration so filtering and deletion use the
identical identifier (affecting consumptionFeatures selection and the cleanup
logic that references feature.selector).
Comment on lines
+68
to
+80
| const consumptionFeatures = energyDevice.features.filter((f) => { | ||
| const isConsumption = | ||
| f.category === DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR && | ||
| f.type === DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION; | ||
| if (!isConsumption) { | ||
| return false; | ||
| } | ||
| if (selectorSet.size === 0) { | ||
| return true; | ||
| } | ||
| const featureSelector = f.selector || f.external_id || f.id; | ||
| return selectorSet.has(featureSelector); | ||
| }); |
There was a problem hiding this comment.
Align feature key usage between selection and state cleanup.
Line 78 matches features with selector || external_id || id, but Lines 195-199 only reset states when feature.selector exists. This can recalculate on top of old states for fallback-matched features.
💡 Suggested fix
+ const getFeatureKey = (feature) =>
+ feature.selector || feature.external_id || (feature.id != null ? String(feature.id) : null);
+
energyDevices.forEach((energyDevice) => {
@@
- const featureSelector = f.selector || f.external_id || f.id;
+ const featureSelector = getFeatureKey(f);
return selectorSet.has(featureSelector);
});
@@
- const consumptionFeaturesToReset = new Map();
+ const consumptionFeaturesToReset = new Map();
devicesWithBothFeatures.forEach((deviceWithBothFeatures) => {
(deviceWithBothFeatures.consumptionFeatures || []).forEach((feature) => {
- if (!feature.selector) {
+ const featureKey = getFeatureKey(feature);
+ if (!featureKey) {
return;
}
- consumptionFeaturesToReset.set(feature.selector, feature);
+ consumptionFeaturesToReset.set(featureKey, { feature, featureKey });
});
});
- await Promise.each(Array.from(consumptionFeaturesToReset.values()), async (feature) => {
- const { selector } = feature;
+ await Promise.each(Array.from(consumptionFeaturesToReset.values()), async ({ featureKey }) => {
if (parsedEndAt) {
- await this.gladys.device.destroyStatesBetween(selector, deletionStartTime, deletionEndTime);
+ await this.gladys.device.destroyStatesBetween(featureKey, deletionStartTime, deletionEndTime);
} else {
- await this.gladys.device.destroyStatesFrom(selector, deletionStartTime);
+ await this.gladys.device.destroyStatesFrom(featureKey, deletionStartTime);
}
});Also applies to: 193-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndexRange.js`
around lines 68 - 80, The feature selection uses featureSelector =
feature.selector || feature.external_id || feature.id, but the later state
cleanup only checks feature.selector; update the code so the same computed key
(featureSelector) is used for both selection and cleanup: compute const
featureSelector = f.selector || f.external_id || f.id when iterating/processing
features (used in consumptionFeatures filter and later cleanup loop) and use
that value to look up/reset states in the state cleanup (remove the conditional
that only runs when feature.selector exists) so fallback-matched features are
also reset.
Comment on lines
+824
to
+839
| device.getDeviceFeatureStates = fake((selector) => { | ||
| if (selector === 'test-energy-device-index') { | ||
| return [ | ||
| { created_at: '2023-10-03T13:30:00.000Z', value: 1000 }, | ||
| { created_at: '2023-10-03T14:00:00.000Z', value: 1010 }, | ||
| ]; | ||
| } | ||
| return []; | ||
| }); | ||
|
|
||
| device.saveHistoricalState.resetHistory(); | ||
|
|
||
| await energyMonitoring.calculateConsumptionFromIndex(testTime, ['non-matching-selector']); | ||
|
|
||
| expect(device.saveHistoricalState.called).to.equal(false); | ||
| }); |
There was a problem hiding this comment.
Whitelist test currently does not exercise the intended path.
Line 825 uses test-energy-device-index, but the fixture selector is test-device-index (see mockDevice). This makes the stub return [], so the assertion can pass even if whitelist logic is broken.
Suggested fix
- device.getDeviceFeatureStates = fake((selector) => {
- if (selector === 'test-energy-device-index') {
+ device.getDeviceFeatureStates = fake((selector) => {
+ if (selector === 'test-device-index') {
return [
{ created_at: '2023-10-03T13:30:00.000Z', value: 1000 },
{ created_at: '2023-10-03T14:00:00.000Z', value: 1010 },
];
}
return [];
});
@@
await energyMonitoring.calculateConsumptionFromIndex(testTime, ['non-matching-selector']);
+ expect(device.getDeviceFeatureStates.calledOnce).to.equal(true);
expect(device.saveHistoricalState.called).to.equal(false);📝 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.
Suggested change
| device.getDeviceFeatureStates = fake((selector) => { | |
| if (selector === 'test-energy-device-index') { | |
| return [ | |
| { created_at: '2023-10-03T13:30:00.000Z', value: 1000 }, | |
| { created_at: '2023-10-03T14:00:00.000Z', value: 1010 }, | |
| ]; | |
| } | |
| return []; | |
| }); | |
| device.saveHistoricalState.resetHistory(); | |
| await energyMonitoring.calculateConsumptionFromIndex(testTime, ['non-matching-selector']); | |
| expect(device.saveHistoricalState.called).to.equal(false); | |
| }); | |
| device.getDeviceFeatureStates = fake((selector) => { | |
| if (selector === 'test-device-index') { | |
| return [ | |
| { created_at: '2023-10-03T13:30:00.000Z', value: 1000 }, | |
| { created_at: '2023-10-03T14:00:00.000Z', value: 1010 }, | |
| ]; | |
| } | |
| return []; | |
| }); | |
| device.saveHistoricalState.resetHistory(); | |
| await energyMonitoring.calculateConsumptionFromIndex(testTime, ['non-matching-selector']); | |
| expect(device.getDeviceFeatureStates.calledOnce).to.equal(true); | |
| expect(device.saveHistoricalState.called).to.equal(false); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateConsumptionFromIndex.test.js`
around lines 824 - 839, The test stub for device.getDeviceFeatureStates uses the
wrong selector string ('test-energy-device-index') which does not match the
fixture's selector ('test-device-index' in mockDevice), so the whitelist path
isn't exercised; update the stub inside the test (the fake passed to
device.getDeviceFeatureStates) to return the non-empty array when selector ===
'test-device-index' (or align the fixture selector to
'test-energy-device-index') so that
energyMonitoring.calculateConsumptionFromIndex(testTime,
['non-matching-selector']) actually runs the intended code path and the
assertion on device.saveHistoricalState is meaningful.
Comment on lines
+30
to
+46
| const clearDuckDb = async () => { | ||
| const tables = [ | ||
| 't_device_feature_state', | ||
| 't_device_feature_state_aggregate', | ||
| 't_energy_price', | ||
| 't_device_feature', | ||
| 't_device_param', | ||
| 't_device', | ||
| ]; | ||
| // eslint-disable-next-line no-restricted-syntax | ||
| for (const table of tables) { | ||
| try { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await db.duckDbWriteConnectionAllAsync(`DELETE FROM ${table}`); | ||
| } catch (e) { | ||
| // ignore missing tables for DuckDB | ||
| } |
There was a problem hiding this comment.
Don’t suppress all database cleanup errors in test helper
On Line 44–46, the catch swallows every failure. That can hide real setup/teardown failures and make tests pass when isolation is actually broken.
Suggested fix
for (const table of tables) {
try {
// eslint-disable-next-line no-await-in-loop
await db.duckDbWriteConnectionAllAsync(`DELETE FROM ${table}`);
} catch (e) {
- // ignore missing tables for DuckDB
+ // ignore only missing-table errors, rethrow everything else
+ if (!/does not exist|no such table/i.test(e.message || '')) {
+ throw e;
+ }
}
}📝 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.
Suggested change
| const clearDuckDb = async () => { | |
| const tables = [ | |
| 't_device_feature_state', | |
| 't_device_feature_state_aggregate', | |
| 't_energy_price', | |
| 't_device_feature', | |
| 't_device_param', | |
| 't_device', | |
| ]; | |
| // eslint-disable-next-line no-restricted-syntax | |
| for (const table of tables) { | |
| try { | |
| // eslint-disable-next-line no-await-in-loop | |
| await db.duckDbWriteConnectionAllAsync(`DELETE FROM ${table}`); | |
| } catch (e) { | |
| // ignore missing tables for DuckDB | |
| } | |
| const clearDuckDb = async () => { | |
| const tables = [ | |
| 't_device_feature_state', | |
| 't_device_feature_state_aggregate', | |
| 't_energy_price', | |
| 't_device_feature', | |
| 't_device_param', | |
| 't_device', | |
| ]; | |
| // eslint-disable-next-line no-restricted-syntax | |
| for (const table of tables) { | |
| try { | |
| // eslint-disable-next-line no-await-in-loop | |
| await db.duckDbWriteConnectionAllAsync(`DELETE FROM ${table}`); | |
| } catch (e) { | |
| // ignore only missing-table errors, rethrow everything else | |
| if (!/does not exist|no such table/i.test(e.message || '')) { | |
| throw e; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateCostFrom.test.js`
around lines 30 - 46, The clearDuckDb helper currently swallows all errors in
its try/catch (inside clearDuckDb and around db.duckDbWriteConnectionAllAsync
calls), hiding real teardown failures; change the catch to only ignore the known
"missing table" condition (or check table existence before DELETE) and for any
other error rethrow or surface it (or at minimum log it via test logger).
Specifically update clearDuckDb to detect the expected duckdb/missing-table
error when calling db.duckDbWriteConnectionAllAsync for each table in the tables
array and ignore only that case; for any other exception, rethrow (or log and
throw) so test setup/teardown failures are not silently suppressed.
Comment on lines
+707
to
+769
| it('should return null when no price matches a state (energyPricesForDate empty)', async () => { | ||
| const energyMonitoring = new EnergyMonitoring(gladys, 'b8c55219-0dc2-4a32-8d3d-6a7b2d4a1c22'); | ||
| // Force a valid hierarchy with one consumption/cost pair | ||
| const getStub = sinon.stub(gladys.device, 'get').resolves([ | ||
| { | ||
| id: 'meter-device', | ||
| name: 'Meter', | ||
| features: [ | ||
| { | ||
| id: 'meter-feature', | ||
| selector: 'meter-feature', | ||
| external_id: 'meter-feature', | ||
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | ||
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.DAILY_CONSUMPTION, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| id: 'plug-device', | ||
| name: 'Plug', | ||
| features: [ | ||
| { | ||
| id: 'plug-consumption', | ||
| selector: 'plug-consumption', | ||
| external_id: 'plug-consumption', | ||
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | ||
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION, | ||
| energy_parent_id: 'meter-feature', | ||
| }, | ||
| { | ||
| id: 'plug-cost', | ||
| selector: 'plug-cost', | ||
| external_id: 'plug-cost', | ||
| category: DEVICE_FEATURE_CATEGORIES.ENERGY_SENSOR, | ||
| type: DEVICE_FEATURE_TYPES.ENERGY_SENSOR.THIRTY_MINUTES_CONSUMPTION_COST, | ||
| energy_parent_id: 'plug-consumption', | ||
| }, | ||
| ], | ||
| }, | ||
| ]); | ||
| const destroyBetweenStub = sinon.stub(gladys.device, 'destroyStatesBetween').resolves(); | ||
| const priceStub = sinon.stub(gladys.energyPrice, 'get').resolves([]); // no prices | ||
| const statesStub = sinon | ||
| .stub(gladys.device, 'getDeviceFeatureStates') | ||
| .resolves([{ created_at: new Date('2025-10-01T00:00:00.000Z') }]); | ||
| const rootStub = sinon | ||
| .stub(gladys.device.energySensorManager, 'getRootElectricMeterDevice') | ||
| .returns({ id: 'meter-feature', device_id: 'meter-device' }); | ||
| const res = await energyMonitoring.calculateCostFrom( | ||
| new Date('2025-10-01T00:00:00.000Z'), | ||
| undefined, | ||
| null, | ||
| '2025-10-02', | ||
| ); | ||
| expect(res).to.equal(null); | ||
| expect(destroyBetweenStub.calledOnce).to.equal(true); | ||
| expect(statesStub.calledOnce).to.equal(true); | ||
| getStub.restore(); | ||
| destroyBetweenStub.restore(); | ||
| priceStub.restore(); | ||
| statesStub.restore(); | ||
| rootStub.restore(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "energy-monitoring.calculateCostFrom.test.js" | head -5Repository: GladysAssistant/Gladys
Length of output: 150
🏁 Script executed:
sed -n '707,769p' server/test/services/energy-monitoring/energy-monitoring.calculateCostFrom.test.jsRepository: GladysAssistant/Gladys
Length of output: 2475
🏁 Script executed:
sed -n '803,865p' server/test/services/energy-monitoring/energy-monitoring.calculateCostFrom.test.jsRepository: GladysAssistant/Gladys
Length of output: 2593
🏁 Script executed:
rg -A 3 "afterEach|describe|beforeEach|sandbox" server/test/services/energy-monitoring/energy-monitoring.calculateCostFrom.test.js | head -40Repository: GladysAssistant/Gladys
Length of output: 391
Always restore stubs in finally (or sandbox) to prevent test leakage
In these test cases, restore calls run only on the happy path. If an assertion throws before the restore statements execute, stubs remain active and leak into subsequent tests, causing flakiness.
Suggested fix pattern
- const getStub = sinon.stub(gladys.device, 'get').resolves([...]);
- const destroyBetweenStub = sinon.stub(gladys.device, 'destroyStatesBetween').resolves();
- ...
- const res = await energyMonitoring.calculateCostFrom(...);
- expect(res).to.equal(null);
- ...
- getStub.restore();
- destroyBetweenStub.restore();
- ...
+ const getStub = sinon.stub(gladys.device, 'get').resolves([...]);
+ const destroyBetweenStub = sinon.stub(gladys.device, 'destroyStatesBetween').resolves();
+ ...
+ try {
+ const res = await energyMonitoring.calculateCostFrom(...);
+ expect(res).to.equal(null);
+ ...
+ } finally {
+ getStub.restore();
+ destroyBetweenStub.restore();
+ ...
+ }Also applies to: lines 803-865
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/test/services/energy-monitoring/energy-monitoring.calculateCostFrom.test.js`
around lines 707 - 769, The test leaks stubs when assertions throw because
restores run only on the happy path; wrap the setup + assertions in a
try/finally (or switch to a sinon.createSandbox and call sandbox.restore() in
finally) and move getStub.restore(), destroyBetweenStub.restore(),
priceStub.restore(), statesStub.restore(), and rootStub.restore() into the
finally block (or replace individual stubs with sandbox.stub(...) and call
sandbox.restore()) for the test that calls energyMonitoring.calculateCostFrom
and the other similar test at lines ~803-865 so stubs are always cleaned up.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.js (1)
51-51: Defaulting to epoch date on invalid input may cause unexpected bulk processing.If
startAtis neither a validDatenor a parseable string,parsedStartAtsilently falls back tonew Date(0)(1970-01-01). This could cause the function to attempt processing decades of data, leading to performance issues or unexpected behavior.Consider throwing an error or logging a warning when
startAtcannot be parsed, rather than silently defaulting to epoch.🛡️ Proposed fix to validate startAt
- const parsedStartAt = parseDateWithBoundary(startAt, 'start') || new Date(0); + const parsedStartAt = parseDateWithBoundary(startAt, 'start'); + if (!parsedStartAt) { + logger.warn(`Invalid startAt value provided: ${startAt}, defaulting to epoch`); + } + const effectiveStartAt = parsedStartAt || new Date(0);Then use
effectiveStartAtthroughout the function. Alternatively, throw an error if a valid start date is required:- const parsedStartAt = parseDateWithBoundary(startAt, 'start') || new Date(0); + const parsedStartAt = parseDateWithBoundary(startAt, 'start'); + if (!parsedStartAt) { + throw new Error(`Invalid startAt value: ${startAt}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.js` at line 51, The current line sets parsedStartAt = parseDateWithBoundary(startAt, 'start') || new Date(0) which silently defaults to epoch on invalid input; update calculateCostFrom to validate parseDateWithBoundary result instead: call parseDateWithBoundary(startAt, 'start') into parsedStartAt, and if it is falsy throw a descriptive error or log a warning and return early (or use an explicitly computed effectiveStartAt only if valid), then use parsedStartAt/effectiveStartAt throughout the function; reference the parseDateWithBoundary, parsedStartAt, startAt and calculateCostFrom identifiers when making the change so you replace the silent fallback with explicit validation/early exit or a controlled fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@server/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.js`:
- Line 51: The current line sets parsedStartAt = parseDateWithBoundary(startAt,
'start') || new Date(0) which silently defaults to epoch on invalid input;
update calculateCostFrom to validate parseDateWithBoundary result instead: call
parseDateWithBoundary(startAt, 'start') into parsedStartAt, and if it is falsy
throw a descriptive error or log a warning and return early (or use an
explicitly computed effectiveStartAt only if valid), then use
parsedStartAt/effectiveStartAt throughout the function; reference the
parseDateWithBoundary, parsedStartAt, startAt and calculateCostFrom identifiers
when making the change so you replace the silent fallback with explicit
validation/early exit or a controlled fallback.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.jsserver/test/services/energy-monitoring/energy-monitoring.calculateCostEveryThirtyMinutes.test.jsserver/test/services/energy-monitoring/energy-monitoring.calculateCostFromEndToEnd.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/test/services/energy-monitoring/energy-monitoring.calculateCostFromEndToEnd.test.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2412 +/- ##
==========================================
+ Coverage 98.79% 98.83% +0.03%
==========================================
Files 1006 1010 +4
Lines 17478 17877 +399
==========================================
+ Hits 17268 17669 +401
+ Misses 210 208 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9 tasks
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
3 participants
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
This PR adds detailed task/job logging around energy recalculation. It is intended to be stacked on top of
Terdious:energy-recalc-date-and-multi-select, after the logging changes have been removed from that branch.Summary by CodeRabbit
New Features
API
Chores
Tests