fix(web): timeline block speed #1233
Conversation
WalkthroughThis pull request involves the removal of specific functions related to time conversion from the Changes
Possibly related PRs
Suggested reviewers
Poem
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
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (3)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (2)
47-61: Optimize playSpeedOptions implementation and add validation.The current implementation has several potential improvements:
- Static options are recreated on every render
- No validation for multiplier value
- Potential confusion if multiplier matches a predefined speed
Consider this optimization:
+ const STATIC_SPEED_OPTIONS = [ + { timeString: "1sec/sec", seconds: 1 }, + { timeString: "0.5min/sec", seconds: 30 }, + { timeString: "1min/sec", seconds: 60 }, + { timeString: "0.1hr/sec", seconds: 360 }, + { timeString: "0.5hr/sec", seconds: 1800 }, + { timeString: "1hr/sec", seconds: 3600 } + ]; const playSpeedOptions = useMemo(() => { + // Validate multiplier is within reasonable bounds + const validMultiplier = multiplier > 0 && multiplier <= 7200 ? multiplier : 1; + return [ { timeString: "Not set", - seconds: multiplier + seconds: validMultiplier }, - { timeString: "1sec/sec", seconds: 1 }, - { timeString: "0.5min/sec", seconds: 30 }, - { timeString: "1min/sec", seconds: 60 }, - { timeString: "0.1hr/sec", seconds: 360 }, - { timeString: "0.5hr/sec", seconds: 1800 }, - { timeString: "1hr/sec", seconds: 3600 } + ...STATIC_SPEED_OPTIONS ]; }, [multiplier]);
43-61: Consider enhancing timeline speed management architecture.While the implementation achieves the goal of adding a "Not set" speed option, consider these architectural improvements:
- Create a dedicated TimelineSpeed type/interface to encapsulate speed-related logic
- Add validation rules for speed values
- Consider implementing a speed conversion utility for better maintainability
Example structure:
interface TimelineSpeed { timeString: string; seconds: number; isCustom?: boolean; } class TimelineSpeedManager { private static validateSpeed(speed: number): boolean { return speed > 0 && speed <= 7200; // 2 hours/second max } static createCustomSpeed(multiplier: number): TimelineSpeed { const validMultiplier = this.validateSpeed(multiplier) ? multiplier : 1; return { timeString: "Not set", seconds: validMultiplier, isCustom: true }; } }web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts (1)
94-94: Add type safety for speed options and improve validation.The speed selection logic could benefit from:
- A type-safe enum or const object for speed options
- Validation in
handleOnSelectto ensure selected values are validConsider adding:
// Speed options type type SpeedOption = { label: string; value: number; }; const SPEED_OPTIONS: readonly SpeedOption[] = [ { label: "Not set", value: 0 }, // or appropriate value // ... other options ] as const; // Update state type const [selected, setSelected] = useState<SpeedOption["label"]>("Not set"); // Update handler with validation const handleOnSelect = useCallback( (value: SpeedOption["label"], second: number) => { if (!inEditor) { setIsOpen(false); if (value !== selected && SPEED_OPTIONS.some(opt => opt.label === value)) { setSelected(value); const speedValue = isPlayingReversed ? second * -1 : second; onSpeedChange?.(speedValue, committer.id); } } }, [committer.id, inEditor, isPlayingReversed, onSpeedChange, selected] );Also applies to: 146-165
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts(1 hunks)web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts(0 hunks)web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts
🔇 Additional comments (2)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (1)
63-63: Verify speed state updates and timeline behavior.
The speed state initialization and update logic looks correct, but we should verify:
- Speed state updates correctly when the multiplier changes
- Timeline behaves correctly with the "Not set" speed option
Also applies to: 156-162
✅ Verification successful
Let me gather more specific information about the speed state implementation and tests.
Let me gather one final piece of information about the playSpeedOptions implementation to understand the "Not set" speed behavior.
Based on the gathered information, I can now provide a final response:
Speed state implementation is correct and handles all cases properly
The implementation correctly handles both requirements:
- Speed state updates properly with multiplier changes:
- Initial speed uses
playSpeedOptions[0].secondswhich is set to themultipliervalue for "Not set" option handleOnSpeedChangecorrectly updates the speed state and propagates changes viaonSpeedChangecallback
- Initial speed uses
- "Not set" speed option is properly implemented:
- It's the first option in
playSpeedOptionsarray - Its value is dynamically set to the current
multiplier - Timeline's speed state is initialized with this value
- It's the first option in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timeline speed-related tests and implementations
echo "Searching for timeline speed tests..."
rg -l "timeline.*speed|speed.*timeline" --type ts
echo "Searching for multiplier usage in tests..."
rg "multiplier" --type test
echo "Checking handleOnSpeedChange usage..."
ast-grep --pattern 'handleOnSpeedChange($$$)'
Length of output: 491
Script:
#!/bin/bash
# Search for speed-related state and handlers in the codebase
echo "Searching for speed state and handlers..."
rg -A 5 "const \[speed, setSpeed\]" --type ts
echo "Searching for playSpeedOptions..."
rg -A 3 "playSpeedOptions" --type ts
echo "Searching for speed-related test files..."
fd ".*\.test\.(ts|tsx)$" | xargs rg -l "speed"
echo "Looking for timeline speed change handlers..."
ast-grep --pattern 'const handleOnSpeedChange = $_'
Length of output: 6069
Script:
#!/bin/bash
# Check the complete implementation of playSpeedOptions
echo "Checking playSpeedOptions implementation..."
rg -A 10 "const playSpeedOptions = useMemo" --type ts
echo "Looking for speed option types..."
rg "interface.*Speed" --type ts
Length of output: 1372
web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts (1)
94-94: Ensure synchronization between "Not set" state and current visualizer speed.
The initial state change to "Not set" aligns with the PR objective. However, there should be a mechanism to ensure this state stays synchronized with visualizerContext?.current?.timeline?.current?.options.multiplier.
Let's verify the speed options handling:
✅ Verification successful
This will help us understand:
- How the hook is implemented and how it manages speed state
- How the Timeline component uses the hook
- The relationship between selected state and speed changes
This will help us understand:
- The complete implementation of the Timeline hook and how it manages the selected state
- The shared timeline hook implementation that defines the speed options and their synchronization
The initial state change to "Not set" is correct and properly synchronized with the visualizer speed
The implementation shows that:
- The
playSpeedOptionsinuseTimelineBlockhook correctly initializes with "Not set" option that uses the current visualizer multiplier value - The selected state in the Timeline hook is properly synchronized through
handleOnSelectwhich triggersonSpeedChangeto update the visualizer - The speed state is properly managed between the Timeline component and the visualizer context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for speed options definition and usage
echo "Searching for speed options definition..."
rg -A 5 "playSpeedOptions|speedOptions"
echo "Searching for multiplier usage..."
rg -A 5 "options.multiplier"
Length of output: 3299
Script:
#!/bin/bash
# Let's check the hook implementation and its usage
echo "Checking hook implementation..."
ast-grep --pattern 'const useTimelineBlock = $_' -A 30
echo "Checking Timeline component usage..."
rg -A 10 "useTimelineBlock|handleOnSelect.*timeString"
echo "Checking speed state management..."
rg -A 5 "setSelected.*timeString|handleOnSelect.*timeString"
Length of output: 6032
Script:
#!/bin/bash
# Let's check the implementation of the hook file and speed handling
echo "Checking Timeline hook implementation..."
cat web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts
echo "Checking shared timeline hook..."
cat web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Length of output: 18534
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (1)
27-38: Consider adding type safety to playSpeedOptions.The implementation correctly follows the requirement of using 0 for the "Not set" option. Consider these type-safety improvements:
+interface SpeedOption { + readonly timeString: string; + readonly seconds: number; +} -const playSpeedOptions = [ +const playSpeedOptions: readonly SpeedOption[] = [ { timeString: "Not set", seconds: 0 }, // ... rest of the options ] as const;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
web/src/beta/features/Editor/Map/DataSourceLayerCreator/CZML/hooks.tsx(1 hunks)web/src/beta/features/Editor/Map/DataSourceLayerCreator/KML/hooks.tsx(1 hunks)web/src/beta/features/Editor/Map/DataSourceLayerCreator/util.ts(1 hunks)web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts(0 hunks)web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts(2 hunks)web/vite.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts
✅ Files skipped from review due to trivial changes (4)
- web/src/beta/features/Editor/Map/DataSourceLayerCreator/CZML/hooks.tsx
- web/src/beta/features/Editor/Map/DataSourceLayerCreator/KML/hooks.tsx
- web/src/beta/features/Editor/Map/DataSourceLayerCreator/util.ts
- web/vite.config.ts
🧰 Additional context used
📓 Learnings (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (2)
Learnt from: mkumbobeaty
PR: reearth/reearth-visualizer#1233
File: web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts:47-59
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In the `useTimelineBlock.ts` file, for the "Not set" option in `playSpeedOptions`, we should set `seconds` to `0` and not use the current multiplier from `visualizerContext?.current?.timeline?.current?.options.multiplier`.
Learnt from: mkumbobeaty
PR: reearth/reearth-visualizer#1233
File: web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts:94-94
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts`, when the speed option is set to "Not set", we intentionally do not set any speed because we don't want to override the existing speed.
🔇 Additional comments (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (1)
157-170:
Improve speed change sequencing and error handling.
The current implementation has potential issues:
- Using
setTimeout(fn, 0)is an anti-pattern for sequencing operations - Race conditions could occur if multiple speed changes are triggered rapidly
- Error state might be inconsistent if the second speed change fails
Consider this more robust implementation:
const handleOnSpeedChange = useCallback(
(speed: number, committerId?: string) => {
try {
- onSpeedChange(TRANSITION_SPEED, committerId);
- setTimeout(() => {
+ // Use requestAnimationFrame for better sequencing
+ onSpeedChange(TRANSITION_SPEED, committerId);
+ requestAnimationFrame(() => {
onSpeedChange(speed, committerId);
setSpeed(speed);
- }, 0);
+ });
} catch (error) {
console.error("Error during speed change:", error);
- setSpeed(playSpeedOptions[0].seconds);
- throw error;
+ // Ensure consistent state
+ const defaultSpeed = playSpeedOptions[0].seconds;
+ onSpeedChange(defaultSpeed, committerId);
+ setSpeed(defaultSpeed);
}
},
- [onSpeedChange]
+ [onSpeedChange, playSpeedOptions]
);Let's verify the impact of rapid speed changes:
✅ Verification successful
Speed change implementation is acceptable.
No additional conflicting speed change handlers or potential race conditions were detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other speed change handlers that might conflict
rg -A 5 "onSpeedChange.*committerId"
# Look for potential race conditions in timeline commits
ast-grep --pattern 'commit({
cmd: "SET_OPTIONS",
$$$
})'
Length of output: 1733
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (2)
Learnt from: mkumbobeaty
PR: reearth/reearth-visualizer#1233
File: web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts:47-59
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In the `useTimelineBlock.ts` file, for the "Not set" option in `playSpeedOptions`, we should set `seconds` to `0` and not use the current multiplier from `visualizerContext?.current?.timeline?.current?.options.multiplier`.
Learnt from: mkumbobeaty
PR: reearth/reearth-visualizer#1233
File: web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts:94-94
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts`, when the speed option is set to "Not set", we intentionally do not set any speed because we don't want to override the existing speed.
🔇 Additional comments (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (1)
36-36:
Potential conflict between TRANSITION_SPEED and "Not set" speed
Setting TRANSITION_SPEED to 0 might conflict with the "Not set" option which should also use 0. Consider using a different transition value to avoid confusion.
-const TRANSITION_SPEED = 0;
+// Use a distinctive value to avoid confusion with "Not set" (0)
+const TRANSITION_SPEED = -1;
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
Outdated
Show resolved
Hide resolved
…/timeline-block-speed
…/timeline-block-speed
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (2)
Learnt from: mkumbobeaty
PR: reearth/reearth-visualizer#1233
File: web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts:47-59
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In the `useTimelineBlock.ts` file, for the "Not set" option in `playSpeedOptions`, we should set `seconds` to `0` and not use the current multiplier from `visualizerContext?.current?.timeline?.current?.options.multiplier`.
Learnt from: mkumbobeaty
PR: reearth/reearth-visualizer#1233
File: web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts:94-94
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Timeline/hook.ts`, when the speed option is set to "Not set", we intentionally do not set any speed because we don't want to override the existing speed.
🔇 Additional comments (1)
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (1)
27-34: 🛠️ Refactor suggestion
Move constant outside module and add "Not set" option.
The playSpeedOptions array should be:
- Moved outside the module scope as it's a static constant
- Include the "Not set" option as per PR objectives
+export const playSpeedOptions = [
+ { timeString: "Not set", seconds: 0 },
+ { timeString: "1sec/sec", seconds: 1 },
+ { timeString: "0.5min/sec", seconds: 30 },
+ { timeString: "1min/sec", seconds: 60 },
+ { timeString: "0.1hr/sec", seconds: 360 },
+ { timeString: "0.5hr/sec", seconds: 1800 },
+ { timeString: "1hr/sec", seconds: 3600 }
+];⛔ Skipped due to learnings
Learnt from: mkumbobeaty
PR: reearth/reearth-visualizer#1233
File: web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts:47-59
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In the `useTimelineBlock.ts` file, for the "Not set" option in `playSpeedOptions`, we should set `seconds` to `0` and not use the current multiplier from `visualizerContext?.current?.timeline?.current?.options.multiplier`.
| { timeString: "1hr/sec", seconds: 3600 } | ||
| ]; | ||
|
|
||
| const TRANSITION_SPEED = 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revise transition speed value and name.
The TRANSITION_SPEED constant:
- Should not be 0 as it could conflict with "Not set" option
- Name should reflect its purpose as an intermediate value
-const TRANSITION_SPEED = 0;
+// Intermediate speed value used to ensure speed changes are registered
+const INTERMEDIATE_SPEED = 100;📝 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.
| const TRANSITION_SPEED = 0; | |
| // Intermediate speed value used to ensure speed changes are registered | |
| const INTERMEDIATE_SPEED = 100; |
| (speed: number, committerId?: string) => { | ||
| onSpeedChange?.(speed, committerId); | ||
| setSpeed(speed); | ||
| try { | ||
| onSpeedChange(TRANSITION_SPEED, committerId); | ||
| setTimeout(() => { | ||
| onSpeedChange(speed, committerId); | ||
| setSpeed(speed); | ||
| }, 0); | ||
| } catch (error) { | ||
| setSpeed(playSpeedOptions[0].seconds); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Simplify speed change handling and respect "Not set" option.
The current implementation has several issues:
- Uses unnecessary
setTimeout - Error handling resets speed inappropriately
- Doesn't handle "Not set" option correctly
const handleOnSpeedChange = useCallback(
(speed: number, committerId?: string) => {
try {
+ // Don't override speed when "Not set" (0) is selected
+ if (speed === 0) return;
+
- onSpeedChange(TRANSITION_SPEED, committerId);
- setTimeout(() => {
- onSpeedChange(speed, committerId);
- setSpeed(speed);
- }, 0);
+ onSpeedChange(INTERMEDIATE_SPEED, committerId);
+ onSpeedChange(speed, committerId);
+ setSpeed(speed);
} catch (error) {
console.error("Error during speed change:", error);
- setSpeed(playSpeedOptions[0].seconds);
throw error;
}
},
[onSpeedChange]
);📝 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.
| (speed: number, committerId?: string) => { | |
| onSpeedChange?.(speed, committerId); | |
| setSpeed(speed); | |
| try { | |
| onSpeedChange(TRANSITION_SPEED, committerId); | |
| setTimeout(() => { | |
| onSpeedChange(speed, committerId); | |
| setSpeed(speed); | |
| }, 0); | |
| } catch (error) { | |
| setSpeed(playSpeedOptions[0].seconds); | |
| throw error; | |
| } | |
| (speed: number, committerId?: string) => { | |
| try { | |
| // Don't override speed when "Not set" (0) is selected | |
| if (speed === 0) return; | |
| onSpeedChange(INTERMEDIATE_SPEED, committerId); | |
| onSpeedChange(speed, committerId); | |
| setSpeed(speed); | |
| } catch (error) { | |
| console.error("Error during speed change:", error); | |
| throw error; | |
| } |
Overview
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores