Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/server/src/services/rollUtils.ts (1)
69-85: Consider reducing worst-case complexity in overnight lookup.This nested scan can become O(n²) in large rundowns. Precomputing overnight candidates (or caching the next overnight index) would keep
loadRoll()closer to linear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/rollUtils.ts` around lines 69 - 85, The nested scan over metadata.playableEventOrder inside loadRoll() makes overnight lookup O(n²); before the main loop, build a precomputed map/array of overnight candidate indices (e.g., nextOvernightIndexByPlayableIndex or overnightCandidates list) by scanning metadata.playableEventOrder once to record indices j where rundown.entries[j].timeStart > timeEnd, then in the inner logic replace the for-loop that checks futureEventId/futureEvent/crossesMidnight with a constant-time lookup into that precomputed structure and call checkIsNow(...) and getTimedIndexFromPlayableIndex(metadata, j) only for those candidates so the overall routine stays near-linear.apps/server/src/stores/__tests__/runtimeState.test.ts (1)
301-397: Please add a pending-roll midnight test path.These tests cover crossing midnight while already running an event, but not while
Playback.Rollis pending onsecondaryTimer. That branch has separate rollover logic and should be regression-covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/stores/__tests__/runtimeState.test.ts` around lines 301 - 397, Add a test that simulates the "pending-roll on secondaryTimer" path: init the same overnight mockRundown (use makeRundown, initRundown, rundownCache.get()), set the system clock to before midnight, mark the playback as pending roll by setting metadata.playback.secondaryTimer to Playback.Roll (and secondaryTimerUntil appropriately) or invoking the helper that schedules a secondary roll, then advance the clock past midnight, call update(), and assert that the rollover logic ran (currentDay incremented to 1 and the roll result reflects the event start). Use the existing helpers roll, update, getState, Playback.Roll, and rundownCache to locate where to set the pending-roll state and to verify the outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/stores/runtimeState.ts`:
- Around line 693-695: The calculation for runtimeState._startDayOffset uses
runtimeState.clock but should use the roll-mode-adjusted time (offsetClock) so
day bucket selection matches roll-mode event start logic; update the call to
findDayOffset(runtimeState.eventNow.timeStart, ...) to pass
runtimeState.offsetClock (or the variable named offsetClock) instead of
runtimeState.clock, and make the same change in the other occurrence that sets
runtimeState._startDayOffset / runtimeState.rundown.currentDay (the similar
block around the later assignment).
- Around line 551-553: The early return inside the block that checks
runtimeState.timer.playback === Playback.Roll and
runtimeState.timer.secondaryTimer !== null (which calls updateIfWaitingToRoll)
prevents advancing rundown.currentDay when midnight is crossed; move or
duplicate the logic that increments currentDay so it runs before returning or
ensure updateIfWaitingToRoll itself advances currentDay when hasCrossedMidnight
is true. Concretely: in the function containing the snippet, update the code
paths at the Playback.Roll early-return (the call to updateIfWaitingToRoll) and
the similar blocks around lines noted (559-562, 601-620) so that
hasCrossedMidnight triggers an increment of rundown.currentDay (or delegates to
updateIfWaitingToRoll to do so) before any early return, and keep the
updateId/state-change behavior consistent with existing increments.
- Line 548: The current midnight detection uses hasCrossedMidnight =
previousClock > now which treats any backward clock move as a day boundary;
change it to detect an actual calendar rollover by comparing dates and ensuring
time progressed forward: compute hasCrossedMidnight as
(previousClock.toDateString() !== now.toDateString()) && (now > previousClock).
Update the logic around previousClock/now/currentDay (referenced symbols:
hasCrossedMidnight, previousClock, now, currentDay) so you only bump currentDay
when the calendar date changed and now is strictly after previousClock, avoiding
DST or manual/NTP backward adjustments being treated as midnight.
---
Nitpick comments:
In `@apps/server/src/services/rollUtils.ts`:
- Around line 69-85: The nested scan over metadata.playableEventOrder inside
loadRoll() makes overnight lookup O(n²); before the main loop, build a
precomputed map/array of overnight candidate indices (e.g.,
nextOvernightIndexByPlayableIndex or overnightCandidates list) by scanning
metadata.playableEventOrder once to record indices j where
rundown.entries[j].timeStart > timeEnd, then in the inner logic replace the
for-loop that checks futureEventId/futureEvent/crossesMidnight with a
constant-time lookup into that precomputed structure and call checkIsNow(...)
and getTimedIndexFromPlayableIndex(metadata, j) only for those candidates so the
overall routine stays near-linear.
In `@apps/server/src/stores/__tests__/runtimeState.test.ts`:
- Around line 301-397: Add a test that simulates the "pending-roll on
secondaryTimer" path: init the same overnight mockRundown (use makeRundown,
initRundown, rundownCache.get()), set the system clock to before midnight, mark
the playback as pending roll by setting metadata.playback.secondaryTimer to
Playback.Roll (and secondaryTimerUntil appropriately) or invoking the helper
that schedules a secondary roll, then advance the clock past midnight, call
update(), and assert that the rollover logic ran (currentDay incremented to 1
and the roll result reflects the event start). Use the existing helpers roll,
update, getState, Playback.Roll, and rundownCache to locate where to set the
pending-roll state and to verify the outcome.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/utils/src/date-utils/checkIsNow.test.tsis excluded by none and included by nonepackages/utils/src/date-utils/checkIsNow.tsis excluded by none and included by none
📒 Files selected for processing (6)
apps/server/src/services/__tests__/rollUtils.test.tsapps/server/src/services/__tests__/timerUtils.test.tsapps/server/src/services/rollUtils.tsapps/server/src/services/timerUtils.tsapps/server/src/stores/__tests__/runtimeState.test.tsapps/server/src/stores/runtimeState.ts
alex-Arc
approved these changes
Mar 2, 2026
cpvalente
force-pushed
the
fix-roll-midnight
branch
3 times, most recently
from
7066ed8 to
87e98fe
Compare
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/client/src/features/operator/operator-event/OperatorEvent.tsx (1)
2-2: Use a type-only import forDay(Line 2).
Dayis only used as a type here, and the file already usesimport typefor other type imports. Preferimport typeto be explicit about type-only imports and maintain consistency with the existing pattern in this file.Proposed change
-import { Day } from 'ontime-types'; +import type { Day } from 'ontime-types';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/features/operator/operator-event/OperatorEvent.tsx` at line 2, The import of Day should be a type-only import: replace the current value import of Day with an explicit type-only import (import type { Day } from 'ontime-types') so it matches the file's other type imports and avoids pulling runtime code; update the import statement that references Day in OperatorEvent (symbol: Day) accordingly.apps/server/src/lib/time-core/__tests__/timeCore.test.ts (1)
214-263: Add DST boundary coverage fordaysSinceStart().Current cases don’t protect against spring-forward/fall-back day-length changes; add at least one case that crosses midnight on a DST transition day.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/lib/time-core/__tests__/timeCore.test.ts` around lines 214 - 263, Add a test to cover a DST boundary for daysSinceStart by simulating system times that cross a DST transition (spring-forward or fall-back) using vi.setSystemTime and timeCore.now; call timeCore.daysSinceStart(startEpoch, currentEpoch) and assert the expected day count. Specifically, create a test that sets startEpoch just before the local DST clock change and currentEpoch just after it (use appropriate UTC timestamps that map to the local DST transition), and verify daysSinceStart returns 1 (or the correct number of full days) to ensure the function handles variable day lengths across DST shifts.apps/server/src/stores/runtimeState.ts (1)
550-556: Consider usingtimeCore.daysSinceStartto avoid duplication.This calculation duplicates the logic already encapsulated in
timeCore.daysSinceStart()(used at lines 709 and 813). Using the utility function improves maintainability and ensures consistent behavior.♻️ Suggested refactor
// calculate currentDay from epoch (days elapsed since playback was started) if (runtimeState._startEpoch !== null && runtimeState._startDayOffset !== null) { - const startClock = timeCore.toTimeOfDay(runtimeState._startEpoch); - const elapsedMs = epoch - runtimeState._startEpoch; - const daysSinceStart = Math.floor((elapsedMs + startClock) / dayInMs); - runtimeState.rundown.currentDay = runtimeState._startDayOffset + daysSinceStart; + runtimeState.rundown.currentDay = runtimeState._startDayOffset + timeCore.daysSinceStart(runtimeState._startEpoch, epoch); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/stores/runtimeState.ts` around lines 550 - 556, Replace the inline days-since-start calculation with the existing utility: call timeCore.daysSinceStart(runtimeState._startEpoch, runtimeState._startDayOffset, epoch) (or the exact signature used elsewhere) and assign its result to runtimeState.rundown.currentDay; remove the manual computation that uses timeCore.toTimeOfDay, elapsedMs, daysSinceStart and dayInMs, and ensure you only run this when runtimeState._startEpoch and _startDayOffset are non-null, matching the original guard and the usages at lines where timeCore.daysSinceStart is already used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/__tests__/rollUtils.test.ts`:
- Around line 725-731: The test fixture for event id '2' in rollUtils.test.ts
has inconsistent duration: timeStart uses 11 * MILLIS_PER_HOUR and timeEnd uses
2 * MILLIS_PER_HOUR (overnight span), but duration is set to 14 *
MILLIS_PER_HOUR; update the duration on the mockEvent for key '2' to 15 *
MILLIS_PER_HOUR so it matches the 11:00 -> 02:00 span (use the same
MILLIS_PER_HOUR constant and modify the duration property on the mockEvent
object).
---
Nitpick comments:
In `@apps/client/src/features/operator/operator-event/OperatorEvent.tsx`:
- Line 2: The import of Day should be a type-only import: replace the current
value import of Day with an explicit type-only import (import type { Day } from
'ontime-types') so it matches the file's other type imports and avoids pulling
runtime code; update the import statement that references Day in OperatorEvent
(symbol: Day) accordingly.
In `@apps/server/src/lib/time-core/__tests__/timeCore.test.ts`:
- Around line 214-263: Add a test to cover a DST boundary for daysSinceStart by
simulating system times that cross a DST transition (spring-forward or
fall-back) using vi.setSystemTime and timeCore.now; call
timeCore.daysSinceStart(startEpoch, currentEpoch) and assert the expected day
count. Specifically, create a test that sets startEpoch just before the local
DST clock change and currentEpoch just after it (use appropriate UTC timestamps
that map to the local DST transition), and verify daysSinceStart returns 1 (or
the correct number of full days) to ensure the function handles variable day
lengths across DST shifts.
In `@apps/server/src/stores/runtimeState.ts`:
- Around line 550-556: Replace the inline days-since-start calculation with the
existing utility: call timeCore.daysSinceStart(runtimeState._startEpoch,
runtimeState._startDayOffset, epoch) (or the exact signature used elsewhere) and
assign its result to runtimeState.rundown.currentDay; remove the manual
computation that uses timeCore.toTimeOfDay, elapsedMs, daysSinceStart and
dayInMs, and ensure you only run this when runtimeState._startEpoch and
_startDayOffset are non-null, matching the original guard and the usages at
lines where timeCore.daysSinceStart is already used.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/types/src/definitions/core/OntimeEntry.tsis excluded by none and included by nonepackages/utils/src/date-utils/checkIsNow.test.tsis excluded by none and included by nonepackages/utils/src/date-utils/checkIsNow.tsis excluded by none and included by nonepackages/utils/src/rundown-utils/entryDefinitions.tsis excluded by none and included by none
📒 Files selected for processing (18)
apps/client/src/common/hooks/useSocket.tsapps/client/src/features/operator/operator-event/OperatorEvent.tsxapps/client/src/features/rundown/rundown-event/RundownEvent.tsxapps/client/src/features/rundown/rundown-event/RundownEventInner.tsxapps/client/src/features/rundown/rundown-event/composite/RundownEventChip.tsxapps/client/src/views/editor/title-list/TitleListItem.tsxapps/client/src/views/timeline/TimelineEntry.tsxapps/server/src/api-data/db/migration/db.migration.v3.tsapps/server/src/api-data/rundown/rundown.parser.tsapps/server/src/lib/time-core/__tests__/timeCore.test.tsapps/server/src/lib/time-core/timeCore.tsapps/server/src/models/demoProject.tsapps/server/src/services/__tests__/rollUtils.test.tsapps/server/src/services/__tests__/timerUtils.test.tsapps/server/src/services/rollUtils.tsapps/server/src/services/timerUtils.tsapps/server/src/stores/__tests__/runtimeState.test.tsapps/server/src/stores/runtimeState.ts
Comment on lines
+74
to
+77
| export function daysSinceStart(startEpoch: Instant, currentEpoch: Instant): Day { | ||
| const startClock = toTimeOfDay(startEpoch); | ||
| const elapsedMs = currentEpoch - startEpoch; | ||
| return Math.floor((elapsedMs + startClock) / dayInMs) as Day; |
Contributor
There was a problem hiding this comment.
daysSinceStart() can miscount day transitions across DST.
This formula assumes every day is exactly dayInMs. That breaks around DST boundaries (e.g., Europe/Copenhagen from March 30, 2025 00:10 local to March 31, 2025 00:05 local crosses midnight once but is <24h in epoch time, so it returns 0).
💡 Suggested fix
export function daysSinceStart(startEpoch: Instant, currentEpoch: Instant): Day {
- const startClock = toTimeOfDay(startEpoch);
- const elapsedMs = currentEpoch - startEpoch;
- return Math.floor((elapsedMs + startClock) / dayInMs) as Day;
+ const start = new Date(startEpoch);
+ const current = new Date(currentEpoch);
+
+ const startDayKey = Date.UTC(start.getFullYear(), start.getMonth(), start.getDate());
+ const currentDayKey = Date.UTC(current.getFullYear(), current.getMonth(), current.getDate());
+
+ return ((currentDayKey - startDayKey) / dayInMs) as Day;
}📝 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
| export function daysSinceStart(startEpoch: Instant, currentEpoch: Instant): Day { | |
| const startClock = toTimeOfDay(startEpoch); | |
| const elapsedMs = currentEpoch - startEpoch; | |
| return Math.floor((elapsedMs + startClock) / dayInMs) as Day; | |
| export function daysSinceStart(startEpoch: Instant, currentEpoch: Instant): Day { | |
| const start = new Date(startEpoch); | |
| const current = new Date(currentEpoch); | |
| const startDayKey = Date.UTC(start.getUTCFullYear(), start.getUTCMonth(), start.getUTCDate()); | |
| const currentDayKey = Date.UTC(current.getUTCFullYear(), current.getUTCMonth(), current.getUTCDate()); | |
| return ((currentDayKey - startDayKey) / dayInMs) as Day; | |
| } |
cpvalente
force-pushed
the
fix-roll-midnight
branch
from
87e98fe to
00f12f6
Compare
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.
No description provided.