feat: overhaul timeline drag/resize, zoom playback, and recording hooks#248
feat: overhaul timeline drag/resize, zoom playback, and recording hooks#248webadderall merged 5 commits intomainfrom
Conversation
- TimelineEditor: rewrite drag/resize with neighbour clamping — items cannot overlap; drag preserves original duration; resize clamps to adjacent item boundaries instead of rejecting; floating drag/resize tooltip via direct DOM (no re-renders) - TimelineWrapper: new wrapper component for cleaner TimelineContext wiring - Track: extract Track component with empty-row hint slot - AudioWaveform, Row, Subrow, KeyframeMarkers, Timeline.module.css: dark theme timeline overhaul - zoomSuggestionUtils: smarter auto-zoom region suggestion from cursor telemetry - videoPlayback (zoom): fix zoomRegionUtils edge cases at timeline bounds; rework zoomTransform for cursor-follow camera - videoPlayback (cursor): cursorFollowCamera, cursorSway, cursorLoopTelemetry, cursorViewport — new cursor motion and snapping pipeline - videoPlayback (layout): layoutUtils, overlayUtils, focusUtils, motionSmoothing - useScreenRecorder: hardened recording lifecycle, pause segment tracking, improved WGC and SCK integration - useVideoDevices / useMicrophoneDevices / useAudioLevelMeter improvements
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Timeline Item & Styles src/components/video-editor/timeline/Item.tsx, src/components/video-editor/timeline/ItemGlass.module.css |
Adds optional `zoomMode?: "auto" |
Timeline Canvas & Small UI src/components/video-editor/timeline/AudioWaveform.tsx, src/components/video-editor/timeline/KeyframeMarkers.tsx, src/components/video-editor/timeline/Row.tsx, src/components/video-editor/timeline/Subrow.tsx, src/components/video-editor/timeline/Timeline.module.css, src/components/video-editor/timeline/TimelineWrapper.tsx |
Waveform stroke color changed from rgba(59,130,246,0.45) to rgba(255,255,255,0.55). Import/formatting tweaks in Keyframe/Row/Subrow. TimelineWrapper: adds resizeHandleWidth={28}, simplifies range clamping logic, memoizes tooltip formatter. New scrollbar styles in Timeline.module.css. |
New Track Component src/components/video-editor/timeline/Track.tsx |
Adds Track default export that wraps useRow, exposes hint, isEmpty, trackStyle, and renders sidebar + row with optional centered hint overlay. |
Zoom Timing & Region Logic src/components/video-editor/videoPlayback/zoomRegionUtils.ts, src/components/video-editor/videoPlayback/zoomAnimation.test.ts |
Introduces ZOOM_ANIMATION_LEAD_MS = 200, increases ZOOM_IN_OVERLAP_MS 500→1000, shifts connected-region timing/strength windows and updates tests to match lead-offset timing changes. |
Zoom Constant Change src/components/video-editor/videoPlayback/constants.ts |
ZOOM_OUT_EARLY_START_MS changed from 0 to 500. |
Zoom Transform Motion Helper src/components/video-editor/videoPlayback/zoomTransform.ts |
Adds internal resetMotionEffects helper and refactors applyZoomTransform to use it when clearing motion/blur state. |
Playback & Cursor Utilities (formatting & minor refactors) src/components/video-editor/videoPlayback/... (multiple files) |
Widespread formatting/import/quote normalization and small local refactors across many playback utilities and tests; mostly non-functional edits except timing/lead and the new helper noted above. |
Hooks & Device Management src/hooks/useAudioLevelMeter.ts, src/hooks/useMicrophoneDevices.ts, src/hooks/useScreenRecorder.ts, src/hooks/useVideoDevices.ts, src/hooks/useScreenRecorder.test.ts |
useVideoDevices adds active-load gating to avoid stale async updates. useScreenRecorder introduces typed desktop capture handling, restructures mic sidecar and recorder stop/recovery flows, and adjusts webcam stop handling; tests added for stopRecording resume-error tolerance. |
Suggestion & Telemetry Utilities (formatting) src/components/video-editor/timeline/zoomSuggestionUtils.ts, src/components/video-editor/videoPlayback/*Telemetry.ts |
Import/order/formatting normalization; no changes to core suggestion/telemetry algorithms. |
Sequence Diagram(s)
(omitted)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~40 minutes
Possibly related PRs
- webadderall/Recordly#122: touches the same timeline/video-editor components (Item, KeyframeMarkers, TimelineWrapper, Track), indicating direct overlap.
- webadderall/Recordly#124: modifies the Item component and related timeline UI, likely overlapping the zoom/UI changes here.
- webadderall/Recordly#246: also updates timeline Item rendering/iconography and may conflict with style/layout edits in this PR.
Suggested labels
Checked
Poem
🐇
I hopped through timelines, nudged a seam,
Swapped glass for hush and tuned the gleam,
Waves now whisper soft in white,
Tracks stand ready, handles light,
A tiny hop — the editor’s dream.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 9.88% 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 clearly summarizes the main changes: timeline drag/resize overhaul, zoom playback improvements, and recording hooks refactoring. It is concise and directly reflects the primary work in the changeset. |
| Description check | ✅ Passed | The PR description is well-structured with a clear summary and detailed breakdown of changes across multiple system areas. All required template sections are addressed: motivation is implicit in the 'why' of each change, type is feature-focused, and testing guidance is provided through the detailed change descriptions. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/timeline-editor
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
src/components/video-editor/videoPlayback/cursorLoopTelemetry.ts (1)
49-67: Extract transient-interaction filtering into a shared helper.The click/double-click/right-click/middle-click exclusion logic is duplicated in two functions; centralizing it will reduce drift risk.
♻️ Suggested refactor
+function isTransientInteraction(interactionType: CursorTelemetryPoint["interactionType"]) { + return ( + interactionType === "click" || + interactionType === "double-click" || + interactionType === "right-click" || + interactionType === "middle-click" + ); +} + function findFirstStableCursorType(samples: CursorTelemetryPoint[]) { for (const sample of samples) { if (!sample.cursorType) { continue; } - if ( - sample.interactionType === "click" || - sample.interactionType === "double-click" || - sample.interactionType === "right-click" || - sample.interactionType === "middle-click" - ) { + if (isTransientInteraction(sample.interactionType)) { continue; } return sample.cursorType; } @@ - if ( - sample.interactionType === "click" || - sample.interactionType === "double-click" || - sample.interactionType === "right-click" || - sample.interactionType === "middle-click" - ) { + if (isTransientInteraction(sample.interactionType)) { continue; }Also applies to: 91-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/videoPlayback/cursorLoopTelemetry.ts` around lines 49 - 67, The duplicate check that excludes transient interactions (interactionType === "click" | "double-click" | "right-click" | "middle-click") should be extracted into a shared predicate (e.g., isTransientInteraction or isTransientClick) and used from findFirstStableCursorType and the other function with the same logic; update findFirstStableCursorType to call the helper instead of repeating the four comparisons, and replace the corresponding duplicated block in the other occurrence (the function around lines 91-126) to call the helper as well so the exclusion logic is centralized and maintainable.src/components/video-editor/videoPlayback/videoEventHandlers.ts (1)
63-79: Extract shared trim-skip behavior into one helper.The skip/clamp/pause flow is duplicated in
updateTimeandhandleSeeked. A single helper would prevent behavior drift and make future playback changes safer.Also applies to: 116-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/videoPlayback/videoEventHandlers.ts` around lines 63 - 79, Extract the duplicated "skip/clamp/pause" trim behavior into a single helper (e.g., applyTrimSkip(video, activeTrimRegion, currentTimeMs, emitTime)) and call that helper from both updateTime and handleSeeked; the helper should encapsulate: compute skipToTime = activeTrimRegion.endMs/1000, if skipToTime >= video.duration then pause, else set video.currentTime = skipToTime and call emitTime(skipToTime). Leave playbackRate setting and emitTime(video.currentTime) in the non-trim path (or return a flag from the helper indicating a skip occurred so callers can avoid duplicate emitTime). Ensure all references to activeTrimRegion, video, currentTimeMs, emitTime, updateTime and handleSeeked are updated to use the helper and remove the duplicated code blocks.src/hooks/useVideoDevices.ts (1)
92-96: Consider adding in-flight protection for overlapping device loads.Multiple rapid
devicechangeevents could trigger overlappingloadDevicescalls. While themountedflag prevents stale updates, adding a simple in-flight flag would prevent redundant permission requests and enumerations.💡 Optional improvement: add in-flight flag
let mounted = true; + let loading = false; const loadDevices = async () => { + if (loading) return; + loading = true; let permissionStream: MediaStream | null = null; try { setIsLoading(true); setError(null); // ... rest of logic } catch (error) { // ... error handling } finally { permissionStream?.getTracks().forEach((track) => track.stop()); + loading = false; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useVideoDevices.ts` around lines 92 - 96, Add an in-flight guard to prevent overlapping loadDevices calls: introduce a boolean (e.g., isLoading) scoped alongside mounted and set it to true at the start of loadDevices and false when it completes (ensuring false is set in all exit paths), then skip early if isLoading is already true; update handleDeviceChange (and any direct calls to loadDevices) to respect this guard so concurrent devicechange events don't trigger redundant permission requests/enumerations. Ensure you reference and modify the existing loadDevices function and handleDeviceChange closure and keep the mounted checks intact.src/hooks/useScreenRecorder.ts (3)
796-797: Consider replacing blockingalert()calls with toast notifications.The code uses
alert()in several places, which blocks the UI and provides a poor user experience. Since the codebase already uses thetoastlibrary from "sonner" (imported at line 3), consider usingtoast.error()ortoast.warning()for these user notifications instead.💡 Example refactor for one alert
if (!selectedSource) { - alert("Please select a source to record"); + toast.error("Please select a source to record"); return; }Also applies to: 1002-1004, 1046-1048, 1213-1217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 796 - 797, Replace blocking alert() calls in useScreenRecorder.ts with non-blocking sonner toast notifications: import toast (already present) and call toast.error(...) or toast.warning(...) instead of alert(...) in the locations within the start/selection/stop flows (the alert at the shown diff plus the other occurrences around lines noted in the review). Keep the existing control flow (e.g., return after notifying), and ensure each message preserves the original text but uses toast.error or toast.warning for user-friendly, non-blocking feedback; update any tests or UI expectations accordingly.
784-784: Consider wrappingstartRecordinginuseCallbackfor consistency.The
startRecordingfunction is not wrapped inuseCallback, unlike other helper functions in this hook (e.g.,preparePermissions,cleanupCapturedMedia,finalizeRecordingSession). While this might not cause immediate issues since it's only called fromtoggleRecording, wrapping it would provide consistency and prevent potential problems if the function is later used as a dependency or passed as a prop.Additionally, the function is ~441 lines long, which makes it difficult to test and maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` at line 784, The startRecording function is not wrapped in useCallback like other helpers (preparePermissions, cleanupCapturedMedia, finalizeRecordingSession) which risks inconsistency when used as a dependency or prop; wrap startRecording with useCallback and ensure its dependency array only includes stable refs/state it actually uses (or use refs for mutable values) and break the ~441-line body into smaller helper functions (e.g., initializeMediaSources, startTimers, attachEventHandlers) called from the new useCallback to keep startRecording short and testable.
70-1385: Consider extracting some logic into smaller hooks or utilities.The
useScreenRecorderhook is ~1385 lines and handles many concerns: platform detection, permissions, source resolution, native/browser recording, webcam recording, audio mixing, lifecycle management, error handling, and session management.While the current implementation works, consider extracting some independent concerns into separate hooks or utility modules to improve testability and maintainability:
useRecordingPermissionsfor platform-specific permission checksuseWebcamRecorderfor webcam recording coordinationaudioMixingUtilsfor audio context setup and mixingrecordingStorageUtilsfor file storage and finalization🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 70 - 1385, The useScreenRecorder hook is doing too many unrelated tasks; extract platform permission checks (preparePermissions, buildNativeCaptureFailureMessage, logNativeCaptureDiagnostics) into a new useRecordingPermissions hook, move webcam acquisition/recorder lifecycle (prepareWebcamRecorder, beginWebcamCapture, stopWebcamRecorder, webcam-related refs like webcamStream/webcamRecorder/webcamChunks/webcamStartTime/webcamStopPromise) into a useWebcamRecorder hook, pull audio mixing and context setup (mixingContext, MIC_GAIN_BOOST logic, creation of MediaStreamAudioSourceNodes and destination) into audioMixingUtils, and isolate storage/finalization (finalizeRecordingSession, calls to window.electronAPI.storeRecordedVideo/storeMicrophoneSidecar/muxNativeWindowsRecording) into recordingStorageUtils; keep startRecording/stop/cancel/pause/resume logic in the hook but call these new utilities/hooks, wire their APIs to accept/return the minimal inputs (streams, blobs, timestamps, options) and ensure tests/mocks can target each unit separately.src/components/video-editor/videoPlayback/zoomRegionUtils.ts (1)
214-228: Consider precomputing connected-pair lookups outside this selector.
findDominantRegion()now sortsregions, rebuilds pair metadata, and then pays repeated linear.find()scans throughconnectedPairson every call. If this runs per playback frame, the cost grows to roughlyO(n log n + n²)per tick. Memoizing the sorted pairs and indexing them by region id would keep this path much cheaper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/videoPlayback/zoomRegionUtils.ts` around lines 214 - 228, findDominantRegion repeatedly recomputes and linearly scans connectedPairs each tick (constructed by getConnectedRegionPairs), causing O(n log n + n²) per frame; precompute and memoize the connected-pair lookups (and sorted regions) outside the selector and index them by region id so runtime calls become O(1)/O(k) instead of repeated full scans. Concretely: compute sorted regions and connectedPairs once (e.g., when regions change), build a Map from region id to its connected partner(s) and any transition/hold metadata, then update findDominantRegion to accept or reference that precomputed map instead of calling getConnectedRegionPairs/getConnectedRegionTransition/getConnectedRegionHold/getActiveRegion on raw arrays every frame.src/components/video-editor/videoPlayback/zoomAnimation.test.ts (1)
352-391: Add boundary assertions for the new+200mshandoff window.These tests only sample inside the connected transition. They never check the first frame between
endMsandtransitionStart, or the first frame aftertransitionEnd, which are the two places most likely to regress with the new timing offset. A couple of explicit assertions there would lock down the continuity guarantees this change is trying to provide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/videoPlayback/zoomAnimation.test.ts` around lines 352 - 391, Add explicit boundary assertions around the new +200ms handoff window: for tests using findDominantRegion and connectZooms, test the first frame immediately after a.endMs (e.g., a.endMs + 1) to assert there is no active region (result.region === null), and test the first frame immediately after the transitionEnd (e.g., transitionEnd + 1) to assert the next region is held (result.region.id === "b" and result.strength === 1). Use the existing ZoomRegion objects and times computed from their endMs / transitionStart / transitionEnd to locate these checks so the test locks the exact boundary behavior.src/components/video-editor/timeline/TimelineWrapper.tsx (1)
347-355: Remove unreachable post-clamp boundary branch.After
const clamped = clampRange(desired);on Line 347,clamped.end > totalMsshould not occur. This branch is dead and can be removed to simplify the path.Suggested simplification
if (totalMs > 0) { const clamped = clampRange(desired); - - if (clamped.end > totalMs) { - const span = Math.min(clamped.end - clamped.start, totalMs); - return { - start: Math.max(0, totalMs - span), - end: totalMs, - }; - } - return clamped; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineWrapper.tsx` around lines 347 - 355, Remove the unreachable post-clamp boundary branch: after calling clampRange(desired) the condition clamped.end > totalMs is impossible, so delete the entire if (clamped.end > totalMs) { ... } block and return clamped (or continue using clamped) directly. Update any surrounding logic that relied on that branch so functions using clampRange(desired), clamped, and totalMs simply use the clamped result without the extra span-adjustment branch.
🤖 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/Item.tsx`:
- Around line 90-116: The visible-content wrapper forces a 24px minimum
(minWidth: 24) which makes short items render wider than their actual span;
update the inner wrapper inside the return (the div using itemContentStyle) to
use the shared MIN_ITEM_PX constant (or remove the hardcoded 24) so its minWidth
matches safeItemStyle (use minWidth: MIN_ITEM_PX), and ensure the glass wrapper
(the div using glassClass) keeps overflow-hidden so the visible glass is clipped
to the real item bounds (verify the class includes overflow-hidden); change any
hardcoded 24 references to MIN_ITEM_PX and keep existing overflow-hidden to
contain overflow and avoid stealing pointer events.
In `@src/components/video-editor/timeline/ItemGlass.module.css`:
- Around line 7-9: Remove the extra blank line before the transition
declarations in the ItemGlass CSS blocks so the transition property directly
follows the previous declaration (e.g., change the block where "transition:
background 0.15s ease, border-color 0.15s ease;" currently has a blank line
above it); apply the same removal to the other glass class blocks that contain
transition (the ones referenced around the other transition occurrences) so
there is no empty line before any "transition" declaration to satisfy the
declaration-empty-line-before rule.
In `@src/components/video-editor/timeline/KeyframeMarkers.tsx`:
- Around line 84-93: The onMouseDown handler currently sets
setDraggingKeyframeId for any mouse button, so right-clicks start a drag; change
the onMouseDown logic to only start dragging for the primary button (check
e.button === 0) while still stopping propagation and selecting the keyframe
(setSelectedKeyframeId). In short: inside the onMouseDown handler, keep
e.stopPropagation() and setSelectedKeyframeId(kf.id) but only call
setDraggingKeyframeId(kf.id) when e.button === 0 so right-click/context-menu
selection doesn’t trigger dragging.
In `@src/components/video-editor/videoPlayback/videoEventHandlers.ts`:
- Around line 64-73: The trim-skip branch pauses the player when skipToTime >=
video.duration without clamping currentTime or emitting the clamped time, which
leaves the UI timeline inconsistent; update the branch in the activeTrimRegion
check so that before calling video.pause() you set video.currentTime =
Math.min(skipToTime, video.duration) (or video.currentTime = video.duration),
call emitTime(video.currentTime) to emit the clamped endpoint, then pause; apply
the same change to the other equivalent trim-skip block (the later branch
handling trim skips around the other activeTrimRegion check) so both paths clamp
currentTime and emit before pausing (references: activeTrimRegion, skipToTime,
video.currentTime, emitTime, video.pause()).
In `@src/components/video-editor/videoPlayback/zoomRegionUtils.ts`:
- Around line 108-120: The suppression currently uses currentRegion.endMs and
transitionEnd which makes the handoff non-continuous; update the logic to key
off the pair.transitionStart: for outgoingPair, only zero its strength when
timeMs >= outgoingPair.transitionStart; for incomingPair, suppress it while
timeMs < incomingPair.transitionStart, and once timeMs >=
incomingPair.transitionStart return strength 1 (do not fall through to
computeRegionStrength). Adjust the checks around
connectedPairs/currentRegion/nextRegion to use pair.transitionStart and keep
computeRegionStrength only for regions not part of an active handoff.
In `@src/components/video-editor/videoPlayback/zoomTransform.ts`:
- Around line 147-154: The early-return in zoomTransform when stageSize or
baseMask dimensions are invalid returns an identity transform but skips clearing
motion-blur/filters, causing stale state to leak; before returning { scale: 1,
x: 0, y: 0 } call the module's existing filter-reset routine (e.g.,
resetMotionBlur / clearFilters / resetFilterState on the stage or mask) to clear
any blur/filter state, and make the same change for the other early-return
branch that checks geometry (the block around the later 227–239 region) so both
paths reset filters before returning.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 624-627: The pause-tracking is updated before attempting to resume
the underlying recorder, which can leave state inconsistent if recorder.resume()
throws; update the logic in useScreenRecorder so that you call recorder.resume()
first and only call markRecordingResumed(Date.now()) after resume succeeds, or
wrap recorder.resume() in a try/catch and only call markRecordingResumed on
success (and rollback or log on failure); locate the block checking
recorderState === "paused" that references recorder.resume() and
markRecordingResumed and apply this change.
In `@src/hooks/useVideoDevices.ts`:
- Around line 43-57: The flag handling around hasRequestedVideoLabels is wrong:
move setting of hasRequestedVideoLabels to after a successful
navigator.mediaDevices.getUserMedia call (and after enumerateDevices) so a
thrown error doesn't block future retries; in useVideoDevices.ts wrap the await
navigator.mediaDevices.getUserMedia(...) and subsequent
enumerateDevices/device-mapping in a try/catch, only set hasRequestedVideoLabels
= true on success, ensure permissionStream is closed/its tracks stopped in
finally or on error, and on error leave hasRequestedVideoLabels false so
needsLabelPermission checks (and later user-granted permissions) can retry.
---
Nitpick comments:
In `@src/components/video-editor/timeline/TimelineWrapper.tsx`:
- Around line 347-355: Remove the unreachable post-clamp boundary branch: after
calling clampRange(desired) the condition clamped.end > totalMs is impossible,
so delete the entire if (clamped.end > totalMs) { ... } block and return clamped
(or continue using clamped) directly. Update any surrounding logic that relied
on that branch so functions using clampRange(desired), clamped, and totalMs
simply use the clamped result without the extra span-adjustment branch.
In `@src/components/video-editor/videoPlayback/cursorLoopTelemetry.ts`:
- Around line 49-67: The duplicate check that excludes transient interactions
(interactionType === "click" | "double-click" | "right-click" | "middle-click")
should be extracted into a shared predicate (e.g., isTransientInteraction or
isTransientClick) and used from findFirstStableCursorType and the other function
with the same logic; update findFirstStableCursorType to call the helper instead
of repeating the four comparisons, and replace the corresponding duplicated
block in the other occurrence (the function around lines 91-126) to call the
helper as well so the exclusion logic is centralized and maintainable.
In `@src/components/video-editor/videoPlayback/videoEventHandlers.ts`:
- Around line 63-79: Extract the duplicated "skip/clamp/pause" trim behavior
into a single helper (e.g., applyTrimSkip(video, activeTrimRegion,
currentTimeMs, emitTime)) and call that helper from both updateTime and
handleSeeked; the helper should encapsulate: compute skipToTime =
activeTrimRegion.endMs/1000, if skipToTime >= video.duration then pause, else
set video.currentTime = skipToTime and call emitTime(skipToTime). Leave
playbackRate setting and emitTime(video.currentTime) in the non-trim path (or
return a flag from the helper indicating a skip occurred so callers can avoid
duplicate emitTime). Ensure all references to activeTrimRegion, video,
currentTimeMs, emitTime, updateTime and handleSeeked are updated to use the
helper and remove the duplicated code blocks.
In `@src/components/video-editor/videoPlayback/zoomAnimation.test.ts`:
- Around line 352-391: Add explicit boundary assertions around the new +200ms
handoff window: for tests using findDominantRegion and connectZooms, test the
first frame immediately after a.endMs (e.g., a.endMs + 1) to assert there is no
active region (result.region === null), and test the first frame immediately
after the transitionEnd (e.g., transitionEnd + 1) to assert the next region is
held (result.region.id === "b" and result.strength === 1). Use the existing
ZoomRegion objects and times computed from their endMs / transitionStart /
transitionEnd to locate these checks so the test locks the exact boundary
behavior.
In `@src/components/video-editor/videoPlayback/zoomRegionUtils.ts`:
- Around line 214-228: findDominantRegion repeatedly recomputes and linearly
scans connectedPairs each tick (constructed by getConnectedRegionPairs), causing
O(n log n + n²) per frame; precompute and memoize the connected-pair lookups
(and sorted regions) outside the selector and index them by region id so runtime
calls become O(1)/O(k) instead of repeated full scans. Concretely: compute
sorted regions and connectedPairs once (e.g., when regions change), build a Map
from region id to its connected partner(s) and any transition/hold metadata,
then update findDominantRegion to accept or reference that precomputed map
instead of calling
getConnectedRegionPairs/getConnectedRegionTransition/getConnectedRegionHold/getActiveRegion
on raw arrays every frame.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 796-797: Replace blocking alert() calls in useScreenRecorder.ts
with non-blocking sonner toast notifications: import toast (already present) and
call toast.error(...) or toast.warning(...) instead of alert(...) in the
locations within the start/selection/stop flows (the alert at the shown diff
plus the other occurrences around lines noted in the review). Keep the existing
control flow (e.g., return after notifying), and ensure each message preserves
the original text but uses toast.error or toast.warning for user-friendly,
non-blocking feedback; update any tests or UI expectations accordingly.
- Line 784: The startRecording function is not wrapped in useCallback like other
helpers (preparePermissions, cleanupCapturedMedia, finalizeRecordingSession)
which risks inconsistency when used as a dependency or prop; wrap startRecording
with useCallback and ensure its dependency array only includes stable refs/state
it actually uses (or use refs for mutable values) and break the ~441-line body
into smaller helper functions (e.g., initializeMediaSources, startTimers,
attachEventHandlers) called from the new useCallback to keep startRecording
short and testable.
- Around line 70-1385: The useScreenRecorder hook is doing too many unrelated
tasks; extract platform permission checks (preparePermissions,
buildNativeCaptureFailureMessage, logNativeCaptureDiagnostics) into a new
useRecordingPermissions hook, move webcam acquisition/recorder lifecycle
(prepareWebcamRecorder, beginWebcamCapture, stopWebcamRecorder, webcam-related
refs like
webcamStream/webcamRecorder/webcamChunks/webcamStartTime/webcamStopPromise) into
a useWebcamRecorder hook, pull audio mixing and context setup (mixingContext,
MIC_GAIN_BOOST logic, creation of MediaStreamAudioSourceNodes and destination)
into audioMixingUtils, and isolate storage/finalization
(finalizeRecordingSession, calls to
window.electronAPI.storeRecordedVideo/storeMicrophoneSidecar/muxNativeWindowsRecording)
into recordingStorageUtils; keep startRecording/stop/cancel/pause/resume logic
in the hook but call these new utilities/hooks, wire their APIs to accept/return
the minimal inputs (streams, blobs, timestamps, options) and ensure tests/mocks
can target each unit separately.
In `@src/hooks/useVideoDevices.ts`:
- Around line 92-96: Add an in-flight guard to prevent overlapping loadDevices
calls: introduce a boolean (e.g., isLoading) scoped alongside mounted and set it
to true at the start of loadDevices and false when it completes (ensuring false
is set in all exit paths), then skip early if isLoading is already true; update
handleDeviceChange (and any direct calls to loadDevices) to respect this guard
so concurrent devicechange events don't trigger redundant permission
requests/enumerations. Ensure you reference and modify the existing loadDevices
function and handleDeviceChange closure and keep the mounted checks intact.
🪄 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: 81144645-3868-4c84-a679-9882b8c9b628
📒 Files selected for processing (32)
src/components/video-editor/timeline/AudioWaveform.tsxsrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/ItemGlass.module.csssrc/components/video-editor/timeline/KeyframeMarkers.tsxsrc/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/Subrow.tsxsrc/components/video-editor/timeline/Timeline.module.csssrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/TimelineWrapper.tsxsrc/components/video-editor/timeline/Track.tsxsrc/components/video-editor/timeline/zoomSuggestionUtils.tssrc/components/video-editor/videoPlayback/constants.tssrc/components/video-editor/videoPlayback/cursorFollowCamera.tssrc/components/video-editor/videoPlayback/cursorLoopTelemetry.tssrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/components/video-editor/videoPlayback/cursorSway.tssrc/components/video-editor/videoPlayback/cursorViewport.test.tssrc/components/video-editor/videoPlayback/cursorViewport.tssrc/components/video-editor/videoPlayback/focusUtils.tssrc/components/video-editor/videoPlayback/index.tssrc/components/video-editor/videoPlayback/layoutUtils.tssrc/components/video-editor/videoPlayback/mathUtils.tssrc/components/video-editor/videoPlayback/motionSmoothing.tssrc/components/video-editor/videoPlayback/overlayUtils.tssrc/components/video-editor/videoPlayback/videoEventHandlers.tssrc/components/video-editor/videoPlayback/zoomAnimation.test.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.tssrc/components/video-editor/videoPlayback/zoomTransform.tssrc/hooks/useAudioLevelMeter.tssrc/hooks/useMicrophoneDevices.tssrc/hooks/useScreenRecorder.tssrc/hooks/useVideoDevices.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/components/video-editor/videoPlayback/zoomAnimation.test.ts (1)
314-320: Test variable naming is misleading but test logic is correct.The local
zoomOutStart = region.endMs - 150doesn't match the implementation's calculation (region.endMs - ZOOM_OUT_EARLY_START_MSwhereZOOM_OUT_EARLY_START_MS = 500). While the test passes because the sampled time (4850 + 700 = 5550ms) falls within the zoom-out transition window, the variable name is confusing.Consider renaming for clarity:
it("falls smoothly during zoom-out", () => { // Zoom-out now starts 200ms later than the original timing. - const zoomOutStart = region.endMs - 150; - const s = computeRegionStrength(region, zoomOutStart + 700); + // Sample a point within the zoom-out transition + const sampleTime = region.endMs + 200; // well into zoom-out phase + const s = computeRegionStrength(region, sampleTime); expect(s).toBeGreaterThan(0); expect(s).toBeLessThan(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/videoPlayback/zoomAnimation.test.ts` around lines 314 - 320, The test's local variable zoomOutStart is misleading because the implementation uses region.endMs - ZOOM_OUT_EARLY_START_MS (ZOOM_OUT_EARLY_START_MS = 500); update the test in zoomAnimation.test.ts to either (A) compute the start the same way as the implementation (const zoomOutStart = region.endMs - ZOOM_OUT_EARLY_START_MS) or (B) rename the variable to indicate it's an adjusted sample point (e.g., zoomOutSamplePoint or sampleTimeOffset) and keep the current numeric offset; ensure the call to computeRegionStrength(region, ...) uses the renamed/updated variable so the test clearly matches intent.
🤖 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/Item.tsx`:
- Around line 104-105: The item is invoking onSelect twice because
onPointerDownCapture already calls onSelect and an inner element also calls
onSelect onClick; remove the redundant onClick handlers (the inner callbacks
that call onSelect?.()) so selection only happens once via onPointerDownCapture
in the Item component, and apply the same change to the similar inner clickable
region referenced around the 131-134 block; keep onPointerDownCapture as the
single selection trigger to avoid duplicate side effects.
In `@src/components/video-editor/videoPlayback/zoomAnimation.test.ts`:
- Around line 404-415: The test is using timeMs=4100 which is inside the
connected transition (transitionEnd=4200) and leaves no hold window because
transitionEnd equals nextRegion.startMs; update the test so timeMs is strictly
greater than transitionEnd and strictly less than the next region's start to
exercise the hold phase — e.g., either set the second region's startMs to a
later value (like 4300) or change the test timeMs to something > transitionEnd
(e.g., 4201) and keep assertions for findDominantRegion,
getConnectedRegionTransition/getConnectedRegionHold expectations unchanged.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 409-416: The stopWebcamRecorder path awaits
webcamStopPromise.current even when recorder.state === "inactive", which can
hang because onstop won't fire; change the logic in stopWebcamRecorder (and the
analogous block around the code handling pendingWebcamPathPromise at 468-471) to
only await webcamStopPromise.current (or the pending variable) when the recorder
was not already "inactive" and when the promise exists, otherwise skip the await
and immediately resolve; update assignment of resolvedWebcamPath.current and
pendingWebcamPathPromise.current accordingly so cleanup doesn't depend on a
never-resolving webcamStopPromise.current.
- Around line 575-579: The early return after a successful
recoverNativeRecordingSession() causes the microphone sidecar persistence block
to be skipped; modify the flow so that when recoverNativeRecordingSession()
returns a recoveredPath you still run the existing sidecar persistence logic
(the block currently at the sidecar/persistence section) before returning—either
by moving/invoking that persistence logic (or a new helper like
persistMicrophoneSidecar(recoveredPath)) in the recovery branch or by
restructuring to only return after the sidecar code has executed, keeping the
rest of the function unchanged.
- Around line 754-756: The unmount cleanup only calls
mediaRecorder.current.stop() when mediaRecorder.current?.state === "recording",
so a "paused" recorder will never be stopped and finalization callbacks won't
run; update the unmount logic in useScreenRecorder (referencing
mediaRecorder.current and its state) to call stop() when the recorder exists and
its state is not "inactive" (e.g., state === "recording" || state === "paused"
or state !== "inactive"), ensuring you check mediaRecorder.current is defined
before calling stop().
---
Nitpick comments:
In `@src/components/video-editor/videoPlayback/zoomAnimation.test.ts`:
- Around line 314-320: The test's local variable zoomOutStart is misleading
because the implementation uses region.endMs - ZOOM_OUT_EARLY_START_MS
(ZOOM_OUT_EARLY_START_MS = 500); update the test in zoomAnimation.test.ts to
either (A) compute the start the same way as the implementation (const
zoomOutStart = region.endMs - ZOOM_OUT_EARLY_START_MS) or (B) rename the
variable to indicate it's an adjusted sample point (e.g., zoomOutSamplePoint or
sampleTimeOffset) and keep the current numeric offset; ensure the call to
computeRegionStrength(region, ...) uses the renamed/updated variable so the test
clearly matches intent.
🪄 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: 25bca8d4-aa0c-4715-9bb1-8b44a514a2d5
📒 Files selected for processing (11)
src/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/ItemGlass.module.csssrc/components/video-editor/timeline/KeyframeMarkers.tsxsrc/components/video-editor/timeline/TimelineWrapper.tsxsrc/components/video-editor/videoPlayback/videoEventHandlers.tssrc/components/video-editor/videoPlayback/zoomAnimation.test.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.tssrc/components/video-editor/videoPlayback/zoomTransform.tssrc/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.tssrc/hooks/useVideoDevices.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/hooks/useVideoDevices.ts
- src/components/video-editor/videoPlayback/zoomTransform.ts
- src/components/video-editor/timeline/TimelineWrapper.tsx
- src/components/video-editor/timeline/KeyframeMarkers.tsx
- src/components/video-editor/videoPlayback/videoEventHandlers.ts
- src/components/video-editor/timeline/ItemGlass.module.css
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/hooks/useScreenRecorder.ts (2)
276-290: Consider movingcomputeBitrateoutside the component.This pure function doesn't depend on any component state or props, so it's recreated on every render unnecessarily. Moving it outside the component (or memoizing with
useCallback) would avoid this overhead.♻️ Proposed refactor
+const computeBitrate = (width: number, height: number) => { + const pixels = width * height; + const highFrameRateBoost = + TARGET_FRAME_RATE >= HIGH_FRAME_RATE_THRESHOLD ? HIGH_FRAME_RATE_BOOST : 1; + + if (pixels >= FOUR_K_PIXELS) { + return Math.round(BITRATE_4K * highFrameRateBoost); + } + + if (pixels >= QHD_PIXELS) { + return Math.round(BITRATE_QHD * highFrameRateBoost); + } + + return Math.round(BITRATE_BASE * highFrameRateBoost); +}; + export function useScreenRecorder(): UseScreenRecorderReturn { const [recording, setRecording] = useState(false); // ... rest of hook - - const computeBitrate = (width: number, height: number) => { - const pixels = width * height; - const highFrameRateBoost = - TARGET_FRAME_RATE >= HIGH_FRAME_RATE_THRESHOLD ? HIGH_FRAME_RATE_BOOST : 1; - - if (pixels >= FOUR_K_PIXELS) { - return Math.round(BITRATE_4K * highFrameRateBoost); - } - - if (pixels >= QHD_PIXELS) { - return Math.round(BITRATE_QHD * highFrameRateBoost); - } - - return Math.round(BITRATE_BASE * highFrameRateBoost); - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 276 - 290, computeBitrate is a pure helper recreated on every render inside the component; move it to module scope (outside the component) or memoize it with useCallback to avoid repeated allocations. Extract the function named computeBitrate (which uses TARGET_FRAME_RATE, HIGH_FRAME_RATE_THRESHOLD, HIGH_FRAME_RATE_BOOST, FOUR_K_PIXELS, QHD_PIXELS, BITRATE_4K, BITRATE_QHD, BITRATE_BASE) to the top-level of the file (or wrap it in useCallback with an empty deps array if you need it closed over component scope), update any references to call the top-level computeBitrate, and ensure the constants it uses remain accessible at module scope.
614-694: Consider using an assignment pattern for thestopRecordingref to avoid stale closures.The
useRef(() => {...})pattern captures closure values at initial render. While most dependencies here are stable,isMacOS(used at line 651) is state that's set asynchronously after mount. IfisMacOSchanges after the ref is initialized, the error message would use the stale initial value.The current code likely works because
isMacOSis set early and doesn't change during a recording session, but reassigning the ref on each render would make this more robust.♻️ Proposed safer pattern
- const stopRecording = useRef(() => { + const stopRecordingFn = useCallback(() => { setPaused(false); if (nativeScreenRecording.current) { // ... rest of implementation } // ... rest of implementation - }); + }, [ + buildNativeCaptureFailureMessage, + cleanupCapturedMedia, + finalizeRecordingSession, + isMacOS, + logNativeCaptureDiagnostics, + markRecordingResumed, + notifyRecordingFinalizationFailure, + recoverNativeRecordingSession, + showRecordingFinalizationToast, + stopMicFallbackRecorder, + stopWebcamRecorder, + storeMicrophoneSidecar, + ]); + + const stopRecording = useRef(stopRecordingFn); + stopRecording.current = stopRecordingFn;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 614 - 694, The stopRecording ref is initialized with useRef(() => { ... }) which can capture stale closure values like isMacOS; change the pattern to assign the handler on each render (e.g. stopRecording.current = () => { ... }) so the function always closes over current state/refs (retain all existing logic that references nativeScreenRecording, nativeWindowsRecording, mediaRecorder, pauseSegmentsRef, pendingWebcamPathPromise, etc.), ensuring isMacOS and other state updates are honored when the handler runs.src/components/video-editor/videoPlayback/zoomRegionUtils.ts (1)
108-130: Avoid repeated linear scans in this per-frame path.
findDominantRegion()runs on every playback/export frame, but this loop does twoconnectedPairs.find(...)scans per region. On larger timelines that turns the active-region lookup into avoidable hot-loop work. Pre-index outgoing/incoming pairs once and do constant-time lookups here.Possible refactor
function getActiveRegion( regions: ZoomRegion[], timeMs: number, connectedPairs: ConnectedRegionPair[], ) { + const outgoingById = new Map( + connectedPairs.map((pair) => [pair.currentRegion.id, pair] as const), + ); + const incomingById = new Map( + connectedPairs.map((pair) => [pair.nextRegion.id, pair] as const), + ); + const activeRegions = regions .map((region) => { - const outgoingPair = connectedPairs.find((pair) => pair.currentRegion.id === region.id); + const outgoingPair = outgoingById.get(region.id); if (outgoingPair && timeMs >= outgoingPair.transitionStart) { return { region, strength: 0 }; } - const incomingPair = connectedPairs.find((pair) => pair.nextRegion.id === region.id); + const incomingPair = incomingById.get(region.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/videoPlayback/zoomRegionUtils.ts` around lines 108 - 130, The regions.map loop is doing two linear finds per region on connectedPairs (outgoingPair/incomingPair), making the per-frame path O(n*m); pre-index connectedPairs into two lookup maps before the loop (e.g., outgoingMap keyed by pair.currentRegion.id and incomingMap keyed by pair.nextRegion.id) and then replace the connectedPairs.find(...) calls inside activeRegions with constant-time lookups from those maps; keep the rest of the logic (checks against transitionStart, nextRegionZoomOutStart and computeRegionStrength(region, timeMs)) unchanged so behavior is preserved.
🤖 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/videoPlayback/zoomRegionUtils.ts`:
- Around line 43-64: computeRegionStrength can produce a sharp drop when
zoomInEnd > zoomOutStart for short regions; fix by detecting that overlap (using
zoomInEnd and zoomOutStart) and clamping/merging the windows before branching:
compute an overlap midpoint (e.g., mid = (zoomInEnd + zoomOutStart) / 2) and if
zoomInEnd > zoomOutStart set zoomInEnd = zoomOutStart = mid and recompute
dependent values (leadInStart, leadOutEnd) so the ramp-up (easeOutZoom) and
ramp-down are continuous; keep the rest of the branch logic (lead-in ramp, full
strength, lead-out ramp) but operate on the adjusted zoomInEnd/zoomOutStart
values to avoid the abrupt strength drop in computeRegionStrength.
---
Nitpick comments:
In `@src/components/video-editor/videoPlayback/zoomRegionUtils.ts`:
- Around line 108-130: The regions.map loop is doing two linear finds per region
on connectedPairs (outgoingPair/incomingPair), making the per-frame path O(n*m);
pre-index connectedPairs into two lookup maps before the loop (e.g., outgoingMap
keyed by pair.currentRegion.id and incomingMap keyed by pair.nextRegion.id) and
then replace the connectedPairs.find(...) calls inside activeRegions with
constant-time lookups from those maps; keep the rest of the logic (checks
against transitionStart, nextRegionZoomOutStart and
computeRegionStrength(region, timeMs)) unchanged so behavior is preserved.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 276-290: computeBitrate is a pure helper recreated on every render
inside the component; move it to module scope (outside the component) or memoize
it with useCallback to avoid repeated allocations. Extract the function named
computeBitrate (which uses TARGET_FRAME_RATE, HIGH_FRAME_RATE_THRESHOLD,
HIGH_FRAME_RATE_BOOST, FOUR_K_PIXELS, QHD_PIXELS, BITRATE_4K, BITRATE_QHD,
BITRATE_BASE) to the top-level of the file (or wrap it in useCallback with an
empty deps array if you need it closed over component scope), update any
references to call the top-level computeBitrate, and ensure the constants it
uses remain accessible at module scope.
- Around line 614-694: The stopRecording ref is initialized with useRef(() => {
... }) which can capture stale closure values like isMacOS; change the pattern
to assign the handler on each render (e.g. stopRecording.current = () => { ...
}) so the function always closes over current state/refs (retain all existing
logic that references nativeScreenRecording, nativeWindowsRecording,
mediaRecorder, pauseSegmentsRef, pendingWebcamPathPromise, etc.), ensuring
isMacOS and other state updates are honored when the handler runs.
🪄 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: a56e4b35-80d3-4876-a558-fb67e92f4dcf
📒 Files selected for processing (4)
src/components/video-editor/timeline/Item.tsxsrc/components/video-editor/videoPlayback/zoomAnimation.test.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.tssrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/timeline/Item.tsx
# Conflicts: # src/components/video-editor/timeline/Item.tsx # src/components/video-editor/timeline/TimelineEditor.tsx
Summary
Full rewrite of the timeline interaction system, zoom/cursor playback pipeline, and recording hooks.
Changes
zoomSuggestionUtils— smarter region suggestions from cursor telemetryzoomRegionUtilsedge cases at timeline bounds; reworkzoomTransformfor cursor-follow cameracursorFollowCamera,cursorSway,cursorLoopTelemetry,cursorViewport— cursor motion smoothing and edge snappinglayoutUtils,overlayUtils,focusUtils,motionSmoothingupdated to support new zoom and cursor systemsuseVideoDevices,useMicrophoneDevices,useAudioLevelMeterimprovementsSummary by CodeRabbit
New Features
Bug Fixes & Improvements
Chores