feat: timer preview #1208
feat: timer preview #1208
Conversation
WalkthroughThe changes involve significant updates across multiple files, focusing on enhancing timer management and message handling functionalities. Modifications include the introduction of new hooks for message previews, restructuring components for better organization, and refactoring the message service from a class-based to a functional approach. Additionally, new validation mechanisms for timer messages have been implemented, improving the overall robustness of the application. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Socket
participant Server
participant MessageService
Client->>Socket: sendMessage(message)
Socket->>Server: forwardMessage(message)
Server->>MessageService: processMessage(message)
MessageService-->>Server: updateState(newState)
Server-->>Socket: sendResponse(response)
Socket-->>Client: receiveResponse(response)
Assessment against linked issues
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
apps/server/tsconfig.jsonis excluded by none and included by nonepackages/types/src/definitions/runtime/MessageControl.type.tsis excluded by none and included by none
Files selected for processing (7)
- apps/client/src/common/hooks/useSocket.ts (3 hunks)
- apps/client/src/features/control/message/MessageControl.module.scss (1 hunks)
- apps/client/src/features/control/message/MessageControl.tsx (1 hunks)
- apps/server/src/api-integration/integration.controller.ts (2 hunks)
- apps/server/src/app.ts (2 hunks)
- apps/server/src/services/message-service/MessageService.ts (1 hunks)
- apps/server/src/services/message-service/messageUtils.ts (1 hunks)
Additional context used
Biome
apps/server/src/services/message-service/MessageService.ts
[error] 48-48: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 48-48: 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 (18)
apps/client/src/features/control/message/MessageControl.module.scss (3)
30-34: LGTM!The
.timerPreviewclass uses a grid layout with specified column ratios, allowing for a structured arrangement of child elements. The changes are consistent with the AI-generated summary.
36-41: LGTM!The
.previewclass centers its content both vertically and horizontally, improving visual focus. The background color set to$ui-blackenhances visual contrast. The changes are consistent with the AI-generated summary.
43-47: LGTM!The
.optionsclass uses a flexbox layout to stack its child elements vertically, promoting better spacing and organization. The defined gap between the child elements improves the spacing. The changes are consistent with the AI-generated summary.apps/server/src/services/message-service/MessageService.ts (3)
6-12: LGTM!The code changes are approved.
14-17: LGTM!The code changes are approved.
19-22: LGTM!The code changes are approved.
apps/server/src/services/message-service/messageUtils.ts (3)
33-33: LGTM!The code segment correctly handles the
secondaryproperty and uses thecoerceSecondaryfunction for validation and coercion.Verify the implementation of the
coerceSecondaryfunction in the file.
41-43: LGTM!The
assertSecondaryfunction correctly asserts the permitted values for thesecondaryproperty.
48-53: LGTM!The
coerceSecondaryfunction correctly ensures that thesecondaryvalue is one of the permitted values and throws an error for invalid values, providing a robust error handling mechanism.apps/client/src/features/control/message/MessageControl.tsx (3)
16-34: LGTM!The
TimerPreviewfunction is a great addition that improves the separation of concerns and makes the logic for handling timer states more explicit.
36-90: LGTM!The
TimerControlsPreviewfunction is a great addition that improves the separation of concerns and makes the logic for handling user interactions more explicit.
Line range hint
92-116: LGTM!The
MessageControlfunction has been refactored to include two new components:TimerPreviewandTimerControlsPreview, which improves the separation of concerns and makes the code more readable and maintainable.apps/client/src/common/hooks/useSocket.ts (3)
1-1: LGTM!The import statement change is approved. The addition of
TimerMessageis consistent with the AI-generated summary, which suggests that the file now handles new message types associated with timers.
35-35: LGTM!The state selection logic change is approved. Replacing
onAirwithphaseindicates a shift in focus towards the timer's current phase rather than its on-air status. This change is consistent with the AI-generated summary, which suggests a broader requirement for timer state management in the application.
46-46: LGTM!The addition of
timerSecondarymethod is approved. The method enhances the communication capabilities of the application by allowing for more granular control over timer states. It is consistent with the AI-generated summary, which indicates that the application now supports additional timer-related functionalities.apps/server/src/app.ts (2)
41-41: LGTM!The code change is approved.
213-213: Verify the impact of the change on theinitmethod.The updated code passes
eventStore.setdirectly to theinitmethod, which implies that the context will be determined by howinitis called. This change may affect the behavior of theinitmethod if it relies on the context ofthis, depending on its implementation.Run the following script to verify the implementation of the
initmethod:apps/server/src/api-integration/integration.controller.ts (1)
6-6: LGTM!The refactor from a default import to a namespace import for
messageServiceis approved.
cpvalente
force-pushed
the
timer-preview
branch
from
69e76ac
to
7bf62f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
apps/server/tsconfig.jsonis excluded by none and included by nonepackages/types/src/definitions/runtime/MessageControl.type.tsis excluded by none and included by none
Files selected for processing (18)
- apps/client/src/common/hooks/useSocket.ts (3 hunks)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx (1 hunks)
- apps/client/src/features/control/message/InputRow.tsx (2 hunks)
- apps/client/src/features/control/message/MessageControl.module.scss (1 hunks)
- apps/client/src/features/control/message/MessageControl.tsx (1 hunks)
- apps/client/src/features/control/message/MessageControlExport.jsx (1 hunks)
- apps/client/src/features/control/message/TimerPreview.tsx (1 hunks)
- apps/client/src/features/control/message/TimerViewControl.tsx (1 hunks)
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx (1 hunks)
- apps/client/src/features/editors/Editor.module.scss (2 hunks)
- apps/client/src/features/editors/EditorMixin.scss (2 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (3 hunks)
- apps/client/src/features/viewers/ViewWrapper.tsx (2 hunks)
- apps/client/src/features/viewers/timer/Timer.tsx (6 hunks)
- apps/server/src/api-integration/integration.controller.ts (2 hunks)
- apps/server/src/app.ts (2 hunks)
- apps/server/src/services/message-service/MessageService.ts (1 hunks)
- apps/server/src/services/message-service/messageUtils.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx
Files skipped from review as they are similar to previous changes (4)
- apps/client/src/features/control/message/MessageControl.module.scss
- apps/server/src/api-integration/integration.controller.ts
- apps/server/src/app.ts
- apps/server/src/services/message-service/messageUtils.ts
Additional context used
Biome
apps/server/src/services/message-service/MessageService.ts
[error] 48-48: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 48-48: 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 (15)
apps/client/src/features/editors/EditorMixin.scss (2)
5-7: Good use of CSS custom properties.The introduction of
--editor--panel__brat the root level promotes consistency and maintainability in styling. Well done!
30-30: Enhanced flexibility with CSS custom properties.Using
var(--editor--panel__br)for theborder-radiusin thepanelmixin is a great way to maintain consistency and ease future modifications.apps/client/src/features/control/message/MessageControlExport.jsx (1)
6-18: Good use of utility function for class management.The introduction and usage of the
cxfunction fromstyleUtilsto dynamically manage class names enhances the flexibility and maintainability of the component's styling. This is a good practice.apps/client/src/features/control/message/MessageControl.tsx (1)
4-28: Streamlined component structure and state management.The restructuring of the
MessageControlfunction to directly accesstimerandexternalproperties simplifies state management. The introduction of theTimerControlsPreviewcomponent enhances focus on essential functionalities. These changes improve readability and maintainability.apps/client/src/features/editors/Editor.module.scss (2)
43-43: Approved: Use of CSS variables for styling consistency.The update to use
var(--editor--panel__br)for theborder-radiusproperty in.playbackand.messagesclasses enhances styling consistency across the application.
66-71: Approved: Addition of.contentColumnLayoutfor enhanced layout management.The new
.contentColumnLayoutclass with flex properties and defined gap improves the structural organization and visual appeal of the content areas. Using$ui-whitefor text color ensures consistency with the theme.apps/server/src/services/message-service/MessageService.ts (2)
28-30: Approved: Use of throttled function ininit.The implementation of a throttled publish function in the
initmethod is a good practice to manage performance and reduce unnecessary load.
35-39: Approved: Simplified state retrieval ingetState.The direct return of the timer and external message states in the
getStatefunction simplifies the code and enhances readability.apps/client/src/features/control/message/TimerViewControl.tsx (1)
9-62: Approved: Well-implemented timer controls with best practices.The
TimerControlsPreviewfunction effectively uses React hooks and functional components to manage and display timer settings. The implementation is clean, and the use of conditional rendering and event handling is well-executed.apps/client/src/features/control/message/InputRow.tsx (1)
Line range hint
23-63: Simplified Component Logic by RemovingreadonlyPropertyThe removal of the
readonlyproperty simplifies the component's logic by always rendering theTooltipActionBtn, which enhances user interaction by allowing visibility toggling at all times. This change streamlines the component and removes unnecessary conditional checks.However, it's important to verify that this change does not affect other functionalities or components that might rely on the
readonlystate.Run the following script to verify the impact of this change on other components:
Verification successful
Simplified Component Logic by Removing
readonlyPropertyThe removal of the
readonlyproperty from theInputRowcomponent does not negatively impact other components, such asMessageControl, which uses thevisibleprop to manage visibility. This change aligns with the intended functionality and simplifies the component logic without affecting other parts of the codebase.
- The
MessageControlcomponent uses theInputRowcomponent with thevisibleprop, ensuring consistent behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if other components are affected by the removal of the `readonly` property. # Test: Search for the usage of `InputRow` component. Expect: No other components should rely on the `readonly` property. rg --type typescript -A 5 $'InputRow'Length of output: 71
Script:
#!/bin/bash # Description: Verify if other components are affected by the removal of the `readonly` property. # Test: Search for the usage of `InputRow` component. Expect: No other components should rely on the `readonly` property. rg --type ts -A 10 'InputRow'Length of output: 4596
apps/client/src/features/control/message/TimerPreview.tsx (1)
15-92: Robust Implementation ofTimerPreviewComponentThe
TimerPreviewcomponent is well-implemented, using hooks effectively to fetch and manage state, and employing conditional rendering to adapt the UI based on the timer's visibility, phase, and secondary messages. The dynamic styling based on the timer phase is a thoughtful touch that enhances the user experience.Ensure that all data dependencies are correctly managed and that the component gracefully handles any potential data fetching issues or delays.
Run the following script to verify the data dependencies:
Verification successful
Verification Successful: Data Dependencies in
TimerPreviewComponentThe
TimerPreviewcomponent's data dependencies are managed correctly. TheuseViewSettingshook includes error handling and retry logic, while theuseMessagePreviewhook is part of a state management system. This ensures that the component can handle data fetching states and potential errors gracefully. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the data dependencies for the `TimerPreview` component. # Test: Search for the usage of hooks in the component. Expect: All hooks should have proper error handling and fallbacks. rg --type typescript -A 5 $'useMessagePreview|useViewSettings'Length of output: 98
Script:
#!/bin/bash # Description: Verify the data dependencies for the `TimerPreview` component. # Test: Search for the usage of hooks in the component. Expect: All hooks should have proper error handling and fallbacks. rg --type ts -A 5 'useMessagePreview|useViewSettings'Length of output: 7209
apps/client/src/features/viewers/ViewWrapper.tsx (1)
Line range hint
71-99: Enhanced Data Handling with Addition ofauxtimer1The addition of the
auxtimer1property to the destructured object from theuseStore(runtimeStore)hook and its subsequent passing as a prop to the wrapped component enhances the component's functionality by allowing it to handle an additional timer state. This change reflects an improvement in the component's data handling capabilities.Ensure that the
auxtimer1property is correctly integrated and used by the wrapped component without causing any unintended side effects.Run the following script to verify the integration and usage of the
auxtimer1property:Verification successful
Successful Integration of
auxtimer1PropertyThe
auxtimer1property is well-integrated across the codebase, with its definition, initialization, and usage evident in multiple files. It is part of the application's state management and is used in real-time updates, confirming its proper integration and functionality.
- Definition:
auxtimer1is defined as aSimpleTimerStateinRuntimeStore.type.ts.- Initialization: Initialized with default values in
app.tsandruntime.ts.- Usage: Passed as a prop in
ViewWrapper.tsxand referenced in socket-related files (socket.ts,useSocket.ts).- Service Interaction: Used in
AuxTimerService.ts, indicating its role in the service layer.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and usage of the `auxtimer1` property. # Test: Search for the usage of `auxtimer1` in the wrapped components. Expect: Proper handling and usage of the property. rg --type typescript -A 5 $'auxtimer1'Length of output: 72
Script:
#!/bin/bash # Description: Verify the integration and usage of the `auxtimer1` property. # Test: Search for the usage of `auxtimer1` in the wrapped components. Expect: Proper handling and usage of the property. rg --type ts -A 5 $'auxtimer1'Length of output: 4065
apps/client/src/common/hooks/useSocket.ts (1)
57-61: Enhancements to message handling approved.The new methods
externalText,externalVisible, andtimerSecondaryin thesetMessageobject are correctly implemented and enhance the application's message handling capabilities, particularly in relation to timer management and external message functionalities.apps/client/src/features/viewers/timer/Timer.tsx (2)
Line range hint
64-211: Enhancements to theTimerfunction approved.The use of the
auxTimerproperty to conditionally render secondary content and the updated logic for external content visibility are correctly implemented, enhancing the component's functionality and aligning with the PR objectives.
51-51: Addition ofauxTimertoTimerPropsapproved.The new property
auxTimerenhances the component's functionality by allowing conditional rendering based on the timer's state. Please ensure that theSimpleTimerStatetype is fully compatible with the intended use cases.Run the following script to verify the type compatibility:
Verification successful
Compatibility of
SimpleTimerStatewithTimerComponent ConfirmedThe
SimpleTimerStatetype is well-defined and used consistently across the codebase, including in theSimpleTimerclass and related tests. This confirms its compatibility with theTimercomponent's requirements. The addition ofauxTimerto theTimerPropsinterface is approved and aligns with the intended functionality enhancements.
- Definition found in
packages/types/src/definitions/runtime/AuxTimer.type.ts- Utilized in
apps/server/src/classes/simple-timer/SimpleTimer.tsand related testsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the compatibility of `SimpleTimerState` type with its usage in the `Timer` component. # Test: Search for the type definition and its usage. Expect: Compatibility with the `Timer` component's requirements. rg --type typescript -A 5 $'SimpleTimerState'Length of output: 79
Script:
#!/bin/bash # Description: Search for the definition and usage of `SimpleTimerState` in TypeScript files. # Search for the definition of `SimpleTimerState` in `.ts` and `.tsx` files. rg 'SimpleTimerState' --glob '**/*.ts' --glob '**/*.tsx' -A 5Length of output: 13347
cpvalente
force-pushed
the
timer-preview
branch
from
7bf62f2
to
55ee7dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
apps/server/tsconfig.jsonis excluded by none and included by nonepackages/types/src/definitions/runtime/MessageControl.type.tsis excluded by none and included by none
Files selected for processing (18)
- apps/client/src/common/hooks/useSocket.ts (3 hunks)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx (1 hunks)
- apps/client/src/features/control/message/InputRow.tsx (2 hunks)
- apps/client/src/features/control/message/MessageControl.module.scss (1 hunks)
- apps/client/src/features/control/message/MessageControl.tsx (1 hunks)
- apps/client/src/features/control/message/MessageControlExport.jsx (1 hunks)
- apps/client/src/features/control/message/TimerPreview.tsx (1 hunks)
- apps/client/src/features/control/message/TimerViewControl.tsx (1 hunks)
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx (1 hunks)
- apps/client/src/features/editors/Editor.module.scss (2 hunks)
- apps/client/src/features/editors/EditorMixin.scss (2 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (3 hunks)
- apps/client/src/features/viewers/ViewWrapper.tsx (2 hunks)
- apps/client/src/features/viewers/timer/Timer.tsx (6 hunks)
- apps/server/src/api-integration/integration.controller.ts (2 hunks)
- apps/server/src/app.ts (2 hunks)
- apps/server/src/services/message-service/MessageService.ts (1 hunks)
- apps/server/src/services/message-service/messageUtils.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx
- apps/client/src/features/editors/EditorMixin.scss
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx
Files skipped from review as they are similar to previous changes (9)
- apps/client/src/common/hooks/useSocket.ts
- apps/client/src/features/control/message/InputRow.tsx
- apps/client/src/features/control/message/MessageControl.module.scss
- apps/client/src/features/control/message/MessageControlExport.jsx
- apps/client/src/features/control/message/TimerPreview.tsx
- apps/client/src/features/control/message/TimerViewControl.tsx
- apps/server/src/api-integration/integration.controller.ts
- apps/server/src/app.ts
- apps/server/src/services/message-service/messageUtils.ts
Additional context used
Biome
apps/server/src/services/message-service/MessageService.ts
[error] 48-48: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 48-48: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
Additional comments not posted (7)
apps/client/src/features/control/message/MessageControl.tsx (2)
11-11: Acknowledgment ofTimerControlsPreview.The function
TimerControlsPreviewis mentioned in the alterations but no specific changes are highlighted in the provided code. If internal changes were made, ensure they align with the overall objectives of the PR.
4-28: Refactor enhances clarity and maintainability.The restructuring of the
MessageControlfunction simplifies the handling of timer and external messages by using destructured properties directly. This change enhances the component's readability and maintainability by reducing complexity.Run the following script to verify the integration with the rest of the application:
Verification successful
Integration of
MessageControlis verified.The
MessageControlcomponent is properly integrated into the application, as evidenced by its usage in multiple files and its dynamic import usingReact.lazy. This confirms that the refactoring maintains the component's functionality within the application.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `MessageControl` with the rest of the application. # Test: Search for the usage of `MessageControl` in other components. Expect: Proper integration without issues. rg --type tsx -A 5 $'MessageControl'Length of output: 63
Script:
#!/bin/bash # Find all .tsx files and search for the usage of `MessageControl` within them. fd --extension tsx --exec rg 'MessageControl' {}Length of output: 697
apps/client/src/features/editors/Editor.module.scss (2)
43-43: Enhanced styling consistency.Updating the
border-radiusproperty to use a variable (var(--editor--panel__br)) enhances styling consistency and maintainability across the application.
66-71: New class enhances layout capabilities.The introduction of the
.contentColumnLayoutclass, applying a flexbox layout with column direction and defined spacing, enhances the layout capabilities and promotes a more structured and visually appealing design.Run the following script to verify the integration of
.contentColumnLayoutwith the rest of the application:Verification successful
Integration of
.contentColumnLayoutverified.The
.contentColumnLayoutclass is used in theMessageControlExport.jsxfile, confirming its integration into the application. No issues were found with its usage.
- File:
apps/client/src/features/control/message/MessageControlExport.jsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `.contentColumnLayout` with the rest of the application. # Test: Search for the usage of `.contentColumnLayout` in other components. Expect: Proper integration without issues. rg --type scss -A 5 $'.contentColumnLayout'Length of output: 71
Script:
#!/bin/bash # Description: Verify the integration of `.contentColumnLayout` with the rest of the application. # Test: Search for the usage of `.contentColumnLayout` in the codebase. rg '.contentColumnLayout'Length of output: 232
apps/server/src/services/message-service/MessageService.ts (2)
6-21: Initialization of default objects and state variables enhances consistency.The introduction of default objects for timer and external messages and their use in initializing state variables ensures a consistent starting state for the service. This change enhances the robustness and maintainability of the service.
28-30: Efficient management of updates with throttled publish function.The setup of a throttled publish function in the
initmethod ensures efficient management of updates, preventing potential performance issues due to rapid state changes.apps/client/src/features/viewers/timer/Timer.tsx (1)
51-51: Approve the addition ofauxTimerand verify its integration.The addition of
auxTimerto theTimerPropsinterface and its usage in the component are approved. The conditional rendering logic based onauxTimerappears to be correctly implemented. However, ensure thatauxTimeris properly integrated and does not introduce any side effects.Run the following script to verify the integration of
auxTimer:Also applies to: 64-64, 137-142
Verification successful
Integration of
auxTimeris verified and correct.The
auxTimeris well-integrated into theTimercomponent and other parts of the codebase. Its usage is consistent with the intended functionality, and it is covered by tests, indicating no side effects. The integration is successfully verified.
Timer.tsx: Declaration and usage in props and rendering logic.integration.controller.ts: Usage in service calls.208-auxtimer.spec.ts: End-to-end tests coveringauxTimerfunctionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `auxTimer` in the Timer component. # Test: Search for the usage of `auxTimer`. Expect: Proper integration without side effects. rg --type typescript -A 5 $'auxTimer'Length of output: 71
Script:
#!/bin/bash # Description: Verify the integration of `auxTimer` in the Timer component. # Test: Search for the usage of `auxTimer`. Expect: Proper integration without side effects. rg --type ts -A 5 $'auxTimer'Length of output: 7247
cpvalente
force-pushed
the
timer-preview
branch
from
55ee7dd
to
5bc8e9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
apps/client/src/features/control/message/TimerPreview.tsx (1)
69-69: Use strict equality operator for consistencyIt's best practice to use the strict equality operator
===in JavaScript/TypeScript to avoid unexpected type coercion. This ensures that both the value and type are compared.Apply this diff to update the equality check:
- const showColourOverride = main == 'Timer'; + const showColourOverride = main === 'Timer';
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/types/src/definitions/runtime/MessageControl.type.tsis excluded by none and included by none
Files selected for processing (18)
- apps/client/src/common/hooks/useSocket.ts (3 hunks)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx (1 hunks)
- apps/client/src/features/control/message/InputRow.tsx (3 hunks)
- apps/client/src/features/control/message/MessageControl.module.scss (1 hunks)
- apps/client/src/features/control/message/MessageControl.tsx (1 hunks)
- apps/client/src/features/control/message/MessageControlExport.jsx (1 hunks)
- apps/client/src/features/control/message/TimerPreview.tsx (1 hunks)
- apps/client/src/features/control/message/TimerViewControl.tsx (1 hunks)
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx (1 hunks)
- apps/client/src/features/editors/Editor.module.scss (2 hunks)
- apps/client/src/features/editors/EditorMixin.scss (2 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (2 hunks)
- apps/client/src/features/viewers/ViewWrapper.tsx (2 hunks)
- apps/client/src/features/viewers/timer/Timer.tsx (6 hunks)
- apps/server/src/api-integration/integration.controller.ts (2 hunks)
- apps/server/src/app.ts (2 hunks)
- apps/server/src/services/message-service/MessageService.ts (1 hunks)
- apps/server/src/services/message-service/messageUtils.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx
- apps/server/src/api-integration/integration.controller.ts
Additional context used
Biome
apps/server/src/services/message-service/MessageService.ts
[error] 48-48: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 48-48: 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 (28)
apps/client/src/features/editors/EditorMixin.scss (3)
4-7: Introduction of CSS variable for border-radius enhances maintainabilityUsing
--editor--panel__bras a CSS variable for the border-radius centralizes the styling and makes it easier to manage or update the border radius across the application.
9-15: Duplicate Comment: Consider reintroducing mixin usage for positioningA past review comment addressed the changes in these lines. Please refer to the previous feedback to avoid redundancy.
30-30: Consistent use of CSS variable for border-radiusUpdating the
border-radiusto use the new CSS variable--editor--panel__brensures consistency and makes future adjustments more manageable.apps/client/src/features/control/message/MessageControlExport.jsx (2)
6-6: Addition ofcxutility improves class name managementImporting
cxfromstyleUtilsenhances the way class names are composed, improving readability and maintainability.
13-14: Refactored className assignment enhances flexibilityUsing the
classesvariable withcxto combinestyle.contentandstyle.contentColumnLayoutallows for more dynamic class management and cleaner code.Also applies to: 18-18
apps/client/src/features/control/message/MessageControl.tsx (3)
4-4: Refactored to useTimerControlsPreviewcomponent for better modularityImporting and including
TimerControlsPreviewsimplifiesMessageControlby extracting timer preview logic into its own component, enhancing modularity and code organization.Also applies to: 10-11
13-18: UpdatedInputRowcomponents to reflect new timer and external message structureAdjusting the
InputRowcomponents to usetimerandexternalfromuseMessageControlaligns with the updated data structure and state management.Also applies to: 21-26
13-18: Ensure timer blink and blackout functionality is preservedThe buttons for toggling the timer's blink and blackout features have been removed from this component. Please verify that this functionality is now correctly implemented within the
TimerControlsPreviewcomponent or elsewhere to maintain existing features.apps/client/src/features/editors/Editor.module.scss (2)
43-43: Updated border-radius to use CSS variable for consistencyChanging the
border-radiustovar(--editor--panel__br)ensures consistent styling across components and makes global updates simpler.
66-71: Added.contentColumnLayoutclass for improved layout managementThe new
.contentColumnLayoutclass enhances layout flexibility by providing a standardized column layout with consistent spacing and styling.apps/client/src/features/control/message/MessageControl.module.scss (1)
1-75: Introduced new styles to enhance message control layout and responsivenessThe addition of multiple new classes and styling rules improves the visual structure and responsiveness of the message control component, enhancing user experience.
apps/client/src/features/control/message/TimerViewControl.tsx (1)
1-55: NewTimerControlsPreviewcomponent enhances timer management UIThe creation of
TimerControlsPreviewprovides a dedicated component for timer controls, improving code modularity and offering a clearer interface for managing timer features.apps/server/src/services/message-service/messageUtils.ts (1)
33-33: Validation of 'secondary' property added tovalidateTimerMessageGreat job enhancing the
validateTimerMessagefunction by adding validation for the 'secondary' property. This ensures that any incoming messages have a validated 'secondary' value, improving the robustness of message handling.apps/server/src/services/message-service/MessageService.ts (2)
28-30: Refactoring to a functionalinitimproves code clarityTransitioning from a class-based
initmethod to a functional approach simplifies the initialization logic and enhances readability.
35-40: Well-implementedgetStatefunction for state exposureThe new
getStatefunction cleanly exposes the internal message state, making it straightforward to retrieve the current state without class properties.apps/client/src/features/control/message/InputRow.tsx (1)
55-63: Verify consistency between 'visible' and 'isPublic' field namesThere appears to be a potential mismatch between the
visibleprop and the'isPublic'field used inactionHandler. Verify whether the field should be'visible'instead of'isPublic'to maintain consistency and prevent confusion.Run the following script to search for usages of
actionHandlerwith'isPublic'and'visible'fields:Verification successful
Usage of 'visible' and 'isPublic' is consistent and appropriate
After a thorough review of the codebase, I can confirm that the usage of 'visible' and 'isPublic' in the InputRow component is consistent and appropriate. Here's why:
- 'visible' is used as a prop for the component and determines the UI state (whether the eye icon is open or closed).
- 'isPublic' is used as the field name when updating the event data through the actionHandler.
This distinction is intentional and correct:
- 'visible' relates to the UI representation of the public/private state.
- 'isPublic' is the actual property name in the event data model, used consistently throughout the codebase for managing event visibility.
The actionHandler is correctly using 'isPublic' as the field name, which aligns with other parts of the codebase that deal with event data. This approach maintains a clear separation between UI state and data model properties.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences where 'actionHandler' updates 'isPublic' or 'visible' fields # Test: Find all instances where 'actionHandler' is called with field 'isPublic' or 'visible' rg "actionHandler\('update', \{ field: 'isPublic'" --type js rg "actionHandler\('update', \{ field: 'visible'" --type jsLength of output: 148
Script:
#!/bin/bash # Search for 'visible' and 'isPublic' in .tsx and .ts files echo "Searching for 'visible' and 'isPublic':" rg "visible|isPublic" --type ts --type tsx # Look for the definition and usage of 'actionHandler' echo -e "\nSearching for 'actionHandler' definition and usage:" rg "function actionHandler|const actionHandler|actionHandler\s*=" --type ts --type tsx # Search for 'TooltipActionBtn' component usage echo -e "\nSearching for 'TooltipActionBtn' usage:" rg "TooltipActionBtn" --type ts --type tsxLength of output: 587
Script:
#!/bin/bash # Find all .ts and .tsx files echo "TypeScript and TypeScript React files:" find . -type f \( -name "*.ts" -o -name "*.tsx" \) # Search for relevant terms in .ts and .tsx files echo -e "\nSearching for 'visible' and 'isPublic':" grep -rn --include=\*.{ts,tsx} -E "visible|isPublic" . echo -e "\nSearching for 'actionHandler':" grep -rn --include=\*.{ts,tsx} -E "function actionHandler|const actionHandler|actionHandler\s*=" . echo -e "\nSearching for 'TooltipActionBtn':" grep -rn --include=\*.{ts,tsx} "TooltipActionBtn" .Length of output: 52960
apps/client/src/features/rundown/event-block/EventBlockInner.tsx (1)
97-97: Simplification by removing unnecessaryTooltiparound "UP NEXT"Removing the
Tooltipwrapper around the "UP NEXT" label simplifies the component and reduces unnecessary complexity, improving performance.apps/client/src/common/hooks/useSocket.ts (3)
40-50: Approved: Addition ofuseMessagePreviewhookThe new
useMessagePreviewhook correctly extracts state properties related to message visibility, content, and timer phases. This enhances the application's messaging capabilities by providing a centralized and efficient way to access these properties.
59-60: Approved: Addition ofexternalTextandexternalVisiblesettersThe new setters
externalTextandexternalVisiblein thesetMessageobject are properly implemented. They enable more granular control over external message properties, improving state management for external messages.
63-63: Approved: Addition oftimerSecondarysetterThe addition of
timerSecondaryto thesetMessageobject appropriately allows control over thesecondaryproperty of timer messages. This enhances the flexibility of message state management.apps/client/src/features/viewers/timer/Timer.tsx (6)
51-51: Approved: Inclusion ofauxTimerinTimerPropsinterfaceAdding
auxTimer: SimpleTimerStateto theTimerPropsinterface allows the component to access auxiliary timer information. This is essential for implementing the new timer preview feature.
64-65: Approved: DestructuringauxTimerfrom propsDestructuring
auxTimerfrom the props ensures that the auxiliary timer state is accessible within theTimercomponent. This is necessary for utilizing the auxiliary timer in the component logic.
137-148: Approved: Implementation ofsecondaryContentfunctionThe
secondaryContentfunction correctly determines the content to display based on thepres.secondaryproperty. It prioritizes displaying the auxiliary timer whenpres.secondaryis'aux'and falls back to the external message when available.
166-166: Approved: AdjustingtimerFontSizebased onsecondaryContentReducing the
timerFontSizewhensecondaryContentis present ensures that both the main timer and the secondary content are displayed properly without overlapping or layout issues.
208-211: Approved: Conditional rendering of secondary contentThe
externaldiv correctly uses conditional classes and styling based on the presence ofsecondaryContent. This ensures that the secondary content is only displayed when available and maintains proper styling.
137-148: Issue: Incorrect usage of string inreplacemethodThe
replacemethod is using a string'/:/g'to represent a regular expression. In JavaScript, when using regular expressions withreplace, you should pass aRegExpobject instead of a string. Using a string will not replace all occurrences of the colon character.Apply this diff to fix the regular expression:
- const stageTimerCharacters = display.replace('/:/g', '').length; + const stageTimerCharacters = display.replace(/:/g, '').length;Likely invalid or redundant comment.
apps/client/src/features/viewers/ViewWrapper.tsx (2)
71-71: Variable Naming Consistency: Renameauxtimer1toauxTimerfor clarity.As previously suggested, consider renaming
auxtimer1toauxTimerin the destructuring assignment to maintain consistent naming conventions and enhance code readability.
99-99: Variable Naming Consistency: UseauxTimerinstead ofauxtimer1when passing as a prop.Following the prior recommendation, after renaming
auxtimer1toauxTimer, pass it asauxTimer={auxTimer}to align the prop name with the variable name for consistency.
cpvalente
force-pushed
the
timer-preview
branch
from
5bc8e9e
to
01d4684
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
apps/client/src/features/control/message/MessageControl.module.scss (1)
59-61: Clarify the purpose and usage of the.blackoutclass.The
.blackoutclass setsdisplay: none, which hides the element. However, the purpose and usage of this class are not clear from the provided context. Please add a comment explaining when and why this class is used.apps/server/src/api-integration/integration.controller.ts (1)
101-101: Reminder: Address the TODO comment.The TODO comment indicates that the legacy message conversion logic should be removed after a specific date (20/09/2024). Please ensure to address this TODO comment and remove the legacy code when the specified date arrives.
Do you want me to open a GitHub issue to track this task?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
packages/types/src/definitions/runtime/MessageControl.type.tsis excluded by none and included by nonepackages/types/src/index.tsis excluded by none and included by nonepackages/types/src/utils/utils.type.tsis excluded by none and included by none
Files selected for processing (23)
- apps/client/src/common/hooks/useSocket.ts (3 hunks)
- apps/client/src/common/stores/runtime.ts (1 hunks)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx (1 hunks)
- apps/client/src/features/control/message/InputRow.tsx (3 hunks)
- apps/client/src/features/control/message/MessageControl.module.scss (1 hunks)
- apps/client/src/features/control/message/MessageControl.tsx (1 hunks)
- apps/client/src/features/control/message/MessageControlExport.jsx (1 hunks)
- apps/client/src/features/control/message/TimerPreview.tsx (1 hunks)
- apps/client/src/features/control/message/TimerViewControl.tsx (1 hunks)
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx (1 hunks)
- apps/client/src/features/editors/Editor.module.scss (2 hunks)
- apps/client/src/features/editors/EditorMixin.scss (2 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (2 hunks)
- apps/client/src/features/viewers/ViewWrapper.tsx (2 hunks)
- apps/client/src/features/viewers/timer/Timer.tsx (6 hunks)
- apps/server/src/api-integration/tests/integration.legacy.test.ts (1 hunks)
- apps/server/src/api-integration/integration.controller.ts (3 hunks)
- apps/server/src/api-integration/integration.legacy.ts (1 hunks)
- apps/server/src/app.ts (2 hunks)
- apps/server/src/services/message-service/MessageService.ts (1 hunks)
- apps/server/src/services/message-service/tests/MessageService.test.ts (3 hunks)
- apps/server/src/services/message-service/tests/messageUtils.test.ts (1 hunks)
- apps/server/src/services/message-service/messageUtils.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx
Files skipped from review as they are similar to previous changes (11)
- apps/client/src/features/control/message/InputRow.tsx
- apps/client/src/features/control/message/MessageControlExport.jsx
- apps/client/src/features/control/message/TimerPreview.tsx
- apps/client/src/features/control/message/TimerViewControl.tsx
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx
- apps/client/src/features/editors/Editor.module.scss
- apps/client/src/features/editors/EditorMixin.scss
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx
- apps/client/src/features/viewers/ViewWrapper.tsx
- apps/client/src/features/viewers/timer/Timer.tsx
- apps/server/src/app.ts
Additional context used
Biome
apps/server/src/services/message-service/MessageService.ts
[error] 52-52: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 52-52: 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 (42)
apps/server/src/services/message-service/__tests__/messageUtils.test.ts (2)
Line range hint
10-34: LGTM!The test cases for the
validateTimerMessagefunction are well-written and cover the expected behavior of the function, including the conversion of string values to boolean and the omission of keys not provided in the input payload.
5-5: Verify the impact of the change invalidateMessagefunction's behavior.The test case suggests that the
validateMessagefunction now simply returns the input string without any validation or transformation. This is a significant change from the original behavior, where the function was expected to validate and convert the input payload to a Message object.Please ensure that this change does not introduce any bugs or unexpected behavior in other parts of the codebase where this function is being used.
Run the following script to verify the usage of
validateMessagefunction:apps/server/src/services/message-service/__tests__/MessageService.test.ts (5)
1-1: LGTM!The change from default import to namespace import for
messageServiceis a valid stylistic preference and does not affect the functionality.
17-17: LGTM!The change in the structure of the
externalproperty from an object to a string value is consistent with the modifications made to the expected output of thepatchfunction in the test case.
23-23: LGTM!The addition of the
secondarySourceproperty to thetimerobject in the expected output is consistent with the modifications made to themessageobject in the test case.
24-24: LGTM!The change in the structure of the
externalproperty from an object to a string value in the expected output is consistent with the modifications made to themessageobject in the test case.
36-37: LGTM!The addition of the
secondarySourceproperty to thetimerobject and the change in the structure of theexternalproperty to an empty string in the expected output are consistent with the modifications made in the previous test case.apps/server/src/api-integration/__tests__/integration.legacy.test.ts (2)
4-16: LGTM!The test case correctly verifies the behavior of the
handleLegacyMessageConversionfunction when the payload is not a legacy message. It uses appropriate test inputs and assertions.
18-35: LGTM!The test case correctly verifies the behavior of the
handleLegacyMessageConversionfunction when the payload is a legacy message with an external message. It uses appropriate test inputs covering different scenarios and assertions.apps/client/src/features/control/message/MessageControl.module.scss (9)
1-7: LGTM!The
.labelclass and its active state are well-defined using variables for consistency. The active state styling provides good visual feedback to the user.
10-14: LGTM!The
.previewContainerclass uses a clean grid layout with a consistent gap between items. The 2:1 column ratio suggests a clear hierarchy of content.
16-22: LGTM!The
.previewclass has a clean and centered layout. The use of a variable for the background color maintains consistency, and the relative positioning allows for flexibility in child element positioning.
30-37: LGTM!The
.eventStatusclass is well-positioned as an overlay on the preview. The column layout with a small gap allows for compact display of multiple status indicators.
39-53: LGTM!The
.mainContentclass has clear font styles and uses a CSS variable for easy color customization. The different color styles based on thedata-phaseattribute provide good visual indicators for different phases, with a reduced opacity for thenonephase. The use of variables for colors maintains consistency.
55-57: LGTM!The
.secondaryContentclass has a clear visual separation from the main content with a top border. The use of a variable for the border color maintains consistency.
63-66: LGTM!The
.timerIndicatorsclass uses a column layout to stack its content vertically, which is appropriate for displaying multiple timer indicators.
68-74: LGTM!The
.statusIconclass has a clear default color and an active state color, both set using variables for consistency. The active state color provides a good visual indication when thedata-activeattribute istrue.
76-78: LGTM!The
.dividerclass provides a clear visual separation between sections with a top border. The use of a variable for the border color maintains consistency.apps/client/src/features/control/message/MessageControl.tsx (3)
6-14: LGTM!The refactoring of the
MessageControlcomponent into smaller, more focused components is a great improvement in terms of code organization and separation of concerns. The removal of button controls also suggests a cleaner, more streamlined design.
16-29: LGTM!The
TimerMessageInputcomponent is well-structured and utilizes theuseTimerMessageInputhook effectively to manage the state of the timer message. TheInputRowcomponent is rendered with the appropriate props, making the code clean and easy to understand.
31-52: LGTM!The
ExternalInputcomponent is well-structured and utilizes theuseExternalMessageInputhook effectively to manage the state of the external message. TheInputRowcomponent is rendered with the appropriate props, and thetoggleExternalfunction handles the visibility of the external message correctly.apps/server/src/services/message-service/messageUtils.ts (3)
Line range hint
18-29: LGTM!The addition of the
secondarySourceproperty validation improves the robustness of the timer message validation process. The function correctly handles the new property and ensures that it has a valid value.Consider adding a test case to verify the behavior of the
validateTimerMessagefunction with the newsecondarySourceproperty:describe('validateTimerMessage', () => { // ...existing test cases it('should validate and coerce the secondarySource property', () => { const message = { text: 'Hello, world!', visible: true, blink: false, blackout: false, secondarySource: 'aux', }; const result = validateTimerMessage(message); expect(result.secondarySource).toBe('aux'); }); it('should throw an error for an invalid secondarySource value', () => { const message = { text: 'Hello, world!', visible: true, blink: false, blackout: false, secondarySource: 'invalid', }; expect(() => validateTimerMessage(message)).toThrowError('Invalid value received'); }); });
35-37: LGTM!The
assertSecondaryfunction provides a clear and concise way to validate the secondary value. It is used by thecoerceSecondaryfunction to ensure that the secondary value is valid.Consider adding a test case to verify the behavior of the
assertSecondaryfunction:describe('assertSecondary', () => { it('should return true for valid secondary values', () => { expect(assertSecondary('aux')).toBe(true); expect(assertSecondary('external')).toBe(true); expect(assertSecondary(null)).toBe(true); }); it('should return false for invalid secondary values', () => { expect(assertSecondary('invalid')).toBe(false); expect(assertSecondary(undefined)).toBe(false); expect(assertSecondary(123)).toBe(false); }); });
42-47: LGTM, but consider enhancing the error message as suggested in the past review.The
coerceSecondaryfunction provides a convenient way to coerce the secondary value to a valid value. However, the error message could be improved to provide more context about the invalid value and the expected valid values.As suggested in the past review, apply this diff to improve the error message:
function coerceSecondary(source: unknown): TimerMessage['secondarySource'] { if (!assertSecondary(source)) { - throw new Error('Invalid value received'); + throw new Error( + `Invalid secondary value received: ${source}. Expected 'aux', 'external', or null.` + ); } return source; }apps/server/src/services/message-service/MessageService.ts (5)
7-13: LGTM!The default value for the
timerobject is correctly defined as a constant with predefined properties.
15-16: LGTM!The
timerandexternalvariables are correctly initialized using the default values.
24-26: LGTM!The
initfunction correctly initializes the message service by assigning the throttledpublishFnto thethrottledSetvariable.
31-34: LGTM!The
clearfunction correctly resets the internal state of the message service by resetting thetimerandexternalvariables to their default values.
49-62: LGTM with a minor suggestion!The
patchfunction correctly updates the internal state based on the providedpatchobject, publishes the new state, and returns it.However, as suggested by the static analysis hints and past review comments, the
DEVlabel is unused and should be removed to clean up the code.Apply this diff to remove the unused label:
- // eslint-disable-next-line no-unused-labels -- dev code path - DEV: { if (throttledSet === null) { throw new Error('MessageService.patch() called before init()'); } - }Tools
Biome
[error] 52-52: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 52-52: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
apps/client/src/common/stores/runtime.ts (1)
26-26: LGTM!The addition of the
secondarySourceproperty aligns with the PR objective of exploring extra data for the timer feature. Initializing it tonullis a valid approach for an optional property.apps/client/src/common/hooks/useSocket.ts (5)
31-39: LGTM!The
useTimerViewControlhook is correctly extracting the relevant properties from theRuntimeStorefor controlling the timer view. The property names match theRuntimeStoreinterface and the hook is correctly typed.
41-48: LGTM!The
useTimerMessageInputhook is correctly extracting the relevant properties from theRuntimeStorefor the timer message input. The property names match theRuntimeStoreinterface and the hook is correctly typed.
50-57: LGTM!The
useExternalMessageInputhook is correctly extracting the relevant properties from theRuntimeStorefor the external message input. The visibility state is correctly set based on the timer's secondary source, and the hook is correctly typed.
59-69: LGTM!The
useMessagePreviewhook is correctly extracting the relevant properties from theRuntimeStorefor the message preview. The various visibility and content checks are based on the appropriate conditions, and the hook is correctly typed.
76-76: LGTM!The new
externalTextandtimerSecondarymethods in thesetMessageobject are correctly sending the respective payloads to the socket. The methods are correctly typed and the payload types match the expected types.Also applies to: 79-80
apps/server/src/api-integration/integration.controller.ts (7)
1-3: LGTM!The new imports are necessary for using the respective types and constants in the code. The import statements are correct.
4-5: LGTM!The new import for
DeepPartialfromts-essentialsis likely intended to enhance type handling within the code. The import statement is correct.
8-8: LGTM!The change from default import to namespace import for
messageServiceallows for more flexible access to its exports. The import statement is correct.
18-18: LGTM!The new import for
handleLegacyMessageConversionfrom./integration.legacy.jsis likely used for processing legacy message format. The import statement is correct.
81-97: LGTM!The new
showExternalaction handler enhances the functionality related to external message display. The use ofDeepPartial<MessageState>ensures type safety while constructing the patch. The conditional logic correctly handles the case when the payload is a non-empty string, and the call tomessageService.patchwith the constructed patch is appropriate.
101-102: LGTM!The TODO comment serves as a reminder to remove the legacy message conversion logic in the future, and the call to
handleLegacyMessageConversionensures that the legacy message format is handled appropriately. The changes ensure that the validation logic works with the migrated payload.
105-106: LGTM!The conditional setting of the
timerandexternalproperties ensures that they are only included in the patch if present in the migrated payload. The use ofvalidateTimerMessageandvalidateMessagefunctions ensures that thetimerandexternalproperties are validated before being included in the patch. The changes improve the robustness of the message handling logic by incorporating validation mechanisms.
cpvalente
force-pushed
the
timer-preview
branch
6 times, most recently
from
1af12e4
to
ed6b49a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
e2e/tests/features/201-message-control.spec.tsis excluded by none and included by nonepackages/types/src/definitions/runtime/MessageControl.type.tsis excluded by none and included by nonepackages/types/src/index.tsis excluded by none and included by nonepackages/types/src/utils/utils.type.tsis excluded by none and included by none
Files selected for processing (25)
- apps/client/src/common/hooks/useSocket.ts (3 hunks)
- apps/client/src/common/stores/runtime.ts (1 hunks)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx (1 hunks)
- apps/client/src/features/control/message/InputRow.tsx (3 hunks)
- apps/client/src/features/control/message/MessageControl.module.scss (1 hunks)
- apps/client/src/features/control/message/MessageControl.tsx (1 hunks)
- apps/client/src/features/control/message/MessageControlExport.jsx (1 hunks)
- apps/client/src/features/control/message/TimerPreview.tsx (1 hunks)
- apps/client/src/features/control/message/TimerViewControl.tsx (1 hunks)
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx (1 hunks)
- apps/client/src/features/editors/Editor.module.scss (2 hunks)
- apps/client/src/features/editors/EditorMixin.scss (2 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (2 hunks)
- apps/client/src/features/viewers/ViewWrapper.tsx (4 hunks)
- apps/client/src/features/viewers/timer/Timer.scss (1 hunks)
- apps/client/src/features/viewers/timer/Timer.tsx (7 hunks)
- apps/client/src/theme/_viewerDefs.scss (1 hunks)
- apps/server/src/api-integration/tests/integration.legacy.test.ts (1 hunks)
- apps/server/src/api-integration/integration.controller.ts (3 hunks)
- apps/server/src/api-integration/integration.legacy.ts (1 hunks)
- apps/server/src/app.ts (3 hunks)
- apps/server/src/services/message-service/MessageService.ts (1 hunks)
- apps/server/src/services/message-service/tests/MessageService.test.ts (3 hunks)
- apps/server/src/services/message-service/tests/messageUtils.test.ts (1 hunks)
- apps/server/src/services/message-service/messageUtils.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx
Files skipped from review as they are similar to previous changes (20)
- apps/client/src/common/hooks/useSocket.ts
- apps/client/src/common/stores/runtime.ts
- apps/client/src/features/control/message/InputRow.tsx
- apps/client/src/features/control/message/MessageControlExport.jsx
- apps/client/src/features/control/message/TimerPreview.tsx
- apps/client/src/features/control/message/TimerViewControl.tsx
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx
- apps/client/src/features/editors/Editor.module.scss
- apps/client/src/features/editors/EditorMixin.scss
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx
- apps/client/src/features/viewers/ViewWrapper.tsx
- apps/client/src/features/viewers/timer/Timer.scss
- apps/client/src/features/viewers/timer/Timer.tsx
- apps/client/src/theme/_viewerDefs.scss
- apps/server/src/api-integration/tests/integration.legacy.test.ts
- apps/server/src/api-integration/integration.controller.ts
- apps/server/src/api-integration/integration.legacy.ts
- apps/server/src/app.ts
- apps/server/src/services/message-service/tests/MessageService.test.ts
- apps/server/src/services/message-service/messageUtils.ts
Additional context used
Biome
apps/server/src/services/message-service/MessageService.ts
[error] 52-52: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 52-52: 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 (18)
apps/client/src/features/control/message/MessageControl.module.scss (10)
1-7: LGTM!The
.labelclass styles are well-structured and follow good practices by using variables for colors and sizes. The active state is also handled correctly.
10-14: LGTM!The
.previewContainerclass creates a clean and responsive grid layout. The use of thefrunit for column sizes is a good choice for flexibility.
16-22: LGTM!The
.previewclass styles the preview element effectively. The use of CSS Grid for centering and relative positioning for child elements is a good approach.
30-37: LGTM!The
.eventStatusclass effectively positions and styles the event status element. The use of absolute positioning and flexbox for layout is a good choice.
39-53: LGTM!The
.mainContentclass styles the main content effectively. The use of conditional styles based on data attributes is a good approach for handling different phases. The CSS variable for color override is also a nice touch.
55-57: LGTM!The
.secondaryContentclass adds a simple border-top to separate the secondary content. The styles are clean and straightforward.
59-61: LGTM!The
.blackoutclass simply hides the blackout element by setting the display property tonone. The styles are clean and straightforward.
63-66: LGTM!The
.timerIndicatorsclass creates a flex container and stacks the child elements vertically. The styles are clean and straightforward.
68-74: LGTM!The
.statusIconclass styles the status icon effectively. The use of a conditional style based on thedata-activeattribute is a good approach for indicating the active state.
76-78: LGTM!The
.dividerclass adds a simple border-top to create a divider. The styles are clean and straightforward.apps/client/src/features/control/message/MessageControl.tsx (4)
1-1: LGTM!The new hooks
useExternalMessageInputanduseTimerMessageInputprovide a cleaner separation of concerns for managing timer and external message states. ThesetMessagefunction will likely be used to update these states.
4-4: Looks good!The new
TimerControlsPreviewcomponent aligns with the PR objective of implementing a timer preview feature.
6-29: Great refactor!The
MessageControlcomponent has been nicely refactored to render three distinct components, improving the organization of the code.The
TimerMessageInputcomponent encapsulates the timer message functionality, using theuseTimerMessageInputhook to manage its state. TheInputRowcomponent is reused to maintain consistency, and thechangeHandlerandactionHandlerprops provide a clean way to update the timer message state and visibility.
31-50: Nicely done!The
ExternalInputcomponent encapsulates the external message functionality, using theuseExternalMessageInputhook to manage its state. ThetoggleExternalfunction provides a clean way to toggle the visibility of the external message by updating thetimerSecondarystate.The
InputRowcomponent is reused to maintain consistency, and thechangeHandlerandactionHandlerprops provide a straightforward way to update the external message state and visibility.apps/server/src/services/message-service/MessageService.ts (4)
7-13: LGTM!The default value for the
timerobject is correctly defined as a constant with predefined properties.
15-16: LGTM!The
timerandexternalvariables are correctly initialized using the default values.
24-26: LGTM!The
initfunction correctly initializes the message service by assigning the throttledpublishFnto thethrottledSetvariable.
31-34: LGTM!The
clearfunction correctly resets the internal state of the message service by resetting thetimerandexternalvariables to their default values.
Nice, I have created a documentation page that hopefully helps |
cpvalente
force-pushed
the
timer-preview
branch
from
ed6b49a
to
44e524c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (7)
apps/client/src/features/control/message/MessageControl.module.scss (5)
1-7: LGTM! Consider using a variable for the active state color.The
.labelclass is well-structured and uses variables effectively. This aligns with the PR objective of improving the timer view.For consistency, consider defining a variable for the active state color (currently
$action-text-color) if it's used elsewhere in the application.
16-22: LGTM! Consider using a variable for the background color.The
.previewclass effectively styles the preview area, contributing to the PR objective of creating a visually distinct timer preview.For improved flexibility and consistency, consider using a variable for the background color (currently
$ui-black) if it's used elsewhere in the application or might need to be changed in the future.
30-37: LGTM! Consider using variables for positioning and spacing values.The
.eventStatusclass effectively positions and styles event status indicators, supporting the PR objective of displaying timer phases in the preview.For improved maintainability, consider using variables for the positioning and spacing values (e.g.,
margin: 0.5rem 0.25rem;andgap: 0.25rem;) if these values are used consistently across the application.
39-53: LGTM! Effective use of data attributes for phase-based styling.The
.mainContentclass effectively implements phase-based styling using data attributes, which aligns well with the PR objective of displaying different timer phases.For consistency, consider using variables for the opacity value (
opacity: $opacity-disabled;) and potentially for the font size and weight if these values are used elsewhere in the application.
68-74: LGTM! Consider using variables for colors.The
.statusIconclass effectively uses data attributes to manage the active state and change colors accordingly. This aligns well with the PR objective of displaying timer phases in the preview.For consistency and maintainability, consider using variables for the colors (
$gray-1000and$active-indicator) if these colors are used elsewhere in the application.apps/client/src/features/control/message/MessageControl.tsx (2)
1-14: Improved component structure and modularityThe refactoring of the
MessageControlcomponent and the introduction of new hooks (useExternalMessageInputanduseTimerMessageInput) have significantly improved the code structure and modularity. This aligns well with the PR objectives of enhancing the timer view and creating a preview.Consider adding a brief comment above the
MessageControlfunction to describe its overall purpose and the role of each sub-component it renders. This would further improve code readability and maintainability.
16-29: Well-structured TimerMessageInput componentThe introduction of the
TimerMessageInputcomponent effectively encapsulates the timer message input functionality. The use of theuseTimerMessageInputhook for state management is a good practice, promoting better separation of concerns.Consider adding prop types for the
InputRowcomponent to improve type safety and code documentation. This could be done using TypeScript interfaces or PropTypes if you're using JavaScript.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
e2e/tests/features/201-message-control.spec.tsis excluded by none and included by nonepackages/types/src/definitions/runtime/MessageControl.type.tsis excluded by none and included by nonepackages/types/src/index.tsis excluded by none and included by nonepackages/types/src/utils/utils.type.tsis excluded by none and included by none
Files selected for processing (25)
- apps/client/src/common/hooks/useSocket.ts (3 hunks)
- apps/client/src/common/stores/runtime.ts (1 hunks)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx (1 hunks)
- apps/client/src/features/control/message/InputRow.tsx (3 hunks)
- apps/client/src/features/control/message/MessageControl.module.scss (1 hunks)
- apps/client/src/features/control/message/MessageControl.tsx (1 hunks)
- apps/client/src/features/control/message/MessageControlExport.jsx (1 hunks)
- apps/client/src/features/control/message/TimerPreview.tsx (1 hunks)
- apps/client/src/features/control/message/TimerViewControl.tsx (1 hunks)
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx (1 hunks)
- apps/client/src/features/editors/Editor.module.scss (2 hunks)
- apps/client/src/features/editors/EditorMixin.scss (2 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (2 hunks)
- apps/client/src/features/viewers/ViewWrapper.tsx (4 hunks)
- apps/client/src/features/viewers/timer/Timer.scss (1 hunks)
- apps/client/src/features/viewers/timer/Timer.tsx (7 hunks)
- apps/client/src/theme/_viewerDefs.scss (1 hunks)
- apps/server/src/api-integration/tests/integration.legacy.test.ts (1 hunks)
- apps/server/src/api-integration/integration.controller.ts (3 hunks)
- apps/server/src/api-integration/integration.legacy.ts (1 hunks)
- apps/server/src/app.ts (3 hunks)
- apps/server/src/services/message-service/MessageService.ts (1 hunks)
- apps/server/src/services/message-service/tests/MessageService.test.ts (3 hunks)
- apps/server/src/services/message-service/tests/messageUtils.test.ts (1 hunks)
- apps/server/src/services/message-service/messageUtils.ts (3 hunks)
Files skipped from review as they are similar to previous changes (21)
- apps/client/src/common/hooks/useSocket.ts
- apps/client/src/common/stores/runtime.ts
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx
- apps/client/src/features/control/message/InputRow.tsx
- apps/client/src/features/control/message/MessageControlExport.jsx
- apps/client/src/features/control/message/TimerPreview.tsx
- apps/client/src/features/control/message/TimerViewControl.tsx
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx
- apps/client/src/features/editors/Editor.module.scss
- apps/client/src/features/editors/EditorMixin.scss
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx
- apps/client/src/features/viewers/ViewWrapper.tsx
- apps/client/src/features/viewers/timer/Timer.scss
- apps/client/src/features/viewers/timer/Timer.tsx
- apps/client/src/theme/_viewerDefs.scss
- apps/server/src/api-integration/tests/integration.legacy.test.ts
- apps/server/src/api-integration/integration.controller.ts
- apps/server/src/api-integration/integration.legacy.ts
- apps/server/src/app.ts
- apps/server/src/services/message-service/tests/MessageService.test.ts
- apps/server/src/services/message-service/messageUtils.ts
Additional context used
Biome
apps/server/src/services/message-service/MessageService.ts
[error] 52-52: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 52-52: 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 (13)
apps/server/src/services/message-service/__tests__/messageUtils.test.ts (2)
Line range hint
9-38: Consider consistency betweenvalidateMessageandvalidateTimerMessageWhile the tests for
validateTimerMessageremain unchanged, the significant alterations tovalidateMessagecreate a potential inconsistency:
validateMessagenow appears to handle simple strings, whilevalidateTimerMessagestill processes complex objects.- This discrepancy in behavior between similarly named functions could be confusing for developers.
Consider the following:
- Should
validateTimerMessagebe updated to align with the new behavior ofvalidateMessage?- If not, would it be beneficial to rename one or both functions to better reflect their distinct purposes?
- Is there a reason for maintaining different validation approaches for messages and timer messages?
To better understand the relationship between these functions, please run the following script:
#!/bin/bash # Search for the implementation of validateMessage and validateTimerMessage rg --type typescript "export function validate(Message|TimerMessage)" -C 5Could you clarify the intended relationship between these two functions and whether any further alignment or differentiation is planned?
Line range hint
1-7: Significant changes tovalidateMessagefunction behavior require clarificationThe modifications to the
validateMessagetest suggest a fundamental change in the function's behavior:
- The function now appears to handle simple string inputs directly, rather than processing complex payload objects as before.
- Previous functionality, such as boolean conversions and handling of partial payloads, seems to have been removed.
These changes raise several concerns:
- Has the purpose of the
validateMessagefunction changed? If so, consider renaming it to better reflect its new behavior.- Is the simplification intentional, or has some functionality been inadvertently lost?
- Are there any implications for existing code that may depend on the previous behavior?
To better understand the extent of these changes, please run the following script to check for other usages of
validateMessage:Could you provide more context on the rationale behind these changes and confirm if any related updates are needed in other parts of the codebase?
apps/client/src/features/control/message/MessageControl.module.scss (6)
10-13: LGTM! Effective use of grid layout for the preview container.The
.previewContainerclass uses grid layout effectively to create a structured preview, which aligns well with the PR objective of creating a timer preview.
55-57: LGTM! Clear visual separation for secondary content.The
.secondaryContentclass effectively separates secondary content with a top border, using a variable for color consistency.
59-61: Clarify the intended use of the.blackoutclass.The
.blackoutclass is currently set todisplay: none;. Could you provide more context on its intended use? If it's for future functionality, consider adding a TODO comment to explain its purpose.
63-66: LGTM! Effective use of flexbox for timer indicators.The
.timerIndicatorsclass uses flexbox to create a column layout, which is appropriate for organizing timer indicators. This aligns with the PR objective of creating a timer preview with phase indicators.
76-78: LGTM! Clear visual divider.The
.dividerclass effectively creates a visual separator using a top border, with a variable used for color consistency.
1-78: Overall, the changes effectively support the PR objectives.The modifications to
MessageControl.module.scsssignificantly enhance the structure and visual presentation of the timer preview. The new classes for preview container, main content, status indicators, and various UI elements align well with the PR objectives of improving the timer view and creating a preview with phase indicators.The consistent use of variables, data attributes for state management, and modern CSS techniques (like grid and flexbox) contribute to a more maintainable and flexible stylesheet. These changes should greatly improve the user experience when interacting with the timer functionality.
apps/server/src/services/message-service/MessageService.ts (5)
1-18: LGTM: Imports and default values are well-structuredThe imports, default values, and global variables are appropriately defined for the new functional approach. The use of
DeepPartialfrom 'ts-essentials' is a good practice for flexible type definitions in thepatchfunction.
20-26: LGTM: Well-implemented initialization functionThe
initfunction is well-documented and correctly sets up a throttled publish function. This approach is good for managing state updates efficiently.
28-34: LGTM: Clear function effectively resets stateThe
clearfunction is well-implemented, using the spread operator to create a new object for the timer state and resetting both timer and external states. This approach ensures a clean state reset.
36-44: LGTM: GetState function provides comprehensive stateThe
getStatefunction is well-implemented, returning a complete view of the current message state by combining both timer and external states. Its simplicity and clarity are commendable.
1-62: Overall: Excellent refactoring to functional approachThe refactoring of
MessageService.tsfrom a class-based structure to a functional approach is well-executed. The new functions (init,clear,getState, andpatch) are well-implemented and documented. The state management is consistent and efficient, using appropriate TypeScript features likeDeepPartialfor flexible updates.The only minor issue was the unused
DEVlabel in thepatchfunction, which has been addressed in a previous comment.This refactoring improves code clarity and maintainability, aligning well with modern JavaScript/TypeScript practices. Great job on the implementation!
Tools
Biome
[error] 52-52: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 52-52: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
cpvalente
force-pushed
the
timer-preview
branch
from
44e524c
to
aef92e0
Compare
|
for this |
cpvalente
force-pushed
the
timer-preview
branch
from
aef92e0
to
bfd27da
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
apps/client/src/features/control/message/MessageControl.tsx (1)
1-4: Remove unused importThe
InputRowimport is not used in this file. Consider removing it to keep the imports clean and avoid confusion.Apply this change:
-import InputRow from './InputRow';apps/client/src/features/control/message/MessageControl.module.scss (5)
10-13: LGTM: Effective use of CSS Grid for layoutThe
.previewContainerclass makes good use of CSS Grid for layout, with a sensible 2:1 column ratio. The use of variables for spacing is commendable.Consider adding a comment explaining the purpose of the 2:1 ratio for better maintainability:
.previewContainer { display: grid; gap: $element-spacing; // 2:1 ratio for main content and sidebar grid-template-columns: 2fr 1fr; }
24-38: LGTM: Well-implemented corner element with hover effectsThe
.cornerclass effectively creates a diagonal corner element with smooth hover transitions. The use of variables for colors and transition time is commendable.Consider adding a
focusstate alongside thehoverstate for better keyboard accessibility:.corner { // ... existing styles ... &:hover, &:focus { color: $ontime-color; } }
46-53: LGTM with suggestion: Consider using variables for consistencyThe
.eventStatusclass is well-structured for overlaying status information. The use of flexbox for layout is appropriate.Consider using variables for the margin and gap values to maintain consistency with the rest of the stylesheet:
.eventStatus { position: absolute; left: 0; margin: $small-spacing $extra-small-spacing; display: flex; flex-direction: column; gap: $extra-small-spacing; }You may need to define these new variables (
$small-spacingand$extra-small-spacing) if they don't already exist.
55-69: LGTM: Well-structured .mainContent class with dynamic stylingThe
.mainContentclass is well-implemented with good use of CSS custom properties for color overrides and data-attributes for different phases. This approach allows for flexible and maintainable styling.Consider using a Sass map for the phase colors to improve maintainability:
$phase-colors: ( 'pending': $ontime-roll, 'overtime': $playback-negative, 'none': rgba($ui-white, $opacity-disabled) ); .mainContent { // ... existing styles ... @each $phase, $color in $phase-colors { &[data-phase='#{$phase}'] { color: $color; } } }This approach makes it easier to add or modify phases in the future.
71-93: LGTM: Clean implementation of utility classesThe new utility classes (.secondaryContent, .blackout, .timerIndicators, .statusIcon, and .divider) are well-structured and focused. The use of variables and data attributes for styling is commendable.
For the
.blackoutclass, consider using a mixin or a utility class for hiding elements instead of directly settingdisplay: none. This can make it easier to implement different hiding strategies in the future:@mixin hidden { display: none; } // Or as a utility class .hidden { display: none; } .blackout { @include hidden; // or @extend .hidden; }This approach provides more flexibility if you need to change how elements are hidden in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (4)
e2e/tests/features/201-message-control.spec.tsis excluded by none and included by nonepackages/types/src/definitions/runtime/MessageControl.type.tsis excluded by none and included by nonepackages/types/src/index.tsis excluded by none and included by nonepackages/types/src/utils/utils.type.tsis excluded by none and included by none
📒 Files selected for processing (25)
- apps/client/src/common/hooks/useSocket.ts (3 hunks)
- apps/client/src/common/stores/runtime.ts (1 hunks)
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx (1 hunks)
- apps/client/src/features/control/message/InputRow.tsx (3 hunks)
- apps/client/src/features/control/message/MessageControl.module.scss (1 hunks)
- apps/client/src/features/control/message/MessageControl.tsx (1 hunks)
- apps/client/src/features/control/message/MessageControlExport.jsx (1 hunks)
- apps/client/src/features/control/message/TimerPreview.tsx (1 hunks)
- apps/client/src/features/control/message/TimerViewControl.tsx (1 hunks)
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx (1 hunks)
- apps/client/src/features/editors/Editor.module.scss (2 hunks)
- apps/client/src/features/editors/EditorMixin.scss (2 hunks)
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx (2 hunks)
- apps/client/src/features/viewers/ViewWrapper.tsx (4 hunks)
- apps/client/src/features/viewers/timer/Timer.scss (1 hunks)
- apps/client/src/features/viewers/timer/Timer.tsx (7 hunks)
- apps/client/src/theme/_viewerDefs.scss (1 hunks)
- apps/server/src/api-integration/tests/integration.legacy.test.ts (1 hunks)
- apps/server/src/api-integration/integration.controller.ts (3 hunks)
- apps/server/src/api-integration/integration.legacy.ts (1 hunks)
- apps/server/src/app.ts (3 hunks)
- apps/server/src/services/message-service/MessageService.ts (1 hunks)
- apps/server/src/services/message-service/tests/MessageService.test.ts (3 hunks)
- apps/server/src/services/message-service/tests/messageUtils.test.ts (1 hunks)
- apps/server/src/services/message-service/messageUtils.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- apps/client/src/common/hooks/useSocket.ts
- apps/client/src/common/stores/runtime.ts
- apps/client/src/features/app-settings/panel/general-panel/ViewSettingsForm.tsx
- apps/client/src/features/control/message/InputRow.tsx
- apps/client/src/features/control/message/MessageControlExport.jsx
- apps/client/src/features/control/message/TimerPreview.tsx
- apps/client/src/features/control/message/TimerViewControl.tsx
- apps/client/src/features/control/playback/aux-timer/AuxTimer.tsx
- apps/client/src/features/editors/Editor.module.scss
- apps/client/src/features/editors/EditorMixin.scss
- apps/client/src/features/rundown/event-block/EventBlockInner.tsx
- apps/client/src/features/viewers/ViewWrapper.tsx
- apps/client/src/features/viewers/timer/Timer.scss
- apps/client/src/features/viewers/timer/Timer.tsx
- apps/client/src/theme/_viewerDefs.scss
- apps/server/src/api-integration/tests/integration.legacy.test.ts
- apps/server/src/api-integration/integration.controller.ts
- apps/server/src/api-integration/integration.legacy.ts
- apps/server/src/app.ts
- apps/server/src/services/message-service/tests/MessageService.test.ts
- apps/server/src/services/message-service/messageUtils.ts
🧰 Additional context used
Biome
apps/server/src/services/message-service/MessageService.ts
[error] 52-52: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 52-52: 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 (13)
apps/server/src/services/message-service/__tests__/messageUtils.test.ts (2)
Line range hint
9-38: Consider reviewing overall message validation strategyWhile the
validateTimerMessagetests remain unchanged, they present a stark contrast to the simplifiedvalidateMessagetest. This discrepancy raises questions about the consistency of message handling across the system.Consider the following points:
- Review the relationship between
validateMessageandvalidateTimerMessage. Should they have similar levels of complexity and validation?- Evaluate whether the current split between general messages and timer messages is still appropriate given the changes to
validateMessage.- Consider creating a consistent interface for all types of messages, which could simplify testing and usage throughout the application.
To facilitate this review, it may be helpful to compare the implementations of both functions:
#!/bin/bash # Compare implementations of validateMessage and validateTimerMessage echo "validateMessage implementation:" ast-grep --lang typescript --pattern 'function validateMessage($_) { $$$ }' -C 3 echo "\nvalidateTimerMessage implementation:" ast-grep --lang typescript --pattern 'function validateTimerMessage($_) { $$$ }' -C 3This comparison will help in deciding whether to align the behaviors of these functions or to maintain their distinct implementations with clear documentation of their differences.
Line range hint
1-7: Significant change invalidateMessagebehavior requires careful considerationThe test for
validateMessagehas been drastically simplified, suggesting a fundamental change in the function's behavior. Previously, it appeared to convert a payload object into a structured Message object, but now it seems to simply return the input string as-is.This change raises several concerns:
- The new behavior may not align with the function name "validateMessage", as it no longer appears to perform any validation.
- The change could have significant implications for other parts of the system that rely on the
validateMessagefunction.- The single test case is insufficient to cover various scenarios and ensure the function behaves correctly for different inputs.
Consider the following actions:
- Review the changes made to the
validateMessagefunction implementation to ensure they align with the intended behavior.- Update the function name if it no longer performs validation (e.g.,
formatMessageorprocessMessage).- Add more test cases to cover different scenarios, such as:
- Empty string input
- Non-string input
- Edge cases relevant to the new implementation
- Update documentation and comments to reflect the new behavior.
- Review and update any code that depends on the old behavior of
validateMessage.To assess the impact of this change, run the following script to find other occurrences of
validateMessage:This will help identify areas of the codebase that may need to be updated due to the changed behavior.
apps/client/src/features/control/message/MessageControl.tsx (1)
6-14: Improved component structureThe refactoring of the
MessageControlcomponent has significantly improved its structure. By delegating responsibilities to sub-components, the code is now more modular and easier to maintain. This aligns well with the PR objectives of improving the timer view and creating a preview.apps/server/src/services/message-service/MessageService.ts (6)
1-18: LGTM: Imports and default values are well-structuredThe imports, type definitions, and default value initializations are clear and appropriate for the file's purpose. The use of a default timer structure and mutable variables for
timerandexternalprovides a good foundation for the message service.
20-26: LGTM: Well-implemented initialization functionThe
initfunction is concise and focused. It properly sets up the throttled publish function, which is a good practice for managing update frequency. The function signature is clear and well-typed.
28-34: LGTM: Clear function effectively resets stateThe
clearfunction properly resets both thetimerandexternalstates. The use of the spread operator fortimerensures a clean reset without any lingering references to the old state.
36-44: LGTM: GetState function correctly exposes internal stateThe
getStatefunction is well-implemented, returning the current state of bothexternalandtimermessages. The return type is correctly specified asMessageState, ensuring type safety for consumers of this function.
1-62: Overall: Excellent refactoring to functional approachThe refactoring of
MessageService.tsfrom a class-based structure to a functional approach is well-executed. This change enhances the clarity and maintainability of the code. Key improvements include:
- Clear separation of concerns with distinct functions for initialization, state management, and updates.
- Proper use of TypeScript for type safety throughout the file.
- Efficient state management with the use of default values and the
patchfunction.- Throttling of updates to prevent excessive publish calls.
The only minor issue (the unused DEV label) has been addressed in a previous comment. Overall, this refactoring aligns well with modern JavaScript/TypeScript practices and should improve the overall quality of the codebase.
🧰 Tools
Biome
[error] 52-52: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 52-52: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
46-62:⚠️ Potential issueRemove unused DEV label and approve other changes
The
patchfunction is well-implemented, correctly updating the internal state, publishing the new state, and returning it. However, theDEVlabel is unnecessary and should be removed to clean up the code.Apply this diff to remove the unused label:
- // eslint-disable-next-line no-unused-labels -- dev code path - DEV: { - if (throttledSet === null) { - throw new Error('MessageService.patch() called before init()'); - } - } + if (throttledSet === null) { + throw new Error('MessageService.patch() called before init()'); + }The rest of the implementation looks good and is approved.
🧰 Tools
Biome
[error] 52-52: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 52-52: Unused label.
The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.(lint/correctness/noUnusedLabels)
apps/client/src/features/control/message/MessageControl.module.scss (4)
1-7: LGTM: Well-structured .label classThe new
.labelclass is well-implemented with appropriate use of variables for consistency. The.activestate provides good visual feedback.
16-22: LGTM: Well-structured .preview classThe
.previewclass is well-implemented, using grid for content centering and setting up relative positioning for potential absolutely positioned children. The use of variables for colors maintains consistency.
40-43: LGTM: Clean implementation of .options classThe
.optionsclass uses flexbox effectively for a column layout. The use of variables for spacing maintains consistency across the stylesheet.
1-93: Overall: Well-implemented styling for the new timer preview featureThe changes in this file successfully implement the styling required for the new timer preview feature, aligning well with the PR objectives. The use of modern CSS techniques (Grid, Flexbox) and variables is consistent and commendable, contributing to a maintainable and flexible stylesheet.
While there are a few minor suggestions for improvements (such as using variables for all spacing values and considering a Sass map for phase colors), the overall quality of the changes is high. The new classes are well-structured and focused, providing a solid foundation for the enhanced timer view.
Great job on implementing these styling changes!
| function TimerMessageInput() { | ||
| const { text, visible } = useTimerMessageInput(); | ||
|
|
||
| export default function MessageControl() { | ||
| const message = useMessageControl(); | ||
| const blink = message.timer.blink; | ||
| const blackout = message.timer.blackout; | ||
| return ( | ||
| <InputRow | ||
| label='Timer Message' | ||
| placeholder='Message shown fullscreen in stage timer' | ||
| text={text} | ||
| visible={visible} | ||
| changeHandler={(newValue) => setMessage.timerText(newValue)} | ||
| actionHandler={() => setMessage.timerVisible(!visible)} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance TimerMessageInput component
The TimerMessageInput component effectively encapsulates the timer message input functionality. Consider the following improvements:
- Add error handling for the
setMessage.timerTextandsetMessage.timerVisiblecalls. - Consider using TypeScript to define prop types for better type safety.
Example implementation with error handling:
import { useState } from 'react';
function TimerMessageInput() {
const { text, visible } = useTimerMessageInput();
const [error, setError] = useState<string | null>(null);
const handleChange = (newValue: string) => {
try {
setMessage.timerText(newValue);
setError(null);
} catch (err) {
setError('Failed to update timer text');
console.error(err);
}
};
const handleVisibilityToggle = () => {
try {
setMessage.timerVisible(!visible);
setError(null);
} catch (err) {
setError('Failed to toggle timer visibility');
console.error(err);
}
};
return (
<>
<InputRow
label='Timer Message'
placeholder='Message shown fullscreen in stage timer'
text={text}
visible={visible}
changeHandler={handleChange}
actionHandler={handleVisibilityToggle}
/>
{error && <div className="error-message">{error}</div>}
</>
);
}| function ExternalInput() { | ||
| const { text, visible } = useExternalMessageInput(); | ||
|
|
||
| const toggleExternal = () => { | ||
| if (visible) { | ||
| setMessage.timerSecondary(null); | ||
| } else { | ||
| setMessage.timerSecondary('external'); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <div className={style.messageContainer}> | ||
| <InputRow | ||
| label='Timer' | ||
| placeholder='Message shown in stage timer' | ||
| text={message.timer.text} | ||
| visible={message.timer.visible} | ||
| changeHandler={(newValue) => setMessage.timerText(newValue)} | ||
| actionHandler={() => setMessage.timerVisible(!message.timer.visible)} | ||
| /> | ||
| <div className={style.buttonSection}> | ||
| <Button | ||
| size='sm' | ||
| className={`${blink ? style.blink : ''}`} | ||
| variant={blink ? 'ontime-filled' : 'ontime-subtle'} | ||
| leftIcon={blink ? <IoSunny size='1rem' /> : <IoSunnyOutline size='1rem' />} | ||
| onClick={() => setMessage.timerBlink(!blink)} | ||
| data-testid='toggle timer blink' | ||
| > | ||
| Blink | ||
| </Button> | ||
| <Button | ||
| size='sm' | ||
| className={style.blackoutButton} | ||
| variant={blackout ? 'ontime-filled' : 'ontime-subtle'} | ||
| leftIcon={blackout ? <IoEye size='1rem' /> : <IoEyeOffOutline size='1rem' />} | ||
| onClick={() => setMessage.timerBlackout(!blackout)} | ||
| data-testid='toggle timer blackout' | ||
| > | ||
| Blackout screen | ||
| </Button> | ||
| </div> | ||
| <InputRow | ||
| label='External Message (read only)' | ||
| placeholder={enDash} | ||
| readonly | ||
| text={message.external.text} | ||
| visible={message.external.visible} | ||
| changeHandler={noop} | ||
| actionHandler={noop} | ||
| /> | ||
| </div> | ||
| <InputRow | ||
| label='External Message' | ||
| placeholder='Message shown as secondary text in stage timer' | ||
| text={text} | ||
| visible={visible} | ||
| changeHandler={(newValue) => setMessage.externalText(newValue)} | ||
| actionHandler={toggleExternal} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance ExternalInput component
The ExternalInput component effectively handles the external message functionality. Consider the following improvements:
- Extract the
toggleExternalfunction to a custom hook or useuseCallbackfor better performance. - Add error handling for the
setMessage.timerSecondaryandsetMessage.externalTextcalls. - Consider using TypeScript to define prop types for better type safety.
- Use a constant for the 'external' string to avoid magic strings.
Example implementation with improvements:
import { useCallback, useState } from 'react';
const EXTERNAL_MESSAGE_TYPE = 'external';
function useExternalMessageToggle(visible: boolean) {
return useCallback(() => {
try {
setMessage.timerSecondary(visible ? null : EXTERNAL_MESSAGE_TYPE);
} catch (err) {
console.error('Failed to toggle external message:', err);
// Consider adding user-facing error handling here
}
}, [visible]);
}
function ExternalInput() {
const { text, visible } = useExternalMessageInput();
const [error, setError] = useState<string | null>(null);
const toggleExternal = useExternalMessageToggle(visible);
const handleChange = (newValue: string) => {
try {
setMessage.externalText(newValue);
setError(null);
} catch (err) {
setError('Failed to update external text');
console.error(err);
}
};
return (
<>
<InputRow
label='External Message'
placeholder='Message shown as secondary text in stage timer'
text={text}
visible={visible}
changeHandler={handleChange}
actionHandler={toggleExternal}
/>
{error && <div className="error-message">{error}</div>}
</>
);
}These changes improve the component's robustness and maintainability.
| import { setMessage, useExternalMessageInput, useTimerMessageInput } from '../../../common/hooks/useSocket'; | ||
|
|
||
| import InputRow from './InputRow'; | ||
| import TimerControlsPreview from './TimerViewControl'; | ||
|
|
||
| import style from './MessageControl.module.scss'; | ||
| export default function MessageControl() { | ||
| return ( | ||
| <> | ||
| <TimerControlsPreview /> | ||
| <TimerMessageInput /> | ||
| <ExternalInput /> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| const noop = () => undefined; | ||
| function TimerMessageInput() { | ||
| const { text, visible } = useTimerMessageInput(); | ||
|
|
||
| export default function MessageControl() { | ||
| const message = useMessageControl(); | ||
| const blink = message.timer.blink; | ||
| const blackout = message.timer.blackout; | ||
| return ( | ||
| <InputRow | ||
| label='Timer Message' | ||
| placeholder='Message shown fullscreen in stage timer' | ||
| text={text} | ||
| visible={visible} | ||
| changeHandler={(newValue) => setMessage.timerText(newValue)} | ||
| actionHandler={() => setMessage.timerVisible(!visible)} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| function ExternalInput() { | ||
| const { text, visible } = useExternalMessageInput(); | ||
|
|
||
| const toggleExternal = () => { | ||
| if (visible) { | ||
| setMessage.timerSecondary(null); | ||
| } else { | ||
| setMessage.timerSecondary('external'); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <div className={style.messageContainer}> | ||
| <InputRow | ||
| label='Timer' | ||
| placeholder='Message shown in stage timer' | ||
| text={message.timer.text} | ||
| visible={message.timer.visible} | ||
| changeHandler={(newValue) => setMessage.timerText(newValue)} | ||
| actionHandler={() => setMessage.timerVisible(!message.timer.visible)} | ||
| /> | ||
| <div className={style.buttonSection}> | ||
| <Button | ||
| size='sm' | ||
| className={`${blink ? style.blink : ''}`} | ||
| variant={blink ? 'ontime-filled' : 'ontime-subtle'} | ||
| leftIcon={blink ? <IoSunny size='1rem' /> : <IoSunnyOutline size='1rem' />} | ||
| onClick={() => setMessage.timerBlink(!blink)} | ||
| data-testid='toggle timer blink' | ||
| > | ||
| Blink | ||
| </Button> | ||
| <Button | ||
| size='sm' | ||
| className={style.blackoutButton} | ||
| variant={blackout ? 'ontime-filled' : 'ontime-subtle'} | ||
| leftIcon={blackout ? <IoEye size='1rem' /> : <IoEyeOffOutline size='1rem' />} | ||
| onClick={() => setMessage.timerBlackout(!blackout)} | ||
| data-testid='toggle timer blackout' | ||
| > | ||
| Blackout screen | ||
| </Button> | ||
| </div> | ||
| <InputRow | ||
| label='External Message (read only)' | ||
| placeholder={enDash} | ||
| readonly | ||
| text={message.external.text} | ||
| visible={message.external.visible} | ||
| changeHandler={noop} | ||
| actionHandler={noop} | ||
| /> | ||
| </div> | ||
| <InputRow | ||
| label='External Message' | ||
| placeholder='Message shown as secondary text in stage timer' | ||
| text={text} | ||
| visible={visible} | ||
| changeHandler={(newValue) => setMessage.externalText(newValue)} | ||
| actionHandler={toggleExternal} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Implement Error Handling and Remove Unused Imports
The verification process identified that the new components lack proper error handling mechanisms and there are unused imports within the apps/client/src/features/control/message/ directory.
To address these issues:
-
Error Handling:
- Implement
try/catchblocks or other error handling strategies in the new components (TimerControlsPreview,TimerMessageInput,ExternalInput) to ensure robustness.
- Implement
-
Unused Imports:
- Remove any unused import statements in the following files to maintain code cleanliness:
TimerViewControl.tsxTimerPreview.tsxInputRow.tsx
- Remove any unused import statements in the following files to maintain code cleanliness:
🔗 Analysis chain
Overall improvements and verification suggestion
The refactoring of MessageControl.tsx has significantly improved the code structure, modularity, and maintainability. The introduction of separate components for timer and external messages, along with the use of custom hooks, addresses the PR objectives of improving the timer view and creating a preview.
To ensure these changes don't introduce any regressions and fully address the confusion reported in issue #1024, consider the following:
- Implement comprehensive unit tests for the new components and hooks.
- Conduct thorough integration testing to verify the interaction between these components.
- Update the documentation to reflect the new structure and functionality.
Run the following script to verify the implementation:
This script will help ensure that the old hook has been fully replaced, the new components are used correctly, error handling is implemented, and there are no leftover unused imports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of new components and hooks
# Test: Check for any remaining uses of the old useMessageControl hook
rg -g "*.ts" -g "*.tsx" "useMessageControl"
# Test: Verify that TimerControlsPreview, TimerMessageInput, and ExternalInput are used correctly
rg -g "*.ts" -g "*.tsx" "<(TimerControlsPreview|TimerMessageInput|ExternalInput)\s*/?>" -A 5
# Test: Check for proper implementation of error handling in the new components
rg -g "*.ts" -g "*.tsx" "(try|catch|error)" apps/client/src/features/control/message/
# Test: Verify the usage of new hooks
rg -g "*.ts" -g "*.tsx" "use(ExternalMessageInput|TimerMessageInput)"
# Test: Check for any unused imports or components
rg -g "*.ts" -g "*.tsx" "import.*from.*" apps/client/src/features/control/message/ | grep -vE "(useExternalMessageInput|useTimerMessageInput|TimerControlsPreview)"
Length of output: 4410
WIP
The idea is to help users manage the timer view and make things friendlier
https://www.figma.com/design/TOjSQ4e8q4dhNDNN5DElD3/ontime?node-id=1722-6&t=9OAa5afIUFi2Ule1-1
This also helps resolve #1024
Questions:
The button for external visibility is duplicated, I experimented without it but it felt strange to have two different inputs for External Message and Timer Message. I am not sure what the best way is here
I opted for not showing the real message and time to be conservative of performance. While this is a nice feature, I didnt feel it was worth the performance investment. However we could do better at showing the current formatting from settings (eg: HH:MM), any ideas for this?