-
-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
refactor: fix roll next day #1472
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 (
|
@@ -497,7 +498,8 @@ export function update(): UpdateResult { | |||
|
|||
// 2. are we waiting to roll? | |||
if (runtimeState.timer.playback === Playback.Roll && runtimeState.timer.secondaryTimer !== null) { | |||
return updateIfWaitingToRoll(); | |||
const hasCrossedMidnight = previousClock > runtimeState.clock; |
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 seams right,
should we guard a bit more?
like it should be less by at least 23 hours
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 think you are right that we should have a safer code path.
I suggest that instead of removing the dayInMs we recalculate the time to start. That way there is no destructive path on us getting this wrong
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.
@alex-Arc I think the new logic covers this
if we think we crossed midnight, we will recalculate the target, which will again check the clock and potentially set the dayMs offset.
This could be a wasted operation (for example, if timer skipped back a little), but I believe it provides the safest outcome with the least amount of logic
No description provided.