chore: add wds multiselect #39307
chore: add wds multiselect #39307
Conversation
WalkthroughThis pull request introduces a comprehensive multi-select widget along with its associated configuration objects, helper functions, validations, and exports. New configuration files have been added for various aspects including display options, property pane details, methods, and setters. Additionally, widget-specific files such as the main widget implementation, constants, and type definitions have been created or updated. Global changes include modifications in the widget export arrays and utility functions to support the new widget functionality and enhanced type safety. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant W as WDSMultiSelectWidget
participant M as Methods & Setters
participant V as Validation Helpers
U->>W: Interact with widget (select options)
W->>W: Call onSelectionChange()
W->>M: Get updated configuration & setter functions
M-->>W: Return updated values and dynamic property paths
W->>V: Validate new selection via helper functions
V-->>W: Return validation status
W->>U: Render updated widget view
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 8, 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (26)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts (1)
8-20: Consider extracting configuration constants.Several improvements could enhance maintainability:
- Move stringified sample data to a constant
- Define version in a central location
- Extract dynamic property paths to a constant
const STRINGIFIED_SAMPLE_DATA = JSON.stringify(SAMPLE_DATA, null, 2); const DYNAMIC_PATHS = [{ key: "sourceData" }]; export const defaultsConfig: WidgetDefaultProps = { // ... other config sourceData: STRINGIFIED_SAMPLE_DATA, dynamicPropertyPathList: DYNAMIC_PATHS, // ... remaining config };app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx (4)
57-80: Derived properties approach is consistent.
CallingparseDerivedPropertiesto dynamically assign widget properties is well-organized. Ensure these derived properties are covered by tests to guarantee correctness.
81-92: Meta properties and default mapping look straightforward.
UsingselectedOptionValuesandisDirtyfor internal widget state is valid, but keep an eye on potential naming conflicts if you introduce new meta properties.
99-110: Lifecycle method usage is reasonable.
componentDidUpdateeffectively resetsisDirtyon default value changes. Consider verifying that this won't unintentionally override user edits when default values are updated in-flight.
130-159: Key-based re-render workaround is valid but might be expensive for large data sets.
If options frequently change or get large, consider more granular updates to minimize unmount/mount cycles.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts (1)
19-23: Consider renaming the accessor for consistency.The accessor
selectedOptionValuedoesn't match the pathdefaultOptionValue. Consider aligning these names for better maintainability.setSelectedOption: { path: "defaultOptionValue", type: "string", - accessor: "selectedOptionValue", + accessor: "defaultOptionValue", },app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts (1)
18-20: Standardize boolean type definitions.Boolean properties should use consistent type definitions. Consider using the same format as other properties.
- isDisabled: "bool", - isValid: "bool", - isDirty: "bool", + isDisabled: { + "!type": "bool", + "!doc": "Disables input to the widget", + }, + isValid: { + "!type": "bool", + "!doc": "Indicates if the widget value is valid", + }, + isDirty: { + "!type": "bool", + "!doc": "Indicates if the widget value has been modified", + },app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/utils.ts (1)
27-39: Consider optimizing the implementation and improving type safety.The function could be optimized and made more type-safe:
- Consider using type narrowing for unknown values
- Array.prototype.every could exit early on first mismatch
export const processOptionArray = ( valuesOrLabels: string[], - sourceData: Record<string, unknown>[], + sourceData: Record<string, string | number>[], ) => { if (!sourceData.length) return []; + + const firstValue = valuesOrLabels[0]; + if (!firstValue || !(firstValue in sourceData[0])) return valuesOrLabels; - const allEqual = valuesOrLabels.every((item, _, arr) => item === arr[0]); - const keyExistsInSource = valuesOrLabels[0] in sourceData[0]; + const allEqual = valuesOrLabels.every((item) => item === firstValue); - return allEqual && keyExistsInSource - ? sourceData.map((d) => d[valuesOrLabels[0]]) + return allEqual + ? sourceData.map((d) => d[firstValue]) : valuesOrLabels; };app/client/packages/design-system/widgets/src/components/Select/src/Select.tsx (1)
48-48: LGTM, with a minor optimization suggestion.The logic is correct, but we can simplify the Boolean conversions.
- isDisabled={Boolean(isDisabled) || Boolean(isLoading)} + isDisabled={!!isDisabled || !!isLoading}app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts (2)
16-27: Add type narrowing for null checks.Consider using explicit type guards for better type safety.
- if (value === "" || _.isNil(value)) { + if (typeof value === "undefined" || value === null || value === "") {
39-55: Enhance array validation error message.The error message could be more descriptive about the invalid value.
- : `Invalid entry at index: ${errorIndex}. Value must be a string`, + : `Invalid entry at index: ${errorIndex}. Found ${typeof value[errorIndex]}, expected string`,app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts (1)
25-31: Add explicit return type annotation.Consider adding a return type for better type safety and documentation.
- getQueryGenerationConfig(widget: WidgetProps) { + getQueryGenerationConfig(widget: WidgetProps): { select: { where: string } } {app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js (2)
24-29: Simplify getIsValid function.Remove unused parameters and make the logic more concise.
- getIsValid: (props, moment, _) => { + getIsValid: (props) => { - return props.isRequired - ? !_.isNil(props.selectedOptionValues) && - props.selectedOptionValues.length !== 0 - : true; + return !props.isRequired || (!_.isNil(props.selectedOptionValues) && props.selectedOptionValues.length > 0); },
46-65: Optimize label lookup performance.Consider using a Map for O(1) label lookups instead of using find operations.
getSelectedOptionLabels: (props, moment, _) => { const values = props.selectedOptionValues; const selectedOptions = props.selectedOptions ?? []; const options = props.options ?? []; + const optionsMap = new Map(options.map(opt => [opt.value, opt.label])); + const selectedOptionsMap = new Map( + selectedOptions.map(opt => [opt.value ?? opt, opt.label]) + ); return values .map((value) => { - const label = options.find((option) => value === option.value)?.label; - if (label) { - return label; - } else { - return selectedOptions.find( - (option) => value === (option.value ?? option), - )?.label; - } + return optionsMap.get(value) ?? selectedOptionsMap.get(value); }) .filter((val) => !_.isNil(val)); },app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (2)
13-25: Enhance error message specificity.Consider providing more context in the error message based on validation requirements.
if (!props.isValid) { return { validationStatus: "invalid", - errorMessage: "Please select an option", + errorMessage: props.isRequired + ? "This field is required. Please select at least one option." + : "Please select a valid option.", }; }
56-68: Enhance type safety in autocomplete data generation.Add explicit type annotations for better code maintainability.
+type AutocompleteData = Record<string, string>; + export function getLabelValueAdditionalAutocompleteData(props: WidgetProps) { const keys = getLabelValueKeyOptions(props); return { item: keys .map((d) => d.label) - .reduce((prev: Record<string, string>, curr: unknown) => { + .reduce<AutocompleteData>((prev, curr) => { prev[curr as string] = ""; return prev; }, {}), }; }app/client/packages/design-system/widgets/src/components/Datepicker/src/Datepicker.tsx (1)
97-97: Consider safer null handling.Using non-null assertion (
!) might lead to runtime errors ifvalueis null. Consider adding a null check or using optional chaining.-onChange={(value) => state.setTimeValue(value!)} +onChange={(value) => value && state.setTimeValue(value)}app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
109-111: Consider using Set for uniqueness check.The array uniqueness check could be optimized using Set.
-const isValid = options.every( - (d: unknown, i: number, arr: unknown[]) => arr.indexOf(d) === i, -); +const isValid = new Set(options).size === options.length;app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (1)
43-61: Add error boundary for JSON parsing.The current try-catch block silently handles JSON parse errors. Consider logging these errors for debugging.
try { const parsedValue = JSON.parse(value) as | (string | number)[] | LabelValue[]; if (Array.isArray(parsedValue) || _.isString(parsedValue)) { value = parsedValue; } } catch (e) { + console.debug('Failed to parse JSON value:', e); const splitByComma = (value as string).split(",") || []; value = splitByComma.map((s) => s.trim()); }app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelectValue.tsx (2)
129-129: Remove redundant Boolean calls.The Boolean constructor is unnecessary when the value is already being coerced to a boolean.
- 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
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
52-104: Consider memoizing overflow calculations.The
computeVisibleItemsfunction is recreated on every render. Consider extracting and memoizing it with useCallback.+ const computeVisibleItems = useCallback((visibleItems: number) => { + if (!domRef.current) return visibleItems; + + 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; + }, [domRef]); const updateOverflow = useCallback(() => { - const computeVisibleItems = (visibleItems: number) => { - // ... existing implementation - }; setVisibleItems(function* () { // ... rest of the implementation using computeVisibleItems }); }, [domRef, selectedKeys, setVisibleItems, computeVisibleItems]);app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx (1)
98-106: Consider using useButton hook for focus management.Manual focus management could be replaced with React Aria's useButton hook for better accessibility and consistent behavior.
Consider refactoring to use useButton hook which handles focus management automatically:
import { useButton } from 'react-aria'; // ... in component const { buttonProps } = useButton({ onPress: () => { if (triggerRef.current) { triggerRef.current.focus(); setInteractionModality("keyboard"); } } });app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx (1)
61-124: Consider adding validation for duplicate values in optionLabel and optionValue.While the current validation is good, adding checks for duplicate values would prevent potential issues.
app/client/packages/design-system/widgets/src/components/MultiSelect/src/styles.module.css (2)
17-37: Consider using CSS custom properties for checkbox dimensions.Extract repeated values into CSS custom properties for better maintainability:
.listBoxItem::before { content: ""; - height: var(--sizing-5); - width: var(--sizing-5); + --checkbox-size: var(--sizing-5); + height: var(--checkbox-size); + width: var(--checkbox-size); --checkbox-border-width: var(--border-width-2); --checkbox-border-color: var(--color-bd-neutral); --checkbox-bg-color: var(--color-bg-accent); --checkbox-box-shadow: 0px 0px 0px var(--checkbox-border-width) var(--checkbox-border-color) inset; box-shadow: var(--checkbox-box-shadow); } .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-size: var(--checkbox-size); background-position: center; background-repeat: no-repeat; --checkbox-border-color: var(--color-bd-accent); }
72-74: Consider making ellipsis text width configurable.The hardcoded width of 7ch might not be suitable for all use cases.
.ellipsisText { - width: 7ch; + width: var(--ellipsis-width, 7ch); }app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx (1)
141-141: Consider handling null values explicitly instead of using non-null assertion.The non-null assertion (
!) assumes the value will never be null, which may not be safe. Consider handling potential null values explicitly.- onChange={(value) => this.handleDateChange(value!)} + onChange={(value) => { + if (value) { + this.handleDateChange(value); + } + }}
📜 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 (46)
app/client/package.json(1 hunks)app/client/packages/design-system/widgets/package.json(1 hunks)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/FieldError/src/index.ts(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/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)app/client/src/modules/ui-builder/ui/wds/WDSDatePickerWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/utils.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/constants.ts(1 hunks)app/client/src/widgets/WidgetUtils.ts(3 hunks)app/client/src/widgets/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (12)
- app/client/packages/design-system/widgets/src/components/MultiSelect/index.ts
- app/client/packages/design-system/widgets/src/components/FieldError/src/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts
- app/client/packages/design-system/widgets/src/components/ListBox/src/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts
- app/client/packages/design-system/widgets/src/components/Select/src/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts
- app/client/packages/design-system/widgets/src/components/ListBoxItem/src/types.ts
- app/client/package.json
🧰 Additional context used
🧠 Learnings (1)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts (1)
Learnt from: riodeuno
PR: appsmithorg/appsmith#30351
File: app/client/src/widgets/wds/WDSModalWidget/config/anvilConfig.ts:1-6
Timestamp: 2024-11-12T08:11:43.278Z
Learning: The `anvilConfig` is optional for widgets that do not participate in the main container's layout flow, such as the modal widget in the current context.
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 30-30: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
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)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
⏰ 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-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 (27)
app/client/packages/design-system/widgets/src/components/Menu/src/Menu.tsx (2)
3-3: LGTM! Consistent with design system updates.The import change aligns with the standardized naming convention across components.
29-32: LGTM! Clean implementation of style composition.The usage of
clsxto combine default styles with custom className provides good flexibility while maintaining design system consistency.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts (1)
1-3: LGTM! Clean and well-organized imports.The imports are properly structured with clear separation of external and internal dependencies.
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts (2)
1-11: LGTM! The anvil configuration looks good.The configuration is well-structured with appropriate type safety and responsive width settings.
6-9:❓ Verification inconclusive
Verify the sizing token value.
Please confirm that "sizing-30" is a valid design token in your system for the 180px breakpoint.
🏁 Script executed:
#!/bin/bash # Search for other usages of the sizing-30 token to verify its validity rg "sizing-30" --type ts --type tsxLength of output: 63
Token Verification: Confirm "sizing-30" is a valid design token for the 180px breakpoint.
The snippet below sets a mapping for the 180px breakpoint to the token"sizing-30". However, our previous search encountered an error using--type tsx, so it’s unclear if this token is defined and used elsewhere in the codebase. Please ensure that"sizing-30"is present in your design tokens configuration.
- Verify across both TypeScript and TSX files. For example, try running:
rg "sizing-30" -g "*.ts" -g "*.tsx" --ignore-dir=node_modules- Confirm that the token is defined in your design tokens file(s).
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx (6)
1-27: Great job organizing imports and dependencies.
All necessary modules appear to be consolidated cleanly, and it's good to see type imports separated from value imports.
28-33: Class inheritance looks correct.
ExtendingBaseWidgetwith the specific prop and state types is clear and type-safe.
34-55: Good use of external config references.
Centralizing widget configuration (e.g. defaults, methods, anvil configuration) in separate files helps maintain modularity and readability.
94-96: Empty stylesheet config is fine.
No issues found; it’s clean to keep an empty stylesheet map if no styling overrides are needed.
112-128: Meta update flow is implemented correctly.
TheonSelectionChangelogic properly batches and commits meta updates.
162-163: Straightforward export.
No issues found here.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts (1)
1-9: Meta configuration is concise and clear.
ThesearchTagsmake it easier to find this widget. Best practice: periodically revisit these tags to ensure they accurately reflect common user terminology.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts (1)
1-14: Type definitions appear consistent.
All required fields and optional booleans are present. However, storingonSelectionChangeas a string might limit some type safety if it’s meant to be a callback. Verify that runtime binding logic is validated properly for event handling.app/client/packages/design-system/widgets/src/components/ListBox/src/ListBox.tsx (1)
6-6: LGTM! Clean implementation of className support.The changes correctly implement className support using clsx, following React best practices.
Also applies to: 9-9, 12-12
app/client/packages/design-system/widgets/src/components/ListBoxItem/src/ListBoxItem.tsx (1)
7-7: LGTM! Consistent implementation with ListBox component.The changes maintain consistency with the ListBox component's className implementation.
Also applies to: 10-10, 13-16
app/client/packages/design-system/widgets/src/components/MultiSelect/src/types.ts (1)
8-39: Well-structured type definitions with clear documentation.The MultiSelectProps interface is well-defined with appropriate type constraints and helpful JSDoc comments.
app/client/packages/design-system/widgets/src/components/TextField/src/TextField.tsx (1)
10-10: Good enhancement for component styling flexibility.The addition of className prop with clsx and data-input attribute improves component customization capabilities.
Also applies to: 30-30, 46-46
app/client/packages/design-system/widgets/src/index.ts (1)
38-38: LGTM!The MultiSelect export follows the established pattern and is correctly placed within the exports list.
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js (1)
30-45: Well implemented selection handling!The function properly handles edge cases and maintains selected values when dirty.
app/client/packages/design-system/widgets/src/components/MultiSelect/stories/MultiSelect.stories.tsx (1)
1-84: Well-structured Storybook configuration!The stories provide comprehensive coverage of component states and variations.
app/client/src/modules/ui-builder/ui/wds/constants.ts (1)
66-66: LGTM!The addition follows the established pattern for widget mapping.
app/client/packages/design-system/widgets/src/components/Select/stories/Select.stories.tsx (1)
90-96: LGTM!The Validation story component maintains the same functionality while improving code readability.
app/client/src/widgets/index.ts (1)
93-93: LGTM!The WDSMultiSelectWidget is properly registered following the established pattern for WDS widgets.
Also applies to: 193-193
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx (2)
20-60: LGTM! Well-structured Data section configuration.The sourceData configuration is well-defined with proper validation and sample data.
125-157:❓ Verification inconclusive
Verify the defaultOptionValues validation logic.
The helper text correctly warns about values needing to exist in source data. Let's verify the validation implementation:
🏁 Script executed:
#!/bin/bash # Search for defaultOptionValueValidation implementation ast-grep --pattern 'function defaultOptionValueValidation($_) { $$$ }'Length of output: 74
Action Required: Verify Implementation of
defaultOptionValueValidationThe ast-grep search did not locate any definition for
defaultOptionValueValidation. Please confirm that this function is indeed implemented (or intentionally omitted) and that its validation logic meets expectations. If the function exists with a different signature or naming, consider updating the search pattern accordingly or manually verifying its behavior.app/client/src/widgets/WidgetUtils.ts (1)
993-1038: Good improvement to type safety.The addition of generic type parameter
T extends Record<string, unknown>enhances type checking while maintaining backward compatibility.app/client/packages/design-system/widgets/package.json (1)
29-29:✅ Verification successful
Verify the react-aria-components version compatibility.
Let's check if this is the latest stable version and review any breaking changes:
🌐 Web query:
What is the latest stable version of react-aria-components and are there any breaking changes between 1.2.1 and 1.6.0?💡 Result:
The latest stable version of react-aria-components is v1.6, as confirmed by its recent NPM update ([5 days ago][9])[^9][^10]. This library provides unstyled components with built-in accessibility and internationalization support[^2][^5].
Breaking Changes Between v1.x Releases
While most updates focus on new features and bug fixes[^4][^7][^9], some notable API adjustments occurred between v1.x releases:
Version Range Key Changes v1.x → v1.y - Deprecated validationStateprop replaced withisInvalidboolean[^4].
- Introduction ofRouterProviderfor client-side routing integration[^4][^7].
- Enhanced React Strict Mode compatibility[^7].v1.x → vx.y - New components like DropZone,FileTrigger,TreeView, and color pickers introduced[^5][^7].
- Virtualization support added for performance improvements[^7].For users upgrading from v1.x, check for deprecated props like
validationStateand update dependencies to ensure compatibility with newer React versions (e.g., React v19 support)[^8][^9]. Full migration details are available in official release notes[^4][^7].Citations:
- 1: https://www.npmjs.com/package/react-aria-components/v/1.0.0-nightly.4295
- 2: https://react-spectrum.adobe.com/react-aria/GridList.html
- 3: https://docs.angularjs.org/guide/migration
- 4: https://react-spectrum.adobe.com/releases/index.html
- 5: https://react-spectrum.adobe.com/react-aria/DropZone.html
- 6: https://www.mkdocs.org/about/release-notes/
- 7: https://github.com/adobe/react-spectrum/releases
- 8: [RAC] React 19 peer dependency issue with @react-aria/collections adobe/react-spectrum#7583
- 9: https://react-spectrum.adobe.com/releases/2024-09-30.html
- 10: https://react-spectrum.adobe.com/react-aria/Calendar.html
Verified react-aria-components Version Upgrade
- The upgrade to
"^1.6.0"is confirmed as it reflects the latest stable version.- Note that between v1.2.1 and v1.6.0, some API adjustments were introduced (e.g., replacing the deprecated
validationStateprop withisInvalidand the addition of components likeRouterProvider). Ensure that your application does not rely on deprecated APIs.
| export const defaultsConfig = { | ||
| animateLoading: true, | ||
| label: "Label", | ||
| sourceData: JSON.stringify(SAMPLE_DATA, null, 2), | ||
| optionLabel: "name", | ||
| optionValue: "code", | ||
| defaultOptionValues: "", | ||
| isRequired: false, | ||
| isDisabled: false, | ||
| isVisible: true, | ||
| isInline: false, | ||
| widgetName: "MultiSelect", | ||
| version: 1, | ||
| responsiveBehavior: ResponsiveBehavior.Fill, | ||
| dynamicPropertyPathList: [{ key: "sourceData" }], | ||
| placeholderText: "Select an item", | ||
| } as unknown as WidgetDefaultProps; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety.
The use of as unknown as WidgetDefaultProps type assertion bypasses TypeScript's type checking. Consider defining the object with proper typing upfront.
-export const defaultsConfig = {
+export const defaultsConfig: WidgetDefaultProps = {
// ... configuration
-} as unknown as WidgetDefaultProps;
+};📝 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.
| export const defaultsConfig = { | |
| animateLoading: true, | |
| label: "Label", | |
| sourceData: JSON.stringify(SAMPLE_DATA, null, 2), | |
| optionLabel: "name", | |
| optionValue: "code", | |
| defaultOptionValues: "", | |
| isRequired: false, | |
| isDisabled: false, | |
| isVisible: true, | |
| isInline: false, | |
| widgetName: "MultiSelect", | |
| version: 1, | |
| responsiveBehavior: ResponsiveBehavior.Fill, | |
| dynamicPropertyPathList: [{ key: "sourceData" }], | |
| placeholderText: "Select an item", | |
| } as unknown as WidgetDefaultProps; | |
| export const defaultsConfig: WidgetDefaultProps = { | |
| animateLoading: true, | |
| label: "Label", | |
| sourceData: JSON.stringify(SAMPLE_DATA, null, 2), | |
| optionLabel: "name", | |
| optionValue: "code", | |
| defaultOptionValues: "", | |
| isRequired: false, | |
| isDisabled: false, | |
| isVisible: true, | |
| isInline: false, | |
| widgetName: "MultiSelect", | |
| version: 1, | |
| responsiveBehavior: ResponsiveBehavior.Fill, | |
| dynamicPropertyPathList: [{ key: "sourceData" }], | |
| placeholderText: "Select an item", | |
| }; |
| <CalendarMonthDropdown state={state!} /> | ||
| <CalendarYearDropdown state={state!} /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider safer null handling for state.
The non-null assertions (!) assume state will never be null. Consider using optional chaining or providing a fallback for safer null handling.
🏁 Script executed:
#!/bin/bash
# Search for CalendarStateContext usage to verify if state can be null
ast-grep --pattern 'useContext(CalendarStateContext)' -A 3Length of output: 66648
Safer state handling in CalendarHeading
In app/client/packages/design-system/widgets/src/components/Calendar/src/CalendarHeading.tsx (lines 22–23), the component passes state using non-null assertions:
<CalendarMonthDropdown state={state!} />
<CalendarYearDropdown state={state!} />Since state is obtained via useContext(CalendarStateContext), this use of state! assumes it will never be null. To make the component more robust, please consider one of the following approaches:
• Add a guard clause before rendering—for example, check if state is null and return a fallback (like null) or a loading indicator.
• Alternatively, if a default state makes sense, provide one when reading from the context (e.g., using optional chaining or a nullish coalescing operator: state={state ?? defaultState}).
• If you are certain that the context always supplies a non-null value, ensure that the provider enforces this so that the non-null assertion isn’t needed.
| if (queryConfig.select) { | ||
| modify = { | ||
| sourceData: queryConfig.select.data, | ||
| optionLabel: formConfig.aliases.find((d) => d.name === "label")?.alias, | ||
| optionValue: formConfig.aliases.find((d) => d.name === "value")?.alias, | ||
| defaultOptionValue: "", | ||
| serverSideFiltering: false, | ||
| onFilterUpdate: queryConfig.select.run, | ||
| }; | ||
|
|
||
| dynamicPropertyPathList.push({ key: "sourceData" }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add null checks for aliases lookup.
The alias lookups could potentially return undefined.
🏁 Script executed:
#!/bin/bash
# Check if there are any undefined checks for aliases in the codebase
rg -A 2 "formConfig\.aliases\.find" | grep -v "undefined"Length of output: 2223
Null Checks for Alias Lookups Needed
The alias lookups using formConfig.aliases.find still rely solely on optional chaining. If the lookup returns undefined, the properties (.alias) remain undefined, which may cause downstream issues. Please add explicit null checks or provide default fallback values (for example, using the nullish coalescing operator ?? '') for both optionLabel and optionValue in:
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts(lines 43–54)- Similar patterns in other files (e.g., in WDSSelectWidget and MultiSelectWidgetV2)
| getOptions: (props, moment, _) => { | ||
| let labels = [], | ||
| values = [], | ||
| sourceData = props.sourceData || []; | ||
|
|
||
| if (typeof props.optionLabel === "string") { | ||
| labels = sourceData.map((d) => d[props.optionLabel]); | ||
| } else if (_.isArray(props.optionLabel)) { | ||
| labels = props.optionLabel; | ||
| } | ||
|
|
||
| if (typeof props.optionValue === "string") { | ||
| values = sourceData.map((d) => d[props.optionValue]); | ||
| } else if (_.isArray(props.optionValue)) { | ||
| values = props.optionValue; | ||
| } | ||
|
|
||
| return sourceData.map((d, i) => ({ | ||
| label: labels[i], | ||
| value: values[i], | ||
| })); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type checking and null safety to getOptions.
The function should validate sourceData elements and add null checks for optionLabel/optionValue access.
- getOptions: (props, moment, _) => {
+ getOptions: (props, _) => {
let labels = [],
values = [],
sourceData = props.sourceData || [];
- if (typeof props.optionLabel === "string") {
+ if (props.optionLabel && typeof props.optionLabel === "string") {
labels = sourceData.map((d) => d?.[props.optionLabel] ?? '');
- } else if (_.isArray(props.optionLabel)) {
+ } else if (props.optionLabel && _.isArray(props.optionLabel)) {
labels = props.optionLabel;
}
- if (typeof props.optionValue === "string") {
+ if (props.optionValue && typeof props.optionValue === "string") {
values = sourceData.map((d) => d?.[props.optionValue] ?? '');
- } else if (_.isArray(props.optionValue)) {
+ } else if (props.optionValue && _.isArray(props.optionValue)) {
values = props.optionValue;
}📝 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.
| getOptions: (props, moment, _) => { | |
| let labels = [], | |
| values = [], | |
| sourceData = props.sourceData || []; | |
| if (typeof props.optionLabel === "string") { | |
| labels = sourceData.map((d) => d[props.optionLabel]); | |
| } else if (_.isArray(props.optionLabel)) { | |
| labels = props.optionLabel; | |
| } | |
| if (typeof props.optionValue === "string") { | |
| values = sourceData.map((d) => d[props.optionValue]); | |
| } else if (_.isArray(props.optionValue)) { | |
| values = props.optionValue; | |
| } | |
| return sourceData.map((d, i) => ({ | |
| label: labels[i], | |
| value: values[i], | |
| })); | |
| }, | |
| getOptions: (props, _) => { | |
| let labels = [], | |
| values = [], | |
| sourceData = props.sourceData || []; | |
| if (props.optionLabel && typeof props.optionLabel === "string") { | |
| labels = sourceData.map((d) => d?.[props.optionLabel] ?? ''); | |
| } else if (props.optionLabel && _.isArray(props.optionLabel)) { | |
| labels = props.optionLabel; | |
| } | |
| if (props.optionValue && typeof props.optionValue === "string") { | |
| values = sourceData.map((d) => d?.[props.optionValue] ?? ''); | |
| } else if (props.optionValue && _.isArray(props.optionValue)) { | |
| values = props.optionValue; | |
| } | |
| return sourceData.map((d, i) => ({ | |
| label: labels[i], | |
| value: values[i], | |
| })); | |
| }, |
| export function getLabelValueKeyOptions(widget: WidgetProps) { | ||
| const sourceData = get(widget, `${EVAL_VALUE_PATH}.sourceData`); | ||
|
|
||
| let parsedValue: Record<string, unknown> | undefined = sourceData; | ||
|
|
||
| if (isString(sourceData)) { | ||
| try { | ||
| parsedValue = JSON.parse(sourceData); | ||
| } catch (e) {} | ||
| } | ||
|
|
||
| if (isArray(parsedValue)) { | ||
| return uniq( | ||
| parsedValue.reduce((keys, obj) => { | ||
| if (isPlainObject(obj)) { | ||
| Object.keys(obj).forEach((d) => keys.push(d)); | ||
| } | ||
|
|
||
| return keys; | ||
| }, []), | ||
| ).map((d: unknown) => ({ | ||
| label: d, | ||
| value: d, | ||
| })); | ||
| } else { | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and type safety.
The empty catch block could hide important parsing errors, and type safety could be enhanced.
if (isString(sourceData)) {
try {
parsedValue = JSON.parse(sourceData);
- } catch (e) {}
+ } catch (e) {
+ console.warn('Failed to parse sourceData:', e);
+ parsedValue = undefined;
+ }
}
if (isArray(parsedValue)) {
return uniq(
- parsedValue.reduce((keys, obj) => {
+ parsedValue.reduce<string[]>((keys, obj) => {
if (isPlainObject(obj)) {
Object.keys(obj).forEach((d) => keys.push(d));
}
return keys;
}, []),
- ).map((d: unknown) => ({
+ ).map((d: string) => ({
label: d,
value: d,
}));
}📝 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.
| export function getLabelValueKeyOptions(widget: WidgetProps) { | |
| const sourceData = get(widget, `${EVAL_VALUE_PATH}.sourceData`); | |
| let parsedValue: Record<string, unknown> | undefined = sourceData; | |
| if (isString(sourceData)) { | |
| try { | |
| parsedValue = JSON.parse(sourceData); | |
| } catch (e) {} | |
| } | |
| if (isArray(parsedValue)) { | |
| return uniq( | |
| parsedValue.reduce((keys, obj) => { | |
| if (isPlainObject(obj)) { | |
| Object.keys(obj).forEach((d) => keys.push(d)); | |
| } | |
| return keys; | |
| }, []), | |
| ).map((d: unknown) => ({ | |
| label: d, | |
| value: d, | |
| })); | |
| } else { | |
| return []; | |
| } | |
| } | |
| export function getLabelValueKeyOptions(widget: WidgetProps) { | |
| const sourceData = get(widget, `${EVAL_VALUE_PATH}.sourceData`); | |
| let parsedValue: Record<string, unknown> | undefined = sourceData; | |
| if (isString(sourceData)) { | |
| try { | |
| parsedValue = JSON.parse(sourceData); | |
| } catch (e) { | |
| console.warn('Failed to parse sourceData:', e); | |
| parsedValue = undefined; | |
| } | |
| } | |
| if (isArray(parsedValue)) { | |
| return uniq( | |
| parsedValue.reduce<string[]>((keys, obj) => { | |
| if (isPlainObject(obj)) { | |
| Object.keys(obj).forEach((d) => keys.push(d)); | |
| } | |
| return keys; | |
| }, []), | |
| ).map((d: string) => ({ | |
| label: d, | |
| value: d, | |
| })); | |
| } else { | |
| return []; | |
| } | |
| } |
| const hasLabelValue = (obj: LabelValue): obj is LabelValue => { | ||
| return ( | ||
| _.isPlainObject(obj) && | ||
| obj.hasOwnProperty("label") && | ||
| obj.hasOwnProperty("value") && | ||
| _.isString(obj.label) && | ||
| (_.isString(obj.value) || _.isFinite(obj.value)) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Improve type safety and security in hasLabelValue.
- Replace hasOwnProperty with Object.hasOwn
- Add type guard for better type safety
-const hasLabelValue = (obj: LabelValue): obj is LabelValue => {
+const hasLabelValue = (obj: unknown): obj is LabelValue => {
return (
_.isPlainObject(obj) &&
- obj.hasOwnProperty("label") &&
- obj.hasOwnProperty("value") &&
+ Object.hasOwn(obj, "label") &&
+ Object.hasOwn(obj, "value") &&
_.isString(obj.label) &&
(_.isString(obj.value) || _.isFinite(obj.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.
| const hasLabelValue = (obj: LabelValue): obj is LabelValue => { | |
| return ( | |
| _.isPlainObject(obj) && | |
| obj.hasOwnProperty("label") && | |
| obj.hasOwnProperty("value") && | |
| _.isString(obj.label) && | |
| (_.isString(obj.value) || _.isFinite(obj.value)) | |
| ); | |
| }; | |
| const hasLabelValue = (obj: unknown): obj is LabelValue => { | |
| return ( | |
| _.isPlainObject(obj) && | |
| Object.hasOwn(obj, "label") && | |
| Object.hasOwn(obj, "value") && | |
| _.isString(obj.label) && | |
| (_.isString(obj.value) || _.isFinite(obj.value)) | |
| ); | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 30-30: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
| <UNSTABLE_Autocomplete filter={filter}> | ||
| <TextField autoFocus className={styles.textField} /> | ||
| <ListBox |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider implications of using unstable API.
The UNSTABLE_Autocomplete component is marked as unstable and may change in future versions. Consider adding a comment about upgrade implications or tracking stable API release.
🌐 Web query:
What is the current status of Autocomplete component in react-aria-components?
💡 Result:
The Autocomplete component in react-aria-components is currently in alpha (as of January 15, 2025)[1][5][9], with limited documentation and ongoing development. Here's its current status:
Key Details
- Installation: Requires
[email protected]+viayarn add react-aria-components[1] - Import path:
import {UNSTABLE_Autocomplete as Autocomplete} from 'react-aria-components'[1][5] - Core functionality: Combines text input with filtered menu options[1], supporting:
- Customizable filtering (
useFilterhook)[1] - Arrow key navigation while keeping input focus[5]
- Flexible input types (TextField/SearchField) and collection components (Menu/ListBox)[5]
- Customizable filtering (
Current Capabilities
Basic implementation structure:
<Autocomplete filter={contains}>
<MySearchField />
<Menu>
<MenuItem>Option 1</MenuItem>
<MenuItem>Option 2</MenuItem>
</Menu>
</Autocomplete>Key props:
| Prop | Type | Description |
|---|---|---|
filter |
(textValue: string, inputValue: string) => boolean |
Custom filter function[1] |
inputValue |
string |
Controlled input value[1] |
defaultInputValue |
string |
Uncontrolled default value[1] |
Limitations & Considerations
- Marked as UNSTABLE with potential API changes[1][5]
- No official mobile/touch optimization yet[3][7]
- Limited styling examples (basic CSS scaffolding provided)[1]
- Documentation explicitly states "Under construction"[1]
Future Roadmap
Planned enhancements per developer comments:
- Expanded mobile support (tray-based interactions)[3]
- Improved async loading patterns[2][4]
- Additional accessibility refinements[7]
- Public API stabilization[5][9]
For production use, Adobe recommends monitoring their release notes for updates on stabilization progress[5][9]. The component remains experimental but shows active development with recent CSS transition support added in January 2025[5].
Citations:
- 1: https://react-spectrum.adobe.com/react-aria/Autocomplete.html
- 2: https://refine.dev/blog/material-ui-autocomplete-component/
- 3: SearchAutocomplete adobe/react-spectrum#2084
- 4: https://mui.com/material-ui/react-autocomplete/
- 5: https://react-spectrum.adobe.com/releases/2025-01-15.html
- 6: https://reactspectrum.blob.core.windows.net/reactspectrum/4a1a5ae9c5940b443ec0cad078d2958d8d010cd5/docs/react-aria/useSearchAutocomplete.html
- 7: https://react-spectrum.adobe.com/blog/building-a-combobox.html
- 8: https://react-spectrum.adobe.com/releases/2024-09-30.html
- 9: https://github.com/adobe/react-spectrum/releases
- 10: https://github.com/adobe/react-spectrum/blob/main/packages/react-aria-components/docs/ComboBox.mdx
Attention: Unstable API and Focus Management Enhancements
- The
UNSTABLE_Autocompletecomponent is currently in an alpha stage and subject to API changes. It's advisable to add an inline comment noting this status and outlining upgrade considerations once a stable release is available. - The manual focus management for label clicks could be streamlined by leveraging the
useButtonhook from react-aria. This change would improve consistency and maintainability.
jsartisan
force-pushed
the
chore/wds-multi-select-widget
branch
from
ecae3ad to
f2d87bc
Compare
jsartisan
force-pushed
the
chore/wds-multi-select-widget
branch
from
96f31be to
c48342e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
61-64:⚠️ Potential issueReplace hasOwnProperty with Object.hasOwn.
Using Object.prototype.hasOwnProperty directly is unsafe. Use Object.hasOwn() instead for better security.
- props.sourceData[0].hasOwnProperty(String(value[0])) + Object.hasOwn(props.sourceData[0], String(value[0]))🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🧹 Nitpick comments (7)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (2)
15-26: Enhance error message clarity.Consider making the error message more user-friendly by specifying that the value cannot be empty or undefined.
- message: `value does not evaluate to type: string | Array<string| number | boolean>`, + message: `Value cannot be empty or undefined. Expected type: string | Array<string | number | boolean>`,
109-124: Consider using Set for uniqueness check.The current implementation uses array.indexOf which has O(n²) complexity. Using Set would be more efficient.
- const isValid = options.every( - (d: unknown, i: number, arr: unknown[]) => arr.indexOf(d) === i, - ); + const isValid = new Set(options).size === options.length;app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (3)
13-25: Enhance validation logic.The current validation only checks the
isValidprop but doesn't validate the actual selected values. Consider adding:
- Validation for empty/null values
- Maximum/minimum selection limits
- Custom validation message support
export function validateInput(props: WDSMultiSelectWidgetProps): Validation { + if (!props.selectedOptions || props.selectedOptions.length === 0) { + return { + validationStatus: "invalid", + errorMessage: props.emptySelectionMessage || "Please select an option", + }; + } + + if (props.maxSelectionLimit && props.selectedOptions.length > props.maxSelectionLimit) { + return { + validationStatus: "invalid", + errorMessage: `Maximum ${props.maxSelectionLimit} selections allowed`, + }; + } + if (!props.isValid) { return { validationStatus: "invalid", - errorMessage: "Please select an option", + errorMessage: props.validationMessage || "Please select an option", }; }
56-68: Improve type safety and performance.Consider using
Object.fromEntriesfor better performance and proper type definitions to avoid type casting.export function getLabelValueAdditionalAutocompleteData(props: WidgetProps) { const keys = getLabelValueKeyOptions(props); return { - item: keys - .map((d) => d.label) - .reduce((prev: Record<string, string>, curr: unknown) => { - prev[curr as string] = ""; - - return prev; - }, {}), + item: Object.fromEntries( + keys.map((d) => [d.label, ""]) + ), }; }
70-78: Add type safety for expression strings.Consider creating a type-safe way to generate these expressions to prevent runtime errors.
+type ExpressionTemplate = `{{${string}}}`; + -export const defaultValueExpressionPrefix = `{{ ((options, serverSideFiltering) => ( `; +export const defaultValueExpressionPrefix: ExpressionTemplate = `{{ ((options, serverSideFiltering) => ( `; -export const getDefaultValueExpressionSuffix = (widget: WidgetProps) => +export const getDefaultValueExpressionSuffix = (widget: WidgetProps): ExpressionTemplate => `))(${widget.widgetName}.options, ${widget.widgetName}.serverSideFiltering) }}`; -export const getOptionLabelValueExpressionPrefix = (widget: WidgetProps) => +export const getOptionLabelValueExpressionPrefix = (widget: WidgetProps): ExpressionTemplate => `{{${widget.widgetName}.sourceData.map((item) => (`; -export const optionLabelValueExpressionSuffix = `))}}`; +export const optionLabelValueExpressionSuffix: ExpressionTemplate = `))}}`;app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (2)
35-39: Consider case-insensitive duplicate checks (optional).
If end users might accidentally enter the same value with different cases (e.g. "Green"/"green"), you might want to handle that scenario to avoid confusion. If exact-case matching is intended, please disregard.
61-111: Consider adding test coverage for these validation paths.
It may be beneficial to ensure each scenario (empty strings, duplicates, numeric values, label-value objects, etc.) is thoroughly unit tested.Would you like test cases generated for these validation scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts
[error] 28-28: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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
🔇 Additional comments (6)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (2)
1-14: LGTM! Well-documented validation rules.The function signature and documentation clearly outline the validation rules and expected types.
30-56: LGTM! Efficient key validation.Good use of Set for efficient key lookup and clear error messaging.
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (2)
1-12: LGTM! Well-organized imports.Individual lodash imports are used which enables better tree-shaking.
27-54: Previous review comment is still applicable.The empty catch block and type safety issues mentioned in the previous review need to be addressed.
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (2)
25-33: UseObject.hasOwn()instead ofhasOwnProperty.
This avoids directly accessingObject.prototype, which can introduce potential security pitfalls. This recommendation was also made in a past review, so tagging as a duplicate.- obj.hasOwnProperty("label") && - obj.hasOwnProperty("value") && + Object.hasOwn(obj, "label") && + Object.hasOwn(obj, "value") &&🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
41-59: Fallback approach from JSON parse to comma-splitting looks solid.
This functionality gracefully handles user input in multiple formats (JSON arrays or comma-separated strings), making the validation more robust.
| interface LabelValue { | ||
| label: string; | ||
| value: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update interface to reflect support for numeric values in value field.
The line 31 check (_.isString(obj.value) || _.isFinite(obj.value)) indicates that value may be a string or number, but the LabelValue interface restricts value to a string. This mismatch may cause confusion or type errors.
Here’s a suggested diff to align the interface with the runtime checks:
-interface LabelValue {
- label: string;
- value: string;
-}
+interface LabelValue {
+ label: string;
+ value: string | number;
+}📝 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.
| interface LabelValue { | |
| label: string; | |
| value: string; | |
| } | |
| interface LabelValue { | |
| label: string; | |
| value: string | number; | |
| } |
| name: "MultiSelect", | ||
| tags: [WIDGET_TAGS.SELECT], | ||
| needsMeta: true, | ||
| searchTags: ["choice", "option", "choose", "pick", "select", "dropdown"], |
There was a problem hiding this comment.
@ichik Could you please verify this?
jsartisan
force-pushed
the
chore/wds-multi-select-widget
branch
from
7c9d78f to
4b653e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (3)
10-14: Consider removing unused lodash parameter.The
_parameter is marked as LoDashStatic but appears unused in the function signature. Consider removing it if not needed.-export function valueKeyValidation( - value: unknown, - props: WDSMultiSelectWidgetProps, - _: LoDashStatic, -) { +export function valueKeyValidation( + value: unknown, + props: WDSMultiSelectWidgetProps, +) {
30-56: Add defensive check for sourceData[0] access.The code assumes sourceData[0] exists when mapping values. Consider adding a guard clause.
- options = sourceData.map((d: Record<string, unknown>) => d[value]); + if (sourceData.length === 0) { + return { + parsed: value, + isValid: false, + messages: [{ name: "ValidationError", message: "Source data is empty" }], + }; + } + options = sourceData.map((d: Record<string, unknown>) => d[value]);
109-124: Consider using Set for uniqueness check.The current implementation uses indexOf which has O(n²) complexity. Using Set would be more efficient.
- const isValid = options.every( - (d: unknown, i: number, arr: unknown[]) => arr.indexOf(d) === i, - ); + const isValid = new Set(options).size === options.length;app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (2)
13-25: Enhance validation logic.The current validation is too simplistic. Consider validating:
- Selected values against options
- Required field validation
- Maximum selection limit
- Custom validation rules
export function validateInput(props: WDSMultiSelectWidgetProps): Validation { + const { isValid, isRequired, selectedValues, options, maxSelections } = props; + if (!props.isValid) { return { validationStatus: "invalid", - errorMessage: "Please select an option", + errorMessage: "Invalid selection", }; } + + if (isRequired && (!selectedValues || selectedValues.length === 0)) { + return { + validationStatus: "invalid", + errorMessage: "This field is required", + }; + } + + if (maxSelections && selectedValues && selectedValues.length > maxSelections) { + return { + validationStatus: "invalid", + errorMessage: `Maximum ${maxSelections} selections allowed`, + }; + } return { validationStatus: "valid", errorMessage: "", }; }
56-68: Add type safety and documentation.The function needs better type safety and documentation explaining its purpose.
+/** + * Creates an autocomplete data object mapping label keys to empty strings. + * @param props Widget properties containing source data + * @returns Object with item property mapping labels to empty strings + */ export function getLabelValueAdditionalAutocompleteData(props: WidgetProps) { const keys = getLabelValueKeyOptions(props); return { item: keys .map((d) => d.label) - .reduce((prev: Record<string, string>, curr: unknown) => { - prev[curr as string] = ""; + .reduce((prev: Record<string, string>, curr: string) => { + prev[curr] = ""; return prev; }, {}), }; }app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1)
209-215: Conditional Padding Reset for Prefix/Suffix Elements
Introducing rules with:has([data-input-prefix])and:has([data-input-suffix])to reset the inline paddings is an effective solution to handle spacing when there are additional elements (like icons or buttons) inside the input group. However, since the:has()pseudo-class is relatively new, please confirm that all target browsers fully support this feature or that a suitable fallback is provided if necessary.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (1)
41-104: Add bounds checking for production safety.The validation logic should include checks for:
- Maximum array length to prevent memory issues
- Maximum string length for labels
- String content validation (e.g., no special characters if needed)
export function defaultOptionValueValidation( value: unknown, props: WDSMultiSelectWidgetProps, _: LoDashStatic, ): ValidationResponse { let isValid = false; let parsed: LabelValue[] | (string | number)[] = []; let message = { name: "", message: "" }; + const MAX_ARRAY_LENGTH = 100; // adjust as needed + const MAX_LABEL_LENGTH = 256; // adjust as needed + + const isValidLength = (arr: unknown[]) => arr.length <= MAX_ARRAY_LENGTH; + const isValidLabel = (label: string) => label.length <= MAX_LABEL_LENGTH;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css(2 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/constants.ts(1 hunks)app/client/src/widgets/WidgetUtils.ts(3 hunks)app/client/src/widgets/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx
- app/client/src/modules/ui-builder/ui/wds/constants.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts
- app/client/src/widgets/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts
- app/client/src/widgets/WidgetUtils.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts
[error] 28-28: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: storybook-tests
🔇 Additional comments (10)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (3)
15-26: LGTM! Thorough null/empty validation.The empty string and null/undefined checks are well implemented with appropriate error messaging.
63-63: Replace hasOwnProperty with Object.hasOwn.Using Object.prototype.hasOwnProperty directly is unsafe. Use Object.hasOwn() instead for better security.
-props.sourceData[0].hasOwnProperty(String(value[0])) +Object.hasOwn(props.sourceData[0], String(value[0]))🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
95-107: LGTM! Clear error handling.The default error case is well handled with appropriate type validation messaging.
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (3)
1-12: LGTM! Good import practices.Using specific lodash imports enables better tree-shaking. Types and constants are properly imported.
27-54: Improve error handling and type safety.The empty catch block could hide important parsing errors, and type safety could be enhanced.
if (isString(sourceData)) { try { parsedValue = JSON.parse(sourceData); - } catch (e) {} + } catch (e) { + console.warn('Failed to parse sourceData:', e); + parsedValue = undefined; + } } if (isArray(parsedValue)) { return uniq( - parsedValue.reduce((keys, obj) => { + parsedValue.reduce<string[]>((keys, obj) => { if (isPlainObject(obj)) { Object.keys(obj).forEach((d) => keys.push(d)); } return keys; }, []), - ).map((d: unknown) => ({ + ).map((d: string) => ({ label: d, value: d, })); }
70-78: Add safety checks for expression generation.The expression generation could fail if widget properties are undefined.
export const getDefaultValueExpressionSuffix = (widget: WidgetProps) => { + if (!widget?.widgetName) { + console.warn('Widget name is required for expression generation'); + return ''; + } return `))(${widget.widgetName}.options, ${widget.widgetName}.serverSideFiltering) }}`; }; export const getOptionLabelValueExpressionPrefix = (widget: WidgetProps) => { + if (!widget?.widgetName) { + console.warn('Widget name is required for expression generation'); + return ''; + } return `{{${widget.widgetName}.sourceData.map((item) => (`; };app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (2)
191-196: Dynamic Sizing Update for Non-Textarea Inputs
The updated use ofcalc(var(--body-line-height) + var(--body-margin-start) + var(--body-margin-end))forblock-sizeand the increasedpadding-blockvalue effectively introduces a more responsive and context-aware sizing for inputs. Please verify that these adjustments integrate well with the rest of the design system, particularly in conjunction with the current box-sizing setting defined in.input.
198-203: Consistent Sizing for Small Inputs
The revised rule for.inputGroup .input[data-size="small"]now applies the same dynamic block-size calculation as the default input, with a tailoredpaddingvalue (var(--inner-spacing-2)). This consistency is beneficial for maintaining proportional spacing in various input sizes—just ensure these changes produce the desired visual effect in different use cases.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (2)
5-8: Update interface to reflect support for numeric values invaluefield.The line 31 check (
_.isString(obj.value) || _.isFinite(obj.value)) indicates thatvaluemay be a string or number, but theLabelValueinterface restrictsvalueto a string. This mismatch may cause confusion or type errors.
25-33: Improve type safety and security in hasLabelValue.The function needs updates for better type safety and to use modern JavaScript features.
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
| const hasUniqueValues = (arr: Array<string>) => { | ||
| const uniqueValues = new Set(arr); | ||
|
|
||
| return uniqueValues.size === arr.length; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in hasUniqueValues function.
The function accepts only string arrays but is used with mixed types (string | number) in the validation logic.
-const hasUniqueValues = (arr: Array<string>) => {
+const hasUniqueValues = (arr: Array<string | number>) => {
const uniqueValues = new Set(arr);
return uniqueValues.size === arr.length;
};📝 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.
| const hasUniqueValues = (arr: Array<string>) => { | |
| const uniqueValues = new Set(arr); | |
| return uniqueValues.size === arr.length; | |
| }; | |
| const hasUniqueValues = (arr: Array<string | number>) => { | |
| const uniqueValues = new Set(arr); | |
| return uniqueValues.size === arr.length; | |
| }; |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13394483716. |
|
Deploy-Preview-URL: https://ce-39307.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1)
27-28: Base Input Padding Consistency Check
The base.inputrule setspadding-block: var(--inner-spacing-1), but a later override for non-textarea inputs usesvar(--inner-spacing-3). Confirm that this contrast in padding for textarea versus non-textarea elements is deliberate as per the design specs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- 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/Input/src/styles.module.css (2)
9-19: Updated InputGroup Styling with Responsive Enhancements
The new declaration for.inputGroupadds amax-inline-size: 100%and updates the border, border-radius, and background properties. This approach helps ensure the component adapts to different container sizes. Please verify that these style changes are consistent with the overall design system guidelines.
33-38: Dynamic Sizing for Non-textarea Inputs Introduced
The new rule for.inputGroup .input:not(textarea)uses acalc()expression to deriveblock-sizebased on--body-line-height,--body-margin-start, and--body-margin-end, along with an increasedpadding-block. This should provide a more flexible and consistent sizing model for input elements that aren’t textareas. Please double-check that this override interacts correctly with the base.inputstyles and doesn’t introduce unintended side effects.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13407881629. |
| @@ -1011,7 +1011,9 @@ export const checkForOnClick = (e: React.MouseEvent<HTMLElement>) => { | |||
| * Main rule to remember is don't deconstruct the props like `const { value } = props;` in the derived property function. | |||
| * Directly access props like `props.value` | |||
| */ | |||
| export function parseDerivedProperties(propertyFns: Record<string, unknown>) { | |||
| export function parseDerivedProperties<T extends Record<string, unknown>>( | |||
| propertyFns: T, | |||
There was a problem hiding this comment.
@jsartisan Can E2E tests fails because of these changes? Why are these changes necessary?
There was a problem hiding this comment.
I just added it to enable auto-completion like this:
Some release migration caused the E2E fails. Anagh is working on it. I saw Shri also facing the same E2E failure issues.
|
Deploy-Preview-URL: https://ce-39307.dp.appsmith.com |
jsartisan
force-pushed
the
chore/wds-multi-select-widget
branch
from
9501571 to
74c0f41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx (1)
141-142: Use a safer key strategy to avoid potential collisions.Joining values with commas might cause collisions if an option contains a comma. A more robust approach could use a stable hash or unique identifiers.
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
76-79: Consider using type predicates for better type safety.The type checking could be improved using TypeScript type predicates for better type safety and readability.
-const errorIndex = value.findIndex( - (d) => - !(_.isString(d) || (_.isNumber(d) && !_.isNaN(d)) || _.isBoolean(d)), -); +const isValidType = (d: unknown): d is string | number | boolean => { + return _.isString(d) || (_.isNumber(d) && !_.isNaN(d)) || _.isBoolean(d); +}; +const errorIndex = value.findIndex((d) => !isValidType(d));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/constants.ts(1 hunks)app/client/src/widgets/WidgetUtils.ts(3 hunks)app/client/src/widgets/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts
- app/client/src/widgets/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts
- app/client/src/modules/ui-builder/ui/wds/constants.ts
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts
- app/client/src/widgets/WidgetUtils.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts
- app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts
[error] 28-28: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-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: perform-test / client-build / client-build
- 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 (6)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (3)
5-8: Ensure the interface accommodates numeric values.The interface restricts
valueto a string, but line 31 checks if it’s numeric. This mismatch can lead to type inconsistencies.
25-33: UseObject.hasOwninstead ofhasOwnProperty.As previously noted, accessing
hasOwnPropertydirectly can be unsafe. Replaceobj.hasOwnProperty("label")andobj.hasOwnProperty("value")withObject.hasOwn(obj, "label")andObject.hasOwn(obj, "value").🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
35-39: UpdatehasUniqueValuesto handle numeric elements as well.Since you check for numeric values in the array above, consider changing its signature to
(arr: Array<string | number>).app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (1)
33-35: Avoid swallowing JSON parsing errors.Consider logging or handling this error to aid debugging (e.g.,
console.warn). Leaving the catch block empty hides potential issues.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
63-63: Replace hasOwnProperty with Object.hasOwn.Using Object.prototype.hasOwnProperty directly is unsafe. Use Object.hasOwn() instead for better security.
-props.sourceData[0].hasOwnProperty(String(value[0])) +Object.hasOwn(props.sourceData[0], String(value[0]))🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx (1)
20-263: Well-structured property pane configuration!The configuration is well-organized into logical sections with comprehensive validation rules and proper dependency tracking.
|
|
||
| if ( | ||
| areAllValuesEqual && | ||
| props.sourceData[0].hasOwnProperty(String(value[0])) |
There was a problem hiding this comment.
Add null check for sourceData[0].
Add a null check before accessing sourceData[0] to prevent potential runtime errors.
-props.sourceData[0].hasOwnProperty(String(value[0]))
+props.sourceData?.[0] && Object.hasOwn(props.sourceData[0], String(value[0]))📝 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.
| props.sourceData[0].hasOwnProperty(String(value[0])) | |
| props.sourceData?.[0] && Object.hasOwn(props.sourceData[0], String(value[0])) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
jsartisan
force-pushed
the
chore/wds-multi-select-widget
branch
from
74c0f41 to
e958178
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
10-125:⚠️ Potential issue
valueKeyValidationcovers string and array logic comprehensively.
- At line 63, avoid
hasOwnProperty. UseObject.hasOwn()for safer checks.- Also, add null checks for
props.sourceDatabefore accessingprops.sourceData[0].- props.sourceData[0].hasOwnProperty(String(value[0])) + props.sourceData?.[0] && Object.hasOwn(props.sourceData[0], String(value[0]))- if ( - areAllValuesEqual && - props.sourceData[0].hasOwnProperty(String(value[0])) - ) { + if ( + areAllValuesEqual && + props.sourceData?.[0] && + Object.hasOwn(props.sourceData[0], String(value[0])) + ) { const parsedValue = /* ... */ }🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (3)
5-8:⚠️ Potential issueUpdate interface to reflect support for numeric values.
The validation logic supports both string and number types for the
valuefield, but the interface only allows string.interface LabelValue { label: string; - value: string; + value: string | number; }
35-39:⚠️ Potential issueFix type mismatch in hasUniqueValues function.
The function accepts only string arrays but is used with mixed types (string | number) in the validation logic.
-const hasUniqueValues = (arr: Array<string>) => { +const hasUniqueValues = (arr: Array<string | number>) => { const uniqueValues = new Set(arr); return uniqueValues.size === arr.length; };
25-33:⚠️ Potential issueImprove type safety and security in hasLabelValue.
The function needs improvements in type checking and property access safety.
-const hasLabelValue = (obj: LabelValue): obj is LabelValue => { +const hasLabelValue = (obj: unknown): obj is LabelValue => { return ( _.isPlainObject(obj) && - obj.hasOwnProperty("label") && - obj.hasOwnProperty("value") && + Object.hasOwn(obj, "label") && + Object.hasOwn(obj, "value") && _.isString(obj.label) && (_.isString(obj.value) || _.isFinite(obj.value)) ); };🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🧹 Nitpick comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (1)
27-54:getLabelValueKeyOptionsparses source data effectively.
However, the emptycatchat lines 33-35 may hide parsing errors. Logging or rethrowing could improve traceability.try { parsedValue = JSON.parse(sourceData); - } catch (e) {} + } catch (e) { + console.warn("Failed to parse sourceData:", e); + parsedValue = undefined; + }app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (2)
41-59: Consider adding error logging for JSON parse failures.While the error handling is robust, logging parse errors could help with debugging issues in production.
} catch (e) { + console.debug("Failed to parse JSON value:", e); // when value is "green, red", JSON.parse throws error const splitByComma = (value as string).split(",") || [];
106-111: Consider allowing multiple validation messages.The current implementation always returns a single message, but there might be cases where multiple validation issues need to be reported.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/constants.ts(1 hunks)app/client/src/widgets/WidgetUtils.ts(3 hunks)app/client/src/widgets/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts
- app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx
- app/client/src/modules/ui-builder/ui/wds/constants.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/constants.ts
- app/client/src/widgets/WidgetUtils.ts
- app/client/src/widgets/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts
- app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts
[error] 28-28: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 29-29: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / client-build / client-build
- 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: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (27)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx (19)
1-26: Imports and type declarations look structured.
Everything necessary is imported, and there are no obvious issues or redundancies.
27-30: Class declaration is straightforward.
The class extendsBaseWidgetwithout issues.
31-35: Use ofmetaConfigfor getConfig is clean.
No concerns with returning this static configuration.
37-39: Default properties logic is clear.
ReturnsdefaultsConfigas expected.
41-43: Method retrieval is correct.
methodsConfigis returned properly for widget methods.
45-47: Anvil config retrieval is straightforward.
ReturninganvilConfigensures proper referencing.
49-54: Dependency map is concise.
MappingoptionLabelandoptionValuetosourceDatais logical.
56-58: Autocomplete definitions usage is consistent.
No performance or correctness issues detected.
60-62: Property pane content configs are returned properly.
Implementation aligns with the widget config structure.
64-66: Property pane style config is empty but acceptable.
No immediate concerns unless styling configs are needed in the future.
68-78: Derived properties map is well-structured.
Use ofparseDerivedPropertiesis appropriate.
80-84: Default properties map is minimal and functional.
TiesselectedOptionValuestodefaultOptionValuesas intended.
86-91: Meta properties map is suitable for widget metadata.
MarkingselectedOptionValuesandisDirtyensures correct tracking.
93-95: Empty stylesheet config is acceptable.
No issues with returning an empty object.
97-106: ResetsisDirtyin componentDidUpdate.
This helps maintain correct widget state when default values change.
107-109: Setter config is properly referenced.
Allows external set operations to be configured.
111-127:onSelectionChangeupdates metadata correctly.
The batch meta updates approach ensures changes are properly triggered.
129-158: Widget view is well-structured.
Forcing re-render by dynamically generating akeyis a sensible workaround for the underlying render issue.
161-161: Named export is correct.
ExportingWDSMultiSelectWidgetpromotes modular usage.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (4)
1-7: Imports are appropriate.
Utility functions from lodash are used effectively.
8-25:validateInputfunction is concise.
Properly checksisValidprop and returns the correct validation object.
56-68:getLabelValueAdditionalAutocompleteDatausage.
LeveragesgetLabelValueKeyOptionsto build mappings. Looks straightforward.
70-79: Expressions for dynamic bindings.
Well-defined constants for constructing default and option expressions.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
1-9: Imports and doc comments are clear.
Guides the user about valid data types.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx (3)
20-159: Well-structured data configuration section!The data section is comprehensive with appropriate validations, helpful tooltips, and clear examples.
175-248: Clean implementation of validation and general settings!Good separation of concerns and proper conditional rendering based on widget state.
249-263: Clear and concise event handling configuration!The onSelectionChange event is well-documented and properly typed.
jsartisan
added
ok-to-test
CleanShot.2025-02-18.at.16.57.25.mp4
/ok-to-test tags="@tag.CurrencyInput"
Summary by CodeRabbit
New Features
Style
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13432464907
Commit: e958178
Cypress dashboard.
Tags: @tag.CurrencyInput
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/Apps/CurrencyInputIssue_Spec.js
- cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInputDynamicCurrencyCode_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInputTest.ts
- cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInput_ShowStepArrows_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInput_spec.js
List of identified flaky tests.Thu, 20 Feb 2025 10:26:43 UTC