fix: resolve timeline scroll sync and audio cursor issues (#481, #490) #492
Conversation
…app#481) - Separate horizontal and vertical scroll sync mechanisms to prevent race conditions - Fix inconsistent cleanup logic for event listeners - Add robust error handling with try-catch blocks - Improve code documentation and maintainability Fixes OpenCut-app#481
|
@naoNao89 is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
WalkthroughThe Timeline component's scroll synchronization logic was refactored to separate horizontal and vertical sync into two independent effects. Each direction now uses its own throttling ref and event listeners, with error handling and cleanup. This change isolates horizontal and vertical syncing, addressing race conditions and improving reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TimelineComponent
participant RulerViewport
participant TracksViewport
participant TrackLabelsViewport
User->>RulerViewport: Scroll horizontally
RulerViewport->>TimelineComponent: onHorizontalScroll event
TimelineComponent->>TracksViewport: Sync horizontal scrollLeft
User->>TracksViewport: Scroll horizontally
TracksViewport->>TimelineComponent: onHorizontalScroll event
TimelineComponent->>RulerViewport: Sync horizontal scrollLeft
User->>TracksViewport: Scroll vertically
TracksViewport->>TimelineComponent: onVerticalScroll event
TimelineComponent->>TrackLabelsViewport: Sync vertical scrollTop
User->>TrackLabelsViewport: Scroll vertically
TrackLabelsViewport->>TimelineComponent: onVerticalScroll event
TimelineComponent->>TracksViewport: Sync vertical scrollTop
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
apps/web/src/components/editor/timeline/index.tsx (1)
482-538: Excellent implementation of independent vertical scroll synchronization.This new effect correctly addresses the core issue #481 by providing dedicated vertical scroll sync with its own throttling state. The bidirectional synchronization between track labels and tracks content is well-implemented.
Consider using separate timing refs for each scroll handler to avoid potential timing conflicts:
+ const lastTrackLabelsSync = useRef(0); + const lastTracksVerticalSync = useRef(0); const handleTrackLabelsScroll = () => { const now = Date.now(); - if (isUpdatingVerticalRef.current || now - lastVerticalSync.current < 16) + if (isUpdatingVerticalRef.current || now - lastTrackLabelsSync.current < 16) return; - lastVerticalSync.current = now; + lastTrackLabelsSync.current = now; // ... rest of handler }; const handleTracksVerticalScroll = () => { const now = Date.now(); - if (isUpdatingVerticalRef.current || now - lastVerticalSync.current < 16) + if (isUpdatingVerticalRef.current || now - lastTracksVerticalSync.current < 16) return; - lastVerticalSync.current = now; + lastTracksVerticalSync.current = now; // ... rest of handler };This would provide even more precise throttling control for each direction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/components/editor/timeline/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{jsx,tsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include alangattribute on the html element.
Always include atitleattribute for iframe elements.
AccompanyonClickwith at least one of:onKeyUp,onKeyDown, oronKeyPress.
AccompanyonMouseOver/onMouseOutwithonFocus/onBlur.
Include caption tracks for audio and video elements.
Use semantic elements instead of role attributes in JSX.
Make sure all anchors are valid and navigable.
Ensure all ARIA properties (aria-*) are valid.
Use valid, non-abstract ARIA roles for elements with...
Files:
apps/web/src/components/editor/timeline/index.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use the comma operator.
Don't write functions that exceed a given Cognitive Complexity score.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use uselessthisaliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names that aren't base 10 or use underscore separators.
Remove redundant terms from logical expressions.
Use while loops instead of for loops when you don't need initializer and update expressions.
Don't reassign const variables....
Files:
apps/web/src/components/editor/timeline/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and...
Files:
apps/web/src/components/editor/timeline/index.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.961Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
apps/web/src/components/editor/timeline/index.tsx (3)
Learnt from: simonorzel26
PR: #324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.961Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded ml-48 class in addition to the calculated leftPosition for proper alignment. This is intentional and needed for the new timeline layout.
Learnt from: CR
PR: OpenCut-app/OpenCut#0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-07-27T22:15:27.748Z
Learning: Applies to **/*.{jsx,tsx} : Only use the scope prop on <th> elements.
Learnt from: CR
PR: OpenCut-app/OpenCut#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-27T22:14:46.402Z
Learning: Applies to **/*.{jsx,tsx} : Only use the scope prop on <th> elements.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (2)
apps/web/src/components/editor/timeline/index.tsx (2)
117-121: LGTM! Excellent separation of concerns.The introduction of separate refs for horizontal and vertical scroll synchronization correctly addresses the race condition issue described in #481. Using independent throttling state prevents the conflicts that occurred with the shared
isUpdatingRefapproach.
427-480: Horizontal Scroll Synchronization Is Well-ImplementedThe
useEffectcleanly sets up bidirectional scroll syncing between the ruler and tracks with:
- A 16 ms throttle (≈60 fps) using
isUpdatingHorizontalRefand timestamp checks- Try-catch blocks that guard against runtime errors
- Proper teardown of event listeners on unmount
A quick search didn’t reveal any other throttling or debounce constants in the repo, so this appears to be the sole timing control. Please manually verify that the 16 ms interval provides smooth scrolling on lower-end devices.
- Use actual content duration instead of timeline duration for playback stopping - Add getEffectivePlaybackDuration() method with comprehensive error handling - Expose timeline store globally to avoid circular dependencies - Maintain full backward compatibility for all existing functionality Fixes OpenCut-app#490: Audio files shorter than 10 seconds now stop at actual duration instead of continuing until the 10-second timeline minimum.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/src/stores/playback-store.ts (1)
186-186: Consider using consistent console method for warnings.The coding guidelines specify "Don't use console" but these console.warn statements appear to be intentional debugging aids. However, consider whether these warnings should use a proper logging system in production or be conditionally enabled based on environment.
If you want to maintain these warnings, consider using a consistent logging approach:
- console.warn("Timeline store getState is not a function, using fallback duration"); + if (process.env.NODE_ENV === 'development') { + console.warn("Timeline store getState is not a function, using fallback duration"); + }Also applies to: 197-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/stores/playback-store.ts(3 hunks)apps/web/src/stores/timeline-store.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/stores/timeline-store.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use the comma operator.
Don't write functions that exceed a given Cognitive Complexity score.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use uselessthisaliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names that aren't base 10 or use underscore separators.
Remove redundant terms from logical expressions.
Use while loops instead of for loops when you don't need initializer and update expressions.
Don't reassign const variables....
Files:
apps/web/src/stores/playback-store.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and...
Files:
apps/web/src/stores/playback-store.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.961Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest)
🔇 Additional comments (4)
apps/web/src/stores/playback-store.ts (4)
4-16: LGTM! Well-structured type definitions for avoiding circular dependencies.The approach of using global type declarations to avoid circular imports between stores is appropriate. The interface definitions are clear and properly typed.
21-21: LGTM! Clean interface extension.The addition of
getEffectivePlaybackDurationmethod to the interface is straightforward and follows TypeScript conventions.
38-46: LGTM! Improved playback stopping logic with clear documentation.The change from using
state.durationtostate.getEffectivePlaybackDuration()directly addresses the issue where audio shorter than 10 seconds would continue playing. The detailed comment explains the fix well.
158-231: Excellent implementation with comprehensive error handling and validation.The
getEffectivePlaybackDurationmethod is well-implemented with:
- Proper error handling using try-catch
- Multiple validation checks for type safety
- Clear fallback behavior
- Detailed JSDoc documentation
- Appropriate console warnings for debugging
The method correctly handles edge cases like:
- Missing timeline store
- Invalid function types
- Non-numeric, NaN, or infinite duration values
- Zero or negative durations
The implementation follows defensive programming principles and provides excellent observability through console warnings.
naoNao89
changed the title
1 participant
Summary
This PR addresses two critical issues in the OpenCut editor:
🎯 Issue #481: Timeline vertical scroll synchronization
🎯 Issue #490: Audio cursor not stopping at end of file
Changes Made
Timeline Scroll Fix (#481)
apps/web/src/components/editor/timeline/index.tsxAudio Cursor Fix (#490)
Modified:
apps/web/src/stores/playback-store.tsgetEffectivePlaybackDuration()method with comprehensive error handlingModified:
apps/web/src/stores/timeline-store.tsQuality Assurance
✅ Code Quality
✅ Functionality Testing
✅ Performance & Reliability
Root Cause Analysis
Timeline Scroll Issue (#481)
Audio Cursor Issue (#490)
Testing
Manual Testing Completed
Browser Testing
Risk Assessment
Implementation Highlights
Timeline Scroll Fix
isUpdatingHorizontalRefandisUpdatingVerticalRefprevent race conditionsAudio Cursor Fix
getEffectivePlaybackDuration()safely accesses timeline store via global referenceCloses
Additional Notes