chore: add wds multiselect component #39300
Conversation
WalkthroughThe changes update package metadata and introduce minor style and export adjustments across multiple components. A dependency version is updated in a package file. Enhancements include adding Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MultiSelect
participant ListBox
User->>MultiSelect: Press ArrowDown/ArrowUp
MultiSelect->>MultiSelect: Open popover
MultiSelect->>ListBox: Render options
User->>ListBox: Select option
ListBox->>MultiSelect: Update selection
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
github-actions
bot
added
the
skip-changelog
![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 (11)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx (2)
77-81: Consider Handling Additional Keyboard NavigationWhile opening the popover on arrow key presses is helpful, you might consider handling navigation focus (e.g., focusing the first or last item in the list) for a more complete multi-select keyboard experience.
169-173: Hide Error Text When Not ApplicableCurrently, the error message container is rendered regardless of the content. For a cleaner UI, consider conditionally rendering this block only if
errorMessagehas content.<div className={fieldErrorStyles.errorText}> - <Text color="negative" size="caption"> - {errorMessage} - </Text> + {errorMessage && ( + <Text color="negative" size="caption"> + {errorMessage} + </Text> + )} </div>app/client/packages/design-system/widgets/src/components/MultiSelect/src/types.ts (1)
18-18: Consider using explicit size values.Instead of using Exclude with SIZES, consider defining the exact allowed values for better type clarity.
- size?: Exclude<keyof typeof SIZES, "xSmall" | "large">; + size?: "small" | "medium";app/client/packages/design-system/widgets/src/components/TextField/src/TextField.tsx (1)
46-46: Document the purpose of data-input attribute.Add a comment explaining the purpose and usage of this data attribute for future maintainers.
app/client/packages/design-system/widgets/src/components/Select/src/Select.tsx (1)
48-48: Simplify boolean logic.The
Boolean()wrapper is redundant as the OR operator already coerces values to boolean.- isDisabled={Boolean(isDisabled) || Boolean(isLoading)} + isDisabled={isDisabled || isLoading}app/client/packages/design-system/widgets/src/components/MultiSelect/stories/MultiSelect.stories.tsx (2)
8-11: Consider sharing test data with Select stories.The items array could be moved to a shared test data file, similar to
selectData.tsused in Select stories.
38-41: Consider extracting size filtering logic.This filtering logic is duplicated from Select stories. Consider extracting it to a shared utility function.
// utils/storybook.ts export const getSupportedSizes = () => Object.keys(SIZES).filter( (size): size is NonNullable<SelectProps["size"]> => !["xSmall", "large"].includes(size) );app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (3)
34-38: Consider memoizing the Array.from conversion.The items array is converted in every render. Consider memoizing the array conversion for better performance.
const selectedItems = useMemo(() => { + const itemsArray = items ? Array.from(items) : []; return [...selectedKeys].map((key) => - items ? Array.from(items).find((item) => item.value === key) : undefined, + itemsArray.find((item) => item.value === key), ); }, [items, selectedKeys]);
123-123: Remove redundant Boolean calls.The Boolean calls are unnecessary as the values will be coerced to boolean.
- data-invalid={Boolean(isInvalid) ? "" : undefined} + data-invalid={isInvalid ? "" : undefined} - {Boolean(isLoading) ? ( + {isLoading ? (Also applies to: 149-149
🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
144-146: Consider accessibility for overflow indicator.The overflow indicator should have an ARIA label for screen readers.
- <Text style={{ width: "7ch" }}> + <Text aria-label={`and ${totalItems - visibleItems} more items`} style={{ width: "7ch" }}> {visibleItems < totalItems && <>...+{totalItems - visibleItems}</>} </Text>app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css (1)
31-37: Consider adding a focus state for the checkbox.The checkbox should have a visible focus state for keyboard navigation.
.listBoxItem[data-selected]::before { background-color: var(--color-bg-accent); background-image: url("data:image/svg+xml;charset=utf-8,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20width%3D%2224%22%20height%3D%2224%22%20viewBox%3D%220%200%2024%2024%22%20fill%3D%22none%22%20stroke%3D%22white%22%20stroke-width%3D%222%22%20stroke-linecap%3D%22round%22%20stroke-linejoin%3D%22round%22%20%3E%20%3Cpath%20d%3D%22M5%2012l5%205l10%20-10%22%20%2F%3E%20%3C%2Fsvg%3E%20"); background-size: var(--sizing-5); background-position: center; background-repeat: no-repeat; --checkbox-border-color: var(--color-bd-accent); + outline: var(--border-width-1) solid var(--color-bd-accent); + outline-offset: var(--border-width-1); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
app/client/package.json(1 hunks)app/client/packages/design-system/widgets/package.json(1 hunks)app/client/packages/design-system/widgets/src/components/Button/src/Button.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/FieldError/src/index.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/ListBox/src/ListBox.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/ListBox/src/index.ts(1 hunks)app/client/packages/design-system/widgets/src/components/ListBox/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/ListBoxItem/src/ListBoxItem.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/index.ts(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/index.ts(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/types.ts(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/stories/MultiSelect.stories.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Select/src/Select.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Select/src/index.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Select/stories/Select.stories.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/TextField/src/TextField.tsx(3 hunks)app/client/packages/design-system/widgets/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- app/client/packages/design-system/widgets/src/components/MultiSelect/index.ts
- app/client/packages/design-system/widgets/src/components/ListBox/src/index.ts
- app/client/packages/design-system/widgets/src/components/ListBox/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx
- app/client/package.json
- app/client/packages/design-system/widgets/src/components/Select/src/index.ts
- app/client/packages/design-system/widgets/src/components/Button/src/Button.tsx
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/index.ts
- app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx
[error] 123-123: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
[error] 149-149: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (11)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx (3)
1-21: Imports and Dependencies Look GoodThe set of imports and dependencies is well managed, bringing in essential react-aria elements, custom styles, and utility libraries. No conflicts or unused imports are evident.
32-38: EmptyState Component ImplementationThis component is straightforward and effective for displaying a default message when no options are found. Its minimal design and concise structure are fine as-is.
92-112: Label Click Focus HandlingGood job replicating the label-to-control focus behavior manually. This ensures strong accessibility compliance in lieu of a specialized multi-select hook.
app/client/packages/design-system/widgets/src/components/FieldError/src/index.ts (1)
3-3: New Styles Export ApprovedExporting
fieldErrorStylesallows for consistency across form error displays. No issues found.app/client/packages/design-system/widgets/src/components/ListBox/src/ListBox.tsx (1)
6-16: Enhanced Styling FlexibilityIntroducing
clsxand exposingclassNameenables consumers to merge custom styles with the default listBox styles. The approach is succinct and correct.app/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.ts (1)
1-12: LGTM! Well-structured type definition with clear documentation.The prefix prop is appropriately typed as ReactNode and well-documented with its intended use case for MultiSelect.
app/client/packages/design-system/widgets/src/components/ListBoxItem/src/ListBoxItem.tsx (1)
7-21: LGTM! Clean implementation of className handling.The use of clsx for combining classNames follows best practices, maintaining existing styles while allowing for customization.
app/client/packages/design-system/widgets/src/index.ts (1)
38-38: LGTM!The MultiSelect export follows the established pattern and is appropriately placed with other form components.
app/client/packages/design-system/widgets/src/components/Select/stories/Select.stories.tsx (1)
90-96: LGTM!The reformatting improves code readability while maintaining the same functionality.
app/client/packages/design-system/widgets/package.json (1)
29-29:❓ Verification inconclusive
Verify react-aria-components upgrade impact.
Please verify the changelog for breaking changes between 1.2.1 and 1.6.0.
🌐 Web query:
What are the breaking changes in react-aria-components between versions 1.2.1 and 1.6.0?💡 Result:
Here are the key breaking changes in react-aria-components between v1.2.1 and v1.6.0 based on available release notes:
Major API Changes
Deprecated
validationState
Replaced with simplerisInvalidboolean prop across form components[4][7]Client-side Routing Changes
Introduced newRouterProvidercomponent for router integration[4]. Existing routing implementations may need migration.Collection Component Props
MovedisDisabled/onActionprops from root components to individual collection items[7].
Example migration:- <Menu onAction={handler} isDisabled> + <MenuItem isDisabled onAction={handler}>Component Behavior Changes
Strict Mode Compatibility
Now requires React 18+ and uses nativeReact.useIdinstead of polyfilled ID generation[4]Selection Behavior
GridList selection handling updated - custom checkboxes now require explicit slot="selection" assignment[1][5]Accessibility Requirements
Added mandatory aria-label/textValue props to several components including GridList/Tree[1][5]CSS/Theme Breaking Changes
Link Styling Reset
Default link styling removed - must be explicitly defined in themes[2]GridList Layout
Requires additional CSS for proper rendering[2]:.react-aria-GridList { display: grid; gap: 8px; }Removed Features
Legacy Color Components
Old color picker APIs removed in favor of new ColorPicker/ColorArea components[7]Deprecated Drag & Drop API
Original implementation replaced with new hooks-based DnD system[4]Always check upgrade guides and test visual regressions when updating between major versions of react-aria-components[3][8]. The library maintains detailed changelogs with migration instructions for each release[4][7].
Citations:
- 1: https://react-spectrum.adobe.com/react-aria/GridList.html
- 2: https://www.jollyui.dev/docs/changelog
- 3: https://docs.angularjs.org/guide/migration
- 4: https://react-spectrum.adobe.com/releases/index.html
- 5: https://react-spectrum.adobe.com/react-aria/Tree.html
- 6: The Radio from react-aria-components is missing excludeFromTabOrder prop adobe/react-spectrum#6569
- 7: https://github.com/adobe/react-spectrum/releases
- 8: https://gitlab.ebi.ac.uk/kamal/backstage/-/blob/master/packages/core-components/CHANGELOG.md
- 9: https://react-spectrum.adobe.com/react-aria/DropZone.html
- 10: https://biomejs.dev/internals/changelog/
React-Aria-Components Upgrade Review
We've confirmed that upgrading from version 1.2.1 to 1.6.0 introduces several breaking changes, notably:
- Deprecated Prop: The old
validationStateprop has been removed in favor of a simplerisInvalidboolean.- Routing Update: A new
RouterProvidercomponent has been introduced, which may require migration of existing routing implementations.- Component Prop Adjustments: Props like
isDisabledandonActionfor collection components have shifted from the root to individual items.- React Compatibility: The upgrade now mandates React 18+ due to its switch from polyfilled ID generation to native
React.useId.- Behavior & Accessibility: Updates include modified selection behavior (e.g., explicit assignment for custom checkboxes) and stricter accessibility requirements, such as mandatory aria-label/textValue props.
- CSS/Theme Changes: There are changes like the removal of default link styling and layout adjustments for components like GridList.
Action Items:
- Verify that all usages in our codebase accommodate these updated APIs.
- Update any references to deprecated properties or components accordingly.
- Run integration tests to ensure that routing, collection components, and UI rendering behave as expected.
app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css (1)
67-70: LGTM: Good use of flex properties.The flex properties ensure proper text truncation and prevent layout issues.
| @@ -26,7 +26,7 @@ | |||
| "@tabler/icons-react": "^3.10.0", | |||
| "clsx": "^2.0.0", | |||
| "lodash": "*", | |||
| "react-aria-components": "^1.2.1", | |||
| "react-aria-components": "^1.6.0", | |||
There was a problem hiding this comment.
We need 1.6.0 version as it has the AutoComplete component that we need in multi select dropdown for filtering list
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/client/packages/design-system/widgets/src/components/Calendar/src/CalendarHeading.tsx (1)
18-18: 💡 Verification agent🧩 Analysis chain
Verify CalendarStateContext usage.
Let's confirm if the context is guaranteed to provide a non-null state value.
🌐 Web query:
Does react-aria-components CalendarStateContext guarantee a non-null state value? What's the recommended way to handle the state from CalendarStateContext?💡 Result:
The React Aria Components
CalendarStateContextdoes not inherently guarantee a non-null state value by default, but its usage patterns suggest specific handling recommendations:
Non-null assertion in child components
When accessingCalendarStateContextwithin calendar children (e.g., custom components inside<Calendar>), developers typically use TypeScript's non-null assertion operator (!) since these components are only valid when rendered within a parent calendar component:let state = React.useContext(CalendarStateContext)!; // Common pattern [1][2]Null safety considerations
While the context itself isn't explicitly typed as non-nullable:
- Parent
<Calendar>components initialize and manage state- Child components are expected to only render within valid calendar contexts
- Attempting to set
nulldirectly via state methods will cause errors [5]Recommended handling
function CalendarChild() { const state = React.useContext(CalendarStateContext); // Optional chaining for safety const dateValue = state?.value?.toDate(getLocalTimeZone()); // Fallback handling if (!state) return null; // Main logic using state... }TypeScript best practices
// Safe access pattern with type guards function CalendarValue() { const state = React.useContext(CalendarStateContext); if (!state) throw new Error('Must be used within <Calendar>'); // Now safely use state properties const date = state.value?.toDate(getLocalTimeZone()); }The documentation examples demonstrate direct access via non-null assertions (
!) due to their guaranteed placement within calendar hierarchies [1][2]. For reusable components, explicit type checking and error boundaries are recommended for robustness [5][9].Citations:
- 1: https://react-spectrum.adobe.com/react-aria/advanced.html
- 2: https://blog.logrocket.com/custom-compositional-api-react-aria/
- 3: https://reactspectrum.blob.core.windows.net/reactspectrum/06217ffc1843141c1b1f5d4e4f8a11ed227d9c0c/docs/react-aria/getting-started.html
- 4: https://react-spectrum.adobe.com/react-aria/useCalendar.html
- 5: Calendar's controlled value prop (in onChange) and setValue() from CalendarStateContext can't be set to null adobe/react-spectrum#6006
- 6: https://react-spectrum.adobe.com/react-aria/DatePicker.html
- 7: https://react-spectrum.adobe.com/react-aria/hooks.html
- 8: https://react-spectrum.adobe.com/react-aria/useRangeCalendar.html
- 9: https://reactspectrum.blob.core.windows.net/reactspectrum/bf57013c7ad271a5655b4a77d7950bcffc2c82d1/docs/react-aria/react-aria-components.html
- 10: https://react-spectrum.adobe.com/react-aria/Calendar.html
Ensure safe usage of CalendarStateContext in CalendarHeading.tsx
Our investigation shows that the React Aria Components’
CalendarStateContextdoes not inherently guarantee a non-null state. While many patterns use the non-null assertion operator—assuming that child components are always rendered within a valid<Calendar>—this can be risky without explicit checks.
- In
app/client/packages/design-system/widgets/src/components/Calendar/src/CalendarHeading.tsxat line 18, the current usage is:const state = useContext(CalendarStateContext);- Recommendation:
- If you're confident that
<CalendarHeading>is always rendered within a valid<Calendar>component, consider using:const state = useContext(CalendarStateContext)!;- Alternatively, add an explicit null check and a meaningful fallback or error:
const state = useContext(CalendarStateContext); if (!state) { throw new Error('CalendarStateContext is not available. Ensure <CalendarHeading> is used within a <Calendar> component.'); }This guard ensures robustness against potential null values, aligning with common best practices when working with context provided by react-aria-components.
🧹 Nitpick comments (4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (3)
16-26: Add JSDoc documentation for the interface.Consider adding JSDoc documentation to describe the purpose and usage of each prop in the
MultiSelectValuePropsinterface.+/** + * Props for the MultiSelectValue component + */ interface MultiSelectValueProps { + /** Whether to exclude the component from tab order */ excludeFromTabOrder?: boolean; + /** Whether the component is disabled */ isDisabled?: boolean; + /** Whether the component is in loading state */ isLoading?: boolean; + /** Set of selected item keys */ selectedKeys?: Selection; + /** Collection of selectable items */ items?: Iterable<{ label: string; value: string }>; + /** Placeholder text when no items are selected */ placeholder?: string; + /** Size variant of the component */ size?: MultiSelectProps<object>["size"]; + /** Reference to the trigger button element */ triggerRef: React.RefObject<HTMLButtonElement>; + /** Whether the component is in an invalid state */ isInvalid?: boolean; }
52-104: Optimize updateOverflow dependencies.The
updateOverflowcallback's dependency array includesselectedKeys, but it's only usingtotalItemswhich is already calculated outside. Consider movingtotalItemscalculation inside the callback to maintain consistency.-const totalItems = Array.from(selectedKeys).length; const updateOverflow = useCallback(() => { + const totalItems = Array.from(selectedKeys).length; // ... rest of the implementation -}, [domRef, selectedKeys, setVisibleItems]); +}, [domRef, setVisibleItems]);
106-120: Add error boundary for resize observer.Consider wrapping the resize observer logic in an error boundary to gracefully handle potential DOM-related errors.
+const handleResize = () => { + try { + updateOverflow(); + } catch (error) { + console.error('Failed to update overflow:', error); + } +}; useResizeObserver({ ref: parentRef, - onResize: updateOverflow, + onResize: handleResize, });app/client/packages/design-system/widgets/src/components/Calendar/src/CalendarHeading.tsx (1)
22-23: Consider safer alternatives to non-null assertions.The non-null assertions (!), while making the code compile, might mask potential runtime issues if
stateis actually null. Consider using optional chaining or adding a null check.- <CalendarMonthDropdown state={state!} /> - <CalendarYearDropdown state={state!} /> + {state && ( + <> + <CalendarMonthDropdown state={state} /> + <CalendarYearDropdown state={state} /> + </> + )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/widgets/src/components/Calendar/src/CalendarHeading.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.ts(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.ts
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx
[error] 129-129: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
[error] 155-155: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (1)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (1)
28-38: LGTM!Clean component implementation with well-organized props and appropriate default values.
| selectStyles.selectTriggerButton, | ||
| styles.multiSelectValue, | ||
| )} | ||
| data-invalid={Boolean(isInvalid) ? "" : undefined} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant Boolean calls.
The static analysis tool correctly identified redundant Boolean calls. These can be simplified.
-data-invalid={Boolean(isInvalid) ? "" : undefined}
+data-invalid={isInvalid ? "" : undefined}
-{Boolean(isLoading) ? (
+{isLoading ? (Also applies to: 155-155
🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
| @@ -94,7 +94,7 @@ export const DatePicker = <T extends DateValue>(props: DatePickerProps<T>) => { | |||
| label="Time" | |||
| maxValue={timeMaxValue} | |||
| minValue={timeMinValue} | |||
| onChange={state.setTimeValue} | |||
| onChange={(value) => state.setTimeValue(value!)} | |||
There was a problem hiding this comment.
Avoid non-null assertion for TimeField value.
The non-null assertion (!) bypasses TypeScript's type checking and could lead to runtime errors if the value is null/undefined.
Consider a safer approach:
-onChange={(value) => state.setTimeValue(value!)}
+onChange={(value) => {
+ if (value) {
+ state.setTimeValue(value);
+ }
+}}📝 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.
| onChange={(value) => state.setTimeValue(value!)} | |
| onChange={(value) => { | |
| if (value) { | |
| state.setTimeValue(value); | |
| } | |
| }} |
There was a problem hiding this comment.
It would also be good to fix this as well, as there may be issues related to this. Do you understand why value can be null?
Anyway, we could at least add a check for null
if (value) {
state.setTimeValue(value);
}
There was a problem hiding this comment.
@KelvinOm Yes, the value in TimeField's onChange can be null in several scenarios:
- When the user clears the time input
- During initial render without a value
- When an invalid time is entered
Your suggestion to add a null check is the right approach to handle these cases safely. I'll add this as a learning for future reviews.
✏️ Learnings added
Learnt from: KelvinOm
PR: appsmithorg/appsmith#39300
File: app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx:97-97
Timestamp: 2025-02-17T12:42:04.141Z
Learning: The TimeField component from react-aria-components can receive null values in its onChange handler, particularly when the user clears the input, during initial render without a value, or when an invalid time is entered. Always add null checks before using the value.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
| @@ -138,7 +138,7 @@ class WDSDatePickerWidget extends BaseWidget< | |||
| label={label} | |||
| maxValue={this.parseDate(maxDate)} | |||
| minValue={this.parseDate(minDate)} | |||
| onChange={this.handleDateChange} | |||
| onChange={(value) => this.handleDateChange(value!)} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling null values safely instead of using non-null assertion.
The non-null assertion (!) assumes the value will never be null, which could lead to runtime errors if the DatePicker allows clearing dates. Consider handling null values explicitly:
- onChange={(value) => this.handleDateChange(value!)}
+ onChange={(value) => value && this.handleDateChange(value)}📝 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.
| onChange={(value) => this.handleDateChange(value!)} | |
| onChange={(value) => value && this.handleDateChange(value)} |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
| @@ -19,8 +19,8 @@ function CalendarHeading( | |||
|
|
|||
| return ( | |||
| <div className={styles.monthYearDropdown}> | |||
| <CalendarMonthDropdown state={state} /> | |||
| <CalendarYearDropdown state={state} /> | |||
| <CalendarMonthDropdown state={state!} /> | |||
There was a problem hiding this comment.
note: had to buypass this typescript check with !.
there seems to be an issue on spectrum side in terms of types.
There was a problem hiding this comment.
export const CalendarStateContext: React.Context<CalendarState | null>;
Judging by the state types, it can indeed be null. I think it would be better if we handled this case from our side. Otherwise, FE can simply fall in case of state null. May I ask you to use correct types here and here instead of !?
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (5)
51-103: Consider memoizing computeVisibleItems functionThe
computeVisibleItemsfunction is recreated on every render withinupdateOverflow. Consider extracting and memoizing it to optimize performance.+const computeVisibleItems = useCallback((visibleItems: number) => { -const computeVisibleItems = (visibleItems: number) => { if (domRef.current) { // ... existing implementation } return visibleItems; -}; +}, []);
128-128: Remove redundant Boolean callsThe Boolean calls are unnecessary as the values will be automatically coerced to boolean.
-data-invalid={Boolean(isInvalid) ? "" : undefined} +data-invalid={isInvalid ? "" : undefined} -{Boolean(isLoading) ? ( +{isLoading ? (Also applies to: 154-154
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
51-103: Consider optimizing the overflow calculation logic.The current implementation might cause multiple rerenders due to the generator function yielding multiple state updates. Consider batching the calculations and updating the state once with the final value.
- setVisibleItems(function* () { - yield { - visibleItems: totalItems, - }; - - const newVisibleItems = computeVisibleItems(totalItems); - const isMeasuring = newVisibleItems < totalItems && newVisibleItems > 0; - - yield { - visibleItems: newVisibleItems, - }; - - if (isMeasuring) { - yield { - visibleItems: computeVisibleItems(newVisibleItems), - }; - } - }); + const newVisibleItems = computeVisibleItems(totalItems); + const isMeasuring = newVisibleItems < totalItems && newVisibleItems > 0; + const finalVisibleItems = isMeasuring ? computeVisibleItems(newVisibleItems) : newVisibleItems; + setVisibleItems({ visibleItems: finalVisibleItems });
39-43: Consider memoizing the items array conversion.The
Array.from(items)conversion is performed on every render within the useMemo hook. Consider moving the conversion outside the mapping function.-return [...selectedKeys].map((key) => - items ? Array.from(items).find((item) => item.value === key) : undefined, -); +const itemsArray = items ? Array.from(items) : []; +return [...selectedKeys].map((key) => + itemsArray.find((item) => item.value === key) +);
128-128: Remove redundant Boolean calls.The Boolean calls are unnecessary as the values will be automatically coerced to boolean.
-data-invalid={Boolean(isInvalid) ? "" : undefined} +data-invalid={isInvalid ? "" : undefined} -{Boolean(isLoading) ? ( +{isLoading ? (Also applies to: 154-154
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx (4)
79-83: Enhance keyboard interaction supportConsider handling additional keyboard events like Enter and Space for better accessibility.
const onKeyDown = (e: React.KeyboardEvent) => { - if (e.key === "ArrowDown" || e.key === "ArrowUp") { + if (["ArrowDown", "ArrowUp", "Enter", " "].includes(e.key)) { setOpen(true); } };
167-174: Consider creating a custom error context for consistency.Since we can't use the FieldErrorContext, consider creating a custom error context to maintain consistency with other form components.
+const MultiSelectErrorContext = React.createContext<{ errorMessage?: string }>({}); + +const useMultiSelectError = () => React.useContext(MultiSelectErrorContext); + <div className={fieldErrorStyles.errorText}> + <MultiSelectErrorContext.Provider value={{ errorMessage }}> <Text color="negative" size="caption"> {errorMessage} </Text> + </MultiSelectErrorContext.Provider> </div>
69-71: Simplify state management using controlled props pattern.The current implementation mixes controlled and uncontrolled patterns. Consider using a custom hook to handle this logic.
-const [_selectedKeys, _setSelectedKeys] = useState<Selection>(); -const selectedKeys = selectedKeysProp ?? _selectedKeys ?? defaultSelectedKeys; -const setSelectedKeys = onSelectionChangeProp ?? _setSelectedKeys; +const useControlledState = (value: Selection | undefined, defaultValue: Selection) => { + const [state, setState] = useState(value ?? defaultValue); + return [value ?? state, value ? onSelectionChangeProp : setState] as const; +}; +const [selectedKeys, setSelectedKeys] = useControlledState(selectedKeysProp, defaultSelectedKeys);
170-174: Consider extracting error handling to a reusable component.The error message implementation could be extracted into a reusable component to maintain consistency across the design system.
+const FieldErrorMessage = ({ message }: { message: string }) => ( + <div className={fieldErrorStyles.errorText}> + <Text color="negative" size="caption"> + {message} + </Text> + </div> +); -<div className={fieldErrorStyles.errorText}> - <Text color="negative" size="caption"> - {errorMessage} - </Text> -</div> +<FieldErrorMessage message={errorMessage} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css
👮 Files not reviewed due to content moderation or server errors (2)
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx
[error] 128-128: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
[error] 154-154: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: parse-tags
- GitHub Check: build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (11)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (6)
1-25: Well-structured interface with appropriate types!The props interface is well-defined with clear optional flags and proper TypeScript types.
121-162: Clean and accessible rendering implementation!The component handles various states effectively with proper accessibility attributes and clear conditional rendering.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
[error] 154-154: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
1-25: Well-structured interface with proper type definitions!The props interface is comprehensive and follows TypeScript best practices.
128-128: Remove redundant Boolean calls.The Boolean calls are unnecessary as the values will be coerced to boolean automatically.
Also applies to: 154-154
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
121-162: Clean and well-structured rendering logic!The component follows React best practices with proper conditional rendering and className composition.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
[error] 154-154: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
1-25: LGTM! Well-structured imports and type definitions.The props interface is comprehensive and type-safe, with all necessary dependencies properly imported.
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx (5)
88-178: Well-structured component composition!The implementation effectively combines React Aria components with custom behavior while maintaining accessibility.
40-47:✅ Verification successful
Consider tracking React Aria's MultiSelect implementation
The current implementation is noted as temporary. Consider creating an issue to track React Aria's progress on a native MultiSelect component for future migration.
🌐 Web query:
Does React Aria have plans for a native MultiSelect component?💡 Result:
React Aria's current Select component (as of February 2025) doesn't include native multi-select capabilities according to available documentation[1][9]. However, there are several indications of related functionality and potential workarounds:
Current state of multi-selection:
- The core
Selectcomponent only supports single selection throughselectedKey/onSelectionChangeprops[1][5]- Multiple selection is explicitly mentioned as unavailable in GitHub issues from late 2023 through 2024[4][9]
Alternative approaches:
ToggleButtonGroup (available since Nov 2024 release):
<ToggleButtonGroup selectionMode="multiple" onSelectionChange={setSelected} > <ToggleButton id="option1">Option 1</ToggleButton> <ToggleButton id="option2">Option 2</ToggleButton> </ToggleButtonGroup>Supports arrow navigation and multiple selection modes[3][7]
Menu component (since Nov 2024):
<Menu selectionMode="multiple"> <Item>Option 1</Item> <Item>Option 2</Item> </Menu>Allows mixing single/multiple selection within sections[3][7]
Future considerations:
- Recent additions like section-level selection in Menu components (Nov 2024) suggest ongoing work on selection mechanics[3][7]
- Community discussions indicate active interest in multi-select combobox functionality[4]
- The ARIA infrastructure supports multi-select through
aria-multiselectable[8], suggesting potential foundation for future implementationWhile no official roadmap mentions a native MultiSelect component specifically, React Aria's pattern of gradually exposing lower-level hooks (like
useSelect) alongside higher-level components suggests this could emerge as an extension point for custom implementations[2][5]. Developers needing multi-select today should consider combining existing primitives or following GitHub discussions for updates[4][9].Citations:
- 1: https://react-spectrum.adobe.com/react-aria/Select.html
- 2: https://github.com/adobe/react-spectrum/blob/main/rfcs/2023-react-aria-components.md
- 3: https://github.com/adobe/react-spectrum/releases
- 4: React aria components are not compatile to multi select combobox state adobe/react-spectrum#6994
- 5: https://react-spectrum.adobe.com/react-aria/useSelect.html
- 6: https://react-spectrum.adobe.com/releases/2024-09-30.html
- 7: https://react-spectrum.adobe.com/releases/index.html
- 8: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-multiselectable
- 9: React aria component Select multiple adobe/react-spectrum#5635
- 10: Disclosure in React Aria Components adobe/react-spectrum#5788
Action: Track Updates on React Aria's MultiSelect Support
The current implementation is a temporary workaround given that React Aria does not yet offer a native MultiSelect component. Please consider creating a tracking issue to monitor any changes or new releases from React Aria that might introduce official multi-select support—this will help ensure we can migrate to a more robust solution when available.
69-72: Well-implemented state management!The component properly handles both controlled and uncontrolled modes with clear default values.
26-26:❓ Verification inconclusive
Consider the implications of using UNSTABLE_Autocomplete.
The component relies on an unstable API which might change in future updates. Consider adding a tracking issue to migrate to the stable API when available.
Also applies to: 141-141
🌐 Web query:
Is there a stable Autocomplete component in react-aria-components?💡 Result:
The React Aria Components library does not currently have a stable Autocomplete component as of February 2025. Here's the key information:
Alpha Status
TheUNSTABLE_Autocompletecomponent remains in alpha according to:
- Official documentation showing an "Under construction" warning[1]
- Installation requiring
UNSTABLE_prefix in imports[1]- Recent January 2025 release notes listing Autocomplete as "alpha"[10]
Current Capabilities
While unstable, it provides core autocomplete functionality:import {UNSTABLE_Autocomplete} from 'react-aria-components' // Basic usage <Autocomplete filter={contains}> <SearchField /> <Menu> <MenuItem>Option 1</MenuItem> </Menu> </Autocomplete>Features include filtering hooks (
useFilter)[1], keyboard navigation[8], and accessibility support[6].Production Considerations
The React Aria team states:"When we say unstable [...] we mean we haven't settled the API yet. Behaviors are pretty stable"[3]
Key implications:
- ✅ Safe for production if you can handle future API changes
- ❌ Not recommended if you require long-term API stability
For teams needing a stable solution, alternatives like Material UI Autocomplete[7] or RSuite AutoComplete[4] might be preferable while React Aria's implementation matures. The React Aria team has been actively developing components throughout 2024[9][10], suggesting stable Autocomplete support could arrive in future releases.
Citations:
- 1: https://react-spectrum.adobe.com/react-aria/Autocomplete.html
- 2: https://www.youtube.com/watch?v=hK9pbVJRgV8
- 3: Is the `@react-aria/collections` inside `react-aria-components` stable enough to be used in production? adobe/react-spectrum#6805
- 4: https://rsuitejs.com/components/auto-complete/
- 5: https://react-spectrum.adobe.com/releases/2024-07-22.html
- 6: https://react-spectrum.adobe.com/blog/building-a-combobox.html
- 7: https://mui.com/material-ui/react-autocomplete/
- 8: https://react-spectrum.adobe.com/react-aria/ListBox.html
- 9: https://react-spectrum.adobe.com/releases/2024-09-30.html
- 10: https://github.com/adobe/react-spectrum/releases
Attention: Unstable API Warning for UNSTABLE_Autocomplete
The component currently uses
UNSTABLE_Autocompletefrom react-aria-components, which is in alpha and subject to change. While it delivers core autocomplete functionality (filtering hooks, keyboard navigation, etc.), its unstable status requires extra caution for long-term maintenance. Please consider the following:
- Add a tracking issue to revisit this component once a stable API becomes available.
- Review usage at:
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsxat line 26- and at line 141.
- Evaluate alternative autocomplete solutions if API stability is critical for your use case.
141-163:❓ Verification inconclusive
Verify stability of UNSTABLE_Autocomplete API.
The component uses an unstable API which might change in future versions. Consider:
- Documenting this dependency
- Adding version pinning
- Planning for future migration
🌐 Web query:
What is the current status of UNSTABLE_Autocomplete in react-aria-components?💡 Result:
As of February 2025, the
UNSTABLE_Autocompletecomponent in react-aria-components remains in alpha status, though it has seen active development since its initial release in January 2025[1][3]. Here's the current state:Key Details
- Latest version: Part of React Aria Components v1.6.0[1]
- Installation: Requires explicit import as an unstable component:
import {UNSTABLE_Autocomplete} from 'react-aria-components'- Core functionality: Combines text input with a filterable menu/ListBox[1][3]
- Key features:
- Supports both TextField and SearchField inputs
- Works with Menu or ListBox collections
- Customizable filtering via
filterprop- Arrow key navigation while keeping input focus[3]
Recent Developments
The component was first introduced in React Aria's January 15, 2025 release as an alpha feature targeting:
- Command palettes
- Searchable menus
- Flexible layout configurations where input and menu are siblings[3]
Limitations
- Documentation is still marked as "coming soon"[1]
- API stability warnings remain (hence the UNSTABLE_ prefix)
- Missing advanced features like type-ahead search or complex item rendering[3]
The React Aria team encourages developers to test this component and provide feedback while they work toward a stable release[3]. For production use cases requiring more maturity, their ComboBox component remains the recommended alternative[3][4].
Citations:
- 1: https://react-spectrum.adobe.com/react-aria/Autocomplete.html
- 2: https://rsuitejs.com/components/auto-complete/
- 3: https://react-spectrum.adobe.com/releases/2025-01-15.html
- 4: https://react-spectrum.adobe.com/releases/index.html
- 5: https://react-spectrum.adobe.com/react-aria/Calendar.html
- 6: https://react-spectrum.adobe.com/releases/2024-09-30.html
- 7: https://react-spectrum.adobe.com/react-aria/index.html
- 8: https://react-spectrum.adobe.com/releases/2023-12-20.html
- 9: https://react-spectrum.adobe.com/react-aria/components.html
- 10: https://react-spectrum.adobe.com/releases/2024-05-01.html
Warning: Usage of UNSTABLE_Autocomplete (Alpha Status)
The implementation relies on the
UNSTABLE_Autocompletecomponent from react-aria-components, which remains in alpha as of February 2025 (React Aria Components v1.6.0). Since this API is unstable and subject to changes, please consider:
- Documenting this dependency: Clearly note that this component is experimental.
- Version pinning: Lock the dependency version for consistency.
- Planning for a future migration: Evaluate a switch to a stable alternative (e.g., ComboBox) if used in production.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (1)
37-102: Consider extracting helper functions for better readability.The overflow calculation logic is complex but well-implemented. Consider extracting the following calculations into helper functions:
- Item width calculations
- Container size checks
Example refactor:
+const calculateItemWidth = (item: HTMLLIElement) => + item.getBoundingClientRect().width; + +const isItemVisible = ( + calculatedSize: number, + containerSize: number, + ellipsisWidth: number +) => + calculatedSize <= containerSize && + calculatedSize + ellipsisWidth < containerSize; + const updateOverflow = useCallback(() => { const computeVisibleItems = (visibleItems: number) => { if (domRef.current) { const containerSize = domRef.current.getBoundingClientRect().width; const listItems = Array.from( domRef.current.children, ) as HTMLLIElement[]; const ellipisisItem = listItems.pop(); const ellipisisItemWidth = ellipisisItem?.getBoundingClientRect().width ?? 0; let calculatedSize = 0; let newVisibleItems = 0; for (const item of listItems) { - const itemWidth = item.getBoundingClientRect().width; + const itemWidth = calculateItemWidth(item); calculatedSize += itemWidth; - if ( - calculatedSize <= containerSize && - calculatedSize + ellipisisItemWidth < containerSize - ) { + if (isItemVisible(calculatedSize, containerSize, ellipisisItemWidth)) { newVisibleItems++; } else { break; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/types.ts
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx
[error] 127-127: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
[error] 153-153: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (4)
1-24: LGTM! Well-structured imports and interface definition.The props interface is well-defined with appropriate types, and imports are organized logically.
26-36: LGTM! Clean component implementation.Props destructuring is well-organized with appropriate default values.
104-118: LGTM! Efficient resize handling implementation.Good use of useResizeObserver and memoization for parent ref.
127-127: Remove redundant Boolean calls.The static analysis tool correctly identified redundant Boolean calls that can be simplified.
Apply this diff:
-data-invalid={Boolean(isInvalid) ? "" : undefined} +data-invalid={isInvalid ? "" : undefined} -{Boolean(isLoading) ? ( +{isLoading ? (Also applies to: 153-153
🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
| @@ -19,8 +19,8 @@ function CalendarHeading( | |||
|
|
|||
| return ( | |||
| <div className={styles.monthYearDropdown}> | |||
| <CalendarMonthDropdown state={state} /> | |||
| <CalendarYearDropdown state={state} /> | |||
| <CalendarMonthDropdown state={state!} /> | |||
There was a problem hiding this comment.
export const CalendarStateContext: React.Context<CalendarState | null>;
Judging by the state types, it can indeed be null. I think it would be better if we handled this case from our side. Otherwise, FE can simply fall in case of state null. May I ask you to use correct types here and here instead of !?
| @@ -94,7 +94,7 @@ export const DatePicker = <T extends DateValue>(props: DatePickerProps<T>) => { | |||
| label="Time" | |||
| maxValue={timeMaxValue} | |||
| minValue={timeMinValue} | |||
| onChange={state.setTimeValue} | |||
| onChange={(value) => state.setTimeValue(value!)} | |||
There was a problem hiding this comment.
It would also be good to fix this as well, as there may be issues related to this. Do you understand why value can be null?
Anyway, we could at least add a check for null
if (value) {
state.setTimeValue(value);
}
app/client/packages/design-system/widgets/src/components/FieldError/src/index.ts
Show resolved
Hide resolved
| @@ -26,7 +26,10 @@ export const Menu = (props: MenuProps) => { | |||
| UNSTABLE_portalContainer={isRootMenu ? root : undefined} | |||
| maxHeight={POPOVER_LIST_BOX_MAX_HEIGHT} | |||
| > | |||
| <HeadlessMenu className={clsx(listStyles.listBox, className)} {...rest}> | |||
| <HeadlessMenu | |||
| className={clsx(listBoxStyles.listBox, className)} | |||
There was a problem hiding this comment.
The sub-menu is broken. Could you take a look?
| return ( | ||
| <div className={styles.emptyState}> | ||
| <Text color="neutral-subtle">No options found</Text> | ||
| </div> |
There was a problem hiding this comment.
We could just pass className to Text component.
<Text className={styles.emptyState} color="neutral-subtle">
No options found
</Text>
There was a problem hiding this comment.
Oh yeah..totally slipped my mind. ( AI and its side effects )
| ); | ||
| }; | ||
|
|
||
| export default MultiSelectValue; |
There was a problem hiding this comment.
Could you please use a named export?
|
|
||
| /* this is a hack to show checkbox in the listbox in the dropdown. We are forced to do this because listitems are not getting re-rendered when the selected keys change. | ||
| * So when we use checkbox component, it does not get checked or unchecked. */ | ||
| .listBoxItem[data-selected]::before { |
There was a problem hiding this comment.
Can we consider passing IsSelected prop to the ListBoxItem and then render the Checkbox there? We do something similar with icon.
There was a problem hiding this comment.
Actually, i tried that only. It didn't work. Their listboxitem just don't re-render.
There was a problem hiding this comment.
The original ListBoxItem component can take the function as a children. So we can modify our components as follows
const renderedChildren = () => {
if (typeof children === "function") {
return (
<HeadlessListBoxItem
{...rest}
className={clsx(styles.listBoxItem, className)}
>
{(renderProps) => children(renderProps)}
</HeadlessListBoxItem>
);
}
return (
<HeadlessListBoxItem
{...rest}
className={clsx(styles.listBoxItem, className)}
>
{icon && <Icon name={icon} />}
<Text lineClamp={1}>{children}</Text>
</HeadlessListBoxItem>
);
};
return renderedChildren();
After that, we can use it like this
<ListBoxItem
className={styles.listBoxItem}
id={item.value}
key={item.value}
textValue={item.label}
>
{({ isSelected }) => (
<>
<Checkbox isSelected={isSelected} />
{item.label}
</>
)}
</ListBoxItem>
)}
</ListBox>
There was a problem hiding this comment.
This really worked. Thanks a lot.
| @@ -45,7 +45,7 @@ export const Select = (props: SelectProps) => { | |||
| {label} | |||
| </FieldLabel> | |||
| <SelectTrigger | |||
| isDisabled={isDisabled} | |||
| isDisabled={Boolean(isDisabled) || Boolean(isLoading)} | |||
There was a problem hiding this comment.
I think this is not entirely correct using isLoading for disabled state., since for disabled state we use opacity, which should not happen for a loading state.
There was a problem hiding this comment.
Reverted. I just did because i was able to open the dropdown when is loading was true.
| export const Menu = (props: MenuProps) => { | ||
| const { children, className, ...rest } = props; | ||
| const root = useRootContainer(); | ||
|
|
||
| const nestingLevel = useContext(MenuNestingContext); |
There was a problem hiding this comment.
@KelvinOm seems all this is not required anymore.
There was a problem hiding this comment.
Nice! Thank you for fixing this!
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
app/client/packages/design-system/widgets/src/components/Calendar/src/CalendarYearDropdown.tsx (2)
15-20:⚠️ Potential issueAdd null check for state before accessing setFocusedDate
The code attempts to access
state.setFocusedDatewithout checking if state is null, which could cause runtime errors.Add null check:
const onChange = (value: Key | null) => { const index = Number(value); const date = years[index].value; - state.setFocusedDate(date); + state?.setFocusedDate?.(date); };
27-27:⚠️ Potential issueRemove hardcoded selectedKey value
The
selectedKey={20}appears to be hardcoded, which might not reflect the actual selected year.The selected key should be derived from the current state:
- selectedKey={20} + selectedKey={years.findIndex(year => + year.value.getFullYear() === state?.focusedDate?.getFullYear() + )}
🧹 Nitpick comments (2)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (2)
21-21: Consider using a more specific type for the size prop.Instead of using the generic
MultiSelectProps<object>["size"], define an explicit union type of allowed sizes for better type safety and documentation.- size?: MultiSelectProps<object>["size"]; + size?: "small" | "medium" | "large";
50-102: Consider adding error boundary protection for overflow calculations.The overflow calculation logic involves DOM measurements that could fail in certain edge cases. Consider wrapping this logic in a try-catch block or implementing an error boundary.
const updateOverflow = useCallback(() => { + try { const computeVisibleItems = (visibleItems: number) => { // ... existing code ... }; setVisibleItems(function* () { // ... existing code ... }); + } catch (error) { + // Fallback to showing all items + setVisibleItems({ visibleItems: totalItems }); + } }, [domRef, selectedKeys, setVisibleItems]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/widgets/src/components/Calendar/src/CalendarYearDropdown.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/index.ts
- app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx
- app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx
[error] 127-127: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
[error] 153-153: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (2)
127-127: Remove redundant Boolean calls.The Boolean calls are unnecessary as the values will be automatically coerced.
-data-invalid={Boolean(isInvalid) ? "" : undefined} +data-invalid={isInvalid ? "" : undefined} -{Boolean(isLoading) ? ( +{isLoading ? (Also applies to: 153-153
🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
163-163: LGTM! Named export implemented as requested.The component is correctly exported using a named export.
app/client/packages/design-system/widgets/src/components/Calendar/src/CalendarYearDropdown.tsx (2)
8-12: Breaking Change: State parameter now accepts nullThis change modifies the public API and requires careful consideration of backward compatibility.
Please verify if this breaking change is intentional and update the component's documentation accordingly.
13-13:❓ Verification inconclusive
Verify useYearOptions handles null state
The
useYearOptionshook is called with potentially null state.
🏁 Script executed:
#!/bin/bash # Check if useYearOptions handles null state ast-grep --pattern 'function useYearOptions($state) { $$$ }'Length of output: 64
I'll need to further verify that the hook’s definition actually handles a null state. The previous script produced no output, so let’s search for where and how
useYearOptionsis defined (whether as a traditional function or an arrow function) to inspect its null-checking logic.Please run the following shell script to locate the definition of
useYearOptionsacross the repository:Once we have the definition output, we can verify if it includes a null-check for the
stateparameter.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for the definition of useYearOptions (handles both traditional and arrow function definitions) rg -n "function useYearOptions" || rg -n "const useYearOptions" -nLength of output: 296
Review: Verify Null State Handling in useYearOptions Hook
- The hook is defined in
app/client/packages/design-system/widgets/src/components/Calendar/utils/calendar.ts:4
as:export function useYearOptions(state: CalendarState) {- Our search did not reveal an explicit null check for the
stateparameter inside the hook.- Verify that the parent component guarantees that
stateis non-null when callinguseYearOptionsor add a defensive null check (e.g.,if (!state) { … }) within the hook to prevent potential runtime issues.
| export const Menu = (props: MenuProps) => { | ||
| const { children, className, ...rest } = props; | ||
| const root = useRootContainer(); | ||
|
|
||
| const nestingLevel = useContext(MenuNestingContext); |
There was a problem hiding this comment.
Nice! Thank you for fixing this!
|
|
||
| /* this is a hack to show checkbox in the listbox in the dropdown. We are forced to do this because listitems are not getting re-rendered when the selected keys change. | ||
| * So when we use checkbox component, it does not get checked or unchecked. */ | ||
| .listBoxItem[data-selected]::before { |
There was a problem hiding this comment.
The original ListBoxItem component can take the function as a children. So we can modify our components as follows
const renderedChildren = () => {
if (typeof children === "function") {
return (
<HeadlessListBoxItem
{...rest}
className={clsx(styles.listBoxItem, className)}
>
{(renderProps) => children(renderProps)}
</HeadlessListBoxItem>
);
}
return (
<HeadlessListBoxItem
{...rest}
className={clsx(styles.listBoxItem, className)}
>
{icon && <Icon name={icon} />}
<Text lineClamp={1}>{children}</Text>
</HeadlessListBoxItem>
);
};
return renderedChildren();
After that, we can use it like this
<ListBoxItem
className={styles.listBoxItem}
id={item.value}
key={item.value}
textValue={item.label}
>
{({ isSelected }) => (
<>
<Checkbox isSelected={isSelected} />
{item.label}
</>
)}
</ListBoxItem>
)}
</ListBox>
| import React, { useRef, useState } from "react"; | ||
|
|
||
| import { useField } from "react-aria"; | ||
| import { type Selection, ListBoxItem } from "react-aria-components"; |
There was a problem hiding this comment.
The idea was to modify WDS component, which is already have needed styles, instead of using spectrum component directly. This way we can reuse it more easily and styles will be used only in it.
There was a problem hiding this comment.
Yea right. Had to update our ListBoxItem to accept renderProps too.
Updated with our ListBoxItem.
| border-radius: 0; | ||
| } | ||
|
|
||
| .listBoxItem::before { |
There was a problem hiding this comment.
These styles are no longer needed, right?
There was a problem hiding this comment.
Removed. Sorry for being sloppy.
| {({ isSelected }) => ( | ||
| <> | ||
| <span className={styles.listBoxItemCheckbox}> | ||
| <Checkbox isSelected={isSelected} /> |
There was a problem hiding this comment.
Why can't we pass the styles directly to the checkbox?
| <Checkbox isSelected={isSelected} /> | |
| <Checkbox className={styles.listBoxItemCheckbox} isSelected={isSelected} /> |
There was a problem hiding this comment.
Yea. Had to update our checkbox API which was not accepting the className prop before. Fixed it.
| } | ||
|
|
||
| .ellipsisText { | ||
| width: 7ch; |
There was a problem hiding this comment.
Shouldn't this be the max-width and increase as the number increases?
There was a problem hiding this comment.
Updated. It seems to be working fine with max-width too.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (2)
38-42: Optimize selectedItems calculation.The current implementation uses multiple Array conversions which can be simplified.
-const selectedItems = useMemo(() => { - return [...selectedKeys].map((key) => - items ? Array.from(items).find((item) => item.value === key) : undefined, - ); -}, [items, selectedKeys]); +const selectedItems = useMemo(() => { + if (!items) return Array.from(selectedKeys).map(() => undefined); + const itemsArray = Array.from(items); + return Array.from(selectedKeys).map((key) => + itemsArray.find((item) => item.value === key) + ); +}, [items, selectedKeys]);
50-102: Add error handling for DOM measurements.The overflow calculations rely heavily on DOM measurements which could fail. Consider adding error boundaries and cleanup.
const updateOverflow = useCallback(() => { const computeVisibleItems = (visibleItems: number) => { if (domRef.current) { + try { const containerSize = domRef.current.getBoundingClientRect().width; const listItems = Array.from( domRef.current.children, ) as HTMLLIElement[]; const ellipisisItem = listItems.pop(); const ellipisisItemWidth = ellipisisItem?.getBoundingClientRect().width ?? 0; let calculatedSize = 0; let newVisibleItems = 0; for (const item of listItems) { const itemWidth = item.getBoundingClientRect().width; calculatedSize += itemWidth; if ( calculatedSize <= containerSize && calculatedSize + ellipisisItemWidth < containerSize ) { newVisibleItems++; } else { break; } } return newVisibleItems; + } catch (error) { + console.error('Error calculating visible items:', error); + return visibleItems; + } } return visibleItems; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/packages/design-system/widgets/src/components/Checkbox/src/Checkbox.tsx(3 hunks)app/client/packages/design-system/widgets/src/components/ListBoxItem/src/ListBoxItem.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx
[error] 127-127: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
[error] 152-152: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (9)
app/client/packages/design-system/widgets/src/components/Checkbox/src/Checkbox.tsx (3)
1-1: LGTM! Good choice using clsx.The addition of clsx for conditional class name management aligns with React best practices.
14-21: LGTM! Props destructuring is clean and well-organized.The addition of the className prop follows React conventions and maintains good code organization.
27-27: LGTM! Proper composition of class names.The use of clsx to combine base styles with custom className maintains component styling flexibility while preserving default styles.
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (3)
14-24: LGTM! Well-structured props interface.Props follow React Aria conventions with appropriate TypeScript types.
127-127: Remove redundant Boolean calls.The static analysis tool correctly identified redundant Boolean calls. These can be simplified.
Also applies to: 152-152
🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
121-158: LGTM! Clean styling implementation.Good use of clsx for class composition and consistent styling approach.
🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
[error] 152-152: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
app/client/packages/design-system/widgets/src/components/ListBoxItem/src/ListBoxItem.tsx (3)
1-1: LGTM! Good choice using clsx.The addition of clsx for class name composition is a standard approach.
13-24: Clean implementation of flexible rendering patterns.Good improvements:
- Proper className composition with clsx
- Support for both render prop and standard children patterns
- Maintained backward compatibility
9-10:❓ Verification inconclusive
Verify ListBoxItemProps type definition.
The className prop is destructured but we need to ensure the type definition includes it.
🏁 Script executed:
#!/bin/bash # Search for ListBoxItemProps type definition ast-grep --pattern 'type ListBoxItemProps = { $$$ }'Length of output: 56
Action Required: Verify
ListBoxItemPropsType Definition Includes "className"The
ListBoxItemcomponent now destructures aclassNameprop, but the verification command we initially ran provided no output. Please manually verify the type definition in
app/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.tsto ensure it has aclassNameproperty (e.g.,className?: string). If it's missing, update the type accordingly.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run the search for the ListBoxItemProps type definition and check for "className" in the file rg "export type ListBoxItemProps" -n app/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.ts rg "className" -n app/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.tsLength of output: 342
Attention: Verify
ListBoxItemPropsType DeclarationThe
ListBoxItemcomponent now destructures aclassNameprop. However, our earlier grep did not produce any output from thetypes.tsfile, so we couldn’t conclusively verify whetherListBoxItemPropsincludes theclassNameproperty. Please manually verify that the type definition (located atapp/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.ts) includes something like:export type ListBoxItemProps = { className?: string; // ...other props }If the property is missing, update the type accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (2)
27-39: Add documentation for the MAX_ELLIPSIS_COUNT constant.Consider adding a JSDoc comment explaining the purpose and significance of this constant.
+/** Maximum number of additional items to show in the ellipsis count */ const MAX_ELLIPSIS_COUNT = 999;
105-105: Remove unused dependency from useCallback.The
selectedKeysdependency isn't used within theupdateOverflowfunction.-}, [domRef, selectedKeys, setVisibleItems]); +}, [domRef, setVisibleItems]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx
[error] 130-130: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
[error] 157-157: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (2)
app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (2)
1-25: LGTM! Well-structured imports and type definitions.The imports are properly organized and the interface is well-defined with clear types.
130-130: Remove redundant Boolean calls.The Boolean calls are unnecessary as the values will be automatically coerced to booleans.
-data-invalid={Boolean(isInvalid) ? "" : undefined} +data-invalid={isInvalid ? "" : undefined} -{Boolean(isLoading) ? ( +{isLoading ? (Also applies to: 157-157
🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
WDS Multi-Select component
CleanShot.2025-02-17.at.13.10.38.mp4
Note: This is a kind of hacky multi-select component. React aria does not provide us with any multi select component. So we are making it with existing components like popover, listbox, and autocomplete.
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
New Features
Enhancements
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13386938922
Commit: 9d486cb
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Tue, 18 Feb 2025 09:33:19 UTC