Add click effects and fix auto zoom and FPS bitrate behavior#630
Merged
Conversation
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds end-to-end configurable cursor click effects (none/ripple/spotlight/echo) with color, scale, opacity, and duration controls across UI, playback rendering, persistence, and export. Also refactors timeline select-all to target zoom regions and updates related keyboard shortcuts and canvas selection behavior. ChangesCursor Click Effects Feature
Timeline Selection Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/video-editor/SettingsPanel.tsx (1)
1804-1825:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset should restore the click-effect color too.
resetCursorSectionresets the new click-effect fields exceptcursorClickEffectColor, so a custom color survives Reset and leaves the cursor section in a partially customized state.Suggested fix
const resetCursorSection = () => { onShowCursorChange?.(initialEditorPreferences.showCursor); onLoopCursorChange?.(initialEditorPreferences.loopCursor); onCursorStyleChange?.(initialEditorPreferences.cursorStyle); onCursorSizeChange?.(initialEditorPreferences.cursorSize); onCursorSmoothingChange?.(initialEditorPreferences.cursorSmoothing); onCursorSpringStiffnessMultiplierChange?.( initialEditorPreferences.cursorSpringStiffnessMultiplier, ); onCursorSpringDampingMultiplierChange?.( initialEditorPreferences.cursorSpringDampingMultiplier, ); onCursorSpringMassMultiplierChange?.(initialEditorPreferences.cursorSpringMassMultiplier); onCursorMotionBlurChange?.(initialEditorPreferences.cursorMotionBlur); onCursorClickEffectChange?.(initialEditorPreferences.cursorClickEffect); + onCursorClickEffectColorChange?.(initialEditorPreferences.cursorClickEffectColor); onCursorClickEffectScaleChange?.(initialEditorPreferences.cursorClickEffectScale); onCursorClickEffectOpacityChange?.(initialEditorPreferences.cursorClickEffectOpacity); onCursorClickEffectDurationMsChange?.(initialEditorPreferences.cursorClickEffectDurationMs); onCursorClickBounceChange?.(initialEditorPreferences.cursorClickBounce); onCursorClickBounceDurationChange?.(initialEditorPreferences.cursorClickBounceDuration); onCursorSwayChange?.(initialEditorPreferences.cursorSway); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/SettingsPanel.tsx` around lines 1804 - 1825, resetCursorSection currently restores all cursor-related prefs except the click-effect color, leaving cursorClickEffectColor unchanged; update resetCursorSection to call onCursorClickEffectColor with initialEditorPreferences.cursorClickEffectColor so the click-effect color is reset alongside cursorClickEffect, cursorClickEffectScale, cursorClickEffectOpacity, cursorClickEffectDurationMs, cursorClickBounce, and cursorClickBounceDuration.src/components/video-editor/videoPlayback/layoutUtils.ts (1)
213-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScale the mask radius from the masked video size, not the outer container.
layout.croppedDisplayWidth/layout.croppedDisplayHeightare the dimensions actually passed todrawSquircleOnGraphics. Using the outer container size here makes corners too round whenever padding or device-frame insets shrink the video area, so preview parity drifts on those layouts.Suggested fix
drawSquircleOnGraphics(maskGraphics, { x: layout.centerOffsetX, y: layout.centerOffsetY, width: layout.croppedDisplayWidth, height: layout.croppedDisplayHeight, - radius: scalePreviewBorderRadius(width, height, borderRadius), + radius: scalePreviewBorderRadius( + layout.croppedDisplayWidth, + layout.croppedDisplayHeight, + borderRadius, + ), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/videoPlayback/layoutUtils.ts` around lines 213 - 220, The mask radius is being scaled using the outer container size (width, height) which makes the squircle corners too round when the actual video area is smaller; update the call that draws the mask (maskGraphics and drawSquircleOnGraphics) to compute the radius from the masked video dimensions by passing layout.croppedDisplayWidth and layout.croppedDisplayHeight into scalePreviewBorderRadius instead of the outer width/height so the radius is scaled relative to layout.croppedDisplayWidth/layout.croppedDisplayHeight and the mask corners match the rendered video area.
🧹 Nitpick comments (3)
src/components/video-editor/timeline/hooks/useTimelineKeyboardShortcuts.ts (1)
71-77: ⚡ Quick winAdd a shortcut-level regression test for zoom-only select-all.
This hook now owns the
Ctrl/Cmd+A+ Delete behavior, but the updated coverage in this cohort only exercisesresolveDeleteSelectionTarget. A focused test here would catch regressions where select-all stops clearing non-zoom selections or Delete stops hitting the bulk-zoom path.Also applies to: 93-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/timeline/hooks/useTimelineKeyboardShortcuts.ts` around lines 71 - 77, Add a shortcut-level regression test for the useTimelineKeyboardShortcuts hook to verify that Ctrl/Cmd+A (matchesShortcut -> key "a", ctrl true) triggers activateSelectAllZooms when only zooms exist and that subsequent Delete hits the bulk-zoom path (resolveDeleteSelectionTarget) and clears non-zoom selections; create a test that simulates keyboard events for Cmd/Ctrl+A and Delete against a timeline with only zoom blocks (and another with mixed selection) and assert that activateSelectAllZooms was called and that resolveDeleteSelectionTarget handles deletion for the zoom-only case to prevent regressions.src/lib/exporter/modernVideoExporter.nativeStaticLayout.test.ts (1)
238-258: ⚡ Quick winAdd the
"none"boundary case here too.This only proves active effects block native static-layout. A paired assertion for
cursorClickEffect: "none"staying eligible would catch truthy checks that accidentally disable the native path for the default style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/modernVideoExporter.nativeStaticLayout.test.ts` around lines 238 - 258, Add a paired assertion that when cursorClickEffect is "none" the native static-layout path remains eligible: call createExporter with showCursor: true, cursorClickEffect: "none" and the same cursorTelemetry, then call exporter.getNativeStaticLayoutSkipReason(...) and assert it does NOT return "unsupported-cursor-click-effect" (or more robustly assert the result is falsy/null). This should sit alongside the existing test (or as a second it block) referencing createExporter and getNativeStaticLayoutSkipReason to catch regressions that use truthy checks for cursorClickEffect.src/lib/exporter/gifExporter.ts (1)
137-200: ⚡ Quick winType
buildGifFrameRendererConfig()againstFrameRenderer.This helper is now the single mapping point, so letting it return an inferred object makes future renderer-config drift easy to miss at compile time.
Suggested change
export function buildGifFrameRendererConfig( config: GifExporterConfig, videoInfo: { width: number; height: number }, -) { +): ConstructorParameters<typeof FrameRenderer>[0] { return { width: config.width, height: config.height, wallpaper: config.wallpaper,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/gifExporter.ts` around lines 137 - 200, The buildGifFrameRendererConfig function currently returns an untyped object which can drift from the renderer shape; change its signature to explicitly return the FrameRenderer type (e.g., export function buildGifFrameRendererConfig(...): FrameRenderer) and update imports to bring in the FrameRenderer type, then ensure the returned object includes all required FrameRenderer properties (or use a single narrowing/cast after verifying fields) so TS will catch future mismatches in buildGifFrameRendererConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1216-1221: The callbacks/effects that currently list
cursorClickBounce, cursorClickBounceDuration, cursorClickEffect,
cursorClickEffectScale, cursorClickEffectOpacity, and
cursorClickEffectDurationMs in their dependency arrays must also include
cursorClickEffectColor because those hooks/readers reference the color; update
each dependency array that contains those symbols (the three occurrences
matching the groups you highlighted) to add cursorClickEffectColor so thumbnail
capture, export, and preference-persistence effects re-run when the click-effect
color changes.
In `@src/components/video-editor/videoPlayback/cursorRenderer.ts`:
- Around line 1621-1632: The click-effect size is being multiplied by
bounceScale before calling drawClickEffectOnCanvas, which makes the canvas path
use a bounced size while the Pixi preview uses the unbounced size; change the
drawHeight calculation in cursorRenderer.ts to exclude bounceScale (i.e.,
compute drawHeight = h * getCursorStyleSizeMultiplier(config.style)) and pass
that unbounced drawHeight to drawClickEffectOnCanvas so both canvas and Pixi use
the same base size (leave bounceScale usage only where the visual bounce is
applied in rendering code, not in the click-effect sizing).
In `@src/i18n/locales/en/settings.json`:
- Around line 121-123: The locale defines a "burst" entry but the click-effect
enum uses "echo", so update the locales to match by renaming or adding the key
for the click-effect style: change the "burst" key to "echo" (or add an "echo"
object that mirrors "burst") and ensure its "label" and "description" fields
match the intended copy for the echo click-effect; verify the settings.json
locale block containing "burst" is updated so the click-effect enum value "echo"
resolves to the proper translated label/description.
---
Outside diff comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1804-1825: resetCursorSection currently restores all
cursor-related prefs except the click-effect color, leaving
cursorClickEffectColor unchanged; update resetCursorSection to call
onCursorClickEffectColor with initialEditorPreferences.cursorClickEffectColor so
the click-effect color is reset alongside cursorClickEffect,
cursorClickEffectScale, cursorClickEffectOpacity, cursorClickEffectDurationMs,
cursorClickBounce, and cursorClickBounceDuration.
In `@src/components/video-editor/videoPlayback/layoutUtils.ts`:
- Around line 213-220: The mask radius is being scaled using the outer container
size (width, height) which makes the squircle corners too round when the actual
video area is smaller; update the call that draws the mask (maskGraphics and
drawSquircleOnGraphics) to compute the radius from the masked video dimensions
by passing layout.croppedDisplayWidth and layout.croppedDisplayHeight into
scalePreviewBorderRadius instead of the outer width/height so the radius is
scaled relative to layout.croppedDisplayWidth/layout.croppedDisplayHeight and
the mask corners match the rendered video area.
---
Nitpick comments:
In `@src/components/video-editor/timeline/hooks/useTimelineKeyboardShortcuts.ts`:
- Around line 71-77: Add a shortcut-level regression test for the
useTimelineKeyboardShortcuts hook to verify that Ctrl/Cmd+A (matchesShortcut ->
key "a", ctrl true) triggers activateSelectAllZooms when only zooms exist and
that subsequent Delete hits the bulk-zoom path (resolveDeleteSelectionTarget)
and clears non-zoom selections; create a test that simulates keyboard events for
Cmd/Ctrl+A and Delete against a timeline with only zoom blocks (and another with
mixed selection) and assert that activateSelectAllZooms was called and that
resolveDeleteSelectionTarget handles deletion for the zoom-only case to prevent
regressions.
In `@src/lib/exporter/gifExporter.ts`:
- Around line 137-200: The buildGifFrameRendererConfig function currently
returns an untyped object which can drift from the renderer shape; change its
signature to explicitly return the FrameRenderer type (e.g., export function
buildGifFrameRendererConfig(...): FrameRenderer) and update imports to bring in
the FrameRenderer type, then ensure the returned object includes all required
FrameRenderer properties (or use a single narrowing/cast after verifying fields)
so TS will catch future mismatches in buildGifFrameRendererConfig.
In `@src/lib/exporter/modernVideoExporter.nativeStaticLayout.test.ts`:
- Around line 238-258: Add a paired assertion that when cursorClickEffect is
"none" the native static-layout path remains eligible: call createExporter with
showCursor: true, cursorClickEffect: "none" and the same cursorTelemetry, then
call exporter.getNativeStaticLayoutSkipReason(...) and assert it does NOT return
"unsupported-cursor-click-effect" (or more robustly assert the result is
falsy/null). This should sit alongside the existing test (or as a second it
block) referencing createExporter and getNativeStaticLayoutSkipReason to catch
regressions that use truthy checks for cursorClickEffect.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a52848c8-d7c8-47e4-b549-d21450ba2644
📒 Files selected for processing (26)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/editorPreferences.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/components/viewport/TimelineCanvas.tsxsrc/components/video-editor/timeline/hooks/useTimelineEditorRuntime.tssrc/components/video-editor/timeline/hooks/useTimelineKeyboardShortcuts.tssrc/components/video-editor/timeline/hooks/useTimelineSelection.tssrc/components/video-editor/timeline/hooks/utils/timelineSelectionUtils.test.tssrc/components/video-editor/timeline/hooks/utils/timelineSelectionUtils.tssrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/cursorFollowCamera.test.tssrc/components/video-editor/videoPlayback/cursorFollowCamera.tssrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/components/video-editor/videoPlayback/cursorViewport.tssrc/components/video-editor/videoPlayback/layoutUtils.test.tssrc/components/video-editor/videoPlayback/layoutUtils.tssrc/i18n/locales/en/settings.jsonsrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.test.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.fallback.test.tssrc/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tssrc/lib/exporter/modernVideoExporter.ts
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@copilot review |
Contributor
Reviewed the current PR state and there are no additional unresolved issues to address in code at this time. |
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
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.
Summary
Validation
Summary by CodeRabbit
New Features
UX Improvements
Behavior Changes
Documentation