fix: respect initial value prop in ButtonToggleGroup #1796
Conversation
Previously the value prop was ignored during initialization. Now properly initializes selectedValues from the value prop, handling type mismatches between single/multi-select modes.
|
@shinokada is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughButtonToggleGroup now normalizes its incoming Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Parent Component
participant BTG as ButtonToggleGroup
participant Buttons as ButtonToggle items
Note over Parent,BTG #DDEBF7: Initialization (new)
Parent->>BTG: pass `value` prop
BTG->>BTG: getInitialValue(value, multiSelect)
BTG->>BTG: set selectedValues (normalized)
Note over Buttons,BTG #F7F1DE: Interaction & notification
Buttons->>BTG: user toggles a button
BTG->>BTG: update selectedValues
BTG-->>Parent: emit onSelect(selectedValues)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/forms/button-toggle/ButtonToggleGroup.svelte (2)
28-50: Normalize and clone arrays to avoid aliasing; small rename helps reuse.
getInitialValue()returns the original array reference whenvalueis an array. If the parent mutates that array, internal state can desync without triggering reactivity. Clone arrays and consider renaming to reflect coercion.Apply:
- // FIX: Initialize selectedValues FROM the value prop - // Handle type mismatches between multiSelect mode and value type - function getInitialValue(): SelectedValue { + // Normalize incoming prop `value` to internal SelectedValue + function getInitialValue(): SelectedValue { if (multiSelect) { // Multi-select mode expects array if (Array.isArray(value)) { - return value; + return [...value]; } else if (value === null || value === undefined) { return []; } else { // Single string passed but multiSelect is true - wrap in array - return [value as string]; + return [value as string]; } } else { // Single-select mode expects string or null if (Array.isArray(value)) { // Array passed but multiSelect is false - take first item return value[0] ?? null; } else { return value; } } }
59-62: Don’t assign to prop; add prop→state sync effect instead.
value = selectedValues;mutates the local prop variable without external effect and can confuse readers. Remove it and add an effect that updates internal state when the parent changesvalue.Apply:
- $effect(() => { - value = selectedValues; - onSelect(selectedValues); - }); + // Notify parent when selection changes + $effect(() => { + (typeof emitSelect === 'function' ? emitSelect : onSelect)(selectedValues); + }); + + // Synchronize internal state when the parent updates `value` + function valuesEqual(a: SelectedValue, b: SelectedValue): boolean { + const isArrA = Array.isArray(a); + const isArrB = Array.isArray(b); + if (isArrA !== isArrB) return false; + if (isArrA && isArrB) { + const aa = a as string[], bb = b as string[]; + if (aa.length !== bb.length) return false; + const set = new Set(aa); + for (const v of bb) if (!set.has(v)) return false; + return true; + } + return a === b; + } + + $effect(() => { + const next = getInitialValue(); + if (!valuesEqual(next, selectedValues)) { + selectedValues = Array.isArray(next) ? [...(next as string[])] : next; + } + });Note: order-insensitive equality for arrays avoids loops if parent reorders values. Verify this matches expected semantics.
src/routes/docs-examples/extend/button-toggle/InitialValue.svelte (1)
1-22: Example is clear; minor TS nicety optional.Looks good and demonstrates both modes. Optionally annotate callback args for clarity:
(v: string | null)and(v: string[]).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/forms/button-toggle/ButtonToggleGroup.svelte(2 hunks)src/routes/docs-examples/extend/button-toggle/InitialValue.svelte(1 hunks)src/routes/docs/extend/button-toggle.md(1 hunks)
🧰 Additional context used
🪛 ESLint
src/lib/forms/button-toggle/ButtonToggleGroup.svelte
[error] 15-15: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
⏰ 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). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (2)
src/routes/docs/extend/button-toggle.md (1)
32-38: Clarify controlled behavior (prop updates after mount).Text says “external state control,” but current implementation initializes from
valueonly. If parent later changesvalue, selection won’t update unless the component syncs from props. Either:
- add a note that only initial
valueis respected, or- implement prop→state sync (see suggested change in ButtonToggleGroup.svelte comment).
src/lib/forms/button-toggle/ButtonToggleGroup.svelte (1)
52-52: LGTM on initial state wiring.Initializing
selectedValuesfrom the normalizedvaluefixes the “ignored on init” bug.
| let { | ||
| multiSelect = false, | ||
| name = "toggle-group", | ||
| value = multiSelect ? [] : null, | ||
| color, | ||
| size = "md", | ||
| roundedSize = "md", | ||
| onSelect = (val: any) => {}, | ||
| children, | ||
| ctxIconClass, | ||
| ctxBtnClass, | ||
| class: className, | ||
| ...restProps | ||
| }: ButtonToggleGroupProps = $props(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch details of the two commits mentioned by the user
gh api repos/themesberg/flowbite-svelte/commits/e0bc710bdb0e47bf59894b1985f4886107d2fe63 --jq '.commit.message, .files[] | select(.filename == "src/lib/forms/button-toggle/ButtonToggleGroup.svelte") | .patch' || echo "Failed to fetch first commit"
echo "---COMMIT SEPARATOR---"
gh api repos/themesberg/flowbite-svelte/commits/181d9caca45bd61eb4fc423ed66c310c564aded3 --jq '.commit.message, .files[] | select(.filename == "src/lib/forms/button-toggle/ButtonToggleGroup.svelte") | .patch' || echo "Failed to fetch second commit"
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Fetch the first commit details
echo "=== Commit e0bc710 ==="
gh api repos/themesberg/flowbite-svelte/commits/e0bc710bdb0e47bf59894b1985f4886107d2fe63 --jq '.commit.message'
gh api repos/themesberg/flowbite-svelte/commits/e0bc710bdb0e47bf59894b1985f4886107d2fe63 --jq '.files[] | select(.filename == "src/lib/forms/button-toggle/ButtonToggleGroup.svelte") | .patch'
echo ""
echo "=== Commit 181d9ca ==="
gh api repos/themesberg/flowbite-svelte/commits/181d9caca45bd61eb4fc423ed66c310c564aded3 --jq '.commit.message'
gh api repos/themesberg/flowbite-svelte/commits/181d9caca45bd61eb4fc423ed66c310c564aded3 --jq '.files[] | select(.filename == "src/lib/forms/button-toggle/ButtonToggleGroup.svelte") | .patch'
Length of output: 1348
@shinokada Thank you for addressing the typing issue! The fix in commit 181d9ca successfully eliminates the any type by simplifying the default function signature. This is a clean solution that resolves the ESLint warning while still allowing consumers to pass callbacks that accept the selection value.
The array cloning in commit e0bc710 is also a good addition to prevent external mutations from affecting internal state. Both changes look great! ✨
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Prevents external array mutations from affecting internal state.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/forms/button-toggle/ButtonToggleGroup.svelte (1)
8-21: Address theanytype inonSelect.The use of
anyfor theonSelectparameter was flagged in a previous review and remains unaddressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/forms/button-toggle/ButtonToggleGroup.svelte(2 hunks)
🧰 Additional context used
🪛 ESLint
src/lib/forms/button-toggle/ButtonToggleGroup.svelte
[error] 15-15: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
⏰ 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). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (2)
src/lib/forms/button-toggle/ButtonToggleGroup.svelte (2)
28-50: Well-designed initialization logic.The
getInitialValue()function properly normalizes the value prop for both modes, handles type mismatches gracefully, and clones arrays to prevent aliasing. The implementation and comments are clear.
52-52: Verify: Shouldvalueprop updates be reactive?The component reads the
valueprop only during initialization. If the parent updatesvalueafter mount,selectedValueswon't reflect that change. Confirm whether this uncontrolled behavior is intentional, or if the component should react to externalvalueupdates.
| let { | ||
| multiSelect = false, | ||
| name = "toggle-group", | ||
| value = multiSelect ? [] : null, | ||
| color, | ||
| size = "md", | ||
| roundedSize = "md", | ||
| onSelect = (val: any) => {}, | ||
| children, | ||
| ctxIconClass, | ||
| ctxBtnClass, | ||
| class: className, | ||
| ...restProps | ||
| }: ButtonToggleGroupProps = $props(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/forms/button-toggle/ButtonToggleGroup.svelte (1)
59-62: The$effectstill fires on mount with initial value.As previously flagged, the
$effectruns immediately when the component mounts, invokingonSelect(selectedValues)with the initial value before any user interaction. This can trigger unintended side effects in parent components during initialization.Consider one of these approaches:
Option 1: Guard the initial run with a mounted flag:
+ let isMounted = $state(false); + // Notify parent when selection changes $effect(() => { + if (!isMounted) { + isMounted = true; + return; + } onSelect(selectedValues); });Option 2: Remove the
$effectand callonSelectonly fromtoggleSelected:function toggleSelected(toggleValue: string) { if (multiSelect) { const currentSelected = [...(selectedValues as string[])]; const index = currentSelected.indexOf(toggleValue); if (index === -1) { selectedValues = [...currentSelected, toggleValue]; } else { currentSelected.splice(index, 1); selectedValues = currentSelected; } } else { selectedValues = toggleValue === selectedValues ? null : toggleValue; } + onSelect(selectedValues); }And remove the
$effectblock entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/forms/button-toggle/ButtonToggleGroup.svelte(2 hunks)src/lib/types.ts(1 hunks)
⏰ 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). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (2)
src/lib/types.ts (1)
330-330: LGTM! Type signature improved.The change from
anytostring | null | string[]provides proper typing that aligns with both single-select (string or null) and multi-select (string array) modes.src/lib/forms/button-toggle/ButtonToggleGroup.svelte (1)
28-50: LGTM! Proper value normalization.The
getInitialValue()function correctly normalizes the incomingvalueprop for both single-select and multi-select modes. Array cloning prevents external mutations, and the type coercion logic handles edge cases appropriately.
| let { | ||
| multiSelect = false, | ||
| name = "toggle-group", | ||
| value = multiSelect ? [] : null, | ||
| color, | ||
| size = "md", | ||
| roundedSize = "md", | ||
| onSelect = () => {}, | ||
| children, | ||
| ctxIconClass, | ||
| ctxBtnClass, | ||
| class: className, | ||
| ...restProps | ||
| }: ButtonToggleGroupProps = $props(); |
There was a problem hiding this comment.
Fix default onSelect signature to accept parameter.
The default no-op () => {} doesn't accept any parameters, but it's invoked with selectedValues at line 61 and the type signature in types.ts specifies (val: string | null | string[]) => void. While JavaScript silently ignores the extra argument, this mismatch is semantically incorrect.
Apply this diff:
- onSelect = () => {},
+ onSelect = (_val: string | null | string[]) => {}, 📝 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.
| let { | |
| multiSelect = false, | |
| name = "toggle-group", | |
| value = multiSelect ? [] : null, | |
| color, | |
| size = "md", | |
| roundedSize = "md", | |
| onSelect = () => {}, | |
| children, | |
| ctxIconClass, | |
| ctxBtnClass, | |
| class: className, | |
| ...restProps | |
| }: ButtonToggleGroupProps = $props(); | |
| let { | |
| multiSelect = false, | |
| name = "toggle-group", | |
| value = multiSelect ? [] : null, | |
| color, | |
| size = "md", | |
| roundedSize = "md", | |
| onSelect = (_val: string | null | string[]) => {}, | |
| children, | |
| ctxIconClass, | |
| ctxBtnClass, | |
| class: className, | |
| ...restProps | |
| }: ButtonToggleGroupProps = $props(); |
🤖 Prompt for AI Agents
In src/lib/forms/button-toggle/ButtonToggleGroup.svelte around lines 8 to 21,
the default onSelect is declared as () => {} but the component calls
onSelect(selectedValues) and the types expect a signature of (val: string | null
| string[]) => void; change the default no-op to accept a parameter (e.g. (val)
=> {} or (_val) => {}) so its signature matches the typed invocation, keeping
behavior identical while satisfying the contract.
There was a problem hiding this comment.
@shinokada, thank you for fixing the onSelect signature! 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/routes/docs-examples/extend/button-toggle/RoundedSize.svelte (1)
6-11: Type guard pattern works correctly but is duplicated across examples.The
handleSingleSelectimplementation correctly handles the widenedonSelectsignature by using a runtime guard to process only string or null values. This pattern is repeated identically across multiple example files (ButtonColor.svelte, Ctx.svelte, Custom.svelte, Default.svelte, Disabled.svelte, GroupColor.svelte).While code duplication is generally discouraged, it's acceptable for documentation examples where self-contained, copy-paste-ready code aids learning. However, if these examples grow more complex, consider extracting a shared utility helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
package.json(1 hunks)src/lib/forms/button-toggle/ButtonToggleGroup.svelte(2 hunks)src/lib/types.ts(1 hunks)src/lib/typography/hr/Hr.svelte(1 hunks)src/routes/docs-examples/extend/button-toggle/ButtonColor.svelte(1 hunks)src/routes/docs-examples/extend/button-toggle/Ctx.svelte(1 hunks)src/routes/docs-examples/extend/button-toggle/Custom.svelte(1 hunks)src/routes/docs-examples/extend/button-toggle/Default.svelte(1 hunks)src/routes/docs-examples/extend/button-toggle/Disabled.svelte(1 hunks)src/routes/docs-examples/extend/button-toggle/GroupColor.svelte(1 hunks)src/routes/docs-examples/extend/button-toggle/InitialValue.svelte(1 hunks)src/routes/docs-examples/extend/button-toggle/RoundedSize.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/lib/typography/hr/Hr.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/docs-examples/extend/button-toggle/InitialValue.svelte
- src/lib/types.ts
⏰ 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). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (10)
src/lib/forms/button-toggle/ButtonToggleGroup.svelte (3)
15-37: Excellent normalization logic for initial values.The
getInitialValue()function correctly handles type mismatches between single-select and multi-select modes, clones arrays to prevent aliasing, and provides sensible defaults. This implementation properly addresses the initialization bug described in the PR objectives.
39-39: Well done—initialization and notification now work correctly.The
selectedValuesinitialization viagetInitialValue()properly respects the incomingvalueprop, and callingonSelect(selectedValues)directly intoggleSelectedensures the callback fires only on user interactions, not during mount. This addresses the previous review concern about unwanted side effects during initialization.Also applies to: 60-60
39-39: Verify the value prop is intentionally non-reactive after initialization.The
valueprop is consumed only once during initialization viagetInitialValue(). After mount, the component does not react to external changes to thevalueprop. This implements an "uncontrolled after initialization" pattern, which may be the intended design. However, users might expect the component to remain synchronized with thevalueprop (controlled component pattern).Please confirm this behavior is intentional and, if so, consider documenting it to clarify that:
- The
valueprop sets the initial state only- After mount, state is managed internally via
selectedValues- Parent components should use the
onSelectcallback to track changes, not expect thevalueprop to control the component reactivelysrc/routes/docs-examples/extend/button-toggle/Disabled.svelte (1)
6-11: Type guard implementation is correct.The
handleSingleSelectfunction properly handles the widened signature with appropriate runtime checks.src/routes/docs-examples/extend/button-toggle/Custom.svelte (1)
7-12: LGTM.The type guard correctly handles the widened
onSelectsignature.src/routes/docs-examples/extend/button-toggle/ButtonColor.svelte (1)
7-12: LGTM.Implementation correctly guards against array inputs for single-select mode.
src/routes/docs-examples/extend/button-toggle/Ctx.svelte (1)
6-11: LGTM.The handler correctly processes only string or null values.
src/routes/docs-examples/extend/button-toggle/GroupColor.svelte (1)
6-11: LGTM.Properly handles the widened type signature with appropriate guards.
src/routes/docs-examples/extend/button-toggle/Default.svelte (2)
7-12: LGTM.The
handleSingleSelectguard correctly processes only string or null values.
14-19: LGTM.The
handleMultiSelectguard correctly processes only array values, complementing the single-select handler's behavior.
1 participant
📑 Description
Updated version of #1661.
Previously the value prop was ignored during initialization. Now properly initializes selectedValues from the value prop, handling type mismatches between single/multi-select modes.
Status
✅ Checks
api-checkdirectory as requiredpnpm check && pnpm test:e2eSummary by CodeRabbit
Bug Fixes
New Features
Documentation
Chores