fix: debounce slider to avoid laggy behavior #18980
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes add support for a new "slider" form type in the Changes
Estimated code review effort2 (~15 minutes) ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx (1)
413-433: Refactor slider value conversion & guard validation defaultsTo clean up duplication and safely handle missing validations, consider pulling out your computed values and defaults at the top of the render case:
+ // Compute once at render-time + const numericValue = Number.isFinite(Number(localValue)) ? Number(localValue) : 0; + const { + validation_min: min = 0, + validation_max: max = 100, + } = parameter.validations[0] ?? {}; case "slider": return ( <div className="flex flex-row items-baseline gap-3"> <Slider id={id} className="mt-2" - value={[numericValue]} + value={[numericValue]} onValueChange={([value]) => { setLocalValue(value.toString()); }} - min={parameter.validations[0]?.validation_min ?? 0} - max={parameter.validations[0]?.validation_max ?? 100} + min={min} + max={max} disabled={disabled} /> - <span className="min-w-8 font-medium"> - {numericValue} - </span> + <span className="min-w-8 font-medium"> + {numericValue} + </span> </div> );• Eliminates repeated
Number()checks by centralizingnumericValue
• Destructuresmin/maxwith clear defaults ifvalidations[0]is absent
• Usesmin-w-8so larger numbers don’t overflow the display spanPlease verify that every slider-type parameter always supplies a first validation object with both
validation_minandvalidation_max; if not, adjust the data model or add an explicit guard.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx (2)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: chromatic
- GitHub Check: test-e2e
- GitHub Check: test-js
- GitHub Check: lint
- GitHub Check: fmt
- GitHub Check: gen
🔇 Additional comments (1)
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx (1)
80-81: Good consolidation of slider handling into debounced field.Routing slider parameters to
DebouncedParameterFieldis the correct approach for addressing laggy behavior, as sliders can generate rapid value changes during user interaction.
resolves #18856
resolves coder/internal#753