[Color Picker] Only trigger onChange callback when internal change is made - #260
Conversation
…-tied internal change was made. Do not trigger it due to external change (aka: init, or value prop change)
|
@joan-teriihoania is attempting to deploy a commit to the Haste Ventures Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdded internal dirty-tracking to distinguish user-initiated color changes from external prop updates. Wrapped setter methods to mark changes as dirty. On external value changes, state is marked clean before syncing color channels. onChange now fires only for dirty (user-driven) updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Prop as External prop (value)
participant CP as ColorPicker
participant State as HSLA State
participant CB as onChange
rect rgba(200,200,255,0.25)
note over Prop,CP: External update flow (clean)
Prop->>CP: value prop changes
CP->>CP: markAsClean()
CP->>State: update hue/sat/light/alpha
CP--xCB: skip onChange (not dirty)
end
rect rgba(200,255,200,0.25)
note over CP,CB: User interaction flow (dirty)
CP->>CP: setHue/setSaturation/setLightness/setAlpha (wrapped)
CP->>CP: markAsDirty()
CP->>State: apply channel update
alt onChange provided
CP->>CB: onChange(newColor)
else
CP-->>CP: no-op
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/color-picker/index.tsx (1)
98-112: Critical: external value sync is mapping RGB → HSL state incorrectly (and alpha scale is wrong).You’re setting HSL state using r/g/b channels and passing alpha in [0–1] to state that expects [0–100]. Convert the incoming value to HSL and scale alpha before updating.
Apply this diff:
// Update color when controlled value changes useEffect(() => { - if (value) { - const color = Color.rgb(value).rgb().object(); - - // Changes to the value prop are considered external changes - // and should not trigger an onChange event. - markAsClean() - - setHue(color.r); - setSaturation(color.g); - setLightness(color.b); - setAlpha(color.a); - } + if (value != null) { + const c = Color(value); + const [h, s, l] = c.hsl().array(); + + // External sync: do not emit onChange. + markAsClean(); + + setHue(h); + setSaturation(s); + setLightness(l); + setAlpha(Math.round(c.alpha() * 100)); + } }, [value]);
🧹 Nitpick comments (1)
packages/color-picker/index.tsx (1)
132-147: Preserve function types; avoid rest parameters in setters.Using
(...args)widens the type and can mask mistakes. Keep the signature(n: number) => voidper context contract.- setHue: (...args) => { - markAsDirty(); - setHue(...args) - }, + setHue: (n: number) => { + markAsDirty(); + setHue(n); + }, - setSaturation: (...args) => { - markAsDirty(); - setSaturation(...args) - }, + setSaturation: (n: number) => { + markAsDirty(); + setSaturation(n); + }, - setLightness: (...args) => { - markAsDirty(); - setLightness(...args) - }, + setLightness: (n: number) => { + markAsDirty(); + setLightness(n); + }, - setAlpha: (...args) => { - markAsDirty(); - setAlpha(...args) - }, + setAlpha: (n: number) => { + markAsDirty(); + setAlpha(n); + },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/color-picker/index.tsx(4 hunks)
🔇 Additional comments (2)
packages/color-picker/index.tsx (2)
72-83: LGTM on dirty-tracking.The isDirty ref + markAsDirty/markAsClean pattern cleanly gates user-initiated updates.
116-116: Gated onChange looks correct.Emits only when dirty; aligns with the PR’s goal and avoids loops when parent echoes value back.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
1 participant
Description
Hello!
Currently, the
ColorPickercomponent will trigger theonChangecallback when:hue,lightness,saturation, andalphavaluesvalueprop is updatedSemantically and logically, we should expect the
onChangecallback to be triggered when an internal value is changed by the user, and the parent component needs to be informed of that change. This is what point 1 is doing.In the same thinking, I do not believe that the callback should be triggered at initialisation (point 2), I would expect something like
onInitto be used for this.Finally,
onChangeshould not be triggered when the internal value state was updated by an external state change. In this case, point 3 is an externally-triggered state change that causes an internal update. Not only could this cause an infinite update loop, but this also goes against the usual behaviour of other HTML elements'onChangecallbacks which are not triggered when the value is updated programatically, but only through an explicit user interaction. Plus,onChangeusually serves as a way to inform the parent of a state change. But in this case, the state change was caused by the parent and thus doesn't need to be notified.For all these reasons, I propose to:
onChangecallback if a state update was made by an internal state change call. This is what theisDirtyref is used for, and why I have made so that a setter call to the provider will mark the current state change asdirtywhich will allow for anonChangecall;valueprop. Which is why I am marking any state change made by it as "clean".Checklist
Additional notes
This is one my first few open-source MRs! Please let me know if I am doing something wrong!
Summary by CodeRabbit