fix bug where timeline name wasn't being forwarded in seek utils #4975
Conversation
WalkthroughThe pull request introduces modifications to the ESLint configuration and 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/playback/eslint.config.mjs (1)
13-18: LGTM! Good addition of React Hooks linting.The React Hooks plugin configuration is properly structured and will help catch common hooks-related issues, which is particularly relevant for the timeline name forwarding changes in this PR.
This addition will help enforce React Hooks best practices across the codebase, potentially preventing similar bugs in the future.
app/packages/playback/src/views/Timeline.tsx (1)
Line range hint
29-115: Consider memoizing more callback functions.While the critical callbacks are memoized, there are inline handlers like
setIsHoveringSeekBarthat could benefit from memoization to prevent unnecessary re-renders.Consider applying this optimization:
+ const handleMouseEnter = React.useCallback(() => setIsHoveringSeekBar(true), []); + const handleMouseLeave = React.useCallback(() => setIsHoveringSeekBar(false), []); return ( <FoTimelineContainer ref={ref} style={style} - onMouseEnter={() => setIsHoveringSeekBar(true)} - onMouseLeave={() => setIsHoveringSeekBar(false)} + onMouseEnter={handleMouseEnter} + onMouseLeave={handleMouseLeave} data-cy="imavid-container" data-timeline-name={name} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
app/packages/playback/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (2)
- app/packages/playback/eslint.config.mjs (1 hunks)
- app/packages/playback/src/views/Timeline.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/playback/src/views/Timeline.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (4)
app/packages/playback/eslint.config.mjs (1)
1-2: LGTM! Clean import structure for ESLint plugins.The imports are well-organized and properly separated from existing imports.
app/packages/playback/src/views/Timeline.tsx (3)
29-30: Props handling refactor looks good.The explicit destructuring of timelineProps improves code clarity while maintaining type safety.
36-36: Good fix: Timeline name is now properly forwarded.The change to pass
nametouseTimelineVizUtilsand derivingseekBarValuefromgetSeekValue()fixes the bug mentioned in the PR title where timeline name wasn't being forwarded correctly.Also applies to: 38-38
57-57: Fixed potential stale closure bug in seek handlers.Adding
nameto the dependency arrays ofonSeekStartandonSeekEndensures that the event handlers always reference the current timeline name. This prevents potential bugs where old timeline names could be referenced after props change.Let's verify there are no other event handlers missing the name dependency:
Also applies to: 65-65
✅ Verification successful
Let me try a different approach to verify the event handlers in the Timeline component.
All event handlers with timeline name dependencies are properly configured
After analyzing the Timeline.tsx file, I can confirm that all event handlers that use the
nameprop have it correctly included in their dependency arrays:
onSeekStart: Correctly includes[pause, name]onSeekEnd: Correctly includes[name]onChangeSeek: Only depends on[seekTo]and doesn't usenamedirectlyThe other React hooks in the component either don't use the
nameprop in their callback body or are not event handlers. The review comment's concern about stale closures has been properly addressed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other event handlers in Timeline.tsx that might need the name dependency ast-grep --pattern 'React.useCallback(($_) => { $$$ name $$$ }, [$$$])'Length of output: 79
Script:
#!/bin/bash # Get the content of Timeline.tsx to analyze useCallback usage fd "Timeline.tsx" --exec cat {}Length of output: 3880
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/packages/playback/src/views/Timeline.tsx (1)
57-57: Consider extracting event name as a constantWhile the event handling is correct, the 'seek' event name should be extracted as a constant to maintain consistency and prevent typos.
Add this at the top of the file:
const SEEK_EVENT = 'seek';Then update the event dispatches:
-new CustomEvent("seek", { +new CustomEvent(SEEK_EVENT, {Also applies to: 65-65
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
app/packages/playback/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (2)
- app/packages/playback/eslint.config.mjs (1 hunks)
- app/packages/playback/src/views/Timeline.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/playback/src/views/Timeline.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (5)
app/packages/playback/eslint.config.mjs (2)
1-2: LGTM: Import statements are properly organizedThe imports for
fixupConfigRulesandhooksPluginare correctly added to support the React Hooks linting configuration.
13-18: LGTM: React Hooks linting configuration is properly implementedThe configuration correctly integrates the React Hooks plugin using the recommended ruleset, which will help enforce React Hooks best practices across the codebase.
app/packages/playback/src/views/Timeline.tsx (3)
29-30: LGTM: Props handling improvementThe refactoring of props handling improves code clarity while maintaining proper TypeScript typing.
36-36: LGTM: Bug fix for timeline name forwardingSuccessfully addresses the reported bug by correctly forwarding the timeline name to the hook.
Line range hint
29-115: Verify the timeline name propagationThe implementation looks correct. Let's verify that the timeline name is properly propagated through all necessary paths.
✅ Verification successful
Let me try a different approach to verify the timeline name propagation.
Timeline name propagation is correctly implemented
Based on the verification results, the timeline name is properly propagated throughout the component hierarchy and related utilities:
- The
nameprop is consistently passed down to all child components viadata-timeline-name- Timeline-related hooks (
useTimeline,useFrameNumber,useTimelineVizUtils,useTimelineBuffers) correctly receive and utilize the timeline name- Custom events for play, pause, and seek operations properly include the timeline name in their payload
- The implementation maintains proper synchronization between different components using the same timeline name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any other potential timeline name usage patterns rg -t typescript -t tsx "timelineName|timeline.*name" app/packages/playback/src/Length of output: 117
Script:
#!/bin/bash # Search for timeline name usage with ripgrep without file type filters rg "timelineName|timeline.*name" app/packages/playback/src/ # Also search for the specific hooks being used rg "useTimeline|useFrameNumber|useTimelineVizUtils|useTimelineBuffers" app/packages/playback/src/ # Look for any custom events related to timeline rg "CustomEvent.*timeline" app/packages/playback/src/Length of output: 17960
|
|
||
| const seekBarValue = React.useMemo(() => getSeekValue(), [frameNumber]); | ||
| const seekBarValue = React.useMemo(() => getSeekValue(), [getSeekValue]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing unnecessary memoization
The useMemo might be unnecessary if getSeekValue is already stable (e.g., from useCallback or another hook). Consider measuring performance impact before and after memoization.
-const seekBarValue = React.useMemo(() => getSeekValue(), [getSeekValue]);
+const seekBarValue = getSeekValue();📝 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 seekBarValue = React.useMemo(() => getSeekValue(), [getSeekValue]); | |
| const seekBarValue = getSeekValue(); |
* fix eslint * forward timeline name
Summary by CodeRabbit
New Features
Bug Fixes
Refactor