Instant for internal calculations #1861

alex-Arc · 2025-11-03T17:01:28Z

creates the branded type Instant
add instant as new property to runtimeState and drive clock from that

coderabbitai · 2025-11-03T17:01:46Z

Walkthrough

Replaces primitive time utilities with epoch/clock abstractions: adds temporal helpers (getEpoch, epochToClock, clockToEpoch, etc.), refactors RuntimeState to use DayMs/EpochMs (adding epoch field), updates logger and tests to use new types, removes legacy time functions, and adds DST-aware Vitest global setup.

Changes

Cohort / File(s)	Summary
Temporal utilities (new) `apps/server/src/utils/temporal.ts`, `apps/server/src/utils/__tests__/temporal.test.ts`	New timezone-aware helpers: `getEpoch`, `epochToClock`, `clockToEpoch`, `timeUntilInstant`, `timeSinceInstant`, `addDurationToInstant` with overloads and null handling; tests for round-trip conversions and DST boundaries.
Runtime state refactor `apps/server/src/stores/runtimeState.ts`	Replaces numeric clock with `DayMs`, adds `epoch: EpochMs`, changes `_startEpoch` to `Maybe<EpochMs>`, and updates initialization, start/pause/stop/update/roll logic to use epoch-based calculations.
Runtime mocks `apps/server/src/stores/__mocks__/runtimeState.mocks.ts`	Mock state updated to include `clock: 0 as DayMs` and `epoch: 0 as EpochMs`; imports extended for `DayMs`/`EpochMs`.
Logger changes `apps/server/src/classes/Logger.ts`	Swaps `timeNow()` usage for `getEpoch()`/`epochToClock()` and `millisToString` to compute log time fields; imports updated.
Legacy time removal `apps/server/src/utils/time.ts`	Removed exported functions `timeNow()` and `getTimeObject()`; retained timezone label utilities.
Restore types & tests `apps/server/src/services/restore-service/restore.type.ts`, `apps/server/src/services/restore-service/__tests__/restore.parser.test.ts`, `apps/server/src/services/restore-service/__tests__/restore.service.test.ts`	`RestorePoint.startEpoch` changed from `MaybeNumber` to `Maybe<EpochMs>`; tests updated to import `EpochMs` and cast numeric `startEpoch` fixtures to `EpochMs`.
Timer & automation tests typing `apps/server/src/services/__tests__/timerUtils.test.ts`, `apps/server/src/api-data/automation/__tests__/automation.service.test.ts`	Tests updated to import `DayMs` and cast clock values/mutations to `DayMs`.
Vitest setup (DST-aware tests) `apps/server/vite.config.ts`, `apps/server/vitest.global-setup.ts`	Adds `globalSetup` to Vitest config and new global setup file forcing `process.env.TZ = 'Europe/Copenhagen'` for DST-consistent tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Refactor runtime service #1722 — Modifies RuntimeState structure; overlaps with epoch/DayMs field changes.
handle unchecked key access #1915 — Touches runtimeState logic and load/next handling; related to epoch-based adjustments.
Invert overtime #1715 — Adjusts timer offset and runtime timing logic; intersects with temporal refactor.

Suggested reviewers

cpvalente

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: introducing an instant-based internal timing system for calculations rather than direct clock manipulation.
Description check	✅ Passed	The description directly relates to the changeset, identifying the branded type introduction and instant property addition as the core changes.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch temporal

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d014d16 and 383d294.

⛔ Files ignored due to path filters (3)

packages/types/src/definitions/core/TimeUnits.type.ts is excluded by none and included by none
packages/types/src/index.ts is excluded by none and included by none
packages/types/src/utils/utils.type.ts is excluded by none and included by none

📒 Files selected for processing (13)

apps/server/src/api-data/automation/__tests__/automation.service.test.ts
apps/server/src/classes/Logger.ts
apps/server/src/services/__tests__/timerUtils.test.ts
apps/server/src/services/restore-service/__tests__/restore.parser.test.ts
apps/server/src/services/restore-service/__tests__/restore.service.test.ts
apps/server/src/services/restore-service/restore.type.ts
apps/server/src/stores/__mocks__/runtimeState.mocks.ts
apps/server/src/stores/runtimeState.ts
apps/server/src/utils/__tests__/temporal.test.ts
apps/server/src/utils/temporal.ts
apps/server/src/utils/time.ts
apps/server/vite.config.ts
apps/server/vitest.global-setup.ts

💤 Files with no reviewable changes (1)

apps/server/src/utils/time.ts

🚧 Files skipped from review as they are similar to previous changes (6)

apps/server/vite.config.ts
apps/server/vitest.global-setup.ts
apps/server/src/classes/Logger.ts
apps/server/src/services/restore-service/tests/restore.service.test.ts
apps/server/src/stores/mocks/runtimeState.mocks.ts
apps/server/src/services/restore-service/tests/restore.parser.test.ts

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2024-10-08T20:08:54.932Z

Learnt from: cpvalente
Repo: cpvalente/ontime PR: 1100
File: apps/server/src/stores/runtimeState.ts:195-200
Timestamp: 2024-10-08T20:08:54.932Z
Learning: In the `runtimeState.ts` file, ensure that `runtimeState.currentBlock.block` is not null before accessing its `id` property to prevent potential null reference errors.

Applied to files:

apps/server/src/stores/runtimeState.ts

🧬 Code graph analysis (2)

apps/server/src/stores/runtimeState.ts (2)

apps/server/src/utils/temporal.ts (2)

getEpoch (8-8)

epochToClock (34-38)

apps/server/src/services/timerUtils.ts (1)

getRuntimeOffset (116-152)

apps/server/src/utils/__tests__/temporal.test.ts (1)

apps/server/src/utils/temporal.ts (4)

getEpoch (8-8)

epochToClock (34-38)

clockToEpoch (18-24)

addDurationToInstant (48-52)

🔇 Additional comments (10)

apps/server/src/api-data/automation/__tests__/automation.service.test.ts (1)

1-1: LGTM! Type annotations align with new temporal model.

The additions of DayMs type casts throughout the test file correctly align mock clock values with the new epoch-based time typing without changing test behavior.

Also applies to: 105-105, 149-149, 157-157, 162-162, 170-170, 175-175, 220-220, 240-240, 262-262, 282-282, 302-302

apps/server/src/services/__tests__/timerUtils.test.ts (1)

2-2: LGTM! Clock mutations properly typed.

The DayMs type casts on clock mutations correctly maintain type safety with the new temporal model. All arithmetic operations preserve the day-relative millisecond semantics before casting.

Also applies to: 557-557, 577-577, 596-596, 615-615, 635-635, 655-655, 674-674, 693-693

apps/server/src/services/restore-service/restore.type.ts (1)

1-1: LGTM! Type refinement improves precision.

The update of startEpoch from MaybeNumber to Maybe<EpochMs> correctly narrows the type to represent epoch milliseconds, consistent with the broader temporal refactoring.

Also applies to: 10-10

apps/server/src/stores/runtimeState.ts (3)

16-18: LGTM! Epoch-based time model properly initialized.

The addition of EpochMs and DayMs types, along with startup-time initialization using getEpoch() and epochToClock(), correctly establishes the new temporal model for runtime state.

Also applies to: 46-47, 68-68, 72-77

409-410: LGTM! State references are now consistent.

The start() and pause() functions correctly use the state parameter for all epoch and clock updates (lines 409-410, 465-466), avoiding the inconsistency previously flagged in past reviews.

Also applies to: 431-431, 465-467

532-533: LGTM! Update loop maintains epoch/clock synchronization.

The update() function correctly synchronizes epoch and clock on every cycle and properly calculates the current day using the epoch delta, maintaining consistency across midnight boundaries.

Also applies to: 543-544

apps/server/src/utils/temporal.ts (4)

1-8: LGTM! Epoch source is correctly defined.

The getEpoch() function provides a clean abstraction for obtaining the current Unix epoch in milliseconds, properly typed as EpochMs.

31-38: LGTM! DST-aware epoch-to-clock conversion is correct.

The epochToClock() implementation correctly uses new Date(instant).getTimezoneOffset() (line 36) to compute the timezone offset for the specific instant, ensuring correct behavior across DST boundaries. This addresses the concern raised in previous reviews.

15-24: LGTM! Clock-to-epoch conversion maintains day context.

The clockToEpoch() function correctly derives the current day boundary from the current epoch and adds the provided clock value, producing a same-day epoch timestamp.

40-52: LGTM! Duration arithmetic is correctly implemented.

The helper functions timeUntilInstant(), timeSinceInstant(), and addDurationToInstant() provide clean abstractions for temporal arithmetic with proper type casting.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

apps/server/src/utils/temporal.ts

cpvalente · 2025-12-18T13:42:55Z

@coderabbitai review

coderabbitai · 2025-12-18T13:43:04Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

apps/server/src/stores/runtimeState.ts (1)

257-269: Runtime guard doesn't properly narrow type before non-nullable assertion.

RestorePoint's startEpoch is correctly typed as Maybe<EpochMs> in the type definition, so updating RestorePoint is unnecessary. However, the guard condition (startEpoch === null || typeof startEpoch === 'number') allows null to pass, then the assertion startEpoch as EpochMs unsafely casts the potentially-null value to non-nullable. Change the guard to typeof startEpoch === 'number' to truly narrow the type, or explicitly handle the null case in the assignment.

🧹 Nitpick comments (1)

apps/server/src/utils/__tests__/temporal.test.ts (1)

5-18: Consider removing or deprecating the reference timeNow() helper.

The comment states this is "here until it is no longer needed for reference." If the new epochToClock implementation is verified and stable, consider removing this helper in a follow-up to reduce maintenance burden.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ba5ebd and d014d16.

⛔ Files ignored due to path filters (3)

packages/types/src/definitions/core/TimeUnits.type.ts is excluded by none and included by none
packages/types/src/index.ts is excluded by none and included by none
packages/types/src/utils/utils.type.ts is excluded by none and included by none

📒 Files selected for processing (13)

apps/server/src/api-data/automation/__tests__/automation.service.test.ts (8 hunks)
apps/server/src/classes/Logger.ts (2 hunks)
apps/server/src/services/__tests__/timerUtils.test.ts (9 hunks)
apps/server/src/services/restore-service/__tests__/restore.parser.test.ts (3 hunks)
apps/server/src/services/restore-service/__tests__/restore.service.test.ts (5 hunks)
apps/server/src/services/restore-service/restore.type.ts (2 hunks)
apps/server/src/stores/__mocks__/runtimeState.mocks.ts (1 hunks)
apps/server/src/stores/runtimeState.ts (15 hunks)
apps/server/src/utils/__tests__/temporal.test.ts (1 hunks)
apps/server/src/utils/temporal.ts (1 hunks)
apps/server/src/utils/time.ts (0 hunks)
apps/server/vite.config.ts (1 hunks)
apps/server/vitest.global-setup.ts (1 hunks)

💤 Files with no reviewable changes (1)

apps/server/src/utils/time.ts

🧰 Additional context used

🧬 Code graph analysis (4)

apps/server/src/classes/Logger.ts (1)

apps/server/src/utils/temporal.ts (2)

epochToClock (34-38)

getEpoch (8-8)

apps/server/src/utils/__tests__/temporal.test.ts (1)

apps/server/src/utils/temporal.ts (3)

getEpoch (8-8)

epochToClock (34-38)

clockToEpoch (18-24)

apps/server/src/api-data/automation/__tests__/automation.service.test.ts (2)

apps/server/src/stores/__mocks__/runtimeState.mocks.ts (1)

makeRuntimeStateData (56-58)

apps/server/src/api-data/automation/automation.service.ts (1)

testConditions (71-124)

apps/server/src/stores/__mocks__/runtimeState.mocks.ts (1)

apps/server/src/stores/runtimeState.ts (1)

RuntimeState (45-70)

🔇 Additional comments (14)

apps/server/vitest.global-setup.ts (1)

1-3: LGTM!

The global setup correctly establishes a consistent timezone for DST-aware temporal testing. The timezone choice is well-documented.

apps/server/src/services/restore-service/restore.type.ts (1)

1-11: LGTM!

The type refinement from MaybeNumber to Maybe<EpochMs> strengthens type safety for epoch-based timestamps. The change is consistent with the broader temporal abstraction introduced in this PR.

apps/server/vite.config.ts (1)

1-8: LGTM!

The configuration correctly references the new global setup file, enabling consistent timezone handling across all tests.

apps/server/src/services/restore-service/__tests__/restore.service.test.ts (1)

1-128: LGTM!

The test fixtures have been correctly updated with explicit EpochMs casts to align with the refined RestorePoint type. The changes are consistent across all test cases and maintain existing test coverage.

apps/server/src/api-data/automation/__tests__/automation.service.test.ts (1)

1-316: LGTM!

Test fixtures have been consistently updated to cast clock values as DayMs, aligning with the temporal type system. The changes maintain test coverage while improving type safety.

apps/server/src/classes/Logger.ts (2)

7-7: LGTM!

The import update correctly references the new temporal utilities.

69-78: LGTM!

The timestamp generation has been successfully migrated to use the epoch-based temporal abstraction. The epochToClock(getEpoch()) pattern correctly derives the clock time with timezone awareness for logging purposes.

