feat: add custom zoom slider with continuous scale control (#513)#518
Open
makaradam wants to merge 4 commits intosiddharthvaddem:mainfrom
Open
feat: add custom zoom slider with continuous scale control (#513)#518makaradam wants to merge 4 commits intosiddharthvaddem:mainfrom
makaradam wants to merge 4 commits intosiddharthvaddem:mainfrom
Conversation
…vaddem#513) Adds a Radix UI slider below the zoom preset buttons allowing any scale between 1.0x and 5.0x. When the slider value matches a preset exactly, that preset button also shows as active. - Add `customScale?: number` to `ZoomRegion` and `getZoomScale()` helper that returns customScale when set, falling back to ZOOM_DEPTH_SCALES[depth] - Overlay indicator, playback renderer, and frame exporter all use getZoomScale() so preview, playback, and export are consistent - Fix focus clamping in zoomRegionUtils and frameRenderer to use actual scale instead of depth-based preset scale, preventing zoom drift with custom values - Fix drag boundary in VideoPlayback to use clampFocusToScale with the actual scale so the full canvas is clickable at high custom zoom levels - Timeline item label shows custom scale value when set - Slider styled dark with green thumb/fill when a custom (non-preset) value is active Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds optional per-zoom-region custom scale (clamped 1.0–5.0, two-decimal precision), a SettingsPanel Radix slider with live and commit callbacks, persists customScale on region creation/depth changes, and propagates the effective zoom scale via a new getZoomScale helper through timeline labels, playback, overlays, and frame rendering. ChangesCustom Zoom Scale Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🤖 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/SettingsPanel.tsx`:
- Around line 596-600: The preset-match logic is using strict float equality
(effectiveScale === ZOOM_DEPTH_SCALES[option.depth]) so a slider value that is
numerically equal to a preset but differs by a tiny float error still appears as
"custom"; change to an approximate-equality check (e.g., Math.abs(a - b) <
epsilon) when computing isActive and when comparing selectedZoomCustomScale to
preset scales so values within a small tolerance are treated as the preset;
update places that key off selectedZoomCustomScale (including the slider
change/commit handlers and the effectiveScale calculation in SettingsPanel) to
either clear selectedZoomCustomScale or coerce it to the preset value when it
matches a preset within that tolerance, using symbols: effectiveScale,
selectedZoomCustomScale, selectedZoomDepth, ZOOM_DEPTH_SCALES, and isActive.
In `@src/components/video-editor/types.ts`:
- Around line 237-240: getZoomScale currently returns region.customScale raw,
which can propagate NaN/Infinity or extreme values; update getZoomScale to
validate and sanitize region.customScale: if customScale is a finite number,
clamp it to a safe range (e.g., min 0.01, max 100) and return that, otherwise
fall back to ZOOM_DEPTH_SCALES[region.depth]; reference the getZoomScale
function and the ZoomRegion.customScale/ZOOM_DEPTH_SCALES symbols when making
the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e17e3029-d8f3-43f3-b434-ff2e155f2e8e
📒 Files selected for processing (10)
src/components/video-editor/SettingsPanel.tsxsrc/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/overlayUtils.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.tssrc/i18n/locales/en/settings.jsonsrc/lib/exporter/frameRenderer.ts
- Clamp and NaN-guard customScale in getZoomScale (defensive sanitization) - Set customScale on preset button click so slider stays green - Set customScale on new zoom region creation so slider lights up immediately
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/VideoEditor.tsx (1)
774-783:⚠️ Potential issue | 🟠 MajorFix focus clamping in
handleZoomFocusChangeto account for custom scales.
clampFocusToDepthhas an unused_depthparameter and doesn't actually clamp based on the effective zoom scale. Whenregion.customScalediffers fromZOOM_DEPTH_SCALES[region.depth], the focus drag isn't properly bounded.Replace with the same pattern used elsewhere in the codebase (VideoPlayback, overlay, export):
focus: clampFocusToScale(focus, getZoomScale(region))This handles both preset depths and custom scales correctly. You'll need to import
getZoomScalefrom./typesandclampFocusToScalefrom./videoPlayback/focusUtils.🤖 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 774 - 783, handleZoomFocusChange currently uses clampFocusToDepth which ignores the region's effective scale (and has an unused _depth param), so focus dragging isn't bounded when region.customScale differs from ZOOM_DEPTH_SCALES; update the mapping in handleZoomFocusChange (for zoomRegions and region) to call clampFocusToScale(focus, getZoomScale(region)) instead of clampFocusToDepth(...), and add imports for getZoomScale from ./types and clampFocusToScale from ./videoPlayback/focusUtils; keep the rest of the updateState/zoomRegions mapping intact so only the focus computation changes.
🤖 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/VideoEditor.tsx`:
- Around line 804-816: The handler handleZoomCustomScaleChange currently writes
raw scale values into zoomRegions.customScale without validating; add a guard to
ensure the incoming scale is a finite number (e.g., using Number.isFinite or
Math.isFinite) before rounding and calling updateState, and if it's not finite,
either return early (no state change) or set a safe default (e.g., 1.0) instead;
update the logic around selectedZoomId, updateState, and the customScale
assignment to only persist the rounded finite value.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 774-783: handleZoomFocusChange currently uses clampFocusToDepth
which ignores the region's effective scale (and has an unused _depth param), so
focus dragging isn't bounded when region.customScale differs from
ZOOM_DEPTH_SCALES; update the mapping in handleZoomFocusChange (for zoomRegions
and region) to call clampFocusToScale(focus, getZoomScale(region)) instead of
clampFocusToDepth(...), and add imports for getZoomScale from ./types and
clampFocusToScale from ./videoPlayback/focusUtils; keep the rest of the
updateState/zoomRegions mapping intact so only the focus computation changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a224048-b744-4c15-8d15-50ee8d6af26f
📒 Files selected for processing (2)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/types.ts
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.
Closes #513
Adds a continuous zoom slider below the preset buttons (1.25×–5×) that lets users set any scale value between presets.
customScale?: numberfield andgetZoomScale()helper — falls back toZOOM_DEPTH_SCALES[depth]for presets, so existing projects are unaffectedzoomRegionUtils.ts) and export (frameRenderer.ts) — focus clamping was using the depth-based scale instead of the actual custom scale, causing the zoom to land at the wrong positionclampFocusToScale4.39×) instead of preset label when custom scale is setframeRenderer.tsuses the samegetZoomScale()helper so exported video matches preview exactlyTest plan
Summary by CodeRabbit
New Features
Localization