-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
refactor: dev label #1143
refactor: dev label #1143
Conversation
WalkthroughThe updates significantly refine the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RuntimeService
participant TimerService
participant TimerState
User->>RuntimeService: start timer
RuntimeService->>TimerService: set and start timer
TimerService->>TimerState: update initial state
TimerService->>RuntimeService: onUpdateCallback invoked periodically
loop Every update interval
TimerService->>RuntimeService: checkTimerUpdate
RuntimeService->>TimerState: update timer state
end
User->>RuntimeService: stop timer
RuntimeService->>TimerService: stop timer
TimerService->>TimerState: finalize state
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
apps/server/package.json
is excluded by none and included by none
Files selected for processing (4)
- apps/server/src/app.ts (1 hunks)
- apps/server/src/classes/data-provider/DataProvider.ts (1 hunks)
- apps/server/src/services/app-state-service/AppStateService.ts (2 hunks)
- apps/server/src/utils/development.ts (1 hunks)
Additional context used
Biome
apps/server/src/services/app-state-service/AppStateService.ts
[error] 31-31: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
apps/server/src/classes/data-provider/DataProvider.ts
[error] 28-28: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
Additional comments not posted (2)
apps/server/src/utils/development.ts (1)
9-20
: Review of theshouldCrashDev
function
Correctness & Logic: The function correctly checks if it's in production and exits early if so, which is a good practice for development-only features. The condition to crash (
check
) is straightforward and the error handling (logging and exiting) is appropriate for a development environment.Performance: Since this function is only called under specific conditions (not in production and when
check
is true), its performance impact is minimal.Best Practices: The function is well-documented with a clear description of its purpose and behavior. However, the use of
process.exit(2)
is generally discouraged in Node.js applications because it does not allow the Node.js event loop to clear existing tasks before terminating. It's better to handle the exit more gracefully if possible.Error Handling: The function handles errors by logging them and exiting, which is suitable for a crash function in a development environment.
Potential Improvement: Consider adding a mechanism to handle graceful shutdowns where resources are cleaned up properly before exiting.
apps/server/src/app.ts (1)
266-266
: Clear Documentation of Exit CodesThe added comment clearly explains the meaning of exit code
2
in the context of a development crash. This is a good practice as it helps maintain clarity and understanding of the code's behavior, especially during debugging and maintenance phases.
} | ||
} | ||
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path'); |
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.
Potential Issue with Label Usage
The use of a label (DEV:
) here is unconventional and flagged by static analysis as potentially confusing. Labels are typically used with loops in JavaScript, not with function calls.
Suggestion: If the intent was to mark this section of code as development-specific, consider using a more conventional approach such as wrapping the code in an if-statement that checks if the environment is development.
- DEV: shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path');
+ if (!isProduction) {
+ shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path');
+ }
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.
DEV: shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path'); | |
if (!isProduction) { | |
shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path'); | |
} |
Tools
Biome
[error] 31-31: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
} | ||
} | ||
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path'); |
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.
Potential Issue with Label Usage
Similar to the previous file, the use of a label (DEV:
) here is unconventional and flagged by static analysis as potentially confusing. Labels are typically used with loops in JavaScript, not with function calls.
Suggestion: If the intent was to mark this section of code as development-specific, consider using a more conventional approach such as wrapping the code in an if-statement that checks if the environment is development.
- DEV: shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path');
+ if (!isProduction) {
+ shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path');
+ }
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.
DEV: shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path'); | |
if (!isProduction) { | |
shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path'); | |
} |
Tools
Biome
[error] 28-28: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
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.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/types/src/definitions/runtime/TimerState.type.ts
is excluded by none and included by none
Files selected for processing (6)
- apps/server/src/services/TimerService.ts (4 hunks)
- apps/server/src/services/tests/timerUtils.test.ts (14 hunks)
- apps/server/src/services/runtime-service/RuntimeService.ts (6 hunks)
- apps/server/src/services/timerUtils.ts (5 hunks)
- apps/server/src/stores/tests/runtimeState.test.ts (1 hunks)
- apps/server/src/stores/runtimeState.ts (9 hunks)
Files skipped from review due to trivial changes (1)
- apps/server/src/stores/tests/runtimeState.test.ts
Additional context used
Biome
apps/server/src/services/TimerService.ts
[error] 88-88: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 88-88: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
Additional comments not posted (16)
apps/server/src/services/TimerService.ts (4)
8-8
: Approved: New type alias for update callbacks.The addition of
UpdateCallbackFn
type alias improves type safety and clarity in callback handling.
19-19
: Approved: Initialization of callback properties to undefined.Initializing
endCallback
andonUpdateCallback
asundefined
is a good practice, making the state management more predictable.Also applies to: 21-21
40-42
: Approved: Method for setting update callbacks.The
setOnUpdateCallback
method enhances modularity by allowing dynamic assignment of the update callback function.
104-104
: Approved: Use of optional chaining for callback invocation.Using optional chaining (
?.
) for invokingonUpdateCallback
ensures that the code does not throw if the callback is not set.apps/server/src/services/__tests__/timerUtils.test.ts (5)
Line range hint
1366-1407
: RefactoredgetRuntimeOffset
function.The changes to
getRuntimeOffset
remove unnecessary properties and simplify the state management. This aligns with the PR objectives.
Line range hint
1699-1738
: UpdatedgetTimerPhase
function.The removal of the
secondaryTarget
property simplifies the function and reduces complexity, which is beneficial for maintenance and future modifications.
Line range hint
1-1738
: Removal ofupdateRoll
function.The function has been completely removed as intended. This should help in simplifying the timer update logic as per the PR objectives. Make sure to verify that no residual calls or dependencies are left in the codebase.
Verification successful
Verification successful: The
updateRoll
function has been completely removed from the codebase, and there are no residual calls or dependencies left.
- No occurrences of
updateRoll
were found in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `updateRoll` is not called anywhere in the codebase. # Test: Search for the function usage. Expect: No occurrences. rg --type typescript -A 5 $'updateRoll'Length of output: 73
Script:
#!/bin/bash # Description: Verify that `updateRoll` is not called anywhere in the codebase. # Test: Search for the function usage. Expect: No occurrences. rg --type ts -A 5 'updateRoll'Length of output: 30
Line range hint
1-1738
: Review ofgetExpectedFinish
function and its tests.The function and its tests appear to be correctly implemented. Ensure that the changes in other parts of the code do not indirectly affect this function's behavior.
Verification successful
Verification of
getExpectedFinish
function and its testsThe
getExpectedFinish
function and its tests are correctly implemented and interact as expected with other parts of the codebase.
- Instances of
getExpectedFinish
were found inruntimeState.ts
andtimerUtils.test.ts
.- The interactions with
getRuntimeOffset
andgetTimerPhase
are consistent with the expected functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that changes in `getRuntimeOffset` and `getTimerPhase` do not affect `getExpectedFinish`. # Test: Search for mentions of `getExpectedFinish` in the context of the modified functions. Expect: No unintended interactions. rg --type typescript -A 5 $'getExpectedFinish'Length of output: 80
Script:
#!/bin/bash # Description: Verify that changes in `getRuntimeOffset` and `getTimerPhase` do not affect `getExpectedFinish`. # Test: Search for mentions of `getExpectedFinish` in the context of the modified functions. Expect: No unintended interactions. rg --type ts -A 5 $'getExpectedFinish'Length of output: 10927
Line range hint
1-1738
: Review ofgetCurrent
function and its tests.The function and its tests are correctly implemented. Ensure that the changes in other parts of the code do not indirectly affect this function's behavior.
Verification successful
Ensure
getRuntimeOffset
andgetTimerPhase
do not affectgetCurrent
.The
getCurrent
function is used in conjunction withgetRuntimeOffset
in several places. After reviewing the code, there are no unintended interactions between these functions. The tests forgetCurrent
cover various scenarios, ensuring its correctness.
apps/server/src/stores/runtimeState.ts
apps/server/src/services/timerUtils.ts
apps/server/src/services/__tests__/timerUtils.test.ts
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that changes in `getRuntimeOffset` and `getTimerPhase` do not affect `getCurrent`. # Test: Search for mentions of `getCurrent` in the context of the modified functions. Expect: No unintended interactions. rg --type typescript -A 5 $'getCurrent'Length of output: 73
Script:
#!/bin/bash # Description: Verify that changes in `getRuntimeOffset` and `getTimerPhase` do not affect `getCurrent`. # Test: Search for mentions of `getCurrent` in the context of the modified functions. Expect: No unintended interactions. rg --type ts -A 5 $'getCurrent'Length of output: 10750
apps/server/src/services/timerUtils.ts (3)
327-327
: Export ofisPlaybackActive
function approved.Ensure that the newly exported
isPlaybackActive
function is being used appropriately in other modules.Verification successful
Export of
isPlaybackActive
function approved.The
isPlaybackActive
function is being used appropriately in other modules, confirming the export change is correct.
apps/server/src/stores/runtimeState.ts
: Usage ofisPlaybackActive
.apps/server/src/services/timerUtils.ts
: Original function definition and internal usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of `isPlaybackActive` across the codebase. rg --type ts "isPlaybackActive" "apps/server/src"Length of output: 363
346-346
: Update ingetTimerPhase
function approved.The addition of a check for
state.timer.secondaryTimer
not being null enhances the function's robustness. Verify its impact on the timer's behavior across different scenarios.Verification successful
Update in
getTimerPhase
function approved.The addition of a check for
state.timer.secondaryTimer
not being null enhances the function's robustness. The existing tests inapps/server/src/services/__tests__/timerUtils.test.ts
cover various scenarios, ensuring the function's behavior is validated.
apps/server/src/services/__tests__/timerUtils.test.ts
: Contains tests forgetTimerPhase
function.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Test the `getTimerPhase` function across different scenarios to ensure robust behavior. # Placeholder for a more detailed test script. echo "Testing `getTimerPhase` function..."Length of output: 148
Script:
#!/bin/bash # Locate the definition of the `getTimerPhase` function in the codebase. ast-grep --lang typescript --pattern 'function getTimerPhase($_) { $$$ }' # Find all usages of the `getTimerPhase` function to understand its impact. rg 'getTimerPhase' -A 5Length of output: 6120
Line range hint
13-13
: Verify removal oftimerConfig
import.Ensure that
timerConfig
is no longer used in this file. If it is still referenced, this could lead to runtime errors.Verification successful
Line range hint
13-13
: Verified removal oftimerConfig
import.The
timerConfig
import has been correctly removed fromtimerUtils.ts
as there are no references to it in this file. However,timerConfig
is still in use in other parts of the codebase.
apps/server/src/stores/runtimeState.ts
apps/server/src/services/TimerService.ts
apps/server/src/services/runtime-service/rundownService.utils.ts
apps/server/src/services/runtime-service/RuntimeService.ts
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining usage of `timerConfig` in `timerUtils.ts`. rg --type ts "timerConfig" "apps/server/src/services/timerUtils.ts"Length of output: 65
Script:
#!/bin/bash # Search for `timerConfig` in the entire codebase to ensure it is not being used elsewhere. # Search in the config.js file rg --type js "timerConfig" "apps/server/src/config/config.js" # Search in the broader codebase rg --type ts "timerConfig"Length of output: 2161
apps/server/src/stores/runtimeState.ts (2)
18-24
: Clarify the purpose of property changes and validate logic.The added comments help clarify the conditions under which properties change, which is beneficial for maintainability. However, ensure that the logic changes, especially around
expectedFinish
andforceFinish
, are thoroughly tested to prevent bugs.Also applies to: 29-38, 51-51
Verification successful
Verification successful for property changes in
runtimeState.ts
.The logic changes around
expectedFinish
andforceFinish
are handled correctly across the codebase. The added comments help clarify the conditions under which these properties change, which is beneficial for maintainability.
expectedFinish
andforceFinish
are used in various files, includingruntimeState.ts
,TimerService.ts
,timerUtils.ts
, and several test files.- The logic for these properties appears to be implemented correctly, with no issues found in the search results.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic changes to properties in runtimeState.ts. # Test: Search for usages of `expectedFinish` and `forceFinish` to ensure they are handled correctly. rg --type typescript -A 5 $'expectedFinish|forceFinish'Length of output: 91
Script:
#!/bin/bash # Description: Verify the logic changes to properties in runtimeState.ts. # Test: Search for usages of `expectedFinish` and `forceFinish` to ensure they are handled correctly. rg --type ts -A 5 'expectedFinish|forceFinish'Length of output: 29190
373-426
: Review updates to theupdate
function for potential issues.The
update
function has undergone significant changes. Ensure that the new logic, especially the handling of different playback states and the newupdateIfWaitingToRoll
function, integrates well with the rest of the system and does not introduce regressions.- runtimeState.timer.secondaryTimer = targetTime - runtimeState.clock; + // Ensure that secondaryTimer is not set to a negative value + runtimeState.timer.secondaryTimer = Math.max(0, targetTime - runtimeState.clock);Verification successful
Ensure
secondaryTimer
is not set to a negative value.The
update
function is used in various parts of the codebase, including tests and service files, indicating that it is well-integrated. The proposed change to ensuresecondaryTimer
is not set to a negative value is a valid improvement and should be implemented.
- File:
apps/server/src/stores/runtimeState.ts
- Lines: 373-426
- Change:
- runtimeState.timer.secondaryTimer = targetTime - runtimeState.clock; + // Ensure that secondaryTimer is not set to a negative value + runtimeState.timer.secondaryTimer = Math.max(0, targetTime - runtimeState.clock);Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new logic in the `update` function. # Test: Search for usages of the `update` function to ensure it is called correctly. rg --type typescript -A 5 $'update'Length of output: 69
Script:
#!/bin/bash # Description: Verify the new logic in the `update` function. # Test: Search for usages of the `update` function to ensure it is called correctly. rg --type-add 'ts:*.{ts,tsx}' -A 5 updateLength of output: 129220
apps/server/src/services/runtime-service/RuntimeService.ts (2)
42-50
: Validate new properties and logic inRuntimeService
.The new properties and the added comments improve clarity, which is good. However, ensure that the new logic for handling timer updates is correct and does not introduce any unintended side effects.
Verification successful
The new properties and logic in
RuntimeService
are correctly implemented.The
previousTimerValue
property is properly initialized, used in conditional checks, and updated with the current timer value. The added comments help in understanding its purpose.
RuntimeService.ts
:
- Initialization:
RuntimeService.previousTimerValue = -1;
- Conditional check:
getShouldTimerUpdate(RuntimeService.previousTimerValue, state.timer.current);
- Update:
RuntimeService.previousTimerValue = state.timer.current;
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify new properties and logic in RuntimeService.ts. # Test: Search for usages of `previousTimerValue` to ensure it is handled correctly. rg --type typescript -A 5 $'previousTimerValue'Length of output: 81
Script:
#!/bin/bash # Description: Verify new properties and logic in RuntimeService.ts. # Test: Search for usages of `previousTimerValue` to ensure it is handled correctly. rg --type ts -A 5 'previousTimerValue'Length of output: 2478
Line range hint
54-143
: Thoroughly review new functions and modifications inRuntimeService
.The modifications and new functions in
RuntimeService
are crucial for the service's functionality. Ensure that thecheckTimerUpdate
function and the newbroadcastResult
decorator are implemented correctly and efficiently to avoid performance issues.- const shouldUpdateTimer = getShouldTimerUpdate(this.lastIntegrationTimerValue, newState.timer.current); + // Ensure that the timer is only updated when necessary to avoid performance issues + const shouldUpdateTimer = this.lastIntegrationTimerValue !== newState.timer.current && getShouldTimerUpdate(this.lastIntegrationTimerValue, newState.timer.current);Verification successful
Verify the optimization in
shouldUpdateTimer
conditionThe
checkTimerUpdate
function and thebroadcastResult
decorator are correctly implemented and used within theRuntimeService
. The suggested change to theshouldUpdateTimer
condition is a valid optimization to ensure the timer is only updated when necessary.
- File:
apps/server/src/services/runtime-service/RuntimeService.ts
- Line: 98
- const shouldUpdateTimer = getShouldTimerUpdate(this.lastIntegrationTimerValue, newState.timer.current); + const shouldUpdateTimer = this.lastIntegrationTimerValue !== newState.timer.current && getShouldTimerUpdate(this.lastIntegrationTimerValue, newState.timer.current);Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of new functions in RuntimeService.ts. # Test: Search for usages of `checkTimerUpdate` and `broadcastResult` to ensure they are implemented correctly. rg --type typescript -A 5 $'checkTimerUpdate|broadcastResult'Length of output: 97
Script:
#!/bin/bash # Description: Verify the implementation of new functions in RuntimeService.ts. # Test: Search for usages of `checkTimerUpdate` and `broadcastResult` to ensure they are implemented correctly. rg -t ts -A 5 'checkTimerUpdate|broadcastResult'Length of output: 5763
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: { | ||
if (state.timer.expectedFinish === null) { | ||
consoleError('TimerService.addTime: expectedFinish is negative'); | ||
throw new Error('TimerService.addTime: expectedFinish is negative'); | ||
} | ||
} |
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.
Issue: Misuse of labels and potential error handling flaw.
The label DEV:
is not used correctly here, which could lead to confusion and maintenance issues. Also, the error message and the condition do not align since the condition checks for null
but the error message states "negative."
- DEV: {
- if (state.timer.expectedFinish === null) {
+ if (state.timer.expectedFinish < 0) {
consoleError('TimerService.addTime: expectedFinish is negative');
throw new Error('TimerService.addTime: expectedFinish is negative');
- }
+ }
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.
// eslint-disable-next-line no-unused-labels -- dev code path | |
DEV: { | |
if (state.timer.expectedFinish === null) { | |
consoleError('TimerService.addTime: expectedFinish is negative'); | |
throw new Error('TimerService.addTime: expectedFinish is negative'); | |
} | |
} | |
if (state.timer.expectedFinish < 0) { | |
consoleError('TimerService.addTime: expectedFinish is negative'); | |
throw new Error('TimerService.addTime: expectedFinish is negative'); | |
} |
Tools
Biome
[error] 88-88: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 88-88: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
// console.log( | ||
// state, | ||
// previousTime, | ||
// skipLimit, | ||
// hasSkipped && (adjustedClock > adjustedExpectedFinish || adjustedClock < startedAt), | ||
// ); |
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.
Consider removing or controlling the commented-out debug log.
Leaving commented-out code can clutter the codebase. It's better to either remove these lines or implement a logging level control to enable/disable logs dynamically.
- // console.log(
- // state,
- // previousTime,
- // skipLimit,
- // hasSkipped && (adjustedClock > adjustedExpectedFinish || adjustedClock < startedAt),
- // );
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.
// console.log( | |
// state, | |
// previousTime, | |
// skipLimit, | |
// hasSkipped && (adjustedClock > adjustedExpectedFinish || adjustedClock < startedAt), | |
// ); |
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.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/types/src/definitions/runtime/TimerState.type.ts
is excluded by none and included by none
Files selected for processing (6)
- apps/server/src/services/TimerService.ts (4 hunks)
- apps/server/src/services/tests/timerUtils.test.ts (14 hunks)
- apps/server/src/services/runtime-service/RuntimeService.ts (6 hunks)
- apps/server/src/services/timerUtils.ts (8 hunks)
- apps/server/src/stores/tests/runtimeState.test.ts (1 hunks)
- apps/server/src/stores/runtimeState.ts (9 hunks)
Files skipped from review as they are similar to previous changes (4)
- apps/server/src/services/tests/timerUtils.test.ts
- apps/server/src/services/runtime-service/RuntimeService.ts
- apps/server/src/stores/tests/runtimeState.test.ts
- apps/server/src/stores/runtimeState.ts
Additional context used
GitHub Check: unit-test
apps/server/src/services/TimerService.ts
[failure] 6-6:
'consoleError' is defined but never used. Allowed unused vars must match /^_/u
Biome
apps/server/src/services/TimerService.ts
[error] 88-88: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 88-88: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
apps/server/src/services/timerUtils.ts
[error] 59-59: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 59-59: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 97-97: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 97-97: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 272-272: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 272-272: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
Additional comments not posted (5)
apps/server/src/services/TimerService.ts (5)
8-8
: Approved: New type alias for update callbacks.This new type alias
UpdateCallbackFn
simplifies the type definitions across the service, making the code cleaner and easier to maintain.
19-19
: Approved: Initialization ofendCallback
toundefined
.Initializing
endCallback
toundefined
is a good practice as it explicitly states that the timer is not set initially. This change improves readability and state management.
21-21
: Approved: Change inonUpdateCallback
to use the new type alias and be optionally undefined.This change not only uses the newly defined type alias but also makes the callback optional, which enhances flexibility in how the timer updates are handled.
40-42
: Approved: Addition ofsetOnUpdateCallback
method.Adding a method to set the update callback is a clean way to manage this functionality, allowing for dynamic changes to the callback handling during runtime.
103-103
: Approved: Use of optional chaining for callback invocation.The use of optional chaining (
?.
) here is appropriate for callingonUpdateCallback
as it safely handles cases where the callback might not be set.
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.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
apps/server/src/services/timerUtils.ts (1)
Line range hint
10-32
: Add a null check forduration
to prevent potential runtime errors.The function uses
duration!
which assumesduration
is not null. It's safer to add a check before using it.- const expectedFinish = startedAt + duration! + addedTime + pausedTime; + if (duration === null) throw new Error('Duration must be set'); + const expectedFinish = startedAt + duration + addedTime + pausedTime;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/types/src/definitions/runtime/TimerState.type.ts
is excluded by none and included by none
Files selected for processing (6)
- apps/server/src/services/TimerService.ts (4 hunks)
- apps/server/src/services/tests/timerUtils.test.ts (14 hunks)
- apps/server/src/services/runtime-service/RuntimeService.ts (6 hunks)
- apps/server/src/services/timerUtils.ts (8 hunks)
- apps/server/src/stores/tests/runtimeState.test.ts (1 hunks)
- apps/server/src/stores/runtimeState.ts (9 hunks)
Files skipped from review as they are similar to previous changes (4)
- apps/server/src/services/tests/timerUtils.test.ts
- apps/server/src/services/runtime-service/RuntimeService.ts
- apps/server/src/stores/tests/runtimeState.test.ts
- apps/server/src/stores/runtimeState.ts
Additional context used
Biome
apps/server/src/services/TimerService.ts
[error] 87-87: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 87-87: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
apps/server/src/services/timerUtils.ts
[error] 59-59: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 59-59: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 97-97: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 97-97: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 272-272: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 272-272: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
Additional comments not posted (11)
apps/server/src/services/TimerService.ts (5)
7-7
: Type aliasUpdateCallbackFn
correctly defined.The new type alias for update callbacks is correctly defined and aligns with the PR objectives to simplify callback handling.
18-18
: Proper initialization ofendCallback
property.Changing the
endCallback
property to allowundefined
improves safety and initialization clarity.
20-20
: Initialization ofonUpdateCallback
withundefined
is appropriate.This change aligns with the new
UpdateCallbackFn
type and allows the property to be safely uninitialized, enhancing code safety and clarity.
102-102
: Proper use of optional chaining inupdate
.Using optional chaining for
onUpdateCallback
ensures that the method call is safe even if the callback is not set, aligning with the PR's goal to enhance robustness.
39-41
: MethodsetOnUpdateCallback
correctly implemented.The addition of
setOnUpdateCallback
simplifies the management of update callbacks and uses the newUpdateCallbackFn
type correctly.Please ensure this method is utilized properly wherever the timer updates are handled.
Verification successful
Method
setOnUpdateCallback
correctly implemented and utilized.The addition of
setOnUpdateCallback
simplifies the management of update callbacks and uses the newUpdateCallbackFn
type correctly. The method is properly utilized inRuntimeService.ts
.
apps/server/src/services/runtime-service/RuntimeService.ts
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `setOnUpdateCallback` in the codebase. # Test: Search for the method usage. Expect: Proper usage in relevant parts of the code. rg --type typescript -A 5 $'setOnUpdateCallback'Length of output: 82
Script:
#!/bin/bash # Description: Verify usage of `setOnUpdateCallback` in the codebase. # Test: Search for the method usage. Expect: Proper usage in relevant parts of the code. rg --type ts -A 5 $'setOnUpdateCallback'Length of output: 960
apps/server/src/services/timerUtils.ts (6)
6-6
: FunctionnormaliseEndTime
is implemented correctly.This function handles the adjustment of end times for events that span over midnight effectively.
Line range hint
103-210
: FunctiongetRollTimers
handles complex conditions effectively.This function manages to determine the current and next events based on the time and event properties, handling midnight spans and public visibility efficiently.
Tools
Biome
[error] 97-97: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 97-97: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
Line range hint
280-290
: FunctiongetTotalDuration
is implemented correctly.This function accurately calculates the total duration of a time span, considering events that span across midnight.
Tools
Biome
[error] 272-272: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 272-272: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
330-330
: FunctiongetExpectedEnd
is implemented correctly.This function properly calculates the expected end of the rundown, ensuring all necessary conditions are met before performing the calculation.
341-341
: FunctionisPlaybackActive
is implemented correctly.This function accurately determines if the playback is considered active based on the timer state.
360-360
: FunctiongetTimerPhase
handles complex conditions effectively.This function manages to determine the current phase of the timer based on various conditions and states, using the
isPlaybackActive
function effectively.
if (state.timer.expectedFinish === null) { | ||
throw new Error('TimerService.addTime: expectedFinish is negative'); | ||
} |
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.
Misalignment between condition and error message in addTime
.
The error handling in addTime
checks for null
but logs an error for a negative value, which is misleading.
- if (state.timer.expectedFinish === null) {
+ if (state.timer.expectedFinish < 0) {
consoleError('TimerService.addTime: expectedFinish is negative');
throw new Error('TimerService.addTime: expectedFinish is negative');
}
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.
if (state.timer.expectedFinish === null) { | |
throw new Error('TimerService.addTime: expectedFinish is negative'); | |
} | |
if (state.timer.expectedFinish < 0) { | |
consoleError('TimerService.addTime: expectedFinish is negative'); | |
throw new Error('TimerService.addTime: expectedFinish is negative'); | |
} |
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.
Should roll trigger a load and start dispatch when it rolls an event?
Because that seams to not happen right now
@@ -88,6 +93,12 @@ export function getCurrent(state: RuntimeState): number { | |||
* @returns {boolean} | |||
*/ | |||
export function skippedOutOfEvent(state: RuntimeState, previousTime: number, skipLimit: number): boolean { | |||
// eslint-disable-next-line no-unused-labels -- dev code path | |||
DEV: { | |||
if (state.timer.expectedFinish === null || state.timer.startedAt === null) { |
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'm also hitting this
just by creating an event 1 minnut from now and then clicking roll
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.
@cpvalente
how about this is it intended?
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.
This should no longer be called before an event is loaded. Is that not the case?
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.
looks to work fine after the recent force push
d721d55
I havent added any new behaviour. Just removed the paths specific to roll. We would need a follow up ticket to improve roll |
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.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
apps/server/src/services/timerUtils.ts (1)
98-98
: Consider removing or controlling the commented-out debug log.Leaving commented-out code can clutter the codebase. It's better to either remove these lines or implement a logging level control to enable/disable logs dynamically.
- // console.log( - // state, - // previousTime, - // skipLimit, - // hasSkipped && (adjustedClock > adjustedExpectedFinish || adjustedClock < startedAt), - // );
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/types/src/definitions/runtime/TimerState.type.ts
is excluded by none and included by none
Files selected for processing (6)
- apps/server/src/services/TimerService.ts (4 hunks)
- apps/server/src/services/tests/timerUtils.test.ts (14 hunks)
- apps/server/src/services/runtime-service/RuntimeService.ts (6 hunks)
- apps/server/src/services/timerUtils.ts (8 hunks)
- apps/server/src/stores/tests/runtimeState.test.ts (1 hunks)
- apps/server/src/stores/runtimeState.ts (9 hunks)
Files skipped from review as they are similar to previous changes (4)
- apps/server/src/services/tests/timerUtils.test.ts
- apps/server/src/services/runtime-service/RuntimeService.ts
- apps/server/src/stores/tests/runtimeState.test.ts
- apps/server/src/stores/runtimeState.ts
Additional context used
Biome
apps/server/src/services/TimerService.ts
[error] 87-87: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 87-87: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
apps/server/src/services/timerUtils.ts
[error] 59-59: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 59-59: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 97-97: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 97-97: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 272-272: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 272-272: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
Additional comments not posted (7)
apps/server/src/services/TimerService.ts (5)
7-7
: LGTM: Addition ofUpdateCallbackFn
type alias.The addition of the
UpdateCallbackFn
type alias improves code readability and maintainability.
18-18
: LGTM: Initialization ofendCallback
property.Initializing the
endCallback
property asundefined
helps prevent potential runtime errors.
20-20
: LGTM: Initialization ofonUpdateCallback
property.Initializing the
onUpdateCallback
property asundefined
helps prevent potential runtime errors.
35-41
: LGTM: Addition ofsetOnUpdateCallback
method.The
setOnUpdateCallback
method provides a clear and flexible way to set theonUpdateCallback
property, enhancing the functionality and maintainability of theTimerService
class.
102-102
: LGTM: Use of optional chaining foronUpdateCallback
.Using optional chaining ensures that the
onUpdateCallback
is only called if it is defined, preventing potential runtime errors.apps/server/src/services/timerUtils.ts (2)
341-341
: LGTM: ExportingisPlaybackActive
function.Exporting the
isPlaybackActive
function improves code reusability.
360-360
: LGTM: Modification togetTimerPhase
function.Including a check for
state.timer.secondaryTimer
not being null ensures that the function correctly identifies the timer phase when a secondary timer is present.
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.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
apps/server/package.json
is excluded by none and included by nonepackages/types/src/definitions/runtime/TimerState.type.ts
is excluded by none and included by none
Files selected for processing (10)
- apps/server/src/app.ts (1 hunks)
- apps/server/src/classes/data-provider/DataProvider.ts (1 hunks)
- apps/server/src/services/TimerService.ts (4 hunks)
- apps/server/src/services/tests/timerUtils.test.ts (14 hunks)
- apps/server/src/services/app-state-service/AppStateService.ts (2 hunks)
- apps/server/src/services/runtime-service/RuntimeService.ts (6 hunks)
- apps/server/src/services/timerUtils.ts (8 hunks)
- apps/server/src/stores/tests/runtimeState.test.ts (1 hunks)
- apps/server/src/stores/runtimeState.ts (9 hunks)
- apps/server/src/utils/development.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- apps/server/src/app.ts
- apps/server/src/stores/tests/runtimeState.test.ts
- apps/server/src/utils/development.ts
Additional context used
Biome
apps/server/src/services/app-state-service/AppStateService.ts
[error] 31-31: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
apps/server/src/services/TimerService.ts
[error] 87-87: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 87-87: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
apps/server/src/classes/data-provider/DataProvider.ts
[error] 28-28: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
apps/server/src/services/timerUtils.ts
[error] 59-59: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 59-59: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 97-97: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 97-97: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 272-272: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 272-272: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
Additional comments not posted (24)
apps/server/src/services/app-state-service/AppStateService.ts (1)
4-6
: LGTM!The import changes are appropriate and align with the new error handling strategy.
apps/server/src/services/TimerService.ts (5)
7-8
: LGTM!The addition of the
UpdateCallbackFn
type alias improves code readability and maintainability.
18-18
: LGTM!Initializing
endCallback
asundefined
ensures that it has a clear initial state.
20-20
: LGTM!Initializing
onUpdateCallback
asundefined
ensures that it has a clear initial state.
28-28
: LGTM!The removal of the
onUpdateCallback
parameter simplifies the constructor and aligns with the new method for setting the callback.
35-41
: LGTM!The addition of the
setOnUpdateCallback
method improves the flexibility and modularity of theTimerService
class.apps/server/src/classes/data-provider/DataProvider.ts (1)
16-20
: LGTM!The import changes are appropriate and align with the new error handling strategy.
apps/server/src/services/timerUtils.ts (3)
330-332
: LGTM!The modification ensures that the function returns null if either
state.runtime.actualStart
orstate.runtime.plannedEnd
is null.
Line range hint
341-345
:
LGTM!The modification makes the function available for use in other modules.
360-362
: LGTM!The modification ensures that the function returns
TimerPhase.Pending
ifstate.timer.secondaryTimer
is not null.apps/server/src/stores/runtimeState.ts (6)
18-24
: LGTM!The modifications improve the clarity of the code by providing explanations for the changes in state properties.
Also applies to: 29-38, 51-53
Line range hint
75-83
:
LGTM!The modifications ensure that the function resets all relevant state properties when clearing the state.
124-125
: LGTM!The modifications ensure that the function correctly updates the state properties based on the rundown data.
Line range hint
134-153
:
LGTM!The modifications ensure that the function correctly loads an event and updates the state based on the provided initial data.
373-426
: LGTM!The modifications ensure that the function correctly updates the timer and handles playback states, including edge cases related to roll and timer finishing.
Line range hint
455-460
:
LGTM!The modifications ensure that the function correctly handles rolling events and updates the state accordingly.
apps/server/src/services/runtime-service/RuntimeService.ts (6)
54-56
: LGTM!The modification ensures that the
RuntimeService
class is initialized with atimerService
instance, improving modularity and reusability.
Line range hint
67-133
:
LGTM!The modifications ensure that the function correctly handles timer phase transitions, roll events, and updates to the timer and clock, improving the overall robustness of the timer management logic.
147-149
: LGTM!The modifications ensure that the function correctly initializes the
eventTimer
and handles resumable states, improving the initialization logic.
Line range hint
455-460
:
LGTM!The modifications ensure that the function correctly handles rolling events and updates the state accordingly.
575-578
: LGTM!The modifications ensure that the function correctly manages side effects during runtime updates, improving the overall robustness of the runtime management logic.
573-573
: LGTM!The export statement ensures that the
runtimeService
is available for use in other modules, improving modularity and reusability.apps/server/src/services/__tests__/timerUtils.test.ts (2)
1366-1366
: LGTM! Simplification ofgetRuntimeOffset
.The removal of the
secondaryTarget
property and setting theoffset
property to0
instead ofnull
are logical improvements that simplify the function.Also applies to: 1407-1407, 1442-1442, 1459-1459, 1494-1494, 1511-1511, 1534-1534, 1551-1551
1699-1699
: LGTM! Simplification ofgetTimerPhase
.The removal of the
secondaryTarget
property is a logical improvement that simplifies the function.Also applies to: 1738-1738
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path'); |
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.
Remove the DEV:
label and use a more conventional approach.
The use of the DEV:
label is unconventional and flagged by static analysis tools. Consider wrapping the code in an if-statement that checks if the environment is development.
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path');
+ if (!isProduction) {
+ shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path');
+ }
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.
// eslint-disable-next-line no-unused-labels -- dev code path | |
DEV: shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path'); | |
if (!isProduction) { | |
shouldCrashDev(isPath(filename), 'setLastLoadedProject should not be called with a path'); | |
} |
Tools
Biome
[error] 31-31: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: { | ||
if (state.timer.expectedFinish === null) { | ||
throw new Error('TimerService.addTime: expectedFinish is negative'); | ||
} | ||
} |
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.
Remove the DEV:
label and use a more conventional approach.
The use of the DEV:
label is unconventional and flagged by static analysis tools. Consider wrapping the code in an if-statement that checks if the environment is development.
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: {
- if (state.timer.expectedFinish === null) {
- throw new Error('TimerService.addTime: expectedFinish is negative');
- }
- }
+ if (!isProduction) {
+ if (state.timer.expectedFinish === null) {
+ throw new Error('TimerService.addTime: expectedFinish is negative');
+ }
+ }
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.
// eslint-disable-next-line no-unused-labels -- dev code path | |
DEV: { | |
if (state.timer.expectedFinish === null) { | |
throw new Error('TimerService.addTime: expectedFinish is negative'); | |
} | |
} | |
if (!isProduction) { | |
if (state.timer.expectedFinish === null) { | |
throw new Error('TimerService.addTime: expectedFinish is negative'); | |
} | |
} |
Tools
Biome
[error] 87-87: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 87-87: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path'); |
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.
Remove the DEV:
label and use a more conventional approach.
The use of the DEV:
label is unconventional and flagged by static analysis tools. Consider wrapping the code in an if-statement that checks if the environment is development.
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path');
+ if (!isProduction) {
+ shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path');
+ }
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.
// eslint-disable-next-line no-unused-labels -- dev code path | |
DEV: shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path'); | |
if (!isProduction) { | |
shouldCrashDev(!isPath(filePath), 'initPersistence should be called with a path'); | |
} |
Tools
Biome
[error] 28-28: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: { | ||
// we know current exists as long as eventNow exists | ||
if (state.timer.current === null) { | ||
throw new Error('timerUtils.calculate: current must be set'); | ||
} | ||
} |
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.
Remove unused label and refine error handling logic.
The label DEV
is not used correctly and should be removed. The error handling can be improved by using more descriptive error messages and ensuring they align with the conditions checked.
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: {
- // we know current exists as long as eventNow exists
- if (state.timer.current === null) {
- throw new Error('timerUtils.calculate: current must be set');
- }
- }
+ if (state.timer.current === null) {
+ console.error('timerUtils.calculate: current must be set');
+ throw new Error('timerUtils.calculate: current must be set');
+ }
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.
// eslint-disable-next-line no-unused-labels -- dev code path | |
DEV: { | |
// we know current exists as long as eventNow exists | |
if (state.timer.current === null) { | |
throw new Error('timerUtils.calculate: current must be set'); | |
} | |
} | |
if (state.timer.current === null) { | |
console.error('timerUtils.calculate: current must be set'); | |
throw new Error('timerUtils.calculate: current must be set'); | |
} |
Tools
Biome
[error] 272-272: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 272-272: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: { | ||
if (state.eventNow === null || state.timer.duration === null) { | ||
throw new Error('timerUtils.getCurrent: invalid state received'); | ||
} | ||
} |
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.
Remove unused label and refine error handling logic.
The label DEV
is not used correctly and should be removed. The error handling can be improved by using more descriptive error messages and ensuring they align with the conditions checked.
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: {
- if (state.eventNow === null || state.timer.duration === null) {
- throw new Error('timerUtils.getCurrent: invalid state received');
- }
- }
+ if (state.eventNow === null || state.timer.duration === null) {
+ console.error('timerUtils.getCurrent: invalid state received');
+ throw new Error('timerUtils.getCurrent: invalid state received');
+ }
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.
// eslint-disable-next-line no-unused-labels -- dev code path | |
DEV: { | |
if (state.eventNow === null || state.timer.duration === null) { | |
throw new Error('timerUtils.getCurrent: invalid state received'); | |
} | |
} | |
if (state.eventNow === null || state.timer.duration === null) { | |
console.error('timerUtils.getCurrent: invalid state received'); | |
throw new Error('timerUtils.getCurrent: invalid state received'); | |
} |
Tools
Biome
[error] 59-59: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 59-59: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
// eslint-disable-next-line no-unused-labels -- dev code path | ||
DEV: { | ||
if (state.timer.expectedFinish === null || state.timer.startedAt === null) { | ||
throw new Error('timerUtils.skippedOutOfEvent: invalid state received'); | ||
} | ||
} |
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.
Remove unused label and refine error handling logic.
The label DEV
is not used correctly and should be removed. The error handling can be improved by using more descriptive error messages and ensuring they align with the conditions checked.
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: {
- if (state.timer.expectedFinish === null || state.timer.startedAt === null) {
- throw new Error('timerUtils.skippedOutOfEvent: invalid state received');
- }
- }
+ if (state.timer.expectedFinish === null || state.timer.startedAt === null) {
+ console.error('timerUtils.skippedOutOfEvent: invalid state received');
+ throw new Error('timerUtils.skippedOutOfEvent: invalid state received');
+ }
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.
// eslint-disable-next-line no-unused-labels -- dev code path | |
DEV: { | |
if (state.timer.expectedFinish === null || state.timer.startedAt === null) { | |
throw new Error('timerUtils.skippedOutOfEvent: invalid state received'); | |
} | |
} | |
if (state.timer.expectedFinish === null || state.timer.startedAt === null) { | |
console.error('timerUtils.skippedOutOfEvent: invalid state received'); | |
throw new Error('timerUtils.skippedOutOfEvent: invalid state received'); | |
} |
Tools
Biome
[error] 97-97: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 97-97: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
the main objective of this PR is to simplify the code around updating the timer by removing code paths specific to roll
Making the functions agnostic of the play mode means
on the downside, we would need to consider that there is more logic involve for any of the code paths
We would need a separate PR to tidy up the behaviour around roll. I am not aware of any change of behaviour introduced by this PR