feat (ui): implement speed controls #1206
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new Changes
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: 4
Outside diff range, codebase verification and nitpick comments (4)
apps/client/src/features/control/playback/PlaybackControl.tsx (1)
27-27: Ensure TimerSpeed component integration with PlaybackControl.The TimerSpeed component is implemented as a standalone function without receiving any props. It contains internal logic but does not interact with the PlaybackControl component's state or functionality. Consider passing relevant props or connecting it to the parent component's state to ensure proper integration.
- File:
apps/client/src/features/control/playback/timer-speed/TimerSpeed.tsxAnalysis chain
Verify TimerSpeed integration with PlaybackControl.
The TimerSpeed component has been added to the JSX structure, which is good. However, it's not receiving any props, which might indicate that it's not fully integrated with the PlaybackControl component's state or functionality.
Please ensure that the TimerSpeed component is properly integrated with the PlaybackControl component. Consider if it needs to receive any props or interact with the parent component's state.
To verify the implementation of TimerSpeed, you can run the following script:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of TimerSpeed component # Test: Search for the TimerSpeed component implementation ast-grep --lang typescript --pattern 'export default function TimerSpeed($_) { $$$ }' # Test: Check if TimerSpeed is using any hooks or props rg --type typescript 'function TimerSpeed' -A 10Length of output: 175
Script:
#!/bin/bash # Description: Check the implementation of TimerSpeed component # Test: Search for the TimerSpeed component implementation using a broader pattern ast-grep --lang typescript --pattern 'function TimerSpeed($_) { $$$ }' # Test: Correct the file type and search for TimerSpeed usage rg --type tsx 'function TimerSpeed' -A 10Length of output: 146
Script:
#!/bin/bash # Description: Locate and check the implementation of TimerSpeed component # Locate files that might contain the TimerSpeed component fd TimerSpeed --type f # Search for TimerSpeed component implementation in located files rg 'function TimerSpeed' -A 10Length of output: 1259
apps/client/src/features/control/playback/timer-speed/TimerSpeed.module.scss (2)
19-34: LGTM: Speed container and regular speed indicator look good.The
.speedContainerand.speedRegularclasses are well-structured to create a visual speed indicator.Consider using CSS custom properties for values like
33.33%to make it easier to adjust in the future:.speedRegular { position: absolute; left: 0; height: 100%; - width: 33.33%; + width: var(--speed-regular-width, 33.33%); background-color: $ui-white; }
8-13: Unify.labelstyles across the codebase.The
.labelclass is defined in multiple SCSS files, suggesting potential inconsistencies in styling. Consider addressing the TODO comment by unifying these styles to ensure consistency across components. This could involve centralizing the styles in a common stylesheet or using a shared mixin.
- Instances found in various files such as:
apps/client/src/features/operator/status-bar/StatusBar.module.scssapps/client/src/features/viewers/timeline/TimelinePage.scssapps/client/src/features/control/playback/timer-speed/TimerSpeed.module.scss- And others...
Analysis chain
Consider unifying panel labels as suggested by the TODO comment.
The
.labelclass looks good, but there's a TODO comment about unifying panel labels. This might indicate a need for consistency across different components.To check for consistency, let's search for similar label styles:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for similar label styles across the codebase # Test: Look for similar label styles. Expect: Consistent label styles across components. rg --type scss -g '*.module.scss' '\.label\s*\{'Length of output: 79
Script:
#!/bin/bash # Description: Search for similar label styles across SCSS files # Test: Look for similar label styles. Expect: Consistent label styles across components. rg '\.label\s*\{' --glob '*.scss'Length of output: 1095
apps/client/src/features/control/playback/timer-speed/TimerSpeed.tsx (1)
1-62: Summary of improvements needed:
- Extract and test the
mapRangefunction.- Implement state management for speed values.
- Complete the handler functions with actual logic.
- Refactor inline styles to SCSS module.
- Improve accessibility with ARIA labels and semantic HTML.
- Consider adding prop types or TypeScript interfaces for the component props.
- Add unit tests for the TimerSpeed component.
Overall, the component structure is good, but it needs refinement in functionality and best practices.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apps/client/src/features/control/playback/PlaybackControl.tsx (2 hunks)
- apps/client/src/features/control/playback/timer-speed/TimerSpeed.module.scss (1 hunks)
- apps/client/src/features/control/playback/timer-speed/TimerSpeed.tsx (1 hunks)
- apps/client/src/theme/ontimeButton.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/client/src/theme/ontimeButton.ts
Additional comments not posted (7)
apps/client/src/features/control/playback/PlaybackControl.tsx (2)
9-9: LGTM: TimerSpeed import added correctly.The import statement for the new TimerSpeed component is properly placed and follows the existing import style in the file.
Line range hint
13-30: Verify the purpose and impact of TimerSpeed in PlaybackControl.The addition of TimerSpeed enhances the functionality of the PlaybackControl component without disrupting the existing structure, which is good. However, it's important to ensure that this addition aligns with the component's overall purpose.
Please consider the following:
- Does TimerSpeed fit logically within the PlaybackControl component?
- Is there any potential impact on performance or user experience?
- Should TimerSpeed interact with other components like PlaybackButtons or PlaybackTimer?
To help verify these considerations, you can run the following script:
Verification successful
Integration of TimerSpeed in PlaybackControl Verified
The
TimerSpeedcomponent is well-integrated into thePlaybackControlcomponent. It operates with its own internal logic and event handlers, fitting logically within the existing structure without disrupting other components. While no performance optimization techniques were found, this does not impact the current integration. Consider exploring optimizations likeReact.memooruseMemoif performance becomes a concern.
PlaybackControlusesusePlaybackControlfor state management.TimerSpeedhas its own handlers:handleApply,handleReset,handleMeetSchedule.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the PlaybackControl component and its children # Test: Check for any shared state or props between components rg --type typescript 'const \w+ = use\w+' apps/client/src/features/control/playback # Test: Look for any performance optimization techniques rg --type typescript 'React.memo|useMemo|useCallback' apps/client/src/features/control/playback # Test: Check for any event handlers or functions that might be shared rg --type typescript 'function \w+\(|const \w+ = \(' apps/client/src/features/control/playbackLength of output: 385
Script:
#!/bin/bash # Description: Analyze the PlaybackControl component and its children # Test: Check for any shared state or props between components rg --glob '*.tsx' 'const \w+ = use\w+' apps/client/src/features/control/playback # Test: Look for any performance optimization techniques rg --glob '*.tsx' 'React.memo|useMemo|useCallback' apps/client/src/features/control/playback # Test: Check for any event handlers or functions that might be shared rg --glob '*.tsx' 'function \w+\(|const \w+ = \(' apps/client/src/features/control/playbackLength of output: 3075
apps/client/src/features/control/playback/timer-speed/TimerSpeed.module.scss (4)
1-6: LGTM: Panel container layout looks good.The
.panelContainerclass is well-structured using flexbox for layout with appropriate spacing.
15-17: LGTM: Highlight class is simple and effective.The
.highlightclass correctly sets the color for emphasized text.
36-42: LGTM: Speed override class is well-implemented.The
.speedOverrideclass effectively uses a CSS custom property for dynamic sizing, which is a good practice for flexibility and maintainability.
44-69: LGTM with a note: Labels styling is comprehensive, but consider edge cases.The
.labelsclass and its nested elements are well-structured for positioning and styling label elements.There's a TODO comment about accounting for the case where we translate all the way left. This edge case should be addressed:
Consider adding a minimum left position to prevent labels from going off-screen:
.labels { // ... existing styles ... > span { position: absolute; transform: translateX(-50%); + min-left: 0; // Prevent labels from going off-screen } // ... rest of the styles ... }apps/client/src/features/control/playback/timer-speed/TimerSpeed.tsx (1)
1-3: LGTM: Imports look good.The necessary components and styles are imported correctly.
| // TODO: extract and test | ||
| function mapRange(value: number, fromA_start: number, fromA_end: number, toB_start: number, toB_end: number): number { | ||
| return ((value - fromA_start) * (toB_end - toB_start)) / (fromA_end - fromA_start) + toB_start; | ||
| } |
There was a problem hiding this comment.
Address TODO: Extract and test mapRange function.
The mapRange function should be moved to a separate utility file and unit tested.
Would you like me to create a new utility file for this function and generate unit tests? I can also open a GitHub issue to track this task if you prefer.
| return ( | ||
| <div className={style.panelContainer}> | ||
| <div className={style.label}>Timer speed</div> | ||
| <div style={{ display: 'flex', gap: '1rem' }}> | ||
| <Button size='sm' variant='ontime-subtle-white' onClick={handleApply}> | ||
| Apply | ||
| </Button> | ||
| <Button size='sm' variant='ontime-subtle-white' onClick={handleReset}> | ||
| Reset | ||
| </Button> | ||
| <Button size='sm' variant='ontime-subtle-white' onClick={handleMeetSchedule}> | ||
| Meet schedule | ||
| </Button> | ||
| </div> | ||
| <div> | ||
| <span>1.0x</span> | ||
| <span>{'->'}</span> | ||
| <span className={style.highlight}>{`${newSpeed}x`}</span> | ||
| </div> | ||
| <div> | ||
| <div className={style.speedContainer}> | ||
| <div className={style.speedOverride} style={{ '--override': newSpeedIndicator }} /> | ||
| <div className={style.speedRegular} /> | ||
| </div> | ||
| <div className={style.labels}> | ||
| <span className={style.override} style={{ left: '66%' }}>{`${newSpeed}x`}</span> | ||
| <span>0.5x</span> | ||
| <span style={{ left: '33.33%' }}>1.0x</span> | ||
| <span style={{ left: '66.66%' }}>1.5x</span> | ||
| <span style={{ left: '100%' }}>2.0x</span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Improve component structure and accessibility.
- Move inline styles to the SCSS module:
- <div style={{ display: 'flex', gap: '1rem' }}>
+ <div className={style.buttonContainer}>Then add to your SCSS module:
.buttonContainer {
display: flex;
gap: 1rem;
}- Add aria labels to improve accessibility:
- <Button size='sm' variant='ontime-subtle-white' onClick={handleApply}>
+ <Button size='sm' variant='ontime-subtle-white' onClick={handleApply} aria-label="Apply new speed">
Apply
</Button>Make similar changes for the other buttons.
- Consider using semantic HTML elements where appropriate, such as
<header>for the "Timer speed" label.
| const handleApply = () => { | ||
| console.log('timerSpeedControl.apply'); | ||
| }; | ||
|
|
||
| const handleReset = () => { | ||
| console.log('timerSpeedControl.apply'); | ||
| }; | ||
|
|
||
| const handleMeetSchedule = () => { | ||
| console.log('timerSpeedControl.apply'); | ||
| }; |
There was a problem hiding this comment.
Implement handler functions.
The handler functions currently only log to the console. Implement the actual functionality for applying speed changes, resetting, and meeting the schedule.
Example for handleApply:
const handleApply = () => {
- console.log('timerSpeedControl.apply');
+ // TODO: Implement logic to apply the new speed
+ // For example:
+ // applyNewSpeed(newSpeed);
};Make similar changes for handleReset and handleMeetSchedule.
Committable suggestion was skipped due to low confidence.
| export default function TimerSpeed() { | ||
| const newSpeed = 1.23; | ||
| const newSpeedIndicator = mapRange(newSpeed, 0.5, 2.0, 0, 100); | ||
|
|
||
| console.log('newSpeedIndicator', newSpeedIndicator); | ||
|
|
There was a problem hiding this comment.
Replace hardcoded values with state management.
The newSpeed value is hardcoded, and newSpeedIndicator is calculated based on it. Consider using React state to manage these values dynamically.
- const newSpeed = 1.23;
- const newSpeedIndicator = mapRange(newSpeed, 0.5, 2.0, 0, 100);
+ const [newSpeed, setNewSpeed] = useState(1.0);
+ const newSpeedIndicator = useMemo(() => mapRange(newSpeed, 0.5, 2.0, 0, 100), [newSpeed]);Also, remove the console.log statement or replace it with proper error logging.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function TimerSpeed() { | |
| const newSpeed = 1.23; | |
| const newSpeedIndicator = mapRange(newSpeed, 0.5, 2.0, 0, 100); | |
| console.log('newSpeedIndicator', newSpeedIndicator); | |
| export default function TimerSpeed() { | |
| const [newSpeed, setNewSpeed] = useState(1.0); | |
| const newSpeedIndicator = useMemo(() => mapRange(newSpeed, 0.5, 2.0, 0, 100), [newSpeed]); | |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/client/src/features/control/playback/timer-speed/TimerSpeed.module.scss (1 hunks)
- apps/client/src/features/control/playback/timer-speed/TimerSpeed.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/client/src/features/control/playback/timer-speed/TimerSpeed.module.scss
Files skipped from review as they are similar to previous changes (1)
- apps/client/src/features/control/playback/timer-speed/TimerSpeed.tsx
cpvalente
force-pushed
the
timer-speed
branch
from
eed8c86 to
3226688
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- apps/client/src/common/hooks/useSocket.ts (1 hunks)
- apps/client/src/features/control/playback/timer-speed/TimerSpeed.tsx (1 hunks)
- apps/server/src/api-integration/integration.controller.ts (1 hunks)
- apps/server/src/services/runtime-service/RuntimeService.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/client/src/features/control/playback/timer-speed/TimerSpeed.tsx
Additional comments not posted (2)
apps/client/src/common/hooks/useSocket.ts (2)
117-123: LGTM!The code changes are approved.
125-129: LGTM!The code changes are approved.
| /* Speed */ | ||
| 'get-speed': () => { | ||
| const factor = runtimeService.getSpeed(); | ||
| return { payload: factor }; | ||
| }, | ||
| 'set-speed': (payload) => { | ||
| // TODO: validate payload type | ||
| const factor = runtimeService.setSpeed(payload as number); | ||
| return { payload: factor }; | ||
| }, | ||
| 'reset-speed': () => { | ||
| const factor = runtimeService.resetSpeed(); | ||
| return { payload: factor }; | ||
| }, |
There was a problem hiding this comment.
LGTM! Address the TODO comment.
The code changes are approved. The new action handlers are correctly implemented and follow the existing pattern of the actionHandlers object.
Please address the TODO comment in the set-speed handler to validate the payload type.
Do you want me to generate the payload validation code or open a GitHub issue to track this task?
| public getSpeed(): number { | ||
| // TODO: call function that calculates speed factor | ||
| console.log('runtimeService.getSpeed'); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Implement the speed calculation logic.
The getSpeed method is currently a placeholder that needs to be implemented to calculate the speed factor based on the remaining time and task duration.
Consider the following steps to implement the speed calculation logic:
- Retrieve the current time and the expected end time of the task.
- Calculate the remaining time by subtracting the current time from the end time.
- Retrieve the remaining duration of the task.
- Calculate the speed factor by dividing the remaining duration by the remaining time.
- Return the calculated speed factor.
For example:
public getSpeed(): number {
const currentTime = Date.now();
const endTime = /* retrieve end time */;
const remainingTime = endTime - currentTime;
const remainingDuration = /* retrieve remaining duration */;
const speedFactor = remainingDuration / remainingTime;
return speedFactor;
}| public setSpeed(time: number): number { | ||
| // TODO: validate state | ||
| // TODO: validate value | ||
| // TODO: call mutating function in state | ||
| console.log('runtimeService.setSpeed', time); | ||
| // TODO: return speed factor after mutating | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Implement the missing logic for setting the speed.
The setSpeed method needs to be implemented to validate the input, mutate the state, and return the applied speed factor. Consider the following suggestions:
- Validate the current state to ensure that setting the speed is allowed (e.g., check if a task is currently running).
- Validate the input time value to ensure it is within an acceptable range.
- Call a mutating function to update the speed factor in the state based on the input time.
- Return the applied speed factor after the mutation.
Here's an example of how the method could be implemented:
public setSpeed(time: number): number {
// Validate the current state
if (!this.isTaskRunning()) {
throw new Error('No task is currently running');
}
// Validate the input time value
if (time <= 0) {
throw new Error('Time must be a positive value');
}
// Call a mutating function to update the speed factor
const speedFactor = this.updateSpeedFactor(time);
// Return the applied speed factor
return speedFactor;
}Make sure to implement the necessary validation and mutation logic based on your specific requirements.
| public resetSpeed(): number { | ||
| // TODO: validate state | ||
| // TODO: call mutating function in state | ||
| console.log('runtimeService.resetSpeed'); | ||
| // TODO: return speed factor after mutating | ||
| return 1.0; | ||
| } |
There was a problem hiding this comment.
Implement the missing logic for resetting the speed.
The resetSpeed method needs to be implemented to validate the state, reset the speed factor to 1, and return the updated value. Consider the following suggestions:
- Validate the current state to ensure that resetting the speed is allowed (e.g., check if a task is currently running).
- Call a mutating function to reset the speed factor to 1 in the state.
- Return the updated speed factor after the mutation.
Here's an example of how the method could be implemented:
public resetSpeed(): number {
// Validate the current state
if (!this.isTaskRunning()) {
throw new Error('No task is currently running');
}
// Call a mutating function to reset the speed factor
const speedFactor = this.resetSpeedFactor();
// Return the updated speed factor
return speedFactor;
}Make sure to implement the necessary validation and mutation logic based on your specific requirements.
| return runtimeState.timer.speed; | ||
| } | ||
|
|
||
| export function calculateSpeed() { |
There was a problem hiding this comment.
please dont see these as answers, but as suggestions
Right now, we are attaching the functionality of time manipulation to a target time. I believe that we should not make the feature in a way that would stop us from doing some time manipulation on its own (ie, set timer to 1.2x)
With this assumption, this is a utility function that gives me the speed that I would need to finish on time. without making mutations to state.
With this in mind
// TODO: what should this return if no timer is running?
Good question.
Considering this as a utility function, I believe the appropriate behaviour is to give me a result, or a reason not to do so. In that case, it is appropriate to throw when the operation is invalid
// TODO: this can produce negative speeds (i.e. the desired finish time is in the past)
// TODO: should there be some clamping or rounding on this number?
I dont think we should do rounding.
Following the previous rule, I believe that we should throw if the desired finish time is in the past
There was a problem hiding this comment.
calculateSpeed does exactly that, simply returns the speed factor necessary to finish on the timer scheduled end. The user would then need to apply that speed factor (or any other speed factor they want). As far as the clamping/rounding the function can return something like 1.234567898765432 I guess the precision is probably "free" to carry around just might only need truncated in displaying to the user.
No description provided.