fix: SliderControl CSS#487
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSliderControl is converted to a memo-wrapped component and now drives visuals with a CSS custom property (--slider-pct). Refs and useEffect perform DOM updates; pointer dragging caches bounds and throttles moves with requestAnimationFrame, updating ARIA/value text and calling onChange with quantized values. ChangesSliderControl CSS-Driven Performance Refactor
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/SliderControl.tsx (1)
137-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
touch-noneto the slider root to prevent mobile scrolling interference.
preventDefault()onpointerdownis insufficient to suppress browser pan/zoom gestures because modern browsers use passive event listeners by default, which ignorepreventDefault(). This allows touch drags to get canceled into page scrolling on mobile devices. Thetouch-noneCSS class (Tailwind'stouch-action: none) is the proper solution and is already used in similar interactive components in this codebase.Suggested fix
- className="relative flex h-10 w-full select-none items-center overflow-hidden rounded-xl bg-editor-bg/80 px-1.5 outline-none focus-visible:ring-1 focus-visible:ring-[`#2563EB`]/40" + className="relative flex h-10 w-full touch-none select-none items-center overflow-hidden rounded-xl bg-editor-bg/80 px-1.5 outline-none focus-visible:ring-1 focus-visible:ring-[`#2563EB`]/40"🤖 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/SliderControl.tsx` around lines 137 - 160, The root slider DIV (the element with role="slider" in SliderControl.tsx that uses rootRef and handlePointerDown) needs the Tailwind touch-none class added to its className to set touch-action: none so mobile pan/zoom/scroll gestures won’t cancel slider drags; update the className for that root element to include "touch-none" (keeping the existing classes and onPointerDown/handlePointerDown logic intact).
🤖 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/SliderControl.tsx`:
- Around line 71-83: The fast-path DOM updates use the raw pointer-derived
currentPct instead of the step-snapped finalValue, causing the visual
fill/handle and ARIA attributes to lag; update the logic in SliderControl so you
compute the percentage from finalValue (using the same normalization used
elsewhere between min/max) and set
rootRef.current.style.setProperty("--slider-pct", `${snappedPct}%`) and update
valueTextRef.current.textContent and the element's ARIA attributes
(aria-valuenow / aria-valuetext) from formatValue(finalValue) before calling
onChange(finalValue); apply the identical change to the other fast-path block
referenced around lines 140-146.
- Around line 109-127: In finishPointer, ensure the final pointer position is
applied before tearing down: if finishEvent.pointerId === pointerId and the
event is a pointerup, call updateValue(finishEvent.clientX) (or otherwise flush
the last position) prior to canceling requestRef.current and removing listeners;
reference the finishPointer function, updateValue, requestRef, pointerId and
target to locate and place this call so quick drag-releases update the slider to
the final release position before cleanup.
---
Outside diff comments:
In `@src/components/video-editor/SliderControl.tsx`:
- Around line 137-160: The root slider DIV (the element with role="slider" in
SliderControl.tsx that uses rootRef and handlePointerDown) needs the Tailwind
touch-none class added to its className to set touch-action: none so mobile
pan/zoom/scroll gestures won’t cancel slider drags; update the className for
that root element to include "touch-none" (keeping the existing classes and
onPointerDown/handlePointerDown logic 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f8f1006-13e0-486a-998d-f0149fa45102
📒 Files selected for processing (1)
src/components/video-editor/SliderControl.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/video-editor/SliderControl.tsx (1)
176-176: ⚡ Quick winCSS calc expression uses invalid unit arithmetic—consider storing slider position as a decimal.
The calc expression
(var(--slider-pct) * 6px / 100%)violates CSS spec constraints. Per CSS Values and Units, multiplication requires at least one operand to be a unitless<number>, but--slider-pctis stored as a percentage string (e.g.,"50%"), makingpercentage * lengthinvalid. Similarly, division requires the divisor to be unitless, so/ 100%is also invalid. While some browsers may tolerate this through lenient parsing, it is not reliable.To ensure spec compliance, store the slider position as a decimal (0–1) and adjust the calc expressions:
♻️ Suggested refactor
At line 57 (useEffect):
-rootRef.current.style.setProperty("--slider-pct", `${pct}%`); +rootRef.current.style.setProperty("--slider-pct", String(pct / 100));At line 76 (updateValue):
-rootRef.current.style.setProperty("--slider-pct", `${finalPct}%`); +rootRef.current.style.setProperty("--slider-pct", String(Number(finalPct) / 100));At line 169 (inline style):
-"--slider-pct": `${pct}%`, +"--slider-pct": String(pct / 100),At line 176:
-width: "calc(var(--slider-pct) - (var(--slider-pct) * 6px / 100%))", +width: "calc(var(--slider-pct) * (100% - 6px))",At line 185:
-left: "calc(2px + var(--slider-pct) - (var(--slider-pct) * 6px / 100%))", +left: "calc(2px + var(--slider-pct) * (100% - 6px))",🤖 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/SliderControl.tsx` at line 176, The CSS calc is invalid because --slider-pct is stored as a percentage string; change --slider-pct to be a unitless decimal (0–1) everywhere (set in the useEffect that initializes it and in updateValue), and update all uses (including the inline style that sets width) to multiply that unitless value by lengths instead of multiplying/dividing percentages; for example replace the width calc with: use "calc(var(--slider-pct) * (100% - 6px))" (and adjust any other expressions at the earlier useEffect and updateValue locations to read/set decimals rather than "50%"/"%"-strings).
🤖 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.
Nitpick comments:
In `@src/components/video-editor/SliderControl.tsx`:
- Line 176: The CSS calc is invalid because --slider-pct is stored as a
percentage string; change --slider-pct to be a unitless decimal (0–1) everywhere
(set in the useEffect that initializes it and in updateValue), and update all
uses (including the inline style that sets width) to multiply that unitless
value by lengths instead of multiplying/dividing percentages; for example
replace the width calc with: use "calc(var(--slider-pct) * (100% - 6px))" (and
adjust any other expressions at the earlier useEffect and updateValue locations
to read/set decimals rather than "50%"/"%"-strings).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7759967-3ce1-4667-809e-0b486f965481
📒 Files selected for processing (1)
src/components/video-editor/SliderControl.tsx
Summary
Slider control had issues in terms of CSS and possibly performance
Description
Here is a summary of the fixes and optimizations implemented for the SliderControl:
Visual & Accuracy Fixes
Removed 0% Clipping: Fixed a bug where the white handle would disappear at 0% due to an incorrect -8px offset. It now stays perfectly visible and aligned at the start.
Eliminated the "15% Jump": Removed a hardcoded 2.1rem minimum width on the fill bar. The slider now scales linearly and accurately from 0 to 100%.
Pixel-Perfect Inset: Corrected a CSS calculation error where offsets were being treated as percentages of the total width instead of fixed pixels (* 6px / 100%). This ensures the slider reaches the absolute visual end regardless of how wide the component is.
Performance Optimizations
Hybrid Rendering (React Bypass): The component now uses a high-performance "uncontrolled" model during interaction. It updates the DOM directly via CSS Variables (--slider-pct) and Refs, bypassing React's re-render cycle for zero-latency dragging.
Layout Stability: Bounding box measurements are now cached on pointerdown. This prevents "layout thrashing" (repeatedly asking the browser for dimensions), which significantly reduces CPU load during movement.
60fps+ Smoothness: Leveraged requestAnimationFrame and CSS hardware acceleration to ensure the smoothest possible visual feedback, even if the parent application is performing heavy background tasks.
Memoization: Wrapped the component in React.memo to ensure it only re-renders when actual props change, keeping the main thread clear.
Type of Change
Screenshots / Video
Before :
Now
Summary by CodeRabbit