Skip to content

feat(timeline): icon-only mode toggles and selection-aware region #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mahdyarief wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(timeline): icon-only mode toggles and selection-aware region #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/components/video-editor/VideoEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ import {
type ZoomFocus,
type ZoomRegion,
type ZoomTransitionEasing,
type TimeSelection,
} from "./types";
import VideoPlayback, { VideoPlaybackRef } from "./VideoPlayback";
import {
Expand Down Expand Up @@ -418,6 +419,8 @@ export default function VideoEditor() {
const [previewVersion, setPreviewVersion] = useState(0);
const [isPreviewReady, setIsPreviewReady] = useState(false);
const [autoSuggestZoomsTrigger, setAutoSuggestZoomsTrigger] = useState(0);
const [timelineMode, setTimelineMode] = useState<'move' | 'select'>('move');
const [timeSelection, setTimeSelection] = useState<TimeSelection | null>(null);
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

const headerLeftControlsPaddingClass = appPlatform === "darwin" ? "pl-[76px]" : "";

const videoPlaybackRef = useRef<VideoPlaybackRef>(null);
Expand Down Expand Up @@ -1879,6 +1882,14 @@ export default function VideoEditor() {
if (!video.paused && !video.ended) {
playback.pause();
} else {
// Selection awareness: if playing with a selection active, jump to start if out of bounds
if (timeSelection) {
const currentTimeMs = Math.round(currentTime * 1000);
const bufferMs = 50; // Small buffer for end boundary
if (currentTimeMs < timeSelection.startMs || currentTimeMs >= timeSelection.endMs - bufferMs) {
handleSeek(timeSelection.startMs / 1000);
}
}
playback.play().catch((err) => console.error("Video play failed:", err));
}
}
Expand Down Expand Up @@ -2315,14 +2326,13 @@ export default function VideoEditor() {
return;
}
e.preventDefault();

const playback = videoPlaybackRef.current;
if (playback?.video) {
if (playback.video.paused) {
playback.play().catch(console.error);
} else {
playback.pause();
}
togglePlayPause();
}
if (!isEditableTarget) {
if (key === "v" || matchesShortcut(e, shortcuts.moveMode, isMac)) {
setTimelineMode("move");
} else if (key === "e" || matchesShortcut(e, shortcuts.selectMode, isMac)) {
setTimelineMode("select");
Comment on lines +2331 to +2335
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}
}
};
Expand Down Expand Up @@ -3300,6 +3310,7 @@ export default function VideoEditor() {
cursorClickBounce={cursorClickBounce}
cursorClickBounceDuration={cursorClickBounceDuration}
cursorSway={cursorSway}
timeSelection={timeSelection}
volume={previewVolume}
/>
</div>
Expand Down Expand Up @@ -3341,6 +3352,10 @@ export default function VideoEditor() {
videoDuration={duration}
currentTime={currentTime}
onSeek={handleSeek}
timelineMode={timelineMode}
onTimelineModeChange={setTimelineMode}
timeSelection={timeSelection}
onTimeSelectionChange={setTimeSelection}
cursorTelemetry={normalizedCursorTelemetry}
autoSuggestZoomsTrigger={autoSuggestZoomsTrigger}
zoomRegions={effectiveZoomRegions}
Expand Down
6 changes: 6 additions & 0 deletions src/components/video-editor/VideoPlayback.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
type CursorStyle,
type WebcamOverlaySettings,
type ZoomTransitionEasing,
type TimeSelection,
} from "./types";
import {
DEFAULT_FOCUS,
Expand Down Expand Up @@ -202,6 +203,7 @@ interface VideoPlaybackProps {
cursorClickBounce?: number;
cursorClickBounceDuration?: number;
cursorSway?: number;
timeSelection?: TimeSelection | null;
volume?: number;
}

Expand Down Expand Up @@ -269,6 +271,7 @@ const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>(
cursorClickBounce = DEFAULT_CURSOR_CLICK_BOUNCE,
cursorClickBounceDuration = DEFAULT_CURSOR_CLICK_BOUNCE_DURATION,
cursorSway = DEFAULT_CURSOR_SWAY,
timeSelection = null,
volume = 1,
},
ref,
Expand Down Expand Up @@ -323,6 +326,8 @@ const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>(
width: number;
height: number;
} | null>(null);
const timeSelectionRef = useRef<TimeSelection | null>(null);
timeSelectionRef.current = timeSelection;
const layoutVideoContentRef = useRef<(() => void) | null>(null);
const trimRegionsRef = useRef<TrimRegion[]>([]);
const speedRegionsRef = useRef<SpeedRegion[]>([]);
Expand Down Expand Up @@ -1183,6 +1188,7 @@ const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>(
onTimeUpdate,
trimRegionsRef,
speedRegionsRef,
timeSelectionRef,
});

video.addEventListener("play", handlePlay);
Expand Down
28 changes: 19 additions & 9 deletions src/components/video-editor/timeline/Item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ interface ItemProps {
zoomDepth?: number;
speedValue?: number;
variant?: 'zoom' | 'trim' | 'annotation' | 'speed' | 'audio';
timelineMode?: 'move' | 'select';
}

// Map zoom depth to multiplier labels
Expand Down Expand Up @@ -46,11 +47,13 @@ export default function Item({
zoomDepth = 1,
speedValue,
variant = "zoom",
timelineMode = "move",
children,
}: ItemProps) {
const { setNodeRef, attributes, listeners, itemStyle, itemContentStyle } = useItem({
id,
span,
disabled: timelineMode !== 'move',
data: { rowId },
});

Expand Down Expand Up @@ -91,33 +94,40 @@ export default function Item({
<div
ref={setNodeRef}
style={safeItemStyle}
{...listeners}
{...attributes}
{...(timelineMode === 'move' ? listeners : {})}
{...(timelineMode === 'move' ? attributes : {})}
onPointerDownCapture={() => onSelect?.()}
onMouseDown={(e) => {
if (timelineMode === 'select') {
e.stopPropagation();
}
}}
className="group h-full"
>
<div className="h-full" style={{ ...itemContentStyle, minWidth: 24, height: "100%" }}>
<div
className={cn(
glassClass,
"w-full h-full overflow-hidden flex items-center justify-center gap-1.5 cursor-grab active:cursor-grabbing relative",
"w-full h-full overflow-hidden flex items-center justify-center gap-1.5 active:cursor-grabbing relative",
timelineMode === 'move' ? "cursor-grab" : "cursor-default",
isSelected && glassStyles.selected
)}
style={{ height: "100%", minHeight: 22, color: '#fff', minWidth: 24 }}
onClick={(event) => {
onClick={(event) => {
// Prevent the timeline background's onClick (seeking) from firing
// when we click on a track item. onSelect is already fired on pointer-down capture.
event.stopPropagation();
onSelect?.();
}}
>
<div
className={cn(glassStyles.zoomEndCap, glassStyles.left)}
style={{ cursor: 'col-resize', pointerEvents: 'auto', width: 8, opacity: 0.9, background: endCapColor }}
title="Resize left"
style={{ cursor: timelineMode === 'move' ? 'col-resize' : 'default', pointerEvents: timelineMode === 'move' ? 'auto' : 'none', width: 8, opacity: 0.9, background: endCapColor }}
title={timelineMode === 'move' ? "Resize left" : undefined}
/>
<div
className={cn(glassStyles.zoomEndCap, glassStyles.right)}
style={{ cursor: 'col-resize', pointerEvents: 'auto', width: 8, opacity: 0.9, background: endCapColor }}
title="Resize right"
style={{ cursor: timelineMode === 'move' ? 'col-resize' : 'default', pointerEvents: timelineMode === 'move' ? 'auto' : 'none', width: 8, opacity: 0.9, background: endCapColor }}
title={timelineMode === 'move' ? "Resize right" : undefined}
/>
{/* Content */}
<div className="relative z-10 flex flex-col items-center justify-center text-white/90 opacity-80 group-hover:opacity-100 transition-opacity select-none overflow-hidden">
Expand Down
Loading