Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds offset-aware sunrise/sunset triggers: frontend UI to select direction and minutes, trigger schema gains integer Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Frontend UI
participant Scene as Scene Model
participant Scheduler as Daily Scheduler
participant EventBus as Event Bus
participant Matcher as Trigger Matcher
participant Queue as Action Queue
User->>UI: Configure trigger (direction + minutes)
UI->>Scene: Save trigger with offset
Scheduler->>Scene: Collect active triggers & offsets
Scheduler->>Scheduler: Deduplicate offsets per house/type
loop For each (house, type, offset)
Scheduler->>Scheduler: Compute base time + offset
Scheduler->>Scheduler: Schedule job
Scheduler->>EventBus: Emit sunrise/sunset event (includes offset)
EventBus->>Matcher: Deliver event
Matcher->>Matcher: Check house selector AND offset match
alt offsets match
Matcher->>Queue: Enqueue trigger actions
else mismatch
Matcher-->>EventBus: Skip
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
server/models/scene.js (1)
119-119: Consider bounding theoffsetrange.Line 119 accepts any integer, including extreme values that can shift execution far outside expected sunrise/sunset behavior. A bounded range (for example ±1440 minutes) would make this safer.
♻️ Suggested schema guard
- offset: Joi.number().integer(), + offset: Joi.number().integer().min(-1440).max(1440),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/models/scene.js` at line 119, The Joi schema for the scene model currently allows any integer for the offset field (symbol: offset in server/models/scene.js); restrict its range by updating the schema validation to a bounded integer (for example use .min(-1440).max(1440)) so offsets cannot exceed ±1440 minutes, and run/update any tests that exercise scene validation to ensure the new bounds are enforced without breaking callers expecting the previous behavior.server/test/lib/scene/scene.dailyUpdate.test.js (1)
185-192: Use shared constants in the dedup test trigger type.Line 185 and Line 191 use
'time.sunrise'directly. PreferEVENTS.TIME.SUNRISEfor consistency and safer refactors.🔧 Small consistency patch
- triggers: [{ type: 'time.sunrise', house: 'house-1', offset: 30 }], + triggers: [{ type: EVENTS.TIME.SUNRISE, house: 'house-1', offset: 30 }], ... - triggers: [{ type: 'time.sunrise', house: 'house-1', offset: 30 }], + triggers: [{ type: EVENTS.TIME.SUNRISE, house: 'house-1', offset: 30 }],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/lib/scene/scene.dailyUpdate.test.js` around lines 185 - 192, Replace the literal trigger type strings 'time.sunrise' in the dedup tests by using the shared constant EVENTS.TIME.SUNRISE to keep consistency and enable safe refactors; locate the two addScene calls on the test where triggers are defined (the ones passing triggers: [{ type: 'time.sunrise', ... }]) and update the type property to EVENTS.TIME.SUNRISE, ensuring EVENTS is imported or available in the test scope alongside the sceneManager/addScene usage.server/lib/scene/scene.dailyUpdate.js (1)
52-57: Normalize offsets before adding to sets to avoid mixed-type duplicates.Line 52 currently accepts raw
trigger.offsetvalues as-is. Normalizing to finite integers makes scheduling more robust against malformed persisted values.🛡️ Defensive normalization
- const offset = trigger.offset || 0; + const parsedOffset = Number(trigger.offset ?? 0); + if (!Number.isFinite(parsedOffset)) { + return; + } + const offset = Math.trunc(parsedOffset);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/scene/scene.dailyUpdate.js` around lines 52 - 57, Normalize trigger.offset to a finite integer before adding to sunriseOffsets or sunsetOffsets to avoid mixed-type duplicates and malformed values: inside the conditional that checks trigger.type against EVENTS.TIME.SUNRISE / EVENTS.TIME.SUNSET and trigger.house === house.selector, coerce trigger.offset (e.g., via Number.parseInt or Math.trunc after Number()) and validate Number.isFinite on the result (default to 0 or skip if invalid), then add that normalized integer to sunriseOffsets or sunsetOffsets instead of the raw trigger.offset.server/test/lib/scene/scene.checkTrigger.test.js (1)
540-577: Add the positive sunset negative-offset match case.You already test
offset=-15mismatch at Line 540. Add the matching case (event.offset = -15) to complete the regression matrix.✅ Suggested companion test
+ it('should execute scene, sunset trigger with offset=-15 when event has offset=-15', async () => { + sceneManager.addScene({ + selector: 'my-scene', + active: true, + actions: [[{ type: ACTIONS.LIGHT.TURN_OFF, devices: ['light-1'] }]], + triggers: [{ type: EVENTS.TIME.SUNSET, house: 'house-1', offset: -15 }], + }); + sceneManager.checkTrigger({ + type: EVENTS.TIME.SUNSET, + house: { selector: 'house-1' }, + offset: -15, + }); + return new Promise((resolve, reject) => { + sceneManager.queue.start(() => { + try { + assert.calledOnce(device.setValue); + resolve(); + } catch (e) { + reject(e); + } + }); + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/lib/scene/scene.checkTrigger.test.js` around lines 540 - 577, Add a positive test case mirroring the existing negative-offset mismatch: create a scene via sceneManager.addScene with a sunset trigger offset: -15 (same as the current test), then call sceneManager.checkTrigger with type: EVENTS.TIME.SUNSET, house: { selector: 'house-1' }, and offset: -15 (matching the scene). In the returned Promise where sceneManager.queue.start(...) runs, assert that device.setValue was called (or calledOnce) to confirm the scene executed; reuse the same structure and identifiers (sceneManager.addScene, sceneManager.checkTrigger, sceneManager.queue.start, device.setValue) as the existing test.front/src/routes/scene/edit-scene/triggers/SunriseSunsetTrigger.jsx (1)
56-62: KeepoffsetMinutesInputsynchronized whentrigger.offsetchanges externally.Line 58 initializes
offsetMinutesInputonce from props. Ifthis.props.trigger.offsetis later replaced by parent state updates, the input can display stale minutes.♻️ Suggested sync guard
+ componentDidUpdate(prevProps) { + if (prevProps.trigger.offset !== this.props.trigger.offset) { + const nextMinutes = Math.abs(this.props.trigger.offset || 0); + this.setState({ + offsetMinutesInput: nextMinutes > 0 ? String(nextMinutes) : '30', + }); + } + } + componentDidMount() { this.getHouses(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/routes/scene/edit-scene/triggers/SunriseSunsetTrigger.jsx` around lines 56 - 62, The constructor currently sets state.offsetMinutesInput from props.trigger.offset only once, causing the input to become stale if props.change later; add a synchronization guard (e.g., componentDidUpdate(prevProps)) that compares prevProps.trigger.offset to this.props.trigger.offset and, if different, recomputes initialMinutes = Math.abs(this.props.trigger.offset || 0) and updates state.offsetMinutesInput (but only if the existing state.offsetMinutesInput does not already equal the desired string value to avoid clobbering user edits). Target the class constructor and add componentDidUpdate in the SunriseSunsetTrigger component, referencing offsetMinutesInput and props.trigger.offset.
🤖 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/config/i18n/fr.json`:
- Line 2618: Update the French translation for the atExactTime key to use the
correct accented capital A: replace the value "A l'heure exacte" with "À l'heure
exacte" (ensure the file is saved with UTF-8 encoding so the accent is preserved
and tests/linting pick up the change).
---
Nitpick comments:
In `@front/src/routes/scene/edit-scene/triggers/SunriseSunsetTrigger.jsx`:
- Around line 56-62: The constructor currently sets state.offsetMinutesInput
from props.trigger.offset only once, causing the input to become stale if
props.change later; add a synchronization guard (e.g.,
componentDidUpdate(prevProps)) that compares prevProps.trigger.offset to
this.props.trigger.offset and, if different, recomputes initialMinutes =
Math.abs(this.props.trigger.offset || 0) and updates state.offsetMinutesInput
(but only if the existing state.offsetMinutesInput does not already equal the
desired string value to avoid clobbering user edits). Target the class
constructor and add componentDidUpdate in the SunriseSunsetTrigger component,
referencing offsetMinutesInput and props.trigger.offset.
In `@server/lib/scene/scene.dailyUpdate.js`:
- Around line 52-57: Normalize trigger.offset to a finite integer before adding
to sunriseOffsets or sunsetOffsets to avoid mixed-type duplicates and malformed
values: inside the conditional that checks trigger.type against
EVENTS.TIME.SUNRISE / EVENTS.TIME.SUNSET and trigger.house === house.selector,
coerce trigger.offset (e.g., via Number.parseInt or Math.trunc after Number())
and validate Number.isFinite on the result (default to 0 or skip if invalid),
then add that normalized integer to sunriseOffsets or sunsetOffsets instead of
the raw trigger.offset.
In `@server/models/scene.js`:
- Line 119: The Joi schema for the scene model currently allows any integer for
the offset field (symbol: offset in server/models/scene.js); restrict its range
by updating the schema validation to a bounded integer (for example use
.min(-1440).max(1440)) so offsets cannot exceed ±1440 minutes, and run/update
any tests that exercise scene validation to ensure the new bounds are enforced
without breaking callers expecting the previous behavior.
In `@server/test/lib/scene/scene.checkTrigger.test.js`:
- Around line 540-577: Add a positive test case mirroring the existing
negative-offset mismatch: create a scene via sceneManager.addScene with a sunset
trigger offset: -15 (same as the current test), then call
sceneManager.checkTrigger with type: EVENTS.TIME.SUNSET, house: { selector:
'house-1' }, and offset: -15 (matching the scene). In the returned Promise where
sceneManager.queue.start(...) runs, assert that device.setValue was called (or
calledOnce) to confirm the scene executed; reuse the same structure and
identifiers (sceneManager.addScene, sceneManager.checkTrigger,
sceneManager.queue.start, device.setValue) as the existing test.
In `@server/test/lib/scene/scene.dailyUpdate.test.js`:
- Around line 185-192: Replace the literal trigger type strings 'time.sunrise'
in the dedup tests by using the shared constant EVENTS.TIME.SUNRISE to keep
consistency and enable safe refactors; locate the two addScene calls on the test
where triggers are defined (the ones passing triggers: [{ type: 'time.sunrise',
... }]) and update the type property to EVENTS.TIME.SUNRISE, ensuring EVENTS is
imported or available in the test scope alongside the sceneManager/addScene
usage.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
front/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/scene/edit-scene/triggers/SunriseSunsetTrigger.jsxserver/lib/scene/scene.dailyUpdate.jsserver/lib/scene/scene.triggers.jsserver/models/scene.jsserver/test/lib/scene/scene.checkTrigger.test.jsserver/test/lib/scene/scene.dailyUpdate.test.js
#4118 Bundle Size — 11.44MiB (+0.03%).88d35a8(current) vs 198578d master#4106(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #4118 |
Baseline #4106 |
|
|---|---|---|
 Initial JS |
6.42MiB(+0.06%) |
6.42MiB |
 Initial CSS |
310.54KiB |
310.54KiB |
 Cache Invalidation |
56% |
0% |
| Chunks |
51 |
51 |
| Assets |
179 |
179 |
| Modules |
1643 |
1643 |
| Duplicate Modules |
21 |
21 |
| Duplicate Code |
0.94% |
0.94% |
| Packages |
136 |
136 |
| Duplicate Packages |
2 |
2 |
Bundle size by type 1 change 1 regression
| Current #4118 |
Baseline #4106 |
|
|---|---|---|
| JS |
8.32MiB (+0.04%) |
8.32MiB |
| IMG |
2.68MiB |
2.68MiB |
| CSS |
328.39KiB |
328.39KiB |
| Fonts |
93.55KiB |
93.55KiB |
| Other |
18.82KiB |
18.82KiB |
| HTML |
13.58KiB |
13.58KiB |
Bundle analysis report Branch cicoub13:feat-sunrise-sunset-del... Project dashboard
Generated by RelativeCI Documentation Report issue
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2474 +/- ##
==========================================
- Coverage 98.80% 98.80% -0.01%
==========================================
Files 1007 1009 +2
Lines 17609 17681 +72
==========================================
+ Hits 17399 17469 +70
- Misses 210 212 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/lib/scene/scene.dailyUpdate.js`:
- Around line 56-67: The code currently uses trigger.offset without validation
before adding to sunriseBase/sunsetBase and scheduling via
sunriseOffsets/sunsetOffsets; validate that trigger.offset is a finite number
and clamp or reject values that would schedule outside the same calendar day
(e.g., ensure resulting time >= startOfDay and < startOfNextDay) before adding
to sunriseOffsets/sunsetOffsets. Update the logic that builds
sunriseOffsets/sunsetOffsets (where trigger.offset is read) to coerce to Number,
check isFinite, and bound the offset (or compute
sunriseBase.add(offset,'minute') and only include it if the resulting Date falls
within the intended day) so the scheduling loops that use sunriseBase/sunsetBase
and sunriseOffsets.forEach / sunsetOffsets.forEach only receive safe offsets.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
front/src/config/i18n/fr.jsonserver/lib/scene/scene.dailyUpdate.jsserver/test/lib/scene/scene.dailyUpdate.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- front/src/config/i18n/fr.json
- server/test/lib/scene/scene.dailyUpdate.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/test/lib/scene/scene.dailyUpdate.test.js (1)
120-148: Tighten emitted offset assertions to exact expectations.Current
includechecks can still pass if extra/wrong offsets are emitted. Assert the full emitted set per scenario.✅ Suggested assertion hardening
- expect(emittedOffsets).to.include(0); - expect(emittedOffsets).to.include(30); + expect(emittedOffsets.sort((a, b) => a - b)).to.deep.equal([0, 0, 30]); ... - expect(emittedOffsets).to.include(0); - expect(emittedOffsets).to.include(-15); + expect(emittedOffsets.sort((a, b) => a - b)).to.deep.equal([-15, 0, 0]);Also applies to: 150-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/lib/scene/scene.dailyUpdate.test.js` around lines 120 - 148, The test currently uses loose include assertions on emittedOffsets; replace them with exact expectations by asserting the array contains exactly the expected offsets (including duplicates) for the scheduled jobs produced by sceneManager.dailyUpdate and executed via sceneManager.jobs callbacks. Specifically, after collecting emittedOffsets from the overridden event.emit, assert emittedOffsets.length === 3 and compare emittedOffsets (or a sorted copy) to the exact expected array [0,0,30] (or the scenario-specific expected values) so the test fails on extra/missing/wrong offsets; update the same pattern in the other test(s) around the same block (150-177) as well.
🤖 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/test/lib/scene/scene.dailyUpdate.test.js`:
- Around line 120-148: The test currently uses loose include assertions on
emittedOffsets; replace them with exact expectations by asserting the array
contains exactly the expected offsets (including duplicates) for the scheduled
jobs produced by sceneManager.dailyUpdate and executed via sceneManager.jobs
callbacks. Specifically, after collecting emittedOffsets from the overridden
event.emit, assert emittedOffsets.length === 3 and compare emittedOffsets (or a
sorted copy) to the exact expected array [0,0,30] (or the scenario-specific
expected values) so the test fails on extra/missing/wrong offsets; update the
same pattern in the other test(s) around the same block (150-177) as well.
Pierre-Gilles
requested changes
Mar 30, 2026
Contributor
Pierre-Gilles
left a comment
Pierre-Gilles
left a comment
There was a problem hiding this comment.
Thanks a lot for this PR! Many people have been asking for this, so it’s a really great addition.
I’ve added a few comments. The one from CodeRabbit is also very relevant.
Let me know if you need any help with anything 🙂
cicoub13
force-pushed
the
feat-sunrise-sunset-delay
branch
from
3f97b0d to
d601476
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/lib/scene/scene.dailyUpdate.js (1)
56-58:⚠️ Potential issue | 🟠 MajorReject offsets against the computed sunrise/sunset day, not just ±24h.
Math.abs(offset) > 24 * 60still accepts values that spill into the previous/next day for the actual event time (for example,+1000after a 06:00 sunrise). Those jobs can then be canceled by the nextdailyUpdate()before they ever fire.Number(trigger.offset) || 0also turns non-numeric values into the exact-time bucket instead of rejecting them.Suggested hardening
- const offset = Number(trigger.offset) || 0; - if (!Number.isInteger(offset) || Math.abs(offset) > 24 * 60) { + const offset = Number(trigger.offset ?? 0); + const base = + trigger.type === EVENTS.TIME.SUNRISE + ? sunriseBase + : trigger.type === EVENTS.TIME.SUNSET + ? sunsetBase + : null; + if (!base || !Number.isInteger(offset) || !base.add(offset, 'minute').isSame(base, 'day')) { + logger.warn(`Ignoring invalid ${trigger.type} offset "${trigger.offset}" for house "${house.selector}"`); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/scene/scene.dailyUpdate.js` around lines 56 - 58, The code currently uses Number(trigger.offset) || 0 and only checks Math.abs(offset) > 24*60 which lets non-numeric offsets become 0 and allows offsets that push the scheduled minute into the previous/next day; update dailyUpdate() to (1) parse trigger.offset with Number/parseInt and reject when it's not a finite numeric value (don't coerce non-numerics to 0), and (2) after computing the base event minute (the computed sunrise/sunset minute for that day), validate that baseMinute + offset remains within the same day (e.g., 0 <= baseMinute + offset < 24*60); if either check fails, return/cancel the trigger. Reference: trigger.offset, offset, and the dailyUpdate() scheduling flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/lib/scene/scene.destroy.js`:
- Around line 47-53: Reorder the sunrise/sunset check to mirror addScene.js by
evaluating hadSunriseSunset from this.scenes[selector] via
hasSunriseSunsetTrigger(...) before calling this.cancelTriggers(selector), then
call await this.dailyUpdate() (not bare this.dailyUpdate()) so destroy()
properly waits for job rescheduling; update the destroy() function to compute
hadSunriseSunset first, then cancelTriggers, delete this.scenes[selector], and
finally await this.dailyUpdate().
---
Duplicate comments:
In `@server/lib/scene/scene.dailyUpdate.js`:
- Around line 56-58: The code currently uses Number(trigger.offset) || 0 and
only checks Math.abs(offset) > 24*60 which lets non-numeric offsets become 0 and
allows offsets that push the scheduled minute into the previous/next day; update
dailyUpdate() to (1) parse trigger.offset with Number/parseInt and reject when
it's not a finite numeric value (don't coerce non-numerics to 0), and (2) after
computing the base event minute (the computed sunrise/sunset minute for that
day), validate that baseMinute + offset remains within the same day (e.g., 0 <=
baseMinute + offset < 24*60); if either check fails, return/cancel the trigger.
Reference: trigger.offset, offset, and the dailyUpdate() scheduling flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05bc0d75-47b4-4612-b226-f53b97efab94
📒 Files selected for processing (14)
front/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/scene/edit-scene/triggers/SunriseSunsetTrigger.jsxserver/lib/scene/scene.addScene.jsserver/lib/scene/scene.dailyUpdate.jsserver/lib/scene/scene.destroy.jsserver/lib/scene/scene.triggers.jsserver/models/scene.jsserver/test/lib/scene/scene.addScene.test.jsserver/test/lib/scene/scene.checkTrigger.test.jsserver/test/lib/scene/scene.dailyUpdate.test.jsserver/test/lib/scene/scene.destroy.test.jsserver/test/lib/scene/triggers/scene.trigger.sunriseSunset.test.js
💤 Files with no reviewable changes (1)
- server/test/lib/scene/scene.checkTrigger.test.js
✅ Files skipped from review due to trivial changes (5)
- server/models/scene.js
- server/lib/scene/scene.addScene.js
- front/src/config/i18n/de.json
- front/src/config/i18n/en.json
- front/src/config/i18n/fr.json
🚧 Files skipped from review as they are similar to previous changes (3)
- server/lib/scene/scene.triggers.js
- server/test/lib/scene/scene.dailyUpdate.test.js
- front/src/routes/scene/edit-scene/triggers/SunriseSunsetTrigger.jsx
cicoub13
force-pushed
the
feat-sunrise-sunset-delay
branch
6 times, most recently
from
3e1a36d to
d4eeaba
Compare
- Check offset range to avoid out of day scheduled job - Move tests from sunset/sunrise triggers into a dedicated file - Trigger dailyUpdate when scene containing sunrise/sunset triggers is modified
cicoub13
force-pushed
the
feat-sunrise-sunset-delay
branch
from
d4eeaba to
9c773c2
Compare
Contributor
Author
|
Last commit optimized Before After |
Pierre-Gilles
requested changes
Apr 4, 2026
Contributor
Pierre-Gilles
left a comment
Pierre-Gilles
left a comment
There was a problem hiding this comment.
Thanks for the fix, very nice PR!
I've spotted a minor issue, but otherwise it looks good :)
I need to try it now!
server/lib/scene/scene.addScene.js
Outdated
| this.scenes[scene.selector] = scene; | ||
| this.brain.addNamedEntity('scene', scene.selector, scene.name); | ||
| if (!skipDailyUpdate && (hasSunriseSunsetTrigger(scene) || hadSunriseSunset)) { | ||
| this.dailyUpdate().catch((e) => logger.error(e)); |
Contributor
There was a problem hiding this comment.
Is this really the behavior we want? Shouldn’t addScene only resolve once dailyUpdate has completed?
I know addScene is currently synchronous, but is there any issue with making it async?
Contributor
Author
There was a problem hiding this comment.
I made the dailyUpdate call behind an await + addScene async.
You can check impacts in all callers in the commit.
I decided to keep the call from scene.init.js without await (if you have 100 scenes, it can cause some latency, even if dailyUpdate is called once at the end).
Contributor
Pierre-Gilles
left a comment
Pierre-Gilles
left a comment
There was a problem hiding this comment.
I’m not sure I see this case handled in the code, but I might be missing something.
What happens if I create a scene with a “sunrise” trigger, then update the scene to remove that trigger?
Is the previously scheduled trigger properly cancelled?
Contributor
Author
|
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
2 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)Add offset before/after sunrise/sunset in scene trigger
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests