More rundown metadata #1429
More rundown metadata #1429
Conversation
|
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 WalkthroughThis pull request introduces two new properties, Changes
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alex-Arc
force-pushed
the
more-rundown-metadata
branch
from
470e55c to
d4397c3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/utils/src/date-utils/isNewLatest.test.ts (1)
4-4: TODO note
A note indicates this test might be tricked by any function returning true. Consider adding negative or boundary test cases to ensure correctness.apps/server/src/services/rundown-service/rundownCache.ts (2)
60-60: Use docstring or comments
You've introduced a globaltotalDaysvariable to track day increments. Consider adding a brief docstring explaining its usage to ensure clarity for future maintainers.
132-138: Day offset logic
This block incrementstotalDaysif an event crosses midnight or ifcheckIsNextDayevaluates to true. It appears correct, but please address the//TODOabout whether to count in days or milliseconds for better clarity.apps/client/src/common/utils/eventsManager.ts (1)
28-29: Consider referencing relevant design docs or usage patterns fordayOffsetandgap.It's good that you've added clear defaults (
0) for the new properties. If there's additional business rationale for these fields, add inline code comments or references to design docs to ease future maintenance and highlight their purpose across the codebase.apps/client/src/features/rundown/event-block/__tests__/EventBlock.utils.test.ts (2)
16-16: Clarify negative gap usage vs. overlap.You’re calling
formatGap(-30 * MILLIS_PER_SECOND, false), then validating'Overlap 30s'. A negative gap suggests an overlap, but consider updating naming or comments to avoid confusion—especially since the function is namedformatGap.
23-25: Consider adding more tests for multi-day scenarios.Verifying
'Gap 30s (next day)'is great. Extend coverage to handle edge cases: multiple consecutive next-day events, very large multi-day offsets, etc.apps/client/src/features/rundown/time-input-flow/TimeInputFlow.tsx (1)
49-49: Revisit the “Over midnight” warning threshold.Currently, “Over midnight” is flagged when
timeStart + duration > dayInMs. Consider user scenarios where crossing midnight by a second is acceptable vs. needing a distinct warning. You might add an adjustable threshold or an alternative label to clarify whether it’s a strict limit or a cautionary notice.apps/server/src/services/sheet-service/__tests__/sheetUtils.test.ts (1)
187-188: MoregapanddayOffsetproperties
Extends the coverage for events that might skip days or times. Keep an eye on how correlation with other fields might impact these values.apps/server/src/services/rundown-service/__tests__/delayUtils.test.ts (2)
190-191: Applying multi-hour gaps alongsidedayOffset
This test clarifies that time-based calculations account for both day offsets and hour-level gaps. Looks solid.
Line range hint
223-224: Zero-gap event in a multi-day scenario
This scenario is helpful to differentiate truly consecutive events from day transitions. Keep the tests flexible for real-world scheduling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/types/src/definitions/core/OntimeEvent.type.tsis excluded by none and included by none
📒 Files selected for processing (21)
apps/client/src/common/utils/eventsManager.ts(1 hunks)apps/client/src/features/rundown/Rundown.tsx(5 hunks)apps/client/src/features/rundown/RundownEntry.tsx(3 hunks)apps/client/src/features/rundown/event-block/EventBlock.tsx(3 hunks)apps/client/src/features/rundown/event-block/EventBlock.utils.ts(2 hunks)apps/client/src/features/rundown/event-block/RundownIndicators.tsx(1 hunks)apps/client/src/features/rundown/event-block/__tests__/EventBlock.utils.test.ts(2 hunks)apps/client/src/features/rundown/time-input-flow/TimeInputFlow.tsx(2 hunks)apps/client/src/views/timeline/timeline.utils.ts(2 hunks)apps/server/src/models/demoProject.ts(14 hunks)apps/server/src/models/eventsDefinition.ts(1 hunks)apps/server/src/services/rundown-service/__tests__/delayUtils.test.ts(3 hunks)apps/server/src/services/rundown-service/__tests__/rundownCache.test.ts(12 hunks)apps/server/src/services/rundown-service/delayUtils.ts(2 hunks)apps/server/src/services/rundown-service/rundownCache.ts(4 hunks)apps/server/src/services/sheet-service/__tests__/sheetUtils.test.ts(6 hunks)apps/server/src/utils/parser.ts(1 hunks)packages/utils/src/date-utils/getTimeFromPrevious.test.ts(1 hunks)packages/utils/src/date-utils/getTimeFromPrevious.ts(1 hunks)packages/utils/src/date-utils/isNewLatest.test.ts(1 hunks)packages/utils/src/date-utils/isNewLatest.ts(1 hunks)
🔇 Additional comments (65)
packages/utils/src/date-utils/isNewLatest.ts (4)
1-3: Imports look consistent
No issues found with these imports. They reflect a standard usage of type imports and utility constants.
13-13: Sensible default return
Returningtruewhen nopreviousEventexists is straightforward and self-explanatory. No issues found.
17-20: Clear day offset handling
UsingdayOffset * dayInMsintegrates well with the concept of multi-day events. The comparison logic is straightforward, and the use of>=to determine the “latest” event is easy to follow.
8-11: Improved function signature
The shift to accepting object parameters rather than multiple numeric arguments makes the code more expressive and easier to maintain. This aligns well with best practices for handling related data as a single entity. Ensure all references to this function in the codebase have been updated accordingly.✅ Verification successful
Function signature change is properly implemented across the codebase
Based on the search results, all current usages of
isNewLatest()in the codebase are already using the new object parameter signature. The function is called in three different locations:
- apps/server/src/services/rundown-service/rundownCache.ts
- apps/client/src/views/timeline/timeline.utils.ts
- apps/client/src/features/rundown/Rundown.tsx
And all test cases in
isNewLatest.test.tsalso use the new object parameter format. No instances of the old signature (with multiple numeric arguments) were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching the entire repo for calls to isNewLatest that may use the old signature ast-grep --pattern $'isNewLatest($_, $_, $_, $_)'Length of output: 50
Script:
#!/bin/bash # Let's search for any usage of isNewLatest function rg "isNewLatest\(" -A 2Length of output: 3140
Script:
#!/bin/bash # Let's check the git history of this file to see the previous implementation git log -p packages/utils/src/date-utils/isNewLatest.ts | head -n 50Length of output: 0
packages/utils/src/date-utils/getTimeFromPrevious.ts (7)
1-2: Imports look good
The new type import aligns with the refactored function signature.
10-11: Enhanced readability with object-based parameters
Transitioning from multiple numeric parameters to an object-based approach promotes clearer usage and is more robust. Great improvement.
14-14: Proper handling of no-previous scenario
Returning 0 makes logical sense when there is no previous event. Straightforward and unambiguous.
18-19: Consistent calculation of adjusted times
The approach to handle multi-day events (via day offset) avoids confusion inherent in separate day-check logic. It’s both readable and flexible.
22-22: Equality check for consecutive events
Comparing start/end times for exact matches is good for identifying back-to-back events.
27-29: Logical gap calculation
When the current start is after the previous end, returning the positive difference is appropriate.
33-33: Negative overlap scenario
Falling back to the difference captures overlap in a single statement. This keeps the logic simple.packages/utils/src/date-utils/isNewLatest.test.ts (1)
8-8: Comprehensive test coverage
Each test scenario methodically verifies the function behavior under different time, duration, and dayOffset settings. These tests are well structured and illustrate multi-day logic thoroughly.Also applies to: 12-17, 21-26, 30-35, 39-44, 48-53
packages/utils/src/date-utils/getTimeFromPrevious.test.ts (2)
1-1: New constant import
ImportingMILLIS_PER_MINUTEis consistent with the test’s usage.
7-12: Improved object-based test inputs
Switching to object parameters in the tests aligns with the updated function signature. The coverage for gap calculation, overlap, next-day logic, and partial overlaps is well demonstrated.Also applies to: 16-19, 23-26, 30-36, 41-46, 51-56
apps/client/src/views/timeline/timeline.utils.ts (2)
145-145: Clarity in getTimeFromPrevious usage
Passing the currentEntry and lastEntry as object parameters demonstrates the object-based refactor. This reduces confusion about argument order and fosters maintainability.
154-154: Robust check with isNewLatest
Using the new structured parameters keeps the code consistent across the application. The straightforward if-statement ensures we only updatelastEntrywhen the current event is indeed more recent.apps/server/src/services/rundown-service/rundownCache.ts (8)
23-24: Imports for day-based calculations
The newcheckIsNextDayanddayInMsimports expand the logic for multi-day event handling. Good addition.
113-113: Reset total days upon generate
Re-initializingtotalDaysat the start ofgenerate()is consistent with the other counters. Looks good.
124-124: Initialize gap
SettingcurrentEntry.gapto 0 ensures a default value, preventing undefined behavior in subsequent calculations.
146-146: Gap assignment
UsinggetTimeFromPreviousto assigncurrentEntry.gapkeeps event timing calculations consistent and streamlined.
148-154: Gap-based duration logic
Treating zero, positive, and negative gaps distinctly is a clear way to handle concurrency, overlaps, or breaks. Consider adding edge-case tests (e.g., extremely large negative overlaps).
158-158: Safe overlap handling
UsingMath.max(currentEntry.duration + currentEntry.gap, 0)ensures total duration never goes below zero.
163-164: Delay reduction logic
Subtracting a positive gap from the accumulated delay makes sense, preventing the carry-over of unused delays.
170-170: Identify latest event
UsingisNewLatestto setlastEntryis a clean approach for tracking events ending last, ensuring accurate next-calculation logic.apps/server/src/services/rundown-service/__tests__/rundownCache.test.ts (12)
580-581: New event properties
IntroducingdayOffsetandgapensures thorough multi-day testing for events.
610-611: Added dayOffset and gap
This aligns tested events with new rundown logic for multi-day spans.
640-641: dayOffset and gap
Their inclusion in test objects affirms coverage of these properties in runtime evaluations.
670-671: Initialize dayOffset/gap
Ensures each test event has a defined baseline for multi-day or overlapping calculations.
712-713: Default zero dayOffset/gap
Having both properties set to 0 by default avoids uninitialized values in test scenarios.
738-739: Insertion of dayOffset/gap
Reinforces test coverage for new scheduling properties introduced in the rundown logic.
768-769: Test event properties
AddingdayOffsetandgapkeeps the test event structure consistent with the updated model.
798-799: Consistent new fields
Ensures that multi-day logic (dayOffset, gap) is exercised during testing.
854-855: Further dayOffset/gap usage
These properties are crucial for verifying correct day transitions and spacing.
884-885: Partial dayOffset/gap setups
Maintaining consistency in test data aligns with the real event model across files.
914-915: Ensure uniform usage
RetainingdayOffsetandgaphere solidifies your multi-day event coverage.
944-945: Comprehensive coverage
SpecifyingdayOffsetandgapfor this block further ensures the logic is fully tested under different day scenarios.apps/client/src/features/rundown/event-block/EventBlock.utils.ts (1)
13-17: New gap formatting function
Swapping outformatOverlapforformatGapsimplifies the logic for presenting day-spanning or overlapping intervals, especially with theisNextDayflag.apps/client/src/features/rundown/event-block/RundownIndicators.tsx (3)
1-1: Updated import
ReplacesformatOverlapwithformatGapto support new event gap logic.
7-9: Extended interface
AddingisNextDayandgapaligns with multi-day awareness and gap-based calculations, removing reliance on previous event timing.
13-15: Using formatGap
DestructuringgapandisNextDayclarifies how gaps and next-day transitions are displayed in these indicators.apps/server/src/models/eventsDefinition.ts (1)
28-29: New event fields
AddingdayOffsetandgapreflects the expanded multi-day coverage in your event model, keeping it in sync with runtime logic.apps/client/src/features/rundown/event-block/__tests__/EventBlock.utils.test.ts (2)
1-3: Adapt existing test coverage to encompass edge cases for both negative and positive gap values.Importing
MILLIS_PER_SECONDis fine, but also consider testing boundary conditions (e.g., extremely large gap values or zero gap) to ensure that your logic gracefully handles these extremes.
19-21: Confirm consistent naming: 'Recognizes a gap' vs. 'Overlaps'.This test checks for a gap instead of an overlap, but make sure the test descriptions and logic match the function’s behavior. If your domain terms are “gap” and “overlap,” keep them consistent throughout.
apps/server/src/services/rundown-service/delayUtils.ts (1)
142-143: Re-evaluate applying gap deduction before checking if delayValue is zero.The logic
delayValue = Math.max(delayValue - currentEntry.gap, 0)is useful, but ifdelayValuebecomes zero prematurely, subsequent events may not get the intended partial delay. Verify if this is the correct design or if you need a more granular approach.apps/client/src/features/rundown/time-input-flow/TimeInputFlow.tsx (1)
9-9: Centralize constants for daily thresholds.Importing
dayInMshere is good. Keep any related constants (e.g., hourInMs, etc.) in the same place to maintain consistency and discoverability in your time-related calculations.apps/client/src/features/rundown/RundownEntry.tsx (3)
35-35: AddedisNextDayprop inRundownEntryProps
This new boolean prop effectively captures whether the event spills over into the following day. Looks good for clarifying day boundaries. Ensure that all downstream components handle day transitions as intended.
54-54: DestructuredisNextDayfrom props
Ensure that any business logic reliant on the next-day condition properly references this extracted boolean. If other files or components also need to check day transitions, consider offering a shared utility.
174-175: PassinggapandisNextDaytoEventBlock
Providing bothgapandisNextDayhere promotes consistency with the updated event model. Verify that related components and logic support the new properties without side effects.apps/server/src/services/sheet-service/__tests__/sheetUtils.test.ts (5)
40-41: IntroducedgapanddayOffsetin the test event
These newly added properties are tested effectively. Confirm that coverage is comprehensive for all edge cases involving day offsets and gaps.
89-90: AddedgapanddayOffset
Ensures the test scenario now reflects multi-day or spaced events. Good for consistency.
139-140: TestinggapanddayOffsetfor boolean coverage
Properly aligns with the new event schema. Make sure further negative or large gap scenarios are tested to ensure robust coverage.
223-224: Extending the coverage for offset events
Confirm that date/time calculations remain correct, especially around boundary transitions or large offsets.
259-260: Deployment ofgapanddayOffsetin final scenario
Allows the test to confirm successful usage in the typical sheet setup as well. Looks consistent with the rest of the tests.apps/client/src/features/rundown/event-block/EventBlock.tsx (3)
50-51: AddedgapandisNextDaytoEventBlockProps
This clarifies event spacing and future-day transitions. Perfect synergy with the newly updated schema.
88-89: DestructuringgapandisNextDay
Reflects the revised props. If multiple components handle similar logic, consider abstracting it for consistency.
276-276: Passing new props toRundownIndicators
Enhances event visuals by factoring ingapand next-day notices. Ensure UI changes remain consistent in different user locales and timezones.apps/server/src/services/rundown-service/__tests__/delayUtils.test.ts (3)
153-157: Addinggapto event definitions
Including the extra gap logic is crucial for correct timing adjustments. Good to see negative or zero-gap cases tested.
168-168: Retaininggapin the updated rundown
Ensures that upon applying delay, the event’s gap remains recognized. Confirm no unintended side effects where gap + linkStart might conflict.
181-182: NewgapanddayOffsetfields
These fields support multi-day events. Validate that negative offsets or large transitions do not break existing delay logic.apps/server/src/models/demoProject.ts (1)
24-25: Consistent addition of dayOffset & gap properties.All these newly introduced properties (
dayOffsetandgap) are uniform across the different event objects. Since both are currently set to zero, ensure there is clarity around whether these values will be updated during runtime and if they need to be persisted or recalculated. Otherwise, there is no functional issue with adding them upfront.Also applies to: 52-53, 80-81, 108-109, 136-137, 169-170, 197-198, 225-226, 253-254, 281-282, 314-315, 342-343, 370-371, 398-399
apps/client/src/features/rundown/Rundown.tsx (4)
16-16: Good use of dayInMs for day-boundary logic.Importing
dayInMsfromontime-utilsclarifies the day boundary checks. This is concise and avoids hardcoding magic numbers.
271-271: Explicit initialization of isNextDay.Setting
isNextDay = falseat the start of the iteration ensures a clean default. Confirm that all usage paths properly update the variable when day changes are detected.
Line range hint
290-307: Refined day-over-day logic.Lines 299-305 check if the new entry is on the next day by comparing
(lastEvent.timeStart + lastEvent.duration) <= dayInMsandentry.dayOffset > lastEvent.dayOffset. Carefully verify that this approach is always accurate for multi-day events or entries shifting across midnight boundaries. If events can span beyond a single day, consider comparing actual day boundaries in the code or adjusting for time zones.
339-339: Passing isNextDay prop to child.Introducing the
isNextDayindicator in the<RundownEntry>increases clarity when rendering day transitions. This looks good.apps/server/src/utils/parser.ts (1)
387-389: Set dayOffset, gap, and delay to 0 by default.Hardcoding these values to zero makes sense if the rundown cache or a subsequent process recalculates them. If these fields must always be regenerated externally, confirm that no code path relies on a non-zero default here.
alex-Arc
force-pushed
the
more-rundown-metadata
branch
from
d4397c3 to
6eb318f
Compare
apps/client/src/features/rundown/event-block/EventBlock.utils.ts
Outdated
Show resolved
Hide resolved
apps/client/src/features/rundown/event-block/__tests__/EventBlock.utils.test.ts
Outdated
Show resolved
Hide resolved
apps/client/src/features/rundown/event-block/__tests__/EventBlock.utils.test.ts
Outdated
Show resolved
Hide resolved
apps/client/src/features/rundown/event-block/__tests__/EventBlock.utils.test.ts
Outdated
Show resolved
Hide resolved
alex-Arc
force-pushed
the
more-rundown-metadata
branch
from
5fc7876 to
7d013d5
Compare
| @@ -1,45 +1,34 @@ | |||
| import { checkIsNextDay } from './checkIsNextDay.js'; | |||
| import type { OntimeEvent } from 'ontime-types'; | |||
|
|
|||
| import { dayInMs } from './conversionUtils.js'; | |||
|
|
|||
| /** | |||
| * Utility returns the time elapsed (gap or overlap) from the previous | |||
| * It uses deconstructed parameters to simplify implementation in UI | |||
| */ | |||
| export function getTimeFromPrevious( | |||
There was a problem hiding this comment.
question: do we still need this function?
There was a problem hiding this comment.
used in rundownCache and timeline.utils.ts
might be able to remove it from timeline at some point...
No description provided.