-
-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dayoffset in rundown #1396
base: collection
Are you sure you want to change the base?
Dayoffset in rundown #1396
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 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 (
|
1d3d448
to
ba7b214
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Alex, I agree that this changes will lead to a better experience dealing with this dataset
I think we are moving in a direction where the server is providing us more metadata. I suggest that we spend some time to cleanup typing and processes around this
apps/client/src/features/rundown/event-block/EventBlock.utils.ts
Outdated
Show resolved
Hide resolved
apps/client/src/features/rundown/event-block/EventBlock.utils.ts
Outdated
Show resolved
Hide resolved
@@ -39,6 +39,7 @@ export type OntimeEvent = OntimeBaseEvent & { | |||
colour: string; | |||
revision: number; | |||
delay?: number; // calculated at runtime | |||
dayOffset?: number; // calculated at runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I wonder if we are starting to complicate our code base a bit with these types
would it be better to have a OntimePersistedEvent
and an OntimeEvent
, where the OntimeEvent
contains all the stuff calculated in rundownCache
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds like a good idea
I've been struggling a bit with the optional so making a new type would be great
Should we make a new type for pre or post cache.generate ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I understood. but I think the two types we need are
- one type with the base data we need, this is stored in the json file
- one type with added metadata, this will be in all the runtime stores and generated in the rundownCache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if there is something definitely better. I think your suggestion is good and makes sense
const timeFromPrevious: number = getTimeFromPrevious( | ||
currentEntry.timeStart, | ||
currentEntry.dayOffset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup: we need to make sure that these properties do not get persisted. I have flagged the entrypoint for the code in this comment
https://github.com/cpvalente/ontime/blob/master/apps/server/src/services/rundown-service/rundownCache.ts#L97
Co-authored-by: Carlos Valente <[email protected]>
eeb6603
to
0b4095b
Compare
No description provided.