Slider: Made min-max clamp only apply if clamping would change the value

[Anders2303]

No description provided.

[Copilot]

🔵 Needs a closer look

A moderate issue remains when controlled value and bounds change together, potentially allowing an out-of-range value to escape clamping.

Pull request overview

Updates Slider range clamping to avoid redundant updates when the clamped value is unchanged.

Changes:

• Adds equality checks before scheduling range clamps.
• Updates controlled slider bound-change handling.

File summaries

File	Summary
frontend/src/lib/components/Slider/slider.tsx	Adds conditional clamp scheduling.

Review details

Suppressed comments (1)

frontend/src/lib/components/Slider/slider.tsx:364

• When value and min/max change together on a controlled slider, setInternalValue(props.value) above is a render-phase update, so internalValue here is still the previous value. If that previous value is already within the new bounds, this check skips queuing a clamp; the rerender has already updated prevMin/prevMax, so the newly supplied out-of-range value is never clamped. Base the bounds-change calculation on the effective controlled value after it is synchronized, or defer the clamp until that value is applied.

        if (!isEqual(newValue, internalValue)) {

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rubenthoms]

Look at Copilot's comment.
When props.value changes, internalValue is mirrored via a render-phase setInternalValue call - but the local internalValue variable in the current render still holds the old value. The clamp-on-bounds-change block at lines 346-367 then derives clampedValue from that stale internalValue and immediately marks prevMin/prevMax as synced. On React's synchronous re-render (triggered by the render-phase setState), internalValue now equals the new (possibly out-of-range) props.value, but the following conditions are already true prevMin === min and prevMax === max, so the clamp block never runs again - the new controlled value escapes clamping entirely.

Possible fix:

// The value `internalValue` is about to become once the render-phase update below is applied.
// Used (instead of `internalValue`) for the bounds-clamp check so a controlled value change and a
// bounds change landing in the same render don't let an out-of-range value escape clamping.
const currentValue = props.value !== undefined ? props.value : internalValue;

and then

let clampedValue = isDualSlider ? clone(currentValue as number[]) : ([currentValue, currentValue] as number[]);

and

if (!isEqual(newValue, currentValue)) {
    setValueToClamp(isDualSlider ? clampedValue : clampedValue[0]);
}

[Copilot]

🟢 Approval recommended

No unresolved review issues were identified.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[rubenthoms]

LGTM