apps/server/src/services/__tests__/timerUtils.test.ts (1)

1-1296: LGTM!

The test file has been correctly updated to use DayMs typing for clock values and mutations. The explicit casts after arithmetic operations maintain type safety while preserving the test logic.

apps/server/src/services/restore-service/__tests__/restore.parser.test.ts (1)

1-67: LGTM!

The test has been correctly updated to use EpochMs typing in valid test fixtures. The invalid test cases appropriately continue using plain numbers to verify validation logic.

apps/server/src/stores/__mocks__/runtimeState.mocks.ts (1)

1-7: LGTM!

The mock state correctly incorporates the new epoch field and applies proper type casts for the branded DayMs and EpochMs types, aligning with the updated RuntimeState interface.

apps/server/src/stores/runtimeState.ts (2)

72-77: Module-level side effect during import.

getEpoch() and epochToClock() are called at module load time to initialize startupEpoch and startupClock. This captures the timestamp when the module is first imported, which may be significantly earlier than when the runtime actually starts. Verify this is the intended behavior.

532-535: LGTM on day offset calculation.

The calculation correctly uses the epoch difference between current time and start time to determine how many days have passed since the runtime started.

apps/server/src/utils/temporal.ts (2)

18-24: LGTM on clockToEpoch logic.

The implementation correctly derives the day boundary from the current epoch and adds the clock offset. The use of epochToClock(now) here is appropriate since we need the current day's boundary.

1-8: Clean type-safe API with proper overloads.

Good use of function overloads to maintain type narrowing for null vs non-null inputs. The branded types (EpochMs, DayMs) provide compile-time safety for time value semantics.

apps/server/src/stores/runtimeState.ts

apps/server/src/utils/__tests__/temporal.test.ts

apps/server/src/utils/temporal.ts

cpvalente · 2025-12-18T14:05:20Z

Hey @alex-Arc , the code looks fine to me, the rabbit made some good observations
But for maintenance sake, I dont think all the casting is positive. I think we need to create a set of abstractions for handling maths with our different types of times

lets chat!

alex-Arc · 2025-12-30T13:47:49Z

@coderabbitai review

coderabbitai · 2025-12-30T13:47:56Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

alex-Arc force-pushed the temporal branch 2 times, most recently from c47db98 to 99ae166 Compare November 4, 2025 11:29

alex-Arc marked this pull request as ready for review November 4, 2025 11:31

alex-Arc changed the title ~~Temporal~~ Instant for internal calculations Nov 4, 2025

alex-Arc requested a review from cpvalente November 8, 2025 06:24

cpvalente reviewed Nov 9, 2025

View reviewed changes

apps/server/src/utils/temporal.ts Outdated Show resolved Hide resolved

alex-Arc force-pushed the temporal branch from 27b7e9e to d014d16 Compare December 6, 2025 15:28

alex-Arc requested a review from cpvalente December 6, 2025 15:30

cpvalente added the priority Issue or PR to be prioritised label Dec 18, 2025

coderabbitai bot reviewed Dec 18, 2025

View reviewed changes

alex-Arc added 8 commits December 27, 2025 22:56

branded types for Instant and Duration

ea40d4d

implement in runtimeState

7231a83

remove all use of old timeNow()

bdb4bc3

test for DST

f872c21

rename to epoch

4ea100a

export type

077ee48

refactor: should refrence the provided state

2e45482

add: temporal util functions

048ac32

alex-Arc force-pushed the temporal branch from c93a7b1 to 048ac32 Compare December 27, 2025 23:40

chore: cleanup comments

383d294

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Instant for internal calculations #1861

Instant for internal calculations #1861

Uh oh!

alex-Arc commented Nov 3, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

cpvalente commented Dec 18, 2025

Uh oh!

coderabbitai bot commented Dec 18, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpvalente commented Dec 18, 2025 •

edited

Loading

Uh oh!

alex-Arc commented Dec 30, 2025

Uh oh!

coderabbitai bot commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Instant for internal calculations #1861

Are you sure you want to change the base?

Instant for internal calculations #1861

Uh oh!

Conversation

alex-Arc commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

Uh oh!

cpvalente commented Dec 18, 2025

Uh oh!

coderabbitai bot commented Dec 18, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpvalente commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-Arc commented Dec 30, 2025

Uh oh!

coderabbitai bot commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alex-Arc commented Nov 3, 2025 •

edited

Loading

coderabbitai bot commented Nov 3, 2025 •

edited

Loading

cpvalente commented Dec 18, 2025 •

edited

Loading