Conversation
WalkthroughThe changes update input styling and enhance the MultiSelect component by adding a specific CSS class. Several new configuration objects and validation functions have been introduced to support the WDSMultiSelectWidget. Additionally, helper functions, constants (including sample data), and a new widget class extending BaseWidget have been implemented. Integration into the widget collection has also been completed. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant W as WDSMultiSelectWidget
participant H as Helpers
participant B as BaseWidget
U->>W: Selects an option
W->>W: onSelectionChange(updatedValues)
W->>H: Validate and process selection
W->>B: Update state (mark as dirty)
B-->>W: Rerender widget view
W-->>U: Updated widget displayed
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (1)
✨ 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/derived.js (1)
1-67: 🛠️ Refactor suggestionConvert file to TypeScript for better type safety.
The file contains complex object manipulations that would benefit from TypeScript's type system.
- Rename file to
derived.ts- Add type definitions:
interface Option { label: string; value: unknown; } interface Props { sourceData: unknown[]; optionLabel: string | string[]; optionValue: string | string[]; options?: Option[]; selectedOptions?: Option[]; selectedOptionValues: unknown[]; isDirty?: boolean; isRequired?: boolean; }
🧹 Nitpick comments (13)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx (1)
129-158: Use a more efficient key for large option sets.Generating a key from all option values can be costly for large arrays. Consider a hash-based or stable unique key to reduce potential performance overhead.
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/anvilConfig.ts (1)
3-11: Optional configuration confirmation.Since the
anvilConfigis not strictly required for widgets that don't rely on main container layouts, confirm if these settings are essential. If unneeded, you may omit or reduce them.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts (1)
4-4: Consider using a more specific type for options.The
optionsproperty type could be more specific to better represent the expected data structure.- options: Record<string, unknown>[] | string; + options: Array<{ label: string; value: string }> | string;app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/defaultsConfig.ts (1)
21-21: Avoid type casting with unknown.Consider defining the config object with proper types instead of using type casting.
-} as unknown as WidgetDefaultProps; +}: WidgetDefaultProps = { + // ... existing properties +};app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/autocompleteConfig.ts (1)
18-21: Use explicit type definitions instead of string literals.Replace string literals with proper TypeScript types for better type safety.
- isDisabled: "bool", - isValid: "bool", - isDirty: "bool", + isDisabled: { "!type": "boolean" }, + isValid: { "!type": "boolean" }, + isDirty: { "!type": "boolean" },app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts (1)
38-55: Consider simplifying array validation logic.The array validation could be more concise using
every()instead offindIndex().- const errorIndex = value.findIndex((item) => !_.isString(item)); - const isValid = errorIndex === -1; + const isValid = value.every(_.isString); return { parsed: isValid ? value : [], isValid, messages: [ { name: isValid ? "" : "ValidationError", - message: isValid - ? "" - : `Invalid entry at index: ${errorIndex}. Value must be a string`, + message: isValid ? "" : "All array entries must be strings", }, ], };app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/methodsConfig.ts (1)
43-54: Add type safety for modify object properties.The
modifyobject properties should be explicitly typed to prevent potential runtime errors.+ interface ModifyConfig { + sourceData: unknown; + optionLabel?: string; + optionValue?: string; + defaultOptionValues: string; + serverSideFiltering: boolean; + onFilterUpdate?: () => void; + } + if (queryConfig.select) { - modify = { + modify: ModifyConfig = { sourceData: queryConfig.select.data, optionLabel: formConfig.aliases.find((d) => d.name === "label")?.alias, optionValue: formConfig.aliases.find((d) => d.name === "value")?.alias,app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/helpers.ts (1)
70-78: Consider using template literals for expression templates.The current string concatenation could be more maintainable using template literals.
-export const defaultValueExpressionPrefix = `{{ ((options, serverSideFiltering) => ( `; +export const createDefaultValueExpression = (widget: WidgetProps) => ` + {{ ((options, serverSideFiltering) => ( + /* Your expression here */ + ))(${widget.widgetName}.options, ${widget.widgetName}.serverSideFiltering) }} +`;app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (2)
76-79: Simplify type checking conditionThe complex condition for type checking could be simplified using _.isNil.
- (d) => - !(_.isString(d) || (_.isNumber(d) && !_.isNaN(d)) || _.isBoolean(d)), + (d) => _.isNil(d) || !(_.isString(d) || _.isFinite(d) || _.isBoolean(d)),
109-111: Simplify uniqueness check using SetThe array uniqueness check could be simplified 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)
35-39: Simplify hasUniqueValues functionThe hasUniqueValues function could be simplified to a one-liner.
- const hasUniqueValues = (arr: Array<string>) => { - const uniqueValues = new Set(arr); - - return uniqueValues.size === arr.length; - }; + const hasUniqueValues = (arr: Array<string>) => new Set(arr).size === arr.length;app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/contentConfig.tsx (2)
31-41: Consider adding validation for label and value aliases.The aliases configuration could benefit from type validation to ensure data consistency.
aliases: [ { name: "label", isSearcheable: true, isRequired: true, + validation: { type: ValidationTypes.TEXT } }, { name: "value", isRequired: true, + validation: { type: ValidationTypes.ANY } }, ],
253-260: Add documentation for event payload structure.The helpText should include information about the event payload structure to help developers understand what data is available in the callback.
- helpText: "when a user changes the selected option", + helpText: "Triggered when selection changes. Provides an array of selected values in the event payload.",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
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/widgets/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/propertyPaneConfig/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/widget/constants.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/index.ts
🧰 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] 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 (8)
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (10)
app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/index.tsx (3)
27-30: Looks structurally sound.Your class extension from
BaseWidgetfor theWDSMultiSelectWidgetis correctly set up. Nice work!
98-105: Ensure all default-to-props transition cases are handled.When the
defaultOptionValueschange, resettingisDirtyto false is correct. Consider whether there are edge cases where other prop changes might also need to resetisDirty.
111-127: Validate potential "all" selection scenario.Depending on the underlying library,
Selectioncan sometimes be"all". Confirm that spreading[...updatedValues]always yields the correct array when selection is"all".app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/metaConfig.ts (1)
3-8: Good metadata definition.The widget name, tags, and
searchTagsseem appropriate. Additional related keywords can be added as needed.app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/widget/types.ts (1)
6-7: Consider using a more specific type for onSelectionChange.The
onSelectionChangeproperty being a string suggests it might be a function name or callback identifier.- onSelectionChange: string; + onSelectionChange: (selectedValues: Set<string>) => void;app/client/src/modules/ui-builder/ui/wds/WDSMultiSelectWidget/config/settersConfig.ts (1)
19-23: Ensure type consistency for setSelectedOption.The
setSelectedOptionsetter's type should matchdefaultOptionValuesproperty type from the widget props.setSelectedOption: { path: "defaultOptionValues", - type: "array", + type: "string", accessor: "selectedOptionValues", },app/client/packages/design-system/widgets/src/components/MultiSelect/src/MultiSelect.tsx (1)
157-161: LGTM!The addition of className to ListBoxItem follows the component's styling pattern.
app/client/src/widgets/index.ts (1)
93-93: LGTM!The addition of WDSMultiSelectWidget follows the established pattern for integrating new WDS widgets.
Also applies to: 193-193
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (2)
33-38: LGTM! Well-structured CSS using modern features.Good use of CSS logical properties and calc() for flexible sizing. The selector specificity is appropriate for the use case.
13-15: Verify color contrast ratios for accessibility.Please ensure the background and border colors meet WCAG 2.1 contrast requirements.
#!/usr/bin/env python3 import colorsys import math def get_relative_luminance(r, g, b): def get_rgb(c): c = c/255 return c/12.92 if c <= 0.03928 else math.pow((c + 0.055)/1.055, 2.4) return 0.2126 * get_rgb(r) + 0.7152 * get_rgb(g) + 0.0722 * get_rgb(b) def check_contrast(bg_color, text_color): # Convert hex to RGB and calculate relative luminance bg_lum = get_relative_luminance(*[int(bg_color[i:i+2], 16) for i in (0, 2, 4)]) text_lum = get_relative_luminance(*[int(text_color[i:i+2], 16) for i in (0, 2, 4)]) # Calculate contrast ratio ratio = (max(bg_lum, text_lum) + 0.05) / (min(bg_lum, text_lum) + 0.05) return ratio >= 4.5 # WCAG 2.1 Level AA requirement # Get colors from CSS variables and verify contrast print("Please verify the contrast ratio between background-color and text color meets WCAG 2.1 Level AA requirements (4.5:1)")
| if (isString(sourceData)) { | ||
| try { | ||
| parsedValue = JSON.parse(sourceData); | ||
| } catch (e) {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in JSON parsing.
The current try-catch block silently fails. Consider logging the error or handling it more explicitly.
if (isString(sourceData)) {
try {
parsedValue = JSON.parse(sourceData);
- } catch (e) {}
+ } catch (e) {
+ console.warn('Failed to parse sourceData:', e);
+ parsedValue = undefined;
+ }
}📝 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.
| if (isString(sourceData)) { | |
| try { | |
| parsedValue = JSON.parse(sourceData); | |
| } catch (e) {} | |
| } | |
| if (isString(sourceData)) { | |
| try { | |
| parsedValue = JSON.parse(sourceData); | |
| } catch (e) { | |
| console.warn('Failed to parse sourceData:', e); | |
| parsedValue = undefined; | |
| } | |
| } |
|
|
||
| if ( | ||
| areAllValuesEqual && | ||
| props.sourceData[0].hasOwnProperty(String(value[0])) |
There was a problem hiding this comment.
Replace hasOwnProperty with Object.hasOwn
Using Object.prototype.hasOwnProperty directly on target object is unsafe as it can be overridden.
- props.sourceData[0].hasOwnProperty(String(value[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])) | |
| 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)
| obj.hasOwnProperty("label") && | ||
| obj.hasOwnProperty("value") && |
There was a problem hiding this comment.
Replace hasOwnProperty with Object.hasOwn
Using Object.prototype.hasOwnProperty directly on target objects is unsafe as it can be overridden.
- obj.hasOwnProperty("label") &&
- obj.hasOwnProperty("value") &&
+ Object.hasOwn(obj, "label") &&
+ Object.hasOwn(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.
| 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)
1 participant
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
New Features
Style
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13407058456
Commit: e4279e6
Cypress dashboard.
Tags: @tag.Anvil
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Anvil/AnvilAppNavigation_spec.ts
- cypress/e2e/Regression/ClientSide/Anvil/AnvilCopyPaste_spec.ts
- cypress/e2e/Regression/ClientSide/Anvil/AnvilDnD_spec.ts
- cypress/e2e/Regression/ClientSide/Anvil/AnvilModal_spec.ts
- cypress/e2e/Regression/ClientSide/Anvil/AnvilOnCanvasUI_spec.ts
- cypress/e2e/Regression/ClientSide/Anvil/AnvilSectionsAndZones_spec.ts
- cypress/e2e/Regression/ClientSide/Anvil/AnvilSpaceDistribution_spec.ts
- cypress/e2e/Regression/ClientSide/Anvil/AnvilSuggestedWidgets_spec.ts
- cypress/e2e/Regression/ClientSide/Anvil/AnvilWidgetClicking_spec.ts
List of identified flaky tests.Wed, 19 Feb 2025 07:33:54 UTC