rundown reporter #1166
rundown reporter #1166
Conversation
WalkthroughThis update introduces a new reporting service in the application that manages event timing data. It includes a new file, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ReportService
participant RuntimeState
Client->>ReportService: Call eventStart(state)
ReportService->>RuntimeState: Set start time for event
Client->>ReportService: Call eventStop(state)
ReportService->>RuntimeState: Update end time and calculate duration
ReportService->>Client: Return updated report data
Client->>ReportService: Call generate()
ReportService->>Client: Return current report state
Possibly related PRs
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 (
|
|
@coderabbitai summary |
Actions performedSummary regeneration 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: 11
Outside diff range and nitpick comments (16)
apps/client/src/common/api/constants.ts (1)
10-10: LGTM! Consider grouping related constants.The addition of the
REPORTconstant is consistent with the existing pattern and purpose of this file. It's correctly implemented as an array containing a single string, which aligns with other similar constants.For improved readability and maintenance, consider grouping related constants together. For example, you could place
REPORTnext toRUNDOWNsince they seem to be related to event data:export const RUNDOWN = ['rundown']; export const REPORT = ['report'];This grouping can make it easier for developers to find and understand related constants.
apps/server/src/api-data/rundown/rundown.router.ts (1)
Line range hint
1-53: Summary of changes and suggestions for further reviewThe changes in this file are minimal and focused on adding a new '/report' route. The implementation looks correct, but there are a few points to consider:
- Ensure that the
getReportfunction in the controller is properly implemented and tested.- Consider whether any input validation is needed for the new route.
- Clarify the intended use of this new route, given that it's not used in the Ontime frontend.
To get a complete picture of this feature implementation, it would be beneficial to review:
- The implementation of the
getReportfunction in the controller file.- Any new or modified types or interfaces related to the report feature.
- Any changes to the database layer or services that might be providing data for this new report endpoint.
apps/server/src/services/report-service/__test__/reportService.test.ts (1)
5-5: Initialize mock state properlyThe mock state is initially set as an empty object and then overwritten in the
beforeEachblock. It's better to initialize it with a proper type from the start.Consider changing the line to:
let mockState: RuntimeState;This way, TypeScript can provide better type checking and autocompletion throughout the file.
apps/client/src/common/hooks-query/useRundown.ts (1)
43-54: LGTM: Well-structureduseReporthook, with a minor suggestion.The new
useReporthook is well-implemented and consistent with the existinguseRundownfunction. It correctly uses theuseQueryhook and includes appropriate parameters for query key, query function, placeholder data, retry logic, and refetch interval.Consider defining a more specific default value for the
dataproperty in the return object. Instead of using an empty object ({}), it might be beneficial to create a placeholderOntimeReportobject with default values. This could help prevent potential runtime errors if consumers of this hook expect certain properties to always be present.For example:
const defaultReport: OntimeReport = { // Add default properties based on OntimeReport structure }; return { data: data ?? defaultReport, ... };apps/client/src/common/utils/__tests__/dateConfig.test.js (2)
34-34: Approved: Simplified output format for durations less than an hour.The change to remove the hour component for durations less than an hour ('+10:30' instead of '+00:10:30') improves readability and conciseness.
Consider adding a comment explaining the rationale behind this format change to improve code maintainability.
39-41: Approved: New test case for durations greater than one hour.The addition of this test case improves coverage by ensuring correct handling of durations exceeding one hour. The expected output format ('+01:17:10') is appropriate for longer time periods.
Consider adding a test case for a duration just over 24 hours to ensure proper handling of multi-day periods, if relevant to the application's use case.
apps/client/src/common/api/rundown.ts (1)
88-94: LGTM: NewfetchReportfunction added correctlyThe new
fetchReportfunction is well-implemented and consistent with the existing code style. It correctly uses axios for the HTTP request and is properly typed.For consistency with other functions in this file, consider adding a brief JSDoc comment describing the function's purpose. For example:
/** * HTTP request to fetch ontime report */ export async function fetchReport(): Promise<OntimeReport> { const res = await axios.get(`${rundownPath}/report`); return res.data; }apps/client/src/features/rundown/event-block/EventBlock.module.scss (1)
135-150: New.overUnderSectionclass looks good, but colors need attentionThe new
.overUnderSectionclass is well-structured and consistent with the existing styling. However, there are two TODO comments regarding color properties that need to be addressed:
- Line 145:
color: $ontime-stop; //TODO: proper dynbamic color- Line 148:
color: $green-500; //TODO: proper dynbamic colorPlease update these color values with the appropriate dynamic colors as indicated in the TODO comments. This will ensure that the styling is complete and consistent with the rest of the application's color scheme.
apps/client/src/features/rundown/RundownEntry.tsx (2)
40-40: LGTM! Consider documenting theoverUnderproperty.The addition of the
overUnderproperty to theRundownEntryPropsinterface is a good way to extend the component's functionality. However, it would be helpful to add a brief comment explaining the purpose of this property and what it represents in the context of a rundown entry.Also, could you please clarify where the
MaybeNumbertype is defined? If it's imported from another module, consider adding an import statement for better code readability.
56-56: LGTM! Consider utilizing theoverUnderproperty in the component logic.The
overUnderproperty has been correctly added to the destructured props. However, I don't see any usage of this property within the component's logic. If there's an intended use for this property, consider implementing it. If it's meant for future use, it might be helpful to add a TODO comment explaining the planned usage.apps/client/src/features/rundown/event-block/EventBlock.tsx (2)
49-49: LGTM! Consider adding JSDoc comment for clarity.The addition of the
overUnderproperty to theEventBlockPropsinterface is well-structured and maintains backward compatibility.Consider adding a JSDoc comment to explain the purpose and expected values of the
overUnderproperty. For example:/** * Represents the over/under value for the event. * @type {number | null | undefined} */ overUnder?: MaybeNumber;
49-49: Overall impact: Well-integrated addition with minimal risk.The addition of the
overUnderproperty toEventBlockPropsand its usage in theEventBlockcomponent is well-implemented and maintains backward compatibility. This change appears to be part of the larger "rundown reporter" feature mentioned in the PR objectives.Consider the following to ensure complete implementation:
- Verify that all necessary components are updated to handle the
overUnderproperty.- Ensure that any data fetching or state management logic is updated to include this new property.
- Update relevant unit tests to cover scenarios involving the
overUnderproperty.- If applicable, update documentation or comments to explain the purpose and usage of this new property in the context of the "rundown reporter" feature.
Also applies to: 87-87, 274-274
apps/client/src/features/rundown/Rundown.tsx (2)
41-41: LGTM: RundownProps interface updated correctlyThe addition of the
report: OntimeReport;property to the RundownProps interface is consistent with the changes in the component and allows it to receive the report data as a prop.Consider adding a brief comment to describe the purpose of the
reportprop, especially if it's not immediately obvious from the type name alone. For example:interface RundownProps { data: RundownCached; /** Report containing timing information for events */ report: OntimeReport; }
281-283: LGTM: overUnder variable added correctlyThe addition of the
overUndervariable and its assignment from thereportprop is consistent with the new functionality. This change correctly introduces the use of the newreportprop within the component logic.Consider initializing the
overUndervariable when it's declared for better readability:let overUnder = report[eventId]?.overUnder;This would allow you to remove the assignment on line 301, making the code more concise.
apps/client/src/features/cuesheet/cuesheetCols.tsx (2)
38-38: Address the TODO comment regarding stylingThere's a
TODOcomment indicating that proper styling should be applied. Ensure that the component's styling aligns with the application's design guidelines.Would you like assistance in implementing the appropriate styling for this component?
35-43: Consider accessibility for color-blind usersUsing red and green colors might not be accessible to all users, especially those with color vision deficiencies. Consider adding icons, text labels, or different styling to convey the over/under status.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/types/src/definitions/core/Report.type.tsis excluded by none and included by nonepackages/types/src/index.tsis excluded by none and included by none
Files selected for processing (18)
- apps/client/src/common/api/constants.ts (1 hunks)
- apps/client/src/common/api/rundown.ts (2 hunks)
- apps/client/src/common/hooks-query/useRundown.ts (2 hunks)
- apps/client/src/common/utils/tests/dateConfig.test.js (1 hunks)
- apps/client/src/common/utils/dateConfig.ts (2 hunks)
- apps/client/src/features/cuesheet/CuesheetWrapper.tsx (2 hunks)
- apps/client/src/features/cuesheet/cuesheetCols.tsx (4 hunks)
- apps/client/src/features/rundown/Rundown.tsx (5 hunks)
- apps/client/src/features/rundown/RundownEntry.tsx (3 hunks)
- apps/client/src/features/rundown/RundownWrapper.tsx (2 hunks)
- apps/client/src/features/rundown/event-block/EventBlock.module.scss (3 hunks)
- apps/client/src/features/rundown/event-block/EventBlock.tsx (3 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (4 hunks)
- apps/server/src/api-data/rundown/rundown.controller.ts (3 hunks)
- apps/server/src/api-data/rundown/rundown.router.ts (2 hunks)
- apps/server/src/services/report-service/ReportService.ts (1 hunks)
- apps/server/src/services/report-service/test/reportService.test.ts (1 hunks)
- apps/server/src/stores/runtimeState.ts (6 hunks)
Additional comments not posted (46)
apps/client/src/features/rundown/RundownWrapper.tsx (2)
2-2: LGTM: New import added correctlyThe
useReporthook is imported alongsideuseRundown, which is a good practice for organizing related imports.
Line range hint
1-19: Overall, good implementation with room for minor improvementsThe changes to
RundownWrapper.tsxsuccessfully introduce the new report feature. The code is well-structured and maintains the existing functionality. Consider implementing the suggested improvements for error handling and conditional rendering to make the component more robust.apps/server/src/services/report-service/ReportService.ts (4)
1-9: LGTM: Imports and constant declarations are well-structured.The imports are appropriate for the service's functionality, and the use of a Map for report storage is an efficient choice. The
blankReportDataconstant provides a consistent initial state for report entries.
11-13: LGTM: generate() function is concise and efficient.The
generate()function effectively converts the Map to an object usingObject.fromEntries(), which is an efficient approach. It adheres to the single responsibility principle and provides the expected OntimeReport type.
15-17: LGTM: clear() function is simple and effective.The
clear()function efficiently uses the built-inclear()method of Map to remove all entries. It's concise and adheres to the single responsibility principle.
1-34: Overall, well-implemented reporting service with room for minor improvements.The
ReportService.tsfile introduces a well-structured reporting service with clear and concise functions. The use of a Map for storage and the implementation of generate, clear, eventStart, and eventStop functions are appropriate for the service's purpose.Main points for improvement:
- Add a safety check in the
eventStart()function for undefined event IDs.- Enhance error handling in the
eventStop()function, particularly for null start times.- Use type assertions in duration calculations to ensure type safety.
These minor adjustments will further improve the robustness and reliability of the service. Great job on the implementation!
apps/client/src/common/utils/dateConfig.ts (1)
22-27: Improved granularity for sub-hour delaysThe changes enhance the function's output by providing more precise timing information for delays less than an hour. This is a good improvement that offers users more detailed information.
However, to improve readability and maintainability, consider refactoring the nested conditions:
} else if (absMillis < MILLIS_PER_HOUR) { - if (absMillis % MILLIS_PER_MINUTE === 0) { - return `${isNegative ? ahead : delayed}${formatFromMillis(absMillis, 'm')} min`; - } else { - return `${isNegative ? ahead : delayed}${formatFromMillis(absMillis, 'mm:ss')}`; - } + const format = absMillis % MILLIS_PER_MINUTE === 0 ? 'm' : 'mm:ss'; + const unit = format === 'm' ? ' min' : ''; + return `${isNegative ? ahead : delayed}${formatFromMillis(absMillis, format)}${unit}`; }This refactoring reduces duplication and makes the logic more concise.
Additionally, ensure that unit tests are updated or added to cover this new logic, especially for edge cases around the hour boundary and for values that are not multiples of minutes.
apps/client/src/common/api/constants.ts (1)
10-10: Verify usage of the new REPORT constantTo ensure the new
REPORTconstant is being used correctly throughout the codebase:Please review the results of this script to ensure:
- The
REPORTconstant is being imported correctly where needed.- It's used consistently as a key in Tanstack store operations.
- There are no naming conflicts or inconsistencies with its usage.
If any issues are found, please address them to maintain consistency across the codebase.
apps/server/src/api-data/rundown/rundown.router.ts (2)
5-5: LGTM: Import statement updated correctlyThe
getReportfunction has been properly imported from the controller file, which is consistent with the new route addition.
34-35: New route added correctly, but clarification neededThe new GET route for '/report' has been added correctly. However, there are a few points to consider:
- The comment indicates that this route is not used in the Ontime frontend. Could you clarify the intended consumers of this endpoint?
- Unlike some other routes in this file, there's no validation middleware applied to this route. Is there any input validation required for the report endpoint?
To ensure consistency and proper implementation, let's verify the
getReportfunction in the controller:apps/server/src/services/report-service/__test__/reportService.test.ts (2)
13-49: Well-structured test setup and teardownThe test setup and teardown process is comprehensive and follows best practices:
- Using fake timers for controlled time manipulation
- Detailed mock state initialization
- Clearing report data before each test
- Proper teardown process
This setup ensures consistent and isolated test environments for each test case.
51-73: Comprehensive test cases for basic functionalityThe test cases cover the main functionalities of the report service:
- Clearing the report data
- Starting an event
- Stopping an event and calculating duration
The tests are well-structured and use appropriate assertions to verify the expected behavior.
apps/client/src/common/hooks-query/useRundown.ts (2)
3-3: LGTM: New imports are consistent with the added functionality.The new imports for
OntimeReport,REPORTconstant, andfetchReportfunction are appropriate for the newuseReporthook. The naming conventions are consistent with the existing code.Also applies to: 6-7
6-6: Verify uniqueness of the REPORT constant.The addition of the
REPORTconstant seems appropriate for the newuseReporthook. However, it's important to ensure that this constant doesn't conflict with any existing constants or variables in the codebase.Let's verify the uniqueness of the
REPORTconstant:If these searches return results, please review them to ensure there are no conflicts or unintended overrides.
apps/client/src/common/utils/__tests__/dateConfig.test.js (3)
37-38: Approved: Consistent format change for negative durations.The modification for negative durations maintains consistency with the positive duration format change, improving overall coherence in the output format.
42-43: Approved: Consistent test case for negative durations greater than one hour.This addition maintains symmetry in testing between positive and negative durations for periods exceeding one hour. The expected output format ('-01:17:10') is consistent with its positive counterpart.
34-43: Overall assessment: Improved test coverage and refined output format.The changes in this file enhance the test suite for the
millisToDelayString()function by:
- Simplifying the output format for durations less than one hour.
- Adding test cases for durations greater than one hour.
- Maintaining consistency between positive and negative duration representations.
These modifications improve the robustness of the test suite and ensure more comprehensive coverage of various time durations.
apps/client/src/common/api/rundown.ts (2)
2-2: LGTM: Import statement updated correctlyThe import statement has been appropriately updated to include
OntimeReportfrom the 'ontime-types' module. This change aligns with the newfetchReportfunction that returns this type.
2-2: Summary: New report fetching capability addedThe changes introduce a new
fetchReportfunction to retrieve an ontime report. This addition enhances the API capabilities of the rundown module, allowing clients to fetch report data. The implementation is consistent with the existing code style and uses appropriate typing.Key points:
- The import statement is correctly updated to include the new
OntimeReporttype.- The new
fetchReportfunction follows the established patterns in the file.These changes appear to be part of implementing the rundown reporter feature mentioned in the PR objectives. They extend the functionality of the rundown API in a clean and consistent manner.
Also applies to: 88-94
apps/client/src/features/cuesheet/CuesheetWrapper.tsx (4)
13-13: LGTM: New hook import for report functionalityThe addition of the
useReporthook import aligns with the PR objective of introducing a new "rundown reporter" feature. This change suggests that the component will now have access to report data, enhancing its functionality.
27-27: LGTM: Proper usage of useReport hookThe
useReporthook is correctly implemented to fetch report data. The destructuring and renaming of thedataproperty toreportfollows React hooks best practices, making the report data available for use within the component.
Line range hint
1-114: Summary: Successful integration of new reporting featureThe changes in this file effectively integrate the new "rundown reporter" feature into the
CuesheetWrappercomponent. The addition of theuseReporthook and its incorporation into thecolumnscalculation demonstrate a well-structured approach to enhancing the component's functionality.Key points:
- The
useReporthook is properly imported and used.- The
columnscalculation now considers the report data, potentially allowing for dynamic column structures based on the report.- The changes follow React best practices and maintain the overall structure of the component.
These modifications successfully lay the groundwork for the new reporting feature while maintaining the existing functionality of the
CuesheetWrappercomponent.
32-32: LGTM: Updated useMemo hook to include report dataThe
useMemohook forcolumnshas been correctly updated to include thereportas both an argument tomakeCuesheetColumnsand as a dependency. This change ensures that the column structure is recalculated when the report data changes, which is consistent with the new reporting feature.To ensure the
makeCuesheetColumnsfunction correctly handles the newreportparameter, please run the following script:apps/client/src/features/rundown/event-block/EventBlock.module.scss (3)
12-12: LGTM: Grid layout updated correctlyThe grid-template-areas have been updated to include the new 'over' area, which aligns with the addition of the
.overUnderSectionclass. This change enhances the layout structure of the event block.
66-66: LGTM: Consistent opacity handling for.overUnderSectionThe
.overUnderSectionhas been correctly added to the list of selectors for opacity change in past events. This ensures consistent styling behavior for the new section.
Line range hint
1-251: Overall, the changes improve the event block styling and layoutThe modifications to
EventBlock.module.scsssuccessfully introduce the new.overUnderSectionand update the grid layout to accommodate it. These changes enhance the event block component's structure and visual presentation. The styling is consistent with the existing code, and the use of SCSS variables maintains the file's maintainability.To complete this update:
- Address the TODO comments for color properties in the
.overUnderSection.- Consider adding comments explaining the purpose of the new
.overUnderSectionfor better code documentation.Great job on improving the event block styling!
apps/server/src/api-data/rundown/rundown.controller.ts (2)
4-4: LGTM: Import statement addition is correct.The addition of
OntimeReportto the import statement is consistent with its usage in the newgetReportfunction.
Line range hint
1-205: Overall, the changes effectively introduce the new reporting feature.The additions to this file successfully implement a new endpoint for generating reports. The changes are consistent with the existing code style and structure. The new functionality is well-integrated into the controller.
To further improve the code:
- Consider using named imports for the report service.
- Add error handling to the
getReportfunction.- Ensure that appropriate tests are added for this new functionality.
These changes enhance the controller's capabilities while maintaining its overall structure and readability.
apps/client/src/features/rundown/RundownEntry.tsx (1)
Line range hint
1-187: Overall, the changes look good. Consider the following suggestions for improvement:
- Add documentation for the
overUnderproperty in theRundownEntryPropsinterface.- Clarify the source of the
MaybeNumbertype.- If applicable, implement the usage of
overUnderwithin theRundownEntrycomponent logic.- Ensure that the
EventBlockcomponent is properly updated to handle the newoverUnderprop.These improvements will enhance code readability and maintainability.
apps/client/src/features/rundown/event-block/EventBlockInner.tsx (3)
12-12: LGTM: Import statement updated correctlyThe addition of
MaybeNumberto the import statement is consistent with its usage in the component props and is correctly placed with other related types.
48-48: LGTM: Interface updated with new propertyThe addition of the optional
overUnderproperty of typeMaybeNumberto theEventBlockInnerPropsinterface is consistent with the component's new functionality and is correctly implemented.
70-70: LGTM: Prop destructuring updated correctlyThe
overUnderproperty has been correctly added to the destructured props, maintaining consistency with the interface change and its usage in the component.apps/client/src/features/rundown/event-block/EventBlock.tsx (1)
87-87: LGTM! VerifyEventBlockInnercomponent usage.The
overUnderprop is correctly destructured from props and passed to theEventBlockInnercomponent. This implementation is consistent with how other props are handled in this component.To ensure the
overUnderprop is correctly utilized, please verify its usage in theEventBlockInnercomponent. Run the following script to check its implementation:Also applies to: 274-274
apps/client/src/features/rundown/Rundown.tsx (4)
10-10: LGTM: Import statement updated correctlyThe addition of
OntimeReportto the import statement is consistent with the changes in the component props and usage. This change is necessary for the new functionality and doesn't introduce any issues.
44-44: LGTM: Rundown function signature updated correctlyThe function signature has been properly updated to include the new
reportprop, which is consistent with the changes to the RundownProps interface. This change allows the component to receive and use thereportprop as intended.
Line range hint
1-344: Overall assessment: Changes implement new reporting feature correctlyThe changes in this file consistently implement the new
reportprop and its usage within the Rundown component. The new functionality to display timing information (overUnder) for events has been added correctly. The modifications are well-implemented and don't introduce any obvious issues.Here's a summary of the changes:
- Added
OntimeReportto the import statement.- Updated the
RundownPropsinterface to include thereportprop.- Modified the
Rundownfunction signature to accept the newreportprop.- Introduced an
overUndervariable to store timing information from the report.- Passed the
overUndervalue as a prop to theRundownEntrycomponent.These changes effectively integrate the new reporting feature into the existing Rundown component structure.
331-331: LGTM: overUnder prop added to RundownEntryThe
overUndervalue is correctly passed as a prop to the RundownEntry component, completing the flow of the report data from the Rundown component to its child RundownEntry component.Ensure that the RundownEntry component is updated to accept and utilize the new
overUnderprop. Run the following script to verify:Verification successful
Verification Successful:
overUnderProp Properly Implemented in RundownEntryThe
overUnderprop is correctly defined in theRundownEntryPropsinterface and successfully passed to theRundownEntrycomponent inRundown.tsx.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that RundownEntry component accepts the overUnder prop # Test: Search for the RundownEntry component definition and its props rg --type typescript "interface RundownEntryProps" -A 20 "apps/client/src/features/rundown" # Test: Search for usage of overUnder prop within RundownEntry component rg --type typescript "const RundownEntry.*?overUnder" -A 10 "apps/client/src/features/rundown"Length of output: 256
Script:
#!/bin/bash # Description: Verify that RundownEntry component accepts the overUnder prop # Test: Search for the RundownEntry component definition and its props rg --glob '*.tsx' 'interface RundownEntryProps' -A 20 apps/client/src/features/rundown # Test: Search for usage of overUnder prop within RundownEntry component rg --glob '*.tsx' 'const RundownEntry.*?overUnder' -A 10 apps/client/src/features/rundownLength of output: 1780
Script:
#!/bin/bash # Description: Further verify the usage of the overUnder prop within the RundownEntry component # Test: Search for any usage of the overUnder prop within RundownEntry.tsx rg --glob '*.tsx' 'overUnder' apps/client/src/features/rundown/RundownEntry.tsx # Test: Search for destructured overUnder in the RundownEntry component rg --glob '*.tsx' 'overUnder' -A 5 apps/client/src/features/rundown/RundownEntry.tsx # Test: Search for the overUnder prop being passed to any child components within RundownEntry rg --glob '*.tsx' 'overUnder={overUnder}' apps/client/src/features/rundown/RundownEntry.tsxLength of output: 1051
Script:
#!/bin/bash # Description: Verify the usage of the overUnder prop within the RundownEntry component with corrected regex # Test: Search for the exact usage of overUnder prop assignment with escaped braces rg --glob '*.tsx' 'overUnder=\{overUnder\}' apps/client/src/features/rundown/RundownEntry.tsx # Test: Alternatively, search for lines where overUnder is assigned to check its usage rg --glob '*.tsx' 'overUnder=' apps/client/src/features/rundown/RundownEntry.tsxLength of output: 236
apps/server/src/stores/runtimeState.ts (6)
29-30: LGTM: New import for report service added.The import statement for the report service has been correctly added, following the existing import style in the file.
176-178: LGTM: Event stop reporting added before state clear.The addition of
report.eventStop(runtimeState)before clearing the state in theloadfunction ensures that the current state is reported before any modifications occur. This enhances the tracking of event states within the runtime management logic.
356-356: LGTM: Event stop reporting added after state update.The addition of
report.eventStop(runtimeState)at the end of theupdateLoadedfunction ensures that the updated state is reported after all modifications. This enhances the tracking of event states within the runtime management logic.
414-414: LGTM: Event start reporting added after state initialization.The addition of
report.eventStart(runtimeState)at the end of thestartfunction ensures that the event start is reported after all state updates. This enhances the tracking of event states within the runtime management logic.
434-436: LGTM: Event stop reporting added before state clear in stop function.The addition of
report.eventStop(runtimeState)before clearing the state in thestopfunction ensures that the current state is reported before any modifications occur. The comment above the call provides a clear explanation of its purpose, enhancing code readability.
Line range hint
1-693: Summary: Successful integration of report service for enhanced event tracking.The changes made to this file consistently integrate the report service to enhance event state tracking. The additions are well-placed at key points in the runtime management logic (load, updateLoaded, start, and stop functions). This implementation will provide better insights into the lifecycle of events within the application.
Some suggestions for further improvement:
- Consider adding error handling for the report service calls to ensure robustness.
- It might be beneficial to add some logging or comments explaining the specific data being reported at each point.
Overall, the changes are well-implemented and improve the functionality of the runtime state management.
apps/client/src/features/cuesheet/cuesheetCols.tsx (3)
109-110: Confirm the update to thedurationcolumn's cell rendererThe cell renderer for the
durationcolumn has been changed toMakeTimer. Verify that this change maintains the intended functionality and displays the duration correctly.
4-4: Verify the correct import ofOntimeReportThe
OntimeReporttype is imported from'ontime-types'. Ensure that this type is correctly exported from the'ontime-types'module and that it matches the expected structure.Run the following script to confirm that
OntimeReportis exported:
66-66: Verify that all calls tomakeCuesheetColumnsare updatedThe function
makeCuesheetColumnsnow accepts an additionalreportparameter. Ensure that all instances where this function is called are updated to pass the new parameter to prevent runtime errors.Run the following script to find all usages of
makeCuesheetColumnsand check for proper arguments:Verification successful
All calls to
makeCuesheetColumnsare updated to include thereportparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to 'makeCuesheetColumns' include the 'report' parameter. # Test: Search for calls to 'makeCuesheetColumns'. Expect: Function calls with two arguments. rg --type typescript 'makeCuesheetColumns\(' -A 1Length of output: 86
Script:
#!/bin/bash # Description: Verify all calls to 'makeCuesheetColumns' include the 'report' parameter by searching in TypeScript files. # Search for calls to 'makeCuesheetColumns' in .ts and .tsx files and include one line of context after each match. rg 'makeCuesheetColumns\(' --glob '*.ts' --glob '*.tsx' -A 1Length of output: 647
apps/server/src/services/report-service/__test__/reportService.test.ts
Outdated
Show resolved
Hide resolved
| export async function getReport(_req: Request, res: Response<OntimeReport>) { | ||
| res.json(report.generate()); | ||
| } |
There was a problem hiding this comment.
Add error handling to improve robustness.
The getReport function is implemented correctly and follows the pattern of other controller functions in the file. However, it lacks error handling, which could lead to unhandled exceptions if report.generate() fails.
Consider adding error handling to make the function more robust:
export async function getReport(_req: Request, res: Response<OntimeReport | ErrorResponse>) {
try {
const reportData = report.generate();
res.json(reportData);
} catch (error) {
const message = getErrorMessage(error);
res.status(500).json({ message });
}
}This change ensures that any errors during report generation are caught and returned as a proper error response.
| {overUnder && ( | ||
| <div className={style.overUnderSection}> | ||
| {/* TODO: better tooltip */} | ||
| <Tooltip label={`Event was palayd with and ${overUnder > 0 ? 'overrun' : 'underrun'}`} {...tooltipProps}> | ||
| <span className={overUnder > 0 ? style.over : style.under}>{millisToDelayString(overUnder)}</span> | ||
| </Tooltip> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
New overUnder feature implemented, but needs minor improvements
The new feature for displaying overrun/underrun information is well-implemented overall. However, there are a few points to address:
- There's a typo in the tooltip label: "palayd" should be "played".
- The TODO comment suggests that the tooltip needs improvement. Please address this and remove the comment once done.
- Consider adding a brief comment explaining the
overUnderlogic for better code maintainability.
Here's a suggested improvement:
- {/* TODO: better tooltip */}
- <Tooltip label={`Event was palayd with and ${overUnder > 0 ? 'overrun' : 'underrun'}`} {...tooltipProps}>
+ {/* overUnder > 0 indicates an overrun, < 0 indicates an underrun */}
+ <Tooltip label={`Event was played with an ${overUnder > 0 ? 'overrun' : 'underrun'}`} {...tooltipProps}>
<span className={overUnder > 0 ? style.over : style.under}>{millisToDelayString(overUnder)}</span>
</Tooltip>Consider enhancing the tooltip to provide more context, such as including the actual duration alongside the over/underrun time.
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.
| {overUnder && ( | |
| <div className={style.overUnderSection}> | |
| {/* TODO: better tooltip */} | |
| <Tooltip label={`Event was palayd with and ${overUnder > 0 ? 'overrun' : 'underrun'}`} {...tooltipProps}> | |
| <span className={overUnder > 0 ? style.over : style.under}>{millisToDelayString(overUnder)}</span> | |
| </Tooltip> | |
| </div> | |
| )} | |
| {overUnder && ( | |
| <div className={style.overUnderSection}> | |
| {/* overUnder > 0 indicates an overrun, < 0 indicates an underrun */} | |
| <Tooltip label={`Event was played with an ${overUnder > 0 ? 'overrun' : 'underrun'}`} {...tooltipProps}> | |
| <span className={overUnder > 0 ? style.over : style.under}>{millisToDelayString(overUnder)}</span> | |
| </Tooltip> | |
| </div> | |
| )} |
| accessorKey: 'report', | ||
| id: 'report', | ||
| header: 'Report', | ||
| cell: (row) => MakeReport(report[row.row.original.id]?.overUnder), |
There was a problem hiding this comment.
Potential redundancy in accessing row.row.original.id
In the report column definition, the cell renderer accesses row.row.original.id. This may be redundant, as row.original.id should suffice. Verify that the correct property is being accessed to prevent potential issues.
Apply this change if necessary:
{
accessorKey: 'report',
id: 'report',
header: 'Report',
- cell: (row) => MakeReport(report[row.row.original.id]?.overUnder),
+ cell: (row) => MakeReport(report[row.original.id]?.overUnder),
size: 75,
},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.
| accessorKey: 'report', | |
| id: 'report', | |
| header: 'Report', | |
| cell: (row) => MakeReport(report[row.row.original.id]?.overUnder), | |
| accessorKey: 'report', | |
| id: 'report', | |
| header: 'Report', | |
| cell: (row) => MakeReport(report[row.original.id]?.overUnder), |
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (2)
apps/client/src/features/rundown/rundown-header/RundownMenu.tsx (1)
Line range hint
1-46: Overall, the changes look good with room for a small improvementThe implementation of the new "Clear report" functionality is well-done and consistent with the existing UI. The new imports, hook usage, and button implementation are all correct and align well with the feature's purpose.
To further improve the user experience and maintain consistency across the application, consider implementing error handling for the
clearReportAllfunction. This could involve using a try-catch block and displaying an error message to the user if the operation fails. For example:const handleClearReport = async () => { try { await clearReportAll(); // Optionally, show a success message } catch (error) { // Display an error message to the user console.error('Failed to clear report:', error); // You might want to use a toast notification or other UI element to show this error } }; // Then use handleClearReport instead of clearReportAll in the button onClickThis addition would make the application more robust and provide better feedback to the user in case of any issues during the report clearing process.
apps/client/src/common/api/rundown.ts (1)
104-107: LGTM with a minor suggestion: Add JSDoc comment todeleteReportAllThe
deleteReportAllfunction is well-implemented:
- It follows the established pattern for delete functions in this file.
- It correctly uses axios for the HTTP DELETE request.
- The return type is properly specified as
Promise<OntimeReport>.- The function is correctly exported for use in other parts of the application.
However, to maintain consistency with other functions in this file, consider adding a JSDoc comment.
Consider adding a JSDoc comment to the
deleteReportAllfunction for consistency:/** * HTTP request to delete all reports */ export async function deleteReportAll(): Promise<OntimeReport> { const res = await axios.delete(`${rundownPath}/report`); return res.data; }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- apps/client/src/common/api/rundown.ts (2 hunks)
- apps/client/src/common/hooks-query/useRundown.ts (2 hunks)
- apps/client/src/features/rundown/Rundown.tsx (5 hunks)
- apps/client/src/features/rundown/RundownEntry.tsx (6 hunks)
- apps/client/src/features/rundown/event-block/EventBlock.module.scss (3 hunks)
- apps/client/src/features/rundown/event-block/EventBlock.tsx (6 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (4 hunks)
- apps/client/src/features/rundown/rundown-header/RundownMenu.tsx (2 hunks)
- apps/server/src/api-data/rundown/rundown.controller.ts (3 hunks)
- apps/server/src/api-data/rundown/rundown.router.ts (2 hunks)
- apps/server/src/services/report-service/ReportService.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- apps/client/src/features/rundown/Rundown.tsx
- apps/client/src/features/rundown/event-block/EventBlock.module.scss
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx
- apps/server/src/services/report-service/ReportService.ts
Additional comments not posted (26)
apps/server/src/api-data/rundown/rundown.router.ts (2)
4-6: LGTM: Import statements are correctly updated.The new functions
clearReportandgetReportare properly imported from the rundown controller. This change is consistent with the existing code structure.
36-37: LGTM: New DELETE routes for report added.The new DELETE routes for clearing reports are correctly implemented. However, consider the following suggestions:
- Consider if any validation or middleware (e.g., authentication) should be applied to these routes.
- It would be helpful to add a comment explaining the difference between the two DELETE routes and when to use each one.
To ensure consistency with other routes, let's check if similar DELETE routes use any middleware:
#!/bin/bash # Description: Check if other DELETE routes use middleware rg --type typescript "router\.delete\(.*,.*,.*\)" apps/server/src/api-data/apps/client/src/features/rundown/rundown-header/RundownMenu.tsx (2)
12-12: LGTM: New icon import added correctlyThe
IoReceipticon import is correctly placed with other icon imports and is used in the new "Clear report" button.
16-16: LGTM: New hook import and usage added correctlyThe
useReporthook is imported correctly, and theclearReportAllfunction is properly extracted from it. This aligns well with the new "Clear report" button's functionality.Also applies to: 23-23
apps/client/src/common/api/rundown.ts (4)
2-2: LGTM: Import statement updated correctlyThe addition of
OntimeReportto the import statement is consistent with the new functions that use this type.
88-94: LGTM:fetchReportfunction implemented correctlyThe
fetchReportfunction is well-implemented:
- It follows the established pattern for fetch functions in this file.
- It correctly uses axios for the HTTP GET request.
- The return type is properly specified as
Promise<OntimeReport>.- The function is correctly exported for use in other parts of the application.
96-102: LGTM:deleteReportIdfunction implemented correctlyThe
deleteReportIdfunction is well-implemented:
- It follows the established pattern for delete functions in this file.
- It correctly uses axios for the HTTP DELETE request.
- The return type is properly specified as
Promise<OntimeReport>.- The function is correctly exported for use in other parts of the application.
- The
eventIdparameter is correctly used in the URL construction.
2-2: Summary: Excellent addition of report management functionsThe changes to this file successfully introduce report management capabilities to the API client:
- The import statement is updated to include the necessary
OntimeReporttype.- Three new functions are added:
fetchReport,deleteReportId, anddeleteReportAll.- These functions follow the established patterns in the file for API requests.
- The new functions enable fetching and deleting reports, both individually and in bulk.
These additions enhance the functionality of the rundown management system, allowing for more comprehensive report handling. The implementation is clean, consistent, and well-typed.
Also applies to: 88-107
apps/client/src/common/hooks-query/useRundown.ts (3)
1-8: LGTM: Import statements are well-organizedThe new imports for the report functionality have been added correctly, and the existing imports remain unchanged. The organization of imports is logical and follows good practices.
Line range hint
10-41: Unchanged code segmentThis section of the code, including the
useRundownanduseFlatRundownfunctions, remains unchanged from the previous version.
45-53: LGTM: Consistent and well-configured useQuery implementationThe
useQueryimplementation for fetching report data is consistent with theuseRundownfunction and well-configured. The use ofplaceholderData, retry logic, andrefetchIntervalis appropriate for ensuring data availability and freshness.apps/server/src/api-data/rundown/rundown.controller.ts (2)
4-4: Consider using named imports for better maintainability.The addition of
OntimeReportimport is correct and aligns with the new functions. However, the suggestion from the previous review to use named imports for the report module is still applicable.Consider refactoring the import statements:
import { OntimeReport } from 'ontime-types'; // ... other imports ... import { generate, clear } from '../../services/report-service/ReportService.js';This change would make it explicit which functions from the ReportService are being used in this file and improve code maintainability.
Also applies to: 32-32
203-205: Add error handling to improve robustness.The
getReportfunction is implemented correctly and follows the pattern of other controller functions in the file. However, it lacks error handling, which could lead to unhandled exceptions ifreport.generate()fails.Consider adding error handling to make the function more robust:
export async function getReport(_req: Request, res: Response<OntimeReport | ErrorResponse>) { try { const reportData = report.generate(); res.json(reportData); } catch (error) { const message = getErrorMessage(error); res.status(500).json({ message }); } }This change ensures that any errors during report generation are caught and returned as a proper error response.
apps/client/src/features/rundown/RundownEntry.tsx (6)
6-6: LGTM: New import for report functionality.The addition of the
useReportimport is consistent with the new report-related features being implemented.
26-27: LGTM: New action added to EventItemActions.The addition of the 'clear-report' action to
EventItemActionsis consistent with the new report-clearing functionality being implemented.
58-58: LGTM: Destructuring of new overUnder prop.The destructuring of the
overUnderprop is consistent with its addition to theRundownEntryPropsinterface and follows the existing pattern in the component.
149-156: LGTM: New case added for clearing reports.The 'clear-report' case in the
actionHandlerfunction is well-implemented:
- It handles both single and multiple event selections.
- It correctly uses the
clearReportIdfunction from theuseReporthook.- The implementation is consistent with the new action added to
EventItemActions.
189-189: LGTM: New overUnder prop passed to EventBlock.The
overUnderprop is correctly passed to theEventBlockcomponent, consistent with its addition to theRundownEntryPropsinterface.Please verify the
EventBlockcomponent implementation:#!/bin/bash # Description: Verify EventBlock component implementation # Test: Search for EventBlock component definition and its props interface rg --type typescript -A 10 'interface.*EventBlockProps|function EventBlock'
42-42: LGTM: New property added to RundownEntryProps.The addition of the
overUnderproperty is consistent with the changes described in the AI-generated summary.Please verify the
MaybeNumbertype definition:Verification successful
Verified:
MaybeNumbertype is properly defined.The
MaybeNumbertype is correctly defined inpackages/types/src/utils/utils.type.tsasnumber | null, ensuring type safety for theoverUnderproperty.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify MaybeNumber type definition # Test: Search for MaybeNumber type definition in TypeScript files rg --type typescript 'type\s+MaybeNumber'Length of output: 112
Script:
#!/bin/bash # Description: Verify MaybeNumber type definition by targeting .ts and .tsx files # Test: Search for MaybeNumber type definition in TypeScript files with explicit extensions rg --glob '*.ts' --glob '*.tsx' 'type\s+MaybeNumber'Length of output: 134
apps/client/src/features/rundown/event-block/EventBlock.tsx (7)
8-8: LGTM: New icon import for IoReceiptThe import for the
IoReceipticon is correctly placed with other icon imports and is likely used for the new "Clear report" menu item.
88-88: LGTM: Props destructuring updatedThe
overUnderproperty is correctly added to the props destructuring, maintaining consistency with the existing code style.
118-118: LGTM: New "Clear report" context menu item addedThe new "Clear report" menu item is correctly implemented:
- Placed at the end of the menu options
- Uses the
IoReceipticon- Calls the action handler with 'clear-report' action
147-147: LGTM: "Clear report" context menu item added for single selectionThe "Clear report" menu item is correctly implemented for single event selection:
- Placed at the end of the menu options
- Uses the
IoReceipticon- Calls the action handler with 'clear-report' action, consistent with the multiple selection menu
Line range hint
1-280: Overall: Well-implemented changes for new reporting featureThe changes in this file are consistent and well-implemented. Key points:
- New "Clear report" functionality added to both single and multiple selection context menus.
- New
overUnderprop added toEventBlockPropsand passed through the component hierarchy.- Proper import of the
IoReceipticon for the new menu items.The code maintains good consistency with the existing style and structure. However, some clarifications and verifications have been requested regarding the purpose of the
overUnderprop and its implementation in child components.
50-50: LGTM: New optional property added to EventBlockPropsThe
overUnderproperty is correctly added as an optionalMaybeNumbertype to theEventBlockPropsinterface.Could you please clarify the purpose of the
overUnderproperty? Its name suggests it might be related to timing or scoring, but additional context would be helpful for future maintainers.Verification successful
Verified:
overUnderProperty Usage ConfirmedThe
overUnderproperty is consistently used across the codebase, and its addition to theEventBlockPropsinterface aligns with existing implementations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of overUnder property rg --type typescript 'overUnder'Length of output: 67
Script:
#!/bin/bash # Search for usage of overUnder property in .ts and .tsx files rg 'overUnder' --glob '*.ts' --glob '*.tsx'Length of output: 2631
277-277: LGTM: overUnder prop passed to EventBlockInnerThe
overUnderprop is correctly passed to the EventBlockInner component.Please ensure that the EventBlockInner component has been updated to handle this new prop. Run the following script to verify:
| router.get('/report', getReport); | ||
| router.delete('/report/:eventId', clearReport); | ||
| router.delete('/report', clearReport); | ||
|
|
There was a problem hiding this comment.
Overall, the new report routes are well-implemented.
The additions enhance the API's capabilities for report management. However, consider the following suggestions for improvement:
-
For better organization, consider grouping related routes together. You might want to move the report routes next to other similar GET or DELETE operations.
-
The comment "not used in Ontime frontend" appears for some existing routes but not for the new report routes. Consider adding a similar comment for the new routes if applicable, or remove these comments if they're no longer relevant.
-
To improve code maintainability, consider extracting the report routes into a separate router (e.g.,
reportRouter) if you expect the report functionality to grow significantly in the future.
| @@ -30,6 +32,10 @@ | |||
|
|
|||
| export const router = express.Router(); | |||
|
|
|||
| router.get('/report', getReport); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Add Middleware to the New GET /report Route
The new /report GET route does not include any middleware, unlike existing GET routes that have validation or other middleware. Consider adding appropriate middleware (e.g., authentication) to maintain consistency and security.
- Location:
apps/server/src/api-data/rundown/rundown.router.ts:35
Analysis chain
LGTM: New GET route for report added.
The new route for fetching reports is correctly implemented. However, consider if any validation or middleware (e.g., authentication) should be applied to this route.
To ensure consistency with other routes, let's check if similar GET routes use any middleware:
Action Required: Correct the file type used in the shell script.
The previous script used an unrecognized file type typescript with rg. Please update the file type to ts to properly search for GET routes using middleware:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other GET routes use middleware
rg --type typescript "router\.get\(.*,.*,.*\)" apps/server/src/api-data/
Length of output: 112
Script:
#!/bin/bash
# Description: Check if other GET routes use middleware
rg --type ts "router\.get\(.*,.*,.*\)" apps/server/src/api-data/
Length of output: 373
| <Button | ||
| size='sm' | ||
| variant='ontime-outlined' | ||
| leftIcon={<IoReceipt />} | ||
| onClick={() => clearReportAll()} | ||
| color='#FA5656' | ||
| isDisabled={appMode === 'run'} | ||
| > | ||
| Clear report | ||
| </Button> |
There was a problem hiding this comment.
LGTM: New "Clear report" button implemented correctly
The new "Clear report" button is well-implemented with appropriate styling, icon, and functionality. The button is correctly disabled in 'run' mode, which is a good safety measure.
Consider adding a confirmation dialog for the "Clear report" action, similar to the existing "Clear rundown" button. This would prevent accidental data loss, especially since the button color suggests a destructive action.
Here's a suggested implementation:
const { isOpen: isReportClearOpen, onOpen: onReportClearOpen, onClose: onReportClearClose } = useDisclosure();
// ... existing code ...
<Button
size='sm'
variant='ontime-outlined'
leftIcon={<IoReceipt />}
onClick={onReportClearOpen}
color='#FA5656'
isDisabled={appMode === 'run'}
>
Clear report
</Button>
{/* ... existing AlertDialog for Clear rundown ... */}
<AlertDialog
variant='ontime'
isOpen={isReportClearOpen}
leastDestructiveRef={cancelRef}
onClose={onReportClearClose}
>
<AlertDialogOverlay>
<AlertDialogContent>
<AlertDialogHeader fontSize='lg' fontWeight='bold'>
Clear report
</AlertDialogHeader>
<AlertDialogBody>
Are you sure you want to clear the report? This action cannot be undone.
</AlertDialogBody>
<AlertDialogFooter>
<Button ref={cancelRef} onClick={onReportClearClose} variant='ontime-ghosted-white'>
Cancel
</Button>
<Button
colorScheme='red'
onClick={() => {
clearReportAll();
onReportClearClose();
}}
ml={4}
>
Clear report
</Button>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialogOverlay>
</AlertDialog>This addition would provide a consistent user experience with the existing "Clear rundown" functionality and add an extra layer of protection against accidental data loss.
| export function useReport() { | ||
| const queryClient = useQueryClient(); | ||
| const { data, status, isError, refetch, isFetching } = useQuery<OntimeReport>({ | ||
| queryKey: REPORT, | ||
| queryFn: fetchReport, | ||
| placeholderData: (previousData, _previousQuery) => previousData, | ||
| retry: 5, | ||
| retryDelay: (attempt) => attempt * 2500, | ||
| refetchInterval: queryRefetchIntervalSlow, | ||
| networkMode: 'always', | ||
| }); | ||
|
|
||
| const clearMutation = useMutation({ | ||
| mutationFn: deleteReportId, | ||
| onSettled: () => { | ||
| queryClient.invalidateQueries({ queryKey: REPORT }); | ||
| }, | ||
| networkMode: 'always', | ||
| }); | ||
|
|
||
| const clearMutationAll = useMutation({ | ||
| mutationFn: deleteReportAll, | ||
| onSettled: () => { | ||
| queryClient.invalidateQueries({ queryKey: REPORT }); | ||
| }, | ||
| networkMode: 'always', | ||
| }); | ||
|
|
||
| const clearReportId = useCallback( | ||
| async (id: string) => { | ||
| clearMutation.mutateAsync(id); | ||
| }, | ||
| [clearMutation], | ||
| ); | ||
|
|
||
| const clearReportAll = useCallback(async () => { | ||
| clearMutationAll.mutateAsync(); | ||
| }, [clearMutationAll]); | ||
|
|
||
| return { data: data ?? {}, status, isError, refetch, isFetching, clearReportId, clearReportAll }; | ||
| } |
There was a problem hiding this comment.
LGTM: Well-structured useReport function
The useReport function is well-implemented, following patterns similar to useRundown for consistency. It properly utilizes React Query hooks for data fetching and mutations, and uses useCallback for memoization of utility functions.
Consider adding error handling for the clearReportId and clearReportAll functions. For example:
const clearReportId = useCallback(
async (id: string) => {
try {
await clearMutation.mutateAsync(id);
} catch (error) {
console.error('Failed to clear report:', error);
// Optionally, you could add a way to notify the user of the error
}
},
[clearMutation]
);
const clearReportAll = useCallback(async () => {
try {
await clearMutationAll.mutateAsync();
} catch (error) {
console.error('Failed to clear all reports:', error);
// Optionally, you could add a way to notify the user of the error
}
}, [clearMutationAll]);This would provide better error handling and debugging capabilities.
| const clearMutation = useMutation({ | ||
| mutationFn: deleteReportId, | ||
| onSettled: () => { | ||
| queryClient.invalidateQueries({ queryKey: REPORT }); | ||
| }, | ||
| networkMode: 'always', | ||
| }); | ||
|
|
||
| const clearMutationAll = useMutation({ | ||
| mutationFn: deleteReportAll, | ||
| onSettled: () => { | ||
| queryClient.invalidateQueries({ queryKey: REPORT }); | ||
| }, | ||
| networkMode: 'always', | ||
| }); |
There was a problem hiding this comment.
LGTM: Well-implemented mutations with proper query invalidation
The clearMutation and clearMutationAll mutations are correctly implemented using useMutation. The invalidation of the REPORT query after mutations is a good practice to ensure data consistency.
Consider adding error handling to the onSettled callbacks. For example:
const clearMutation = useMutation({
mutationFn: deleteReportId,
onSettled: (data, error) => {
if (error) {
console.error('Error clearing report:', error);
// Optionally, you could add a way to notify the user of the error
}
queryClient.invalidateQueries({ queryKey: REPORT });
},
networkMode: 'always',
});This would provide better error handling and debugging capabilities for the mutations.
| export async function clearReport(req: Request, res: Response<OntimeReport>) { | ||
| report.clear(req.params?.eventId); | ||
| res.json(report.generate()); | ||
| } |
There was a problem hiding this comment.
Improve error handling and type safety in clearReport function.
The clearReport function correctly clears and regenerates the report. However, there are a few improvements that can be made:
- Add error handling to catch potential exceptions from
report.clear()orreport.generate(). - Consider using a more specific type for
req.paramsto ensureeventIdis always present.
Here's a suggested implementation with these improvements:
export async function clearReport(
req: Request<{ eventId: string }>,
res: Response<OntimeReport | ErrorResponse>
) {
try {
report.clear(req.params.eventId);
const newReport = report.generate();
res.json(newReport);
} catch (error) {
const message = getErrorMessage(error);
res.status(500).json({ message });
}
}This implementation:
- Adds error handling.
- Uses a typed
Requestto ensureeventIdis always present. - Removes the need for optional chaining.
- Stores the result of
report.generate()in a variable for clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/server/src/services/report-service/ReportService.ts (1)
1-50: Summary of ReportService.ts reviewThe ReportService implementation provides a solid foundation for managing event timing data. The use of a Map for storing reports and the clear separation of concerns across the four main functions (generate, clear, eventStart, and eventStop) are commendable.
However, there are several areas for improvement:
- Enhance error handling and input validation, particularly in the eventStart and eventStop functions.
- Address static analysis issues by refactoring the development checks.
- Implement the suggested safety checks and type assertions to improve robustness.
- Consider the TODO item for handling pauses in the report calculation.
- Investigate and fix the failing unit tests related to the eventStop function.
Addressing these points will significantly improve the reliability and maintainability of the ReportService.
🧰 Tools
GitHub Check: unit-test
[failure] 38-38: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > normal playback cycle
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:97:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > runtime offset
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:172:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from load
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:283:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from play
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:292:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll continue with offset > no gaps
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:308:7Biome
[error] 25-25: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 25-25: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 36-36: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 36-36: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/server/src/services/report-service/ReportService.ts (1 hunks)
🧰 Additional context used
GitHub Check: unit-test
apps/server/src/services/report-service/ReportService.ts
[failure] 38-38: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > normal playback cycle
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:97:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > runtime offset
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:172:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from load
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:283:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from play
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:292:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll continue with offset > no gaps
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:308:7
Biome
apps/server/src/services/report-service/ReportService.ts
[error] 25-25: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 25-25: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 36-36: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 36-36: 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 (3)
apps/server/src/services/report-service/ReportService.ts (3)
1-9: LGTM: Imports and global variables are well-structured.The imports, report Map, and blankReportData constant are appropriately defined for the report service functionality. The use of a Map for storing reports provides efficient key-based access, and the blankReportData constant ensures consistent initialization of report entries.
11-13: LGTM: generate() function is concise and effective.The
generate()function efficiently converts the report Map to an OntimeReport object usingObject.fromEntries(). This implementation is both concise and effective.
15-21: LGTM: clear() function provides flexible clearing options.The
clear(id?: string)function effectively handles both cases of clearing a specific entry or all entries based on the optional 'id' parameter. This implementation provides flexibility in managing the report data.
| export function eventStart(state: RuntimeState) { | ||
| // eslint-disable-next-line no-unused-labels -- dev code path | ||
| DEV: { | ||
| if (state.eventNow === null) { | ||
| throw new Error('report.eventStart: called without eventNow present'); | ||
| } | ||
| } | ||
|
|
||
| report.set(state.eventNow.id, { ...blankReportData, startAt: state.timer.startedAt }); | ||
| } |
There was a problem hiding this comment.
Enhance robustness and address static analysis issues in eventStart().
- Consider implementing a safety check for
state.eventNow.idas suggested in a previous review:
export function eventStart(state: RuntimeState) {
+ if (state.eventNow?.id === undefined) {
+ console.warn('eventStart called with undefined event ID');
+ return;
+ }
report.set(state.eventNow.id, { ...blankReportData, startAt: state.timer.startedAt });
}- Remove the
DEVlabel and use a different approach for development checks to address static analysis errors:
export function eventStart(state: RuntimeState) {
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: {
- if (state.eventNow === null) {
- throw new Error('report.eventStart: called without eventNow present');
- }
- }
+ if (process.env.NODE_ENV === 'development' && state.eventNow === null) {
+ throw new Error('report.eventStart: called without eventNow present');
+ }
report.set(state.eventNow.id, { ...blankReportData, startAt: state.timer.startedAt });
}These changes will improve the function's robustness and resolve static analysis issues.
📝 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.
| export function eventStart(state: RuntimeState) { | |
| // eslint-disable-next-line no-unused-labels -- dev code path | |
| DEV: { | |
| if (state.eventNow === null) { | |
| throw new Error('report.eventStart: called without eventNow present'); | |
| } | |
| } | |
| report.set(state.eventNow.id, { ...blankReportData, startAt: state.timer.startedAt }); | |
| } | |
| export function eventStart(state: RuntimeState) { | |
| if (state.eventNow?.id === undefined) { | |
| console.warn('eventStart called with undefined event ID'); | |
| return; | |
| } | |
| if (process.env.NODE_ENV === 'development' && state.eventNow === null) { | |
| throw new Error('report.eventStart: called without eventNow present'); | |
| } | |
| report.set(state.eventNow.id, { ...blankReportData, startAt: state.timer.startedAt }); | |
| } |
🧰 Tools
Biome
[error] 25-25: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 25-25: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
| export function eventStop(state: RuntimeState) { | ||
| // eslint-disable-next-line no-unused-labels -- dev code path | ||
| DEV: { | ||
| if (state.eventNow === null) { | ||
| throw new Error('report.eventStop: called without eventNow present'); | ||
| } | ||
| } | ||
|
|
||
| //TODO: How should pauses be calculated into the report? | ||
| const prevReport = report.get(state.eventNow.id); | ||
| if (prevReport !== undefined && prevReport.startAt !== null) { | ||
| prevReport.endAt = state.clock; | ||
| const expectedDuration = state.eventNow.duration; | ||
| const actualDuration = prevReport.endAt - prevReport.startAt; | ||
| prevReport.overUnder = actualDuration - expectedDuration; | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance robustness, address static analysis issues, and fix failing tests in eventStop().
- Implement additional safety checks and type assertions:
export function eventStop(state: RuntimeState) {
if (state.eventNow?.id === undefined) {
return;
}
const prevReport = report.get(state.eventNow.id);
- if (prevReport !== undefined && prevReport.startAt !== null) {
+ if (prevReport && prevReport.startAt !== null && prevReport.startAt !== undefined) {
prevReport.endAt = state.clock;
const expectedDuration = state.eventNow.duration;
- const actualDuration = prevReport.endAt - prevReport.startAt;
+ const actualDuration = (prevReport.endAt as number) - (prevReport.startAt as number);
prevReport.overUnder = actualDuration - expectedDuration;
+ } else {
+ console.warn('eventStop called for an event with no valid start time');
}
}- Remove the
DEVlabel and use a different approach for development checks:
export function eventStop(state: RuntimeState) {
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: {
- if (state.eventNow === null) {
- throw new Error('report.eventStop: called without eventNow present');
- }
- }
+ if (process.env.NODE_ENV === 'development' && state.eventNow === null) {
+ console.warn('report.eventStop: called without eventNow present');
+ return;
+ }
// Rest of the function...
}- The TODO comment about handling pauses in the report calculation should be addressed:
Would you like me to create a GitHub issue to track the task of implementing pause handling in the report calculation?
- To address the failing unit tests, consider making the function more robust when
eventNowis not present:
export function eventStop(state: RuntimeState) {
+ if (!state.eventNow) {
+ console.warn('eventStop called without an active event');
+ return;
+ }
// Rest of the function...
}These changes will improve the function's robustness, resolve static analysis issues, and potentially fix the failing unit tests.
📝 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.
| export function eventStop(state: RuntimeState) { | |
| // eslint-disable-next-line no-unused-labels -- dev code path | |
| DEV: { | |
| if (state.eventNow === null) { | |
| throw new Error('report.eventStop: called without eventNow present'); | |
| } | |
| } | |
| //TODO: How should pauses be calculated into the report? | |
| const prevReport = report.get(state.eventNow.id); | |
| if (prevReport !== undefined && prevReport.startAt !== null) { | |
| prevReport.endAt = state.clock; | |
| const expectedDuration = state.eventNow.duration; | |
| const actualDuration = prevReport.endAt - prevReport.startAt; | |
| prevReport.overUnder = actualDuration - expectedDuration; | |
| } | |
| } | |
| export function eventStop(state: RuntimeState) { | |
| if (process.env.NODE_ENV === 'development' && state.eventNow === null) { | |
| console.warn('report.eventStop: called without eventNow present'); | |
| return; | |
| } | |
| if (!state.eventNow) { | |
| console.warn('eventStop called without an active event'); | |
| return; | |
| } | |
| if (state.eventNow?.id === undefined) { | |
| return; | |
| } | |
| //TODO: How should pauses be calculated into the report? | |
| const prevReport = report.get(state.eventNow.id); | |
| if (prevReport && prevReport.startAt !== null && prevReport.startAt !== undefined) { | |
| prevReport.endAt = state.clock; | |
| const expectedDuration = state.eventNow.duration; | |
| const actualDuration = (prevReport.endAt as number) - (prevReport.startAt as number); | |
| prevReport.overUnder = actualDuration - expectedDuration; | |
| } else { | |
| console.warn('eventStop called for an event with no valid start time'); | |
| } | |
| } |
🧰 Tools
GitHub Check: unit-test
[failure] 38-38: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > normal playback cycle
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:97:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > runtime offset
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:172:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from load
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:283:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from play
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:292:7
[failure] 38-38: src/stores/tests/runtimeState.test.ts > roll mode > roll continue with offset > no gaps
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:38:13
❯ Module.load src/stores/runtimeState.ts:177:10
❯ src/stores/tests/runtimeState.test.ts:308:7
Biome
[error] 36-36: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 36-36: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/server/src/services/report-service/ReportService.ts (3)
31-31: Fix typo in commentThere's a typo in the comment: "potentaly" should be "potentially".
Apply this diff to correct the typo:
- // this clears out potentaly old data + // this clears out potentially old data
40-40: Address the TODO: Incorporate pauses into report calculationThe TODO comment indicates uncertainty about handling pauses in the report calculations. Properly accounting for pauses is crucial for accurate reporting.
I can assist in implementing a solution to handle pauses within the report calculations. Would you like me to help with this or open a new GitHub issue to track this task?
44-44: Correct typos in commentsIn the comments on lines 44 and 49, "the is no start" should be "there is no start".
Apply these diffs to fix the typos:
- //we can't stop it if the is no start + //we can't stop it if there is no start- //we can't stop it if the is no start, so better to clear out bad data + //we can't stop it if there is no start, so better to clear out bad dataAlso applies to: 49-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/server/src/services/report-service/ReportService.ts (1 hunks)
🧰 Additional context used
🪛 Biome
apps/server/src/services/report-service/ReportService.ts
[error] 25-25: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 25-25: 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 (1)
apps/server/src/services/report-service/ReportService.ts (1)
32-32: Add a safety check forstate.eventNow.idThe code assumes that
state.eventNow.idis always defined. To improve robustness, consider adding a safety check before accessing it.
| // eslint-disable-next-line no-unused-labels -- dev code path | ||
| DEV: { | ||
| if (state.eventNow === null) { | ||
| throw new Error('report.eventStart: called without eventNow present'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove the 'DEV' label to fix static analysis errors
The use of the label DEV is causing static analysis errors because labels are only permitted for loops. To resolve these errors, remove the DEV label and adjust the code accordingly.
Apply this diff to fix the issue:
export function eventStart(state: RuntimeState) {
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: {
if (state.eventNow === null) {
throw new Error('report.eventStart: called without eventNow present');
}
- }
// this clears out potentially old data
report.set(state.eventNow.id, { ...blankReportData, startAt: state.timer.startedAt });
}Alternatively, wrap the check in a development-only condition:
export function eventStart(state: RuntimeState) {
+ if (process.env.NODE_ENV === 'development' && state.eventNow === null) {
+ throw new Error('report.eventStart: called without eventNow present');
+ }
// this clears out potentially old data
report.set(state.eventNow.id, { ...blankReportData, startAt: state.timer.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.
| // eslint-disable-next-line no-unused-labels -- dev code path | |
| DEV: { | |
| if (state.eventNow === null) { | |
| throw new Error('report.eventStart: called without eventNow present'); | |
| } | |
| } | |
| export function eventStart(state: RuntimeState) { | |
| if (state.eventNow === null) { | |
| throw new Error('report.eventStart: called without eventNow present'); | |
| } | |
| // this clears out potentially old data | |
| report.set(state.eventNow.id, { ...blankReportData, startAt: state.timer.startedAt }); | |
| } |
🧰 Tools
🪛 Biome
[error] 25-25: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 25-25: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
alex-Arc
force-pushed
the
reporter
branch
2 times, most recently
from
63a1300
to
34ca4e3
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
apps/server/src/api-data/report/report.router.ts (1)
6-6: Consider implementing input validation.The TODO comment suggests that input validation might be needed. It's crucial to implement proper validation for API endpoints to ensure data integrity and security. Consider using a validation middleware like express-validator or joi to validate incoming requests.
Would you like assistance in implementing input validation for these routes?
apps/server/src/services/report-service/__test__/reportService.test.ts (1)
13-49: Consider extracting mockState initialization to a separate function.The test setup is well-structured. To improve readability and maintainability, consider extracting the mockState initialization into a separate function.
Here's a suggested refactor:
function initializeMockState(): RuntimeState { return { clock: 666, eventNow: null, publicEventNow: null, eventNext: null, publicEventNext: null, runtime: { selectedEventIndex: null, numEvents: 0, }, timer: { addedTime: 0, current: null, duration: null, elapsed: null, expectedFinish: null, finishedAt: null, playback: Playback.Stop, secondaryTimer: null, startedAt: null, }, _timer: { pausedAt: null, }, } as RuntimeState; } beforeEach(() => { vi.useFakeTimers(); vi.setSystemTime(0); mockState = initializeMockState(); clear(); });This change would make the beforeEach block more concise and the mockState initialization more reusable.
apps/server/src/stores/runtimeState.ts (3)
174-176: LGTM: Event stop reporting added before state clear.Good addition of event stop reporting before clearing the state. This ensures the final state of the event is captured.
Consider adding a brief comment explaining why the event stop is reported here, for better code readability:
+ // Report the event stop before clearing state to capture final event state report.eventStop(runtimeState);
415-415: LGTM: Event start reporting added at the end of start function.Good addition of event start reporting after all state updates. This ensures the reported start state includes all changes.
For consistency with the
loadfunction, consider adding a brief comment:+ // Report the event start after all state updates report.eventStart(runtimeState);
Line range hint
1-724: Overall assessment: Reporting functionality well integrated.The changes introduce event reporting at key points in the runtime state management (load, start, and stop functions). These additions enhance the system's ability to track and log runtime events without interfering with existing functionality. The placement of the reporting calls is consistent and logical, ensuring that the correct state is captured at each point.
A few minor suggestions for improvement:
- Consider adding brief comments explaining the reason for event reporting in the
startfunction, similar to those inloadandstopfunctions, for consistency.- Ensure that the
ReportServicehandles potential errors or edge cases, such as when theruntimeStatemight be in an unexpected state.As the codebase grows, consider implementing a more robust logging or telemetry system that can handle different levels of event reporting (e.g., debug, info, warning, error) to provide more granular control over what gets reported and when.
🛑 Comments failed to post (6)
apps/server/src/api-data/report/report.controller.ts (3)
5-7:
⚠️ Potential issueAdd error handling to
getReportfunction.While the function looks good overall, it's important to handle potential errors that might occur during report generation.
Consider wrapping the report generation in a try-catch block:
export async function getReport(_req: Request, res: Response<OntimeReport>) { - res.json(report.generate()); + try { + const generatedReport = report.generate(); + res.json(generatedReport); + } catch (error) { + console.error('Error generating report:', error); + res.status(500).json({ error: 'Failed to generate report' }); + } }This will prevent the server from crashing if an error occurs during report generation and provide a more informative response to the client.
📝 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.export async function getReport(_req: Request, res: Response<OntimeReport>) { try { const generatedReport = report.generate(); res.json(generatedReport); } catch (error) { console.error('Error generating report:', error); res.status(500).json({ error: 'Failed to generate report' }); } }
1-3: 🛠️ Refactor suggestion
Consider using named imports for ReportService.
While the current imports are functional, it's generally better to use named imports for clarity and to avoid potential naming conflicts. This also helps with tree-shaking in production builds.
Consider replacing the wildcard import with named imports:
-import * as report from '../../services/report-service/ReportService.js'; +import { generate, clear } from '../../services/report-service/ReportService.js';Then update the function calls in the controller accordingly.
Committable suggestion was skipped due to low confidence.
9-12:
⚠️ Potential issueImprove error handling and add parameter validation in
clearReportfunction.The function needs improvement in error handling and parameter validation.
Consider implementing the following changes:
- Add parameter validation for
eventId.- Implement error handling for both
report.clear()andreport.generate().- Use a more specific HTTP status code for successful clearing of a report.
Here's a suggested implementation:
export async function clearReport(req: Request, res: Response<OntimeReport>) { - report.clear(req.params?.eventId); - res.json(report.generate()); + const eventId = req.params?.eventId; + if (!eventId) { + return res.status(400).json({ error: 'Event ID is required' }); + } + + try { + await report.clear(eventId); + const generatedReport = report.generate(); + res.status(204).json(generatedReport); + } catch (error) { + console.error('Error clearing or generating report:', error); + res.status(500).json({ error: 'Failed to clear or generate report' }); + } }This implementation validates the
eventId, handles potential errors, and uses HTTP status code 204 (No Content) to indicate successful clearing of the report.📝 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.export async function clearReport(req: Request, res: Response<OntimeReport>) { const eventId = req.params?.eventId; if (!eventId) { return res.status(400).json({ error: 'Event ID is required' }); } try { await report.clear(eventId); const generatedReport = report.generate(); res.status(204).json(generatedReport); } catch (error) { console.error('Error clearing or generating report:', error); res.status(500).json({ error: 'Failed to clear or generate report' }); } }apps/server/src/services/report-service/ReportService.ts (3)
43-44:
⚠️ Potential issueCorrect typo in comment
In the comment on line 44, "if the is no start" should be "if there is no start".
Apply this diff to fix the typo:
- //we can't stop it if the is no start + //we can't stop it if there is no start📝 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 (prevReport === undefined) { //we can't stop it if there is no start
31-31:
⚠️ Potential issueFix typo in comment
There's a typo on line 31. The word "potentaly" should be "potentially".
Apply this diff to correct the typo:
- // this clears out potentaly old data + // this clears out potentially old data📝 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.// this clears out potentially old data
48-49:
⚠️ Potential issueCorrect typo in comment
In the comment on line 49, "if the is no start" should be "if there is no start".
Apply this diff to fix the typo:
- //we can't stop it if the is no start, so better to clear out bad data + //we can't stop it if there is no start, so better to clear out bad data📝 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 (prevReport.startAt === null) { //we can't stop it if there is no start, so better to clear out bad data
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apps/server/src/services/report-service/ReportService.ts (1)
1-62: Overall implementation is solid with room for improvementThe ReportService implementation is well-structured and provides the necessary functionality for managing event timing reports. Here's a summary of the strengths and areas for improvement:
Strengths:
- Efficient use of Map for report storage.
- Clear separation of concerns with distinct functions for different operations.
- Flexible clear() function allowing selective or complete data clearing.
Areas for improvement:
- Error handling in eventStart() and eventStop() functions (addressed in previous comments).
- Implementation of pause handling in report calculations.
- Potential for additional type safety measures, especially in duration calculations.
Consider the following for future enhancements:
- Implement a more robust error handling strategy, possibly using a custom error class.
- Add input validation for critical parameters like state.eventNow.id and state.timer.startedAt.
- Consider implementing a locking mechanism if this service might be used in a multi-threaded environment.
🧰 Tools
🪛 GitHub Check: unit-test
[failure] 40-40: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > normal playback cycle
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:97:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > runtime offset
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:172:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from load
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:283:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from play
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:292:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll continue with offset > no gaps
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:308:7🪛 Biome
[error] 26-26: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 26-26: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 29-29: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
[error] 39-39: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 39-39: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 42-42: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
apps/server/src/stores/runtimeState.ts (2)
174-176: LGTM: Event stop reporting added before state clear.The addition of
report.eventStop(runtimeState)before clearing the state ensures that the event stop is logged with the current state data. This change aligns well with the goal of integrating event reporting functionality.Consider adding a comment explaining why the event stop is reported before clearing the state, to improve code readability and maintainability.
+ // Report the event stop before clearing the state to ensure accurate logging report.eventStop(runtimeState);
Line range hint
29-437: Overall: Event reporting successfully integrated.The changes in this file consistently integrate event reporting functionality across different functions (load, start, and stop). The placement of the report calls ensures that events are logged with accurate state data.
Consider adding error handling for the report calls to ensure robustness:
+ try { report.eventStop(runtimeState); + } catch (error) { + console.error('Failed to report event stop:', error); + }This pattern could be applied to all report calls throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/types/src/definitions/core/Report.type.tsis excluded by none and included by nonepackages/types/src/index.tsis excluded by none and included by none
📒 Files selected for processing (5)
- apps/server/src/api-data/report/report.controller.ts (1 hunks)
- apps/server/src/api-data/report/report.router.ts (1 hunks)
- apps/server/src/services/report-service/ReportService.ts (1 hunks)
- apps/server/src/services/report-service/test/reportService.test.ts (1 hunks)
- apps/server/src/stores/runtimeState.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/server/src/api-data/report/report.controller.ts
- apps/server/src/api-data/report/report.router.ts
- apps/server/src/services/report-service/test/reportService.test.ts
🧰 Additional context used
🪛 GitHub Check: unit-test
apps/server/src/services/report-service/ReportService.ts
[failure] 40-40: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > normal playback cycle
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:97:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > runtime offset
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:172:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from load
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:283:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from play
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:292:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll continue with offset > no gaps
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:308:7
🪛 Biome
apps/server/src/services/report-service/ReportService.ts
[error] 26-26: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 26-26: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 29-29: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
[error] 39-39: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 39-39: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 42-42: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (5)
apps/server/src/services/report-service/ReportService.ts (2)
1-9: LGTM: Appropriate imports and data structureThe import statements and the report data structure are well-defined. Using a Map for report storage is an efficient choice, and the
blankReportDataconstant ensures consistency across report entries.
11-21: LGTM: Well-implemented generate() and clear() functionsThe
generate()andclear()functions are concise and effective. Theclear()function's flexibility to delete a specific report or all reports is a good design choice.apps/server/src/stores/runtimeState.ts (3)
29-30: LGTM: New import for report service added.The import statement for the report module is correctly formatted and follows the module import syntax. The use of the .js extension in the import path is noted and might be related to the project's module resolution configuration.
415-415: LGTM: Event start reporting added at the end of start function.The addition of
report.eventStart(runtimeState)at the end of the start function ensures that all state updates are completed before logging the event start. This placement is appropriate and aligns well with the goal of integrating event reporting functionality.
435-437: LGTM: Event stop reporting added before state clear in stop function.The addition of
report.eventStop(runtimeState)before clearing the state in the stop function is consistent with the similar change in the load function. This ensures that the event stop is logged with the current state data before it's cleared. The comment explaining the reason for this placement enhances code readability and maintainability.
| export function eventStop(state: RuntimeState) { | ||
| if (state.eventNow === null) { | ||
| // eslint-disable-next-line no-unused-labels -- dev code path | ||
| DEV: { | ||
| throw new Error('report.eventStop: called without eventNow present'); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| //TODO: How should pauses be calculated into the report? | ||
| const prevReport = report.get(state.eventNow.id); | ||
|
|
||
| if (prevReport === undefined) { | ||
| //we can't stop it if the is no start | ||
| return; | ||
| } | ||
|
|
||
| if (prevReport.startAt === null) { | ||
| //we can't stop it if the is no start, so better to clear out bad data | ||
| report.delete(state.eventNow.id); | ||
| } | ||
|
|
||
| prevReport.endAt = state.clock; | ||
| const expectedDuration = state.eventNow.duration; | ||
| const actualDuration = prevReport.endAt - prevReport.startAt; | ||
| prevReport.overUnder = actualDuration - expectedDuration; | ||
| } |
There was a problem hiding this comment.
Improve error handling, remove DEV label, and enhance type safety
The eventStop() function needs several improvements:
- Remove the
DEVlabel to resolve static analysis errors. - Improve error handling to fix failing unit tests.
- Enhance type safety in duration calculations.
- Address the TODO comment about pause handling.
Apply this diff to improve the function:
export function eventStop(state: RuntimeState) {
if (state.eventNow === null) {
- // eslint-disable-next-line no-unused-labels -- dev code path
- DEV: {
- throw new Error('report.eventStop: called without eventNow present');
- }
- return;
+ console.warn('report.eventStop: called without eventNow present');
+ return;
}
- //TODO: How should pauses be calculated into the report?
+ // TODO: Implement pause handling in report calculation
const prevReport = report.get(state.eventNow.id);
if (prevReport === undefined || prevReport.startAt === null) {
report.delete(state.eventNow.id);
return;
}
prevReport.endAt = state.clock;
const expectedDuration = state.eventNow.duration;
- const actualDuration = prevReport.endAt - prevReport.startAt;
+ const actualDuration = (prevReport.endAt as number) - (prevReport.startAt as number);
prevReport.overUnder = actualDuration - expectedDuration;
}These changes address static analysis issues, fix failing unit tests, and improve type safety in calculations.
Would you like me to create a GitHub issue to track the task of implementing pause handling in the report calculation?
📝 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.
| export function eventStop(state: RuntimeState) { | |
| if (state.eventNow === null) { | |
| // eslint-disable-next-line no-unused-labels -- dev code path | |
| DEV: { | |
| throw new Error('report.eventStop: called without eventNow present'); | |
| } | |
| return; | |
| } | |
| //TODO: How should pauses be calculated into the report? | |
| const prevReport = report.get(state.eventNow.id); | |
| if (prevReport === undefined) { | |
| //we can't stop it if the is no start | |
| return; | |
| } | |
| if (prevReport.startAt === null) { | |
| //we can't stop it if the is no start, so better to clear out bad data | |
| report.delete(state.eventNow.id); | |
| } | |
| prevReport.endAt = state.clock; | |
| const expectedDuration = state.eventNow.duration; | |
| const actualDuration = prevReport.endAt - prevReport.startAt; | |
| prevReport.overUnder = actualDuration - expectedDuration; | |
| } | |
| export function eventStop(state: RuntimeState) { | |
| if (state.eventNow === null) { | |
| console.warn('report.eventStop: called without eventNow present'); | |
| return; | |
| } | |
| // TODO: Implement pause handling in report calculation | |
| const prevReport = report.get(state.eventNow.id); | |
| if (prevReport === undefined || prevReport.startAt === null) { | |
| report.delete(state.eventNow.id); | |
| return; | |
| } | |
| prevReport.endAt = state.clock; | |
| const expectedDuration = state.eventNow.duration; | |
| const actualDuration = (prevReport.endAt as number) - (prevReport.startAt as number); | |
| prevReport.overUnder = actualDuration - expectedDuration; | |
| } |
🧰 Tools
🪛 GitHub Check: unit-test
[failure] 40-40: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > normal playback cycle
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:97:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > mutation on runtimeState > playback operations > runtime offset
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:172:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from load
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:283:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll takover > from play
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:292:7
[failure] 40-40: src/stores/tests/runtimeState.test.ts > roll mode > roll continue with offset > no gaps
Error: report.eventStop: called without eventNow present
❯ Module.eventStop src/services/report-service/ReportService.ts:40:13
❯ Module.load src/stores/runtimeState.ts:175:10
❯ src/stores/tests/runtimeState.test.ts:308:7
🪛 Biome
[error] 39-39: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 39-39: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
[error] 42-42: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
alex-Arc
force-pushed
the
reporter
branch
2 times, most recently
from
1c9074b
to
1d62dee
Compare
Idea for the shape of a reporter
#1139