Conversation
Owner
Author
|
@coderabbitai review |
📝 WalkthroughWalkthroughThis PR introduces a complete Slider component for the PeakFit design system. It includes the core React component with drag-based interaction, a comprehensive CSS module with multiple size and state variants, and an example component demonstrating continuous, discrete, and various size configurations. Changes
Sequence DiagramsequenceDiagram
actor User
participant Slider
participant Parent
User->>Slider: Mouse/Touch Down on Thumb
activate Slider
Slider->>Slider: Set isDragging = true
Slider->>Slider: Show Tooltip
loop While Dragging
User->>Slider: Move Mouse/Touch
Slider->>Slider: Calculate Position
Slider->>Slider: Snap to Step (if discrete)
Slider->>Slider: Clamp to Min/Max
Slider->>Parent: Call onChange(newValue)
activate Parent
Parent->>Parent: Update State
Parent->>Slider: Re-render with New Value
deactivate Parent
Slider->>Slider: Update Display & Tooltip
end
User->>Slider: Mouse/Touch Up
Slider->>Slider: Set isDragging = false
Slider->>Slider: Hide Tooltip
deactivate Slider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/components/slider/Slider.tsx (1)
112-121: MemoizegenerateStepsand fix the shadowedstepvariable.
generateSteps()is called on every render. For a wide continuous range (e.g.min=0, max=500, step=1) this allocates 501 objects per render. Wrap it inuseMemo.The map callback on line 153 also names its parameter
step, shadowing thestepprop from the outer scope — rename it to avoid confusion.♻️ Proposed refactor
- const generateSteps = () => { - const steps = []; - for (let i = min; i <= max; i += step) { - steps.push({ - value: i, - isActive: i <= value, - }); - } - return steps; - }; + const steps = useMemo(() => { + const result = []; + for (let i = min; i <= max; i += step) { + result.push({ value: i, isActive: i <= value }); + } + return result; + }, [min, max, step, value]);And in JSX:
- {generateSteps().map((step, i) => ( + {steps.map((stepItem, i) => ( <div - key={i} - className={`${classes.stepMarker} ${step.isActive ? classes.active : ""}`} + key={stepItem.value} + className={`${classes.stepMarker} ${stepItem.isActive ? classes.active : ""}`} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/slider/Slider.tsx` around lines 112 - 121, Wrap the generateSteps logic in useMemo to avoid regenerating the steps array on every render (especially for large ranges) by memoizing based on [min, max, step, value]; update the existing generateSteps function to return the memoized array (e.g., const steps = useMemo(() => { ... }, [min, max, step, value])). Also rename the map callback parameter currently named step (used in the JSX map over steps) to avoid shadowing the step prop — use a name like s or stepItem and update references (e.g., s.value, s.isActive) accordingly so the outer step prop is not shadowed.
🤖 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/slider/Example.tsx`:
- Line 29: The JSX in Example.tsx uses a bare global class name `caption` and an
inaccurate numeric format: change the class reference to the CSS-module export
used in this component (e.g., replace `className={`caption`}` with the module
reference such as `className={styles.caption}` after importing the module used
by this component) or explicitly confirm/declare a global `.caption` rule if you
intend it to be global; also update the display of `continuousValue` (the
variable `continuousValue` in the component) to use integer formatting like
`toFixed(0)` or `String(continuousValue)` instead of `toFixed(1)` so the label
matches the slider’s step precision.
In `@src/components/slider/Slider.module.css`:
- Around line 226-233: The .stepMarker CSS rule contains two background
declarations; remove the dead first background: delete the line setting
background: var(--surface-hover) inside the .stepMarker rule so only background:
var(--primary-400) remains (locate the .stepMarker selector in
Slider.module.css).
- Around line 192-201: Rename the `@keyframes` identifier tooltipFadeIn to
tooltip-fade-in and update the reference inside the .tooltip rule so both match
the new kebab-case name; locate the `@keyframes` block named tooltipFadeIn and the
.tooltip selector that uses animation/animation-name and change its reference to
tooltip-fade-in to satisfy the Stylelint keyframes-name-pattern rule.
In `@src/components/slider/Slider.tsx`:
- Around line 42-59: updateValue closes over onChange causing a stale-closure
bug and listener churn; make onChange stable for the mouse/touch listeners by
storing it in a ref and reading that ref inside updateValue. Concretely, add an
onChangeRef = useRef(onChange) and sync it in a small effect
(onChangeRef.current = onChange) whenever onChange changes, then change
updateValue to call onChangeRef.current(newValue) instead of onChange(newValue);
finally, adjust the effect that registers pointer/mouse listeners (the useEffect
that currently depends on value and registers updateValue) to remove onChange
and value from its dependency array so listeners are stable and only re-register
when disabled/sliderRef/type/min/max/step change.
- Line 39: The percentage calculation in Slider.tsx (const percentage = ((value
- min) / (max - min)) * 100) can divide by zero when min === max; guard by first
clamping value between min and max and computing percentage only if (max - min)
!== 0, otherwise set percentage to 0 (or 100 if you prefer full). Update the
computation used for style width/left to use that safe percentage (reference the
percentage constant and the value/min/max props) so style={{ width:
`${percentage}%` }} and left use a non-NaN numeric percentage.
- Line 125: The label element in Slider.tsx uses a bare global "label" class
string alongside the CSS module class (className={`label ${classes.label}`})
which is incorrect; update the label rendering (the JSX that references label
and classes.label) to remove the plain "label" or replace it with the module
equivalent (e.g., classes.labelRoot or the correct exported key from the CSS
module), so className only uses valid CSS-module identifiers (or a single string
from the module) and ensure the CSS module file exports the corresponding class
name.
- Line 52: The slider currently ignores the passed step in continuous mode
because actualStep is set to 1 when type !== "discrete"; update the logic around
actualStep in Slider.tsx (the computation using actualStep and the handling in
functions like onChange/onPointerMove that use it) so that actualStep = step
regardless of type (or validate/normalize step for continuous mode) and ensure
any value rounding/clamping uses this actualStep; alternatively, if you choose
to keep discrete-only behavior, update the prop JSDoc for the step prop to
explicitly state that step is only honored when type === "discrete" and add
runtime validation/warnings (e.g., console.warn) when a consumer passes step
with type "continuous".
- Around line 123-187: The thumb element in the Slider component lacks keyboard
and ARIA support; make the thumb focusable and screen-reader friendly by adding
role="slider", tabIndex={disabled ? -1 : 0}, and the ARIA attributes
aria-valuemin={min}, aria-valuemax={max}, aria-valuenow={value} (or computed
value), plus aria-label or aria-labelledby that references the label when
provided; implement an onKeyDown handler (e.g., handleKeyDown) on the thumb to
handle ArrowLeft/ArrowRight/ArrowUp/ArrowDown/Home/End to update value using
existing change logic (reuse setValue/handleMove functions) and ensure focus and
tooltip behavior (setShowTooltip/isDragging) integrate with keyboard
interactions. Ensure the same ARIA/keyboard behavior is applied for both
continuous and discrete Slider types and avoid keyboard focus when disabled.
---
Nitpick comments:
In `@src/components/slider/Slider.tsx`:
- Around line 112-121: Wrap the generateSteps logic in useMemo to avoid
regenerating the steps array on every render (especially for large ranges) by
memoizing based on [min, max, step, value]; update the existing generateSteps
function to return the memoized array (e.g., const steps = useMemo(() => { ...
}, [min, max, step, value])). Also rename the map callback parameter currently
named step (used in the JSX map over steps) to avoid shadowing the step prop —
use a name like s or stepItem and update references (e.g., s.value, s.isActive)
accordingly so the outer step prop is not shadowed.
Comment on lines
+192
to
+201
| @keyframes tooltipFadeIn { | ||
| from { | ||
| opacity: 0; | ||
| transform: translateX(-50%) translateY(-4px); | ||
| } | ||
| to { | ||
| opacity: 1; | ||
| transform: translateX(-50%) translateY(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Rename tooltipFadeIn to tooltip-fade-in to satisfy the Stylelint keyframes-name-pattern rule.
The @keyframes name and its reference in .tooltip (line 175) must be updated together.
🐛 Proposed fix
-@keyframes tooltipFadeIn {
+@keyframes tooltip-fade-in {And in .tooltip:
- animation: tooltipFadeIn 0.2s ease-out;
+ animation: tooltip-fade-in 0.2s ease-out;📝 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
| @keyframes tooltipFadeIn { | |
| from { | |
| opacity: 0; | |
| transform: translateX(-50%) translateY(-4px); | |
| } | |
| to { | |
| opacity: 1; | |
| transform: translateX(-50%) translateY(0); | |
| } | |
| } | |
| `@keyframes` tooltip-fade-in { | |
| from { | |
| opacity: 0; | |
| transform: translateX(-50%) translateY(-4px); | |
| } | |
| to { | |
| opacity: 1; | |
| transform: translateX(-50%) translateY(0); | |
| } | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 192-192: Expected keyframe name "tooltipFadeIn" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.module.css` around lines 192 - 201, Rename the
`@keyframes` identifier tooltipFadeIn to tooltip-fade-in and update the reference
inside the .tooltip rule so both match the new kebab-case name; locate the
`@keyframes` block named tooltipFadeIn and the .tooltip selector that uses
animation/animation-name and change its reference to tooltip-fade-in to satisfy
the Stylelint keyframes-name-pattern rule.
Comment on lines
+226
to
+233
| .stepMarker { | ||
| width: 4px; | ||
| height: 4px; | ||
| background: var(--surface-hover); | ||
| border-radius: 50%; | ||
| transition: all 0.2s ease; | ||
| background: var(--primary-400); | ||
| } |
There was a problem hiding this comment.
Remove the dead first background declaration on .stepMarker.
Stylelint flags a duplicate background property (lines 229 and 232). The first value (var(--surface-hover)) is immediately overridden and never applied.
🐛 Proposed fix
.stepMarker {
width: 4px;
height: 4px;
- background: var(--surface-hover);
border-radius: 50%;
transition: all 0.2s ease;
background: var(--primary-400);
}📝 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
| .stepMarker { | |
| width: 4px; | |
| height: 4px; | |
| background: var(--surface-hover); | |
| border-radius: 50%; | |
| transition: all 0.2s ease; | |
| background: var(--primary-400); | |
| } | |
| .stepMarker { | |
| width: 4px; | |
| height: 4px; | |
| border-radius: 50%; | |
| transition: all 0.2s ease; | |
| background: var(--primary-400); | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 229-229: Unexpected duplicate "background" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.module.css` around lines 226 - 233, The
.stepMarker CSS rule contains two background declarations; remove the dead first
background: delete the line setting background: var(--surface-hover) inside the
.stepMarker rule so only background: var(--primary-400) remains (locate the
.stepMarker selector in Slider.module.css).
| const sliderRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| // Calculate percentage from value | ||
| const percentage = ((value - min) / (max - min)) * 100; |
There was a problem hiding this comment.
min === max produces NaN and breaks rendering.
((value - min) / (max - min)) * 100 divides by zero when min === max, yielding NaN. This propagates to style={{ width: "NaN%" }} and left: "NaN%", silently breaking the visual output.
🛡️ Proposed guard
- const percentage = ((value - min) / (max - min)) * 100;
+ const percentage = max === min ? 0 : ((value - min) / (max - min)) * 100;📝 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
| const percentage = ((value - min) / (max - min)) * 100; | |
| const percentage = max === min ? 0 : ((value - min) / (max - min)) * 100; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.tsx` at line 39, The percentage calculation in
Slider.tsx (const percentage = ((value - min) / (max - min)) * 100) can divide
by zero when min === max; guard by first clamping value between min and max and
computing percentage only if (max - min) !== 0, otherwise set percentage to 0
(or 100 if you prefer full). Update the computation used for style width/left to
use that safe percentage (reference the percentage constant and the
value/min/max props) so style={{ width: `${percentage}%` }} and left use a
non-NaN numeric percentage.
Comment on lines
+42
to
+59
| const updateValue = (clientX: number) => { | ||
| if (!sliderRef.current || disabled) return; | ||
|
|
||
| const rect = sliderRef.current.getBoundingClientRect(); | ||
| const offsetX = Math.max(0, Math.min(clientX - rect.left, rect.width)); | ||
| const percent = offsetX / rect.width; | ||
|
|
||
| let newValue = min + percent * (max - min); | ||
|
|
||
| // Snap to step | ||
| const actualStep = type === "discrete" ? step : 1; | ||
| newValue = Math.round(newValue / actualStep) * actualStep; | ||
| newValue = Math.max(min, Math.min(max, newValue)); | ||
|
|
||
| if (newValue !== value) { | ||
| onChange(newValue); | ||
| } | ||
| }; |
There was a problem hiding this comment.
onChange is missing from the useEffect dependency array — stale closure bug.
updateValue (lines 42–58) closes over onChange. Because onChange is absent from the dep array at line 104, if the parent passes a non-stable callback that captures external state (e.g., useCallback((v) => submit(v, theme), [theme])), the effect won't re-run when theme changes and the stale version of onChange is called throughout the drag.
The cleanest fix avoids both the stale closure and the listener churn caused by value being in the dep array (every drag event triggers cleanup + re-registration):
🔧 Proposed fix — stable ref for updateValue
+ const updateValueRef = useRef(updateValue);
+ useEffect(() => {
+ updateValueRef.current = updateValue;
+ });
useEffect(() => {
if (!isDragging) return;
const handleMouseMove = (e: MouseEvent) => {
- updateValue(e.clientX);
+ updateValueRef.current(e.clientX);
};
const handleTouchMove = (e: TouchEvent) => {
e.preventDefault();
- updateValue(e.touches[0].clientX);
+ updateValueRef.current(e.touches[0].clientX);
};
const handleEnd = () => {
setIsDragging(false);
};
document.addEventListener("mousemove", handleMouseMove);
document.addEventListener("mouseup", handleEnd);
document.addEventListener("touchmove", handleTouchMove, { passive: false });
document.addEventListener("touchend", handleEnd);
return () => {
document.removeEventListener("mousemove", handleMouseMove);
document.removeEventListener("mouseup", handleEnd);
document.removeEventListener("touchmove", handleTouchMove);
document.removeEventListener("touchend", handleEnd);
};
- }, [isDragging, value, min, max, step, type, disabled]);
+ }, [isDragging]);Also applies to: 77-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.tsx` around lines 42 - 59, updateValue closes
over onChange causing a stale-closure bug and listener churn; make onChange
stable for the mouse/touch listeners by storing it in a ref and reading that ref
inside updateValue. Concretely, add an onChangeRef = useRef(onChange) and sync
it in a small effect (onChangeRef.current = onChange) whenever onChange changes,
then change updateValue to call onChangeRef.current(newValue) instead of
onChange(newValue); finally, adjust the effect that registers pointer/mouse
listeners (the useEffect that currently depends on value and registers
updateValue) to remove onChange and value from its dependency array so listeners
are stable and only re-register when disabled/sliderRef/type/min/max/step
change.
| let newValue = min + percent * (max - min); | ||
|
|
||
| // Snap to step | ||
| const actualStep = type === "discrete" ? step : 1; |
There was a problem hiding this comment.
step prop is silently ignored in "continuous" mode.
In continuous mode actualStep is hardcoded to 1, meaning even if the consumer passes step={0.5}, the slider will only produce integer values. This is surprising undocumented behavior — consumers would reasonably expect step to be respected in both modes.
Either honour step in continuous mode or document the constraint in the prop's JSDoc.
♻️ Option: honour step in both modes
- const actualStep = type === "discrete" ? step : 1;
+ const actualStep = step;📝 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
| const actualStep = type === "discrete" ? step : 1; | |
| const actualStep = step; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.tsx` at line 52, The slider currently ignores
the passed step in continuous mode because actualStep is set to 1 when type !==
"discrete"; update the logic around actualStep in Slider.tsx (the computation
using actualStep and the handling in functions like onChange/onPointerMove that
use it) so that actualStep = step regardless of type (or validate/normalize step
for continuous mode) and ensure any value rounding/clamping uses this
actualStep; alternatively, if you choose to keep discrete-only behavior, update
the prop JSDoc for the step prop to explicitly state that step is only honored
when type === "discrete" and add runtime validation/warnings (e.g.,
console.warn) when a consumer passes step with type "continuous".
Comment on lines
+123
to
+187
| return ( | ||
| <div className={`${classes.container} ${className}`}> | ||
| {label && <label className={`label ${classes.label}`}>{label}</label>} | ||
|
|
||
| <div className={classes.sliderWrapper}> | ||
| {/* Min value */} | ||
| {showMinMax && ( | ||
| <span className={`${classes.value} ${classes.minValue}`}> | ||
| {formatValue(min)} | ||
| </span> | ||
| )} | ||
|
|
||
| {/* Slider track */} | ||
| <div | ||
| ref={sliderRef} | ||
| className={`${classes.sliderTrack} ${classes[size]} ${disabled ? classes.disabled : ""}`} | ||
| onMouseDown={handleMouseDown} | ||
| onTouchStart={handleTouchStart} | ||
| > | ||
| {/* Filled track */} | ||
| <div | ||
| // Add the dragging conditional class here: | ||
| className={`${classes.sliderFill} ${isDragging ? classes.dragging : ""}`} | ||
| style={{ width: `${percentage}%` }} | ||
| /> | ||
|
|
||
| {/* Discrete step markers */} | ||
| {type === "discrete" && ( | ||
| <div className={classes.stepsContainer}> | ||
| <div className={classes.stepsSubContainer}> | ||
| {generateSteps().map((step, i) => ( | ||
| <div | ||
| key={i} | ||
| className={`${classes.stepMarker} ${step.isActive ? classes.active : ""}`} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Thumb */} | ||
| <div | ||
| className={`${classes.thumb} ${isDragging ? classes.dragging : ""}`} | ||
| style={{ | ||
| left: `${percentage === 0 ? 1 : percentage === 100 ? 99 : percentage}%`, | ||
| }} | ||
| onMouseEnter={() => setShowTooltip(true)} | ||
| onMouseLeave={() => !isDragging && setShowTooltip(false)} | ||
| > | ||
| {/* Tooltip */} | ||
| {(showTooltip || isDragging) && ( | ||
| <div className={classes.tooltip}>{formatValue(value)}</div> | ||
| )} | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Max value */} | ||
| {showMinMax && ( | ||
| <span className={`${classes.value} ${classes.maxValue}`}> | ||
| {formatValue(max)} | ||
| </span> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Missing keyboard accessibility and ARIA attributes.
The slider is not keyboard-accessible or screen-reader-friendly. For a design system component this is a blocker for WCAG 2.1 Level AA compliance:
- No
role="slider"on the thumb. - No
aria-valuemin,aria-valuemax,aria-valuenow. - No
tabIndex→ component cannot be focused with Tab. - No
onKeyDown→ Arrow keys, Home, and End do nothing. - The
labelprop is not associated to the interactive element viaaria-labelledbyoraria-label.
🛡️ Suggested additions to the thumb element
<div
className={`${classes.thumb} ${isDragging ? classes.dragging : ""}`}
style={{
left: `${percentage === 0 ? 1 : percentage === 100 ? 99 : percentage}%`,
}}
+ role="slider"
+ tabIndex={disabled ? -1 : 0}
+ aria-valuemin={min}
+ aria-valuemax={max}
+ aria-valuenow={value}
+ aria-label={label}
+ aria-disabled={disabled}
+ onKeyDown={(e) => {
+ if (disabled) return;
+ const actualStep = type === "discrete" ? step : 1;
+ if (e.key === "ArrowRight" || e.key === "ArrowUp") {
+ e.preventDefault();
+ onChange(Math.min(max, Math.round((value + actualStep) / actualStep) * actualStep));
+ } else if (e.key === "ArrowLeft" || e.key === "ArrowDown") {
+ e.preventDefault();
+ onChange(Math.max(min, Math.round((value - actualStep) / actualStep) * actualStep));
+ } else if (e.key === "Home") {
+ e.preventDefault();
+ onChange(min);
+ } else if (e.key === "End") {
+ e.preventDefault();
+ onChange(max);
+ }
+ }}
onMouseEnter={() => setShowTooltip(true)}
onMouseLeave={() => !isDragging && setShowTooltip(false)}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.tsx` around lines 123 - 187, The thumb element
in the Slider component lacks keyboard and ARIA support; make the thumb
focusable and screen-reader friendly by adding role="slider", tabIndex={disabled
? -1 : 0}, and the ARIA attributes aria-valuemin={min}, aria-valuemax={max},
aria-valuenow={value} (or computed value), plus aria-label or aria-labelledby
that references the label when provided; implement an onKeyDown handler (e.g.,
handleKeyDown) on the thumb to handle
ArrowLeft/ArrowRight/ArrowUp/ArrowDown/Home/End to update value using existing
change logic (reuse setValue/handleMove functions) and ensure focus and tooltip
behavior (setShowTooltip/isDragging) integrate with keyboard interactions.
Ensure the same ARIA/keyboard behavior is applied for both continuous and
discrete Slider types and avoid keyboard focus when disabled.
|
|
||
| return ( | ||
| <div className={`${classes.container} ${className}`}> | ||
| {label && <label className={`label ${classes.label}`}>{label}</label>} |
There was a problem hiding this comment.
"label" is a bare global class string, not a CSS module reference.
className={\label ${classes.label}`}prepends a plain"label"string. If no global.label` rule exists this is a no-op. Either remove it or replace it with the appropriate module class.
♻️ Proposed fix
- {label && <label className={`label ${classes.label}`}>{label}</label>}
+ {label && <label className={classes.label}>{label}</label>}📝 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
| {label && <label className={`label ${classes.label}`}>{label}</label>} | |
| {label && <label className={classes.label}>{label}</label>} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.tsx` at line 125, The label element in
Slider.tsx uses a bare global "label" class string alongside the CSS module
class (className={`label ${classes.label}`}) which is incorrect; update the
label rendering (the JSX that references label and classes.label) to remove the
plain "label" or replace it with the module equivalent (e.g., classes.labelRoot
or the correct exported key from the CSS module), so className only uses valid
CSS-module identifiers (or a single string from the module) and ensure the CSS
module file exports the corresponding class name.
📝 WalkthroughWalkthroughA new Slider component is introduced with accompanying CSS styling and example component, providing interactive continuous and discrete slider functionality with configurable size, labels, and tooltip displays. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/slider/Slider.tsx (1)
153-158: Map callback parameterstepshadows the outerstepprop.Inside the
.mapcallback,steprefers to the{value, isActive}object, not the numeric step size. Any future edit touchingstepinside this callback risks using the wrong binding silently.🔧 Proposed rename
- {generateSteps().map((step, i) => ( + {generateSteps().map((stepMarker, i) => ( <div key={i} - className={`${classes.stepMarker} ${step.isActive ? classes.active : ""}`} + className={`${classes.stepMarker} ${stepMarker.isActive ? classes.active : ""}`} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/slider/Slider.tsx` around lines 153 - 158, The map callback in generateSteps().map currently uses the parameter name "step", which shadows the outer "step" prop; rename the iterator parameter (e.g., to "stepObj" or "stepItem") inside the map so references inside the callback clearly target the step object returned by generateSteps() and not the outer prop, updating usages like step.isActive and the JSX key accordingly (look for generateSteps(), classes.stepMarker, classes.active and the outer step prop to ensure no other references still shadow).src/components/slider/Slider.module.css (1)
51-51: Deadwidth: 100px—flex: 1already controls the track width.Since
.sliderWrapperis alwaysdisplay: flex,flex: 1on.sliderTrackoverrideswidth: 100pxunconditionally. The explicitwidthdeclaration is never applied.🧹 Proposed cleanup
.sliderTrack { position: relative; flex: 1; background: var(--surface-tertiary); border: 1px solid var(--border-default); - width: 100px; border-radius: 100px; cursor: pointer; transition: all 0.2s ease; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/slider/Slider.module.css` at line 51, Remove the dead width declaration from the CSS: delete the "width: 100px" rule under the .sliderTrack selector since .sliderWrapper is always display:flex and .sliderTrack already uses flex: 1 to control width; leaving both is redundant—remove the width line so .sliderTrack relies solely on flex sizing.
🤖 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/slider/Slider.tsx`:
- Around line 89-91: Tooltip remains visible when dragging ends outside the
thumb because handleEnd only clears isDragging; update handleEnd (and/or the
global pointerup handler) to also clear showTooltip by calling
setShowTooltip(false) so the tooltip is hidden whenever a drag ends outside the
thumb (refer to handleEnd, setIsDragging, showTooltip, setShowTooltip, and
onMouseLeave to implement and ensure consistency).
- Around line 112-121: generateSteps can infinite-loop when step <= 0 and
updateValue produces NaN; add guards in both generateSteps and updateValue:
compute an actualStep = step > 0 ? step : 1 (or return an empty steps array in
generateSteps) and use actualStep in the for loop (for (let i = min; i <= max; i
+= actualStep)) and for rounding in updateValue (Math.round(newValue /
actualStep) * actualStep); also ensure you never call onChange with NaN by
checking Number.isFinite(result) before calling onChange in updateValue.
Reference the generateSteps and updateValue functions to locate the changes.
---
Duplicate comments:
In `@src/components/slider/Example.tsx`:
- Line 29: The caption line uses a bare global class and wrong precision:
replace the hardcoded className={`caption`} with the component's CSS-module
class (e.g. styles.caption or the correct imported identifier used elsewhere in
Example.tsx) and change the display of continuousValue from toFixed(1) to
toFixed(0) (or remove toFixed and render the raw number) so the shown value
matches the integer slider steps; update the JSX that references continuousValue
and the caption class accordingly.
In `@src/components/slider/Slider.module.css`:
- Around line 226-233: The .stepMarker CSS rule contains a duplicate background
declaration—remove the first "background: var(--surface-hover)" in the
.stepMarker block so only the intended "background: var(--primary-400)" remains;
locate the .stepMarker selector in Slider.module.css and delete the redundant
background property to satisfy Stylelint's
declaration-block-no-duplicate-properties.
- Line 175: Rename the keyframes identifier tooltipFadeIn to tooltip-fade-in in
both the `@keyframes` declaration and where it’s referenced; update the `@keyframes`
tooltipFadeIn {...} block to `@keyframes` tooltip-fade-in and change the .tooltip
rule’s animation property from tooltipFadeIn to tooltip-fade-in so the keyframes
name pattern matches stylelint expectations.
In `@src/components/slider/Slider.tsx`:
- Line 39: Guard the division by zero when computing percentage: change the
percentage calculation in Slider.tsx so it checks for max === min and uses a
safe fallback (e.g. 0 or 100 depending on desired behavior) instead of
performing ((value - min) / (max - min)) * 100; update the symbol percentage
(which is used for style width and left) to be assigned via a conditional or a
safe clamp using value, min, and max to avoid NaN propagation to style={{ width
}} and left.
- Line 125: The JSX for the label element uses a bare global "label" class
string (`<label className={`label ${classes.label}`}>`) which will not match the
CSS module; update the label element in Slider.tsx to rely on the CSS module
only (use classes.label) or merge module classes via your existing class-merge
helper if needed, ensuring you remove the literal "label" token so the element
uses the scoped CSS module class (target the label element rendering in
Slider.tsx).
- Around line 163-176: The thumb div currently lacks keyboard and ARIA support;
change the element identified by classes.thumb (the div that uses isDragging,
percentage, setShowTooltip and displays formatValue(value)) into a focusable,
accessible slider by giving it role="slider", tabIndex={0}, aria-valuemin and
aria-valuemax (match the component's min/max), aria-valuenow={value} and
aria-valuetext={formatValue(value)}, and add an onKeyDown handler that adjusts
the value on ArrowLeft/ArrowRight (and PageUp/PageDown/Home/End as appropriate)
while calling the existing value update logic and preserving
isDragging/showTooltip behavior so keyboard users can move the thumb.
- Around line 77-104: The effect registering document listeners uses updateValue
which closes over onChange but onChange is not in the dependency array and value
is included causing needless teardown; fix by making updateValue stable (wrap
updateValue in useCallback or move its latest implementation into a ref) and
then update the useEffect deps to include onChange (and not value) so listeners
aren’t re-registered during dragging; specifically adjust the functions
updateValue and the useEffect that listens to
isDragging/handleMouseMove/handleTouchMove/handleEnd so updateValue captures the
current onChange (or reads it from a ref) and the effect depends on isDragging,
min, max, step, type, disabled and onChange (remove value).
- Line 52: actualStep currently forces a value of 1 for continuous sliders,
ignoring the passed step prop; change the calculation in Slider.tsx so
actualStep uses the provided step for both modes (falling back to a sensible
default like 1 only if step is undefined). Update the expression that computes
actualStep (the constant named actualStep that checks type === "discrete") to
read the step prop directly (e.g., use step ?? 1) so step={0.5} works in
continuous mode as well.
---
Nitpick comments:
In `@src/components/slider/Slider.module.css`:
- Line 51: Remove the dead width declaration from the CSS: delete the "width:
100px" rule under the .sliderTrack selector since .sliderWrapper is always
display:flex and .sliderTrack already uses flex: 1 to control width; leaving
both is redundant—remove the width line so .sliderTrack relies solely on flex
sizing.
In `@src/components/slider/Slider.tsx`:
- Around line 153-158: The map callback in generateSteps().map currently uses
the parameter name "step", which shadows the outer "step" prop; rename the
iterator parameter (e.g., to "stepObj" or "stepItem") inside the map so
references inside the callback clearly target the step object returned by
generateSteps() and not the outer prop, updating usages like step.isActive and
the JSX key accordingly (look for generateSteps(), classes.stepMarker,
classes.active and the outer step prop to ensure no other references still
shadow).
Comment on lines
+89
to
+91
| const handleEnd = () => { | ||
| setIsDragging(false); | ||
| }; |
There was a problem hiding this comment.
Tooltip stays visible after drag ends outside the thumb.
When the user hovers the thumb (showTooltip → true), drags away, and releases outside the thumb, handleEnd only clears isDragging. onMouseLeave never fires (cursor isn't over the thumb), so showTooltip stays true and the tooltip persists indefinitely.
🛡️ Proposed fix
const handleEnd = () => {
setIsDragging(false);
+ setShowTooltip(false);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.tsx` around lines 89 - 91, Tooltip remains
visible when dragging ends outside the thumb because handleEnd only clears
isDragging; update handleEnd (and/or the global pointerup handler) to also clear
showTooltip by calling setShowTooltip(false) so the tooltip is hidden whenever a
drag ends outside the thumb (refer to handleEnd, setIsDragging, showTooltip,
setShowTooltip, and onMouseLeave to implement and ensure consistency).
Comment on lines
+112
to
+121
| const generateSteps = () => { | ||
| const steps = []; | ||
| for (let i = min; i <= max; i += step) { | ||
| steps.push({ | ||
| value: i, | ||
| isActive: i <= value, | ||
| }); | ||
| } | ||
| return steps; | ||
| }; |
There was a problem hiding this comment.
generateSteps causes an infinite loop when step ≤ 0 in discrete mode.
for (let i = min; i <= max; i += step) never terminates if step is 0 (or negative with min < max), freezing the tab. Concurrently, updateValue computes Math.round(newValue / actualStep) * actualStep which evaluates to NaN when actualStep = 0, and since NaN !== value is always true, onChange(NaN) fires on every pointer move.
Add an early guard in both locations:
🛡️ Proposed fix
+ // Guard: step must be positive
+ const safeStep = Math.max(step, Number.EPSILON);
// Handle value calculation and update
const updateValue = (clientX: number) => {
...
- const actualStep = type === "discrete" ? step : 1;
+ const actualStep = type === "discrete" ? safeStep : 1;
newValue = Math.round(newValue / actualStep) * actualStep;
...
};
const generateSteps = () => {
+ if (safeStep <= 0) return [];
const steps = [];
- for (let i = min; i <= max; i += step) {
+ for (let i = min; i <= max; i += safeStep) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/slider/Slider.tsx` around lines 112 - 121, generateSteps can
infinite-loop when step <= 0 and updateValue produces NaN; add guards in both
generateSteps and updateValue: compute an actualStep = step > 0 ? step : 1 (or
return an empty steps array in generateSteps) and use actualStep in the for loop
(for (let i = min; i <= max; i += actualStep)) and for rounding in updateValue
(Math.round(newValue / actualStep) * actualStep); also ensure you never call
onChange with NaN by checking Number.isFinite(result) before calling onChange in
updateValue. Reference the generateSteps and updateValue functions to locate the
changes.
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.
Summary by CodeRabbit