feat(timeline): icon-only mode toggles and selection-aware region#133
Open
mahdyarief wants to merge 1 commit intowebadderall:mainfrom
Open
feat(timeline): icon-only mode toggles and selection-aware region#133mahdyarief wants to merge 1 commit intowebadderall:mainfrom
mahdyarief wants to merge 1 commit intowebadderall:mainfrom
Conversation
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Core Type Definition src/components/video-editor/types.ts |
Added exported TimeSelection interface with startMs and endMs properties. |
Keyboard Shortcuts src/lib/shortcuts.ts |
Added moveMode and selectMode shortcuts (bound to v and e keys respectively) with corresponding labels. |
VideoEditor State & Routing src/components/video-editor/VideoEditor.tsx |
Introduced timelineMode and timeSelection state; updated global keyboard handler to toggle modes and pass selection state to child components; selection-aware playback resumes from selection start when applicable. |
VideoPlayback & Event Handling src/components/video-editor/VideoPlayback.tsx, src/components/video-editor/videoPlayback/videoEventHandlers.ts |
Added timeSelection prop to VideoPlayback; extended createVideoEventHandlers to enforce loop-back (duration >100ms) or pause-at-boundary (duration ≤100ms) behavior when selection is active. |
Timeline Component Enhancements src/components/video-editor/timeline/Item.tsx, src/components/video-editor/timeline/TimelineEditor.tsx |
Extended Item to disable drag/drop in "select" mode and conditionally apply DnD handlers; substantially refactored TimelineEditor (+302/−48 lines) to add visual selection overlay, selection drag logic with pixel threshold, mode toggle buttons, selection-aware region placement, and PlaybackCursor snapping to selection boundaries. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant VideoEditor as VideoEditor<br/>(State)
participant TimelineEditor
participant Timeline
participant VideoPlayback
participant Video
User->>VideoEditor: Press 'e' (selectMode shortcut)
VideoEditor->>VideoEditor: Set timelineMode = 'select'
VideoEditor->>TimelineEditor: Pass timelineMode='select'
VideoEditor->>TimelineEditor: Pass onTimeSelectionChange callback
User->>TimelineEditor: Drag on timeline background
TimelineEditor->>TimelineEditor: Start selection drag<br/>(pointer-down threshold)
TimelineEditor->>TimelineEditor: Calculate timeSelection<br/>(startMs, endMs)
TimelineEditor->>TimelineEditor: Render selection overlay
TimelineEditor->>VideoEditor: Emit onTimeSelectionChange(selection)
VideoEditor->>VideoEditor: Update timeSelection state
VideoEditor->>VideoPlayback: Pass timeSelection prop
User->>VideoEditor: Press Space (play/pause)
VideoEditor->>VideoEditor: Call togglePlayPause()
alt Selection active & before startMs
VideoEditor->>Video: Seek to timeSelection.startMs
end
VideoEditor->>VideoPlayback: Resume playback
VideoPlayback->>Video: Update currentTime during playback
loop During playback with active selection
Video->>VideoPlayback: Emit timeupdate events
VideoPlayback->>VideoPlayback: Check if currentTime<br/>outside selection bounds
alt Duration > 100ms
VideoPlayback->>Video: Seek back to startMs
else Duration ≤ 100ms
VideoPlayback->>Video: Pause playback
end
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- Revert "feat: Advanced Video Editor Implementation, Native WGC Integration, and AI Auto-Captions" #124: Reverts the same code-level changes introduced here, including
TimeSelectiontype, time selection state propagation, video event handler loop logic, timeline mode toggling, and shortcut definitions.
Poem
🐰 A timeline now bends to my will,
With select mode, I pause or unfill,
Press 'e' to choose, press 'v' to move free,
Looping through moments with jubilee! ✨
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title specifically references the main UI improvements (icon-only mode toggles) and a key feature change (selection-aware region), both central to this PR's scope. |
| Description check | ✅ Passed | The description comprehensively addresses all template sections: clear motivation, type of change marked, screenshots/video provided, detailed testing guide, and all checklist items completed. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Comment @coderabbitai help to get the list of available commands and usage tips.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 416-421: The inline onMouseDown/onClick handlers on the axis are
manually invoking parent handlers ((e.currentTarget.parentElement as
any)?.__handleMouseDown and __handleTimelineClick), causing duplicate
invocations and noExplicitAny errors; remove these two handlers from the axis
JSX so events bubble naturally to the outer Timeline container (where
handleMouseDown and handleTimelineClick already run), and then delete the
useEffect that assigns __handleMouseDown / __handleTimelineClick onto
localTimelineRef.current so there is no manual axis-to-parent bridge.
- Around line 1708-1735: The icon-only mode buttons in TimelineEditor (the
Button instances that call onTimelineModeChange with 'move' and 'select' and
render MousePointer2/BoxSelect) lack accessible names and pressed state; add
explicit accessible attributes: provide aria-label (e.g. "Move tool (V)" and
"Select tool (E)"), and set aria-pressed to timelineMode === 'move' or
timelineMode === 'select' respectively so screen readers announce the active
state; ensure the title can remain but do not rely on it for accessibility.
In `@src/components/video-editor/VideoEditor.tsx`:
- Line 423: The editor-level timeSelection state (timeSelection /
setTimeSelection) persists across project/video loads and can reference ranges
invalid for the newly loaded asset; add a small reset function (e.g.,
resetTimelineInteractionState) that calls setTimeSelection(null) and resets
timelineMode via setTimelineMode("move"), and invoke this reset from the code
paths that replace the active video/project (for example inside
applyLoadedProject and other branches that load or replace the active video) so
the stale selection cannot affect playback or region placement on the new asset.
- Around line 2331-2335: The handler currently uses hard-coded "v" and "e"
checks which override user rebindings; remove the literal key comparisons and
only use matchesShortcut against shortcuts.moveMode and shortcuts.selectMode
(with isMac) before calling setTimelineMode, keeping the existing
isEditableTarget guard so the capture-phase listener won't flip modes for keys
that the user has rebound.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3b02f97b-d550-4ed2-89be-3ffeb972690c
📒 Files selected for processing (7)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/videoEventHandlers.tssrc/lib/shortcuts.ts
Comment on lines
+416
to
+421
| onMouseDown={(e) => { | ||
| (e.currentTarget.parentElement as any)?.__handleMouseDown?.(e); | ||
| }} | ||
| onClick={(e) => { | ||
| (e.currentTarget.parentElement as any)?.__handleTimelineClick?.(e); | ||
| }} |
There was a problem hiding this comment.
Remove the manual axis-to-parent event bridge.
handleMouseDown and handleTimelineClick already run on the outer Timeline container. Calling them manually here invokes them once with the axis as currentTarget and again when the event bubbles, so axis clicks can resolve the wrong time and selection drags can register duplicate global mouse listeners. This bridge is also what introduces the Biome noExplicitAny errors.
Suggested simplification
- onMouseDown={(e) => {
- (e.currentTarget.parentElement as any)?.__handleMouseDown?.(e);
- }}
- onClick={(e) => {
- (e.currentTarget.parentElement as any)?.__handleTimelineClick?.(e);
- }}Then delete the useEffect that assigns __handleMouseDown / __handleTimelineClick on localTimelineRef.current.
Also applies to: 665-668
🧰 Tools
🪛 Biome (2.4.9)
[error] 417-417: Unexpected any. Specify a different type.
(lint/suspicious/noExplicitAny)
[error] 420-420: Unexpected any. Specify a different type.
(lint/suspicious/noExplicitAny)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 416 -
421, The inline onMouseDown/onClick handlers on the axis are manually invoking
parent handlers ((e.currentTarget.parentElement as any)?.__handleMouseDown and
__handleTimelineClick), causing duplicate invocations and noExplicitAny errors;
remove these two handlers from the axis JSX so events bubble naturally to the
outer Timeline container (where handleMouseDown and handleTimelineClick already
run), and then delete the useEffect that assigns __handleMouseDown /
__handleTimelineClick onto localTimelineRef.current so there is no manual
axis-to-parent bridge.
Comment on lines
+1708
to
+1735
| <Button | ||
| onClick={() => onTimelineModeChange?.('move')} | ||
| variant="ghost" | ||
| size="icon" | ||
| className={cn( | ||
| "h-7 w-7 transition-all rounded-md border border-transparent", | ||
| timelineMode === 'move' | ||
| ? "bg-[#2563EB] text-white hover:bg-[#3b82f6] shadow-lg shadow-blue-500/20 border-white/10" | ||
| : "text-white/50 hover:text-white/80 hover:bg-white/5" | ||
| )} | ||
| title="Move Tools (V)" | ||
| > | ||
| <MousePointer2 className={cn("w-3.5 h-3.5", timelineMode === 'move' ? "text-white" : "text-white/40")} /> | ||
| </Button> | ||
| <Button | ||
| onClick={() => onTimelineModeChange?.('select')} | ||
| variant="ghost" | ||
| size="icon" | ||
| className={cn( | ||
| "h-7 w-7 transition-all rounded-md border border-transparent", | ||
| timelineMode === 'select' | ||
| ? "bg-[#2563EB] text-white hover:bg-[#3b82f6] shadow-lg shadow-blue-500/20 border-white/10" | ||
| : "text-white/50 hover:text-white/80 hover:bg-white/5" | ||
| )} | ||
| title="Select Tools (E)" | ||
| > | ||
| <BoxSelect className={cn("w-3.5 h-3.5", timelineMode === 'select' ? "text-white" : "text-white/40")} /> | ||
| </Button> |
There was a problem hiding this comment.
Add accessible toggle semantics to the icon-only mode buttons.
After removing the text labels, these controls only expose a title. Screen readers still need an accessible name and a pressed state to announce what each button does and which tool is active.
Suggested change
<Button
onClick={() => onTimelineModeChange?.('move')}
+ aria-label="Move tools"
+ aria-pressed={timelineMode === 'move'}
variant="ghost"
size="icon"
className={cn(
"h-7 w-7 transition-all rounded-md border border-transparent",
timelineMode === 'move'
? "bg-[`#2563EB`] text-white hover:bg-[`#3b82f6`] shadow-lg shadow-blue-500/20 border-white/10"
: "text-white/50 hover:text-white/80 hover:bg-white/5"
)}
title="Move Tools (V)"
>
...
<Button
onClick={() => onTimelineModeChange?.('select')}
+ aria-label="Select tools"
+ aria-pressed={timelineMode === 'select'}
variant="ghost"
size="icon"
className={cn(
"h-7 w-7 transition-all rounded-md border border-transparent",
timelineMode === 'select'
? "bg-[`#2563EB`] text-white hover:bg-[`#3b82f6`] shadow-lg shadow-blue-500/20 border-white/10"
: "text-white/50 hover:text-white/80 hover:bg-white/5"
)}
title="Select Tools (E)"
>📝 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.
Suggested change
| <Button | |
| onClick={() => onTimelineModeChange?.('move')} | |
| variant="ghost" | |
| size="icon" | |
| className={cn( | |
| "h-7 w-7 transition-all rounded-md border border-transparent", | |
| timelineMode === 'move' | |
| ? "bg-[#2563EB] text-white hover:bg-[#3b82f6] shadow-lg shadow-blue-500/20 border-white/10" | |
| : "text-white/50 hover:text-white/80 hover:bg-white/5" | |
| )} | |
| title="Move Tools (V)" | |
| > | |
| <MousePointer2 className={cn("w-3.5 h-3.5", timelineMode === 'move' ? "text-white" : "text-white/40")} /> | |
| </Button> | |
| <Button | |
| onClick={() => onTimelineModeChange?.('select')} | |
| variant="ghost" | |
| size="icon" | |
| className={cn( | |
| "h-7 w-7 transition-all rounded-md border border-transparent", | |
| timelineMode === 'select' | |
| ? "bg-[#2563EB] text-white hover:bg-[#3b82f6] shadow-lg shadow-blue-500/20 border-white/10" | |
| : "text-white/50 hover:text-white/80 hover:bg-white/5" | |
| )} | |
| title="Select Tools (E)" | |
| > | |
| <BoxSelect className={cn("w-3.5 h-3.5", timelineMode === 'select' ? "text-white" : "text-white/40")} /> | |
| </Button> | |
| <Button | |
| onClick={() => onTimelineModeChange?.('move')} | |
| aria-label="Move tools" | |
| aria-pressed={timelineMode === 'move'} | |
| variant="ghost" | |
| size="icon" | |
| className={cn( | |
| "h-7 w-7 transition-all rounded-md border border-transparent", | |
| timelineMode === 'move' | |
| ? "bg-[`#2563EB`] text-white hover:bg-[`#3b82f6`] shadow-lg shadow-blue-500/20 border-white/10" | |
| : "text-white/50 hover:text-white/80 hover:bg-white/5" | |
| )} | |
| title="Move Tools (V)" | |
| > | |
| <MousePointer2 className={cn("w-3.5 h-3.5", timelineMode === 'move' ? "text-white" : "text-white/40")} /> | |
| </Button> | |
| <Button | |
| onClick={() => onTimelineModeChange?.('select')} | |
| aria-label="Select tools" | |
| aria-pressed={timelineMode === 'select'} | |
| variant="ghost" | |
| size="icon" | |
| className={cn( | |
| "h-7 w-7 transition-all rounded-md border border-transparent", | |
| timelineMode === 'select' | |
| ? "bg-[`#2563EB`] text-white hover:bg-[`#3b82f6`] shadow-lg shadow-blue-500/20 border-white/10" | |
| : "text-white/50 hover:text-white/80 hover:bg-white/5" | |
| )} | |
| title="Select Tools (E)" | |
| > | |
| <BoxSelect className={cn("w-3.5 h-3.5", timelineMode === 'select' ? "text-white" : "text-white/40")} /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 1708 -
1735, The icon-only mode buttons in TimelineEditor (the Button instances that
call onTimelineModeChange with 'move' and 'select' and render
MousePointer2/BoxSelect) lack accessible names and pressed state; add explicit
accessible attributes: provide aria-label (e.g. "Move tool (V)" and "Select tool
(E)"), and set aria-pressed to timelineMode === 'move' or timelineMode ===
'select' respectively so screen readers announce the active state; ensure the
title can remain but do not rely on it for accessibility.
| const [isPreviewReady, setIsPreviewReady] = useState(false); | ||
| const [autoSuggestZoomsTrigger, setAutoSuggestZoomsTrigger] = useState(0); | ||
| const [timelineMode, setTimelineMode] = useState<'move' | 'select'>('move'); | ||
| const [timeSelection, setTimeSelection] = useState<TimeSelection | null>(null); |
There was a problem hiding this comment.
Reset timeSelection when loading a different asset.
This new editor-wide state survives the project/video load paths below, so a selection from the previous asset can still drive playback and region placement on the next one. If the new video is shorter, the stale range can even sit past totalMs and make add-region actions fail immediately.
Possible fix
const resetTimelineInteractionState = () => {
setTimeSelection(null);
setTimelineMode("move");
};
// Call this from applyLoadedProject(...) and the branches that replace the active video/project.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoEditor.tsx` at line 423, The editor-level
timeSelection state (timeSelection / setTimeSelection) persists across
project/video loads and can reference ranges invalid for the newly loaded asset;
add a small reset function (e.g., resetTimelineInteractionState) that calls
setTimeSelection(null) and resets timelineMode via setTimelineMode("move"), and
invoke this reset from the code paths that replace the active video/project (for
example inside applyLoadedProject and other branches that load or replace the
active video) so the stale selection cannot affect playback or region placement
on the new asset.
Comment on lines
+2331
to
+2335
| if (!isEditableTarget) { | ||
| if (key === "v" || matchesShortcut(e, shortcuts.moveMode, isMac)) { | ||
| setTimelineMode("move"); | ||
| } else if (key === "e" || matchesShortcut(e, shortcuts.selectMode, isMac)) { | ||
| setTimelineMode("select"); |
There was a problem hiding this comment.
Use only the configured mode bindings here.
moveMode and selectMode were added to the shortcut config, but these literal v/e checks keep both keys permanently reserved. After rebinding, this capture-phase handler can still flip the mode while TimelineEditor runs the user-configured action in its own keydown listener.
Suggested change
- if (key === "v" || matchesShortcut(e, shortcuts.moveMode, isMac)) {
+ if (matchesShortcut(e, shortcuts.moveMode, isMac)) {
setTimelineMode("move");
- } else if (key === "e" || matchesShortcut(e, shortcuts.selectMode, isMac)) {
+ } else if (matchesShortcut(e, shortcuts.selectMode, isMac)) {
setTimelineMode("select");
}📝 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.
Suggested change
| if (!isEditableTarget) { | |
| if (key === "v" || matchesShortcut(e, shortcuts.moveMode, isMac)) { | |
| setTimelineMode("move"); | |
| } else if (key === "e" || matchesShortcut(e, shortcuts.selectMode, isMac)) { | |
| setTimelineMode("select"); | |
| if (!isEditableTarget) { | |
| if (matchesShortcut(e, shortcuts.moveMode, isMac)) { | |
| setTimelineMode("move"); | |
| } else if (matchesShortcut(e, shortcuts.selectMode, isMac)) { | |
| setTimelineMode("select"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoEditor.tsx` around lines 2331 - 2335, The
handler currently uses hard-coded "v" and "e" checks which override user
rebindings; remove the literal key comparisons and only use matchesShortcut
against shortcuts.moveMode and shortcuts.selectMode (with isMac) before calling
setTimelineMode, keeping the existing isEditableTarget guard so the
capture-phase listener won't flip modes for keys that the user has rebound.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Enhances the timeline editor UI and user experience by transitioning to icon-only mode toggles and making region addition context-aware of time selections.
Motivation
Type of Change
Screenshots / Video
Icon-Only Mode Toggles:
The "MOVE" and "SELECT" buttons have been updated to use icons only (
MousePointer2andBoxSelect) for a cleaner look.Screenshot (if applicable):
Testing Guide
Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features